mirror of
https://github.com/rails/rails.git
synced 2022-11-09 12:12:34 -05:00
Fix save
in after_create_commit
won't invoke extra after_create_commit
Since a record is already persisted in `after_create_commit`, so `save` should invoke only `after_update_commit`. This bug is caused by depending on `@_start_transaction_state` for rollback to consider whether it was `new_record` before being committed. If after commit callbacks caused another commit, the state before last commit is no longer `new_record`. Fixes #32831. Closes #18367. Closes #31106.
This commit is contained in:
parent
f7fd680a6d
commit
ae028984d9
2 changed files with 25 additions and 10 deletions
|
@ -328,10 +328,12 @@ module ActiveRecord
|
||||||
# but call it after the commit of a destroyed object.
|
# but call it after the commit of a destroyed object.
|
||||||
def committed!(should_run_callbacks: true) #:nodoc:
|
def committed!(should_run_callbacks: true) #:nodoc:
|
||||||
if should_run_callbacks && (destroyed? || persisted?)
|
if should_run_callbacks && (destroyed? || persisted?)
|
||||||
|
@_committed_already_called = true
|
||||||
_run_commit_without_transaction_enrollment_callbacks
|
_run_commit_without_transaction_enrollment_callbacks
|
||||||
_run_commit_callbacks
|
_run_commit_callbacks
|
||||||
end
|
end
|
||||||
ensure
|
ensure
|
||||||
|
@_committed_already_called = false
|
||||||
force_clear_transaction_record_state
|
force_clear_transaction_record_state
|
||||||
end
|
end
|
||||||
|
|
||||||
|
@ -380,6 +382,7 @@ module ActiveRecord
|
||||||
end
|
end
|
||||||
|
|
||||||
private
|
private
|
||||||
|
attr_reader :_committed_already_called, :_trigger_update_callback, :_trigger_destroy_callback
|
||||||
|
|
||||||
# Save the new record state and id of a record so it can be restored later if a transaction fails.
|
# Save the new record state and id of a record so it can be restored later if a transaction fails.
|
||||||
def remember_transaction_record_state
|
def remember_transaction_record_state
|
||||||
|
@ -390,6 +393,15 @@ module ActiveRecord
|
||||||
frozen?: frozen?,
|
frozen?: frozen?,
|
||||||
)
|
)
|
||||||
@_start_transaction_state[:level] = (@_start_transaction_state[:level] || 0) + 1
|
@_start_transaction_state[:level] = (@_start_transaction_state[:level] || 0) + 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
|
||||||
|
@_new_record_before_last_commit = @_start_transaction_state[:new_record]
|
||||||
|
end
|
||||||
end
|
end
|
||||||
|
|
||||||
# Clear the new record state and id of a record.
|
# Clear the new record state and id of a record.
|
||||||
|
@ -421,22 +433,16 @@ module ActiveRecord
|
||||||
end
|
end
|
||||||
end
|
end
|
||||||
|
|
||||||
# Determine if a record was created or destroyed in a transaction. State should be one of :new_record or :destroyed.
|
|
||||||
def transaction_record_state(state)
|
|
||||||
@_start_transaction_state[state]
|
|
||||||
end
|
|
||||||
|
|
||||||
# Determine if a transaction included an action for :create, :update, or :destroy. Used in filtering callbacks.
|
# Determine if a transaction included an action for :create, :update, or :destroy. Used in filtering callbacks.
|
||||||
def transaction_include_any_action?(actions)
|
def transaction_include_any_action?(actions)
|
||||||
actions.any? do |action|
|
actions.any? do |action|
|
||||||
case action
|
case action
|
||||||
when :create
|
when :create
|
||||||
persisted? && transaction_record_state(:new_record)
|
persisted? && @_new_record_before_last_commit
|
||||||
when :destroy
|
|
||||||
defined?(@_trigger_destroy_callback) && @_trigger_destroy_callback
|
|
||||||
when :update
|
when :update
|
||||||
!(transaction_record_state(:new_record) || destroyed?) &&
|
!(@_new_record_before_last_commit || destroyed?) && _trigger_update_callback
|
||||||
(defined?(@_trigger_update_callback) && @_trigger_update_callback)
|
when :destroy
|
||||||
|
_trigger_destroy_callback
|
||||||
end
|
end
|
||||||
end
|
end
|
||||||
end
|
end
|
||||||
|
|
|
@ -147,6 +147,15 @@ class TransactionCallbacksTest < ActiveRecord::TestCase
|
||||||
assert_equal [:commit_on_destroy], new_record.history
|
assert_equal [:commit_on_destroy], new_record.history
|
||||||
end
|
end
|
||||||
|
|
||||||
|
def test_save_in_after_create_commit_wont_invoke_extra_after_create_commit
|
||||||
|
new_record = TopicWithCallbacks.new(title: "New topic", written_on: Date.today)
|
||||||
|
add_transaction_execution_blocks new_record
|
||||||
|
new_record.after_commit_block(:create) { |r| r.save! }
|
||||||
|
|
||||||
|
new_record.save!
|
||||||
|
assert_equal [:commit_on_create, :commit_on_update], new_record.history
|
||||||
|
end
|
||||||
|
|
||||||
def test_only_call_after_commit_on_create_and_doesnt_leaky
|
def test_only_call_after_commit_on_create_and_doesnt_leaky
|
||||||
r = ReplyWithCallbacks.new(content: "foo")
|
r = ReplyWithCallbacks.new(content: "foo")
|
||||||
r.save_on_after_create = true
|
r.save_on_after_create = true
|
||||||
|
|
Loading…
Reference in a new issue