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
Mutation tracker should be cleared before continuing around callbacks #33689
Conversation
387e006
to
c052662
Compare
Thanks for the pull request, and welcome! The Rails team is excited to review your changes, and you should hear from @eileencodes (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. |
c052662
to
f1e4c15
Compare
`changes_applied` should be called before continuing around callback chain. Otherwise the mutation tracker returns old value for methods like `changed`? or `id_in_database` in around callbacks. Also methods depend on `id_in_database`, like `update_column`, are not working in `around_create` callbacks. ``` class Foo < ActiveRecord::Base around_create :around_create_callback def around_create_callback ... yield p id_in_database # => nil update_column(:generated_column, generate_value) # silently fails end ... end ```
f1e4c15
to
34f075f
Compare
partial_writes? ? super(keys_for_partial_write) : super | ||
affected_rows = partial_writes? ? super(keys_for_partial_write) : super | ||
changes_applied | ||
affected_rows |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm prefer not to care about what kind of value is returned in this module.
Can you use ensure
instead?
diff --git a/activerecord/lib/active_record/attribute_methods/dirty.rb b/activerecord/lib/active_record/attribute_methods/dirty.rb
index 233ee29fac..58613f5e88 100644
--- a/activerecord/lib/active_record/attribute_methods/dirty.rb
+++ b/activerecord/lib/active_record/attribute_methods/dirty.rb
@@ -169,10 +169,14 @@ def write_attribute_without_type_cast(attr_name, _)
def _update_record(*)
partial_writes? ? super(keys_for_partial_write) : super
+ ensure
+ changes_applied
end
def _create_record(*)
partial_writes? ? super(keys_for_partial_write) : super
+ ensure
+ changes_applied
end
def keys_for_partial_write
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hm... I've confirmed using ensure
would change current behavior if any exception is raised from database.
We should not use ensure
at least in this point.
Thank you, and congratulations on your first PR contribution to Rails 🎉 |
@kamipo Should we backport it to |
Yeah, backported in bd6ac44. |
Mutation tracker should be cleared before continuing around callbacks
This is a change in behavior that will cause breakage in apps. We need to go through a deprecation cycle for this the same way we did for |
Scratch that, I see it was a change in behavior between 5.1 and 5.2 |
Summary
changes_applied
should be called before continuing around callback chain. Otherwise the mutation tracker returns old value for methods likechanged
? orid_in_database
in around callbacks. Also methods depend onid_in_database
, likeupdate_column
, are not working inaround_create
callbacks.Other Information
We found this issue with
update_column
called fromaround_create
in our code (our generated value contains id, so we should generate after actual save of record).It was working in Rails 5.1 and not in Rails 5.2.
The bug of
update_column
seems to be emerged by 41484eb, which changes method to fetch primary key fromid
toid_in_database
.Thanks for debugging this issue: @kimihito @elim