Accept columns passed with options in remove_index

Before this patch, column names could only be passed as a positional
argument when no other options were supplied:

    remove_index :reports, :report_id

Passing column names positionally along with other options would fail:

    remove_index :reports, :report_id, unique: true
    # => ArgumentError: wrong number of arguments (given 3, expected 1..2)

activerecord/CHANGELOG.md

+*   Allow column names to be passed to `remove_index` positionally along with other options.
+
+    Passing other options can be necessary to make `remove_index` correctly reversible.
+
+    Before:
+
+        add_index    :reports, :report_id               # => works
+        add_index    :reports, :report_id, unique: true # => works
+        remove_index :reports, :report_id               # => works
+        remove_index :reports, :report_id, unique: true # => ArgumentError
+
+    After:
+
+        remove_index :reports, :report_id, unique: true # => works
+
+    *Eugene Kenny*
+
 *   Allow bulk `ALTER` statements to drop and recreate indexes with the same name.
 
     *Eugene Kenny*

activerecord/lib/active_record/connection_adapters/abstract/schema_definitions.rb

@@ -631,10 +631,11 @@ def remove(*column_names)
       #   t.remove_index(:branch_id)
       #   t.remove_index(column: [:branch_id, :party_id])
       #   t.remove_index(name: :by_branch_party)
+      #   t.remove_index(:branch_id, name: :by_branch_party)
       #
       # See {connection.remove_index}[rdoc-ref:SchemaStatements#remove_index]
-      def remove_index(options = {})
-        @base.remove_index(name, options)
+      def remove_index(column_name = nil, options = {})
+        @base.remove_index(name, column_name, options)
       end
 
       # Removes the timestamp columns (+created_at+ and +updated_at+) from the table.

activerecord/lib/active_record/connection_adapters/abstract/schema_statements.rb

@@ -807,6 +807,10 @@ def add_index(table_name, column_name, options = {})
       #
       #   remove_index :accounts, name: :by_branch_party
       #
+      # Removes the index on +branch_id+ named +by_branch_party+ in the +accounts+ table.
+      #
+      #   remove_index :accounts, :branch_id, name: :by_branch_party
+      #
       # Removes the index named +by_branch_party+ in the +accounts+ table +concurrently+.
       #
       #   remove_index :accounts, name: :by_branch_party, algorithm: :concurrently
@@ -816,8 +820,8 @@ def add_index(table_name, column_name, options = {})
       # Concurrently removing an index is not supported in a transaction.
       #
       # For more information see the {"Transactional Migrations" section}[rdoc-ref:Migration].
-      def remove_index(table_name, options = {})
-        index_name = index_name_for_remove(table_name, options)
+      def remove_index(table_name, column_name = nil, options = {})
+        index_name = index_name_for_remove(table_name, column_name, options)
         execute "DROP INDEX #{quote_column_name(index_name)} ON #{quote_table_name(table_name)}"
       end
 
@@ -1265,17 +1269,18 @@ def quoted_columns_for_index(column_names, **options)
           add_options_for_index_columns(quoted_columns, **options).values
         end
 
-        def index_name_for_remove(table_name, options = {})
-          return options[:name] if can_remove_index_by_name?(options)
+        def index_name_for_remove(table_name, column_name, options)
+          if column_name.is_a?(Hash)
+            options = column_name.dup
+            column_name = options.delete(:column)
+          end
+
+          return options[:name] if can_remove_index_by_name?(column_name, options)
 
           checks = []
 
-          if options.is_a?(Hash)
-            checks << lambda { |i| i.name == options[:name].to_s } if options.key?(:name)
-            column_names = index_column_names(options[:column])
-          else
-            column_names = index_column_names(options)
-          end
+          checks << lambda { |i| i.name == options[:name].to_s } if options.key?(:name)
+          column_names = index_column_names(column_name)
 
           if column_names.present?
             checks << lambda { |i| index_name(table_name, i.columns) == index_name(table_name, column_names) }
@@ -1406,8 +1411,8 @@ def extract_new_default_value(default_or_changes)
         end
         alias :extract_new_comment_value :extract_new_default_value
 
-        def can_remove_index_by_name?(options)
-          options.is_a?(Hash) && options.key?(:name) && options.except(:name, :algorithm).empty?
+        def can_remove_index_by_name?(column_name, options)
+          column_name.nil? && options.key?(:name) && options.except(:name, :algorithm).empty?
         end
 
         def bulk_change_table(table_name, operations)

activerecord/lib/active_record/connection_adapters/abstract_mysql_adapter.rb

@@ -668,8 +668,8 @@ def add_index_for_alter(table_name, column_name, options = {})
           "ADD #{index_type} INDEX #{quote_column_name(index_name)} #{index_using} (#{index_columns})#{index_algorithm}"
         end
 
-        def remove_index_for_alter(table_name, options = {})
-          index_name = index_name_for_remove(table_name, options)
+        def remove_index_for_alter(table_name, column_name = nil, options = {})
+          index_name = index_name_for_remove(table_name, column_name, options)
           "DROP INDEX #{quote_column_name(index_name)}"
         end
 

activerecord/lib/active_record/connection_adapters/postgresql/schema_statements.rb

@@ -448,10 +448,15 @@ def add_index(table_name, column_name, options = {}) #:nodoc:
           end
         end
 
-        def remove_index(table_name, options = {}) #:nodoc:
+        def remove_index(table_name, column_name = nil, options = {}) #:nodoc:
           table = Utils.extract_schema_qualified_name(table_name.to_s)
 
