Skip to content
This repository was archived by the owner on Aug 17, 2017. It is now read-only.

Revisiting problems with multi-parameter attributes #34

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

fidothe
Copy link

@fidothe fidothe commented Sep 5, 2012

I've been looking at multi-parameter attributes quite a bit over the last week: I have a Rails app which isn't using an ActiveRecord instance to process the params, but is using the model-ish objects with the form helpers, producing date_select output of the "object[date(1i)]" form.

Looking over the activity around this stuff in ActiveRecord and more widely in Rails it looks like strong_parameters is a good place to start. I've seen the recent activity here (#17 and #21) and Carlos' refactorings happening in e.g. rails/rails@ec31680

I've been wondering about an approach which separates out the multiparameter parsing code from ActiveRecord::AttributeAssignment and, eventually, pushes the stitching-together of multiparameter attributes into ActionDispatch, that doesn't do anything with higher-level (model) datatypes, but does know about:

params = {"person" => {"date(1i)"=>"2012", "date(2i)"=>"9", "date(3i)"=>"5"}} 
# (post Rack::Utils.parse_nested_query...)

MultiParameterProcessor.process(params)
#=> {"person" => { "date" => [2012, 9, 5] }}

# or
MultiParameterProcessor.process({"person" => {"name(1s)"=>"Matt", "name(2s)"=>"Patterson"}}
#=> {"person" => { "name" => ["Matt", "Patterson"] }}

# I'm not aware of anything but `(1i)` form being used inside ActionView, btw

This should allow the multi-parameter code to select for mult-parameter-ness based on whether the value is an Array or not, much as with nested attrs now:

https://github.com/rails/rails/blob/master/activerecord/lib/active_record/attribute_assignment.rb#L97-104

(I realise nested_attributes_for might require a bit more thought, but it doesn't feel too big to me.)

If this was in the param processing chain before the controller method then the failing test in this pull request would cease being be a failing test. It'd be good to have the complexity for multi-parameter attrs in one place. Putting it in ActionDispatch seems like, if nothing else, a win for code DRYness: strong_parameters will need to know about this stuff otherwise...

Obviously, actually implementing this will require changes in ActiveRecord and ActionPack. If there are no objections, I'll ping Carlos and talk to him about how and where to safely extract this functionality (I have a good idea, I just don't want to tread on the work being done there already), and also start reaching out to people working on ActionDispatch.

@carlosantoniodasilva
Copy link
Member

Just for the record, @fidothe and I have started a conversation about this and ways to extract multiparameter assignment from Active Record, but we haven't get to any code so far.

The main problem is that we need to know the attribute type so that we can instantiate the right object, ie a Date or Time object, and we use the column information from Active Record for that (or aggregation from composed_of).

One idea is to extract the parsing to something completely external to Active Record, happening before even touching the controller layer, and give a more useful definition to Active Record. I'm still unsure about how this can improve things for non Active Record objects, but lets see where our initial conversation will take us.

If anyone wants to help and give feedback, please feel free and let us know, thanks!

@svoop
Copy link
Contributor

svoop commented Oct 5, 2012

Related to #39

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants