From: Peter Rabbitson <ribasushi@cpan.org>
Date: Mon, 13 Sep 2010 00:21:37 +0000 (+0200)
Subject: Fix the exception-hungry exception_action
X-Git-Tag: v0.08124~77
X-Git-Url: http://git.shadowcat.co.uk/gitweb/gitweb.cgi?a=commitdiff_plain;h=c3e9f7189094e94137356251c4f0b1f1cbfeb04a;p=dbsrgits%2FDBIx-Class.git

Fix the exception-hungry exception_action
---

diff --git a/Changes b/Changes
index fc93f08..2195bab 100644
--- a/Changes
+++ b/Changes
@@ -15,6 +15,9 @@ Revision history for DBIx::Class
         - Optimized RowNum based Oracle limit-dialect (RT#61277)
 
     * Fixes
+        - Make sure exception_action does not allow exception-hiding
+          due to badly-written handlers (the mechanism was never meant
+          to be able to suppress exceptions)
         - Fixed rels ending with me breaking subqueried limit realiasing
         - Oracle sequence detection now *really* works across schemas
           (fixed some ommissions from 0.08123)
diff --git a/lib/DBIx/Class/Schema.pm b/lib/DBIx/Class/Schema.pm
index 8046903..77a7fe7 100644
--- a/lib/DBIx/Class/Schema.pm
+++ b/lib/DBIx/Class/Schema.pm
@@ -4,7 +4,7 @@ use strict;
 use warnings;
 
 use DBIx::Class::Exception;
-use Carp::Clan qw/^DBIx::Class/;
+use Carp::Clan qw/^DBIx::Class|^Try::Tiny/;
 use Try::Tiny;
 use Scalar::Util 'weaken';
 use File::Spec;
@@ -438,14 +438,13 @@ L<DBIx::Class::Storage::DBI::Replicated> for an example of this.
 
 =back
 
-If C<exception_action> is set for this class/object, L</throw_exception>
-will prefer to call this code reference with the exception as an argument,
-rather than L<DBIx::Class::Exception/throw>.
+When L</throw_exception> is invoked and L</exception_action> is set to a code
+reference, this reference will be called instead of
+L<DBIx::Class::Exception/throw>, with the exception message passed as the only
+argument.
 
-Your subroutine should probably just wrap the error in the exception
-object/class of your choosing and rethrow.  If, against all sage advice,
-you'd like your C<exception_action> to suppress a particular exception
-completely, simply have it return true.
+Your custom throw code B<must> rethrow the exception, as L</throw_exception> is
+an integral part of DBIC's internal execution control flow.
 
 Example:
 
@@ -459,9 +458,6 @@ Example:
    my $schema_obj = My::Schema->connect( .... );
    $schema_obj->exception_action(sub { My::ExceptionClass->throw(@_) });
 
-   # suppress all exceptions, like a moron:
-   $schema_obj->exception_action(sub { 1 });
-
 =head2 stacktrace
 
 =over 4
@@ -1031,11 +1027,29 @@ default behavior will provide a detailed stack trace.
 
 =cut
 
+my $false_exception_action_warned;
 sub throw_exception {
   my $self = shift;
 
-  DBIx::Class::Exception->throw($_[0], $self->stacktrace)
-    if !$self->exception_action || !$self->exception_action->(@_);
+  if (my $act = $self->exception_action) {
+    if ($act->(@_)) {
+      DBIx::Class::Exception->throw(
+          "Invocation of the exception_action handler installed on $self did *not*"
+        .' result in an exception. DBIx::Class is unable to function without a reliable'
+        .' exception mechanism, ensure that exception_action does not hide exceptions'
+        ." (original error: $_[0])"
+      );
+    }
+    elsif(! $false_exception_action_warned++) {
+      carp (
+          "The exception_action handler installed on $self returned false instead"
+        .' of throwing an exception. This behavior has been deprecated, adjust your'
+        .' handler to always rethrow the supplied error.'
+      );
+    }
+  }
+
+  DBIx::Class::Exception->throw($_[0], $self->stacktrace);
 }
 
 =head2 deploy
diff --git a/t/34exception_action.t b/t/34exception_action.t
index 0fec11a..e19ef1f 100644
--- a/t/34exception_action.t
+++ b/t/34exception_action.t
@@ -2,6 +2,8 @@ use strict;
 use warnings;
 
 use Test::More;
+use Test::Exception;
+use Test::Warn;
 use lib qw(t/lib);
 use DBICTest;
 
@@ -16,29 +18,42 @@ sub throwex { $schema->resultset("Artist")->search(1,1,1); }
 my $ex_regex = qr/Odd number of arguments to search/;
 
 # Basic check, normal exception
-eval { throwex };
-my $e = $@; # like() seems to stringify $@
-like($@, $ex_regex);
+throws_ok { throwex }
+  $ex_regex;
+
+my $e = $@;
 
 # Re-throw the exception with rethrow()
-eval { $e->rethrow };
+throws_ok { $e->rethrow }
+  $ex_regex;
 isa_ok( $@, 'DBIx::Class::Exception' );
-like($@, $ex_regex);
 
 # Now lets rethrow via exception_action
 $schema->exception_action(sub { die @_ });
-eval { throwex };
-like($@, $ex_regex);
+throws_ok { throwex }
+  $ex_regex;
 
+#
+# This should have never worked!!!
+#
 # Now lets suppress the error
 $schema->exception_action(sub { 1 });
-eval { throwex };
-ok(!$@, "Suppress exception");
+throws_ok { throwex }
+  qr/exception_action handler .+ did \*not\* result in an exception.+original error: $ex_regex/;
 
 # Now lets fall through and let croak take back over
 $schema->exception_action(sub { return });
-eval { throwex };
-like($@, $ex_regex);
+throws_ok {
+  warnings_are { throwex }
+    qr/exception_action handler installed .+ returned false instead throwing an exception/;
+} $ex_regex;
+
+# again to see if no warning
+throws_ok {
+  warnings_are { throwex }
+    [];
+} $ex_regex;
+
 
 # Whacky useless exception class
 {
@@ -60,12 +75,11 @@ like($@, $ex_regex);
 
 # Try the exception class
 $schema->exception_action(sub { DBICTest::Exception->throw(@_) });
-eval { throwex };
-like($@, qr/DBICTest::Exception is handling this: $ex_regex/);
+throws_ok { throwex }
+  qr/DBICTest::Exception is handling this: $ex_regex/;
 
 # While we're at it, lets throw a custom exception through Storage::DBI
-eval { $schema->storage->throw_exception('floob') };
-like($@, qr/DBICTest::Exception is handling this: floob/);
-
+throws_ok { $schema->storage->throw_exception('floob') }
+  qr/DBICTest::Exception is handling this: floob/;
 
 done_testing;