my ($self, $code, @args) = @_;
return $code->(@args) if $self->in_transaction;
- if (wantarray) {
- my @result;
- return $self->do_transaction(sub { @result = $code->(@args) }) ? @result : ();
- } else {
- my $result;
- return $self->do_transaction(sub { $result = $code->(@args) }) ? $result : undef;
- }
+ my (@result, $result);
+ my $rv = 1;
+
+ local $@;
+
+ eval {
+ wantarray
+ ? $self->do_transaction(sub { @result = $code->(@args) })
+ : $self->do_transaction(sub { $result = $code->(@args) });
+ } or do {
+ my $error = $self->error;
+ if (ref $error) {
+ if ($error->isa('SL::X::DBError')) {
+ # gobble the exception
+ } else {
+ $error->rethrow;
+ }
+ } else {
+ die $self->error;
+ }
+ };
+
+ return wantarray ? @result : $result;
}
1;
# do stuff with $self
});
-There are two big differences between C<with_transaction> and
-L<Rose::DB/do_transaction>: the handling of an already-running
-transaction and the handling of return values.
+This is a wrapper around L<Rose::DB/do_transaction> that does a few additional
+things, and should always be used in favour of the other:
-The first difference revolves around when a transaction is started and
-committed/rolled back. Rose's C<do_transaction> will always start one,
-then execute the code reference and commit afterwards (or roll back if
-an exception occurs).
+=over 4
-This prevents the caller from combining several pieces of code using
-C<do_transaction> reliably as results committed by an inner
-transaction will be permanent even if the outer transaction is rolled
-back.
+=item Composition of transactions
-Therefore our C<with_transaction> works differently: it will only
-start a transaction if no transaction is currently active on the
-database connection.
+When C<with_transaction> is called without a running transaction, a new one is
+created. If it is called within a running transaction, it performs no
+additional handling. This means that C<with_transaction> can be safely used
+within another C<with_transaction>, whereas L<Rose::DB/do_transaction> can not.
-The second big difference to L<Rose::DB/do_transaction> is the
-handling of returned values. Basically our C<with_transaction> will
-return the values that the code reference C<$code_ref> returns (or
-C<undef> if the transaction was rolled back). Rose's C<do_transaction>
-on the other hand will only return a value signaling the transaction's
-status.
+=item Return values
-In more detail:
+C<with_transaction> adopts the behaviour of C<eval> in that it returns the
+result of the inner block, and C<undef> if an error occured. This way you can
+use the same pattern you would normally use with C<eval> for
+C<with_transaction>:
-=over 2
+ SL::DB->client->with_transaction(sub {
+ # do stuff
+ # and return nominal true value
+ 1;
+ }) or do {
+ # transaction error handling
+ my $error = SL::DB->client->error;
+ }
-=item * If a transaction is already active then C<with_transaction>
-will simply return the result of calling C<$code_ref> as-is preserving
-context.
+or you can use it to safely calulate things.
-=item * If no transaction is started then C<$code_ref> will be wrapped
-in one. C<with_transaction>'s return value depends on the result of
-that transaction. If the it succeeds then the return value of
-C<$code_ref> will be returned preserving context. Otherwise C<undef>
-will be returned in scalar context and an empty list in list context.
+=item Error handling
-=back
+The original L<Rose::DB/do_transaction> gobbles up all execptions and expects
+the caller to manually check return value and error, and then to process all
+exceptions as strings. This is very fragile and generally a step backwards from
+proper exception handling.
+
+C<with_transaction> only gobbles up exception that are used to signal an
+error in the transaction, and returns undef on those. All other exceptions
+bubble out of the transaction like normal, so that it is transparent to typoes,
+runtime exceptions and other generally wanted things.
+
+If you just use the snippet above, your code will catch everything related to
+the transaction aborting, but will not catch other errors that might have been
+thrown. The transaction will be rollbacked in both cases.
-So if you want to differentiate between "transaction failed" and
-"succeeded" then your C<$code_ref> should never return C<undef>
-itself.
+If you want to play nice in case your transaction is embedded in another
+transaction, just rethrow the error:
+
+ $db->with_transaction(sub {
+ # code deep in the engine
+ 1;
+ }) or die $db->error;
+
+=back
=back
package SL::DB::Helper::Metadata;
use strict;
+use SL::X;
use Rose::DB::Object::Metadata;
use SL::DB::Helper::ConventionManager;
SL::DB::Helper::Attr::auto_make($self->class);
}
+sub handle_error {
+ my($self, $object) = @_;
+
+ # these are used as Rose internal canaries, don't wrap them
+ die $object->error if UNIVERSAL::isa($object->error, 'Rose::DB::Object::Exception');
+
+ die SL::X::DBRoseError->new(
+ error => $object->error,
+ class => ref($object),
+ metaobject => $self,
+ object => $object,
+ );
+}
+
1;
SL::DB::Object::Hooks::run_hooks($self, 'after_save', $result);
1;
- }) || die $self->error;
+ }) || die $self->db->error;
return $result;
}
SL::DB::Object::Hooks::run_hooks($self, 'after_delete', $result);
1;
- }) || die $self->error;
+ }) || die $self->db->error;
return $result;
}
}
sub dberror {
- $main::lxdebug->enter_sub();
-
my ($self, $msg) = @_;
- $self->error("$msg\n" . $DBI::errstr);
-
- $main::lxdebug->leave_sub();
+ die SL::X::DBError->new(
+ msg => $msg,
+ error => $DBI::errstr,
+ );
}
sub isblank {
use Exception::Lite qw(declareExceptionClass);
declareExceptionClass('SL::X::FormError');
-declareExceptionClass('SL::X::DBHookError', [ '%s hook \'%s\' for object type \'%s\' failed', qw(when hook object_type object) ]);
+declareExceptionClass('SL::X::DBError');
+declareExceptionClass('SL::X::DBHookError', 'SL::X::DBError', [ '%s hook \'%s\' for object type \'%s\' failed', qw(when hook object_type object) ]);
+declareExceptionClass('SL::X::DBRoseError', 'SL::X::DBError', [ '\'%s\' in object of type \'%s\' occured', qw(error class) ]);
+declareExceptionClass('SL::X::DBUtilsError', 'SL::X::DBError', [ '%s: %s', qw(msg error) ]);
1;
--- /dev/null
+use Test::More tests => 17;
+use Test::Exception;
+
+use strict;
+
+use lib 't';
+use utf8;
+
+use Carp;
+use Data::Dumper;
+use Support::TestSetup;
+use SL::DB::Part;
+use SL::Dev::Part;
+
+Support::TestSetup::login();
+
+SL::DB::Manager::Part->delete_all(all => 1, cascade => 1);
+
+# silence the Test::Harness warn handler
+local $SIG{__WARN__} = sub {};
+
+# test simple transaction
+
+my $part = create_part();
+SL::DB->client->with_transaction(sub {
+ $part->save;
+ ok 1, 'part saved';
+ 1;
+}) or do {
+ ok 0, 'error saving part';
+};
+
+# test failing transaction
+my $part2 = create_part(partnumber => $part->partnumber); # woops, duplicate partnumber
+SL::DB->client->with_transaction(sub {
+ $part2->save;
+ ok 0, 'part saved';
+ 1;
+}) or do {
+ ok 1, 'saving part with duplicate partnumber generates graceful error';
+};
+
+# test transaction with run time exception
+dies_ok {
+ SL::DB->client->with_transaction(sub {
+ $part->method_that_does_not_exist;
+ ok 0, 'this should have died';
+ 1;
+ }) or do {
+ ok 0, 'this should not get here';
+ };
+} 'method not found in transaction died as expect';
+
+# test transaction with hook error
+# TODO - not possible to test without locally adding hooks in run time
+
+# test if error gets correctly stored in db->error
+$part2 = create_part(partnumber => $part->partnumber); # woops, duplicate partnumber
+SL::DB->client->with_transaction(sub {
+ $part2->save;
+ ok 0, 'part saved';
+ 1;
+}) or do {
+ like(SL::DB->client->error, qr/duplicate key value violates unique constraint/, 'error is in db->error');
+};
+
+# test stacked transactions
+# 1. test that it works
+SL::DB->client->with_transaction(sub {
+ $part->sellprice(1);
+ $part->save;
+
+ SL::DB->client->with_transaction(sub {
+ $part->sellprice(2);
+ $part->save;
+ }) or do {
+ ok 0, 'error saving part';
+ };
+
+ $part->sellprice(3);
+ $part->save;
+ 1;
+}) or do {
+ ok 0, 'error saving part';
+};
+
+$part->load;
+is $part->sellprice, "3.00000", 'part saved';
+
+# 2. with a transaction rollback
+SL::DB->client->with_transaction(sub {
+ $part->sellprice(1);
+ $part2->save;
+ $part->save;
+
+ SL::DB->client->with_transaction(sub {
+ $part->sellprice(2);
+ $part->save;
+ }) or do {
+ ok 0, 'should not get here';
+ };
+
+ $part->sellprice(3);
+ $part->save;
+ ok 0, 'should not get here';
+ 1;
+}) or do {
+ ok 1, 'sql error skips rest of the transaction';
+};
+
+
+SL::DB->client->with_transaction(sub {
+ $part->sellprice(1);
+ $part->save;
+
+ SL::DB->client->with_transaction(sub {
+ $part->sellprice(2);
+ $part->save;
+ $part2->save;
+ }) or do {
+ ok 0, 'should not get here';
+ };
+
+ $part->sellprice(3);
+ $part->save;
+ ok 0, 'should not get here';
+ 1;
+}) or do {
+ ok 1, 'sql error in nested transaction rolls back';
+ like(SL::DB->client->error, qr/duplicate key value violates unique constraint/, 'error from nested transaction is in db->error');
+};
+
+$part->load;
+is $part->sellprice, "3.00000", 'saved part is not affected';
+
+
+
+SL::DB->client->with_transaction(sub {
+ $part->sellprice(1);
+ $part->save;
+
+ SL::DB->client->with_transaction(sub {
+ $part->sellprice(2);
+ $part->save;
+ }) or do {
+ ok 0, 'should not get here';
+ };
+
+ $part->sellprice(4);
+ $part->save;
+ $part2->save;
+ ok 0, 'should not get here';
+ 1;
+}) or do {
+ ok 1, 'sql error after nested transaction rolls back';
+};
+
+$part->load;
+is $part->sellprice, "3.00000", 'saved part is not affected';
+
+eval {
+ SL::DB->client->with_transaction(sub {
+ $part->sellprice(1);
+ $part->not_existing_function();
+ $part->save;
+
+ SL::DB->client->with_transaction(sub {
+ $part->sellprice(2);
+ $part->save;
+ }) or do {
+ ok 0, 'should not get here';
+ };
+
+ $part->sellprice(4);
+ $part->save;
+ ok 0, 'should not get here';
+ 1;
+ }) or do {
+ ok 0, 'should not get here';
+ };
+ 1;
+} or do {
+ ok 1, 'runtime exception error before nested transaction rolls back';
+};
+
+$part->load;
+is $part->sellprice, "3.00000", 'saved part is not affected';
+
+eval {
+ SL::DB->client->with_transaction(sub {
+ $part->sellprice(1);
+ $part->save;
+
+ SL::DB->client->with_transaction(sub {
+ $part->sellprice(2);
+ $part->not_existing_function();
+ $part->save;
+ }) or do {
+ ok 0, 'should not get here';
+ };
+
+ $part->sellprice(4);
+ $part->save;
+ ok 0, 'should not get here';
+ 1;
+ }) or do {
+ ok 0, 'should not get here';
+ };
+ 1;
+} or do {
+ ok 1, 'runtime exception error in nested transaction rolls back';
+};
+
+$part->load;
+is $part->sellprice, "3.00000", 'saved part is not affected';
+
+
+eval {
+ SL::DB->client->with_transaction(sub {
+ $part->sellprice(1);
+ $part->save;
+
+ SL::DB->client->with_transaction(sub {
+ $part->sellprice(2);
+ $part->save;
+ }) or do {
+ ok 0, 'should not get here';
+ };
+
+ $part->sellprice(4);
+ $part->save;
+ $part->not_existing_function();
+ ok 0, 'should not get here';
+ 1;
+ }) or do {
+ ok 0, 'should not get here';
+ };
+ 1;
+} or do {
+ ok 1, 'runtime exception error after nested transaction rolls back';
+};
+
+$part->load;
+is $part->sellprice, "3.00000", 'saved part is not affected';
+
projects / kivitendo-erp.git / commitdiff