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

Fix change tracking when fetching from collections or associations #298

Merged
merged 3 commits into from
Apr 13, 2018

Conversation

chewi
Copy link
Contributor

@chewi chewi commented Nov 14, 2014

This should close #295.

@chewi
Copy link
Contributor Author

chewi commented Feb 5, 2015

Just rebased this for your convenience. Please merge! 😺

@chewi chewi force-pushed the fix-change-tracking branch from d6b1b47 to 825db00 Compare November 27, 2015 16:27
@chewi
Copy link
Contributor Author

chewi commented Nov 27, 2015

Rebased again!

@chewi
Copy link
Contributor Author

chewi commented Jun 10, 2016

Hmmm. This newest fix only works with Rails 4.2+. I therefore made it optional by checking for the presence of a method but I assumed that the specs would always use the latest version. I forgot that Travis also checks older ones.

@edtjones
Copy link
Collaborator

hi @chewi I've just got commit access on this repo and plan to get back on track with maintenance over time - will be in touch soon!

@chewi chewi force-pushed the fix-change-tracking branch from 6825f06 to 550663a Compare February 20, 2017 15:13
Copy link
Collaborator

@zacharywelch zacharywelch left a comment

Choose a reason for hiding this comment

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

specs look 💯.

left a few comments before merging. can you also take a look at the travis build?

@edtjones we'll need to update ActiveModel too which should help us clean up some of the dirty attributes logic.

@@ -25,6 +25,7 @@ def initialize(attributes={})

attributes = self.class.default_scope.apply_to(attributes)
assign_attributes(attributes)
@changed_attributes = {} if persisted?
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should be covered in #404.

entry.send("#{inverse_of}=", @parent)

# This fix requires ActiveModel 4.2+.
if entry.respond_to?(:clear_attribute_changes, true)
Copy link
Collaborator

Choose a reason for hiding this comment

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

nice 👍

Once we update ActiveModel we should probably look at clear_changes_information after a record is fetched or reloaded and changes_applied after persistence.

builder.use Her::Middleware::FirstLevelParseJSON
builder.use Faraday::Request::UrlEncoded
builder.adapter :test do |stub|
stub.get("/users/1") { [200, {}, { id: 1, fullname: "Lindsay Fünke" }.to_json] }
stub.get("/users/2") { [200, {}, { id: 2, fullname: "Maeby Fünke" }.to_json] }
stub.get("/users") { [200, {}, [user1, user2].to_json] }
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we stick w/ the current, explicit style to keep the specs consistent? Also think it helps to see the json w/ each stub 😺

@chewi chewi force-pushed the fix-change-tracking branch from 550663a to 6dfbfd5 Compare November 10, 2017 17:43
@chewi
Copy link
Contributor Author

chewi commented Nov 10, 2017

Sorry, this fell beneath the pile. I have rebased this, fixed the specs as requested, and changed the second commit to not require AR 4.2+. I believe assigning directly to the attributes hash should be safe here.

I see that #411 hasn't been merged yet. They deal with the same issue but I think this solution handles more cases.

Copy link
Collaborator

@zacharywelch zacharywelch left a comment

Choose a reason for hiding this comment

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

@chewi thanks for your effort on this! Had one small request before merging. Let me know if it's something you can look at otherwise I could amend the commit before moving onto #318 😺

@@ -32,6 +32,7 @@ def initialize(attributes={})
attributes = self.class.default_scope.apply_to(attributes)
assign_attributes(attributes)
yield self if block_given?
@changed_attributes = {} if persisted?
Copy link
Collaborator

@zacharywelch zacharywelch Jan 5, 2018

Choose a reason for hiding this comment

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

Can we reset changed_attributes inside instantiate_record like #411? That would be closer to the recommendation and correct this small difference

# ActiveRecord behavior
user = User.new(id: 1)
user.changes # =>  {"id"=>[nil, 1]}

