Merge branch 'pl-cop-migration-no-multiple-fks-in-with-lock-retries' into 'master'

Avoid adding more than one foreign key within `with_lock_retries`

See merge request gitlab-org/gitlab!44138

db/migrate/20200123090839_remove_analytics_repository_table_fks_on_projects.rb

@@ -5,22 +5,26 @@ class RemoveAnalyticsRepositoryTableFksOnProjects < ActiveRecord::Migration[5.2]
 
   DOWNTIME = false
 
+  disable_ddl_transaction!
+
   def up
+    # Requires ExclusiveLock on all tables. analytics_* tables are empty
     with_lock_retries do
-      # Requires ExclusiveLock on all tables. analytics_* tables are empty
-      remove_foreign_key :analytics_repository_files, :projects
-      remove_foreign_key :analytics_repository_file_edits, :projects if table_exists?(:analytics_repository_file_edits) # this table might be already dropped on development environment
-      remove_foreign_key :analytics_repository_file_commits, :projects
+      remove_foreign_key_if_exists(:analytics_repository_files, :projects)
     end
-  end
 
-  def down
     with_lock_retries do
-      # rubocop:disable Migration/AddConcurrentForeignKey
-      add_foreign_key :analytics_repository_files, :projects, on_delete: :cascade
-      add_foreign_key :analytics_repository_file_edits, :projects, on_delete: :cascade
-      add_foreign_key :analytics_repository_file_commits, :projects, on_delete: :cascade
-      # rubocop:enable Migration/AddConcurrentForeignKey
+      remove_foreign_key_if_exists(:analytics_repository_file_edits, :projects) if table_exists?(:analytics_repository_file_edits) # this table might be already dropped on development environment
     end
+
+    with_lock_retries do
+      remove_foreign_key_if_exists(:analytics_repository_file_commits, :projects)
+    end
+  end
+
+  def down
+    add_concurrent_foreign_key(:analytics_repository_files, :projects, column: :project_id, on_delete: :cascade)
+    add_concurrent_foreign_key(:analytics_repository_file_edits, :projects, column: :project_id, on_delete: :cascade)
+    add_concurrent_foreign_key(:analytics_repository_file_commits, :projects, column: :project_id, on_delete: :cascade)
   end
 end

db/migrate/20200123091422_remove_analytics_repository_files_fk_on_other_analytics_tables.rb

@@ -5,20 +5,21 @@ class RemoveAnalyticsRepositoryFilesFkOnOtherAnalyticsTables < ActiveRecord::Mig
 
   DOWNTIME = false
 
+  disable_ddl_transaction!
+
   def up
+    # Requires ExclusiveLock on all tables. analytics_* tables are empty
     with_lock_retries do
-      # Requires ExclusiveLock on all tables. analytics_* tables are empty
-      remove_foreign_key :analytics_repository_file_edits, :analytics_repository_files if table_exists?(:analytics_repository_file_edits) # this table might be already dropped on development environment
-      remove_foreign_key :analytics_repository_file_commits, :analytics_repository_files
+      remove_foreign_key_if_exists(:analytics_repository_file_edits, :analytics_repository_files) if table_exists?(:analytics_repository_file_edits) # this table might be already dropped on development environment
     end
-  end
 
-  def down
     with_lock_retries do
-      # rubocop:disable Migration/AddConcurrentForeignKey
-      add_foreign_key :analytics_repository_file_edits, :analytics_repository_files, on_delete: :cascade
-      add_foreign_key :analytics_repository_file_commits, :analytics_repository_files, on_delete: :cascade
-      # rubocop:enable Migration/AddConcurrentForeignKey
+      remove_foreign_key_if_exists(:analytics_repository_file_commits, :analytics_repository_files)
     end
   end
+
+  def down
+    add_concurrent_foreign_key(:analytics_repository_file_edits, :analytics_repository_files, column: :analytics_repository_file_id, on_delete: :cascade)
+    add_concurrent_foreign_key(:analytics_repository_file_commits, :analytics_repository_files, column: :analytics_repository_file_id, on_delete: :cascade)
+  end
 end

doc/development/migration_style_guide.md

@@ -331,7 +331,7 @@ end
 
 **Usage with `disable_ddl_transaction!`**
 
-Generally the `with_lock_retries` helper should work with `disabled_ddl_transaction!`. A custom RuboCop rule ensures that only allowed methods can be placed within the lock retries block.
+Generally the `with_lock_retries` helper should work with `disable_ddl_transaction!`. A custom RuboCop rule ensures that only allowed methods can be placed within the lock retries block.
 
 ```ruby
 disable_ddl_transaction!
@@ -348,7 +348,7 @@ end
 The RuboCop rule generally allows standard Rails migration methods, listed below. This example will cause a Rubocop offense:
 
 ```ruby
-disabled_ddl_transaction!
+disable_ddl_transaction!
 
 def up
   with_lock_retries do

rubocop/cop/migration/with_lock_retries_disallowed_method.rb

@@ -29,9 +29,14 @@ module RuboCop
         ].sort.freeze
 
         MSG = "The method is not allowed to be called within the `with_lock_retries` block, the only allowed methods are: #{ALLOWED_MIGRATION_METHODS.join(', ')}"
+        MSG_ONLY_ONE_FK_ALLOWED = "Avoid adding more than one foreign key within the `with_lock_retries`. See https://docs.gitlab.com/ee/development/migration_style_guide.html#examples"
 
         def_node_matcher :send_node?, <<~PATTERN
-        send
+          send
+        PATTERN
+
+        def_node_matcher :add_foreign_key?, <<~PATTERN
+          (send nil? :add_foreign_key ...)
         PATTERN
 
         def on_block(node)
@@ -53,6 +58,17 @@ module RuboCop
 
           name = node.children[1]
           add_offense(node, location: :expression) unless ALLOWED_MIGRATION_METHODS.include?(name)
+          add_offense(node, location: :selector, message: MSG_ONLY_ONE_FK_ALLOWED) if multiple_fks?(node)
+        end
+
+        def multiple_fks?(node)
+          return unless add_foreign_key?(node)
+
+          count = node.parent.each_descendant(:send).count do |node|
+            add_foreign_key?(node)
+          end
+
+          count > 1
         end
       end
     end

spec/rubocop/cop/migration/with_lock_retries_disallowed_method_spec.rb

@@ -53,6 +53,22 @@ RSpec.describe RuboCop::Cop::Migration::WithLockRetriesDisallowedMethod, type: :
 
       expect(cop.offenses.size).to eq(0)
     end
+
+    describe 'for `add_foreign_key`' do
+      it 'registers an offense when more than two FKs are added' do
+        message = described_class::MSG_ONLY_ONE_FK_ALLOWED
+
+        expect_offense <<~RUBY
+          with_lock_retries do
+            add_foreign_key :imports, :projects, column: :project_id, on_delete: :cascade
+            ^^^^^^^^^^^^^^^ #{message}
+            add_column :projects, :name, :text
+            add_foreign_key :imports, :users, column: :user_id, on_delete: :cascade
+            ^^^^^^^^^^^^^^^ #{message}
+          end
+        RUBY
+      end
+    end
   end
 
   context 'outside of migration' do