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

save attributes changed by callbacks after update_attribute #27780

Merged

Conversation

@mikelikesbikes
Copy link
Contributor

mikelikesbikes commented Jan 23, 2017

update_attribute stops execution, before running callbacks as part of save, if the record's attributes hadn't changed. The documentation
says that "Callbacks are invoked", which was not happening if the persisted attributes hadn't changed before the before_* callbacks were run.

Here's a test that demonstrates the bug.

@rails-bot

This comment has been minimized.

Copy link

rails-bot commented Jan 23, 2017

Thanks for the pull request, and welcome! The Rails team is excited to review your changes, and you should hear from @rafaelfranca (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

This repository is being automatically checked for code quality issues using Code Climate. You can see results for this analysis in the PR status below. Newly introduced issues should be fixed before a Pull Request is considered ready to review.

Please see the contribution instructions for more information.

@mikelikesbikes

This comment has been minimized.

Copy link
Contributor Author

mikelikesbikes commented Jan 23, 2017

Had some unrelated (I think) broken tests that look like they may be fixed in master.

@arthurnn

This comment has been minimized.

Copy link
Member

arthurnn commented Jan 31, 2017

this was added by @prathamesh-sonpatki , in here 0fcd4cf #18501

Seems like he was solving issue #18400 , So if wonder if removing this would be an regression.

Also, I wonder why the regression test Prathamesh added is not failing
cc @spastorino

@mikelikesbikes

This comment has been minimized.

Copy link
Contributor Author

mikelikesbikes commented Jan 31, 2017

It was my understanding that the code underlying save, only runs the update statement if the record's attributes have changed. So, I think the regression test is passing because the test doesn't cause the attributes to have changed.

What I'm seeing in the app I'm working on is no callbacks run when update_attribute doesn't change one of the persisted attributes. But, the before_save callback in my app is what changes the persisted attribute. We've worked around the issue by using update_attributes instead, BUT if it's the case that callbacks aren't always run when calling update_attribute, the docs need updated to reflect that reality. As it stands, the docs explicitly say that callbacks are called for update_attribute.

@arthurnn

This comment has been minimized.

Copy link
Member

arthurnn commented Jan 31, 2017

BUT if it's the case that callbacks aren't always run when calling update_attribute, the docs need updated to reflect that reality. As it stands, the docs explicitly say that callbacks are called for update_attribute.

I would like to define if this is a docs or a code issue before we make the docs change.
However, I 100% agree that as of right now the docs are wrong.

@mikelikesbikes

This comment has been minimized.

Copy link
Contributor Author

mikelikesbikes commented Jan 31, 2017

I would like to define if this is a docs or a code issue before we make the docs change.

Agreed.

@spastorino

This comment has been minimized.

Copy link
Member

spastorino commented Feb 2, 2017

@arthurnn please do investigate :), to me both requests seems right. Callbacks should be called and running sql updates should be avoided if there's nothing to change.

@prathamesh-sonpatki

This comment has been minimized.

Copy link
Member

prathamesh-sonpatki commented Feb 3, 2017

Callbacks should be called and running sql updates should be avoided if there's nothing to change.

👍 I think @rafaelfranca had also commented into original issue that save method should take care of this. Will investigate.

@prathamesh-sonpatki prathamesh-sonpatki self-assigned this Feb 3, 2017
@sachin21

This comment has been minimized.

Copy link
Contributor

sachin21 commented Feb 3, 2017

Same here. Thank you for the fix. I'm waiting for merging.

@mikelikesbikes

This comment has been minimized.

Copy link
Contributor Author

mikelikesbikes commented Mar 14, 2017

Updated this branch with newest code from master.

@shioyama

This comment has been minimized.

Copy link
Contributor

shioyama commented Sep 15, 2017

👍 Would love to see this merged.

@pwim

This comment has been minimized.

Copy link
Contributor

pwim commented Sep 15, 2017

I'm working with a gem that has a bug due to the extra check that update_attribute currently performs.

The test that was added in #18501 still passes with this PR, so its clear save now handles avoiding extra queries. Behaving differently with regards to callbacks was an unintentional side effect of that PR. So merging this both simplifies the code, and avoids subtle bugs.

@mikelikesbikes

This comment has been minimized.

Copy link
Contributor Author

mikelikesbikes commented Nov 20, 2017

@spastorino @prathamesh-sonpatki any further thoughts this?

@kamipo

This comment has been minimized.

Copy link
Member

kamipo commented Dec 22, 2017

Added the test in #18501 had passed without #18501 changes because UPDATE will not be queried unless attributes changed, and assert_queries only counts normal queries (not included BEGIN and COMMIT).
#18501 will suppress empty BEGIN/COMMIT queries for performance, but it also have wrong side effects that callbacks will not run.
I think that suppressing empty BEGIN/COMMIT should be addressed in the save, not in the update_attribute. Related #17937.
So #18501 should be reverted and backported to fix the regression.

@kamipo
kamipo approved these changes Dec 22, 2017
Copy link
Member

kamipo left a comment

Can you rebase your branch to include only your commit?

update_attribute previously stopped execution, before saving and before
running callbacks, if the record's attributes hadn't changed. [The
documentation](http://api.rubyonrails.org/classes/ActiveRecord/Persistence.html#method-i-update_attribute)
says that "Callbacks are invoked", which was not happening if the
persisted attributes hadn't changed.
@mikelikesbikes mikelikesbikes force-pushed the mikelikesbikes:fix-update-attribute-callbacks-issue branch to c53287b Dec 22, 2017
@mikelikesbikes

This comment has been minimized.

Copy link
Contributor Author

mikelikesbikes commented Dec 22, 2017

@kamipo thanks for the review! I rebased the branch to the current master. Let me know if there's anything else I can address. 😄

@kamipo kamipo merged commit c53287b into rails:master Dec 25, 2017
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 Dec 25, 2017
…llbacks-issue

save attributes changed by callbacks after update_attribute
kamipo added a commit that referenced this pull request Jan 3, 2018
…llbacks-issue

save attributes changed by callbacks after update_attribute
tarebyte added a commit to tarebyte/rails that referenced this pull request May 11, 2018
dsteelma-umd added a commit to dsteelma-umd/student-applications that referenced this pull request Aug 30, 2018
In Rails 5.0.7, the "update_attribute" only actually saves if it detects
a change, which it doesn't seem to do for associations such as
"day_times". This was apparently fixed in Rails 5.2.0
(see rails/rails#27780).

Added a "set_day_times" method that sets the attribute using the setter
and then calls the "save!" method.

https://issues.umd.edu/browse/LIBITD-1220
dsteelma-umd added a commit to dsteelma-umd/student-applications that referenced this pull request Aug 31, 2018
In Rails 5.0.7, the "update_attribute" only actually saves if it detects
a change, which it doesn't seem to do for associations such as
"day_times". This was apparently fixed in Rails 5.2.0
(see rails/rails#27780).

Added a "set_day_times" method that sets the attribute using the setter
and then calls the "save!" method.

https://issues.umd.edu/browse/LIBITD-1231
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
You can’t perform that action at this time.