Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

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

Conversation

Projects
None yet
5 participants
Contributor

akaspick commented Dec 15, 2011

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.

Contributor

dmitriy-kiriyenko commented Dec 15, 2011

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

Contributor

josevalim commented Dec 15, 2011

This looks good to me. /cc @jonleighton

Contributor

akaspick commented Dec 15, 2011

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. ;)

Member

jonleighton commented Dec 15, 2011

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...)

Contributor

josevalim commented Dec 15, 2011

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.

Member

jonleighton commented Dec 15, 2011

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

@jonleighton jonleighton added a commit that referenced this pull request Dec 15, 2011

@jonleighton jonleighton Merge pull request #3991 from akaspick/attrfix
Allow nested attributes in associations to update values in it's owner o...
0ddb9d6

@jonleighton jonleighton merged commit 0ddb9d6 into rails:master Dec 15, 2011

Contributor

akaspick commented Dec 15, 2011

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.

Contributor

akaspick commented Dec 15, 2011

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

Do you think there's another way of fixing that? The code doesn't work with mass assignment protection turned on. I had to remove it on master. Check the pull request for details.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment