Merge branch '229852-index-helpers-dont-respect-where-clause' into 'master'

Add cop to detect complex indexes without names

See merge request gitlab-org/gitlab!39503

.rubocop.yml

@@ -509,3 +509,8 @@ Cop/PutGroupRoutesUnderScope:
   Include:
     - 'config/routes/group.rb'
     - 'ee/config/routes/group.rb'
+
+Migration/ComplexIndexesRequireName:
+  Exclude:
+    - !ruby/regexp /\Adb\/(post_)?migrate\/201.*\.rb\z/
+    - !ruby/regexp /\Adb\/(post_)?migrate\/20200[1-7].*\.rb\z/

db/migrate/20200813143356_remove_old_external_diff_migration_index.rb

@@ -18,7 +18,8 @@ def down
     add_concurrent_index(
       :merge_request_diffs,
       [:merge_request_id, :id],
-      where: { stored_externally: [nil, false] }
+      where: 'NOT stored_externally OR stored_externally IS NULL',
+      name: 'index_merge_request_diffs_on_merge_request_id_and_id_partial'
     )
   end
 end

rubocop/cop/migration/complex_indexes_require_name.rb

+# frozen_string_literal: true
+
+require_relative '../../migration_helpers'
+
+module RuboCop
+  module Cop
+    module Migration
+      class ComplexIndexesRequireName < RuboCop::Cop::Cop
+        include MigrationHelpers
+
+        MSG = 'indexes added with custom options must be explicitly named'
+
+        def_node_matcher :match_add_index_with_options, <<~PATTERN
+          (send _ {:add_concurrent_index} _ _ (hash $...))
+        PATTERN
+
+        def_node_matcher :name_option?, <<~PATTERN
+          (pair {(sym :name) (str "name")} _)
+        PATTERN
+
+        def_node_matcher :unique_option?, <<~PATTERN
+          (pair {(:sym :unique) (str "unique")} _)
+        PATTERN
+
+        def on_def(node)
+          return unless in_migration?(node)
+
+          node.each_descendant(:send) do |send_node|
+            next unless add_index_offense?(send_node)
+
+            add_offense(send_node, location: :selector)
+          end
+        end
+
+        private
+
+        def add_index_offense?(send_node)
+          match_add_index_with_options(send_node) { |option_nodes| needs_name_option?(option_nodes) }
+        end
+
+        def needs_name_option?(option_nodes)
+          return false if only_unique_option?(option_nodes)
+
+          option_nodes.none? { |node| name_option?(node) }
+        end
+
+        def only_unique_option?(option_nodes)
+          option_nodes.size == 1 && unique_option?(option_nodes.first)
+        end
+      end
+    end
+  end
+end

spec/rubocop/cop/migration/complex_indexes_require_name_spec.rb

+# frozen_string_literal: true
+#
+require 'fast_spec_helper'
+require 'rubocop'
+require_relative '../../../../rubocop/cop/migration/complex_indexes_require_name'
+
+RSpec.describe RuboCop::Cop::Migration::ComplexIndexesRequireName, 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 indexes are configured with an options hash, but no name' do
+      it 'registers an offense' do
+        expect_offense(<<~RUBY)
+          class TestComplexIndexes < ActiveRecord::Migration[6.0]
+            DOWNTIME = false
+
+            INDEX_NAME = 'my_test_name'
+
+            disable_ddl_transaction!
+
+            def up
+              add_concurrent_index :test_indexes, :column1
+
+              add_concurrent_index :test_indexes, :column2, where: "column2 = 'value'", order: { column4: :desc }
+              ^^^^^^^^^^^^^^^^^^^^ #{described_class::MSG}
+
+              add_concurrent_index :test_indexes, :column3, where: 'column3 = 10', name: 'idx_equal_to_10'
+            end
+
+            def down
+              add_concurrent_index :test_indexes, :column4, 'unique' => true
+
+              add_concurrent_index :test_indexes, :column4, 'unique' => true, where: 'column4 IS NOT NULL'
+              ^^^^^^^^^^^^^^^^^^^^ #{described_class::MSG}
+
+              add_concurrent_index :test_indexes, :column5, using: :gin, name: INDEX_NAME
+
+              add_concurrent_index :test_indexes, :column6, using: :gin, opclass: :gin_trgm_ops
+              ^^^^^^^^^^^^^^^^^^^^ #{described_class::MSG}
+            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 TestComplexIndexes < ActiveRecord::Migration[6.0]
+          DOWNTIME = false
+
+          disable_ddl_transaction!
+
+          def up
+            add_concurrent_index :test_indexes, :column1, where: "some_column = 'value'"
+          end
+
+          def down
+            add_concurrent_index :test_indexes, :column2, unique: true
+          end
+        end
+      RUBY
+    end
+  end
+end