Bug 42391: (follow-up) Restore DB exception warnings in Object::store
Bug 42391 removed the warn from Koha::Object::store when it moved
exception translation to the exception_action hook. This lost useful
debugging information from the Plack error logs.
This patch restores the warning by adding a try/catch in store() that
stringifies and warns the exception before rethrowing. The warning now
contains the translated Koha exception message (e.g. 'Broken FK
constraint') rather than the raw DBI error, since translation happens
before the exception reaches store().
Note: delete() does not warn. Ideally the logging would live in the
ExceptionMapper itself (via an injected logger), covering all code paths
uniformly. That is a larger refactor tracked separately.
Tests that assert on these warnings are updated to match the new
message format.
Test plan:
1. Apply patch
2. Run:
$ ktd --shell
k$ prove t/db_dependent/Koha/Object.t \
t/db_dependent/Koha/Objects.t \
t/db_dependent/Koha/Schema/ExceptionMapper.t \
t/db_dependent/Koha/Item.t \
t/db_dependent/Koha/Suggestions.t \
t/db_dependent/Koha/Account.t \
t/db_dependent/Koha/ApiKey.t \
t/db_dependent/Koha/Checkouts/ReturnClaim.t \
t/db_dependent/Circulation/MarkIssueReturned.t \
t/db_dependent/Suggestions.t \
t/db_dependent/Biblio.t
=> SUCCESS: Tests pass!
3. Sign off :-D
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
This commit is contained in:
parent
880f33a4bb
commit
4189d42ee6
|
|
@ -170,7 +170,13 @@ sub store {
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
return $self->_result()->update_or_insert() ? $self : undef;
|
try {
|
||||||
|
return $self->_result()->update_or_insert() ? $self : undef;
|
||||||
|
} catch {
|
||||||
|
warn "$_";
|
||||||
|
$_->rethrow if ref($_) && $_->can('rethrow');
|
||||||
|
die $_;
|
||||||
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
=head3 discard_changes
|
=head3 discard_changes
|
||||||
|
|
|
||||||
|
|
@ -175,7 +175,7 @@ subtest 'Manually pass a return date' => sub {
|
||||||
'Koha::Exceptions::Object::BadValue',
|
'Koha::Exceptions::Object::BadValue',
|
||||||
'An exception is thrown on bad date';
|
'An exception is thrown on bad date';
|
||||||
},
|
},
|
||||||
qr{Incorrect datetime value: 'bad_date' for column .*returndate}
|
qr/Invalid value/
|
||||||
);
|
);
|
||||||
|
|
||||||
$schema->storage->txn_rollback;
|
$schema->storage->txn_rollback;
|
||||||
|
|
|
||||||
|
|
@ -383,7 +383,7 @@ subtest 'add_debit() tests' => sub {
|
||||||
'Koha::Exceptions::Account::UnrecognisedType',
|
'Koha::Exceptions::Account::UnrecognisedType',
|
||||||
'Expected validation exception thrown (type)';
|
'Expected validation exception thrown (type)';
|
||||||
},
|
},
|
||||||
qr{a foreign key constraint fails}
|
qr/Broken FK constraint/
|
||||||
);
|
);
|
||||||
|
|
||||||
throws_ok {
|
throws_ok {
|
||||||
|
|
|
||||||
|
|
@ -112,7 +112,7 @@ subtest 'store() tests' => sub {
|
||||||
'Koha::Exceptions::Object::FKConstraint',
|
'Koha::Exceptions::Object::FKConstraint',
|
||||||
'Invalid patron ID raises exception';
|
'Invalid patron ID raises exception';
|
||||||
},
|
},
|
||||||
qr{a foreign key constraint fails}
|
qr/Broken FK constraint/
|
||||||
);
|
);
|
||||||
is( $@->message, 'Broken FK constraint', 'Exception message is correct' );
|
is( $@->message, 'Broken FK constraint', 'Exception message is correct' );
|
||||||
is( $@->broken_fk, 'patron_id', 'Exception field is correct' );
|
is( $@->broken_fk, 'patron_id', 'Exception field is correct' );
|
||||||
|
|
|
||||||
|
|
@ -165,7 +165,7 @@ subtest "resolve() tests" => sub {
|
||||||
'Koha::Exceptions::Object::FKConstraint',
|
'Koha::Exceptions::Object::FKConstraint',
|
||||||
"Exception thrown on invalid resolver";
|
"Exception thrown on invalid resolver";
|
||||||
},
|
},
|
||||||
qr{a foreign key constraint fails}
|
qr/Broken FK constraint/
|
||||||
);
|
);
|
||||||
|
|
||||||
my $today = dt_from_string;
|
my $today = dt_from_string;
|
||||||
|
|
|
||||||
|
|
@ -946,7 +946,7 @@ subtest "Test update method" => sub {
|
||||||
|
|
||||||
subtest 'store() tests' => sub {
|
subtest 'store() tests' => sub {
|
||||||
|
|
||||||
plan tests => 16;
|
plan tests => 18;
|
||||||
|
|
||||||
# Using Koha::Library::Groups to test Koha::Object>-store
|
# Using Koha::Library::Groups to test Koha::Object>-store
|
||||||
# Simple object with foreign keys and unique key
|
# Simple object with foreign keys and unique key
|
||||||
|
|
@ -966,9 +966,12 @@ subtest 'store() tests' => sub {
|
||||||
);
|
);
|
||||||
|
|
||||||
my $dbh = $schema->storage->dbh;
|
my $dbh = $schema->storage->dbh;
|
||||||
throws_ok { $library_group->store }
|
warning_like {
|
||||||
'Koha::Exceptions::Object::FKConstraint',
|
throws_ok { $library_group->store }
|
||||||
'Exception is thrown correctly';
|
'Koha::Exceptions::Object::FKConstraint',
|
||||||
|
'Exception is thrown correctly';
|
||||||
|
}
|
||||||
|
qr/Broken FK constraint/, 'DB exception logged for FK constraint';
|
||||||
is(
|
is(
|
||||||
$@->message,
|
$@->message,
|
||||||
"Broken FK constraint",
|
"Broken FK constraint",
|
||||||
|
|
@ -989,9 +992,12 @@ subtest 'store() tests' => sub {
|
||||||
}
|
}
|
||||||
);
|
);
|
||||||
|
|
||||||
throws_ok { $new_library_group->store }
|
warning_like {
|
||||||
'Koha::Exceptions::Object::DuplicateID',
|
throws_ok { $new_library_group->store }
|
||||||
'Exception is thrown correctly';
|
'Koha::Exceptions::Object::DuplicateID',
|
||||||
|
'Exception is thrown correctly';
|
||||||
|
}
|
||||||
|
qr/Duplicate ID/, 'DB exception logged for duplicate ID';
|
||||||
|
|
||||||
is(
|
is(
|
||||||
$@->message,
|
$@->message,
|
||||||
|
|
@ -1059,14 +1065,17 @@ subtest 'store() tests' => sub {
|
||||||
|
|
||||||
subtest 'Bad value tests' => sub {
|
subtest 'Bad value tests' => sub {
|
||||||
|
|
||||||
plan tests => 3;
|
plan tests => 4;
|
||||||
|
|
||||||
my $patron = $builder->build_object( { class => 'Koha::Patrons' } );
|
my $patron = $builder->build_object( { class => 'Koha::Patrons' } );
|
||||||
|
|
||||||
throws_ok {
|
warning_like {
|
||||||
$patron->lastseen('wrong_value')->store;
|
throws_ok {
|
||||||
|
$patron->lastseen('wrong_value')->store;
|
||||||
|
}
|
||||||
|
'Koha::Exceptions::Object::BadValue', 'Exception thrown correctly';
|
||||||
}
|
}
|
||||||
'Koha::Exceptions::Object::BadValue', 'Exception thrown correctly';
|
qr/Invalid value/, 'DB exception logged for bad value';
|
||||||
|
|
||||||
like( $@->property, qr/(borrowers\.)?lastseen/, 'Column should be the expected one' )
|
like( $@->property, qr/(borrowers\.)?lastseen/, 'Column should be the expected one' )
|
||||||
; # The table name is not always displayed, it depends on the DBMS version
|
; # The table name is not always displayed, it depends on the DBMS version
|
||||||
|
|
|
||||||
|
|
@ -21,6 +21,7 @@ use Modern::Perl;
|
||||||
use Test::NoWarnings;
|
use Test::NoWarnings;
|
||||||
use Test::More tests => 11;
|
use Test::More tests => 11;
|
||||||
use Test::Exception;
|
use Test::Exception;
|
||||||
|
use Test::Warn;
|
||||||
|
|
||||||
use Koha::Database;
|
use Koha::Database;
|
||||||
use Koha::Schema::ExceptionMapper;
|
use Koha::Schema::ExceptionMapper;
|
||||||
|
|
@ -185,7 +186,7 @@ subtest 'exception_action integration' => sub {
|
||||||
};
|
};
|
||||||
|
|
||||||
subtest 'Object::store integration' => sub {
|
subtest 'Object::store integration' => sub {
|
||||||
plan tests => 1;
|
plan tests => 2;
|
||||||
|
|
||||||
$schema->storage->txn_begin;
|
$schema->storage->txn_begin;
|
||||||
|
|
||||||
|
|
@ -195,10 +196,13 @@ subtest 'Object::store integration' => sub {
|
||||||
# Force a duplicate cardnumber
|
# Force a duplicate cardnumber
|
||||||
$patron2->cardnumber( $patron->cardnumber );
|
$patron2->cardnumber( $patron->cardnumber );
|
||||||
|
|
||||||
throws_ok {
|
warning_like {
|
||||||
$patron2->store();
|
throws_ok {
|
||||||
|
$patron2->store();
|
||||||
|
}
|
||||||
|
'Koha::Exceptions::Object::DuplicateID', 'Object::store gets automatic exception translation';
|
||||||
}
|
}
|
||||||
'Koha::Exceptions::Object::DuplicateID', 'Object::store gets automatic exception translation';
|
qr/Duplicate ID/, 'DB exception logged on store failure';
|
||||||
|
|
||||||
$schema->storage->txn_rollback;
|
$schema->storage->txn_rollback;
|
||||||
};
|
};
|
||||||
|
|
|
||||||
|
|
@ -202,7 +202,7 @@ subtest 'constraints' => sub {
|
||||||
'Koha::Exceptions::Object::FKConstraint',
|
'Koha::Exceptions::Object::FKConstraint',
|
||||||
'store raises an exception on invalid managerid';
|
'store raises an exception on invalid managerid';
|
||||||
},
|
},
|
||||||
qr{a foreign key constraint fails}
|
qr/Broken FK constraint/
|
||||||
);
|
);
|
||||||
my $manager = $builder->build_object( { class => "Koha::Patrons" } );
|
my $manager = $builder->build_object( { class => "Koha::Patrons" } );
|
||||||
$suggestion->managedby( $manager->borrowernumber )->store;
|
$suggestion->managedby( $manager->borrowernumber )->store;
|
||||||
|
|
@ -222,7 +222,7 @@ subtest 'constraints' => sub {
|
||||||
'Koha::Exceptions::Object::FKConstraint',
|
'Koha::Exceptions::Object::FKConstraint',
|
||||||
'store raises an exception on invalid acceptedby id';
|
'store raises an exception on invalid acceptedby id';
|
||||||
},
|
},
|
||||||
qr{a foreign key constraint fails}
|
qr/Broken FK constraint/
|
||||||
);
|
);
|
||||||
my $acceptor = $builder->build_object( { class => "Koha::Patrons" } );
|
my $acceptor = $builder->build_object( { class => "Koha::Patrons" } );
|
||||||
$suggestion->acceptedby( $acceptor->borrowernumber )->store;
|
$suggestion->acceptedby( $acceptor->borrowernumber )->store;
|
||||||
|
|
@ -242,7 +242,7 @@ subtest 'constraints' => sub {
|
||||||
'Koha::Exceptions::Object::FKConstraint',
|
'Koha::Exceptions::Object::FKConstraint',
|
||||||
'store raises an exception on invalid rejectedby id';
|
'store raises an exception on invalid rejectedby id';
|
||||||
},
|
},
|
||||||
qr{a foreign key constraint fails}
|
qr/Broken FK constraint/
|
||||||
);
|
);
|
||||||
my $rejecter = $builder->build_object( { class => "Koha::Patrons" } );
|
my $rejecter = $builder->build_object( { class => "Koha::Patrons" } );
|
||||||
$suggestion->rejectedby( $rejecter->borrowernumber )->store;
|
$suggestion->rejectedby( $rejecter->borrowernumber )->store;
|
||||||
|
|
@ -260,7 +260,7 @@ subtest 'constraints' => sub {
|
||||||
'Koha::Exceptions::Object::FKConstraint',
|
'Koha::Exceptions::Object::FKConstraint',
|
||||||
'store raises an exception on invalid budgetid';
|
'store raises an exception on invalid budgetid';
|
||||||
},
|
},
|
||||||
qr{a foreign key constraint fails}
|
qr/Broken FK constraint/
|
||||||
);
|
);
|
||||||
my $fund = $builder->build_object( { class => "Koha::Acquisition::Funds" } );
|
my $fund = $builder->build_object( { class => "Koha::Acquisition::Funds" } );
|
||||||
$suggestion->budgetid( $fund->id )->store;
|
$suggestion->budgetid( $fund->id )->store;
|
||||||
|
|
|
||||||
|
|
@ -346,7 +346,7 @@ warning_like(
|
||||||
sub {
|
sub {
|
||||||
ModSuggestion($mod_suggestion4);
|
ModSuggestion($mod_suggestion4);
|
||||||
},
|
},
|
||||||
qr{Incorrect date value: 'invalid date!' for column .*manageddate}
|
qr/Invalid value/
|
||||||
);
|
);
|
||||||
$messages = C4::Letters::GetQueuedMessages( { borrowernumber => $borrowernumber2 } );
|
$messages = C4::Letters::GetQueuedMessages( { borrowernumber => $borrowernumber2 } );
|
||||||
|
|
||||||
|
|
|
||||||
Loading…
Reference in New Issue