Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Finalize transaction record state after real transaction #32911

Merged
merged 1 commit into from May 19, 2018

Conversation

@eugeneius
Copy link
Member

@eugeneius eugeneius commented May 16, 2018

Fixes #32847.
Fixes #22066.

After a real (non-savepoint) transaction has committed or rolled back, the original persistence-related state for all records modified in that transaction is discarded or restored, respectively.

When the model has transactional callbacks, this happens synchronously in the committed! or rolled_back! methods; otherwise, it happens lazily the next time the record's persistence-related state is accessed.

The synchronous code path always finalizes the state of the record, but the lazy code path only pops one "level" from the transaction counter, assuming it will always reach zero immediately after a real transaction. As the test cases included here demonstrate, that isn't always the case.

By using the same logic as the synchronous code path, we ensure that the record's state is always updated after a real transaction has finished.

@rails-bot
Copy link

@rails-bot rails-bot commented May 16, 2018

r? @sgrif

(@rails-bot has picked a reviewer for you, use r? to override)

raise ActiveRecord::Rollback
end

assert_predicate topic, :persisted?

This comment has been minimized.

@kamipo

kamipo May 18, 2018
Member

What does this test? This test case will pass without this PR.

This comment has been minimized.

@eugeneius

eugeneius May 19, 2018
Author Member

If the parts of this PR related to rollbacks are applied on their own, this test will start to fail; in other words, it demonstrates that this new line is necessary.

Alternatively, this test will fail on master if the final assertion is duplicated.

This comment has been minimized.

@kamipo

kamipo May 19, 2018
Member

I see. How about adding one extra assert_not_predicate topic, :new_record? then?

This comment has been minimized.

@eugeneius

eugeneius May 19, 2018
Author Member

👍 done!

After a real (non-savepoint) transaction has committed or rolled back,
the original persistence-related state for all records modified in that
transaction is discarded or restored, respectively.

When the model has transactional callbacks, this happens synchronously
in the `committed!` or `rolled_back!` methods; otherwise, it happens
lazily the next time the record's persistence-related state is accessed.

The synchronous code path always finalizes the state of the record, but
the lazy code path only pops one "level" from the transaction counter,
assuming it will always reach zero immediately after a real transaction.
As the test cases included here demonstrate, that isn't always the case.

By using the same logic as the synchronous code path, we ensure that the
record's state is always updated after a real transaction has finished.
@eugeneius eugeneius force-pushed the eugeneius:finalize_transaction_record_state branch to 5359428 May 19, 2018
@kamipo kamipo merged commit 8707660 into rails:master May 19, 2018
2 checks passed
2 checks passed
codeclimate All good!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
kamipo added a commit that referenced this pull request May 23, 2018
…state

Finalize transaction record state after real transaction
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants