RFC: Improving the rack parameter normalization

I have struggled much of the day today to do something that on the
surface seems like it should be a piece of cake. But either it's not,
or I'm too dense to see the easy way to do it.

I'm leaning toward modifying the rack parameter parser to (a) make it
far cleaner and more intelligible than it currently is (including
documenting exactly what it does!), and (b) make it easy to do what
I'm after. I think I can do this in a mostly backward-compatible
fashion, and could provide a compatibility mode if needed for anyone
who needs the old behavior.

I'd appreciate any feedback the experts may have before I embark on
this.

Here's a simplified version of what I'm trying to do that ended up
being (I think) impossible without resorting to some javascript that
really should not be required:

Say I've got a nested model structure that looks like this:

class Project < ActiveRecord::Base
  has_many :tasks
  accepts_nested_parameters_for :tasks
end

class Task < ActiveRecord::Base
  belongs_to :project
end

I'd like to be able create an edit form for :project that includes a
dynamically varying list of :task subforms. I'll have a delete button
next to each :task subform, and an "Add a Task" button at the bottom
of the last. All fairly standard and not very hard, especially with
the new accepts_nested_parameters_for feature.

But now here's the kicker: I also want to use the "sortable_element"
helper so that the user can reorder the tasks - including newly added
tasks - within the project. In order to save the final ordering, this
means I need to be able to detect, from the request parameters, the
order that the :task subforms appeared in the overall form at the time
it was submitted. This should be easy, because browsers are required
to send form parameters in the order in which they appear at the time
of submission, and sortable_element works by physically reordering the
subforms in the overall DOM structure.

