Bug #56440 for MongoDB: 'safe' operations returning 0 on fail

Fri Apr 09 15:48:50 2010 EWILHELM [...] cpan.org - Ticket created

Subject:

'safe' operations returning 0 on fail

The insert() method in version 0.28 would throw an error with the {safe => 1} option, but newer versions silently return 0. Throwing an error was a better design, because " or die MongoDB::Database->last_error" is tedious to write, easily missed, and prone to synchronization errors because the error is disconnected from the origin. Using eval {} or try/catch gives more flexibility for catching errors at various depths of scope and thus cleaner code. The new remove(), update(), and ensure_index() 'safe' options seem to have this same misfeature. If there is some good reason why these operations cannot throw an error, the Changes file should have noted this incompatible API change because it will cause silent breakage of any code which was expecting the error semantics. Thanks, Eric

Fri Apr 09 15:58:13 2010 KRISTINA [...] cpan.org - Correspondence added

I've put this here: http://jira.mongodb.org/browse/PERL-75 I see your point, but the people on IRC I consulted before changing this unanimously thought that dies were annoying to deal with and it wasn't a serious enough error to warrant their use. I'd like to get some more opinions. On Fri Apr 09 15:48:50 2010, EWILHELM wrote: Show quoted text

> The insert() method in version 0.28 would throw an error with the > {safe => 1} option, but newer versions silently return 0. Throwing an > error was a better design, because " or die > MongoDB::Database->last_error" is tedious to write, easily missed, and > prone to synchronization errors because the error is disconnected from > the origin. Using eval {} or try/catch gives more flexibility for > catching errors at various depths of scope and thus cleaner code. > > The new remove(), update(), and ensure_index() 'safe' options seem to > have this same misfeature. > > If there is some good reason why these operations cannot throw an > error, the Changes file should have noted this incompatible API change > because it will cause silent breakage of any code which was expecting > the error semantics. > > Thanks, > Eric

Fri Apr 09 15:58:15 2010 The RT System itself - Status changed from 'new' to 'open'

Fri Apr 09 18:29:41 2010 enobacon [...] gmail.com - Correspondence added

Subject:	Re: [rt.cpan.org #56440] 'safe' operations returning 0 on fail
Date:	Fri, 9 Apr 2010 15:29:23 -0700
To:	bug-MongoDB [...] rt.cpan.org
From:	Eric Wilhelm <enobacon [...] gmail.com>

# from Kristina Chodorow via RT # on Friday 09 April 2010 12:58: Show quoted text

>I see your point, but the people on IRC I consulted before changing > this unanimously thought that dies were annoying to deal with and it > wasn't a serious enough error to warrant their use.

Hi Kristina, I'm not sure which channel gave you this kind of opinions, but that's counter to most modern APIs and more like C than Perl. It's trivial to use eval {} to trap or ignore errors, so IMO any case where the code has failed to carry out its duty is severe enough to throw an error. That is, the flip-side of "dies are annoying" is that silent failure causes silly bugs (or: having to write "or die" after every call is annoying.) Throwing exceptions gives you a side-channel for handling errors, which means that you don't have to clutter-up the main logic with checking whether or not every method did its job (i.e. produced the intended side-effect.) Imagine in the following code that we're working at one or two method calls deep in scope. How do we get out without throwing an error? Each level has to check return values -- and if 0 or undef is a valid answer at any level we'll have to jump through hoops. unless($coll->insert(...)) { # insert failed, now perform some contortions } This just gets more complicated when some failures could throw an error and others could return zero. vs. the typical "let the errors propagate" usage: $coll->insert(...); which occasionally involves the "or try something else" pattern: eval {$coll->insert(...)}; if($@) { # retry or whatever } Using "throw an error" semantics, the logic of trapping the error can happen at whichever depth, which is far cleaner than "silently return a failure indicator" where you potentially end up with error handling logic at every level of a multi-level abstraction. More logic means more potential bugs. I can see some code in Collection.pm which is already growing extra logic because of not throwing errors. Thanks, Eric

Fri Apr 09 19:15:25 2010 EWILHELM [...] cpan.org - Correspondence added

See attached patch.

Subject:

exceptions.patch

diff --git a/lib/MongoDB/Connection.pm b/lib/MongoDB/Connection.pm index 819d87c..6e11670 100644 --- a/lib/MongoDB/Connection.pm +++ b/lib/MongoDB/Connection.pm @@ -255,6 +255,12 @@ has _last_error => ( is => 'rw', ); +sub croak (@) { + # Mouse delegation is doing something weird here - and confess is overkill + local $Carp::CarpLevel = 4; + Carp::croak(@_); +} + sub _get_hosts { my ($self) = @_; my @hosts; @@ -352,8 +358,8 @@ sub query { sub insert { my ($self, $ns, $object, $options) = @_; - my @id = $self->batch_insert($ns, [$object], $options); - return exists $id[0] ? $id[0] : 0; + my ($id) = $self->batch_insert($ns, [$object], $options); + return $id; } sub _make_safe { @@ -370,10 +376,8 @@ sub _make_safe { my $ok = $cursor->next(); $self->_last_error($ok); + croak $ok->{err} if $ok->{err}; - if ($ok->{err}) { - return 0; - } return 1; } diff --git a/t/collection.t b/t/collection.t index 8db91b5..1ea784e 100644 --- a/t/collection.t +++ b/t/collection.t @@ -350,8 +350,8 @@ is($obj->{data}, 3.3); { $coll->drop; $coll->insert({_id => 1}, {safe => 1}); - $ok = $coll->insert({_id => 1}, {safe => 1}); - is($ok, 0); + eval {$coll->insert({_id => 1}, {safe => 1})}; + ok($@ and $@ =~ /^E11000/, 'duplicate key exception'); SKIP: { skip "the version of the db you're running doesn't give error codes, you may wish to consider upgrading", 1 if !exists $db->last_error->{code};

Fri Apr 16 15:01:11 2010 KRISTINA [...] cpan.org - Correspondence added

The majority says it should croak, so I've applied your patch and it will show up in 0.32! Thanks! http://github.com/mongodb/mongo-perl- driver/commit/706620c4cbe9c28a29461bf213a8b8bb80723395

Fri Apr 16 15:01:12 2010 KRISTINA [...] cpan.org - Status changed from 'open' to 'resolved'

Bug #56440 for MongoDB: 'safe' operations returning 0 on fail

Preferred bug tracker

Maintainer(s)' notes