-          if options.is_a?(Hash) && options.key?(:name)
+          if column_name.is_a?(Hash)
+            options = column_name.dup
+            column_name = options.delete(:column)
+          end
+
+          if options.key?(:name)
             provided_index = Utils.extract_schema_qualified_name(options[:name].to_s)
 
             options[:name] = provided_index.identifier
@@ -462,9 +467,9 @@ def remove_index(table_name, options = {}) #:nodoc:
             end
           end
 
-          index_to_remove = PostgreSQL::Name.new(table.schema, index_name_for_remove(table.to_s, options))
+          index_to_remove = PostgreSQL::Name.new(table.schema, index_name_for_remove(table.to_s, column_name, options))
           algorithm =
-            if options.is_a?(Hash) && options.key?(:algorithm)
+            if options.key?(:algorithm)
               index_algorithms.fetch(options[:algorithm]) do
                 raise ArgumentError.new("Algorithm must be one of the following: #{index_algorithms.keys.map(&:inspect).join(', ')}")
               end

activerecord/lib/active_record/connection_adapters/sqlite3_adapter.rb

@@ -218,8 +218,8 @@ def primary_keys(table_name) # :nodoc:
         pks.sort_by { |f| f["pk"] }.map { |f| f["name"] }
       end
 
-      def remove_index(table_name, options = {}) #:nodoc:
-        index_name = index_name_for_remove(table_name, options)
+      def remove_index(table_name, column_name, options = {}) #:nodoc:
+        index_name = index_name_for_remove(table_name, column_name, options)
         exec_query "DROP INDEX #{quote_column_name(index_name)}"
       end
 

activerecord/lib/active_record/migration/command_recorder.rb

@@ -187,16 +187,19 @@ def invert_add_index(args)
         end
 
         def invert_remove_index(args)
-          table, options_or_column = *args
-          if (options = options_or_column).is_a?(Hash)
-            unless options[:column]
-              raise ActiveRecord::IrreversibleMigration, "remove_index is only reversible if given a :column option."
-            end
-            options = options.dup
-            [:add_index, [table, options.delete(:column), options]]
-          elsif (column = options_or_column).present?
-            [:add_index, [table, column]]
+          table, columns, options = *args
+          options ||= {}
+
+          if columns.is_a?(Hash)
+            options = columns.dup
+            columns = options.delete(:column)
           end
+
+          unless columns
+            raise ActiveRecord::IrreversibleMigration, "remove_index is only reversible if given a :column option."
+          end
+
+          [:add_index, [table, columns, options]]
         end
 
         alias :invert_add_belongs_to :invert_add_reference

activerecord/test/cases/adapter_test.rb

@@ -104,6 +104,17 @@ def test_remove_index_when_name_and_wrong_column_name_specified
       @connection.remove_index(:accounts, name: index_name)
     end
 
+    def test_remove_index_when_name_and_wrong_column_name_specified_positional_argument
+      index_name = "accounts_idx"
+
+      @connection.add_index :accounts, :firm_id, name: index_name
+      assert_raises ArgumentError do
+        @connection.remove_index :accounts, :wrong_column_name, name: index_name
+      end
+    ensure
+      @connection.remove_index(:accounts, name: index_name)
+    end
+
     def test_current_database
       if @connection.respond_to?(:current_database)
         assert_equal ARTest.connection_config["arunit"]["database"], @connection.current_database

activerecord/test/cases/migration/change_table_test.rb

@@ -244,8 +244,8 @@ def test_remove_drops_multiple_columns
 
       def test_remove_index_removes_index_with_options
         with_change_table do |t|
-          @connection.expect :remove_index, nil, [:delete_me, { unique: true }]
-          t.remove_index unique: true
+          @connection.expect :remove_index, nil, [:delete_me, :bar, { unique: true }]
+          t.remove_index :bar, unique: true
         end
       end
 

activerecord/test/cases/migration/command_recorder_test.rb

@@ -254,7 +254,12 @@ def test_invert_add_index_with_algorithm_option
 
       def test_invert_remove_index
         add = @recorder.inverse_of :remove_index, [:table, :one]
-        assert_equal [:add_index, [:table, :one]], add
+        assert_equal [:add_index, [:table, :one, {}]], add
+      end
+
+      def test_invert_remove_index_with_positional_column
+        add = @recorder.inverse_of :remove_index, [:table, [:one, :two], { options: true }]
+        assert_equal [:add_index, [:table, [:one, :two], options: true]], add
       end
 
       def test_invert_remove_index_with_column

activerecord/test/cases/migration/index_test.rb

@@ -171,6 +171,9 @@ def test_add_index
         connection.add_index("testings", ["last_name", "first_name"], length: { last_name: 10, first_name: 20 })
         connection.remove_index("testings", ["last_name", "first_name"])
 
+        connection.add_index("testings", "key", unique: true)
+        connection.remove_index("testings", "key", unique: true)
+
         connection.add_index("testings", ["key"], name: "key_idx", unique: true)
         connection.remove_index("testings", name: "key_idx", unique: true)
 

activerecord/test/cases/migration_test.rb

@@ -770,7 +770,7 @@ def test_drop_index_from_table_named_values
 
     assert_nothing_raised do
       connection.add_index :values, :value
-      connection.remove_index :values, column: :value
+      connection.remove_index :values, :value
     end
   ensure
     connection.drop_table :values rescue nil
@@ -786,7 +786,7 @@ def test_drop_index_by_name
 
     assert_nothing_raised do
       connection.add_index :values, :value, name: "a_different_name"
-      connection.remove_index :values, column: :value, name: "a_different_name"
+      connection.remove_index :values, :value, name: "a_different_name"
     end
   ensure
     connection.drop_table :values rescue nil