The problem is that rack loses this ordering information, and I'm
pretty much convinced that it can't be made to keep it. You can ALMOST
get this to work by specifying :index=>'' and :child_index=>'' in all
your fields_for calls. But this results in illegal HTML (strictly
speaking, though it works, at least in the browser I've tried), and
it's also slightly risky. The illegal part comes from the fact that
you'll end up with multiple input elements in your form that have the
same id value. E.g. a text field for task 'name' would appear in
every :task subform, and in each appearance it would carry the same id
value. And not only would that be illegal, it would also make it
impossible to have per-subform radio button groups. The radio button
input elements would all have the same name across all subforms, so
they'd all be linked, rather than only within a subform.

Even without those problems, the strategy is dangerous because of the
way rack detects when it's time to start a new hash, rather than
continuing to fill a current hash. This comes into play for a
parameter with a name like 'project[tasks_attributes][][name]'. The
the empty brackets call for an array of hashes at that point in the
structure, and rack will look at whether 'name' appears as a key in
the current last element of that array. If so, it opens up a new
element in the array, otherwise it adds to the current last element.

This mechanism will fail if not all your subforms offer the same set
of fields. E.g you might have separate buttons for "add quick task"
and "add task," both of which add the same sort of object, but "add
quick task" provides an abbreviated form for easier entry. You'd have
to add the other fields as hidden fields in the abbreviated subform in
order to be safe, and it'd probably be a hellish debugging exercise to
figure that out!

OK, so here's my idea:

1. When "[]" appears in a parameter name it always indicates an array
at that point in the structure. What fallows goes into a new element
at the end of the array, using logic equivalent to what is currently
provided by rack.
2. When "[.index]" appears, what follows will be be added to the hash
that is the current last element of the array. The leading dot
prevents this index from itself being a hash key in what would then
have to be a containing hash, rather than a containing array, so
ordering of the substructures is preserved.
4. When "[+index]" appears, a new hash is added to the end of the
array, and what follows goes into that hash.
5. When [index] appears in a parameter name (without leading dot or
plus), the index is treated as a hash key for what follows same as
current behavior.

Here's an example:

   project[title]='My Project'
   project[tasks_attributes][+0][id]=1
   project[tasks_attributes][.0][name]=Sleep
   project[tasks_attributes][+1][id]=5
   project[tasks_attributes][.1][id]=Sleep more
   project[id]=3

The params structure would look like this:
   {:title=>"My Project", :id=>"3",
       :tasks_attributes=>[{:id=>"1", :name=>"Sleep"},
{:id=>"5", :name=>"Sleep more"}]
   }

The current rake parameter processor would yield (assuming dots and
pluses are removed from the above):
   {:title=>"My Project, :id=>"3".
       :tasks_attributes=>{"1"=>{:id=>"5", :name=>"Sleep more"}, "0"=>
{:id=>"1", :name=>"Sleep"}}
   }

With the new structure it would be simple matter for the controller to
take a pass through params[:project][:tasks_attributes] and
assign :position values for the benefit of acts_as_list in order to
retain the final subform ordering. With the current structure, one
would need to use a javascript callback before submit to add those
values to the DOM, or AJAX calls to update positions every time
something gets reordered. Neither would be all that difficult, but
it's infuriating, all the required information is already available
for free in the raw form submission.

To make this really work well, the form helpers would also need to be
modified to make use of the enhanced naming scheme. If I do this, my
goal will be to do both the enhanced rack parameter parser and the
form helper modifications to take advantage.

Thanks for any comments or recommendations.

-andy

I've got a slightly modified form of this working.

The rules for parameter names in my scheme are:

- may optionally begin with an unbracketed name
- thereafter must be a sequence of bracketed names
- any bracketed name starting with '.' must be immediately followed by
one that does not
- the whole thing may optionally be terminated with '[]'

A "bracketed name" in the above is just any string that doesn't
include '[' or ']'
characters, surrounded by '[' and ']'

My replacement normalize_params implementation in rack/utils.rb checks
the parameter name it's initially called with to see if it meets these
criteria.
Moreover, I'm pretty certain that any name that follows the rules but
does not
have any bracketed names beginning with '.' will yield the same
results via
either the old or the new function, so what I've done should be
backward
compatible.

The new scheme uses the dotted names to introduce arrays of hashes.
The
part of the name following the '.' is saved discretely in the last
hash of that
array (under a key named '.index'), and a new hash is allocated
whenever
the current dotted name is different from the one that was most
recently
stashed. The stashed .index keys are all removed before the parameter
structure is finally returned, so they won't screw up attributes
setters.

This scheme means you don't need to worry about using '+xxx' for the
first
parameter in a group, the rest of which will use '.xxx' instead, as
per my
original proposal.

With these changes in place (plus a small change to acts_as_list to
avoid
assigning a position value to a new record if one has already been
assigned), my original problem is very easily solved, as follows:

In my :task partial template, I use:

    <%= project_form.fields_for :tasks, task,
                :child_index=>".#{task.id || 'REPLACE_ME'}" do |tf| %>

My "add a task" helper needs to replace 'REPLACE_ME' with a unique
value via javascript (as is already common practice in these dynamic
multiform scenarios).

This yields parameter names in the subform like:

   project[tasks_attributes][.13][name]

Then in my projects controller, my update action is defined like this:

  def update
    params[:project][:tasks_attributes].each_with_index { |ta,i| ta
[:position] = i }
    @project = Project.find(params[:id])
    ...
  end

That's it. I'm actually thinking now that, since it's so
straightforward
to elect to use this feature (you just need to supply a :child_index
that
starts with a dot in the nested fields_for call), it may not require
any
changes to the form builders as I had originally expected.

I haven't yet tried this with more deeply nested forms, but I will do
so
shortly.

If anyone has any comments or recommendations, please post. I'd like,
soon, to publish a patch for this to be incorporated into the rails
distro.

Thanks

-andy

Hi Andy,

I have in the past built an extension to the form_for helper family to accept a :element_id_prefix option that handles the “multiple html elements with the same ID” problem. It’s been sitting on my to-do list for awhile to convert it to a rails patch and submit it for consideration.

I like that you want to take advantage of the standards already defined for html form submission and POST data to achieve ordering, but your solution seems needlessly roundabout for the end goal. You seem to actually be addressing two distinct issues here:

  1. Clean up implementation of parameter parsing so that it maintains the order in the originally POSTed data string.
  2. Introduce a :child_index value to fields_for to avoid the “multiple html elements with the same ID” problem.

In regards to #1, what you get by doing this is your ability to concisely auto-assign position values to the children objects in your controller (which still requires you to iterate over the passed in params). This seems to me to not be a necessarily common need. So I’m inclined to ask, what else do we get out of the redesign of the parameter parsing? And do we really need to change the scheme for encoding field names to achieve this? Can we not achieve it by only changing the parsing code?

#2 seems completely useful to me. That said, I would also like it on the form_* family of functions as well. If it’s a problem with fields_for, it’s also a problem with form_for (and one I’ve obviously encountered before).

Stepping back a moment and looking at your specific problem, I would have simply solved it by outputting a hidden :position field as part of the form, and then setting the :onUpdate parameter of sortable_element to set those hidden fields (only a couple of lines of javascript). This in conjunction with a solution for “multiple html elements with the same ID” would seem to completely serve your needs, and you wouldn’t need any additional controller code to actually set the position field on the child elements.

-John

Thanks for your comments, John.

Re the need for changing the parameter parser to achieve #1, I think
it's absolutely necessary. I'm pretty convinced that the existing
parser is simply incapable of retaining ordering of record structures
unless those record structures are populated via *identical* parameter
names. Anything you do to disambiguate parameter names will force the
parameter parser to treat those disambiguating chunks as keys in a
hash, and their relative order of appearance will thereby be lost. The
key thing I've done with my reimplementation is to come up with a
syntax that tells the parameter parser to omit certain "words" in the
parameter name (i.e. strings of non-bracket characters) from the final
structure, using those words solely to control allocations of new
hashes in the surrounding array. The syntax I've currently got for
this is an initial dot. I'm thinking of switching to double brackets
instead (e.g. "[[index]]" instead of "[.index]" because it wouldn't
preclude developers using dotted index values if they really want to.
(The corresponding argument to not take away developers' ability to
use double-bracketed indexes is weaker because doing so with the
current parser accomplishes nothing - the double brackets are
processed identically as the same indexes with single bracketing.)

Also, keep in mind that the current parameter parser is actually
potentially dangerous, in that values from different subforms can end
up in the same record. My "quick edit" vs "full edit" example is one
dangerous case, but another is when your subform collection is
actually polymorphic (the situation I'm working on right now and which
spawned all this). The current parser relies on seeing a repeated
attribute to know when to start filling in a new record, so
polymorphism in the subforms has the potential to screw things up! I
think my new parser would be able to completely reliably handle all
parameter names produced by rails' form helpers, using a much more
restrictive syntax than is accepted by the current parser (that parser
is EXTREMELY lax and will parse totally screwy parameter names). With
the backward compatibility features, this shouldn't affect non-rails
uses of rack.

Re :child_index, I didn't actually introduce that - it's part of the
existing nested attributes support, I assume introduced as part of the
new accepts_nested_attributes_for feature. Normally you don't need to
supply a :child_index value, and record position will be used
automatically in most cases. But here I want to use a value that will
trigger the proper parameter ordering.

Ultimately, I'm thinking that this sort of handling would be a better
default than what's currently done - i.e. there's really no benefit
that I can see from having the records come back in the params
structure in an arbitrary order, and with this change it'd be quite
easy to make as-submitted order be a "sensible" default. Then imagine
altering acts_as_list so that the attributes= method would fix
positions when handed an array of records, and suddenly my controller
cruft could disappear, and this order maintenance would be in the
model, where it belongs, rather than in the controller or in
javascript.

So in summary, you're right that the javascript approach could work
and would be simple, but I'd argue that the parameter parsing change
would open the door to a far more "sensible" default in any case. Then
I have the option of fixing positions with a one-liner in the
controller, and you could still use your javascript approach if you
prefer it. Eventually acts_as_list can handle it and we can both
retire our extra logic.

-andy

Correct me if I'm wrong, but couldn't this be solved by having rack return ordered hashes?

Hmm... I think you're right, and especially with ordered hashes being
standard in ruby 1.9 it's a very natural choice.

It will still leave a pretty ugly and ill-defined piece of code in
rack, and maybe that should be addressed, but it's not needed for this
if we adopt your approach. By ill-defined I mean that trying to
describe what it actually does in any sensible fashion is extremely
difficult. Descriptions of what it's intended to do are generally
quite succinct, but in fact, it'll take a parameter assignment like 'a
[[[[[][][b][][c]][][[[[[[[[x]=1' and yield {"a"=>{"b"=>[{"c"=>
[{"x"=>1}]}]}}, and assigning "a]b[=3" yields {"a"=>{"b"=>nil}} and
other such bizarre cases. And even seemingly well-formed cases can
yield unexpected results, like the situations I've already talked
about where a nonuniform set of subforms might, with rack's current
behavior for embedded "[]", end up mixing attributes from different
subforms into each others' records.

So I know rack is a separate offering from rails, but it seems the
rack parameter parser was largely adopted from rails, and the
connection is pretty tight. If a general clean-up of the rack
parameter parser is worth doing, would that likely be driven by this
community?

-andy

As for doing a cleanup in the first place, unexpected results would imply that there are bugs

even seemingly well-formed cases can
yield unexpected results, like the situations I've already talked
about where a nonuniform set of subforms might, with rack's current
behavior for embedded "[]", end up mixing attributes from different
subforms into each others' records.

If this is the case, then I a gree that a refactoring is in order...

So I know rack is a separate offering from rails, but it seems the
rack parameter parser was largely adopted from rails, and the
connection is pretty tight. If a general clean-up of the rack
parameter parser is worth doing, would that likely be driven by this
community?

Considering rack seems to have its own fairly active community and mailing list, I think it would be best to approach them first. Have you tried the rack-devel list?

Unless I'm missing something, why wouldn't you just use hidden fields
that indicate the order?

That could be part of a solution, but to make it dynamic would mean some javascript to update the position fields as subforms are moved around by the user, or just prior to submit. What I’m looking for is a “dead-simple” way of doing what seems a pretty obvious use-case, namely: the user gets to reorder subforms via some drag-and-drop feature like the sortable_element helper, and when the form is submitted, the order of the subforms at the time of submission gets reflected in the database. Using hidden fields and javascript to manipulate them seems unreasonably complicated when the browsers are required to send form values in the order of appearance anyway, since that’s the order we want for this use case. But the current rack parameter parser either destroys the relative ordering of the subforms or requires that the field names & ids be identical from one subform to the next. The latter results in illegal HTML, potential mixing of subform data, and an inability to use checkboxes and radio groups properly in the subforms. Hence you can’t really take advantage of this very convenient browser behavior.

I’ve been away and hence quiet for a few days, but per recommendations earlier in this thread, I’ll take it up with the rack community to see about making their parameter parser use ordered hashes (shouldn’t be a hard sell, since it’ll happen with ruby 1.9 anyway), and also a general cleanup of the current parser. I may also look into updates to the rails acts_as_list plugin, which I think could make the whole thing fully automatic (so no need for a controller hack to update subform positions).

-andy