Merge branch 'add_rubocop_rule_for_creating_table_with_two_foreign_keys' into 'master'

Implement a Rubocop cop to check db style guide

See merge request gitlab-org/gitlab!40858

.rubocop.yml

@@ -538,3 +538,8 @@ Migration/ReferToIndexByName:
     - !ruby/regexp /\Adb\/(post_)?migrate\/201.*\.rb\z/
     - !ruby/regexp /\Adb\/(post_)?migrate\/20200[1-7].*\.rb\z/
     - !ruby/regexp /\Aee\/db\/geo\/(post_)?migrate\/201.*\.rb\z/
+
+Migration/CreateTableWithForeignKeys:
+  # Disable this cop for all the existing migrations
+  Exclude:
+    - !ruby/regexp /\Adb\/(?:post_)?migrate\/(?:201[0-9]\d+|20200[0-8][0-9][0-9][0-9][0-9][0-9][0-9][0-9][0-9])_.+\.rb\z/

rubocop/cop/migration/create_table_with_foreign_keys.rb

+# frozen_string_literal: true
+
+require_relative '../../migration_helpers'
+
+module RuboCop
+  module Cop
+    module Migration
+      class CreateTableWithForeignKeys < RuboCop::Cop::Cop
+        include MigrationHelpers
+
+        MSG = 'Creating a table with more than one foreign key at once violates our migration style guide. ' \
+          'For more details check the https://docs.gitlab.com/ce/development/migration_style_guide.html#examples'
+
+        def_node_matcher :create_table_with_block?, <<~PATTERN
+          (block
+            (send nil? :create_table ...)
+            (args (arg _var))
+            _)
+        PATTERN
+
+        def_node_search :belongs_to_and_references, <<~PATTERN
+          (send _var {:references :belongs_to} $...)
+        PATTERN
+
+        def_node_search :foreign_key_options, <<~PATTERN
+          (_pair
+            {(sym :foreign_key) (str "foreign_key")}
+            {(hash _) (true)}
+          )
+        PATTERN
+
+        def_node_search :to_table, <<~PATTERN
+          (_pair
+            {(sym :to_table) (str "to_table")} {(sym $...) (str $...)}
+          )
+        PATTERN
+
+        def_node_search :argument_name, <<~PATTERN
+          {(sym $...) (str $...)}
+        PATTERN
+
+        def_node_search :standalone_foreign_keys, <<~PATTERN
+          (send _var :foreign_key $...)
+        PATTERN
+
+        def on_send(node)
+          return unless in_migration?(node)
+          return unless node.command?(:create_table)
+          return unless create_table_with_block?(node.parent)
+
+          add_offense(node) if violates?(node.parent)
+        end
+
+        private
+
+        def violates?(node)
+          tables = all_target_tables(node).uniq
+
+          tables.length > 1 && !(tables & high_traffic_tables).empty?
+        end
+
+        def all_target_tables(node)
+          belongs_to_and_references_foreign_key_targets(node) + standalone_foreign_key_targets(node)
+        end
+
+        def belongs_to_and_references_foreign_key_targets(node)
+          belongs_to_and_references(node).select { |candidate| has_fk_option?(candidate) }
+                                         .flat_map { |definition| definition_to_table_names(definition) }
+                                         .compact
+        end
+
+        def standalone_foreign_key_targets(node)
+          standalone_foreign_keys(node).flat_map { |definition| definition_to_table_names(definition) }
+                                       .compact
+        end
+
+        def has_fk_option?(candidate)
+          foreign_key_options(candidate.last).first
+        end
+
+        def definition_to_table_names(definition)
+          table_name_from_options(definition.last) || arguments_to_table_names(definition)
+        end
+
+        def table_name_from_options(options)
+          to_table(options).to_a.first&.first
+        end
+
+        def arguments_to_table_names(arguments)
+          arguments.flat_map { |argument| argument_name(argument).to_a.flatten.first.to_s.pluralize.to_sym }
+        end
+      end
+    end
+  end
+end

rubocop/migration_helpers.rb

@@ -20,6 +20,10 @@ module RuboCop
 
     TABLE_METHODS = %i(create_table create_table_if_not_exists change_table).freeze
 
+    def high_traffic_tables
+      @high_traffic_tables ||= rubocop_migrations_config.dig('Migration/UpdateLargeTable', 'DeniedTables')
+    end
+
     # Returns true if the given node originated from the db/migrate directory.
     def in_migration?(node)
       in_deployment_migration?(node) || in_post_deployment_migration?(node)
@@ -52,5 +56,13 @@ module RuboCop
     def dirname(node)
       File.dirname(node.location.expression.source_buffer.name)
     end
+
+    def rubocop_migrations_config
+      @rubocop_migrations_config ||= YAML.load_file(File.join(rubocop_path, 'rubocop-migrations.yml'))
+    end
+
+    def rubocop_path
+      File.expand_path(__dir__)
+    end
   end
 end

spec/rubocop/cop/migration/create_table_with_foreign_keys_spec.rb

+# frozen_string_literal: true
+
+require 'fast_spec_helper'
+require 'rubocop'
+require_relative '../../../../rubocop/cop/migration/create_table_with_foreign_keys'
+
+RSpec.describe RuboCop::Cop::Migration::CreateTableWithForeignKeys, type: :rubocop do
+  include CopHelper
+
+  let(:cop) { described_class.new }
+
+  context 'outside of a migration' do
+    it 'does not register any offenses' do
+      expect_no_offenses(<<~RUBY)
+        def up
+          create_table(:foo) do |t|
+            t.references :bar, foreign_key: { on_delete: 'cascade' }
+            t.references :zoo, foreign_key: { on_delete: 'cascade' }
+          end
+        end
+      RUBY
+    end
+  end
+
+  context 'in a migration' do
+    before do
+      allow(cop).to receive(:in_migration?).and_return(true)
+    end
+
+    context 'without foreign key' do
+      it 'does not register any offenses' do
+        expect_no_offenses(<<~RUBY)
+          def up
+            create_table(:foo) do |t|
+              t.references :bar
+            end
+          end
+        RUBY
+      end
+    end
+
+    context 'with foreign key' do
+      context 'with just one foreign key' do
+        context 'when the foreign_key targets a high traffic table' do
+          it 'does not register any offenses' do
+            expect_no_offenses(<<~RUBY)
+              def up
+                create_table(:foo) do |t|
+                  t.references :project, "foreign_key" => { on_delete: 'cascade', to_table: 'projects' }
+                end
+              end
+            RUBY
+          end
+        end
+
+        context 'when the foreign_key does not target a high traffic table' do
+          it 'does not register any offenses' do
+            expect_no_offenses(<<~RUBY)
+              def up
+                create_table(:foo) do |t|
+                  t.references :bar, foreign_key: { on_delete: 'cascade' }
+                end
+              end
+            RUBY
+          end
+        end
+      end
+
+      context 'with more than one foreign keys' do
+        let(:offense) do
+          'Creating a table with more than one foreign key at once violates our migration style guide. ' \
+          'For more details check the https://docs.gitlab.com/ce/development/migration_style_guide.html#examples'
+        end
+
+        shared_examples 'target to high traffic table' do |dsl_method, table_name|
+          context 'when the target is defined as option' do
+            it 'registers an offense ' do
+              expect_offense(<<~RUBY)
+                def up
+                  create_table(:foo) do |t|
+                  ^^^^^^^^^^^^^^^^^^ #{offense}
+                    t.#{dsl_method} :#{table_name.singularize} #{explicit_target_opts}
+                    t.#{dsl_method} :zoo #{implicit_target_opts}
+                  end
+                end
+              RUBY
+            end
+          end
+
+          context 'when the target has implicit definition' do
+            it 'registers an offense ' do
+              expect_offense(<<~RUBY)
+                def up
+                  create_table(:foo) do |t|
+                  ^^^^^^^^^^^^^^^^^^ #{offense}
+                    t.#{dsl_method} :#{table_name.singularize} #{implicit_target_opts}
+                    t.#{dsl_method} :zoo #{implicit_target_opts}
+                  end
+                end
+              RUBY
+            end
+          end
+        end
+
+        shared_context 'when there is a target to a high traffic table' do |dsl_method|
+          %w[
+            audit_events
+            ci_build_trace_sections
+            ci_builds
+            ci_builds_metadata
+            ci_job_artifacts
+            ci_pipeline_variables
+            ci_pipelines
+            ci_stages
+            deployments
+            events
+            gitlab_subscriptions
+            issues
+            merge_request_diff_commits
+            merge_request_diff_files
+            merge_request_diffs
+            merge_request_metrics
+            merge_requests
+            namespaces
+            note_diff_files
+            notes
+            project_authorizations
+            projects
+            project_ci_cd_settings
+            project_features
+            push_event_payloads
+            resource_label_events
+            routes
+            sent_notifications
+            system_note_metadata
+            taggings
+            todos
+            users
+            web_hook_logs
+          ].each do |table|
+            let(:table_name) { table }
+
+            it_behaves_like 'target to high traffic table', dsl_method, table
+          end
+        end
+
+        context 'when the foreign keys are defined as options' do
+          context 'when there is no target to a high traffic table' do
+            it 'does not register any offenses' do
+              expect_no_offenses(<<~RUBY)
+                def up
+                  create_table(:foo) do |t|
+                    t.references :bar, foreign_key: { on_delete: 'cascade', to_table: :bars }
+                    t.references :zoo, 'foreign_key' => { on_delete: 'cascade' }
+                    t.references :john, 'foreign_key' => { on_delete: 'cascade' }
+                    t.references :doe, 'foreign_key' => { on_delete: 'cascade', to_table: 'doe' }
+                  end
+                end
+              RUBY
+            end
+          end
+
+          include_context 'when there is a target to a high traffic table', :references do
+            let(:explicit_target_opts) { ", foreign_key: { to_table: :#{table_name} }" }
+            let(:implicit_target_opts) { ", foreign_key: true" }
+          end
+        end
+
+        context 'when the foreign keys are defined by standlone migration helper' do
+          context 'when there is no target to a high traffic table' do
+            it 'does not register any offenses' do
+              expect_no_offenses(<<~RUBY)
+                def up
+                  create_table(:foo) do |t|
+                    t.foreign_key :bar
+                    t.foreign_key :zoo, to_table: 'zoos'
+                  end
+                end
+              RUBY
+            end
+          end
+
+          include_context 'when there is a target to a high traffic table', :foreign_key do
+            let(:explicit_target_opts) { ", to_table: :#{table_name}" }
+            let(:implicit_target_opts) { }
+          end
+        end
+      end
+    end
+  end
+end