From 660c7e5312f7fae7766b731f7001e5e8197c6887 Mon Sep 17 00:00:00 2001 From: =?utf8?q?Sven=20Sch=C3=B6ling?= Date: Thu, 26 Jan 2017 16:38:21 +0100 Subject: [PATCH] DB Transaktionen - Fehler nach oben durchreichen --- SL/DB.pm | 109 +++++++++------ SL/DB/Helper/Metadata.pm | 15 ++ SL/DB/Object.pm | 4 +- SL/Form.pm | 9 +- SL/X.pm | 5 +- t/db_helper/with_transaction.t | 245 +++++++++++++++++++++++++++++++++ 6 files changed, 338 insertions(+), 49 deletions(-) create mode 100644 t/db_helper/with_transaction.t diff --git a/SL/DB.pm b/SL/DB.pm index f12584754..40c8a9596 100644 --- a/SL/DB.pm +++ b/SL/DB.pm @@ -126,14 +126,30 @@ sub with_transaction { 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; @@ -173,50 +189,61 @@ starting one only if none is currently active. Example: # do stuff with $self }); -There are two big differences between C and -L: the handling of an already-running -transaction and the handling of return values. +This is a wrapper around L 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 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 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 works differently: it will only -start a transaction if no transaction is currently active on the -database connection. +When C 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 can be safely used +within another C, whereas L can not. -The second big difference to L is the -handling of returned values. Basically our C will -return the values that the code reference C<$code_ref> returns (or -C if the transaction was rolled back). Rose's C -on the other hand will only return a value signaling the transaction's -status. +=item Return values -In more detail: +C adopts the behaviour of C in that it returns the +result of the inner block, and C if an error occured. This way you can +use the same pattern you would normally use with C for +C: -=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 -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'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 -will be returned in scalar context and an empty list in list context. +=item Error handling -=back +The original L 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 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 -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 diff --git a/SL/DB/Helper/Metadata.pm b/SL/DB/Helper/Metadata.pm index 9ce1f1192..4eaa106f4 100644 --- a/SL/DB/Helper/Metadata.pm +++ b/SL/DB/Helper/Metadata.pm @@ -1,6 +1,7 @@ package SL::DB::Helper::Metadata; use strict; +use SL::X; use Rose::DB::Object::Metadata; use SL::DB::Helper::ConventionManager; @@ -31,4 +32,18 @@ sub make_attr_auto_helpers { 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; diff --git a/SL/DB/Object.pm b/SL/DB/Object.pm index 485fab2ce..3304ed9b6 100755 --- a/SL/DB/Object.pm +++ b/SL/DB/Object.pm @@ -148,7 +148,7 @@ sub save { SL::DB::Object::Hooks::run_hooks($self, 'after_save', $result); 1; - }) || die $self->error; + }) || die $self->db->error; return $result; } @@ -164,7 +164,7 @@ sub delete { SL::DB::Object::Hooks::run_hooks($self, 'after_delete', $result); 1; - }) || die $self->error; + }) || die $self->db->error; return $result; } diff --git a/SL/Form.pm b/SL/Form.pm index c9d5932ea..344f1f0bc 100644 --- a/SL/Form.pm +++ b/SL/Form.pm @@ -344,13 +344,12 @@ sub numtextrows { } 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 { diff --git a/SL/X.pm b/SL/X.pm index 552e5ef29..51021215a 100644 --- a/SL/X.pm +++ b/SL/X.pm @@ -5,6 +5,9 @@ use strict; 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; diff --git a/t/db_helper/with_transaction.t b/t/db_helper/with_transaction.t new file mode 100644 index 000000000..6d49a209b --- /dev/null +++ b/t/db_helper/with_transaction.t @@ -0,0 +1,245 @@ +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'; + -- 2.20.1