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:
------------------
* This patch was written with the intent of simplifying the
implementation of UrlEncodedPairParser, as well as making its
behaviour somewhat more predictable/logical.
* Changes to semantics are minimal: only breaks seven actionpack
unit tests which are either testing broken inputs or strange
behaviour in the old parser.
* Of those seven unit tests, five can be made to pass with some
minimal changes to the parser.
* New semantics would make nested forms and/or manipulation of
collections in forms much easier to deal with.
------------------------------------------------------------------------
The semantic changes are as follows:
INPUT: "a[][x]=5&a[][y]=10"
OLD PARSER OUTPUT: {"a" => [{"x" => "5",
"y" => "10"}]}
NEW PARSER OUTPUT: {"a" => [{"x" => "5"},
{"y" => "10"}]}
INPUT: "a[0][x]=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][x]=5&a[1][y]=10"
OLD PARSER OUTPUT: {"a" => {"0" => {"x"
=> "5"}, "1" => {"y" => "10"}}}
NEW PARSER OUTPUT: {"a" => [{"x" => "5"},
{"y" => "10"}]}
------------------------------------------------------------------------
Issues to be resolved:
1. Should the parser be modified to pass the old tests for invalid
inputs?
2. "a[0]=1&a[999]=2" currently yields a 1000 element array with
998
nil values. Should we implement a SparseArray class that minimizes
memory usage for such a situation, or force sequential indexes?
-----------------
Would appreciate any feedback and/or recommendations.
Cheers,
Tom
--~--~---------~--~----~------------~-------~--~----~
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
-~----------~----~----~----~------~----~------~--~---
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:> 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: > > ------------------ > > * This patch was written with the intent of simplifying the > implementation of UrlEncodedPairParser, as well as making its > behaviour somewhat more predictable/logical. > * Changes to semantics are minimal: only breaks seven actionpack > unit tests which are either testing broken inputs or strange > behaviour in the old parser. > * Of those seven unit tests, five can be made to pass with some > minimal changes to the parser. > * New semantics would make nested forms and/or manipulation of > collections in forms much easier to deal with. > > ------------------------------------------------------------------------ > > The semantic changes are as follows: > > INPUT: "a[][x]=5&a[][y]=10" > OLD PARSER OUTPUT: {"a" => [{"x" => "5", "y" => "10"}]} > NEW PARSER OUTPUT: {"a" => [{"x" => "5"}, {"y" => "10"}]} > > INPUT: "a[0][x]=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][x]=5&a[1][y]=10" > OLD PARSER OUTPUT: {"a" => {"0" => {"x" => "5"}, "1" => {"y" => "10"}}} > NEW PARSER OUTPUT: {"a" => [{"x" => "5"}, {"y" => "10"}]} > > > ------------------------------------------------------------------------ > > Issues to be resolved: > > 1. Should the parser be modified to pass the old tests for invalid > inputs? > 2. "a[0]=1&a[999]=2" currently yields a 1000 element array with 998 > nil values. Should we implement a SparseArray class that minimizes > memory usage for such a situation, or force sequential indexes? > > ----------------- > > Would appreciate any feedback and/or recommendations. > > Cheers, > Tom > > > >--~--~---------~--~----~------------~-------~--~----~ 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 -~----------~----~----~----~------~----~------~--~---
On 30 Sep 2008, at 11:23, Thomas Lee wrote:> > The semantic changes are as follows: > > INPUT: "a[][x]=5&a[][y]=10" > OLD PARSER OUTPUT: {"a" => [{"x" => "5", "y" => "10"}]} > NEW PARSER OUTPUT: {"a" => [{"x" => "5"}, {"y" => "10"}]} > > INPUT: "a[0][x]=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][x]=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> ------------------------------------------------------------------------ > > Issues to be resolved: > > 1. Should the parser be modified to pass the old tests for invalid > inputs? > 2. "a[0]=1&a[999]=2" currently yields a 1000 element array with 998 > nil values. Should we implement a SparseArray class that > minimizes > memory usage for such a situation, or force sequential indexes? > > ----------------- > > Would appreciate any feedback and/or recommendations. > > Cheers, > Tom > > >--~--~---------~--~----~------------~-------~--~----~ 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 -~----------~----~----~----~------~----~------~--~---
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. -- Cheers, Koz --~--~---------~--~----~------------~-------~--~----~ 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 -~----------~----~----~----~------~----~------~--~---
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? --~--~---------~--~----~------------~-------~--~----~ 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 -~----------~----~----~----~------~----~------~--~---
Hi Fred, Thanks for the response. Counter-arguments and flamebait below. :)>> The semantic changes are as follows: >> >> INPUT: "a[][x]=5&a[][y]=10" >> OLD PARSER OUTPUT: {"a" => [{"x" => "5", "y" => "10"}]} >> NEW PARSER OUTPUT: {"a" => [{"x" => "5"}, {"y" => "10"}]} >> >> INPUT: "a[0][x]=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][x]=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][x]=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. :)> 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 --~--~---------~--~----~------------~-------~--~----~ 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 -~----------~----~----~----~------~----~------~--~---
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:> 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?
>
> >
--~--~---------~--~----~------------~-------~--~----~
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
-~----------~----~----~----~------~----~------~--~---
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 --~--~---------~--~----~------------~-------~--~----~ 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 -~----------~----~----~----~------~----~------~--~---
On Sep 30, 12:23 pm, Thomas Lee <t...@vector-seven.com> wrote:> http://rails.lighthouseapp.com/projects/8994-ruby-on-rails/tickets/11... > > 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: > > ------------------ > > * This patch was written with the intent of simplifying the > implementation of UrlEncodedPairParser, as well as making its > behaviour somewhat more predictable/logical. > * Changes to semantics are minimal: only breaks seven actionpack > unit tests which are either testing broken inputs or strange > behaviour in the old parser. > * Of those seven unit tests, five can be made to pass with some > minimal changes to the parser. > * New semantics would make nested forms and/or manipulation of > collections in forms much easier to deal with. > > ------------------------------------------------------------------------ > > The semantic changes are as follows: > > INPUT: "a[][x]=5&a[][y]=10" > OLD PARSER OUTPUT: {"a" => [{"x" => "5", "y" => "10"}]} > NEW PARSER OUTPUT: {"a" => [{"x" => "5"}, {"y" => "10"}]} > > INPUT: "a[0][x]=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][x]=5&a[1][y]=10" > OLD PARSER OUTPUT: {"a" => {"0" => {"x" => "5"}, "1" => {"y" => "10"}}} > NEW PARSER OUTPUT: {"a" => [{"x" => "5"}, {"y" => "10"}]} > > ------------------------------------------------------------------------ > > Issues to be resolved: > > 1. Should the parser be modified to pass the old tests for invalid > inputs? > 2. "a[0]=1&a[999]=2" currently yields a 1000 element array with 998 > nil values. Should we implement a SparseArray class that minimizes > memory usage for such a situation, or force sequential indexes? > > ----------------- > > Would appreciate any feedback and/or recommendations. > > Cheers, > TomI''ve posted my feedback as a Lighthouse comment. --~--~---------~--~----~------------~-------~--~----~ 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 -~----------~----~----~----~------~----~------~--~---
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:> 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: > >> 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? >> >> > > > > >--~--~---------~--~----~------------~-------~--~----~ 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 -~----------~----~----~----~------~----~------~--~---