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

Deprecate the behavior of AR::Dirty inside of after_(create|update|save) callbacks #25337

Merged
merged 1 commit into from Nov 1, 2016

Conversation

@sgrif
Copy link
Contributor

sgrif commented Jun 9, 2016

We pretty frequently get bug reports that "dirty is broken inside of after callbacks". Intuitively they are correct. You'd expect Model.after_save { puts changed? }; model.save to do the same thing as model.save; puts model.changed?, but it does not.

However, changing this goes much farther than just making the behavior more intuitive. There are a ton of places inside of AR that can be drastically simplified with this change. Specifically, autosave associations, timestamps, touch, counter cache, and just about anything else in AR that works with callbacks have code to try to avoid "double save" bugs which we will be able to flat out remove with this change.

We introduce two new sets of methods, both with names that are meant to be more explicit than dirty. The first set maintains the old behavior, and their names are meant to center that they are about changes that occurred during the save that just happened. They are equivalent to previous_changes when called outside of after callbacks, or once the deprecation cycle moves.

The second set is the new behavior. Their names imply that they are talking about changes from the database representation. The fact that this is what we really care about became clear when looking at BelongsTo.touch_record when tests were failing. I'm unsure that this set of methods should be in the public API. Outside of after callbacks, they are equivalent to the existing methods on dirty.

I am not married to any of the method names. Please bikeshed the shit out of them, I am open to alternatives.

Dirty itself is not deprecated, nor are the methods inside of it. They will only emit the warning when called inside of after callbacks. The scope of this breakage is pretty large, but the migration path is simple. Given how much this can improve our codebase, and considering that it makes our API more intuitive, I think it's worth doing.

Unresolved questions

Do we want the "new behavior" methods to be in the public API at all? They are straight aliases to the existing methods in dirty after 5.2/6.0. However, since we get these bug reports, someone probably does want the new behavior today.

Still left todo

I need to improve the commit messages, and move the deprecation warning up to the caller of mutation_tracker to include the exact method to call instead. The current implementation is also emitting deprecation warnings in places where the calls are valid. This will be fixed by moving the warning to the right place.

@sgrif
Copy link
Contributor Author

sgrif commented Jun 9, 2016

/cc @matthewd @rafaelfranca This is not ready to merge, but the remaining changes I have to make are to commit messages and where the deprecation warning is emitted, which shouldn't affect the discussion around this change.

end

# Alias for `changed?`
def has_changes_to_save?

This comment has been minimized.

Copy link
@olivierlacan

olivierlacan Jun 9, 2016

Contributor

Wouldn't changes_to_be_saved? be more coherent with attribute_change_to_be_saved?

This comment has been minimized.

Copy link
@sgrif

sgrif Jun 13, 2016

Author Contributor

The goal was to make the difference between some of the methods more clear. Do you think that model.changes_to_be_saved? reads clearly enough? Compared to model.has_changes_to_save?

end

# Alias for `changed_attributes`
def attributes_in_database

This comment has been minimized.

Copy link
@olivierlacan

olivierlacan Jun 9, 2016

Contributor

persisted_attributes seems a bit more straightforward as a method name, no?

This comment has been minimized.

Copy link
@sgrif

sgrif Jun 13, 2016

Author Contributor

I'm not happy with either one. Both names imply to me that they would return the opposite of changed (e.g. return an array of all of the attribute names which are not changed)

# Behaves similarly to +attribute_was+. This method is useful in after
# callbacks to get the original value of an attribute before the save that
# just occurred
def attribute_before_last_save(attr_name)

This comment has been minimized.

Copy link
@olivierlacan

olivierlacan Jun 9, 2016

Contributor

Will this return the attribute before the last save or the attribute before the last change?

This comment has been minimized.

Copy link
@sgrif

sgrif Jun 13, 2016

Author Contributor

Neither? It will return the value of the attribute in the database before the most recent save. This could definitely be more clear in the method name. Not sure how to express it without going full cocoa

@olivierlacan
Copy link
Contributor

olivierlacan commented Jun 10, 2016

I feel like I need to wrap my mind around the name changes in the public API outside of the code to fully focus on their semantics (I hope that's not weird and distracting from the PR convo). I hope this helps.

Legend:

  • => in the meaning column means there's a non-boolean return value
  • empty columns in Prior Name mean the method didn't exist before
  • * before a new method name means it's an alias to the prior method
Prior Name New Name Meaning
attribute_was(attr_name) *attribute_in_database(attr_name) current database value of attribute
attribute_change(attr_name) *attribute_change_to_be_saved => the changed value for attribute
attribute_changed?(attr_name) *will_save_change_to_attribute?(attr_name) was attribute modified since last save
attribute_before_last_save(attr_name) => attribute value from before last save
changed *changed_attribute_names_to_save => name of attributes with changed values since last save
changes *changes_to_save => the changes to be saved to DB
changed? *has_changes_to_save? are there any changes to be saved to the DB?
changed_attributes *attributes_in_database => Hash of attribute names & new unsaved values
changed? has_changes_to_save? record has unsaved changes
saved_changes? did last save change record attributes?
saved_changes => Hash of all changes in last save
saved_change_to_attribute?(attr_name) did attribute change when record last saved?
saved_change_to_attribute(attr_name) => array of original & saved value from last save

@sgrif Please feel free to edit the above to correct any incorrect meaning.

@nateberkopec
Copy link
Contributor

nateberkopec commented Jun 10, 2016

Are those aliases intended to be public? Wasn't sure. If they are public, I agree, they were sort of hard to wrap my head around.

…ve) callbacks

We pretty frequently get bug reports that "dirty is broken inside of
after callbacks". Intuitively they are correct. You'd expect
`Model.after_save { puts changed? }; model.save` to do the same thing as
`model.save; puts model.changed?`, but it does not.

However, changing this goes much farther than just making the behavior
more intuitive. There are a _ton_ of places inside of AR that can be
drastically simplified with this change. Specifically, autosave
associations, timestamps, touch, counter cache, and just about anything
else in AR that works with callbacks have code to try to avoid "double
save" bugs which we will be able to flat out remove with this change.

We introduce two new sets of methods, both with names that are meant to
be more explicit than dirty. The first set maintains the old behavior,
and their names are meant to center that they are about changes that
occurred during the save that just happened. They are equivalent to
`previous_changes` when called outside of after callbacks, or once the
deprecation cycle moves.

The second set is the new behavior. Their names imply that they are
talking about changes from the database representation. The fact that
this is what we really care about became clear when looking at
`BelongsTo.touch_record` when tests were failing. I'm unsure that this
set of methods should be in the public API. Outside of after callbacks,
they are equivalent to the existing methods on dirty.

Dirty itself is not deprecated, nor are the methods inside of it. They
will only emit the warning when called inside of after callbacks. The
scope of this breakage is pretty large, but the migration path is
simple. Given how much this can improve our codebase, and considering
that it makes our API more intuitive, I think it's worth doing.
@sgrif sgrif force-pushed the sgrif:sg-changes-in-callbacks branch to 16ae3db Nov 1, 2016
@sgrif
Copy link
Contributor Author

sgrif commented Nov 1, 2016

Going to move forward with this change. If anyone has suggested improvements to the names of the new methods, please open a PR.

@sgrif sgrif merged commit 29b3b5d into rails:master Nov 1, 2016
0 of 2 checks passed
0 of 2 checks passed
codeclimate Code Climate is analyzing this code.
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
@sgrif sgrif deleted the sgrif:sg-changes-in-callbacks branch Nov 1, 2016
sgrif added a commit that referenced this pull request Nov 1, 2016
With the changes in #25337, double save bugs are pretty much impossible,
so we can just lift this restriction with pretty much no change. There
were a handful of cases where we were relying on specific quirks in
tests that had to be updated. The change to has_one associations was due
to a particularly interesting test where an autosaved has_one
association was replaced with a new child, where the child failed to
save but the test wanted to check that the parent id persisted to `nil`.

I think this is almost certainly the wrong behavior, and I may change
that behavior later. But ultimately the root cause was because we never
remove the parent in memory when nullifying the child. This makes #23197
no longer needed, but it is what we'll do to fix some issues on 5.0

