RFC: associations with :accessible => true should allow updating

Recently the following patch

http://github.com/rails/rails/commit/e0750d6a5c7f621e4ca12205137c0b135cab444a

was committed to rails trunk. This patch allows for associations to be
flagged as :accessible => true and then hydrated from nested hashes
(i.e. nested forms)

class Post < ActiveRecord::Base
  belongs_to :author, :accessible => true
  has_many :comments, :accessible => true
end

post = Post.create({
  :title => 'Accessible Attributes',
  :author => { :name => 'David Dollar' },
  :comments => [
    { :body => 'First Post!' },
    { :body => 'Nested Hashes are great!' }
  ]
})

post.comments << { :body => 'Another Comment' }

I have done some work on another patch to allow this same mechanism to
be used for updating existing rows using nested hashes. This completes
the work as far as dynamically generating forms and being able to
simply and intuitively push them back into the database.

http://github.com/ddollar/rails/commit/14a16844bbb3ba9edb14269ce2d0b61c9a43637e

This allows for the following:

# create from a basic hash

p = Post.create(:title => 'Test Post', :author => { :name => 'David' })

=> #<Post id: 8, author_id: 8, title: "Test Post", created_at:
"2008-07-14 15:22:53", updated_at: "2008-07-14 15:22:53">

# update a singular reference

p.author = { :name => 'Joe' }

=> {:name=>"Joe"}

# it 'updates' the row in sql, notice the id is still the same

p.author

=> #<Author id: 8, name: "Joe", created_at: "2008-07-14 15:22:53",
updated_at: "2008-07-14 15:23:03">

# create an author with posts from a hash

a = Author.create(:name => 'David', :posts => [ { :title => 'Post 1' }, { :title => 'Post 2' } ])

=> #<Author id: 14, name: "David", created_at: "2008-07-14 15:38:18",
updated_at: "2008-07-14 15:38:18">

# show the posts

a.posts

=> [#<Post id: 17, author_id: 14, title: "Post 1", created_at:
"2008-07-14 15:38:18", updated_at: "2008-07-14 15:38:18">, #<Post id:
18, author_id: 14, title: "Post 2", created_at: "2008-07-14 15:38:18",
updated_at: "2008-07-14 15:38:18">]

# use << to update existing entries (as well as add new ones,
demonstrated later)

a.posts << { :id => 17, :title => 'Post 1 Updated' }

=> [#<Post id: 17, author_id: 14, title: "Post 1 Updated", created_at:
"2008-07-14 15:38:18", updated_at: "2008-07-14 15:38:53">, #<Post id:
18, author_id: 14, title: "Post 2", created_at: "2008-07-14 15:38:18",
updated_at: "2008-07-14 15:38:18">]

# show posts to verify the update

a.posts

=> [#<Post id: 17, author_id: 14, title: "Post 1 Updated", created_at:
"2008-07-14 15:38:18", updated_at: "2008-07-14 15:38:53">, #<Post id:
18, author_id: 14, title: "Post 2", created_at: "2008-07-14 15:38:18",
updated_at: "2008-07-14 15:38:18">]

# can't update posts that don't belong to the author

a.posts << { :id => 1, :title => 'Not Allowed' }

ActiveRecord::RecordNotFound: Couldn't find Post with ID=1 AND
("posts".author_id = 14)
        from /Users/ddollar/Code/EdgeRailsApp/vendor/rails/
activerecord/lib/active_record/base.rb:1393:in `find_one'
        from /Users/ddollar/Code/EdgeRailsApp/vendor/rails/
activerecord/lib/active_record/base.rb:1376:in `find_from_ids'
        from /Users/ddollar/Code/EdgeRailsApp/vendor/rails/
activerecord/lib/active_record/base.rb:537:in `find'
        from /Users/ddollar/Code/EdgeRailsApp/vendor/rails/
activerecord/lib/active_record/associations/association_collection.rb:
47:in `find'
        from /Users/ddollar/Code/EdgeRailsApp/vendor/rails/
activerecord/lib/active_record/associations/association_collection.rb:
103:in `<<'
        from /Users/ddollar/Code/EdgeRailsApp/vendor/rails/
activerecord/lib/active_record/associations/association_collection.rb:
99:in `each'
        from /Users/ddollar/Code/EdgeRailsApp/vendor/rails/
