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:
Ryuta Kamizono 2018-06-04 02:56:52 +09:00
parent f7fd680a6d
commit ae028984d9
2 changed files with 25 additions and 10 deletions

View File

@ -328,10 +328,12 @@ module ActiveRecord
# but call it after the commit of a destroyed object.
def committed!(should_run_callbacks: true) #:nodoc:
if should_run_callbacks && (destroyed? || persisted?)
@_committed_already_called = true
_run_commit_without_transaction_enrollment_callbacks
_run_commit_callbacks
end
ensure
@_committed_already_called = false
force_clear_transaction_record_state
end
@ -380,6 +382,7 @@ module ActiveRecord
end
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.
def remember_transaction_record_state
@ -390,6 +393,15 @@ module ActiveRecord
frozen?: frozen?,
)
@_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
# Clear the new record state and id of a record.
@ -421,22 +433,16 @@ module ActiveRecord
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.
def transaction_include_any_action?(actions)
actions.any? do |action|
case action
when :create
persisted? && transaction_record_state(:new_record)
when :destroy
defined?(@_trigger_destroy_callback) && @_trigger_destroy_callback
persisted? && @_new_record_before_last_commit
when :update
!(transaction_record_state(:new_record) || destroyed?) &&
(defined?(@_trigger_update_callback) && @_trigger_update_callback)
!(@_new_record_before_last_commit || destroyed?) && _trigger_update_callback
when :destroy
_trigger_destroy_callback
end
end
end

View File

@ -147,6 +147,15 @@ class TransactionCallbacksTest < ActiveRecord::TestCase
assert_equal [:commit_on_destroy], new_record.history
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
r = ReplyWithCallbacks.new(content: "foo")
r.save_on_after_create = true