Skip to content
This repository

Rails multi parameter support #30

Closed
eladmeidar opened this Issue February 26, 2010 · 28 comments
Elad Meidar

I've added a support for the Rails specific convention of handling multi parameter attributes (ex: Date form fields are submitted as more than 1 parameter: date_field(i1), date_field(i2)...).

i've added a module named Mongoid::Rails::MultiParameterAttributes to the mongoid/rails folder (which i also created). would appreciate feedback before doing a pull request.

http://github.com/eladmeidar/mongoid/blob/master/lib/mongoid/rails/multi_parameter_attributes.rb

Robby Colvin

+1 for multi-parameter support.

Sascha Brink
sbrink commented June 14, 2010

+1

Ruben Fonseca

+1 please merge this :)

Felipe Rodrigues de Almeida

+1

Victor Hugo Germano

Hello, I was wondering: how well tested is this patch?
Can you show us a few specs on the multi_parameter_attributes running?

Thank you!

Jacques Crocker

this one definitely needs to get merged in. I'll review and merge in soon

Jacques Crocker

Took a look, however I think this'll need some specs to get merged in. Here are the specs for ActiveRecord... maybe we can reuse some of the logic
http://github.com/rails/rails/blob/master/activerecord/test/cases/base_test.rb#L1053-1257

Elad Meidar

This is actually an extraction of the AR handling of multiparams. i SWEAR i had tests somewhere for that :)
anyway, i'll pull-request it again with some tests soon enough.

just out of curiosity, is the mongoid/rails/* convention fine ?

Jacques Crocker

Yup, that convention is fine. Thanks a ton for your work on this!

Anton Bangratz

Oh my, did not find this issue until after I was done - I got a fork and branch for you including the functionality, but admittedly included directly into attribute handling, adding comments and specs from rails. Could also use the convention mentioned above, and do the helpers magic. Interested?

Location: (topic branch, and re-merged to quite recent master)
http://github.com/abangratz/mongoid/tree/multi_parameters

Durran Jordan
Owner
durran commented July 27, 2010

I appreciate everyone's work on this, however I am still skeptical about merging these into master right now because the A.R. code that people are pulling over is not something I want in the Mongoid codebase. I know it works, but just because A.R. deemed it ok for their codebase does not necessarily mean Mongoid will. (It is some horrendous code and definitely not flexible.)

I have pulled down abangratz branch locally and am trying to refactor this into something nice (and maybe it can be sent back to Rails as a patch) - but there must be an elegant way to handle this! cry

My other problem with this feature is that it promotes bad user interface design from a Rails perspective. There are many better ways to do date or date/time selects than a series of pulldowns - even a text box leveraging chronic's DSL is much better...

So because of the second issue - I will for the first time put my foot down on pulling this in until I see a well designed solution to it. Once again I appreciate everyone's work so far - I know it's not your code, its AR's code.

Anton Bangratz

Thanks for the heads up, and actually, using a text field and a Javascript date picker works better than the convoluted setup - anyways, I had it ready and the related specs included. I do agree on the codebase and flexibility issues also.

Elad Meidar

Not sure i agree with that.
I originally separated it into an independent module (Mongoid::Rails::MultiParameter if i am not wrong) that is not included by default.
My direction was towards providing a Rails solution, not to provide "date handling" solution and therefore i think i was impaired to use the Rails current API (who.. who thought about this stupid(2i) stuff? geez...).
Anyway, as a set of Rails specific tools i think that keeping the current AR replicated-solution is a good idea. hi, if you don't want it, don't include it - but it does makes it easier to work with Rails date crap.

Durran Jordan
Owner
durran commented July 28, 2010

I agree it does make it easier to work with the Rails date stuff - no argument there... I just don't like the A.R. code... However the more I look at it the more I realize how difficult it is to provide an elegant solution to this exact problem. Damn you Rails! We'll see in the upcoming days - I know a lot of people want this.

Sérgio Santos

The code isn't pretty, I agree, but a better solution for the given constraints is hard. After reading the code I found some specific problems:

  • The Time#to_date instance method is only added on ActiveSupport, it's not core
  • All nil date values are converted to 1, while the 4th Date parameter default is ITALY=2299161
  • Support for the array type is documented but not supported

I can help correct these issues, but as mentioned, I would prefer a better strategy/implementation to the whole problem.

Jacques Crocker

now that railties has its own Document module that gets included (used to add _destroy), maybe thats a good place now to include this hack

Matt Powell

Here's a somewhat cleaner version that seeks to emulate the documented behaviour of ActiveRecord in a more Mongoidy way, and without copying AR code over:

http://github.com/fauxparse/mongoid/commit/64155c05d7b4847f8eb28bea54ec3884bd04df3a

Includes specs, and support for string and array parameters (documented but not implemented in AR). Unlike the AR code, it doesn't throw half a dozen extra methods into the instance, and because it hooks into #process, it respects protected attributes.

I'd appreciate some help with testing this, particularly with respect to timezone support.

Jacques Crocker

fauxparse: wow, that looks great! nice and clean. we should wait till it gets a bit of testing and feedback before merging it in. but thanks a ton for your work on this!

Matt Powell

I've had something like it in a production app for a couple of weeks, but I think the timezones are going to be the weak spot (they've never been my specialty).

Matt Powell

OK, a couple of updates.

This one respects Mongoid::Config.instance.use_utc:

http://github.com/fauxparse/mongoid/commit/acbc2a91c72e8ecdd1fe939f1c1bbb83748c6853

...and this one raises errors when invalid arguments (e.g. bad dates) are sent.

http://github.com/fauxparse/mongoid/commit/450424acc5ac03f8dd6c933c75c6d3526645e61f

Both include specs.

Please test and comment, all you plus-oners.

Jacques Crocker

nice!

Rémy Coutable

Works for me ! :)

Thanks a lot !

Rémy Coutable

If all date params are blank, it throws an exception. Instead, it should set the date to nil, like AR does.

Here is my patch:
http://github.com/rymai/mongoid/commit/7b14b43b533b8aef18400ba7cf7c81facaa001bd

Let me know.

sanford

Anything holding this back? Just ran into the issue today.

Jacques Crocker

nope, nothing holding this back. can you give these patches a try and verify its working? if you give the go ahead, i'll merge in

Sean Kirby

Please check out #358

Jacques Crocker

Thanks to everyone for contributing to this. Its been merged into master, and seems pretty solid. Please retest on master and verify we didnt screw up anything in the merge. Closing this issue for now.

Thanks again. You guys rock!

joe1chen

This is indeed fixed in Mongoid now, BUT it is NOT enabled by default out-of-the-box (from the perspective of someone who had been using ActiveRecord). It took me way too long to figure out the solution, with the plethora of out-dated solutions on StackOverflow and other forums.

For the benefit of anyone who is still struggling with this, you simply need to add

include Mongoid::MultiParameterAttributes

to your model.

For instance:

class Post
  include Mongoid::Document
  include Mongoid::MultiParameterAttributes

  field :published_at, type: DateTime
end
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.