mirror of
https://github.com/rails/rails.git
synced 2022-11-09 12:12:34 -05:00
Restore ActiveRecord states after a rollback for models w/o callbacks
This fixes a regression (#13744) that was caused by67d8bb9
. In67d8bb9
, we introduced lazy rollback for records, such that the record's internal states and attributes are not restored immediately after a transaction rollback, but deferred until they are first accessed. This optimization is only performed when the model does not have any transactional callbacks (e.g. `after_commit` and `after_create`). Unfortunately, the models used to test the affected codepaths all comes with some sort of transactional callbacks. Therefore this codepath remains largely untested until now and as a result there are a few issues in the implementation that remains hidden until now. First, the `sync_with_transaction_state` (or more accurately, `update_attributes_from_transaction_state`) would perform the synchronization prematurely before a transaction is finalized (i.e. comitted or rolled back). As a result, when the actuall rollback happens, the record will incorrectly assumes that its internal states match the transaction state, and neglect to perform the restore. Second, `update_attributes_from_transaction_state` calls `committed!` in some cases. This in turns checks for the `destroyed?` state which also requires synchronization with the transaction stae, which causes an infnite recurrsion. This fix works by deferring the synchronization until the transaction has been finalized (addressing the first point), and also unrolled the `committed!` and `rolledback!` logic in-place (addressing the second point). It should be noted that the primary purpose of the `committed!` and `rolledback!` methods are to trigger the relevant transactional callbacks. Since this code path is only entered when there are no transactional callbacks on the model, this shouldn't be necessary. By unrolling the method calls, the intention here (to restore the states when necessary) becomes more clear.
This commit is contained in:
parent
35b85d2e7d
commit
7386ffc781
4 changed files with 26 additions and 6 deletions
|
@ -1,3 +1,11 @@
|
|||
* ActiveRecord states are now correctly restored after a rollback for
|
||||
models that did not define any transactional callbacks (i.e.
|
||||
`after_commit`, `after_rollback` or `after_create`).
|
||||
|
||||
Fixes #13744.
|
||||
|
||||
*Godfrey Chan*
|
||||
|
||||
* Make `touch` fire the `after_commit` and `after_rollback` callbacks.
|
||||
|
||||
*Harry Brundage*
|
||||
|
|
|
@ -23,6 +23,10 @@ module ActiveRecord
|
|||
@parent = nil
|
||||
end
|
||||
|
||||
def finalized?
|
||||
@state
|
||||
end
|
||||
|
||||
def committed?
|
||||
@state == :committed
|
||||
end
|
||||
|
|
|
@ -397,13 +397,10 @@ module ActiveRecord
|
|||
end
|
||||
|
||||
def update_attributes_from_transaction_state(transaction_state, depth)
|
||||
if transaction_state && !has_transactional_callbacks?
|
||||
if transaction_state && transaction_state.finalized? && !has_transactional_callbacks?
|
||||
unless @reflects_state[depth]
|
||||
if transaction_state.committed?
|
||||
committed!
|
||||
elsif transaction_state.rolledback?
|
||||
rolledback!
|
||||
end
|
||||
restore_transaction_record_state if transaction_state.rolledback?
|
||||
clear_transaction_record_state
|
||||
@reflects_state[depth] = true
|
||||
end
|
||||
|
||||
|
|
|
@ -430,17 +430,26 @@ class TransactionTest < ActiveRecord::TestCase
|
|||
end
|
||||
|
||||
def test_restore_active_record_state_for_all_records_in_a_transaction
|
||||
topic_without_callbacks = Class.new(ActiveRecord::Base) do
|
||||
self.table_name = 'topics'
|
||||
end
|
||||
|
||||
topic_1 = Topic.new(:title => 'test_1')
|
||||
topic_2 = Topic.new(:title => 'test_2')
|
||||
topic_3 = topic_without_callbacks.new(:title => 'test_3')
|
||||
|
||||
Topic.transaction do
|
||||
assert topic_1.save
|
||||
assert topic_2.save
|
||||
assert topic_3.save
|
||||
@first.save
|
||||
@second.destroy
|
||||
assert topic_1.persisted?, 'persisted'
|
||||
assert_not_nil topic_1.id
|
||||
assert topic_2.persisted?, 'persisted'
|
||||
assert_not_nil topic_2.id
|
||||
assert topic_3.persisted?, 'persisted'
|
||||
assert_not_nil topic_3.id
|
||||
assert @first.persisted?, 'persisted'
|
||||
assert_not_nil @first.id
|
||||
assert @second.destroyed?, 'destroyed'
|
||||
|
@ -451,6 +460,8 @@ class TransactionTest < ActiveRecord::TestCase
|
|||
assert_nil topic_1.id
|
||||
assert !topic_2.persisted?, 'not persisted'
|
||||
assert_nil topic_2.id
|
||||
assert !topic_3.persisted?, 'not persisted'
|
||||
assert_nil topic_3.id
|
||||
assert @first.persisted?, 'persisted'
|
||||
assert_not_nil @first.id
|
||||
assert !@second.destroyed?, 'not destroyed'
|
||||
|
|
Loading…
Reference in a new issue