Merge pull request #31604 from fatkodima/reverting-transaction

Fix `transaction` reverting for migrations

7 files changed, 86 insertions, 5 deletions

diff --git a/activerecord/CHANGELOG.md b/activerecord/CHANGELOG.md
index 9acc21510a..7bcf755258 100644
--- a/activerecord/CHANGELOG.md
+++ b/activerecord/CHANGELOG.md
@@ -1,3 +1,10 @@
+*   Fix `transaction` reverting for migrations.
+
+    Before: Commands inside a `transaction` in a reverted migration ran uninverted.
+    Now: This change fixes that by reverting commands inside `transaction` block.
+
+    *fatkodima*, *David Verhasselt*
+
 *   Raise an error instead of scanning the filesystem root when `fixture_path` is blank.
 
     *Gannon McGibbon*, *Max Albrecht*
diff --git a/activerecord/lib/active_record/migration.rb b/activerecord/lib/active_record/migration.rb
index 9651e69edd..d712d4b3cf 100644
--- a/activerecord/lib/active_record/migration.rb
+++ b/activerecord/lib/active_record/migration.rb
@@ -678,15 +678,13 @@ module ActiveRecord
         if connection.respond_to? :revert
           connection.revert { yield }
         else
-          recorder = CommandRecorder.new(connection)
+          recorder = command_recorder
           @connection = recorder
           suppress_messages do
             connection.revert { yield }
           end
           @connection = recorder.delegate
-          recorder.commands.each do |cmd, args, block|
-            send(cmd, *args, &block)
-          end
+          recorder.replay(self)
         end
       end
     end
@@ -962,6 +960,10 @@ module ActiveRecord
           yield
         end
       end
+
+      def command_recorder
+        CommandRecorder.new(connection)
+      end
   end
 
   # MigrationProxy is used to defer loading of the actual migration classes
diff --git a/activerecord/lib/active_record/migration/command_recorder.rb b/activerecord/lib/active_record/migration/command_recorder.rb
index dea6d4ec08..82f5121d94 100644
--- a/activerecord/lib/active_record/migration/command_recorder.rb
+++ b/activerecord/lib/active_record/migration/command_recorder.rb
@@ -108,11 +108,17 @@ module ActiveRecord
         yield delegate.update_table_definition(table_name, self)
       end
 
+      def replay(migration)
+        commands.each do |cmd, args, block|
+          migration.send(cmd, *args, &block)
+        end
+      end
+
       private
 
         module StraightReversions # :nodoc:
           private
-            { transaction:       :transaction,
+            {
               execute_block:     :execute_block,
               create_table:      :drop_table,
               create_join_table: :drop_join_table,
@@ -133,6 +139,17 @@ module ActiveRecord
 
         include StraightReversions
 
+        def invert_transaction(args)
+          sub_recorder = CommandRecorder.new(delegate)
+          sub_recorder.revert { yield }
+
+          invertions_proc = proc {
+            sub_recorder.replay(self)
+          }
+
+          [:transaction, args, invertions_proc]
+        end
+
         def invert_drop_table(args, &block)
           if args.size == 1 && block == nil
             raise ActiveRecord::IrreversibleMigration, "To avoid mistakes, drop_table is only reversible if given options or a block (can be empty)."
diff --git a/activerecord/lib/active_record/migration/compatibility.rb b/activerecord/lib/active_record/migration/compatibility.rb
index 0edaaa0cf9..8f6fcfcaea 100644
--- a/activerecord/lib/active_record/migration/compatibility.rb
+++ b/activerecord/lib/active_record/migration/compatibility.rb
@@ -16,6 +16,21 @@ module ActiveRecord
       V6_0 = Current
 
       class V5_2 < V6_0
+        module CommandRecorder
+          def invert_transaction(args, &block)
+            [:transaction, args, block]
+          end
+        end
+
+        private
+
+          def command_recorder
+            recorder = super
+            class << recorder
+              prepend CommandRecorder
+            end
+            recorder
+          end
       end
 
       class V5_1 < V5_2
diff --git a/activerecord/test/cases/invertible_migration_test.rb b/activerecord/test/cases/invertible_migration_test.rb
index 363beb4780..6cf17ac15d 100644
--- a/activerecord/test/cases/invertible_migration_test.rb
+++ b/activerecord/test/cases/invertible_migration_test.rb
@@ -22,6 +22,14 @@ module ActiveRecord
       end
     end
 
+    class InvertibleTransactionMigration < InvertibleMigration
+      def change
+        transaction do
+          super
+        end
+      end
+    end
+
     class InvertibleRevertMigration < SilentMigration
       def change
         revert do
@@ -271,6 +279,14 @@ module ActiveRecord
       assert_not revert.connection.table_exists?("horses")
     end
 
+    def test_migrate_revert_transaction
+      migration = InvertibleTransactionMigration.new
+      migration.migrate :up
+      assert migration.connection.table_exists?("horses")
+      migration.migrate :down
+      assert_not migration.connection.table_exists?("horses")
+    end
+
     def test_migrate_revert_change_column_default
       migration1 = ChangeColumnDefault1.new
       migration1.migrate(:up)
diff --git a/activerecord/test/cases/migration/command_recorder_test.rb b/activerecord/test/cases/migration/command_recorder_test.rb
index 199818fc90..01f8628fc5 100644
--- a/activerecord/test/cases/migration/command_recorder_test.rb
+++ b/activerecord/test/cases/migration/command_recorder_test.rb
@@ -360,6 +360,16 @@ module ActiveRecord
           @recorder.inverse_of :remove_foreign_key, [:dogs]
         end
       end
+
+      def test_invert_transaction_with_irreversible_inside_is_irreversible
+        assert_raises(ActiveRecord::IrreversibleMigration) do
+          @recorder.revert do
+            @recorder.transaction do
+              @recorder.execute "some sql"
+            end
+          end
+        end
+      end
     end
   end
 end
diff --git a/activerecord/test/cases/migration/compatibility_test.rb b/activerecord/test/cases/migration/compatibility_test.rb
index 69a50674af..017ee7951e 100644
--- a/activerecord/test/cases/migration/compatibility_test.rb
+++ b/activerecord/test/cases/migration/compatibility_test.rb
@@ -127,6 +127,20 @@ module ActiveRecord
         assert_match(/LegacyMigration < ActiveRecord::Migration\[4\.2\]/, e.message)
       end
 
+      def test_legacy_migrations_not_raise_exception_on_reverting_transaction
+        migration = Class.new(ActiveRecord::Migration[5.2]) {
+          def change
+            transaction do
+              execute "select 1"
+            end
+          end
+        }.new
+
+        assert_nothing_raised do
+          migration.migrate(:down)
+        end
+      end
+
       if current_adapter?(:PostgreSQLAdapter)
         class Testing < ActiveRecord::Base
         end