Allow `remove_foreign_key` with both `to_table` and `options`

Foreign keys could be created to the same table.
So `remove_foreign_key :from_table, :to_table` is sometimes ambiguous.
This allows `remove_foreign_key` to remove the select one on the same
table with giving both `to_table` and `options`.

7 files changed, 32 insertions, 33 deletions

diff --git a/activerecord/lib/active_record/connection_adapters/abstract/schema_definitions.rb b/activerecord/lib/active_record/connection_adapters/abstract/schema_definitions.rb
index 11d4b4a503..4861872129 100644
--- a/activerecord/lib/active_record/connection_adapters/abstract/schema_definitions.rb
+++ b/activerecord/lib/active_record/connection_adapters/abstract/schema_definitions.rb
@@ -105,13 +105,9 @@ module ActiveRecord
         !ActiveRecord::SchemaDumper.fk_ignore_pattern.match?(name) if name
       end
 
-      def defined_for?(to_table_ord = nil, to_table: nil, **options)
-        if to_table_ord
-          self.to_table == to_table_ord.to_s
-        else
-          (to_table.nil? || to_table.to_s == self.to_table) &&
-            options.all? { |k, v| self.options[k].to_s == v.to_s }
-        end
+      def defined_for?(to_table: nil, **options)
+        (to_table.nil? || to_table.to_s == self.to_table) &&
+          options.all? { |k, v| self.options[k].to_s == v.to_s }
       end
 
       private
diff --git a/activerecord/lib/active_record/connection_adapters/abstract/schema_statements.rb b/activerecord/lib/active_record/connection_adapters/abstract/schema_statements.rb
index b2cc60f363..3dc81ffccf 100644
--- a/activerecord/lib/active_record/connection_adapters/abstract/schema_statements.rb
+++ b/activerecord/lib/active_record/connection_adapters/abstract/schema_statements.rb
@@ -1002,10 +1002,10 @@ module ActiveRecord
       # with an addition of
       # [<tt>:to_table</tt>]
       #   The name of the table that contains the referenced primary key.
-      def remove_foreign_key(from_table, options_or_to_table = {})
+      def remove_foreign_key(from_table, to_table = nil, **options)
         return unless supports_foreign_keys?
 
-        fk_name_to_delete = foreign_key_for!(from_table, options_or_to_table).name
+        fk_name_to_delete = foreign_key_for!(from_table, to_table: to_table, **options).name
 
         at = create_alter_table from_table
         at.drop_foreign_key fk_name_to_delete
@@ -1024,8 +1024,8 @@ module ActiveRecord
       #   # Checks to see if a foreign key with a custom name exists.
       #   foreign_key_exists?(:accounts, name: "special_fk_name")
       #
-      def foreign_key_exists?(from_table, options_or_to_table = {})
-        foreign_key_for(from_table, options_or_to_table).present?
+      def foreign_key_exists?(from_table, to_table = nil, **options)
+        foreign_key_for(from_table, to_table: to_table, **options).present?
       end
 
       def foreign_key_column_for(table_name) # :nodoc:
@@ -1341,14 +1341,14 @@ module ActiveRecord
           end
         end
 
-        def foreign_key_for(from_table, options_or_to_table = {})
+        def foreign_key_for(from_table, **options)
           return unless supports_foreign_keys?
-          foreign_keys(from_table).detect { |fk| fk.defined_for? options_or_to_table }
+          foreign_keys(from_table).detect { |fk| fk.defined_for?(options) }
         end
 
-        def foreign_key_for!(from_table, options_or_to_table = {})
-          foreign_key_for(from_table, options_or_to_table) || \
-            raise(ArgumentError, "Table '#{from_table}' has no foreign key for #{options_or_to_table}")
+        def foreign_key_for!(from_table, to_table: nil, **options)
+          foreign_key_for(from_table, to_table: to_table, **options) ||
+            raise(ArgumentError, "Table '#{from_table}' has no foreign key for #{to_table || options}")
         end
 
         def extract_foreign_key_action(specifier)
diff --git a/activerecord/lib/active_record/connection_adapters/postgresql/schema_statements.rb b/activerecord/lib/active_record/connection_adapters/postgresql/schema_statements.rb
index d694a4f47d..a38c1325c0 100644
--- a/activerecord/lib/active_record/connection_adapters/postgresql/schema_statements.rb
+++ b/activerecord/lib/active_record/connection_adapters/postgresql/schema_statements.rb
@@ -623,10 +623,10 @@ module ActiveRecord
         #   validate_foreign_key :accounts, name: :special_fk_name
         #
         # The +options+ hash accepts the same keys as SchemaStatements#add_foreign_key.
-        def validate_foreign_key(from_table, options_or_to_table = {})
+        def validate_foreign_key(from_table, to_table = nil, **options)
           return unless supports_validate_constraints?
 
-          fk_name_to_validate = foreign_key_for!(from_table, options_or_to_table).name
+          fk_name_to_validate = foreign_key_for!(from_table, to_table: to_table, **options).name
 
           validate_constraint from_table, fk_name_to_validate
         end
diff --git a/activerecord/lib/active_record/connection_adapters/sqlite3/schema_statements.rb b/activerecord/lib/active_record/connection_adapters/sqlite3/schema_statements.rb
index bd649451d6..e64e995e1a 100644
--- a/activerecord/lib/active_record/connection_adapters/sqlite3/schema_statements.rb
+++ b/activerecord/lib/active_record/connection_adapters/sqlite3/schema_statements.rb
@@ -72,7 +72,7 @@ module ActiveRecord
             table = strip_table_name_prefix_and_suffix(table)
             fk_to_table = strip_table_name_prefix_and_suffix(fk.to_table)
             fk_to_table == table && options.all? { |k, v| fk.options[k].to_s == v.to_s }
-          end || raise(ArgumentError, "Table '#{from_table}' has no foreign key for #{to_table}")
+          end || raise(ArgumentError, "Table '#{from_table}' has no foreign key for #{to_table || options}")
 
           foreign_keys.delete(fkey)
           alter_table(from_table, foreign_keys)
diff --git a/activerecord/lib/active_record/migration.rb b/activerecord/lib/active_record/migration.rb
index c20274420f..997b7f763a 100644
--- a/activerecord/lib/active_record/migration.rb
+++ b/activerecord/lib/active_record/migration.rb
@@ -308,7 +308,7 @@ module ActiveRecord
   #   named +column_name+ from the table called +table_name+.
   # * <tt>remove_columns(table_name, *column_names)</tt>: Removes the given
   #   columns from the table definition.
-  # * <tt>remove_foreign_key(from_table, options_or_to_table)</tt>: Removes the
+  # * <tt>remove_foreign_key(from_table, to_table = nil, **options)</tt>: Removes the
   #   given foreign key from the table called +table_name+.
   # * <tt>remove_index(table_name, column: column_names)</tt>: Removes the index
   #   specified by +column_names+.
diff --git a/activerecord/lib/active_record/migration/command_recorder.rb b/activerecord/lib/active_record/migration/command_recorder.rb
index 82f5121d94..8e7f596076 100644
--- a/activerecord/lib/active_record/migration/command_recorder.rb
+++ b/activerecord/lib/active_record/migration/command_recorder.rb
@@ -231,24 +231,15 @@ module ActiveRecord
         end
 
         def invert_remove_foreign_key(args)
-          from_table, options_or_to_table, options_or_nil = args
+          options = args.extract_options!
+          from_table, to_table = args
 
-          to_table = if options_or_to_table.is_a?(Hash)
-            options_or_to_table[:to_table]
-          else
-            options_or_to_table
-          end
-
-          remove_options = if options_or_to_table.is_a?(Hash)
-            options_or_to_table.except(:to_table)
-          else
-            options_or_nil
-          end
+          to_table ||= options.delete(:to_table)
 
           raise ActiveRecord::IrreversibleMigration, "remove_foreign_key is only reversible if given a second table" if to_table.nil?
 
           reversed_args = [from_table, to_table]
-          reversed_args << remove_options if remove_options.present?
+          reversed_args << options unless options.empty?
 
           [:add_foreign_key, reversed_args]
         end
diff --git a/activerecord/test/cases/migration/foreign_key_test.rb b/activerecord/test/cases/migration/foreign_key_test.rb
index 6547ebb5d1..da82f62406 100644
--- a/activerecord/test/cases/migration/foreign_key_test.rb
+++ b/activerecord/test/cases/migration/foreign_key_test.rb
@@ -385,6 +385,18 @@ if ActiveRecord::Base.connection.supports_foreign_keys?
           assert_equal "Table 'astronauts' has no foreign key for rockets", e.message
         end
 
+        def test_remove_foreign_key_by_the_select_one_on_the_same_table
+          @connection.add_foreign_key :astronauts, :rockets
+          @connection.add_reference :astronauts, :myrocket, foreign_key: { to_table: :rockets }
+
+          assert_equal 2, @connection.foreign_keys("astronauts").size
+
+          @connection.remove_foreign_key :astronauts, :rockets, column: "myrocket_id"
+
+          assert_equal [["astronauts", "rockets", "rocket_id"]],
+            @connection.foreign_keys("astronauts").map { |fk| [fk.from_table, fk.to_table, fk.column] }
+        end
+
         if ActiveRecord::Base.connection.supports_validate_constraints?
           def test_add_invalid_foreign_key
             @connection.add_foreign_key :astronauts, :rockets, column: "rocket_id", validate: false