From e0a315b38aae8cb748597a89786c2249627c4495 Mon Sep 17 00:00:00 2001 From: Ryuta Kamizono Date: Tue, 30 Apr 2019 21:23:03 +0900 Subject: [PATCH] Should take the record's state of first action in the transaction If the same id's records are saved and/or destroyed in the transaction, commit callbackes will only run for the first enrolled record. https://github.com/rails/rails/blob/a023e2180093ebc517a642aaf21f3c7241c67657/activerecord/lib/active_record/connection_adapters/abstract/transaction.rb#L115-L119 The regression #36132 is caused due to #35920 changed the enrollment order that the first action's record will be enrolled to last in the transaction. We could not change the the enrollment order as long as someone depends on the enrollment order. Fixes #36132. --- activerecord/lib/active_record/transactions.rb | 17 +++++++++-------- .../test/cases/transaction_callbacks_test.rb | 2 ++ 2 files changed, 11 insertions(+), 8 deletions(-) diff --git a/activerecord/lib/active_record/transactions.rb b/activerecord/lib/active_record/transactions.rb index bf781b23eb..9e03deede7 100644 --- a/activerecord/lib/active_record/transactions.rb +++ b/activerecord/lib/active_record/transactions.rb @@ -333,7 +333,7 @@ module ActiveRecord # 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 && (destroyed? || persisted?) + if should_run_callbacks && trigger_transactional_callbacks? @_committed_already_called = true _run_commit_without_transaction_enrollment_callbacks _run_commit_callbacks @@ -346,7 +346,7 @@ module ActiveRecord # 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 + if should_run_callbacks && trigger_transactional_callbacks? _run_rollback_callbacks _run_rollback_without_transaction_enrollment_callbacks end @@ -364,7 +364,9 @@ module ActiveRecord def with_transaction_returning_status status = nil self.class.transaction do - unless has_transactional_callbacks? + if has_transactional_callbacks? + add_to_transaction + else sync_with_transaction_state if @transaction_state&.finalized? @transaction_state = self.class.connection.transaction_state end @@ -372,11 +374,6 @@ module ActiveRecord status = yield raise ActiveRecord::Rollback unless status - ensure - if has_transactional_callbacks? && - (@_new_record_before_last_commit && !new_record? || _trigger_update_callback || _trigger_destroy_callback) - add_to_transaction - end end status end @@ -460,6 +457,10 @@ module ActiveRecord 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 diff --git a/activerecord/test/cases/transaction_callbacks_test.rb b/activerecord/test/cases/transaction_callbacks_test.rb index 53fe31e087..135e2cb382 100644 --- a/activerecord/test/cases/transaction_callbacks_test.rb +++ b/activerecord/test/cases/transaction_callbacks_test.rb @@ -36,6 +36,8 @@ class TransactionCallbacksTest < ActiveRecord::TestCase has_many :replies, class_name: "ReplyWithCallbacks", foreign_key: "parent_id" + before_destroy { self.class.find(id).touch if persisted? } + before_commit { |record| record.do_before_commit(nil) } after_commit { |record| record.do_after_commit(nil) } after_save_commit { |record| record.do_after_commit(:save) }