accepts_nested_attributes_for patch request

rubyonrails-core
1

Nice work on the accepts nested attributes for, I've been waiting for this for a while and haven't had the time to write it.

I am curious to know about the feature for deleting attributes. For a lot of reasons, I prefer to just not send back the items I want deleted. Do you think it would make sense to add an attribute called something like deletes_when_not_present that would essentially update or create the nested objects, and then deletes any that are present still in the database that weren't in the hash? Here is the basic of the workaround I'm using...

old_ids = parent.get_children_ids parent.update(params[:parent]) old_ids.each do |oid|   oid.member? parent.children_ids || Parent.destroy(oid) end

Is there a reason it was skipped?

I'd be more than happy to write a patch for it, but I don't want to waste time if its breaking convention. I work in very large xml data sets with flex and sending redundant data gets costly and cuts down on efficiency.

-Josh

2

Eloy would know for sure, but I believe it was left out because of the risk in accidentally deleting objects just because you disabled a form element, or was paginating in a form, or something similar.

3

It would also cause problems if you have a multi-user environment. User 1 downloads the form, User 2 completes an edit form that adds a new association to the collection, User 1 posts their form without having record of the newly created record from User 2 → User 1 accidentally deletes User 2’s addition.

I suspect the same scenario is possible for plain old attributes in a form, but seems more harmful in the case of accepts_nested_attributes_for.

-John

4

Nice work on the accepts nested attributes for, I've been waiting for this for a while and haven't had the time to write it.

I am curious to know about the feature for deleting attributes. For a lot of reasons, I prefer to just not send back the items I want deleted. Do you think it would make sense to add an attribute called something like deletes_when_not_present that would essentially update or create the nested objects, and then deletes any that are present still in the database that weren't in the hash? Here is the basic of the workaround I'm using...

old_ids = parent.get_children_ids parent.update(params[:parent]) old_ids.each do |oid| oid.member? parent.children_ids || Parent.destroy(oid) end

Is there a reason it was skipped?

Eloy would know for sure, but I believe it was left out because of the risk in accidentally deleting objects just because you disabled a form element, or was paginating in a form, or something similar.

Yep those were the reasons. It was removed especially because it was all highly theoretical.

We could of course add an option to accepts_nested_attributes_for which enables this. But I would suggest you'd first use your code as is for now. Once you feel you've dealt with multiple types of associations and/or edge cases then create a patch which abstracts your code.

That's just a suggestion though, any patches are welcome of course.

Cheers, Eloy

5

I can agree that its dangerous to destroy attributes in this means, but for my case there is no other way, and each record is only modified by one person at a time anyways. I realized my code above for a workaround was wrong and without the ability to know what association ids were updated its easier to skip the 2.3.2 nested_attributes.rb features and go back to the same way I've done it since pre 2.0.

So now I am using my own version called dangerous_nested_attributes.rb which includes a :destroy_missing => true property.

I put a post about it on my site, here is the link to the article =>

Here is a link to the file =>http://dangerous_nested_attributes.rb

I'm somewhat new to these groups but if there is some other way I should share my code with the community, or if I should submit it as a patch let me know.

-Josh

6

Here is the file

here is the article

7

"I can agree that its dangerous to destroy attributes in this means, but for my case there is no other way"

That's a pretty strong claim. Could you explain a little?

8

I am not using rails views for this project and instead I am using advanced custom flex (and Air) forms that operates with or without connectivity until the user saves. For those of you familir with it, there are multiple xml bound lists, and controls emulating itemrenderers, some with internal lists. The data is being sent back and fourth as xml, and the rails params hashing/netsing works wonderfully for this. It is difficult to explain but there are forms that build forms, and there are administrative interfaces to decide which forms provision which data for the particular forms.

There are two main reasons I can't send the _delete with id methods. For one the user is able to create instances of things within the flex forms, and then append children, which contain large control sets of buttons, dropdowns, etc. They can also completely remove items, then build them back.

I suppose in summary the forms themselves generate sample reports and reflect all their data in some means, and the users approve the reports and E-Sign before submission. Unfortunately the nature of my need to have a form save and completely revise its database tables is crucial to keeping that approval process accurate and keeping old references is more risky then deleting new ones since the user is then approving what is in front of them, and I send it all back as on xml object.

If this were Ajax/JS I would just do background removal of information as the user goes along, but its just too dynamic to support sending these requests....

9

Sorry, the second main reason is that (especially with XML), I feel like sending back blank tags is redundant, especially with the number of controls I have, the doctor could create and destroy hundreds of little xml objects while they are working on or updating their reading/ diagnosis, and again create as many. This potentially tags on lots of information since at the moment, I only send back the user input from activated controls.

