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

rubyonrails-core
1

Recently the following patch

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

2

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

3

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

4

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 (Tickets - Ruby on Rails - rails 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.

5

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

6

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

7

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

8

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.

9

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

10

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

11

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

12

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.

13

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.

14

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

15

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.

16

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

17

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

18

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)

19

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.

20

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