Document#find_by_id in MongoMapper 0.3.1

[David James]

I'd like to call attention to a change in 0.3.1 that is affecting my
project and might affect others too.

Summary: In some cases Document#find_by_id(the_id) gives nil, in other
cases it explodes.

The breakdown goes like this:
* If the_id can be converted to the ObjectID format, and is found, it
returns the document.
* If the_id can be converted to the ObjectID format, but is not found,
it returns nil.
* If the_id cannot be converted to the ObjectID format, an
MongoMapper::DocumentNotFound exception is raised.

This is a change from 0.2.

I'm not sure how I feel about it. On one hand, it feels like extra
work to have to worry about both failure modes. On the other hand,
maybe it makes sense: if you are constructing bad document id's you
might want to know about it right away.

[David James]

After looking at the code, I have a suggestion that doesn't change the
behavior, but might be a technical improvement. It would involve small
changes to both mongo ruby driver and mongomapper...

I noticed that the mongo ruby driver does not use a named exception
class corresponding to 'illegal ObjectID format'. It just raises an
exception with that string. Might it be better to give it a name, e.g.
IllegalObjectIDFormat? This would allow MongoMapper to rescue that
exception specifically -- or pass it along if it wanted.

This is what the code looks like now in mongomapper-0.3.1/lib/
mongomapper.rb:

def self.mm_typecast(value)
begin
if value.is_a?(XGen::Mongo::Driver::ObjectID)
value
else
XGen::Mongo::Driver::ObjectID::from_string(value.to_s)
end
rescue => exception
if exception.message == 'illegal ObjectID format'
raise MongoMapper::DocumentNotFound
else
raise exception
end
end
end


And the code in mongodb-mongo-0.10.1/lib/mongo/types/objectid.rb:

# Given a string representation of an ObjectID, return a new
ObjectID
# with that value.
def self.from_string(str)
raise "illegal ObjectID format" unless legal?(str)
data = []
BYTE_ORDER.each_with_index { |string_position, data_index|
data[data_index] = str[string_position * 2, 2].to_i(16)
}
self.new(data)
end

[John Nunemaker]

Ah good catch. I'll fix this up. I have some ideas.



On Jul 31, 2009, at 6:22 PM, David James

[scottmotte]

John, shouldn't it just return nil so we can raise NotFound or
whatever we want in our application?

I don't see a use case why it would be useful to raise a 500 error for
an illegal id. It unduly makes your app look broken. A user might type
in /tweets/thisisafaketweet - this is an illegal id because it doesn't
stick with the mongodb format, but should it cause a 500 error? I
think it should return nil.

def show(id)
@tweet = Tweet.find(id) # currently if i put 'thisisafaketweet' I
get a 500 error illegal id. should just return nil so I can raise my
404 page.
raise NotFound unless @tweet
display @tweet
end

[Brian Smith]

seconded

[John Nunemaker]

Find should never return nil. The goal really is to send 404 if something isn't found. I would just use rails rescue from to rescue document not found and illegal is and render a 404. 

[Scott Motte]

John, are you sure about that? I'm 90% sure Datamapper returns nil. ActiveRecord is kinda sloppy in my opinion the way it returns exceptions.

I would disagree that the the goal is to send a 404 if something isn't found. My two cents, is that raising exceptions should be the task of the developer in the framework of their choice. The orm should focus on mapping things together and make the data easy to access, but when it's not there it should return nil.

For example, sometimes I want to raise a 401 Unauthorized error if I can't find the user. And once in a while, I might even want to raise a 402 Payment required if I can't find the paid user in the database. Then (in merb at least) I'm able to create my own custom 404, 401, and 402 pages that make sense to the user and read cleanly in my actions.

I'm coming from a merb perspective though that does exceptions in a different and improved manner to rails, but your rescue idea does work. It just feels dirty.

[Sho Fukamachi]

Thirded but with the stipulation that this seems to be a
philosophical / stylistic issue about which reasonable people can and
do disagree.

Perhaps a compromise is possible by creating both a "soft" and "hard"
find, ie #find and #find! - one returns a nil and the other raises
upon a RecordNotFound event. Best of both worlds?

[John Nunemaker]

I like the way AR does exceptions just fine. "Improved" is probably relative. I'll think about it but changing that is low priority for me right now. 

[David James]

Fourthed.

I agree with Sho's assessment. To sum it up: "to raise or not to
raise, that is the question."

I also like Sho's compromise solution. #find should return nil if a
record is not found. My supporting reasons are:
1. It is not 'exceptional' behavior that a find fail. So raising an
exception doesn't make sense.
2. I like the idea of following the lead of Ruby's Enumerable#find. It
doesn't raise an exception.
3. If someone really wants MongoMapper to raise an exception, then let
them have #find!

[Scott Motte]

Yea, I really like Sho's idea too.

[John Nunemaker]

I've been thinking this over and I think I'm cool with find returning
nil and find! raising an exception. I'll put in a ticket. If anyone
wants to tackle the implementation that will give me a good start. If
not, I'll get to it when I can.