Rails Ruby on Rails

Comments and changes to this ticket

• You flagged this item as spam.
  

  cainlevy February 6th, 2009 @ 05:26 AM

  • Tag changed from “2.3, attributes, block, nested” to “2.3, attributes, block, nested, patch”

  I found time sooner than I expected. :-)

  This patch moves ids inside the attributes hash. New records are determined by the absence of hashes. The _delete key still works as before.

  I feel like this is an important change because it is necessary in order to support both modification and replacement of existing belongs_to/has_one records. Furthermore, it affects how people will build their forms and so it is important to make happen before the first public release version including nested attributes.

  I have not delved into ActionView yet to modify fields_for, but I will do that if this patch gathers enough interest.

  • move_id_inside_attributes.diff 33.9 KB

• You flagged this item as spam.
  

  cainlevy February 6th, 2009 @ 05:27 AM

  I forgot to mention: this patch also rolls in the patch from ticket #1870.

• You flagged this item as spam.
  

  Pascal Ehlert February 6th, 2009 @ 08:21 AM

  Can you give us a real world scenario where you need to replace an existing record and can not do it by adding a _delete key to the old one?

  The clear problem with your approach that it allows replacement only for one-to-one associations, has_many associations are left out.

  Besides that this new API would harder to ready than the old one and I don't like it too much in general.

  I think we decided for the other one and should stick with it if it doesn't cause us severe problem.

  How about adding a "replace" option to the accessor definition so that it will always replace the records instead of updating them if you need to?

• 

  Eloy Duran February 6th, 2009 @ 08:42 AM

  As a matter of fact I was gonna write a patch this weekend to put the id's on the inside of the attribute hashes, because it will solve some other issues. I have already looked into the form builder so this should be an easy fix.

  However, like Pascal, I'm not yet convinced about the replacing part. So a real world scenario would indeed be nice. The same goes for removing :reject_if, but I'll respond to the original ticket about this.

• You flagged this item as spam.
  

  cainlevy February 6th, 2009 @ 09:04 AM

  Has_many associations already naturally offer replacement. You simply submit one record hash with a _delete key, and another without an id key. That removes the first and adds the second. How is this a clear problem?

  The primary goal of this API should not be readability, actually. It should be to enable HTML-only nested forms. The only time readability would be important here would be with log parsing and debugging, right? In that case there's no harm in using ids or "new_XX" for the hash keys ... in this setup they can be anything at all as long as they are unique.

  When I was following the debate around http://gist.github.com/10793, most of the arguments about putting ids inside or outside the attributes were based on personal preference. This is the first time I've thought of a solid technical limitation to putting ids outside the attributes.

  If this is too theoretical, I can dream up a use case:

  Suppose a conference session has one speaker. While editing the conference session, I may wish to do any of the following:

  • create a new speaker for the session.
  • fix a typo in the name of the current speaker.
  • replace the current speaker with another, but leave the current speaker intact for other sessions.

  Note that a "replace" option would not be sufficient here since this use case requires simultaneous support of both modification and replacement.

• You flagged this item as spam.
  

  cainlevy February 6th, 2009 @ 09:06 AM

  Eloy, you snuck in while I was typing. :-)

  Does the conference session use case work for a "real world" argument?

• You flagged this item as spam.
  

  Pascal Ehlert February 6th, 2009 @ 09:42 AM

  • Assigned user set to “Michael Koziarski”

  Sounds good to me! I just couldn't think of a proper use case, but the conference example sounds good.. +1 for the change then.

• 

  Eloy Duran February 6th, 2009 @ 10:40 AM

  • Assigned user cleared.

  Hehe. Yes the conference session does work. But there is a difference in the one-to-one and one-to-many replace scenarios; In your example of the one-to-one conference session it is possible to replace but not delete a record. In the many example, however, replacing a record can indeed be done by deleting one and adding a new one, but the existing record will be destroyed and thus not left intact for possible other associations.

  I wonder if this is a problem or not… Anyways, I don't see the replacement issue as a blocker for 2.3 as it is an improvement not a fix, I'd rather wait and give this some time for people to come up with real encountered issues and the way they solved it. Or do you actually have this issue in an application?

  About the keys inside the attrs hash, yes in the discussion it seemed to be mostly about personal preference. But a real issue is that of people who use composite ids, these can not be used as index keys like integer keys can and sorting them can fail (depending on the type of composite ids). Plus for people not using HTML it becomes much less cumbersome to write the code needed to create and update records.

  The last question for me would be to choose between:

  
  { 1 => { :id => '23', :name => 'joe the plumber' }}
  

  Or:

  
  [{ :id => '23', :name => 'joe the plumber' }]
  

  I feel the last one is preferable as it completely removes the need for an ID to be generated, which is an issue when not using the form helper.

  My limited experiments have shown that both alternatives can be done easily. The first one would have HTML like:

  
  <input id="post_comments_attributes_0_id" name="post[comments_attributes][0][id]" type="hidden" value="1" />
  <input id="post_comments_attributes_0_name" name="post[comments_attributes][0][name]" size="30" type="text" value="comment #1" />
  <input id="post_comments_attributes_1_id" name="post[comments_attributes][1][id]" type="hidden" value="2" />
  <input id="post_comments_attributes_1_name" name="post[comments_attributes][1][name]" size="30" type="text" value="comment #2" />
  

  And the second one which Rails parses as an array:

  
  <input id="post_comments_attributes__id" name="post[comments_attributes][][id]" type="hidden" value="1" />
  <input id="post_comments_attributes__name" name="post[comments_attributes][][name]" size="30" type="text" value="comment #1" />
  <input id="post_comments_attributes__id" name="post[comments_attributes][][id]" type="hidden" value="2" />
  <input id="post_comments_attributes__name" name="post[comments_attributes][][name]" size="30" type="text" value="comment #2" />
  

  So it seems the parser creates a new hash when it encounters a key which has been used before. Can anyone think of a problem with this approach?

• 

  Eloy Duran February 6th, 2009 @ 12:24 PM

  There's at least one problem with the second option in my last comment, which is that there shouldn't be any duplicate element ids. But that seems an easy to solve problem.

• 

  DHH February 6th, 2009 @ 02:09 PM

  • Assigned user set to “Michael Koziarski”

• You flagged this item as spam.
  

  Ryan Bates February 6th, 2009 @ 04:13 PM

  I much prefer to use a hash with unique keys when passing nested attributes through a form. Here are a few reasons.

  • the id and name of each form field element will be unique making it pass validation and also easier to reference elements through javascript.
  • the order of the nested models can be ensured (sort by the key)
  • possible to handle deeply nested associations

  The last one is the big one. With the array approach (the empty square brackets) it is not possible to deeply nest model associations.

  That said, I'm okay if both approaches 3 & 4 are supported. If a hash is supplied it would be converted to an array, where the elements are sorted by the hash key. In that case the id would always be listed in the attributes.

• You flagged this item as spam.
  

  Ryan Bates February 6th, 2009 @ 04:16 PM

  Oops, I meant to say I'm okay with supporting both approaches 1 & 4 where the nested attributes could either be given as an array or a hash. If it's a hash, the key would be arbitrary, and only used to sort the elements when converting to an array.

• 

  Eloy Duran February 6th, 2009 @ 04:51 PM

  Ryan, thanks for your thoughts!

  Ok, so I will support Hash and Array in the model and use the hash variant in the form to support ids and to handle deeply nested associations.

• You flagged this item as spam.
  

  cainlevy February 6th, 2009 @ 08:34 PM

  This new patch supports arrays as well as hashes. I modified the documentation to prefer the array approach, so people aren't distracted by the hash keys. :-) Although I'm still not clear what the non-HTML scenario is ... an API call? If I'm in Ruby, I'm just going to play with the ActiveRecord objects myself.

  @Eloy: Regarding your question about being able to destroy a has_one record while replacing, I think the HasOneAssociation#replace method supports that if :dependent is configured?

  This patch still doesn't have the :reject_if option. That may or may not need to be changed, depending on final response to ticket #1870.

  • move_id_inside_attributes.diff 39.7 KB

