Merge branch 'handle-not-valid-fk-addition-in-cop' into 'master'

Adjust rubocop rule for not valid FKs

See merge request gitlab-org/gitlab!30288

db/migrate/20200215222507_drop_forked_project_links_fk.rb

@@ -19,7 +19,6 @@ class DropForkedProjectLinksFk < ActiveRecord::Migration[6.0]
     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

db/migrate/20200407171133_add_protected_tag_create_access_levels_user_id_foreign_key.rb

 # frozen_string_literal: true
 
-# rubocop: disable Migration/AddConcurrentForeignKey
 # rubocop: disable Migration/WithLockRetriesWithoutDdlTransaction
 class AddProtectedTagCreateAccessLevelsUserIdForeignKey < ActiveRecord::Migration[6.0]
   include Gitlab::Database::MigrationHelpers

db/migrate/20200408154331_add_protected_branch_merge_access_levels_user_id_foreign_key.rb

 # frozen_string_literal: true
 
-# rubocop: disable Migration/AddConcurrentForeignKey
 # rubocop: disable Migration/WithLockRetriesWithoutDdlTransaction
 class AddProtectedBranchMergeAccessLevelsUserIdForeignKey < ActiveRecord::Migration[6.0]
   include Gitlab::Database::MigrationHelpers

db/migrate/20200408154411_add_path_locks_user_id_foreign_key.rb

 # frozen_string_literal: true
 
-# rubocop: disable Migration/AddConcurrentForeignKey
 # rubocop: disable Migration/WithLockRetriesWithoutDdlTransaction
 class AddPathLocksUserIdForeignKey < ActiveRecord::Migration[6.0]
   include Gitlab::Database::MigrationHelpers

db/migrate/20200408154455_add_protected_branch_push_access_levels_user_id_foreign_key.rb

 # frozen_string_literal: true
 
-# rubocop: disable Migration/AddConcurrentForeignKey
 # rubocop: disable Migration/WithLockRetriesWithoutDdlTransaction
 class AddProtectedBranchPushAccessLevelsUserIdForeignKey < ActiveRecord::Migration[6.0]
   include Gitlab::Database::MigrationHelpers

db/migrate/20200408154604_add_u2f_registrations_user_id_foreign_key.rb

 # frozen_string_literal: true
 
-# rubocop: disable Migration/AddConcurrentForeignKey
 # rubocop: disable Migration/WithLockRetriesWithoutDdlTransaction
 class AddU2fRegistrationsUserIdForeignKey < ActiveRecord::Migration[6.0]
   include Gitlab::Database::MigrationHelpers

doc/development/database/add_foreign_key_to_existing_column.md

@@ -64,7 +64,7 @@ class AddNotValidForeignKeyToEmailsUser < ActiveRecord::Migration[5.2]
 
   def up
     # safe to use: it requires short lock on the table since we don't validate the foreign key
-    add_foreign_key :emails, :users, on_delete: :cascade, validate: false # rubocop:disable Migration/AddConcurrentForeignKey
+    add_foreign_key :emails, :users, on_delete: :cascade, validate: false
   end
 
   def down

rubocop/cop/migration/add_concurrent_foreign_key.rb

@@ -10,17 +10,29 @@ module RuboCop
 
         MSG = '`add_foreign_key` requires downtime, use `add_concurrent_foreign_key` instead'.freeze
 
+        def_node_matcher :false_node?, <<~PATTERN
+        (false)
+        PATTERN
+
         def on_send(node)
           return unless in_migration?(node)
 
           name = node.children[1]
 
-          add_offense(node, location: :selector) if name == :add_foreign_key
+          if name == :add_foreign_key && !not_valid_fk?(node)
+            add_offense(node, location: :selector)
+          end
         end
 
         def method_name(node)
           node.children.first
         end
+
+        def not_valid_fk?(node)
+          node.each_node(:pair).any? do |pair|
+            pair.children[0].children[0] == :validate && false_node?(pair.children[1])
+          end
+        end
       end
     end
   end

spec/rubocop/cop/migration/add_concurrent_foreign_key_spec.rb

@@ -33,5 +33,11 @@ describe RuboCop::Cop::Migration::AddConcurrentForeignKey do
         expect(cop.offenses.map(&:line)).to eq([1])
       end
     end
+
+    it 'does not register an offense when a `NOT VALID` foreign key is added' do
+      inspect_source('def up; add_foreign_key(:projects, :users, column: :user_id, validate: false); end')
+
+      expect(cop.offenses).to be_empty
+    end
   end
 end