Merge branch 'ab/rubocop-add-reference' into 'master'

Only allow add_reference for newly created tables

See merge request gitlab-org/gitlab!16618

db/migrate/20190611161641_add_target_project_id_to_merge_trains.rb

@@ -6,9 +6,9 @@ class AddTargetProjectIdToMergeTrains < ActiveRecord::Migration[5.1]
   DOWNTIME = false
 
   def change
-    # rubocop: disable Rails/NotNullColumn
+    # rubocop:disable Rails/NotNullColumn, Migration/AddReference
     add_reference :merge_trains, :target_project, null: false, index: true, foreign_key: { on_delete: :cascade, to_table: :projects }, type: :integer
     add_column :merge_trains, :target_branch, :text, null: false
-    # rubocop: enable Rails/NotNullColumn
+    # rubocop:enable Rails/NotNullColumn, Migration/AddReference
   end
 end

db/migrate/20190712040400_add_environment_id_to_clusters_kubernetes_namespaces.rb

@@ -4,7 +4,9 @@ class AddEnvironmentIdToClustersKubernetesNamespaces < ActiveRecord::Migration[5
   DOWNTIME = false
 
   def change
+    # rubocop:disable Migration/AddReference
     add_reference :clusters_kubernetes_namespaces, :environment,
       index: true, type: :bigint, foreign_key: { on_delete: :nullify }
+    # rubocop:enable Migration/AddReference
   end
 end

db/migrate/20190715042813_add_issue_id_to_versions.rb

@@ -4,7 +4,9 @@ class AddIssueIdToVersions < ActiveRecord::Migration[5.2]
   DOWNTIME = false
 
   def up
+    # rubocop:disable Migration/AddReference
     add_reference :design_management_versions, :issue, index: true, foreign_key: { on_delete: :cascade }
+    # rubocop:enable Migration/AddReference
   end
 
   def down

db/migrate/20190724112147_add_column_for_self_monitoring_project_id.rb

@@ -4,11 +4,13 @@ class AddColumnForSelfMonitoringProjectId < ActiveRecord::Migration[5.2]
   DOWNTIME = false
 
   def change
+    # rubocop:disable Migration/AddReference
     add_reference(
       :application_settings,
       :instance_administration_project,
       index: { name: 'index_applicationsettings_on_instance_administration_project_id' },
       foreign_key: { to_table: :projects, on_delete: :nullify }
     )
+    # rubocop:enable Migration/AddReference
   end
 end

rubocop/cop/migration/add_reference.rb

@@ -4,22 +4,62 @@ require_relative '../../migration_helpers'
 module RuboCop
   module Cop
     module Migration
-      # Cop that checks if a foreign key constraint is added and require a index for it
+      # add_reference can only be used with newly created tables.
+      # Additionally, the cop here checks that we create an index for the foreign key, too.
       class AddReference < RuboCop::Cop::Cop
         include MigrationHelpers
 
-        MSG = '`add_reference` requires `index: true` or `index: { options... }`'
+        MSG = '`add_reference` requires downtime for existing tables, use `add_concurrent_foreign_key` instead. When used for new tables, `index: true` or `index: { options... } is required.`'
 
-        def on_send(node)
+        def on_def(node)
           return unless in_migration?(node)
 
-          name = node.children[1]
+          new_tables = []
 
-          return unless name == :add_reference
+          node.each_descendant(:send) do |send_node|
+            first_arg = first_argument(send_node)
 
+            # The first argument of "create_table" / "add_reference" is the table
+            # name.
+            new_tables << first_arg if create_table?(send_node)
+
+            next if method_name(send_node) != :add_reference
+
+            # Using "add_reference" is fine for newly created tables as there's no
+            # data in these tables yet.
+            if existing_table?(new_tables, first_arg)
+              add_offense(send_node, location: :selector)
+            end
+
+            # We require an index on the foreign key column.
+            if index_missing?(node)
+              add_offense(send_node, location: :selector)
+            end
+          end
+        end
+
+        private
+
+        def existing_table?(new_tables, table)
+          !new_tables.include?(table)
+        end
+
+        def create_table?(node)
+          method_name(node) == :create_table
+        end
+
+        def method_name(node)
+          node.children[1]
+        end
+
+        def first_argument(node)
+          node.children[2]
+        end
+
+        def index_missing?(node)
           opts = node.children.last
 
-          add_offense(node, location: :selector) unless opts && opts.type == :hash
+          return true if opts && opts.type == :hash
 
           index_present = false
 
@@ -27,11 +67,9 @@ module RuboCop
             index_present ||= index_enabled?(pair)
           end
 
-          add_offense(node, location: :selector) unless index_present
+          !index_present
         end
 
-        private
-
         def index_enabled?(pair)
           return unless hash_key_type(pair) == :sym
           return unless hash_key_name(pair) == :index

spec/rubocop/cop/migration/add_reference_spec.rb

@@ -25,11 +25,55 @@ describe RuboCop::Cop::Migration::AddReference do
       allow(cop).to receive(:in_migration?).and_return(true)
     end
 
+    let(:offense) { '`add_reference` requires downtime for existing tables, use `add_concurrent_foreign_key` instead. When used for new tables, `index: true` or `index: { options... } is required.`' }
+
+    context 'when the table existed before' do
+      it 'registers an offense when using add_reference' do
+        expect_offense(<<~RUBY)
+          def up
+            add_reference(:projects, :users)
+            ^^^^^^^^^^^^^ #{offense}
+          end
+        RUBY
+      end
+
+      it 'registers an offense when using add_reference with index enabled' do
+        expect_offense(<<~RUBY)
+        def up
+          add_reference(:projects, :users, index: true)
+          ^^^^^^^^^^^^^ #{offense}
+        end
+        RUBY
+      end
+
+      it 'registers an offense if only a different table was created' do
+        expect_offense(<<~RUBY)
+        def up
+          create_table(:foo) do |t|
+            t.string :name
+          end
+          add_reference(:projects, :users, index: true)
+          ^^^^^^^^^^^^^ #{offense}
+        end
+        RUBY
+      end
+    end
+
+    context 'when creating the table at the same time' do
+      let(:create_table_statement) do
+        <<~RUBY
+          create_table(:projects) do |t|
+            t.string :name
+          end
+        RUBY
+      end
+
       it 'registers an offense when using add_reference without index' do
         expect_offense(<<~RUBY)
-        call do
+        def up
+          #{create_table_statement}
           add_reference(:projects, :users)
-          ^^^^^^^^^^^^^ `add_reference` requires `index: true` or `index: { options... }`
+          ^^^^^^^^^^^^^ #{offense}
         end
         RUBY
       end
@@ -37,8 +81,9 @@ describe RuboCop::Cop::Migration::AddReference do
       it 'registers an offense when using add_reference index disabled' do
         expect_offense(<<~RUBY)
         def up
+          #{create_table_statement}
           add_reference(:projects, :users, index: false)
-          ^^^^^^^^^^^^^ `add_reference` requires `index: true` or `index: { options... }`
+          ^^^^^^^^^^^^^ #{offense}
         end
         RUBY
       end
@@ -46,6 +91,7 @@ describe RuboCop::Cop::Migration::AddReference do
       it 'does not register an offense when using add_reference with index enabled' do
         expect_no_offenses(<<~RUBY)
         def up
+          #{create_table_statement}
           add_reference(:projects, :users, index: true)
         end
         RUBY
@@ -54,9 +100,11 @@ describe RuboCop::Cop::Migration::AddReference do
       it 'does not register an offense when the index is unique' do
         expect_no_offenses(<<~RUBY)
         def up
+          #{create_table_statement}
           add_reference(:projects, :users, index: { unique: true } )
         end
         RUBY
       end
     end
+  end
 end