[PATCH] refactored UrlEncodedPairParser to handle array params in depth-first order

works for me +1

awesome.. I applied your patch and it fixed my prototype form problem(s). Seems stable (2 days of usage) and exhibits (what I would consider) expected behaviour. Thanks.

Works like a charm here, too. Please consider merging it into the core.

Replying to svoop:

Works like a charm here, too. Please consider merging it into the core.

Sorry, have to correct that: All params are present, but the order is lost. (I haven't tried it on Ruby 1.9 which maintains Array member order though.) Here's an example:

Relations:
Project has_many Tasks, Task has_many Events

Nested Forms:
- project
-- task1
--- event1
--- event2
-- task2
--- event3
--- event4

Expected Params:
project: task1: event1, event2 / task2: event3, event4

Received Params:
project: task1: event1 / task2: event2, event3, event4

To lastubelus:

If you have a spare minute, can you tell me if the issue I describe on http://www.ruby-forum.com/topic/136569 is indeed related to the UrlEncodedPairParser and your patch?

You can reach me as "echo65 {AT} delirium {PNT} ch" - thanks a lot!

This fixes my problem and works well for me.

+1

Hi svoop,

Unfortunately I missed the mail from your comment on http://dev.rubyonrails.org/ticket/10101 so I only saw it today.

The issue you described is definitely related to the issue/ticket I posted.

To understand why my parser is not working for you I would need to see the rails controllers & views.

One thing to be aware of is that a restriction of using my parser is that every row in an array of elements must contain the complete set of attributes being used in all the rows.

That's a difficult sentence to parse, so I'll give an example:

<form>

<div>

<input name=bob[][a] value="a1"> <input name=bob[][b] value="b1">

</div> <div>

<input name=bob[][a] value="a2">

</div> <div>

<input name=bob[][a] value="a3"> <input name=bob[][b value="b3"]>

</div>

</form>

this WONT WORK properly, because my parser requires the array to not have any holes. This will parse with my parser as:

{ :bob => [ { :a => 'a1', :b => 'b1' }, { :a => 'a2', :b => 'b3' }, { :a => 'a1'} ] }

Unfortunately, the rails checkbox hack will not work with an array of elements with either my parser OR the original parser. Because there are no row markers to indicate to the parser where rows start & end, there is no way to tell that multiple params with the same name should be associated with a single row.

Because I am currently working on a project where the spec mandates heavy use of nested resources, I am working on a fix for this over the next couple days.

The way my fix will work is instead of putting a hidden field after a checkbox field, it will put it before, and it will prepend the name with an exclamation mark:

<input type="hidden" value="0" name="bob[][!test]"> <input type="checkbox" value="1" name="bob[][test]">

When submitted, the parser will use the value of bob[][!test] if there is no bob[][test] before the next bob[][!test]

It should be ready by Jan 15 and I will post it here.

Also it is possible there may be issues with attributes on objects in an array that useselect with multiple=true, on either or both my parser and the original parser. After the checkboxes are handled, I will look at select#multiple=true and make sure it works properly as well with my parser.

If anyone wants to submit test cases -- either as a patch or as pseudocode detailed enough to be implementable, I would welcome them.

Hi lastobelus

To understand why my parser is not working for you I would need to see the rails controllers & views.

I have two complex forms nexted into one another:

offer (_form) -> _combo -> _condition

Thus an offer has many combos and a combo has many conditions. Following the Railscast on complex forms, the form elements get the following names (assuming there's a "title" column for offer, combo and condition in Db):

offer[title] offer[combo_attributes][][title] offer[combo_attributes][][condition_attributes][][title]

And as I'm using lists of checkboxes on the condition level, the "worst" name to appear is even:

offer[combo_attributes][][condition_attributes][][service_ids][]

I understand the "holes" problem and thus made sure that no holes are present on any level. Still, the parser does not like such deep nesting and returns bad results.

As a workaround, I planted a onSubmit javascript that adds indexes into the names:

offer[combo_attributes][0][condition_attributes][0][service_ids][]

This way the parser correctly mirrors the hierarchy in the params hash, however, with hashes rather than arrays. But two lines in the controller reverse this and the resulting params hash does work for ActiveRecord.

Because there are no row markers to indicate to the parser where rows start & end, there is no way to tell that multiple params with the same name should be associated with a single row. ... The way my fix will work is instead of putting a hidden field after a checkbox field, it will put it before, and it will prepend the name with an exclamation mark:

Sounds easy enough to implement in the checkbox form helper.

For now, I have a working solution. But still, the parser seems to be a weak point in current Rails, so I hope a better implementation (maybe yours?) makes it into Rails at some point as nested complex forms come in increasingly handy.

I'll keep an eye on this and will give your parser a shot with my app at a later point. Right now, I have to finish it to make the client happy :-)

Hi Everyone,

I have applied your patch and am trying to run the following.

<% for contact in @applicant.contacts %>

<% fields_for "applicant[contacts][]", contact do |contact_form| %>

<label for="Contact Name"><%= contact.contact_type %> Name:</label> <%= contact_form.hidden_field :contact_type %> <%= contact_form.text_field :full_name %><br/> <label for="Contact Name"><%= contact.contact_type %> Title:</label> <%= contact_form.text_field :title %><br/>

<% for phone_number in contact.phone_numbers %>

<% fields_for "applicant[contacts][][contact_phone_numbers][]", phone_number do |phone_form| %>

<label for="Phone_numbers"><%= phone_number.name %>:</label> <%= phone_form.hidden_field :name %> <%= phone_form.text_field :number %><br/>

<% end %>

<% end %>

<% end %>

<% end %>

This posts the following:

{"commit"=>"Save changes",

"credit_application"=>{"contacts"=>[{"title"=>"title 1", "contact_type"=>"Principal", "contact_phone_numbers"=>[{"name"=>"Phone Number"}], "full_name"=>"name 1"},

{"title"=>"title 2", "contact_type"=>"Receiving Contact", "contact_phone_numbers"=>[{"number"=>"phone 1"}], "full_name"=>"name 2"}, {"title"=>"title 3", "contact_type"=>"Accounts Payable", "contact_phone_numbers"=>[{"name"=>"Fax Number"}], "full_name"=>"name 3"}, {"contact_phone_numbers"=>[{"number"=>"fax 1"}]}, {"contact_phone_numbers"=>[{"name"=>"Phone Number"}]}, {"contact_phone_numbers"=>[{"number"=>"phone 2"}]}, {"contact_phone_numbers"=>[{"name"=>"Fax Number"}]}, {"contact_phone_numbers"=>[{"number"=>"fax 2"}]}, {"contact_phone_numbers"=>[{"name"=>"Phone Number"}]}, {"contact_phone_numbers"=>[{"number"=>"phone 3"}]}, {"contact_phone_numbers"=>[{"name"=>"Fax Number"}]}, {"contact_phone_numbers"=>[{"number"=>"fax 3"}]}]}}

I believe this is the same problem that svoop is having. Is there any way to get around this? When I change the second set of parameters with the following:

<% fields_for "applicant[contacts][x][contact_phone_numbers][]", phone_number do |phone_form| %> where x is the iteration i am on for the first loop of contacts i get an expected hash and not array.

Thanks for all of your help!

@ svoop & lundie:

Can you guys please post the raw query string that is being submitted by your forms? To do that, put a debug statement in actionpack/lib/action_controller/request.rb as the first line of the parse_query_parameters method:

class << self

def parse_query_parameters(query_string)

RAILS_DEFAULT_LOGGER.debug(".mj. request.rb - 405: #{CGI.unescape(query_string)}") return {} if query_string.blank? ...

I need to be able to reproduce what the UrlEncodedPairParser is seeing.

ok I wrote a test with three-levels of nested attributes and verified that my parser is failing when that deeply nested (the original parser would fail too.)

I thought I had tested that, but my test only had one element in first level of nesting, which passes, but multiple elements with multiple levels of nesting is failing.

I'll work on fixing that.

In the meantime, I've attached the patch that implements the new checkbox hack. Unless you are dealing with the checkbox's hidden field counterpart directly, it should be transparent.

I've also added an indexes_id_only option, so that you can pass in the array index and have unique ids on all your fields, while having the names still have empty [].

after thinking about it a little, it is impossible to make either parser handle more than one level of nesting without introducing meta-information.

Consider:

<{"a"=>

{"b"=>

[{"c"=>{"d"=>["1a", "1b", "1c"]}},

{"c"=>{"d"=>["2a", "2b", "2c"]}}, {"c"=>{"d"=>["3a", "3b", "3c"]}}],

"x"=>"hi"}}>

and

<{"a"=>

{"b"=>

[{"c"=>{"d"=>1a?}},

{"c"=>{"d"=>1b?}}, {"c"=>{"d"=>1c?}}, {"c"=>{"d"=>2a?}}, {"c"=>{"d"=>2b?}}, {"c"=>{"d"=>2c?}}, {"c"=>{"d"=>3a?}}, {"c"=>{"d"=>3b?}}, {"c"=>{"d"=>3c?}}],

"x"=>"hi"}}>

for a params parser to be able to distinguish between them, you would have add meta-information to demarcate where the row boundaries are.

There are a couple possible alternatives:

1. use the index parameter, explicitly. Both my & the original parser will then parse the rows as dictionaries. Add a utility function to controller that converts hashes with only numeric keys to arrays. This function would be called explicitly in the actions that need multiply nested attributes. This would potentially collide with using the auto-index functionality, but since it is only done explicitly and not automatically thats alright.

one issue with doing this is that it makes designing the ajax functions that add new empty rows in a nested association difficult to set-up, as you have to keep track of the row indices in javascript.

2. use a special notation in the index parameter, that the parser understands and builds arrays instead of dictionaries. If the parser ignored the actual index and pushed each element onto the array as it was encountered, you could add new rows with javascript and just use [] with no index, and everything would just work. DOM re-ordering would also work. However, this is going to make the patch with the new parser even harder to sell to the core maintainers. As long as the special notation was something that would normally never be encountered in the rails magic way of doing things because it makes an illegal identifier, this would be a completely transparent change. I would recommend using # prepended to the index.

I am going to implement option 2 and make it available as an incremental patch on my first patch.

oops, forgot to blockquote my examples:

{"a"=>
  {"b"=>
    [{"c"=>{"d"=>["1a", "1b", "1c"]}},
     {"c"=>{"d"=>["2a", "2b", "2c"]}},
     {"c"=>{"d"=>["3a", "3b", "3c"]}}],
     "x"=>"hi"}}

cannot be distinguished from

{"a"=>
  {"b"=>
    [{"c"=>{"d"=>["1a"]}},
     {"c"=>{"d"=>["1b"]}},
     {"c"=>{"d"=>["1c"]}},
     {"c"=>{"d"=>["2a"]}},
     {"c"=>{"d"=>["2b"]}},
     {"c"=>{"d"=>["2c"]}},
     {"c"=>{"d"=>["3a"]}},
     {"c"=>{"d"=>["3b"]}},
     {"c"=>{"d"=>["3c"]}}],
   "x"=>"hi"}}

by either parser

That would be great. I am going to wait, and try your patch before I attempt to even try option 1. Do you have an estimate as to how long before you have something up?

Thanks again for all of your help. It's really appreciated!

I have posted a cleaned up patch. I think this version of the new parser provides a good solution for nesting attributes.

The second patch, properly_handle_depth_first_array_param_order_as_from_prototype_form_serialize.r8628.diff did not actually have the form_helper component of the revised checkbox hack -- the new patch does. Sorry for the omission.

The new patch:

• handles a single level of nesting (array) with the array elements in either depth-first or breadth-first order
• allows explicit array indexing, which allows multiple levels of nesting arrays, by supplying the array indexing. The parser is forgiving of holes when explicit indices are provided. Also, if for a particular element no index is provided, it will be pushed onto the end of an existing array. To use this feature, prepend your array indices with "#" and pass them to the form helpers with :index.
• allows the option to add indexes to the html id only, not the name. This allows you to use the object[subobject][] syntax with an array, and get unique ids on your elements. To use this use :index => ix, :indexes_id_only => true
• makes better ids when using arrays, by replacing ][ with a single underscore

An example of how to do deep nesting

This example is taken from the test app that I have also attached

<div class="l1">
Bob: <%= bob_form.check_box :test %>
  <% @bob.things.each_with_index do |thing, thing_ix| -%>
    <div class="l2">
      <% fields_for "bob[things][]", thing do |thing_fields| %>
          <%= thing_fields.text_field :name, :index => "##{thing_ix}" %>
          <%= thing_fields.check_box :test, :index => "##{thing_ix}"  %><br />
          <% thing.cubbyholes.each_with_index do |cubbyhole, cubbyhole_ix| -%>
            <div class="l3">
              <% fields_for "bob[things][##{thing_ix}][cubbyholes][]", cubbyhole do |cubbyhole_fields| %>
                  <%= cubbyhole_fields.text_field :name, :index => "##{cubbyhole_ix}" %>
                  <%= cubbyhole_fields.check_box :test, :index => "##{cubbyhole_ix}"  %><br />
                  <% cubbyhole.marbles.each_with_index do |marble, marble_ix| -%>
                    <div class="l4">
                      <% fields_for "bob[things][##{thing_ix}][cubbyholes][##{cubbyhole_ix}][marbles][]", marble do |marble_fields| %>
                          <%= marble_fields.text_field :name, :index => "##{marble_ix}" %>
                          <%= marble_fields.check_box :test, :index => "##{marble_ix}"  %><br />
                      <% end %>
                    </div>
                  <% end -%>
              <% end %>
            </div>
          <% end -%>
      <% end %>
    </div>
  <% end -%>
</div>

Test App

I've attached a test app for playing with nested attributes & checkboxes. To use it, download, then freeze rails, then apply the patch, which is included in the lib/patches dir of the test app.

parses single nested array params depth-first or breadth-first. parses multiply nested array params by recognizing explicit index format '#0', '#1' etc. Revises checkbox hack to use a prefix on hidden field name so parser can correctly parse depthfirst or breadthfirst.

test app for testing new parser. to use, freeze rails then apply patch in lib/patches

I have been using this for about a week without any issues. I will let you know if I come up with anything. Thanks for your work!

+1

wow, thanks for supplying a patch to that. I discovered deep form nesting recently, and it really rocks, especially with make_resourceful controller macros. The patch is against release 1.2, isn't it?

I need the patch functionality for a 2.0.1 project, so is it advisable to wait a few days for a new patch or reimplement your changes in 2.0.1?

just pretend I never wrote that, applied the patch correctly and everything just works. really good work, I'm hoping so much this patch makes it to the next release!

+1

Worked great for me, but it looks like the "labelled_form_for" helper method changed since then and was rejected.

PLEASE commit this!

We really need this new check_box hack, because check_boxes inside a fields_for don't work. If you have a view like the one below, and you check the first two check_boxes, the current check_box hack breaks into a new "row" when it sees the different value between the checked box, and the hidden field.

Look at this view, then the resulting params below.

<% form_for(@post) do |f| %>
  <p>
  <%= f.text_field :name %><br />
  
  <% fields_for "post[add_comments][]", Comment.new do |cf| %>
    <%= cf.text_field :text %>
    <%= cf.check_box :one %>
    <%= cf.check_box :two %>
    <%= cf.check_box :three %>
  <% end %>
  </p>
  <p>
    <%= f.submit "Create" %>
  </p>
<% end %>

Edited for clarity, to show it produces 3 "comments" instead of 1.

Parameters: {"commit"=>"Create", "post"=>{"name"=>"The Name", 

"add_comments"=>[{"text"=>"The Text", "one"=>"1"}, {"two"=>"1", "one"=>"0"}, {"two"=>"0", "three"=>"0"}]},

"authenticity_token"=>"...", "action"=>"create", "controller"=>"posts"}

+1

This solves the described problem and is working fine for me aswell.

+1

This works for me too. This is pretty important for our application.

+1

One of the applications I'm working on is due for an upgrade to the latest edge rails soon, and I will look at this patch then, and create a new patch that works with the latest edge and incorporates the fix in 10797 (not sure how that patch will affect my parser, have to test it)

Since the nested entity functionality has been important to every client I've had for rails applications so far, I will continue supporting it even if it doesn't get integrated into the core.

Fixes to the current parser may make it sort of work for nested entities, but it will never work if you have checkboxes in your nested entities using the current checkbox hack.

+1 works great. Would use again.

+1 I tried it and it worked. Please include this in the next release

Has anyone had luck with this patch, and form_remote_for? Im not sure if it is related, but as soon as I attempt to submit in an ajax call, I begin to receive:

Status: 500 Internal Server Error

Conflicting types for parameter containers. Expected an instance of Array but found an instance of HashWithIndifferentAccess. This can be caused by colliding Array and Hash parameters like qs[]=value&qs[key]=value.

Does it make sense that this has to do with this patch? or am I doing something else incorrectly?

Thanks!

it is possible and I would like to see a (simplest possible) example of code that creates this problem so I can investigate & hopefully fix it.

This does not work, and gives the following error:

Status: 500 Internal Server Error

Conflicting types for parameter containers. Expected an instance of Array but found an instance of HashWithIndifferentAccess. This can be caused by colliding Array and Hash parameters like qs[]=value&qs[key]=value.

<% form_remote_for :member, :url => {:action => 'update', :id => @member.id}, :update => 'company_information' do |f| %> 

	<% for phone_number in @member.phone_numbers %>
		
		<% fields_for "member[phone_number_attributes][]", phone_number do |phone_form| %>
			<div class="field_title"><%=phone_number.name%></div>
			<%= phone_form.hidden_field :name %>
			<div class="field_value"><%= phone_form.text_field :number, :index => nil, :class => "full_length" %></div>
			<% if ! phone_number.id.nil? %>
        		<%= phone_form.hidden_field :id, :index => nil %>
      		<% end %>
			
		<% end %>
	<% end %>

<%= submit_tag 'Save Member' %>
<% end %>

But if i use this same code in a normal form, it works fine:

<% form_for :member, :url => {:action => 'create_member'} do |f| %> 
	<% for phone_number in @member.phone_numbers %>
		
		<% fields_for "member[phone_number_attributes][]", phone_number do |phone_form| %>
			<div class="field_title"><%=phone_number.name%></div>
			<%= phone_form.hidden_field :name %>
			<div class="field_value"><%= phone_form.text_field :number, :index => nil, :class => "full_length" %></div>
			<% if ! phone_number.id.nil? %>
        		<%= phone_form.hidden_field :id, :index => nil %>
      		<% end %>
			
		<% end %>
	<% end %>

<%= submit_tag 'Save Member' %>
<% end %>

Thanks again!

A workaround for some of the multiple-model forms I'm working on is to specify an index instead of relying on the parsing of the empty brackets [].

For example:

 <% @product.blurbs.each do |blurb| %>
 <% fields_for "product[add_multiple_blurbs][]", blurb do |blurb_form| %>

Becomes:

 <% @product.blurbs.each_with_index do |blurb, blurb_index| %>
 <% fields_for "blurbs[#{blurb_index}]", blurb do |blurb_form| %>

Sorry, my example here is also different in that it doesn't rely on a virtual attribute "add_multiple_blurbs" any more either.

Take a look at the server log, because the submitted params come in the form of a hash instead of an array, but you can then use the hash values in the controller as your array of params.

I'm trying to get the go ahead to spend a couple days re-integrating this patch with the latest rails and having a look at this issue while I do it. It may not happen this week, but it will happen :)

Removing verified while we wait for lastobelus to do the reintegrating.

Hi!

Although I said a few weeks ago that this patch worked perfectly for me, I've found that it breaks when using checkboxes in multipart forms. The problem with in the added prefix on the hidden field name of checkboxes. I'll try to explain myself.

Before applying this patch, when a checked checkbox was submitted in a multipart form, this was what the server got ("in_stock" is the checkbox field):

{"product[in_stock]" => [1,0], "product[name]" => "My Product"}

that is, the parameter "product[in_stock]" got 2 values, which were later parsed by the request processor to ignore the second one (the order could be trusted since both values are in the same array)

However, once the patch is applied, this is what the server gets for the same form:

{"product[!in_stock]" => 0, "product[name]" => "My Product", "product[in_stock]" => 1}

That is, as the name of the field is no longer the same (in_stock vs. !in_stock) it's splitted in two separate parameters, whose order can no longer be assured since we are dealing with a hash. The modified parser assumes "!in_stock" goes right after "in_stock", which is not always true as I already said.

I don't very well understand the reason for the addition of this prefix, but I found the way to maintain it and have the request processor process checkboxes correctly just replacing 1 line of code. Here it is:

In /vendor/rails/actionpack/lib/action_controller/request.rb line 656, replace

if next_key == @last_key

with

if pointer.is_a?(Hash) && pointer[prev_key] && pointer[prev_key].is_a?(Hash) && pointer[prev_key].keys.include?(next_key)

In other words, instead of checking weather the name of the previous field is equal to the current one (without the prefix), we check wheather the output already has a value for that key.

Unfortunately, this won't make checkboxes work with an array of elements, which doesn't work neither with the original parser nor with lastobesus parser (as he commented in a previous comment: http://dev.rubyonrails.org/ticket/10101#comment:7)

I hope I made myself clear.

It's the first time that I contribute something to the community, so if I should to it in any other way, please tell me how.

Is there any chance of getting this patched into rails 2.1?

+1

This patch *IS* quite important, but it looks like it needs to be rebased against HEAD. Can the original author rebase the patch?