Close #23197
@dhh

This comment has been minimized.

Copy link
Member

dhh commented on 16ae3db Dec 14, 2016

If you have a before_action callback that uses changes/attribute_changed?, then it'll emit a warning if that's triggered twice in a transaction, like on double save. Here's a failing test:

  test "changed? in before callback thats run twice in a transaction should not be deprecated" do
    klass = Class.new(ActiveRecord::Base) do
      self.table_name = "people"
      @@save_counter = 0

      before_update do
        first_name_changed?
      end

      after_update do
        @@save_counter = @@save_counter + 1
        save if @@save_counter < 2
      end
    end

    assert_not_deprecated do
      person = klass.create!(first_name: "Sean")
      person.update! first_name: 'Blue!'
    end
  end

This comment has been minimized.

Copy link
Member

dhh replied Dec 14, 2016

I guess this isn't a big deal if people are just encouraged to switch to the specific checks against changes pending save / changes that have been saved. But don't feel like we're currently doing the best job of guiding in that direction.

This comment has been minimized.

Copy link
Contributor

Schwad replied May 23, 2018

Just wanted to say @sgrif thanks for the discussion here, currently implementing this as a part of my 5.2 upgrade. I couldn't find places in the 5.1/5.2 Changelog/README updates where this was discussed or documented, is this commit considered the official documentation on this change?

This comment has been minimized.

Copy link

jbielick replied Oct 16, 2019

Did the deprecation warning and the removal/change occur at the same time? This caught us completely off guard.

@krtschmr
Copy link

krtschmr commented Apr 19, 2017

@sgrif what happened to attribute_changed? like after_save :analyze_file_async, if: :file_changed?

i can't figure the new naming. this looks ugly to me though: if: ->{ saved_change_to_attribute?(:file?) }

why we don't make if : :file_was_updated? so we make

def #{attribute}_was_updated?
  saved_change_to_attribute?(attr)
end

