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

Clarify behavior of touch in transactions with callbacks in RDoc [ci skip] #49833

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

dhavalsingh
Copy link

Motivation / Background

This Pull Request has been created because touch is used in a lot of places but it's interaction is not documented anywhere wrt transactions. Bot how the queries are coalesced and the order in which callbacks are fired can be very confusing if not mentioned.

Detail

This Pull Request changes the Rdoc of touch.

Additional information

#18606 coalesce PR
https://gist.github.com/dhavalsingh/0b353121345b1b74f6d16a41afb1bc5e Github gist for how callbacks are executed with touch and transaction blocks.

Checklist

Before submitting the PR make sure the following are checked:

  • [ x] This Pull Request is related to one change. Changes that are unrelated should be opened in separate PRs.
  • [x ] Commit message has a detailed description of what changed and why. If this PR fixes a related issue include it in the commit message. Ex: [Fix #issue-number]
  • [x ] Tests are added or updated if you fix a bug or add a feature.
  • [x ] CHANGELOG files are updated for the changed libraries if there is a behavior change or additional feature. Minor bug fixes and documentation changes should not be included.

@dhavalsingh
Copy link
Author

Can someone add the docs label to this?

@zzak zzak added the docs label Oct 30, 2023
@dhavalsingh
Copy link
Author

Can someone review this? Should I use a different example or make some changes?
or is this something that is not worth putting in the API docs?

@dhavalsingh
Copy link
Author

bumping this for reviews

Copy link
Contributor

@ignacio-chiazzo ignacio-chiazzo left a comment

Choose a reason for hiding this comment

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

I like this! I was having the same issue and realized the behaviour through tests (and looking at the original PR).

Comment on lines +1144 to +1150
# ActiveRecord::Base.transaction do
# Brake.find(1).update(wear: 'moderate') # Assuming this brake belongs to car with id 1
# Brake.find(2).update(wear: 'low') # Assuming this brake also belongs to car with id 1
# # The 'touch: true' on the car is triggered by both brake updates,
# # but the actual SQL update and the 'log_update' callback execution for the car
# # are deferred until the transaction commits.
# end
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
# ActiveRecord::Base.transaction do
# Brake.find(1).update(wear: 'moderate') # Assuming this brake belongs to car with id 1
# Brake.find(2).update(wear: 'low') # Assuming this brake also belongs to car with id 1
# # The 'touch: true' on the car is triggered by both brake updates,
# # but the actual SQL update and the 'log_update' callback execution for the car
# # are deferred until the transaction commits.
# end
# car = Car.find(1)
# break_1, break_2 = car.breaks.first(2)
# ActiveRecord::Base.transaction do
# break_1.update(wear: 'moderate')
# break_2.update(wear: 'low')
# # The 'touch: true' on the car is only executed once at the end of the transaction.
# end

Comment on lines +1152 to +1155
# After the transaction commits, the +after_commit+ callbacks associated with the touch operations are executed.
# Due to the coalescing of SQL commands in the transaction, the "Car #{id} was updated at #{updated_at}" log message
# from the Car's +after_commit+ callback is printed only once. In the context of the example, this callback is
# triggered after the update of Brake with id 1 and before the update of Brake with id 2 within the same transaction.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
# After the transaction commits, the +after_commit+ callbacks associated with the touch operations are executed.
# Due to the coalescing of SQL commands in the transaction, the "Car #{id} was updated at #{updated_at}" log message
# from the Car's +after_commit+ callback is printed only once. In the context of the example, this callback is
# triggered after the update of Brake with id 1 and before the update of Brake with id 2 within the same transaction.
# After the transaction commits, the +after_commit+ callbacks associated with the touch operations are executed.
# Due to the coalescing of SQL commands in the transaction, the `log_update` function
# from the Car's +after_commit+ callback is called only once. In the context of the example, this callback is
# triggered after the update of `break_1` and before the update of `break_2` within the same transaction.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants