Merge pull request #36190 from kamipo/should_attempt_tx_callbacks_to_all_records

Should attempt `committed!`/`rolledback!` to all enrolled records in the transaction

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

@@ -98,9 +98,13 @@ def materialized?
       end
 
       def rollback_records
-        ite = records.uniq
+        ite = records.uniq(&:object_id)
+        already_run_callbacks = {}
         while record = ite.shift
-          record.rolledback!(force_restore_state: full_rollback?)
+          trigger_callbacks = record.trigger_transactional_callbacks?
+          should_run_callbacks = !already_run_callbacks[record] && trigger_callbacks
+          already_run_callbacks[record] ||= trigger_callbacks
+          record.rolledback!(force_restore_state: full_rollback?, should_run_callbacks: should_run_callbacks)
         end
       ensure
         ite.each do |i|
@@ -113,10 +117,14 @@ def before_commit_records
       end
 
       def commit_records
-        ite = records.uniq
+        ite = records.uniq(&:object_id)
+        already_run_callbacks = {}
         while record = ite.shift
           if @run_commit_callbacks
-            record.committed!
+            trigger_callbacks = record.trigger_transactional_callbacks?
+            should_run_callbacks = !already_run_callbacks[record] && trigger_callbacks
+            already_run_callbacks[record] ||= trigger_callbacks
+            record.committed!(should_run_callbacks: should_run_callbacks)
           else
             # if not running callbacks, only adds the record to the parent transaction
             connection.add_transaction_record(record)

activerecord/lib/active_record/touch_later.rb

@@ -23,6 +23,7 @@ def touch_later(*names) # :nodoc:
 
       surreptitiously_touch @_defer_touch_attrs
       add_to_transaction
+      @_new_record_before_last_commit ||= false
 
       # touch the parents as we are not calling the after_save callbacks
       self.class.reflect_on_all_associations(:belongs_to).each do |r|

activerecord/lib/active_record/transactions.rb

@@ -333,7 +333,7 @@ def before_committed! # :nodoc:
     # Ensure that it is not called if the object was never persisted (failed create),
     # but call it after the commit of a destroyed object.
     def committed!(should_run_callbacks: true) #:nodoc:
-      if should_run_callbacks && trigger_transactional_callbacks?
+      if should_run_callbacks
         @_committed_already_called = true
         _run_commit_without_transaction_enrollment_callbacks
         _run_commit_callbacks
@@ -346,7 +346,7 @@ def committed!(should_run_callbacks: true) #:nodoc:
     # Call the #after_rollback callbacks. The +force_restore_state+ argument indicates if the record
     # state should be rolled back to the beginning or just to the last savepoint.
     def rolledback!(force_restore_state: false, should_run_callbacks: true) #:nodoc:
-      if should_run_callbacks && trigger_transactional_callbacks?
+      if should_run_callbacks
         _run_rollback_callbacks
         _run_rollback_without_transaction_enrollment_callbacks
       end
@@ -378,6 +378,11 @@ def with_transaction_returning_status
       status
     end
 
+    def trigger_transactional_callbacks? # :nodoc:
+      (@_new_record_before_last_commit || _trigger_update_callback) && persisted? ||
+        _trigger_destroy_callback && destroyed?
+    end
+
     private
       attr_reader :_committed_already_called, :_trigger_update_callback, :_trigger_destroy_callback
 
@@ -392,10 +397,7 @@ def remember_transaction_record_state
           level: 0
         }
         @_start_transaction_state[:level] += 1
-        remember_new_record_before_last_commit
-      end
 
-      def remember_new_record_before_last_commit
         if _committed_already_called
           @_new_record_before_last_commit = false
         else
@@ -457,10 +459,6 @@ def add_to_transaction
         self.class.connection.add_transaction_record(self)
       end
 
-      def trigger_transactional_callbacks?
-        @_new_record_before_last_commit && !new_record? || _trigger_update_callback || _trigger_destroy_callback
-      end
-
       def has_transactional_callbacks?
         !_rollback_callbacks.empty? || !_commit_callbacks.empty? || !_before_commit_callbacks.empty?
       end

activerecord/test/cases/transaction_callbacks_test.rb

@@ -551,6 +551,8 @@ def test_before_commit_update_in_same_transaction
 end
 
 class CallbacksOnDestroyUpdateActionRaceTest < ActiveRecord::TestCase
+  self.use_transactional_tests = false
+
   class TopicWithHistory < ActiveRecord::Base
     self.table_name = :topics
 
@@ -564,11 +566,22 @@ def self.history
   end
 
   class TopicWithCallbacksOnDestroy < TopicWithHistory
-    after_commit(on: :destroy) { |record| record.class.history << :destroy }
+    after_commit(on: :destroy) { |record| record.class.history << :commit_on_destroy }
+    after_rollback(on: :destroy) { |record| record.class.history << :rollback_on_destroy }
+
+    before_destroy :before_destroy_for_transaction
+
+    private
+      def before_destroy_for_transaction; end
   end
 
   class TopicWithCallbacksOnUpdate < TopicWithHistory
-    after_commit(on: :update) { |record| record.class.history << :update }
+    after_commit(on: :update) { |record| record.class.history << :commit_on_update }
+
+    before_save :before_save_for_transaction
+
+    private
+      def before_save_for_transaction; end
   end
 
   def test_trigger_once_on_multiple_deletions
@@ -576,10 +589,39 @@ def test_trigger_once_on_multiple_deletions
     topic = TopicWithCallbacksOnDestroy.new
     topic.save
     topic_clone = TopicWithCallbacksOnDestroy.find(topic.id)
+
+    topic.define_singleton_method(:before_destroy_for_transaction) do
+      topic_clone.destroy
+    end
+
     topic.destroy
-    topic_clone.destroy
 
-    assert_equal [:destroy], TopicWithCallbacksOnDestroy.history
+    assert_equal [:commit_on_destroy], TopicWithCallbacksOnDestroy.history
+  end
+
+  def test_rollback_on_multiple_deletions
+    TopicWithCallbacksOnDestroy.clear_history
+    topic = TopicWithCallbacksOnDestroy.new
+    topic.save
+    topic_clone = TopicWithCallbacksOnDestroy.find(topic.id)
+
+    topic.define_singleton_method(:before_destroy_for_transaction) do
+      topic_clone.update!(author_name: "Test Author Clone")
+      topic_clone.destroy
+    end
+
+    TopicWithCallbacksOnDestroy.transaction do
+      topic.update!(author_name: "Test Author")
+      topic.destroy
+      raise ActiveRecord::Rollback
+    end
+
+    assert_not_predicate topic, :destroyed?
+    assert_not_predicate topic_clone, :destroyed?
+    assert_equal [nil, "Test Author"], topic.author_name_change_to_be_saved
+    assert_equal [nil, "Test Author Clone"], topic_clone.author_name_change_to_be_saved
+
+    assert_equal [:rollback_on_destroy], TopicWithCallbacksOnDestroy.history
   end
 
   def test_trigger_on_update_where_row_was_deleted
@@ -587,7 +629,11 @@ def test_trigger_on_update_where_row_was_deleted
     topic = TopicWithCallbacksOnUpdate.new
     topic.save
     topic_clone = TopicWithCallbacksOnUpdate.find(topic.id)
-    topic.destroy
+
+    topic_clone.define_singleton_method(:before_save_for_transaction) do
+      topic.destroy
+    end
+
     topic_clone.author_name = "Test Author"
     topic_clone.save