# Her behavior
user = User.new(id: 1)
user.changes # => {}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well spotted. About to head out the door. I think this may miss some cases but I have an idea that I will try on Monday. Thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've tried to make this work but regardless of which approach I use, the id parameter is ignored here. I haven't figured out why yet and I'm very busy this week but hopefully I'll get time on Friday.

Copy link
Contributor Author

@chewi chewi Jan 19, 2018

Choose a reason for hiding this comment

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

Found the reason. In use_setter_methods, both id and the actual primary key name are treated as reserved so assignment methods are not created for them when the record is instantiated. They end up in the unset_attributes hash in the assign_attributes method and simply get merged into the @attributes hash. This means that no change tracking is applied. If you directly assign new values using setter methods afterwards then change tracking does work because method_missing is invoked, which creates attribute methods without checking a reserved list first.

If I change use_setter_methods to not treat these as reserved then the above works but it breaks the handles new resource with custom primary key test. I need to find a way for the id method to not be redefined without blocking the creation of an id= method.

@chewi chewi force-pushed the fix-change-tracking branch from 6dfbfd5 to d407ef4 Compare January 22, 2018 15:12
@chewi
Copy link
Contributor Author

chewi commented Jan 22, 2018

Okay @zacharywelch, I've made a change to how primary keys are handled that I believe makes things cleaner and deals with the issue we were seeing above. I've also changed the handling of @changed_attributes as you suggested.

I forgot to mention in the last commit message that ActiveRecord also allows id= to change the primary key rather than treating it as a separate attribute.

@chewi
Copy link
Contributor Author

chewi commented Jan 23, 2018

Hmm looks like this approach needs adjusting for Rails 3.2. It's also not quite right for subclasses either. Bear with me.

chewi added 3 commits February 9, 2018 17:24
Not doing do was leading to change tracking not being applied in some
circumstances.

With a custom primary key, id is now treated as a formal alias. This
alters the behaviour slightly but setting an attribute with id= and
then getting a totally different value back with id is just as useless
as it is confusing. Some tests have had to be adjusted accordingly.

Note that you cannot set the primary key back to :id if it was set to
something else in an ancestor class. Undoing attribute aliases leads
to infinite loops.
@chewi chewi force-pushed the fix-change-tracking branch from d407ef4 to e0e41f5 Compare February 9, 2018 18:30
@chewi
Copy link
Contributor Author

chewi commented Feb 9, 2018

I've put in a restriction to avoid the subclass and Rails 3.2 issues. Haven't had time to fully test this yet but I thought I'd push it up anyway because my break next week. Maybe I'll test it out at home if I get a chance.

@chewi
Copy link
Contributor Author

chewi commented Feb 23, 2018

Okay, I've tried this against our own test suite and it all looks good.

@zacharywelch zacharywelch merged commit e0e41f5 into remi:master Apr 13, 2018
@zacharywelch
Copy link
Collaborator

Unfortunately had to remove the primary key changes before merging to master, but we solved the original issue of freshly-loaded resources being marked as dirty 🎩 💯

Still open to a solution of the primary key problem that doesn't involve raising an error if anyone's interested in putting together a PR. Glad we were able to identify some of the challenges.

Thanks!

@zacharywelch
Copy link
Collaborator

I may have found a solution to the primary key problem @chewi. Working on a PR.

@chewi
Copy link
Contributor Author

chewi commented Apr 13, 2018

Thanks. Do you mean the cannot change primary key to :id if ancestors use other name exception? Is there an issue with it? I thought this was extremely unlikely to be encountered in practise. I did find that it perhaps didn't matter if the attribute aliases were not undone, meaning the exception could potentially be avoided, but I took what I felt was the safer route.

@chewi chewi deleted the fix-change-tracking branch April 13, 2018 10:53
@zacharywelch
Copy link
Collaborator

Right, would rather find a solution that solves both use cases and remains backwards compatible. In the process of testing I uncovered another pre-existing issue related to primary_key so it just needs some ❤️. Lemme type up an issue so we don't lose track of our findings.

Thanks!

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

Successfully merging this pull request may close these issues.

Changes not tracked properly on instance pulled from collection
3 participants