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

Allow nested attributes in associations to update values in it's owner o... #3991

Merged
merged 1 commit into from
Dec 15, 2011
Merged

Conversation

akaspick
Copy link
Contributor

This patch allow nested attributes on an association to update the owner object. This worked in Rails 3.0.x, but stopped working in Rails 3.1.

Here's the example between Rails 3.0 and 3.1

class Task < AR
  accepts_nested_attributes_for :project
end

project = Project.find 1
params[:task] = {:name => 'task', :project_attributes => {:id => '1', :name => 'new name'}}

This works in rails 3.0.x (the project name is assigned correctly), but it does not work in rails 3.1.x

project.tasks.create params[:task]
puts project.name # 'new name'

In rails 3.1.x an exception is raised...
ActiveRecord::RecordNotFound, "Couldn't find Project with ID=1 for Task with ID="

The following works in rails 3.1.x even though we're creating the new task from the relation owned by the project object. We still need to pass the project id in explicitly so that it updates correctly.

project.tasks.create params[:task].merge(:project_id => project.id)
puts project.name # 'new name'

The patch allows the association to work as it did in Rails 3.0.

To fix the issue, the patch does the following...

  • Merges in the creation attributes of the owner object so that the new association has reference to the owner.
  • Defers assignment of any attributes that contain "sub-attributes" (in hashes) and makes sure that the base values (the first level of attribute keys) are assigned first as this is where the owner foreign key and other references for the new object will be set.
  • The deferred nested attributes are then assigned now that the first level of data has been set.

The solution given above for 3.1 can work, but has side affects because of possible hash ordering issues. The :project_id is merged in, but happens to be assigned before the :project_attributes nested attributes are. This of course would fail if Ruby references the :project_attributes attribute before the :project_id and the exception would again be raised.

This patch prevents this issue altogether since the nested attribute assignment is deferred until the base attributes are first set.

@dmitriy-kiriyenko
Copy link
Contributor

Well, it indeed should work, so +1, but if you are doing this in a real project, it's something wrong with the code.

@josevalim
Copy link
Contributor

This looks good to me. /cc @jonleighton

@akaspick
Copy link
Contributor Author

Sure, looking at the example you might think needing to do this seems a bit odd, but trust me, where I need this (in a complex form) it makes things a lot cleaner. Plus it worked fine in 3.0 and stopped working in 3.1, so lets make it work again. ;)

@jonleighton
Copy link
Member

Hmmm.... this doesn't feel like the cleanest thing in the world. Are you sure that this worked flawlessly on 3.0? I had a glance at the code but couldn't really see how this same problem wouldn't have occurred there.

Have you tried using :inverse_of on your Project#tasks association? Really that should be the 'proper' solution so that the tasks.project inverse gets set correctly when projects.tasks.create is called, and thus your :project_attributes gets assigned to the appropriate project.

I'm not against merging this, just hesitant if there is a better solution. But if it's definitely a regression we should probably just merge (but I am still curious what changed to actually cause the regression...)

@josevalim
Copy link
Contributor

Jon, I am not sure about what changed but I like the fact that this patch
changes Active Record to just apply nested attributes associations after
applying the owner attributes. It is a much more deterministic process.

@jonleighton
Copy link
Member

Alright, let's just merge this. If anyone wants to investigate why 3.0 is different then I'd be interested, but whatevs :)

jonleighton added a commit that referenced this pull request Dec 15, 2011
Allow nested attributes in associations to update values in it's owner o...
@jonleighton jonleighton merged commit 0ddb9d6 into rails:master Dec 15, 2011
@akaspick
Copy link
Contributor Author

The "owner values" were being set differently in 3.0, so the addition of merging creation attributes was one part of the fix. There was a fair amount of refactoring done here between 3.0 and 3.1 in the ActiveRecord API.

The issue of nested attributes being assigned before the base attributes were set didn't come up for some reason in 3.0, but it was an issue in 3.1, so making sure base attributes were assigned first was the final step in getting things to work. The hash "order" seemed to be "good enough" in 3.0, but not in 3.1.

@akaspick
Copy link
Contributor Author

Also as Jose says, the attribute assignment is much more deterministic now... no guess work on "hash order".

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.

None yet

5 participants