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 has_many assocation w/select load after create #7859

Merged
merged 1 commit into from Oct 10, 2012
Merged

Fix has_many assocation w/select load after create #7859

merged 1 commit into from Oct 10, 2012

Conversation

ernie
Copy link
Contributor

@ernie ernie commented Oct 5, 2012

If you create a new record via a collection association proxy that has
not loaded its target, and which selects additional attributes through
the association, then when the proxy loads its target, it will
inadvertently trigger an ActiveModel::MissingAttributeError during
attribute writing when CollectionAssociation#merge_target_lists attempts
to do its thing, since the newly loaded records will possess attributes
the created record does not.

This error also raises a bogus/confusing deprecation warning when
accessing the association in Rails 3.2.x, so cherry-pick would be
appreciated!

If you create a new record via a collection association proxy that has
not loaded its target, and which selects additional attributes through
the association, then when the proxy loads its target, it will
inadvertently trigger an ActiveModel::MissingAttributeError during
attribute writing when CollectionAssociation#merge_target_lists attempts
to do its thing, since the newly loaded records will possess attributes
the created record does not.

This error also raises a bogus/confusing deprecation warning when
accessing the association in Rails 3.2.x, so cherry-pick would be
appreciated!
@ernie
Copy link
Contributor Author

ernie commented Oct 5, 2012

Quick note: an alternative (but slightly more cumbersome to implement) fix might be to detect those loaded records which possess attributes the in-memory versions don't have, and swap the attribute replacement out to the changes from the in-memory version. I'm working on that fix now.

@ernie
Copy link
Contributor Author

ernie commented Oct 5, 2012

On second thought, no I'm not. That has some pretty serious drawbacks, in that the object would no longer be the same instance as the one that the user may have received as a return from the call to create.

@rafaelfranca
Copy link
Member

@tenderlove could you take a look?

@tenderlove
Copy link
Member

Yes, I'll take a look in the morning. :-)

Aaron Patterson
http://tenderlovemaking.com/
I'm on an iPhone so I apologize for top posting.

On Oct 5, 2012, at 10:31 PM, Rafael Mendonça França notifications@github.com wrote:

@tenderlove could you take a look?


Reply to this email directly or view it on GitHub.

@ghost ghost assigned tenderlove Oct 6, 2012
@ernie
Copy link
Contributor Author

ernie commented Oct 8, 2012

Aloha, @tenderlove! I've been thinking about this and as much as I really wish there was a good way for an in-memory record to have identical attributes to those read through the association, the use case would be sufficiently rare that the solution here is probably "good enough."

Internal AR calls shouldn't be raising errors/deprecations (depending on version) that were intended to reflect changes being necessary to user code, so this patch avoids that. Swapping out the in-memory object for its from-disk counterpart would be bad for the reasons I mentioned the other day, and trying to step around the write_attribute protection against creating new attributes on an existing object (to use the in-memory version but graft on the additional attributes) is painful enough that I think the code is trying to tell us we'd be nuts to do it.

@tenderlove
Copy link
Member

@ernie ok! I'll merge as-is then! Thanks.

tenderlove added a commit that referenced this pull request Oct 10, 2012
…select

Fix has_many assocation w/select load after create
@tenderlove tenderlove merged commit 269adae into rails:master Oct 10, 2012
@ernie
Copy link
Contributor Author

ernie commented Oct 12, 2012

@tenderlove Thanks! Any chance it could get cherry-picked to 3-2-stable as well? It doesn't raise an error there, but generates a bogus deprecation message:

DEPRECATION WARNING: You're trying to create an attribute `some_attribute'. Writing arbitrary attributes on a model is deprecated. Please just use `attr_writer` etc. (called from some_method at /some_file:5)

@ernie
Copy link
Contributor Author

ernie commented Oct 12, 2012

Or, if you want, I can work out a test for 3-2-stable and submit a separate PR

@rafaelfranca
Copy link
Member

@ernie please submit a pull request

@ernie
Copy link
Contributor Author

ernie commented Oct 12, 2012

@rafaelfranca done!

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