For review: rewrite UrlEncodedPairParser

http://rails.lighthouseapp.com/projects/8994-ruby-on-rails/tickets/1142-simplified-parameter-parsing-codehttp://rails.lighthouseapp.com/projects/8994-ruby-on-rails/tickets/1142-simplified-parameter-parsing-code

Based on the prototype plugin, form_collections.git:

http://www.vector-seven.com/git/rails/plugins/form_collections.git/

Copying the description from the ticket:

Something else I meant to ask when posting this:

Does anybody know why on earth ActionController::AbstractRequest::parse_request_parameters expects the value of each element of the "params" parameter to be an Array?

Cheers, T

Thomas Lee wrote:

The semantic changes are as follows:

INPUT: "a=5&a[y]=10" OLD PARSER OUTPUT: {"a" => [{"x" => "5", "y" => "10"}]} NEW PARSER OUTPUT: {"a" => [{"x" => "5"}, {"y" => "10"}]}

INPUT: "a[0]=5&a[0][y]=10" OLD PARSER OUTPUT: {"a" => {"0" => {"x" => "5", "y" => "10"}}} NEW PARSER OUTPUT: {"a" => [{"x" => "5"}, {"y" => "10"}]}

INPUT: "a[0]=5&a[1][y]=10" OLD PARSER OUTPUT: {"a" => {"0" => {"x" => "5"}, "1" => {"y" =>
"10"}}} NEW PARSER OUTPUT: {"a" => [{"x" => "5"}, {"y" => "10"}]}

I'm curious as to the motivation behind some of these changes. In
particular the last two - is it not desirable to be able to build up
an array of hashes keyed by some value (whether that value happens to
be an index, an id or something else is a bit immaterial as far as I'm
concerned). ?

Fred

Thomas Lee wrote:

Something else I meant to ask when posting this:

Does anybody know why on earth ActionController::AbstractRequest::parse_request_parameters expects the value of each element of the "params" parameter to be an Array?

That's what the cgi.parameters api returns:

  Parameters      The method #params() returns a hash of all parameters in the      request as name/value-list pairs, where the value-list is an Array      of one or more values. The CGI object itself also behaves as a hash      of parameter names to values, but only returns a single value (as a      String) for each parameter name.

When these issues (with the original parser) were first raised because of prototype.js parser being different, and yet yielded the wanted results, I first opposed them. Lately I’ve seen how they could tighten up and improve complex forms.

Having said that, I’m worried about backwards-compatibility. I don’t think a single parser can support both schemes – or am I wrong?

Still, should there be a switch one can toggle to start using the new parser after Rails 2.2?

Hi Fred,

Thanks for the response. Counter-arguments and flamebait below. :slight_smile:

The semantic changes are as follows:

INPUT: "a=5&a[y]=10" OLD PARSER OUTPUT: {"a" => [{"x" => "5", "y" => "10"}]} NEW PARSER OUTPUT: {"a" => [{"x" => "5"}, {"y" => "10"}]}

INPUT: "a[0]=5&a[0][y]=10" OLD PARSER OUTPUT: {"a" => {"0" => {"x" => "5", "y" => "10"}}} NEW PARSER OUTPUT: {"a" => [{"x" => "5"}, {"y" => "10"}]}

First up, I made a typo in the second example. I've updated the ticket to reflect this. Sorry for the confusion:

INPUT: "a[0]=5&a[0][y]=10" OLD PARSER OUTPUT: {"a" => {"0" => {"x" => "5", "y" => "10"}}} NEW PARSER OUTPUT: {"a" => [{"x" => "5", "y" => "10"}]}

INPUT: "a[0]=5&a[1][y]=10" OLD PARSER OUTPUT: {"a" => {"0" => {"x" => "5"}, "1" => {"y" => "10"}}} NEW PARSER OUTPUT: {"a" => [{"x" => "5"}, {"y" => "10"}]}

I'm curious as to the motivation behind some of these changes.

Because Rails sucks at complex forms and I'd prefer it not to. :slight_smile:

In particular the last two - is it not desirable to be able to build up an array of hashes keyed by some value (whether that value happens to be an index, an id or something else is a bit immaterial as far as I'm concerned). ?   

If you're working with ActiveRecord and you want to specify an id, take the following example into consideration:

"parent[children][0][id]=1234&parent[children][0][name]=bob&parent[children][0][status]=ACTIVE&parent[children][1][name]=new&parent[children][1][status]=ACTIVE"

With the new parser, this would parse to:

{"parent" => {"children" => [{"id" => "1234", "name" => "bob", "status" => "ACTIVE"}, {"name" => "bob", "status" => "ACTIVE"}]}}

In this manner, the elements of "children" are both logically grouped and ordered. Better still, this is exactly how the has_many association is currently modelled in ActiveRecord (i.e. has_many associations are arrays of ActiveRecords). Further, if no "id" attribute is set for any given element of "children", one can assume it is a new object rather than an update to an existing object. Think back to the :accessible option on ActiveRecord from a while back: would behaviour like this not have been useful?

How would you do any of this with the current parser?

I'd encourage you -- and anybody else doubting its utility -- to *try* the patch or the plugin in a simple application. I would be interested to know if you honestly thought the old parser was a better choice.

Feel free to email me directly if you have any questions or problems.

Cheers, T

Hi Mislav,

By adding a little hack here and there, I can get very close to the original semantics such that only one or two tests would fail. In the patch, I opted to change the tests rather than the parser code to get them passing since most of the failing tests are checking the behaviour of the parser on invalid inputs (where I'd argue the garbage in, garbage out adage applies). Also, I wanted to avoid introducing hacks to the new parser before it was deemed necessary.

I agree though, backwards compatibility is important. I'm perfectly willing to concede that changes will need to be made to assure some level of backwards compatibility, although I'd still argue that testing the output of the parser for any set of invalid inputs is a waste of time since the options are:

1. Bad output. 2. No output. 3. An error.

I don't think a single parser could support both schemes due to the old parser's treatment of "foo[0]=5" as {"foo" => {"0" => "5"}}, and the new parser's treatment of the same thing as {"foo" => ["5"]}. Having said that, maybe it could be a Rails::Configuration setting if need be?

Cheers, T

Mislav Marohnić wrote:

Michael Koziarski wrote:

Thomas Lee wrote:   

Something else I meant to ask when posting this:

Does anybody know why on earth ActionController::AbstractRequest::parse_request_parameters expects the value of each element of the "params" parameter to be an Array?      That's what the cgi.parameters api returns:

  Parameters      The method #params() returns a hash of all parameters in the      request as name/value-list pairs, where the value-list is an Array      of one or more values. The CGI object itself also behaves as a hash      of parameter names to values, but only returns a single value (as a      String) for each parameter name.

Had a feeling it might be something like that. Thanks for the info!

Cheers, T

I've posted my feedback as a Lighthouse comment.

For anybody interested, this patch has been updated such that the new UrlEncodedPairParser implementation is pluggable (i.e. applications that rely on the behavior of the old parser can now opt to use the old parser).

This is the best of both worlds: forms become easier for developers to manipulate in new applications, but developers who rely on the old behaviour can keep their existing applications working as normal.

See this lighthouse post for the patch & the details:

http://rails.lighthouseapp.com/projects/8994/tickets/1142-simplified-parameter-parsing-code#ticket-1142-8

Any further feedback would be much appreciated.

Cheers, Tom

Thomas Lee wrote: