:throw option for AR finders that don't throw RecordNotFound exceptions

[Tim Lucas]

When using ActiveRecord finders that don't by throw a RecordNotFound
exception I'm finding myself writing code like the following:

def show
@tag = Tag.find_by_name(params[:id]) or raise
ActiveRecord::RecordNotFound
end

I don't really like the idea of raising a RecordNotFound myself, as,
at the least, I don't get the exception message that ActiveRecord
includes if it throws the exception internally ("Couldn't find Tag
with ID=9999")... and it just feels... wrong.

Wouldn't it be better to have a finder option to specify you want an
exception thrown if there's no record found?

def show
@tag = Tag.find_by_name(params[:id], :throw_not_found => true)
end

Other possible APIs:

:throw_if_not_found => true
:throw_if_nil => true
:throw_when_not_found => true
:throw_when_nil => true
:throw_on_not_found => true
:throw_on_nil => true
:not_found => :throw

Thoughts?

-- tim lucas

[Jamis Buck]

Tim,

I like the idea. I'd suggest :must_exist => true as another possible API.

- Jamis

[Kevin Clark]

Isn't find_by_name handled in method missing? Could we add a check for
a bang at the end so:

@tag.find_by_name # => doesn't throw if nil
@tag.find_by_name! # => throws with nil

PDI

Kev

--
Kevin Clark
http://glu.ttono.us

[Michael Koziarski]

> Isn't find_by_name handled in method missing? Could we add a check for
> a bang at the end so:
>
> @tag.find_by_name # => doesn't throw if nil
> @tag.find_by_name! # => throws with nil
>
> PDI

I prefer the bang approach to the :some_hash_key thing. In the past
I've wanted:

Whatever.find(:one, ...) which is like find first but throws for <> 1
rows.... But this seems like a nice middle ground.

--
Cheers

Koz

[Tim Lucas]

On 30/08/2006, at 2:43 PM, Jamis Buck wrote:

> I like the idea. I'd suggest :must_exist => true as another
> possible API.
>
> - Jamis

I knew somebody would have a better suggestion :)

-- tim

[Jeremy Kemper]

On 8/29/06, Tim Lucas <t.l...@toolmantim.com> wrote:

Wouldn't it be better to have a finder option to specify you want an
exception thrown if there's no record found?

I'd prefer a bang! method.  I think it's a good indication that there should be separate 'loading' and 'finding' API -- passing ever more options to find is getting weary.

Loading a unique record from the database is semantically different than finding a record that meets some criteria and it could be treated as such.

jeremy

[Tim Lucas]

On 30/08/2006, at 2:44 PM, Kevin Clark wrote:

> Isn't find_by_name handled in method missing? Could we add a check for
> a bang at the end so:
>
> @tag.find_by_name # => doesn't throw if nil
> @tag.find_by_name! # => throws with nil

You'd then also have to add a find! and document this in both the
dynamic finders section of the docs as well as the find! method.

Seems simpler to add it as an option, but i do like the bang. hrmm...

Also, with a namespace of 1, could the bang possibly be needed for
another purpose in the future? Nothing really comes to mind.

-- tim

[Kevin Clark]

> You'd then also have to add a find! and document this in both the
> dynamic finders section of the docs as well as the find! method.

def find!(*args)
val = find(*args)
raise blah if val.nil? or (val == [])
return val
end

Or something like that. I wrote it in gmail so it isn't tested.

[Jarkko Laine]

On 30.8.2006, at 8.43, Kevin Clark wrote:

>
>> You'd then also have to add a find! and document this in both the
>> dynamic finders section of the docs as well as the find! method.
>
>
> def find!(*args)
> val = find(*args)
> raise blah if val.nil? or (val == [])

val.blank?, man ;-)

> return val
> end
>
> Or something like that. I wrote it in gmail so it isn't tested.
>
> --
> Kevin Clark
> http://glu.ttono.us
>

> --~--~---------~--~----~------------~-------~--~----~
> 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 rubyonra...@googlegroups.com
> To unsubscribe from this group, send email to rubyonrails-core-
> unsub...@googlegroups.com
> For more options, visit this group at http://groups.google.com/
> group/rubyonrails-core
> -~----------~----~----~----~------~----~------~--~---
>

