Merge branch '229852-refer-to-index-by-name-only' into 'master'

Add cop so indexes are referred to by name

See merge request gitlab-org/gitlab!39864

.rubocop.yml

@@ -522,3 +522,9 @@ Migration/ComplexIndexesRequireName:
   Exclude:
     - !ruby/regexp /\Adb\/(post_)?migrate\/201.*\.rb\z/
     - !ruby/regexp /\Adb\/(post_)?migrate\/20200[1-7].*\.rb\z/
+
+Migration/ReferToIndexByName:
+  Exclude:
+    - !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/

db/migrate/20200805071842_add_index_on_end_date_and_namespace_id_to_gitlab_subscriptions.rb

@@ -12,6 +12,7 @@ class AddIndexOnEndDateAndNamespaceIdToGitlabSubscriptions < ActiveRecord::Migra
   end
 
   def down
-    remove_concurrent_index :gitlab_subscriptions, [:end_date, :namespace_id]
+    remove_concurrent_index :gitlab_subscriptions, [:end_date, :namespace_id],
+      name: 'index_gitlab_subscriptions_on_end_date_and_namespace_id'
   end
 end

db/migrate/20200820204041_create_ci_platform_metrics.rb

@@ -22,6 +22,6 @@ class CreateCiPlatformMetrics < ActiveRecord::Migration[6.0]
 
   def down
     drop_table :ci_platform_metrics
-    remove_concurrent_index :ci_variables, :key
+    remove_concurrent_index :ci_variables, :key, name: 'index_ci_variables_on_key'
   end
 end

rubocop/cop/migration/refer_to_index_by_name.rb

+# frozen_string_literal: true
+
+require_relative '../../migration_helpers'
+
+module RuboCop
+  module Cop
+    module Migration
+      class ReferToIndexByName < RuboCop::Cop::Cop
+        include MigrationHelpers
+
+        MSG = 'migration methods that refer to existing indexes must do so by name'
+
+        def_node_matcher :match_index_exists, <<~PATTERN
+          (send _ :index_exists? _ _ (hash $...) ?)
+        PATTERN
+
+        def_node_matcher :match_remove_index, <<~PATTERN
+          (send _ :remove_index _ $_)
+        PATTERN
+
+        def_node_matcher :match_remove_concurrent_index, <<~PATTERN
+          (send _ :remove_concurrent_index _ _ (hash $...) ?)
+        PATTERN
+
+        def_node_matcher :name_option?, <<~PATTERN
+          (pair {(sym :name) (str "name")} _)
+        PATTERN
+
+        def on_def(node)
+          return unless in_migration?(node)
+
+          node.each_descendant(:send) do |send_node|
+            next unless index_exists_offense?(send_node) || removing_index_offense?(send_node)
+
+            add_offense(send_node, location: :selector)
+          end
+        end
+
+        private
+
+        def index_exists_offense?(send_node)
+          match_index_exists(send_node) { |option_nodes| needs_name_option?(option_nodes) }
+        end
+
+        def removing_index_offense?(send_node)
+          remove_index_offense?(send_node) || remove_concurrent_index_offense?(send_node)
+        end
+
+        def remove_index_offense?(send_node)
+          match_remove_index(send_node) do |column_or_options_node|
+            break true unless column_or_options_node.type == :hash
+
+            column_or_options_node.children.none? { |pair| name_option?(pair) }
+          end
+        end
+
+        def remove_concurrent_index_offense?(send_node)
+          match_remove_concurrent_index(send_node) { |option_nodes| needs_name_option?(option_nodes) }
+        end
+
+        def needs_name_option?(option_nodes)
+          option_nodes.empty? || option_nodes.first.none? { |node| name_option?(node) }
+        end
+      end
+    end
+  end
+end

spec/rubocop/cop/migration/refer_to_index_by_name_spec.rb

+# frozen_string_literal: true
+#
+require 'fast_spec_helper'
+require 'rubocop'
+require_relative '../../../../rubocop/cop/migration/refer_to_index_by_name'
+
+RSpec.describe RuboCop::Cop::Migration::ReferToIndexByName, type: :rubocop do
+  include CopHelper
+
+  subject(:cop) { described_class.new }
+
+  context 'in migration' do
+    before do
+      allow(cop).to receive(:in_migration?).and_return(true)
+    end
+
+    context 'when existing indexes are referred to without an explicit name' do
+      it 'registers an offense' do
+        expect_offense(<<~RUBY)
+          class TestReferToIndexByName < ActiveRecord::Migration[6.0]
+            DOWNTIME = false
+
+            INDEX_NAME = 'my_test_name'
+
+            disable_ddl_transaction!
+
+            def up
+              if index_exists? :test_indexes, :column1, name: 'index_name_1'
+                remove_index :test_indexes, column: :column1, name: 'index_name_1'
+              end
+
+              if index_exists? :test_indexes, :column2
+                 ^^^^^^^^^^^^^ #{described_class::MSG}
+                remove_index :test_indexes, :column2
+                ^^^^^^^^^^^^ #{described_class::MSG}
+              end
+
+              remove_index :test_indexes, column: column3
+              ^^^^^^^^^^^^ #{described_class::MSG}
+
+              remove_index :test_indexes, name: 'index_name_4'
+            end
+
+            def down
+              if index_exists? :test_indexes, :column4, using: :gin, opclass: :gin_trgm_ops
+                 ^^^^^^^^^^^^^ #{described_class::MSG}
+                remove_concurrent_index :test_indexes, :column4, using: :gin, opclass: :gin_trgm_ops
+                ^^^^^^^^^^^^^^^^^^^^^^^ #{described_class::MSG}
+              end
+
+              if index_exists? :test_indexes, :column3, unique: true, name: 'index_name_3', where: 'column3 = 10'
+                remove_concurrent_index :test_indexes, :column3, unique: true, name: 'index_name_3', where: 'column3 = 10'
+              end
+            end
+          end
+        RUBY
+
+        expect(cop.offenses.map(&:cop_name)).to all(eq("Migration/#{described_class.name.demodulize}"))
+      end
+    end
+  end
+
+  context 'outside migration' do
+    before do
+      allow(cop).to receive(:in_migration?).and_return(false)
+    end
+
+    it 'registers no offenses' do
+      expect_no_offenses(<<~RUBY)
+        class TestReferToIndexByName < ActiveRecord::Migration[6.0]
+          DOWNTIME = false
+
+          disable_ddl_transaction!
+
+          def up
+            if index_exists? :test_indexes, :column1
+              remove_index :test_indexes, :column1
+            end
+          end
+
+          def down
+            if index_exists? :test_indexes, :column1
+              remove_concurrent_index :test_indexes, :column1
+            end
+          end
+        end
+      RUBY
+    end
+  end
+end