Move multi-parameter attributes from ActiveRecord to ActiveModel

[georgebrock]

Multi-parameter attributes (used to build a complex type like a Date from several simple values that can be easily POSTed) are currently implemented in ActiveRecord::AttributeAssignment. This is pretty frustrating if you're trying to use an ActiveModel::Model with Date or DateTime fields.

I've fixed this by moving multi-parameter conversion to the ActionController::Parameters class, so it happens before the value hits the model. IMO the model layer shouldn't need to be aware of the format that's used to pass the data from the client to the server.

To support this the type that the values should be re-assembled into is included in the form (e.g. <input type="hidden" name="my_model[created_at(type)]" value="Date">) instead of being determined from the model schema.

The main aim was to get multi-parameter attributes working with ActiveModel, but a nice side effect of this implementation is that it becomes very easy to register custom types. For example, you could easily submit an amount of money as a currency and an amount and have it converted to your own custom Money class:

    # In an initializer somewhere:
    ActionController::MultiParameterConverter.register_type('Money') { |currency, amount|
      Money.new(currency, amount)
    }

    # View:
    <input type="text" name="my_model[price(1)]" value="$">
    <input type="text" name="my_model[price(2f)]" value="50.00">
    <input type="hidden" name="my_model[price(type)]" value="Money">

This isn't a finished implementation yet. It needs documentation, and the ActiveRecord implementation needs to be deprecated or removed. I've added multi-parameter support to ActionController and updated the date_select, datetime_select and time_select helpers to use the new implementation. I figured this was far enough to get some feedback.

---

Edit: Renamed from Move multi-parameter attributes from ActiveRecord to ActionController to Move multi-parameter attributes from ActiveRecord to ActiveModel to reflect the current state of the change.

[carlosantoniodasilva]

I'm not sure I like the idea of having this type going and coming back like that, mixed feelings... There's also the issue that Active Record multiparameter logic also handles aggregate attributes (generated by composed_of), which some people might be using out there, we probably couldn't just break the compatibility.

I haven't looked carefully at the code though, but I'm 👍 for the spirit, we just need to figure out how to make all this work.

[rafaelfranca]

This seems a good idea but the aggregate attributes compatibility concerns me. I would not move it to Action Controller thought.

I think is better and easy to maintain compatibility with aggregate attributes to move it to Active Model and inherit to this new model in Active Record adding the composed_of compatibility.

I don't see too much gain in moving this to Action Controller beyond the "model layer shouldn't need to be aware of the format that's used to pass the data from the client to the server".

About the side effect that make "very easy to register custom types" this is already possible using composed_of.

That said, I would gladly accept a feature making possible to use multi-parameters with Active Model classes but I'd not made it to Action Controller.

@josevalim @spastorino @jeremy @wycats your thoughts here would be great.

[pixeltrix]

I'm 👎 on the idea of moving it to Action Controller. If you take it to it's logical conclusion you'd only accept the correct types for boolean, integer, decimal, etc. columns, which is obviously crazy. If we just do it for multi-parameter attributes then we have split the responsibility for typecasting params.

The plan suggest by @rafaelfranca is a better idea.

[georgebrock]

Thanks for the feedback.

I hadn't considered the impact on aggregate attributes, which would need some work, but I thought they were being removed in Rails 4 anyway? (#6743)

I take @pixeltrix's point about splitting responsibility for type conversion, but as things stand now responsibility for multi-parameter support is already split: ActionController::Parameters parses multi-parameter attributes so that they will work with strong parameters, and then ActiveRecord::AttributeAssigment parses them again to do the conversion, which also feels a little messy.

Perhaps it would be cleaner for ActionController::Parameters to convert from the multi-parameter attribute format into an Array (e.g. {'foo(1i)' => '13', 'foo(2)' => 'bar'} would become {'foo' => [13, 'bar']}) and then we leave it up to the model layer to convert from an array to whatever the final type is going to be?

[georgebrock]

Ignoring the ActionController stuff for a minute, here's a quick implementation of moving ActiveRecord::AttributeAssignment to ActiveModel without changing anything else: https://github.com/georgebrock/rails/compare/active_model_attribute_assignment

Currently you would use it like this:

class Person
  include ActiveModel::Model

  attr_accessor :name, :date_of_birth

  def class_for_attribute(attr)
    return Date if attr == 'date_of_birth'
  end
end

george = Person.new(
  'name' => 'George',
  'date_of_birth(1i)' => '1984',
  'date_of_birth(2i)' => '2',
  'date_of_birth(3i)' => '20'
)

george.date_of_birth #=> Mon, 20 Feb 1984

If this seems sane I'll clean it up, add tests and provide a nicer way of specifying date, time and datetime attributes without needing to override the class_for_attribute method every time.

[jorgemanrubia]

I like this idea a lot. I have had problems too with having multi-parameters assignments depending so heavily on active record.

[rafaelfranca]

@georgebrock sorry for the delay. The overral implementation seems good. Some considerations:

• I don't think ActiveModel::AttributeAssignment should be included in ActiveModel::Model. ActiveModel::Model was created to give a minimal implementation work with Action Pack and AttributeAssignment is not necessary to this.
• We will need to provide the Active Record errors to backward compatibility (maybe deprecated)

[senny]

