Merge pull request #20226 from EpicH0liday/reversible-remove-foreign-key

Make remove_foreign_key reversible

Conflicts:
	activerecord/CHANGELOG.md

4 files changed, 50 insertions, 2 deletions

diff --git a/activerecord/CHANGELOG.md b/activerecord/CHANGELOG.md
index bf21851ed0..14906e441b 100644
--- a/activerecord/CHANGELOG.md
+++ b/activerecord/CHANGELOG.md
@@ -1,3 +1,8 @@
+*   Make `remove_foreign_key` reversible. Any foreign key options must be
+    specified, similar to `remove_column`.
+
+    *Aster Ryan*
+
 *   Add `:enum_prefix`/`:enum_suffix` option to `enum` definition.
 
     Fixes #17511 and #17415
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 dd7dcb9cdd..c8be038d76 100644
--- a/activerecord/lib/active_record/connection_adapters/abstract/schema_statements.rb
+++ b/activerecord/lib/active_record/connection_adapters/abstract/schema_statements.rb
@@ -777,7 +777,10 @@ module ActiveRecord
         execute schema_creation.accept(at)
       end
 
-      # Removes the given foreign key from the table.
+      # Removes the given foreign key from the table. Any option parameters provided
+      # will be used to re-add the foreign key in case of a migration rollback.
+      # It is recommended that you provide any options used when creating the foreign
+      # key so that the migration can be reverted properly.
       #
       # Removes the foreign key on +accounts.branch_id+.
       #
@@ -791,6 +794,7 @@ module ActiveRecord
       #
       #   remove_foreign_key :accounts, name: :special_fk_name
       #
+      # The +options+ hash accepts the same keys as SchemaStatements#add_foreign_key.
       def remove_foreign_key(from_table, options_or_to_table = {})
         return unless supports_foreign_keys?
 
diff --git a/activerecord/lib/active_record/migration/command_recorder.rb b/activerecord/lib/active_record/migration/command_recorder.rb
index 36256415df..ee4545ed71 100644
--- a/activerecord/lib/active_record/migration/command_recorder.rb
+++ b/activerecord/lib/active_record/migration/command_recorder.rb
@@ -184,6 +184,16 @@ module ActiveRecord
         [:remove_foreign_key, [from_table, options]]
       end
 
+      def invert_remove_foreign_key(args)
+        from_table, to_table, remove_options = args
+        raise ActiveRecord::IrreversibleMigration, "remove_foreign_key is only reversible if given a second table" if to_table.nil? || to_table.is_a?(Hash)
+
+        reversed_args = [from_table, to_table]
+        reversed_args << remove_options if remove_options
+
+        [:add_foreign_key, reversed_args]
+      end
+
       # Forwards any missing method call to the \target.
       def method_missing(method, *args, &block)
         if @delegate.respond_to?(method)
diff --git a/activerecord/test/cases/migration/command_recorder_test.rb b/activerecord/test/cases/migration/command_recorder_test.rb
index 3844b1a92e..24d3c085a7 100644
--- a/activerecord/test/cases/migration/command_recorder_test.rb
+++ b/activerecord/test/cases/migration/command_recorder_test.rb
@@ -281,17 +281,42 @@ module ActiveRecord
         assert_equal [:remove_foreign_key, [:dogs, :people]], enable
       end
 
+      def test_invert_remove_foreign_key
+        enable = @recorder.inverse_of :remove_foreign_key, [:dogs, :people]
+        assert_equal [:add_foreign_key, [:dogs, :people]], enable
+      end
+
       def test_invert_add_foreign_key_with_column
         enable = @recorder.inverse_of :add_foreign_key, [:dogs, :people, column: "owner_id"]
         assert_equal [:remove_foreign_key, [:dogs, column: "owner_id"]], enable
       end
 
+      def test_invert_remove_foreign_key_with_column
+        enable = @recorder.inverse_of :remove_foreign_key, [:dogs, :people, column: "owner_id"]
+        assert_equal [:add_foreign_key, [:dogs, :people, column: "owner_id"]], enable
+      end
+
       def test_invert_add_foreign_key_with_column_and_name
         enable = @recorder.inverse_of :add_foreign_key, [:dogs, :people, column: "owner_id", name: "fk"]
         assert_equal [:remove_foreign_key, [:dogs, name: "fk"]], enable
       end
 
-      def test_remove_foreign_key_is_irreversible
+      def test_invert_remove_foreign_key_with_column_and_name
+        enable = @recorder.inverse_of :remove_foreign_key, [:dogs, :people, column: "owner_id", name: "fk"]
+        assert_equal [:add_foreign_key, [:dogs, :people, column: "owner_id", name: "fk"]], enable
+      end
+
+      def test_invert_remove_foreign_key_with_primary_key
+        enable = @recorder.inverse_of :remove_foreign_key, [:dogs, :people, primary_key: "person_id"]
+        assert_equal [:add_foreign_key, [:dogs, :people, primary_key: "person_id"]], enable
+      end
+
+      def test_invert_remove_foreign_key_with_on_delete_on_update
+        enable = @recorder.inverse_of :remove_foreign_key, [:dogs, :people, on_delete: :nullify, on_update: :cascade]
+        assert_equal [:add_foreign_key, [:dogs, :people, on_delete: :nullify, on_update: :cascade]], enable
+      end
+
+      def test_invert_remove_foreign_key_is_irreversible_without_to_table
         assert_raises ActiveRecord::IrreversibleMigration do
           @recorder.inverse_of :remove_foreign_key, [:dogs, column: "owner_id"]
         end
@@ -299,6 +324,10 @@ module ActiveRecord
         assert_raises ActiveRecord::IrreversibleMigration do
           @recorder.inverse_of :remove_foreign_key, [:dogs, name: "fk"]
         end
+
+        assert_raises ActiveRecord::IrreversibleMigration do
+          @recorder.inverse_of :remove_foreign_key, [:dogs]
+        end
       end
     end
   end