then we can use it `after_save :analyze_file_async, if: :file_was_updated?

@dsandstrom
Copy link

dsandstrom commented May 19, 2017

Are the new method names documented anywhere besides here? Maybe update http://api.rubyonrails.org/classes/ActiveModel/Dirty.html .

@feliperaul
Copy link

feliperaul commented May 24, 2018

@vlymar I agree, this is so far our hugest pain point in upgrading from Rails 4.2 to Rails 5.2

@feliperaul
Copy link

feliperaul commented May 24, 2018

Just want to point that this blog posts was the best resource I could find about theses changes, and it's a must-read for now specially considering that, as far as I could tell, the changelogs and upgrade guides didn't mention this huge breaking change anywhere :/

Update: for some reason, when googling for ActiveRecord Dirty I'm taken to the ActiveModel::Dirty API page on the docs, and there's no sign (up until page 5 of google results) of the ActiveRecord::AttributeMethods::Dirty, which has different methods and docs: http://api.rubyonrails.org/v5.2.0/classes/ActiveRecord/AttributeMethods/Dirty.html

@pedrofurtado
Copy link

pedrofurtado commented Mar 1, 2019

I feel like I need to wrap my mind around the name changes in the public API outside of the code to fully focus on their semantics (I hope that's not weird and distracting from the PR convo). I hope this helps.

Legend:

  • => in the meaning column means there's a non-boolean return value
  • empty columns in Prior Name mean the method didn't exist before
  • * before a new method name means it's an alias to the prior method

Prior Name New Name Meaning
attribute_was(attr_name) *attribute_in_database(attr_name) current database value of attribute
attribute_change(attr_name) *attribute_change_to_be_saved => the changed value for attribute
attribute_changed?(attr_name) *will_save_change_to_attribute?(attr_name) was attribute modified since last save
attribute_before_last_save(attr_name) => attribute value from before last save
changed *changed_attribute_names_to_save => name of attributes with changed values since last save
changes *changes_to_save => the changes to be saved to DB
changed? *has_changes_to_save? are there any changes to be saved to the DB?
changed_attributes *attributes_in_database => Hash of attribute names & new unsaved values
changed? has_changes_to_save? record has unsaved changes
saved_changes? did last save change record attributes?
saved_changes => Hash of all changes in last save
saved_change_to_attribute?(attr_name) did attribute change when record last saved?
saved_change_to_attribute(attr_name) => array of original & saved value from last save
@sgrif Please feel free to edit the above to correct any incorrect meaning.

That table definitely MUST be present in official Ruby on Rails documentation, with highlights (if possible 😆 ) 👍

@mainameiz
Copy link

mainameiz commented Mar 6, 2019

There are duplicates in the table

azul added a commit to riseuplabs/crabgrass-core that referenced this pull request Jun 2, 2019
see rails/rails#25337.

Basically User.name_changed? behaved differently
in after_save callbacks then after the save finished.

This was confusing. So now there are two distinct methods:
* User.will_save_change_to_name - to use before the save
* User.saved_change_to_name? - to use in after_save callbacks

This way it is clear what the function is supposed to mean.
jrafanie added a commit to jrafanie/manageiq that referenced this pull request Jun 5, 2019
5.1 added new methods for checking changes, specifically in before
callbacks.

Summary:
5.0           => 5.1
name_changed? => will_save_change_to_name? (before callback)
name_changed? => saved_change_to_name? (after callback)

A larger table of changes can be found here:
rails/rails#25337 (comment)

Use latest ancestry with changes for the changed change:
https://github.com/stefankroes/ancestry/pull/441/files
jrafanie added a commit to jrafanie/manageiq that referenced this pull request Jun 5, 2019
5.1 added new methods for checking changes, specifically in before
callbacks.

Summary:
5.0           => 5.1
name_changed? => will_save_change_to_name? (before callback)
name_changed? => saved_change_to_name? (after callback)

A larger table of changes can be found here:
rails/rails#25337 (comment)

Use latest ancestry with changes for the changed change:
https://github.com/stefankroes/ancestry/pull/441/files
jrafanie added a commit to jrafanie/manageiq that referenced this pull request Jun 5, 2019
5.1 added new methods for checking changes, specifically in before
callbacks.

Summary:
5.0           => 5.1
name_changed? => will_save_change_to_name? (before callback)
name_changed? => saved_change_to_name? (after callback)

A larger table of changes can be found here:
rails/rails#25337 (comment)

Use latest ancestry with changes for the changed change:
https://github.com/stefankroes/ancestry/pull/441/files
jrafanie added a commit to jrafanie/manageiq that referenced this pull request Jun 6, 2019
5.1 added new methods for checking changes, specifically in before
callbacks.

Summary:
5.0           => 5.1
name_changed? => will_save_change_to_name? (before callback)
name_changed? => saved_change_to_name? (after callback)

A larger table of changes can be found here:
rails/rails#25337 (comment)

Use latest ancestry with changes for the changed change:
https://github.com/stefankroes/ancestry/pull/441/files
jrafanie added a commit to jrafanie/manageiq that referenced this pull request Jun 10, 2019
5.1 added new methods for checking changes, specifically in before
callbacks.

Summary:
5.0           => 5.1
name_changed? => will_save_change_to_name? (before callback)
name_changed? => saved_change_to_name? (after callback)

A larger table of changes can be found here:
rails/rails#25337 (comment)

Use latest ancestry with changes for the changed change:
https://github.com/stefankroes/ancestry/pull/441/files
jrafanie added a commit to jrafanie/manageiq that referenced this pull request Jun 11, 2019
5.1 added new methods for checking changes, specifically in before
callbacks.

Summary:
5.0           => 5.1
name_changed? => will_save_change_to_name? (before callback)
name_changed? => saved_change_to_name? (after callback)

A larger table of changes can be found here:
rails/rails#25337 (comment)

Use latest ancestry with changes for the changed change:
https://github.com/stefankroes/ancestry/pull/441/files
jrafanie added a commit to jrafanie/manageiq that referenced this pull request Jun 12, 2019
5.1 added new methods for checking changes, specifically in before
callbacks.

Summary:
5.0           => 5.1
name_changed? => will_save_change_to_name? (before callback)
name_changed? => saved_change_to_name? (after callback)

A larger table of changes can be found here:
rails/rails#25337 (comment)

Use latest ancestry with changes for the changed change:
https://github.com/stefankroes/ancestry/pull/441/files
jrafanie added a commit to jrafanie/manageiq that referenced this pull request Jun 13, 2019
5.1 added new methods for checking changes, specifically in before
callbacks.

Summary:
5.0           => 5.1
name_changed? => will_save_change_to_name? (before callback)
name_changed? => saved_change_to_name? (after callback)

A larger table of changes can be found here:
rails/rails#25337 (comment)

Use latest ancestry with changes for the changed change:
https://github.com/stefankroes/ancestry/pull/441/files
jrafanie added a commit to jrafanie/manageiq that referenced this pull request Jun 14, 2019
5.1 added new methods for checking changes, specifically in before
callbacks.

Summary:
5.0           => 5.1
name_changed? => will_save_change_to_name? (before callback)
name_changed? => saved_change_to_name? (after callback)

A larger table of changes can be found here:
rails/rails#25337 (comment)

Use latest ancestry with changes for the changed change:
https://github.com/stefankroes/ancestry/pull/441/files
jrafanie added a commit to jrafanie/manageiq that referenced this pull request Jun 14, 2019
5.1 added new methods for checking changes, specifically in before
callbacks.

Summary:
5.0           => 5.1
name_changed? => will_save_change_to_name? (before callback)
name_changed? => saved_change_to_name? (after callback)

A larger table of changes can be found here:
rails/rails#25337 (comment)

Use latest ancestry with changes for the changed change:
https://github.com/stefankroes/ancestry/pull/441/files
jrafanie added a commit to jrafanie/manageiq that referenced this pull request Jun 18, 2019
5.1 added new methods for checking changes, specifically in before
callbacks.

Summary:
5.0           => 5.1
name_changed? => will_save_change_to_name? (before callback)
name_changed? => saved_change_to_name? (after callback)

A larger table of changes can be found here:
rails/rails#25337 (comment)

Use latest ancestry with changes for the changed change:
https://github.com/stefankroes/ancestry/pull/441/files
jrafanie added a commit to jrafanie/manageiq that referenced this pull request Jun 18, 2019
5.1 added new methods for checking changes, specifically in before
callbacks.

Summary:
5.0           => 5.1
name_changed? => will_save_change_to_name? (before callback)
name_changed? => saved_change_to_name? (after callback)

A larger table of changes can be found here:
rails/rails#25337 (comment)

Use latest ancestry with changes for the changed change:
https://github.com/stefankroes/ancestry/pull/441/files
jrafanie added a commit to jrafanie/manageiq that referenced this pull request Jun 18, 2019
5.1 added new methods for checking changes, specifically in before
callbacks.

Summary:
5.0           => 5.1
name_changed? => will_save_change_to_name? (before callback)
name_changed? => saved_change_to_name? (after callback)

A larger table of changes can be found here:
rails/rails#25337 (comment)

Use latest ancestry with changes for the changed change:
https://github.com/stefankroes/ancestry/pull/441/files
jrafanie added a commit to jrafanie/manageiq that referenced this pull request Jun 19, 2019
5.1 added new methods for checking changes, specifically in before
callbacks.

Summary:
5.0           => 5.1
name_changed? => will_save_change_to_name? (before callback)
name_changed? => saved_change_to_name? (after callback)

A larger table of changes can be found here:
rails/rails#25337 (comment)

Use latest ancestry with changes for the changed change:
https://github.com/stefankroes/ancestry/pull/441/files
azul added a commit to riseuplabs/crabgrass-core that referenced this pull request Feb 28, 2020
see rails/rails#25337.

Basically User.name_changed? behaved differently
in after_save callbacks then after the save finished.

This was confusing. So now there are two distinct methods:
* User.will_save_change_to_name - to use before the save
* User.saved_change_to_name? - to use in after_save callbacks

This way it is clear what the function is supposed to mean.
iurev added a commit to coursalytics/paper_trail that referenced this pull request May 20, 2020
rails/rails#25337

Note: most probably methods in the previous version weren't exactly correct.

Note: there are still ~20 broken tests
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

You can’t perform that action at this time.