activerecord/lib/active_record/associations/association_collection.rb:
99:in `<<'
        from /Users/ddollar/Code/EdgeRailsApp/vendor/rails/
activerecord/lib/active_record/connection_adapters/abstract/
database_statements.rb:66:in `transaction'
        from /Users/ddollar/Code/EdgeRailsApp/vendor/rails/
activerecord/lib/active_record/transactions.rb:79:in `transaction'
        from /Users/ddollar/Code/EdgeRailsApp/vendor/rails/
activerecord/lib/active_record/transactions.rb:98:in `transaction'
        from /Users/ddollar/Code/EdgeRailsApp/vendor/rails/
activerecord/lib/active_record/associations/association_collection.rb:
98:in `<<'
        from (irb):12

# use = to outright replace all posts

a.posts = [ { :title => 'Replace Posts' } ]

=> [#<Post id: 19, author_id: 14, title: "Replace Posts", created_at:
"2008-07-14 15:40:30", updated_at: "2008-07-14 15:40:30">]

# can even 'replace' using existing posts, the post attributes will be
updated

a.posts = [ { :id => 19, :title => 'Can Replace This Way Too' } ]

=> [#<Post id: 19, author_id: 14, title: "Can Replace This Way Too",
created_at: "2008-07-14 15:40:30", updated_at: "2008-07-14 15:40:49">]

# use << also for adding brand new items

a.posts << { :title => 'New Post' }

=> [#<Post id: 19, author_id: 14, title: "Replace Posts", created_at:
"2008-07-14

The patch can be found at

http://rails.lighthouseapp.com/projects/8994-ruby-on-rails/tickets/619-allow-accessible-true-associations-to-update-records

I was asked to submit this patch to the mailing list and solicit any
comments. Does anyone see any obvious holes or ways this functionality
should change?

Thanks,
David Dollar

Has anyone any strong opinions against this ? I'm very interested in
hearing more thoughts on this and/or if you have any suggestions to
improvise David's approaches.

Thanks,
Pratik

Wow, great news! I am very in favor of this patch. When learning
Rails, I was surprised that objects couldn't be created from a nested
hash. Now with this patch, it will be very useful in complex forms
(the problem is well covered in Railscasts Complex Forms series). I
will take a closer look at the patch soon.

A big +1 on this and thanks for the patch

Hique
http://hiquepedia.com

I'm generally in favor of this for pretty much the same reason as
Hique.
One thing, though: If this kind of behavior was merged into core, the
fields_for method should also be updated to accept an array as its
first parameter to facilitate the use of nested hashes.

Right now, we have to do something like the following:
<% form_for :product do |f| %>
<p><%= f.text_field :name %></p>
<% fields_for "product[price]" do |f| -%>
<p><%= f.text_field :quantity %></p>
<p><%= f.text_field :price %></p>
<% end -%>
<% end %>

It would be great to have it like this:
<% form_for :product do |f| %>
<p><%= f.text_field :name %></p>
<% fields_for [:product, :price] do |f| -%>
<p><%= f.text_field :quantity %></p>
<p><%= f.text_field :price %></p>
<% end -%>
<% end %>

Currently, fields_for does accept an array but the implementation is
faulty/illogical. I've just posted a ticket that addresses this issue
(http://rails.lighthouseapp.com/projects/8994-ruby-on-rails/tickets/
641-formhelper-fields_for-has-illogical-and-unused-option). It would
be great if the above code could produce a nested hash much like
form_for builds nested routes from an array.

Has anyone any strong opinions against this ? I'm very interested in
hearing more thoughts on this and/or if you have any suggestions to
improvise David's approaches.

Thanks,
Pratik

Is the :accessible specifier necessary? In other words, are we afraid
something might break if ActiveRecord::Base#new and #create natively
understood nested hashes as well as the flat hashes that it can accept
today?

Jeff

Has anyone any strong opinions against this ? I'm very interested in
hearing more thoughts on this and/or if you have any suggestions to
improvise David's approaches.

Thanks,
Pratik

Is the :accessible specifier necessary? In other words, are we afraid
something might break if ActiveRecord::Base#new and #create natively
understood nested hashes as well as the flat hashes that it can accept
today?

Jeff

The main concern that I have seen presented is that form injection
could cause data to be created/updated around your database
that you did not intend should the attacker carefully craft a form for
submission.

David

Good point David, this can be very dangerous if always enabled due to
form injection. But when intentionally enabled, this has the potential
to greatly simplify multi-model forms - which I love!

Out of curiosity, have you tried making a multi-model form with this?
I can if you want. There are a lot of complexities involved,
specifically with regards to when the associated models are saved and
what happens when validation fails.

Ryan

Good point David, this can be very dangerous if always enabled due to
form injection. But when intentionally enabled, this has the potential
to greatly simplify multi-model forms - which I love!

Out of curiosity, have you tried making a multi-model form with this?
I can if you want. There are a lot of complexities involved,
specifically with regards to when the associated models are saved and
what happens when validation fails.

It'd be a great idea to go through the requirements you cover in your
awesome screencasts, and see how this helps or hinders each case.
That'd help us get a handle on how this is going to play out.

Good point David, this can be very dangerous if always enabled due to
form injection. But when intentionally enabled, this has the potential
to greatly simplify multi-model forms - which I love!

Out of curiosity, have you tried making a multi-model form with this?
I can if you want. There are a lot of complexities involved,
specifically with regards to when the associated models are saved and
what happens when validation fails.

Yep, exactly. I talked with David Dollar on IRC about this. I've
done this in a way that is similar to your recipe in ARR, but I create/
update the associated models in a before_validation callback, rather
than in the setter. That keeps everything in one transaction and
makes it a little easier to handle validation issues. I did like your
approach of splitting the setters into two pieces for create and
update. The one thing that keeps bugging me though is doing deletes -
I've still yet to figure out a really nice way to handle that.

--josh

I made a GitHub project to catalog the various approaches to the multi-
model form problem.
http://github.com/ryanb/complex-form-examples/

You can find my approach using David Dollar's fork here:
http://github.com/ryanb/complex-form-examples/tree/ddollar-accessible

It greatly simplifies the code, however there are a couple issues:
- an exception is raised when validation fails for task
- tasks are updated even if validation fails for project

I'm unsure the best way to solve these. If you have a solution, or an
entirely different approach, please fork this project and send a pull
request.

Ryan

I'm going to have to try using a before_validation callback. Regarding
deletes, are you not happy with removing all children that are missing
in the params hash (as the recipe does)? I suppose it is a little
dangerous, but I can't think of an alternative solution which is close
to the same simplicity.

Ryan

On a related-yet-slightly-different-topic note:

p = Project.find(:first)
p.tasks.first.name = 'New Name'
p.save

does not update the Task in the database. I'd like to get some
thoughts/opinions on using dirty tracking to make cascadable saves
possible.

If I could cascade the saves, it becomes a lot easier to bubble up
validations in a format that makes sense for nested associations.

Whoa pardner, that's dangerously close to the Unit-of-Work pattern there! I like this idea (it's been discussed before many times), but it's a huge change to how things work. Much moreso than just partial updates. I'm all for it though.

The big issue I see is finding the associated objects that need saving. AR isn't good at dealing with partially loaded has_many association collections. I think you'd need a new API on associations to enumerate the already loaded records and see which are dirty and need saving.

p = Project.find(some_id)
task = p.tasks.find_by_name("send invoice")
task.due_date = 3.days.from_now
p.build_task(task_params)
p.save

There's a lot that would need to happen to make that example work, and maybe that's even reaching a bit too far. Anyway, just thinking out loud.

I think the key here is that the new :accessible=>true option allows
for mass assignments on create, but doesn't support a good method for
doing updates. It should be both or neither, therefore I strongly
believe that this is something that belongs in core as opposed to a
plugin. We shouldn't have to add 'the other half' of the
functionality needed from a plugin. This patch works extremely well
and I think it's one of the best nested update systems available.

Drew

I'd love to see something like this make it to core.

I'm a bit late in weighing in here, but for completeness, I should
mention that I attempted to solve the same problem with a plugin I
wrote called single_save (http://github.com/zilkey/single_save/tree/
master), based in large part off of Ryan's screencast. I also created
a demo app with it (http://github.com/zilkey/complex_forms_demo_app/
tree/master). There's also a similar attempt at
http://github.com/giraffesoft/attribute_fu/tree/master from James
Golick. I haven't had time to check out David's plugin in-depth, so I
apologize if anything here is duplicate info.

I think that the params should be explicit about what they are
deleting. Deleting everything that isn't in the params can lead to a
lot of unintended consequences re concurrency. If two people add
child items and click the save button at the same time, the added
items from one will be deleted. If instead there were an explicit
"marked_as_deleted" flag that was sent along in the params, both
people's input would be saved.

As far as deletes go, any parameter-hacking solution must also be able
to cover standard validation scenarios to be useful. If you remove an
child item from a form, and the form is invalid, when the form posts
back you should not child that you removed. If you cancel, the one
you removed should still be in the database. This should also hold
true even if the form is submitted multiple times with invalid data.

It seems to me that accessible attributes should be able to do the
following:

* if the parameter set is there, with an id, update it
* if the parameter set is there, without an id, add it
* if the parameter set is there, with an id, and it's set to
marked_as_deleted, then delete it as part of the transaction
after_save
* if there is an object in the database but no matching set of params,
do nothing

Another important point about deletes is that uniqueness validations
can become a problem - let's say you have a uniqueness constraint on a
task name, you load a project with those tasks, delete one, then add
one back:

class Task
  validates_uniquess_of :name, :scope => :project
end

p = Project.first
p.tasks.first.name # => "do something"
p.attributes = some_hash_that_marks_the_first_task_as_deleted
p.tasks_without_deleted # => []
p.attributes = some_hash_that_adds_a_new_task_named_do_something
p.save! # => throws validation error because there is already a task
named "do something"

This is something I haven't solved yet, but I can imagine two complex
solutions:

* run all deletes, then run validations, then run the inserts/
updates. If the validations fail, roll it back.
* create validations that are smarter - that can execute exists
statements passing in the ids of the children who are marked for
deletion.

I suspect that dealing with uniqueness will be a problem if you
implement marked_as_deleted or not.

I'm psyched to see this discussion on the core mailing list - if we
can get this right I would use it in almost every app I've written.

Bit late to the party I know, but can't you just do:

<% form_for :product do |product| %>
<%= product.text_field :name %>
<% product.fields_for :price do |price| %>
<%= price.text_field :quantity %>
<% end %>
<% end %>

I hope you can, because I've been using it in apps for ages!

Tom

Yep, that works (assuming product has only one price). However I'm not
sure if this fetches @price or @product.price object. I think the
former, but haven't tested.

Ryan

Sorry for a little off-topic but does anyone of know how to support an
object that inherits all attributes through STI when I
use :accessible=>true?

For example:

Object A belongs_to object B and B has_many A objects.
Object B is accessible for A.
Objects C and D inherit their fields through STI from the B object.

I want to create a form that will allow me to fill object's A fields
along with C or D's fields.
The form should have a type selector so that user can choose if he
wants to fill a C object or D.

Form has:
- object A fields
- choose object type list
- depending on the type selected in the list above a set of fields for
a specific B inheriting object (C or D)

There's actually a somewhat major issue with this patch, with respect
to multi-model forms.

Because the elements are submitted as an array [{}, {}, {}], they are
completely incompatible with ajax submissions of forms. When rails
receives params in an array format (param[]), it reads each element in
the order it receives it, until it reaches a duplicate key, that's
when it decides to create a new element in the array that your
controller ultimately sees. When forms are submitted via ajax, it
isn't really feasible to make sure that elements are submitted in
order. This is partly why I wrote attribute_fu, even when Ryan Bates'
solution was wrapped up in the multi-model forms plugin.

The hash that a_f generates is ugly, but it works in all cases. This
patch, unless amended to support those ugly hashes instead, is going
to cause people a lot of headache when they can't figure out why their
ajax forms won't save right. Or worse, when the attributes are all
getting crossed up between the different elements.

Just wanted to give an update since I've been somewhat quiet on the topic.

Thanks to the great suggestions both here and in #rails-contrib, I'm working
on a slightly different angle to this patch. Hopefully I'll have the code finished
in another day or two to post up here for review, but hopefully I can address
the many legitimate concerns that have been raised.

- David