Rails core - Feb 2008 - x-post : find_or_initialize_by ActiveRecord bug? Ignores :conditions

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.

http://api.rubyonrails.org/classes/ActiveRecord/Base.html

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.
--~--~---------~--~----~------------~-------~--~----~
You received this message because you are subscribed to the Google Groups
"Ruby on Rails: Core" group.
To post to this group, send email to rubyonrails-core@googlegroups.com
To unsubscribe from this group, send email to
rubyonrails-core-unsubscribe@googlegroups.com
For more options, visit this group at
http://groups.google.com/group/rubyonrails-core?hl=en
-~----------~----~----~----~------~----~------~--~---

On Feb 8, 2008 2:00 PM, Glenn Rempe <glenn.rempe@gmail.com>
wrote:
>
> 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?
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.

-- 
Cheers

Koz

--~--~---------~--~----~------------~-------~--~----~
You received this message because you are subscribed to the Google Groups
"Ruby on Rails: Core" group.
To post to this group, send email to rubyonrails-core@googlegroups.com
To unsubscribe from this group, send email to
rubyonrails-core-unsubscribe@googlegroups.com
For more options, visit this group at
http://groups.google.com/group/rubyonrails-core?hl=en
-~----------~----~----~----~------~----~------~--~---

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:

http://api.rubyonrails.org/classes/ActiveRecord/Base.html

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.  :-)  I can help with a documentation patch
if useful.

Thanks,

Glenn


On Feb 9, 12:28 am, "Michael Koziarski" <mich...@koziarski.com>
wrote:
> On Feb 8, 2008 2:00 PM, Glenn Rempe <glenn.re...@gmail.com> wrote:
>
>
>
>
>
> > 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?
>
> 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.
>
> --
> Cheers
>
> Koz
--~--~---------~--~----~------------~-------~--~----~
You received this message because you are subscribed to the Google Groups
"Ruby on Rails: Core" group.
To post to this group, send email to rubyonrails-core@googlegroups.com
To unsubscribe from this group, send email to
rubyonrails-core-unsubscribe@googlegroups.com
For more options, visit this group at
http://groups.google.com/group/rubyonrails-core?hl=en
-~----------~----~----~----~------~----~------~--~---

On Feb 9, 2008 7:35 PM, Glenn Rempe <glenn.rempe@gmail.com> wrote:
>
> 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 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.

--~--~---------~--~----~------------~-------~--~----~
You received this message because you are subscribed to the Google Groups
"Ruby on Rails: Core" group.
To post to this group, send email to rubyonrails-core@googlegroups.com
To unsubscribe from this group, send email to
rubyonrails-core-unsubscribe@googlegroups.com
For more options, visit this group at
http://groups.google.com/group/rubyonrails-core?hl=en
-~----------~----~----~----~------~----~------~--~---

On Feb 9, 1:44 pm, "Mislav Marohnić" <mislav.maroh...@gmail.com>
wrote:
> On Feb 9, 2008 7:35 PM, Glenn Rempe <glenn.re...@gmail.com> wrote:
>
> > 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.  :-)

> 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. :-)


--~--~---------~--~----~------------~-------~--~----~
You received this message because you are subscribed to the Google Groups
"Ruby on Rails: Core" group.
To post to this group, send email to rubyonrails-core@googlegroups.com
To unsubscribe from this group, send email to
rubyonrails-core-unsubscribe@googlegroups.com
For more options, visit this group at
http://groups.google.com/group/rubyonrails-core?hl=en
-~----------~----~----~----~------~----~------~--~---