RFC: Improving the rack parameter normalization

rubyonrails-core
1

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

2

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

3

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

4

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

5

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

6

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]][[[[[[[=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

7

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?

8

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

9

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