Skip to content

Commit

Permalink
Fix save in after_create_commit won't invoke extra `after_create_…
Browse files Browse the repository at this point in the history
…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.
  • Loading branch information
kamipo committed Jun 3, 2018
1 parent f7fd680 commit ae02898
Show file tree
Hide file tree
Showing 2 changed files with 25 additions and 10 deletions.
26 changes: 16 additions & 10 deletions activerecord/lib/active_record/transactions.rb
Expand Up @@ -328,10 +328,12 @@ def before_committed! # :nodoc:
# 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


Expand Down Expand Up @@ -380,6 +382,7 @@ def with_transaction_returning_status
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
Expand All @@ -390,6 +393,15 @@ def remember_transaction_record_state
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.
Expand Down Expand Up @@ -421,22 +433,16 @@ def restore_transaction_record_state(force = false)
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
Expand Down
9 changes: 9 additions & 0 deletions activerecord/test/cases/transaction_callbacks_test.rb
Expand Up @@ -147,6 +147,15 @@ def test_only_call_after_commit_on_destroy_after_transaction_commits_for_destroy
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
Expand Down

3 comments on commit ae02898

@eugeneius
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

😍 👏

@vfonic
Copy link
Contributor

@vfonic vfonic commented on ae02898 Jun 4, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've been following this issue, looking at fixing it myself, but never got around to actually doing it.

Thank you! <3

@badosu
Copy link
Contributor

@badosu badosu commented on ae02898 Jun 7, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @kamipo! I gave my try at #32303 but you managed to grok the issue, congrats!

Please sign in to comment.