--
Jarkko Laine
http://jlaine.net
http://odesign.fi

[Kevin Clark]

Does .blank? handle [] as well as "" and nil? I was going to use
.empty? but then I'd have to check that the method existed.

On 8/29/06, Jarkko Laine <jar...@jlaine.net> wrote:
>
> On 30.8.2006, at 8.43, Kevin Clark wrote:
>
> >
> >> You'd then also have to add a find! and document this in both the
> >> dynamic finders section of the docs as well as the find! method.
> >
> >
> > def find!(*args)
> > val = find(*args)
> > raise blah if val.nil? or (val == [])
>
> val.blank?, man ;-)
>
> > return val
> > end
> >
> > Or something like that. I wrote it in gmail so it isn't tested.
> >
> > --
> > Kevin Clark
> > http://glu.ttono.us
> >
> > > >
>

> --
> Jarkko Laine
> http://jlaine.net
> http://odesign.fi
>
>
>
>

[Andrew Kaspick]

blank? handles []

class Array #:nodoc:
alias_method :blank?, :empty?
end

[Andrew Kaspick]

From the source comments actually, so {} and " " too...

# "", " ", nil, [], and {} are blank

[Jarkko Laine]

On 30.8.2006, at 9.24, Kevin Clark wrote:
>
> Does .blank? handle [] as well as "" and nil? I was going to use
> .empty? but then I'd have to check that the method existed.

Oh yes, it's ingeniuos. See http://dev.rubyonrails.org/browser/trunk/
activesupport/lib/active_support/core_ext/blank.rb

//jarkko

[Kevin Clark]

wonderous. Either way, easy to write find!

On 8/29/06, Jarkko Laine <jar...@jlaine.net> wrote:

[Michael Koziarski]

On 8/30/06, Andrew Kaspick <akas...@gmail.com> wrote:
>

> blank? handles []
>
> class Array #:nodoc:
> alias_method :blank?, :empty?
> end
>

>> [].blank?
=> true
>> nil.blank?
=> true
>> "".blank?
=> true

--
Cheers

Koz

[Tim Lucas]

On 30/08/2006, at 4:50 PM, Kevin Clark wrote:

> wonderous. Either way, easy to write find!

module ActiveRecord
class Base
def find!(*args)
records = find(args)
raise RecordNotFound, "Couldn't find #{name} without an ID" if
records.blank?
records
end
end
end

works a treat. Adding ! to the method_missing finders might be a bit
trickier.

I've filed a ticket seeing as Aksimet seems to be in a good mood at
the moment:
http://dev.rubyonrails.org/ticket/5974

-- tim

[Jeremy Kemper]

On 8/30/06, Tim Lucas <t.l...@toolmantim.com> wrote:

I've filed a ticket seeing as Aksimet seems to be in a good mood at
the moment:

Akismet is disabled due to its patch gobbling.

jeremy

[Chris Wanstrath]

