Add disable transaction validate_foreign_key cop

Add a cop to deny the use of disable_ddl_transaction!
when only validate_foreign_key is used, as PostgreSQL will add and
implicit transaction for single statements anyway.

.rubocop_todo/rspec/missing_feature_category.yml

@@ -6975,6 +6975,7 @@ RSpec/MissingFeatureCategory:
     - 'spec/rubocop/cop/migration/migration_record_spec.rb'
     - 'spec/rubocop/cop/migration/prevent_global_enable_lock_retries_with_disable_ddl_transaction_spec.rb'
     - 'spec/rubocop/cop/migration/prevent_index_creation_spec.rb'
+    - 'spec/rubocop/cop/migration/prevent_single_statement_with_disable_ddl_transaction_spec.rb'
     - 'spec/rubocop/cop/migration/prevent_strings_spec.rb'
     - 'spec/rubocop/cop/migration/refer_to_index_by_name_spec.rb'
     - 'spec/rubocop/cop/migration/remove_column_spec.rb'

db/post_migrate/20230119095024_validate_fk_on_ci_builds_runner_session_partition_id_and_build_id.rb

 # frozen_string_literal: true
 
 class ValidateFkOnCiBuildsRunnerSessionPartitionIdAndBuildId < Gitlab::Database::Migration[2.1]
-  disable_ddl_transaction!
-
   TABLE_NAME = :ci_builds_runner_session
   FK_NAME = :fk_rails_70707857d3_p
   COLUMNS = [:partition_id, :build_id]

rubocop/cop/migration/prevent_single_statement_with_disable_ddl_transaction.rb

+# frozen_string_literal: true
+
+require_relative '../../migration_helpers'
+
+module RuboCop
+  module Cop
+    module Migration
+      # Cop that prevents usage of `disable_ddl_transaction!`
+      # if the only statement being called in the migration is :validate_foreign_key.
+      #
+      # We do this because PostgreSQL will add an implicit transaction for single
+      # statements. So there's no reason to add the disable_ddl_transaction!.
+      #
+      # This cop was introduced to clarify the need for disable_ddl_transaction!
+      # and to avoid bike-shedding and review back-and-forth.
+      #
+      # @examples
+      #
+      #   # bad
+      #   class SomeMigration < Gitlab::Database::Migration[2.1]
+      #     disable_ddl_transaction!
+      #     def up
+      #       validate_foreign_key :emails, :user_id
+      #     end
+      #     def down
+      #       # no-op
+      #     end
+      #   end
+      #
+      #   # good
+      #   class SomeMigration < Gitlab::Database::Migration[2.1]
+      #     def up
+      #       validate_foreign_key :emails, :user_id
+      #     end
+      #     def down
+      #       # no-op
+      #     end
+      #   end
+      class PreventSingleStatementWithDisableDdlTransaction < RuboCop::Cop::Base
+        include MigrationHelpers
+
+        MSG = "PostgreSQL will add an implicit transaction for single statements. " \
+              "So there's no reason to use `disable_ddl_transaction!`, if you're only " \
+              "executing validate_foreign_key."
+
+        def_node_matcher :disable_ddl_transaction?, <<~PATTERN
+          (send _ :disable_ddl_transaction! ...)
+        PATTERN
+
+        def_node_matcher :validate_foreign_key?, <<~PATTERN
+          (send :validate_foreign_key ...)
+        PATTERN
+
+        def on_begin(node)
+          return unless in_migration?(node)
+
+          disable_ddl_transaction_node = nil
+
+          # Only perform cop if disable_ddl_transaction! is present
+          node.each_descendant(:send) do |send_node|
+            disable_ddl_transaction_node = send_node if disable_ddl_transaction?(send_node)
+          end
+
+          return unless disable_ddl_transaction_node
+
+          # For each migration method, check if :validate_foreign_key is the only statement.
+          node.each_descendant(:def) do |def_node|
+            break unless [:up, :down, :change].include? def_node.children[0]
+
+            statement_count = 0
+            has_validate_foreign_key = false
+
+            def_node.each_descendant(:send) do |send_node|
+              has_validate_foreign_key = true if send_node.children[1] == :validate_foreign_key
+              statement_count += 1
+            end
+
+            if disable_ddl_transaction_node && has_validate_foreign_key && statement_count == 1
+              add_offense(disable_ddl_transaction_node, message: MSG)
+            end
+          end
+        end
+      end
+    end
+  end
+end

spec/rubocop/cop/migration/prevent_single_statement_with_disable_ddl_transaction_spec.rb

+# frozen_string_literal: true
+
+require 'rubocop_spec_helper'
+require_relative '../../../../rubocop/cop/migration/prevent_single_statement_with_disable_ddl_transaction'
+
+RSpec.describe RuboCop::Cop::Migration::PreventSingleStatementWithDisableDdlTransaction do
+  context 'when in migration' do
+    before do
+      allow(cop).to receive(:in_migration?).and_return(true)
+    end
+
+    it 'registers an offense when `disable_ddl_transaction!` is only for the :validate_foreign_key statement' do
+      code = <<~RUBY
+      class SomeMigration < Gitlab::Database::Migration[2.1]
+        disable_ddl_transaction!
+        def up
+          validate_foreign_key :emails, :user_id
+        end
+        def down
+          # no-op
+        end
+      end
+      RUBY
+
+      expect_offense(<<~RUBY, node: code, msg: described_class::MSG)
+        class SomeMigration < Gitlab::Database::Migration[2.1]
+          disable_ddl_transaction!
+          ^^^^^^^^^^^^^^^^^^^^^^^^ %{msg}
+          def up
+            validate_foreign_key :emails, :user_id
+          end
+          def down
+            # no-op
+          end
+        end
+      RUBY
+    end
+
+    it 'registers no offense when `disable_ddl_transaction!` is used with more than one statement' do
+      expect_no_offenses(<<~RUBY)
+        class SomeMigration < Gitlab::Database::Migration[2.1]
+          disable_ddl_transaction!
+          def up
+            add_concurrent_foreign_key :emails, :users, column: :user_id, on_delete: :cascade, validate: false
+            validate_foreign_key :emails, :user_id
+          end
+          def down
+            remove_foreign_key_if_exists :emails, column: :user_id
+          end
+        end
+      RUBY
+    end
+  end
+
+  context 'when outside of migration' do
+    it 'registers no offense' do
+      expect_no_offenses(<<~RUBY)
+        class SomeMigration
+          disable_ddl_transaction!
+          def up
+            validate_foreign_key :deployments, :environment_id
+          end
+        end
+      RUBY
+    end
+  end
+end