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
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.
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
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?
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
On Jul 14, 6:38 pm, David Dollar <ddol...@gmail.com> wrote:
> 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
> 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.
> 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?
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.
> 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
> On Jul 14, 6:38 pm, David Dollar <ddol...@gmail.com> wrote:
> > 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
> > 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.
> > # can even 'replace' using existing posts, the post attributes will be
> > updated> a.posts = [ { :id => 19, :title => 'Can Replace This Way Too' } ]
> > 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?
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.
On Jul 17, 2:57 am, Pratik <pratikn...@gmail.com> wrote:
> 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
> On Jul 14, 6:38 pm, David Dollar <ddol...@gmail.com> wrote:
> > 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
> > 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.
> > # can even 'replace' using existing posts, the post attributes will be
> > updated> a.posts = [ { :id => 19, :title => 'Can Replace This Way Too' } ]
> > 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?
On Jul 16, 7:57 pm, Pratik <pratikn...@gmail.com> wrote:
> 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?
> > 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
> > 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.
> > # can even 'replace' using existing posts, the post attributes will be
> > updated> a.posts = [ { :id => 19, :title => 'Can Replace This Way Too' } ]
> > 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?
> On Jul 16, 7:57 pm, Pratik <pratikn...@gmail.com> wrote: >> 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.
>>> 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
>>> 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.
>>> # can even 'replace' using existing posts, the post attributes >>> will be >>> updated> a.posts = [ { :id => 19, :title => 'Can Replace This Way >>> Too' } ]
>>> 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?
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
On Jul 17, 4:41 pm, David Dollar <ddol...@gmail.com> wrote:
> > On Jul 16, 7:57 pm, Pratik <pratikn...@gmail.com> wrote:
> >> 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
> >> On Jul 14, 6:38 pm, David Dollar <ddol...@gmail.com> wrote:
> >>> 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
> >>> 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.
> >>> # can even 'replace' using existing posts, the post attributes
> >>> will be
> >>> updated> a.posts = [ { :id => 19, :title => 'Can Replace This Way
> >>> Too' } ]
> >>> 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?
> 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.
> On Jul 17, 4:41 pm, David Dollar <ddol...@gmail.com> wrote: >>> On Jul 16, 7:57 pm, Pratik <pratikn...@gmail.com> wrote: >>>> 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
>>>> On Jul 14, 6:38 pm, David Dollar <ddol...@gmail.com> wrote:
>>>>> 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
>>>>> 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.
>>>>> # can even 'replace' using existing posts, the post attributes >>>>> will be >>>>> updated> a.posts = [ { :id => 19, :title => 'Can Replace This Way >>>>> Too' } ]
>>>>> 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?
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
On Jul 18, 1:47 pm, Josh Susser <j...@hasmanythrough.com> wrote:
> > 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
> > Ryan
> > On Jul 17, 4:41 pm, David Dollar <ddol...@gmail.com> wrote:
> >>> On Jul 16, 7:57 pm, Pratik <pratikn...@gmail.com> wrote:
> >>>> 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
> >>>> On Jul 14, 6:38 pm, David Dollar <ddol...@gmail.com> wrote:
> >>>>> 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
> >>>>> 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.
> >>>>> # can even 'replace' using existing posts, the post attributes
> >>>>> will be
> >>>>> updated> a.posts = [ { :id => 19, :title => 'Can Replace This Way
> >>>>> Too' } ]
> >>>>> 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?
On Jul 18, 1:47 pm, Josh Susser <j...@hasmanythrough.com> wrote:
> 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.
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.
> On Jul 18, 1:47 pm, Josh Susser <j...@hasmanythrough.com> wrote: >> 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.
> 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
> --~--~---------~--~----~------------~-------~--~----~ > You received this message because you are subscribed to the Google > Groups "Ruby on Rails: Core" group. > To post to this group, send email to rubyonrails-core@googlegroups.com > To unsubscribe from this group, send email to rubyonrails-core-unsubscribe@googlegroups.com > For more options, visit this group at http://groups.google.com/group/rubyonrails-core?hl=en > -~----------~----~----~----~------~----~------~--~---
> 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.
On Jul 16, 6:57 pm, Pratik <pratikn...@gmail.com> wrote:
> Has anyone any strong opinions against this ?
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.
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.
On Jul 20, 10:38 am, Drew <drew.b...@gmail.com> wrote:
> On Jul 16, 6:57 pm, Pratik <pratikn...@gmail.com> wrote:
> > Has anyone any strong opinions against this ?
> 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.
On Thu, Jul 17, 2008 at 12:14 PM, Clemens <clem...@railway.at> wrote:
> 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 %>
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!
On Jul 20, 11:21 pm, "Tom Ward" <t...@popdog.net> wrote:
> <% form_for :product do |product| %>
> <%= product.text_field :name %>
> <% product.fields_for :price do |price| %>
> <%= price.text_field :quantity %>
> <% end %>
> <% end %>
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.
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.
On Jul 22, 6:38 am, raffael <pallan.la...@gmail.com> wrote:
> 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)
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.
> 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.
> On Jul 22, 6:38 am, raffael <pallan.la...@gmail.com> wrote: >> 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) > --~--~---------~--~----~------------~-------~--~----~ > You received this message because you are subscribed to the Google > Groups "Ruby on Rails: Core" group. > To post to this group, send email to rubyonrails-core@googlegroups.com > To unsubscribe from this group, send email to rubyonrails-core-unsubscribe@googlegroups.com > For more options, visit this group at http://groups.google.com/group/rubyonrails-core?hl=en > -~----------~----~----~----~------~----~------~--~---
> 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.
It seems like some form of cascading save is important if :accessible
=> true is going to apply to updates, given that those updates need to
be in the same transaction. Perhaps a simplistic API to tell AR about
related objects would be a good stepping-stone to a full solution:
I'm a bit late to join this conversation, and honestly I haven't read
it very closely yet. But I wrote something like this a few months ago
and attempted to address a lot of the edge cases and issues (updating
existing rows, etc). I have tests for it but they are integrated too
tightly with my application so I'll just give you the code to the
plugin. I'm using Javascript heavily with the forms so I haven't
thought much about how this might fit in with rails form helpers but I
don't imagine there would be much work.
> > 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.
> It seems like some form of cascading save is important if :accessible
> => true is going to apply to updates, given that those updates need to
> be in the same transaction. Perhaps a simplistic API to tell AR about
> related objects would be a good stepping-stone to a full solution:
I would also love to see something like this make it into core.
I had half-implemented a crappy version of this myself for my app, by
monkey-patching the setter methods generated by the has_many
association (e.g. "contacts=") to accept a hash.
The main problem I ran into was with creating new records with several
nested levels of association, e.g.
{ "school" => {
"contact" => [
{ "name" => "abc",
"telephone" => [
{ "area_code" => "nnn", "number" => "nnn" },
{ "area_code" => "nnn", "number" => "nnn" } ] } ] } }
i.e. There is a single form for a school, from which you can add/edit/
delete contacts and their associated telephones. Each school has_many
contacts and each contact has_many telephones.
Updating existing contacts/telephones works ok, because you don't have
nested arrays but nested hashes keyed by (if you're using the
technique from Ryan Bates' recipe) the record's id, e.g.
"contact" => {
"15" => {
"name" => "abc",
"telephone" => {
"33" => { ... },
"34" => { ... } } } }
My workaround was to always use nested hashes, but keyed by a loop
index (0, 1, 2, etc) and differentiating new records from existing
records by the absence or presence of the id parameter.
> 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
> 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.
> 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?
FWIW, I'm happy to write a patch for this -- the plugin was simply to get feedback on a potential implementation and the semantics. Similar patches have been rejected in the past because they changed the semantics and just tried to "work around" the existing broken parser code. A patch based off the plugin would both simplify the parser implementation and fix the "array" semantics.
Since I haven't really seen much outside of your response and Koz's, I assumed that either nobody has any feedback or nobody has really run into this problem and so doesn't particularly care. I find both hard to believe -- I know how opinionated the ruby community can get ;).
I understand it's a subtle change, but I would have thought the scenario you just described is relatively common. I know we've run into it more than once, and it was the reason the plugin came about in the first place.
In any case, thanks for putting in your two cents.
Dave Rothlisberger wrote: > I would also love to see something like this make it into core.
> I had half-implemented a crappy version of this myself for my app, by > monkey-patching the setter methods generated by the has_many > association (e.g. "contacts=") to accept a hash.
> The main problem I ran into was with creating new records with several > nested levels of association, e.g. > { "school" => { > "contact" => [ > { "name" => "abc", > "telephone" => [ > { "area_code" => "nnn", "number" => "nnn" }, > { "area_code" => "nnn", "number" => "nnn" } ] } ] } }
> i.e. There is a single form for a school, from which you can add/edit/ > delete contacts and their associated telephones. Each school has_many > contacts and each contact has_many telephones.
> Updating existing contacts/telephones works ok, because you don't have > nested arrays but nested hashes keyed by (if you're using the > technique from Ryan Bates' recipe) the record's id, e.g. > "contact" => { > "15" => { > "name" => "abc", > "telephone" => { > "33" => { ... }, > "34" => { ... } } } }
> My workaround was to always use nested hashes, but keyed by a loop > index (0, 1, 2, etc) and differentiating new records from existing > records by the absence or presence of the id parameter.
>> 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
>> 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.
>> # 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' }
>> 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?
+ 1 for keeping and improving mass assignment support in core.
In fact I find that the initial checkin (e0750d6a5c7f621e4ca12205137c0b135cab444a) works quite well.
I've swapped out a few custom association_attributes= style sections (ala Ryan Bates complex forms style) for the new accessible => true support and it really seems to work great - even with updates and deletes.
On Thu, Jul 24, 2008 at 4:19 PM, Thomas Lee <t...@vector-seven.com> wrote:
> FWIW, I'm happy to write a patch for this -- the plugin was simply to > get feedback on a potential implementation and the semantics. Similar > patches have been rejected in the past because they changed the > semantics and just tried to "work around" the existing broken parser > code. A patch based off the plugin would both simplify the parser > implementation and fix the "array" semantics.
> Since I haven't really seen much outside of your response and Koz's, I > assumed that either nobody has any feedback or nobody has really run > into this problem and so doesn't particularly care. I find both hard to > believe -- I know how opinionated the ruby community can get ;).
> I understand it's a subtle change, but I would have thought the scenario > you just described is relatively common. I know we've run into it more > than once, and it was the reason the plugin came about in the first place.
> In any case, thanks for putting in your two cents.
> Cheers, > T
> Dave Rothlisberger wrote: >> I would also love to see something like this make it into core.
>> I had half-implemented a crappy version of this myself for my app, by >> monkey-patching the setter methods generated by the has_many >> association (e.g. "contacts=") to accept a hash.
>> The main problem I ran into was with creating new records with several >> nested levels of association, e.g. >> { "school" => { >> "contact" => [ >> { "name" => "abc", >> "telephone" => [ >> { "area_code" => "nnn", "number" => "nnn" }, >> { "area_code" => "nnn", "number" => "nnn" } ] } ] } }
>> i.e. There is a single form for a school, from which you can add/edit/ >> delete contacts and their associated telephones. Each school has_many >> contacts and each contact has_many telephones.
>> Updating existing contacts/telephones works ok, because you don't have >> nested arrays but nested hashes keyed by (if you're using the >> technique from Ryan Bates' recipe) the record's id, e.g. >> "contact" => { >> "15" => { >> "name" => "abc", >> "telephone" => { >> "33" => { ... }, >> "34" => { ... } } } }
>> My workaround was to always use nested hashes, but keyed by a loop >> index (0, 1, 2, etc) and differentiating new records from existing >> records by the absence or presence of the id parameter.
>>> 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
>>> 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.
>>> # 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' }
>>> 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?
