x-post : find_or_initialize_by ActiveRecord bug? Ignores :conditions

rubyonrails-core
1

Hello, I posted this on the rails talk group but received no response. Perhaps someone on this list could weigh in on whether this is expected/desired behavior or a bug that I should file and develop failing tests for (I doubt I have the active record method_missing fu to actually patch it).

Test Scenario: I would like to find or initialize a new user and base the find on the users email address, however the user found has to also match one of two possible 'state' values (passive || pending_singup). While trying to implement this I discovered that the find_or_(initialize| create)_by_* dynamic finders totally and silently ignore any conditions passed.

Bug?

Should'nt these dynamic finders behave the same as find_by_* or find_all_by_* dynamic finders and use the :conditions passed to them? The documentation seems to indicate that they should.

Dynamic attribute-based finders documentation

"It's even possible to use all the additional parameters to find. For example, the full interface for Payment.find_all_by_amount is actually Payment.find_all_by_amount(amount, options). And the full interface to Person.find_by_user_name is actually Person.find_by_user_name(user_name, options). So you could call Payment.find_all_by_amount(50, :order => "created_on"). The same dynamic finder style can be used to create the object if it doesn't already exist. This dynamic finder is called with find_or_create_by_ and will return the object if it already exists and otherwise creates it, then returns it."

Here are a couple of examples. I know these are contrived since I could use 'find_or_initialize_by_name_and_state()' however what I am really trying to show is that the :conditions options are ignored. Check out the actual SQL run by each of these finds in a console to see what I mean.

PASTIE w/ examples:

http://pastie.caboo.se/148479

Original Post: http://groups.google.com/group/rubyonrails-talk/browse_thread/thread/63ab809535b1be9d/312e09737ab18b7b?lnk=gst&q=glenn#312e09737ab18b7b

Thanks.

2

Not really. Because the second argument to those dynamic initializers is an attributes hash to be added to the newly created object, not arguments for find. So it's treating them as if you want to call

@user.conditions= {:state => 'passive'}

if it doesn't find a user with that email address.

3

Thanks for the response Michael and the clarification.

I also noted the following patch which seems related to the functionality you describe.

http://dev.rubyonrails.org/ticket/7368

However, I would like to suggest the following three points and get your feedback:

1) This is poorly documented. I checked out:

and found the section on dynamic finders somewhat vague on the topic. There does seem to be one example that covers what you just described (The last in the dynamic finders topic) but I think there is a lot of room for improvement, especially since the dynamic finders are not documented in the API docs with rdocs dedicated to each find_or_initialize_by or find_or_create_by method. In other words, no method signature is easily available since these are method_missing dynamic methods. How can we improve this?

2) Understanding what the current functionality is, and accepting that, don't you think that the find_or_create|initialize_by methods *should* accept conditions for the find operation to be consistent with the normal find_by methods? So in that case you could make a call like:

@user = User.find_or_initialize_by_email(:email => 'foo@bar.com', :name => 'Foo Bar', :conditions => {:state => 'passive'})

Which would do:

- Find a user with the email 'foo@bar.com' WHERE state='passive' - set the @user.name attribute to 'Foo Bar'

This approach would seem to be more consistent with the other non- instantiating/creating dynamic finders. Is there a reason this should not be the case?

3) If the approach in (2) is not appropriate for some reason, shouldn't find_or_create|initialize_by raise an exception/warning if passed :conditions instead of silently swallowing them (which gives the user the false impression that they are actually being considered as part of the find since that is the behavior they are used to with other dynamic finders). This is what bit me, as I assumed based on my read of the docs that they would take conditions, but only later when I watched the SQL being generated did I see the error of my ways. This was a silent bug in my code which I had to implement using standard finders and an if statement to determine whether to create a new object or update the attributes of an existing one.

I will be willing to help with these, although my method_missing 'fu' is weak... so I am hoping that someone else will agree and patch it which I will surely +1 on. :slight_smile: I can help with a documentation patch if useful.

Thanks,

Glenn

4

With better documentation on the class level. I don’t think there is any other way (except for hacking RDoc maybe with a custom feature, good luck with that).

I strongly oppose your suggestion #2; it’s unintuitive and hard to implement (e.g. what if the user has a “conditions” attribute).

About #3: better documentation is definitely a way to go. As you were surprised yourself and thinking others may be, write a doc patch to stress out how conditions are not working – the name of the finder is a condition itself.

5

> In other words, no > method signature is easily available since these are method_missing > dynamic methods. How can we improve this?

With better documentation on the class level. I don't think there is any other way (except for hacking RDoc maybe with a custom feature, good luck with that).

I agree. I wouldn't want to suggest going near rdoc for this one.

I strongly oppose your suggestion #2; it's unintuitive and hard to implement (e.g. what if the user has a "conditions" attribute).

Hmmm... Playing devils advocate I would say that not having it there is counterintuitive since the current docs, and my experience with all other types of finders, seem to imply to me that it should work. Are there other find methods that don't except :conditions except these two? :conditions is something that should work across all finders to be truly consistent. Is the consistency with other find methods in the API worth ignoring the edge case where the model has a conditions attribute? I have no doubt it may be hard to implement, but the question I am posing for the moment is if it is the right thing to do, not how hard it is. I hear your 'no' vote though. :slight_smile:

About #3: better documentation is definitely a way to go. As you were surprised yourself and thinking others may be, write a doc patch to stress out how conditions are not working -- the name of the finder is a condition itself.

I will see if I can work up a documentation patch that clarifies somewhat while the collective rails mind thinks about whether any of my other idea has merit. :slight_smile: