Merge branch '229852-require-name-for-all-complex-index-creation' into 'master'

Require name for all complex indexes

See merge request gitlab-org/gitlab!40435

rubocop/cop/migration/complex_indexes_require_name.rb

@@ -10,8 +10,12 @@ module RuboCop
 
         MSG = 'indexes added with custom options must be explicitly named'
 
+        def_node_matcher :match_create_table_index_with_options, <<~PATTERN
+          (send _ {:index } _ (hash $...))
+        PATTERN
+
         def_node_matcher :match_add_index_with_options, <<~PATTERN
-          (send _ {:add_concurrent_index} _ _ (hash $...))
+          (send _ {:add_index :add_concurrent_index} _ _ (hash $...))
         PATTERN
 
         def_node_matcher :name_option?, <<~PATTERN
@@ -26,7 +30,7 @@ module RuboCop
           return unless in_migration?(node)
 
           node.each_descendant(:send) do |send_node|
-            next unless add_index_offense?(send_node)
+            next unless create_table_with_index_offense?(send_node) || add_index_offense?(send_node)
 
             add_offense(send_node, location: :selector)
           end
@@ -34,6 +38,10 @@ module RuboCop
 
         private
 
+        def create_table_with_index_offense?(send_node)
+          match_create_table_index_with_options(send_node) { |option_nodes| needs_name_option?(option_nodes) }
+        end
+
         def add_index_offense?(send_node)
           match_add_index_with_options(send_node) { |option_nodes| needs_name_option?(option_nodes) }
         end

spec/rubocop/cop/migration/complex_indexes_require_name_spec.rb

@@ -14,32 +14,83 @@ RSpec.describe RuboCop::Cop::Migration::ComplexIndexesRequireName, type: :ruboco
       allow(cop).to receive(:in_migration?).and_return(true)
     end
 
+    context 'when creating complex indexes as part of create_table' do
       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'
+              def up
+                create_table :test_table do |t|
+                  t.integer :column1, null: false
+                  t.integer :column2, null: false
+                  t.jsonb :column3
+
+                  t.index :column1, unique: true
+                  t.index :column2, where: 'column1 = 0'
+                    ^^^^^ #{described_class::MSG}
+                  t.index :column3, using: :gin
+                    ^^^^^ #{described_class::MSG}
+                end
+              end
 
-            disable_ddl_transaction!
+              def down
+                drop_table :test_table
+              end
+            end
+          RUBY
 
-            def up
-              add_concurrent_index :test_indexes, :column1
+          expect(cop.offenses.map(&:cop_name)).to all(eq("Migration/#{described_class.name.demodulize}"))
+        end
+      end
 
-              add_concurrent_index :test_indexes, :column2, where: "column2 = 'value'", order: { column4: :desc }
-              ^^^^^^^^^^^^^^^^^^^^ #{described_class::MSG}
+      context 'when indexes are configured with an options hash and name' do
+        it 'registers no offense' do
+          expect_no_offenses(<<~RUBY)
+            class TestComplexIndexes < ActiveRecord::Migration[6.0]
+              DOWNTIME = false
 
-              add_concurrent_index :test_indexes, :column3, where: 'column3 = 10', name: 'idx_equal_to_10'
+              def up
+                create_table :test_table do |t|
+                  t.integer :column1, null: false
+                  t.integer :column2, null: false
+                  t.jsonb :column3
+
+                  t.index :column1, unique: true
+                  t.index :column2, where: 'column1 = 0', name: 'my_index_1'
+                  t.index :column3, using: :gin, name: 'my_gin_index'
+                end
               end
 
               def down
-              add_concurrent_index :test_indexes, :column4, 'unique' => true
+                drop_table :test_table
+              end
+            end
+          RUBY
+        end
+      end
+    end
 
-              add_concurrent_index :test_indexes, :column4, 'unique' => true, where: 'column4 IS NOT NULL'
-              ^^^^^^^^^^^^^^^^^^^^ #{described_class::MSG}
+    context 'when indexes are added to an existing table' do
+      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
+
+              disable_ddl_transaction!
 
-              add_concurrent_index :test_indexes, :column5, using: :gin, name: INDEX_NAME
+              def up
+                add_index :test_indexes, :column1
+
+                add_index :test_indexes, :column2, where: "column2 = 'value'", order: { column4: :desc }
+                ^^^^^^^^^ #{described_class::MSG}
+              end
+
+              def down
+                add_index :test_indexes, :column4, 'unique' => true, where: 'column4 IS NOT NULL'
+                ^^^^^^^^^ #{described_class::MSG}
 
                 add_concurrent_index :test_indexes, :column6, using: :gin, opclass: :gin_trgm_ops
                 ^^^^^^^^^^^^^^^^^^^^ #{described_class::MSG}
@@ -50,6 +101,35 @@ RSpec.describe RuboCop::Cop::Migration::ComplexIndexesRequireName, type: :ruboco
           expect(cop.offenses.map(&:cop_name)).to all(eq("Migration/#{described_class.name.demodulize}"))
         end
       end
+
+      context 'when indexes are configured with an options hash and a name' do
+        it 'registers no offenses' do
+          expect_no_offenses(<<~RUBY)
+            class TestComplexIndexes < ActiveRecord::Migration[6.0]
+              DOWNTIME = false
+
+              INDEX_NAME = 'my_test_name'
+
+              disable_ddl_transaction!
+
+              def up
+                add_index :test_indexes, :column1
+
+                add_index :test_indexes, :column2, where: "column2 = 'value'", order: { column4: :desc }, name: 'my_index_1'
+
+                add_concurrent_index :test_indexes, :column3, where: 'column3 = 10', name: 'idx_equal_to_10'
+              end
+
+              def down
+                add_index :test_indexes, :column4, 'unique' => true, where: 'column4 IS NOT NULL', name: 'my_index_2'
+
+                add_concurrent_index :test_indexes, :column6, using: :gin, opclass: :gin_trgm_ops, name: INDEX_NAME
+              end
+            end
+          RUBY
+        end
+      end
+    end
   end
 
   context 'outside migration' do
@@ -65,7 +145,13 @@ RSpec.describe RuboCop::Cop::Migration::ComplexIndexesRequireName, type: :ruboco
           disable_ddl_transaction!
 
           def up
-            add_concurrent_index :test_indexes, :column1, where: "some_column = 'value'"
+            create_table :test_table do |t|
+              t.integer :column1
+
+              t.index :column1, where: 'column2 IS NOT NULL'
+            end
+
+            add_index :test_indexes, :column1, where: "some_column = 'value'"
           end
 
           def down