ping.

What is the status on this one?

[woto]

+1 for this feature.

[georgebrock]

@senny Sorry for the delay, life got in the way. I should have some time to pick this up again next week.

@rafaelfranca Thanks for the feedback. Agreed on both points.

[georgebrock]

I've pushed an updated version:

• There's nothing moved to ActionController. That wasn't popular. I'd still like to revisit the fact that strong parameters and AttributeAssignment both need to understand the multiparameter format, but I'll save that for another pull request.
• ActiveRecord::AttributeAssignment has become ActiveModel::AttributeAssignment, so date_select will work with non-AR models. I'm not including this in ActiveModel::Model, as Rafael requested.
• Various exceptions from ActiveRecord are now duplicated in ActiveModel, but ActiveRecord models will still use the ActiveRecord versions. The duplication is a little unpleasant, but it means we won't break backward compatibility.

There are still a couple of things to do before this lands:

• ActiveModel::Model#initialize does mass assignment using a different mechanism. If a class includes ActiveModel::Model and ActiveModel::AttributeAssignment we should probably support multiparameter attribute in the constructor.
• I've not updated any documentation, and I should.

More next Friday if I don't get to it before then.

[calebhearth]

How do you feel about moving the error classes to the bottom of the file so that coming into the file the documentation on AttributeAssignment is the first thing we see?

[georgebrock]

Good call. Thanks Caleb.

[georgebrock]

@carlosantoniodasilva, @rafaelfranca: I think this is ready to go, let me know what you think.

[rafaelfranca]

I'm sorry but we are too close to 4.0 to merge this one.

But we will revisit after the release.

[georgebrock]

Absolutely, I wasn't expecting it to make 4.0 at this late stage. Let me know if there's more I can do to get this merged when the release is out of the way.

[h0jeZvgoxFepBQ2C]

Cant you include it into rails 3.2.x? Would be really nice..? :)

[dmathieu]

@lichtamberg: no, this can't make it to 3.2.x. The 3.2 branch is only for bugfixes now.
@rafaelfranca: this can be merged now, as 4.0 is in the branch 4-0-stable. It'll land into 4.1.

[rafaelfranca]

@dmathieu sure. I'll review this soon. I need to discuss some API related things with the Rails core

[ollym]

@rafaelfranca any updates on this?

[georgebrock]

I've rebased this onto the current master. Anything else I can do towards getting it merged?

/cc @rafaelfranca

[rafaelfranca]

This is a major change and I don't want to take the decision too soon. Right now this is on hold until we start the development of Rails 4.1

@georgebrock thank for rebasing. I'll ping you when we are going to merge.

[mrbrdo]

Making strong parameters work with ActiveModel::Model would be very nice. Thanks for this contribution I am looking forward to it.

[woto]

Hi, any plans? :(

[georgebrock]

I've just rebased again to keep this close to master and easy to merge. Tests still passing.

[mhuggins]

So this is still not available in Rails 4.x? Anyone have a workaround?

[mrbrdo]

CC @rafaelfranca @tenderlove ;) would be nice to get this in if everything is in order

[secretfader]

I need this today, as I'm upgrading a critical Rails app to 4.0.2. Is there a way to pull this in from git and use it, or am I out of luck?

[calebhearth]

Fork rails, add github.com/georgebrock/rails as a remote, merge this branch into rails/4.0.2 (the tag), and then use your fork of Rails: gem 'rails', github: 'yourusername/rails'

I'm not recommending this, but that is what you would do.

On Mon, Jan 20, 2014 at 5:09 PM, Nicholas Young notifications@github.com
wrote:

I need this today, as I'm upgrading a critical Rails app to 4.0.2. Is there a way to pull this in from git and use it, or am I out of luck?

Reply to this email directly or view it on GitHub:
#8189 (comment)

[secretfader]

Thanks Caleb. With Mongoid dropping multi-param attributes at the same time, this whole thing is a huge mess. I'll try to sort it out.

On Jan 20, 2014, at 4:18 PM, Caleb Thompson notifications@github.com wrote:

Fork rails, add github.com/georgebrock/rails as a remote, merge this branch into rails/4.0.2 (the tag), and then use your fork of Rails: gem 'rails', github: 'yourusername/rails'

I'm not recommending this, but that is what you would do.

On Mon, Jan 20, 2014 at 5:09 PM, Nicholas Young notifications@github.com
wrote:

I need this today, as I'm upgrading a critical Rails app to 4.0.2. Is there a way to pull this in from git and use it, or am I out of luck?

Reply to this email directly or view it on GitHub:
#8189 (comment)
—
Reply to this email directly or view it on GitHub.

[mhuggins]

@nicholaswyoung I needed this in my project, so I just created a concern using the code here, along with an Rspec support file to test the functionality on my model. Note that I only built tests around the date functionality since I'm personally not in need of the time related code.

Here's a gist that shows how I put the code here to use: https://gist.github.com/mhuggins/6c3d343fd800cf88f28e

[dmitry]

Is there are any possibility to speedup this pull-request to be merged?

[stevegraham]

bump

+1

[rafaelfranca]

We are still open to the idea but the implementation should leverage the attributes API introduced in Rails 5.2 in Active Model.

Thank you again for this PR.

[Papipo]

How is this coming along?