Each control has a descriptor xml object which tells it how it should look, and what group, placement on the form. Then there is another descriptor that comes nested telling it whether or not the user has selected it, or what the user typed into it, clicked on, where it was dragged to. There are an average 300 of these controls throughout the form and its tabs and renderers. I at the moment delete the nested attributes (from my xml in flex) and add new references back in when the user selects, or deselects/destroys them, and it means the smallest amount of data transmission from the user to the sever.

I suppose my situation sits this way. I can either send only the params/xml representing the controls the user has activated. Or I could send back a params/xml block representing every control, some with ids and _deleted, others just blank. Sending back blank data for all 300+++ (as scope creeps) could would mean 2 to 10 times more data (depending on the situation) going back up to the server.

-Josh

10

first off, nested attributes are wickedly cool, so thanks for that.

i just built a form that has a bunch of STI models, each with a 'value' attribute. with STI + nested_attributes not only was i able to handle all the children in the same fashion, but i was also able to merge the new/create/edit actions. here's the view

  http://gist.github.com/91531

and the associated controller, which is of course less interesting except maybe the 'blank!' method which just hangs empty associations off of the model object

  http://gist.github.com/91532

now re-using the view for all those actions makes this a bit more complicated (but also nicer to the user), nevertheless three things stood out as being *too* hard for most people of even modest skill:

  1) building up the right ids and names - the patch i sent it to name them in a POLS fashion, which you reviewed, fixes this completely although the names/ids are still verbose there's no way around it

  2) the whole blank association thing isn't quite obvious. i have ":reject_if => {|h| h.values.all?{|v| v.blank?}}" of course, but this pattern of hanging blank objects off the parent in the right places in actions (such as when an error occurs on a post and you need the error page to have the blank objects you just rejected as they were all blank) could use some thought.

  3) finally, as josh mentions. deleting is a bitch, esp if you are intermixing valid updates with some deletes. maybe i missed an easier way but the simplest paradigm would seem to be 'copy and element rails made and munge the fuck out of it until it makes a valid deletion hash when unpacked into params'

so, on 3 i am *sort* of with josh. my 2cts, however, is that doing automatic object deletion is a *huge* deal that could make some users really *mad* if done right for all the reasons you mention. because of that i think and new AR method is the ticket: what josh and i really want is a 'replace_attributes' method. that name is so explicit, and has no un-intended side effects (like nuking stuff in the middle of a paged post) that i think it would be the best way. the impl is obvious, transaction{ nuke; update; } and i'd be happy to build a patch, but would like some feedback on the idea.

kind regards.

-a

11

On any update attributes method we don't destroy attributes that aren't present, But when an attribute is present we don't combine the old and new attribute I am not certain about has_one, but I think for sure we treat it like a real attribute, so a blank nested attribute should mean unlink, if not destroy it... (or an option for such!)

In a has many a nested attribute isn't an attribute at all, its a foriegn key reference to another table. So, maybe instead of making a total destruction patch, at least have the feature

have a accepts_nested_attributes_for something :unlink_not_present => true

This will at least remove the reference to the id in a join table.

Maybe, maybe not?

12

Josh,

Rails already have keywords for that: :nullify in associations means :unlink. So why do not do like this:

accepts_nested_attributes_for :something, :when_missing => :nullify

Those values could be: :nullify, :destroy, :destroy_all, :delete, :delete_all. Which are all the options supported by :dependent in associations.

13

Hi Josh and Ara,

I'm gonna respond inline to all thoughts/questions:

Josh,

Rails already have keywords for that: :nullify in associations means :unlink. So why do not do like this:

accepts_nested_attributes_for :something, :when_missing => :nullify

Those values could be: :nullify, :destroy, :destroy_all, :delete, :delete_all. Which are all the options supported by :dependent in associations.

I agree that if we are gonna add any of these types of linking/unlinking methods they should reflect those that AR already supports. I myself made the mistake of using _delete instead of _destroy, which is what actually happens, so that would have to be fixed as well then. Sorry :slight_smile:

On any update attributes method we don't destroy attributes that aren't present, But when an attribute is present we don't combine the old and new attribute I am not certain about has_one, but I think for sure we treat it like a real attribute, so a blank nested attribute should mean unlink, if not destroy it... (or an option for such!)

In a has many a nested attribute isn't an attribute at all, its a foriegn key reference to another table. So, maybe instead of making a total destruction patch, at least have the feature

have a accepts_nested_attributes_for something :unlink_not_present => true

This will at least remove the reference to the id in a join table.

Maybe, maybe not?

You should already be able to at least replace a single associated with a new one by not sending the :id in the attributes.

But I'd love to see improvements in this area which cover collection associations as well and in a way which makes it as transparent as possible, so for instance re-using the keywords from the AR API as suggested by José.

first off, nested attributes are wickedly cool, so thanks for that.

Glad you like it :slight_smile:

now re-using the view for all those actions makes this a bit more complicated (but also nicer to the user), nevertheless three things stood out as being *too* hard for most people of even modest skill:

  1) building up the right ids and names - the patch i sent it to name them in a POLS fashion, which you reviewed, fixes this completely although the names/ids are still verbose there's no way around it

  2) the whole blank association thing isn't quite obvious. i have ":reject_if => {|h| h.values.all?{|v| v.blank?}}" of course, but this pattern of hanging blank objects off the parent in the right places in actions (such as when an error occurs on a post and you need the error page to have the blank objects you just rejected as they were all blank) could use some thought.

By “the whole blank association thing” are you referring to instantiating new records before building a form with empty entries so the user can create new ones? If so, that seems imo to something you already have in your new/edit action flow so it shouldn't be a too big hurt, but does allow the developer as much flexibility as possible.

However, there is in fact a patch which could make this a great deal easier by Mike Breen, please verify and/or respond to the ticket if this idea would fit your (anyone's) applications: #2365 add :new_if_blank option to fields_for for nested associations - Ruby on Rails - rails

I'm not sure if you were suggesting some default option which does: ":reject_if => {|h| h.values.all?{|v| v.blank?}}"? If so, trying to abstract this from the developer went wrong quite fast, because the hash might in fact not be empty at all for various reasons while from the developers pov it was considered "blank". This can happen because you use the checkfields helper which always add a value, or have another blank nested attributes hash inside the "blank" hash, etc. Trying to come up with a way to deal with those issues would require complicated code and was at least at that time a reason for me to leave it as non ambiguous as possible, which was letting the developer handle it exactly as she wanted.

  3) finally, as josh mentions. deleting is a bitch, esp if you are intermixing valid updates with some deletes. maybe i missed an easier way but the simplest paradigm would seem to be 'copy and element rails made and munge the fuck out of it until it makes a valid deletion hash when unpacked into params'

I don't quite follow. Correct me if I misunderstood; If you have a form which contains fields for an existing record, then deleting it means setting its :id attribute to a truthy value, that seems fairly easy to me.

so, on 3 i am *sort* of with josh. my 2cts, however, is that doing automatic object deletion is a *huge* deal that could make some users really *mad* if done right for all the reasons you mention. because of that i think and new AR method is the ticket: what josh and i really want is a 'replace_attributes' method. that name is so explicit, and has no un-intended side effects (like nuking stuff in the middle of a paged post) that i think it would be the best way. the impl is obvious, transaction{ nuke; update; } and i'd be happy to build a patch, but would like some feedback on the idea.

Yes, we should _never_ turn on any automatic deletion/unlinking method by default. However, I don't think it's necessary to add an extra accessor, we simply need an implementation.

Let me put together what I have said throughout earlier similar discussions. I'm open to any of these sort of changes under the following circumstances: * It should be an extraction, not a theoretical thing. Since I didn't need that at the time it was safer to leave this kind of behaviour out and see what others who do need it come up with. (Which could have been myself as well, but the situation hasn't occurred yet.) * As transparent as possible, so the API should be familiar to developers using AR. * It should be as naive and simple as possible, ie no magic, to not obstruct a developer in creating complex forms or abstract her too far away from the metal. Because these types of forms can become complicated in ways we can never imagine and we should not limit that possibility.

So seeing the two of you actually have applications that need to deal with these situations; Yes please, a patch would be great! What a long answer huh? :wink:

Also see the following tickets which have been discussing this as well: * #1892 nested attributes should not have meaningful hash keys - Ruby on Rails - rails (near the end) * #2036 Linking and unlinking via accepts_nested_attributes_for - Ruby on Rails - rails

Cheers, Eloy

14

Eloy, maybe we should think on an API that support all cases without making things messy.

We have those actions:

  :destroy, :delete, :delete_all, :nullify

And two events:

  :request (when :_delete is sent and is default) and :missing

So my suggestion is:

  accepts_nested_attributes_for :association, :allow => :destroy

This is the same as :allow_destroy => true, except that :_destroy should be sent. We can also have more than one action:

  accepts_nested_attributes_for :association, :allow => [ :destroy, :nullify ]

If we want that to work on missing cases, we should do:

  accepts_nested_attributes_for :association, :allow => :destroy, :on => :missing

What do you think? I could help on the implementation as well.

ps: only one thing is not clear on this approach, when :on => :missing is given, we should also accept :_destroy or :_delete to be sent?