So, if this `find!' bidness gets introduced it means an exception can
get raised a) on record not found via find! / find_by_*! or b) on
record not found via find(id). Raised on bangs or on non-bang.

I know the non-bang is a very specific circumstance, but does this
bother anyone else? I suppose one way of looking at this is that the
bang methods are just sugar for existing methods, none of which
change. But it sure would be nice to just say "methods with a bang
raise an exception on blank, methods without do not."

I can deal with it; I love that this change is being discussed. But
won't someone think of the children?

--
Chris Wanstrath
http://errtheblog.com

[Steven A Bristol]

+1 to Chris.

I often override find(id) to return nil instead of raising. I would love to see find(*) return ||= nil and find!(*) return ||= raise.

Steven A Bristol
http://stevenbristol.blogspot.com

[Rick Olson]

-1

I count on the fact that find(id) raises an exception. If I'm looking
up a model by id and it's not there, there's usually a problem. The
raises RecordNotFound exception is a convenient hook to render a 404
page.

If you really want it to return nil, why not use find_by_id ?

--
Rick Olson
http://weblog.techno-weenie.net
http://mephistoblog.com

[Jeremy Kemper]

On 8/30/06, Chris Wanstrath <ch...@ozmm.org> wrote:

I remarked earlier that I think it's a good indication that there should be separate 'loading' and 'finding' API.  Find is way overloaded, though that can be valuable trait, as with Ruby's relatively small set of builtin collection classes each serving many needs.

Perhaps:
  Person.load(1)
  Person.load_by_name('bob')
meaning "give me the record that I've uniquely identified for you" versus
  Person.find_by_name('bob')
meaning "look for records matching the criteria I've given and return the first one"

jeremy

[Zack Chandler]

-1

I agree with Rick here. I count on find(id) throwing an exception. I
use find_by_id(id) if I want it the other way.

--
Zack Chandler
http://depixelate.com

[Jamie van Dyke]

-1 

I use find_by_id if I want nil, there's no need to change behaviour if it can be achieved using a different method.

Cheers,

Jamie van Dyke

Fear of Fish

http://www.fearoffish.co.uk

[Chris Wanstrath]

On Aug 30, 2006, at 12:39 PM, Zack Chandler wrote:
> On 8/30/06, Rick Olson <techno...@gmail.com> wrote:
>> If you really want it to return nil, why not use find_by_id ?
>

> I agree with Rick here. I count on find(id) throwing an exception. I
> use find_by_id(id) if I want it the other way.

I agree with you guys, all I'm saying is that if we go ahead with
find! we will have a `find' which does about a million different
things. It sometimes raises exceptions if there's a bang and
sometimes raises exceptions if there's not a bang. Complexity?

I like what Jeremy is saying about load vs find. I think that's what
I'm reaching for. When you find(13) you're asking Rails to load a
specific record. When you find_by_id(13) you're like "hey give me
this if you can, if not that's cool too." Maybe that is a route to
discuss.

[evn]

I think maybe the issue is that the behavior of find_by_id(id) and
find(id) is un-least-surprising.

I would be very happy if bang-find always threw an exception on empty,
no matter the parameters or dynamic inlined fields in the method name,
and no-bang-find never did. We already use ! for that purpose on create
and save as had been noted so I don't think it would make the API less
Ruby-esque.

Evan

[jo...@hasmanythrough.com]

Chris Wanstrath wrote:
> I agree with you guys, all I'm saying is that if we go ahead with
> find! we will have a `find' which does about a million different
> things. It sometimes raises exceptions if there's a bang and
> sometimes raises exceptions if there's not a bang. Complexity?
>
> I like what Jeremy is saying about load vs find. I think that's what
> I'm reaching for. When you find(13) you're asking Rails to load a
> specific record. When you find_by_id(13) you're like "hey give me
> this if you can, if not that's cool too." Maybe that is a route to
> discuss.

Yep. I brought up the point of find() being way overloaded back in Feb,
and got no traction. The current find() API is about 3 different APIs
rolled into one method. When not finding a record, find will either throw
an exception, return nil, or return an empty array, depending on if it was
called by id, :first, or :all. Having the method parse its params to
determine how to handle things is ugly. Yes, it's the same as
render(:whatever), but it's still ugly, moreso since the id case can
accept several kinds of values (int, list, array). Adding a find! and
find_by_whatever! and find_or_create_by_whatever! might have advantages in
places, but if we're really going there, can we maybe come up with a big
picture of where we want to be long-term and then figure out how to get
there in a way that's not too painful?

load() vs find() could be a good start, but it's obviously a 2.0 kind of
change since it is not backward compatible. My vote would include
separating the find(id), find(:first) and find(:all) cases into separate
methods. I know find_first and find_all are deprecated (for good reason -
the positional parameter API sucked), but perhaps something like find_one
and find_many would work.

--
Josh Susser
http://blog.hasmanythrough.com

[PJ Hyett]

Perhaps that's only because you've become accustomed to the convention?

I'm bothered by the inconsistency:

>> User.find_by_id(10)
=> nil
>> User.find(:first, :conditions => ["id = ?", 10])
=> nil
>> User.find(10)
ActiveRecord::RecordNotFound: Couldn't find User with ID=10

IMO, this makes quite a bit more sense when looking at the rest of the API:

>> User.find(10)
nil
>> User.find!(10)
ActiveRecord::RecordNotFound: Couldn't find User with ID=10

-PJ
http://www.pjhyett.com

On 8/30/06, Rick Olson <techno...@gmail.com> wrote:
>

[Rick Olson]

> I know find_first and find_all are deprecated (for good reason -
> the positional parameter API sucked), but perhaps something like find_one
> and find_many would work.

public :find_initial
public :find_every

there you go.

[Damian Janowski]

IMHO bang wouldn't follow Ruby's convention (ie that "!" makes something
destructive to the object).

Something along "find_or_raise" would follow Rails' "find_or_create"
convention.

But +1 to load vs find. Sounds cool.

[Jonathan Viney]

Another thought .... how about replacing find(:first) with find(:one). :first implies some sort of order where none actually exists.

-Jonathan.

[Damian Janowski]

Because find(:first) will actually bring you the first record of the result set. If you include :conditions and :order, then you have a restricted set with a specific order. Then :first makes sense.

 

Although find(:one) would be awesome for doing a random -- "bring me a random record from this result set" :)

---

From: rubyonra...@googlegroups.com [mailto:rubyonra...@googlegroups.com] On Behalf Of Jonathan Viney
Sent: Wednesday, August 30, 2006 8:48 PM
To: rubyonra...@googlegroups.com

[Rob Sanheim]

On 8/30/06, Damian Janowski <djan...@dimaion.com> wrote:
>
> IMHO bang wouldn't follow Ruby's convention (ie that "!" makes something
> destructive to the object).
>
> Something along "find_or_raise" would follow Rails' "find_or_create"
> convention.
>
> But +1 to load vs find. Sounds cool.

Actually, if you search around ruby-talk the bang methods can just be
used more generally to mean a "dangerous operation". At least this is
what I found in a thread from 04:

http://blade.nagaokaut.ac.jp/cgi-bin/scat.rb/ruby/ruby-talk/120093

So using find! would be totally appropriate....for a similiar example
in core ruby see Kernel#exit and exit!.

- rob

--
http://www.robsanheim.com
http://www.seekingalpha.com
http://www.ajaxian.com

[Jonathan Viney]

It seems to me that :first only makes sense if you add an :order option, because then there is a record that can logically be first. But when you use it on its own it seems to imply some sort of order that doesn't actually exist.

When I've been teaching Rails I've often found that people expect find(:first) to return the record with the lowest id number, which is often not the case. find(:one) would probably remove that bit of confusion.

But I agreee that once you add an :order clause, find(:first, :order => ... ) makes more sense :).

[Damian Janowski]

I stand corrected :) Thanks for the link.

I'm still having a hard time looking for the semantics of "find!" (I mean -
what's the dangerous operation being performed?)

I'll keep reading your posts and think about it :)

-----Original Message-----
From: rubyonra...@googlegroups.com
[mailto:rubyonra...@googlegroups.com] On Behalf Of Rob Sanheim
Sent: Wednesday, August 30, 2006 10:14 PM
To: rubyonra...@googlegroups.com
Subject: [Rails-core] Re: :throw option for AR finders that don't throw
RecordNotFound exceptions

[Marcus Brito]

Jonathan Viney wrote:

> Another thought .... how about replacing find(:first) with find(:one).
> :first implies some sort of order where none actually exists.

Not replacing, but creating a new option. find :first would find only
the first record; if there are more than one, only the first is
returned. find :one would imply that one and only one match should
exist: if no match is found, RecordNotFound would be raised; if more
than one match is found, something like NonUniqueResult would be raised.

-- Marcus Brito

[Martin Emde]

On 8/30/06, Damian Janowski <djan...@dimaion.com> wrote:

I'm still having a hard time looking for the semantics of "find!" (I mean -
what's the dangerous operation being performed?)

There's a few examples in Rails core for this exact sort of behaviour:

ActiveRecord::Base.create vs. ActiveRecord::Base.create!

If create fails it returns an unsaved record. If create! fails you end up with a raised exception.
Base.create! calls Base.save! which, again, raises an exception instead of returning false on failure.

To me the bang on a method that is to perform the same actions as the non-bang method would mean "if this fails, you MUST handle it" instead of the non-bang method which fails silently with only a nil/false return value.

I also second the idea of a some sort of api split between loads and finds but it certainly needs refinement. I think an api change like this early on is better than a confusing or overloaded api that eventually drives people away from the framework.

--
Martin Emde