• You flagged this item as spam.
  

  cainlevy February 7th, 2009 @ 01:01 AM

  This updated patch keeps the :reject_if option.

  • move_id_inside_attributes.diff 41.7 KB
• 

  Eloy Duran February 7th, 2009 @ 01:43 PM

  @Lance: Could you please rebase your patch? It unfortunately does not apply for me. Hope you read this soon as I actually wanted to fix this today…

  I think I wasn't completely clear on my question about replacing a record I think; What I meant is that your idea allows a one-to-one associated record to be replaced and not deleted. And you said that replacing a record in a many association was already possible. However, that's not completely the case. It's only possible to replace a record by deleting one and adding one. It's not possible to replace a record and not have it destroyed.

• 

  Eloy Duran February 7th, 2009 @ 01:45 PM

  Oh and yes the non HTML scenario is API calls :)

• You flagged this item as spam.
  

  cainlevy February 7th, 2009 @ 08:20 PM

  Ahhh, yes. There is no way to simply disassociate a record from a has_many.

  I wonder if that could be controlled by :dependent => :destroy as well? For example:

  • :dependent => :nullify -- the _delete flag would disassociate records
  • :dependent => :destroy -- the _delete flag would destroy records
  • :dependent => nil -- default behavior, probably same as :nullify

  That could take care of has_many, but not habtm, right?

  Pascal's idea to use a replace flag could also work. Would need to think about how that affects the UI. A developer would probably want to pick between a replace flag or a _destroy flag on a per-form basis.

  I assume this is a conversation we should take somewhere else?

• 

  Michael Koziarski February 7th, 2009 @ 10:14 PM

  • Milestone cleared.
• 

  Eloy Duran February 8th, 2009 @ 01:26 PM

  Applied Lance’s changes and made the FormBuilder work with it and also fixed a bug with composite ids in the db.

  Should be good to go. I have updated my complex-form-examples fork as well for this change.

  • move_ids_inside_attrs_hash_and_fixed_composite_ids.diff 48.5 KB
• 

  Eloy Duran February 8th, 2009 @ 01:28 PM

  Btw, the patch has been normalized, so if applied please add Lance Ivy to the changelog as well :)

• 

  Eloy Duran February 8th, 2009 @ 01:30 PM

  @Lance & Pascal: Yes please open a new ticket and possibly compile your thoughts/ideas on the replacing/destroying of records.

• 

  Repository February 13th, 2009 @ 09:32 AM

  • State changed from “new” to “committed”

  (from [5dbc9d40a49f5f0f50c2f3ebe6dda942f0e61562]) Changed API of NestedAttributes to take an array, or hash with index keys, of hashes that have the id on the inside of the attributes hash and updated the FormBuilder to produce such hashes. Also fixed NestedAttributes with composite ids.

  Signed-off-by: Michael Koziarski michael@koziarski.com Signed-off-by: Eloy Duran eloy.de.enige@gmail.com [#1892 state:committed] http://github.com/rails/rails/co...

• 

  Eloy Duran February 23rd, 2009 @ 08:37 AM

  @Lance & Pascal: You guys might want to chip in on #2036. It's about linking/unlinking records in a habtm association, which would probably overlap with the replace functionality you guys were discussing as well.

• You flagged this item as spam.
  

  Beren February 23rd, 2009 @ 10:59 PM

  How do I go about installing this patch? (I'm very new to rails and this error comes up when going through the very first tutorial.

• 

  Ryan Bigg October 9th, 2010 @ 09:56 PM

  • Tag cleared.

  Automatic cleanup of spam.

• 

  Jeff Kreeftmeijer November 7th, 2010 @ 04:51 PM

  Automatic cleanup of spam.

• 

  Ryan Bigg November 8th, 2010 @ 01:52 AM

  Automatic cleanup of spam.