I'm a couple of weeks late, but I just finished reviewing Rails 2.3 Nested Object Forms. While a very nice and "magical" feature, I've got to admit that I'm really not that crazy about how it works.
Let me be the first to admit that there's no one right way to do things. In fact, I'm writing this post particularly because I have a few objections to how this functionality is ultimately exposed, and I'd like to hear arguments from those who disagree. In other words, let me acknowledge the possibility that I am the misguided one.
Review of Nested Object Forms
As a quick review, we can now prepare our models for nested object mass assignment:
class Person < ActiveRecord::Base
validates_presence_of :name
has_many :children
accepts_nested_attributes_for :children
# can also be used on has_one etc.. associations
end
class Child < ActiveRecord::Base
belongs_to :person
end
We specify which nested relationships can be passed in via mass assignment using the accepts_nested_attributes_for() function. Continuing the example, we can now execute the following to actually save a person and two children at the same time:
person = Person.create!(:name => 'George', :children_attributes => {"new_1" => {:name => 'Amy'}, "new_2" => {:name => 'Stephanie'}})
For full disclosure, my console session from start to finish:
john-mbp:playground john$ rails -v
Rails 2.3.0
john-mbp:playground john$ rails anewapp
...
...
john-mbp:playground john$ cd anewapp/
john-mbp:anewapp john$ ruby script/generate model person name:string
...
john-mbp:anewapp john$ ruby script/generate model child name:string person_id:integer
...
# edit app/models/person.rb as above
# edit app/models.child.rb as above
john-mbp:anewapp john$ rake db:migrate
(in /Users/john/playground/anewapp)
== CreatePeople: migrating ===================================================
-- create_table(:people)
-> 0.0044s
== CreatePeople: migrated (0.0046s) ==========================================
== CreateChildren: migrating =================================================
-- create_table(:children)
-> 0.0041s
== CreateChildren: migrated (0.0045s) ========================================
john-mbp:anewapp john$ ruby script/console
Loading development environment (Rails 2.3.0)
>> person = Person.create!(:name => 'George', :children_attributes => {"new_1" => {:name => 'Amy'}, "new_2" => {:name => 'Stephanie'}})
=> #<Person id: 1, name: "George", created_at: "2009-02-24 00:49:21", updated_at: "2009-02-24 00:49:21">
>> person.children
=> [#<Child id: 1, name: "Amy", person_id: 1, created_at: "2009-02-24 00:49:21", updated_at: "2009-02-24 00:49:21">, #<Child id: 2, name: "Stephanie", person_id: 1, created_at: "2009-02-24 00:49:21", updated_at: "2009-02-24 00:49:21">]
>> person.children.size
=> 2
Pretty cool, huh? I agree. I think this is fantastic, and with the integration with fields_for, it will really ease a lot of the pain that previously existed with multi-model forms.
This said, I've got a few major bones to pick with how it's implemented.
So What's Wrong?
The primary issue is the fact that this functionality is statically defined (in that you can't turn it on/off in specific scenarios). Why is this an issue? Well, this is a potentially dangerous functionality to expose everywhere. Let's say that you wanted to take advantage of this new feature to allow administrators to create new accounts that came pre-loaded with a complementary $X credit to their accounts. So you built a form for admins to fill out that included a fields_for :credits to achieve this single-form implementation of an account with credits. Now, in order to avail of this, I need to turn on the nested mass assignment.
class Person < ActiveRecord::Base
has_many :credits
accepts_nested_attributes_for :credits
end
class Credit < ActiveRecord::Base
belongs_to :person
end
Wait....I've just turned this feature on globally.....I can't just make it available at the controller level? But isn't the controller where I define authorization, who can edit/update/create what? Precisely.
So....unless I want to go old school and rewrite this all by hand, anytime I want to render a person form (e.g. on an "Edit My Profile" link for authenticated users), I need to recall that I've exposed myself to a possible POST data hack (anyone who understands this feature in rails will know how to handcraft the POST data). This is a fairly ridiculous thing to expect on a project of any nontrivial size.
However, there's another way around this issue. Rather than rewriting all of the multi-model form stuff by hand, I could simply ward off the extra parameters at the controller level, e.g.
class ProfileController < ApplicationController
before_filter :remove_nested_params
def update
@user = User.find(current_user.id)
@user.update_attributes!(params[:user])
redirect_to profile_path
rescue
render :action => 'edit'
end
protected
def remove_nested_params
params[:user].delete(:credits)
end
end
This works fine in isolation or for a one-off, but again, this is a piece of information that is going to be lost as the project trudges along. Your team will forget this, and someone is going to forget to "close the hole" when adding that new "quick edit profile" form that only shows their email address and display name.
Counter Suggestions
I would advocate any solution where this behavior was pushed out to controllers. That said, I don't have an airtight procedure planned out. I will, however, leave you with a few suggestions.
We could implement it around_filter style where the feature is enabled/disabled in an around_filter. The primary benefit of this approach would be that the majority of the code could probably stay put in ActiveRecord. We'd just need to expose hooks to turn it on/off. Then we could define a controller-level meta-function (e.g. filter_parameter_logging) that could set which actions to apply the behavior on, or which models can be nested. The syntax could look like:
class ProfileController < ApplicationController
allow_nested_assignment_for User, [:children], :only => [:update]
end
I'll be perfectly honest in that I don't love this suggestion. It incorrectly scopes this authorization tweak to the whole action and any before/after filters that execute in between it (still leaving holes, albeit smaller) as opposed to scoping it directly to the single save! action that we really want to special-case. If we perhaps exposed a separate save_with_nested() function, then we could isolate the scope down to the specific call. Needless to say, I doubt this is a course of action rails would take, and it's not one I would advocate.
Another alternative would be to keep the functionality as is, but then allow you to override or extend it at the controller level (a la my initial suggestion). This would allow developers to avail of the benefits in the less-risky areas, and then conditionally avail of them in more risky areas by simply turning it on for specific controller/action combos. In this case, we'd define a basic harmless nested assignment baseline in the model, and the more risky nested assignment in those controllers where we wanted to isolate it, e.g.
class Person < ActiveRecord::Base
has_many :children
has_many :credits
accepts_nested_attributes_for :children
end
class Admin::PeopleController < ActiveRecord::Base
allow_nested_assignment_for User, :credits, :only => [:create, :update]
end
The primary benefit of an approach like this is that it aligns properly to a developer's train of thought. When you go to add a trivial feature, you don't think "I need to close that gap my unrelated nontrivial feature added for me." However, when you go to add a nontrival feature (like allowing an admin to auto-create credits in their new account form), you do think "what do I need to differently?" In the former case, the developer thinks his new feature is trivial, and doesn't expect to have do anything exceptional for it. In the latter case, the developer thinks his new feature is nontrivial, and will think about it from a "what extra work does that mean for me?" standpoint.
Closing Thoughts
One of the biggest things that worries me is that no one is talking about this. The whole community requested this feature, and at this point it has received quite a bit of fanfare. But there are some clear ways to accidentally screw yourself that just aren't being mentioned as caveats.
To be entirely blunt, I just feel like this is plugin material at this point. Though I commend rails for opening up its feature set to a community vote, I fear that we may have misstepped with this feature in particular. I'm very curious to hear what others think. (Be gentle with the flame)
