Merge branch 'cop-for-with-lock-retries-ddl-transaction' into 'master'

Cop for with_lock_retries without DDL transaction

See merge request gitlab-org/gitlab!28132

db/migrate/20200215222507_drop_forked_project_links_fk.rb

@@ -8,17 +8,21 @@ class DropForkedProjectLinksFk < ActiveRecord::Migration[6.0]
   disable_ddl_transaction!
 
   def up
+    # rubocop: disable Migration/WithLockRetriesWithoutDdlTransaction
     with_lock_retries do
       remove_foreign_key_if_exists :forked_project_links, column: :forked_to_project_id
     end
+    # rubocop: enable Migration/WithLockRetriesWithoutDdlTransaction
   end
 
   def down
     unless foreign_key_exists?(:forked_project_links, :projects, column: :forked_to_project_id)
+      # rubocop: disable Migration/WithLockRetriesWithoutDdlTransaction
       with_lock_retries do
         # rubocop: disable Migration/AddConcurrentForeignKey
         add_foreign_key :forked_project_links, :projects, column: :forked_to_project_id, on_delete: :cascade, validate: false
       end
+      # rubocop: enable Migration/WithLockRetriesWithoutDdlTransaction
     end
 
     fk_name = concurrent_foreign_key_name(:forked_project_links, :forked_to_project_id, prefix: 'fk_rails_')

db/migrate/20200304085423_add_user_type.rb

@@ -5,8 +5,6 @@ class AddUserType < ActiveRecord::Migration[6.0]
 
   DOWNTIME = false
 
-  disable_ddl_transaction!
-
   def up
     with_lock_retries do
       add_column :users, :user_type, :integer, limit: 2

rubocop/cop/migration/with_lock_retries_without_ddl_transaction.rb

+# frozen_string_literal: true
+
+require_relative '../../migration_helpers'
+
+module RuboCop
+  module Cop
+    module Migration
+      # Cop that prevents usage of `with_lock_retries` with `disable_ddl_transaction!`
+      class WithLockRetriesWithoutDdlTransaction < RuboCop::Cop::Cop
+        include MigrationHelpers
+
+        MSG = '`with_lock_retries` cannot be used with disabled DDL transactions (`disable_ddl_transaction!`). ' \
+          'Please remove the `disable_ddl_transaction!` call from your migration.'.freeze
+
+        def_node_matcher :disable_ddl_transaction?, <<~PATTERN
+        (send _ :disable_ddl_transaction!)
+        PATTERN
+
+        def_node_matcher :with_lock_retries?, <<~PATTERN
+          (send _ :with_lock_retries)
+        PATTERN
+
+        def on_send(node)
+          return unless in_migration?(node)
+          return unless with_lock_retries?(node)
+
+          node.each_ancestor(:begin) do |begin_node|
+            disable_ddl_transaction_node = begin_node.children.find { |n| disable_ddl_transaction?(n) }
+
+            add_offense(node, location: :expression) if disable_ddl_transaction_node
+          end
+        end
+      end
+    end
+  end
+end

spec/rubocop/cop/migration/with_lock_retries_without_ddl_transaction_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+require 'rubocop'
+require 'rubocop/rspec/support'
+
+require_relative '../../../../rubocop/cop/migration/with_lock_retries_without_ddl_transaction'
+
+describe RuboCop::Cop::Migration::WithLockRetriesWithoutDdlTransaction do
+  include CopHelper
+
+  let(:valid_source) { 'class MigrationClass < ActiveRecord::Migration[6.0]; def up; with_lock_retries {}; end; end' }
+  let(:invalid_source) { 'class MigrationClass < ActiveRecord::Migration[6.0]; disable_ddl_transaction!; def up; with_lock_retries {}; end; end' }
+
+  subject(:cop) { described_class.new }
+
+  context 'in migration' do
+    before do
+      allow(cop).to receive(:in_migration?).and_return(true)
+    end
+
+    it 'registers an offense when `with_lock_retries` is used with `disable_ddl_transaction!` method' do
+      inspect_source(invalid_source)
+
+      aggregate_failures do
+        expect(cop.offenses.size).to eq(1)
+        expect(cop.offenses.map(&:line)).to eq([1])
+      end
+    end
+
+    it 'registers no offense when `with_lock_retries` is used inside an `up` method' do
+      inspect_source(valid_source)
+
+      expect(cop.offenses.size).to eq(0)
+    end
+  end
+
+  context 'outside of migration' do
+    it 'registers no offense' do
+      inspect_source(invalid_source)
+
+      expect(cop.offenses.size).to eq(0)
+    end
+  end
+end