From 4189d42ee63c212deff8702962154625e48a5a98 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tom=C3=A1s=20Cohen=20Arazi?= Date: Tue, 21 Jul 2026 11:02:29 -0300 Subject: [PATCH] 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 --- Koha/Object.pm | 8 ++++- .../Circulation/MarkIssueReturned.t | 2 +- t/db_dependent/Koha/Account.t | 2 +- t/db_dependent/Koha/ApiKey.t | 2 +- t/db_dependent/Koha/Checkouts/ReturnClaim.t | 2 +- t/db_dependent/Koha/Object.t | 31 ++++++++++++------- t/db_dependent/Koha/Schema/ExceptionMapper.t | 12 ++++--- t/db_dependent/Koha/Suggestions.t | 8 ++--- t/db_dependent/Suggestions.t | 2 +- 9 files changed, 44 insertions(+), 25 deletions(-) mode change 100644 => 100755 t/db_dependent/Koha/Schema/ExceptionMapper.t diff --git a/Koha/Object.pm b/Koha/Object.pm index 3b30f63af7..cadd00e786 100644 --- a/Koha/Object.pm +++ b/Koha/Object.pm @@ -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 diff --git a/t/db_dependent/Circulation/MarkIssueReturned.t b/t/db_dependent/Circulation/MarkIssueReturned.t index 6c8e30ec26..615961ff01 100755 --- a/t/db_dependent/Circulation/MarkIssueReturned.t +++ b/t/db_dependent/Circulation/MarkIssueReturned.t @@ -175,7 +175,7 @@ subtest 'Manually pass a return date' => sub { 'Koha::Exceptions::Object::BadValue', 'An exception is thrown on bad date'; }, - qr{Incorrect datetime value: 'bad_date' for column .*returndate} + qr/Invalid value/ ); $schema->storage->txn_rollback; diff --git a/t/db_dependent/Koha/Account.t b/t/db_dependent/Koha/Account.t index 242a06601e..65b7b4c745 100755 --- a/t/db_dependent/Koha/Account.t +++ b/t/db_dependent/Koha/Account.t @@ -383,7 +383,7 @@ subtest 'add_debit() tests' => sub { 'Koha::Exceptions::Account::UnrecognisedType', 'Expected validation exception thrown (type)'; }, - qr{a foreign key constraint fails} + qr/Broken FK constraint/ ); throws_ok { diff --git a/t/db_dependent/Koha/ApiKey.t b/t/db_dependent/Koha/ApiKey.t index f953d7a49f..d7df41d2d5 100755 --- a/t/db_dependent/Koha/ApiKey.t +++ b/t/db_dependent/Koha/ApiKey.t @@ -112,7 +112,7 @@ subtest 'store() tests' => sub { 'Koha::Exceptions::Object::FKConstraint', '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( $@->broken_fk, 'patron_id', 'Exception field is correct' ); diff --git a/t/db_dependent/Koha/Checkouts/ReturnClaim.t b/t/db_dependent/Koha/Checkouts/ReturnClaim.t index 2b5fbe0d43..dde32fe15a 100755 --- a/t/db_dependent/Koha/Checkouts/ReturnClaim.t +++ b/t/db_dependent/Koha/Checkouts/ReturnClaim.t @@ -165,7 +165,7 @@ subtest "resolve() tests" => sub { 'Koha::Exceptions::Object::FKConstraint', "Exception thrown on invalid resolver"; }, - qr{a foreign key constraint fails} + qr/Broken FK constraint/ ); my $today = dt_from_string; diff --git a/t/db_dependent/Koha/Object.t b/t/db_dependent/Koha/Object.t index 8c5a1720e2..94f679890d 100755 --- a/t/db_dependent/Koha/Object.t +++ b/t/db_dependent/Koha/Object.t @@ -946,7 +946,7 @@ subtest "Test update method" => sub { subtest 'store() tests' => sub { - plan tests => 16; + plan tests => 18; # Using Koha::Library::Groups to test Koha::Object>-store # Simple object with foreign keys and unique key @@ -966,9 +966,12 @@ subtest 'store() tests' => sub { ); my $dbh = $schema->storage->dbh; - throws_ok { $library_group->store } - 'Koha::Exceptions::Object::FKConstraint', - 'Exception is thrown correctly'; + warning_like { + throws_ok { $library_group->store } + 'Koha::Exceptions::Object::FKConstraint', + 'Exception is thrown correctly'; + } + qr/Broken FK constraint/, 'DB exception logged for FK constraint'; is( $@->message, "Broken FK constraint", @@ -989,9 +992,12 @@ subtest 'store() tests' => sub { } ); - throws_ok { $new_library_group->store } - 'Koha::Exceptions::Object::DuplicateID', - 'Exception is thrown correctly'; + warning_like { + throws_ok { $new_library_group->store } + 'Koha::Exceptions::Object::DuplicateID', + 'Exception is thrown correctly'; + } + qr/Duplicate ID/, 'DB exception logged for duplicate ID'; is( $@->message, @@ -1059,14 +1065,17 @@ subtest 'store() tests' => sub { subtest 'Bad value tests' => sub { - plan tests => 3; + plan tests => 4; my $patron = $builder->build_object( { class => 'Koha::Patrons' } ); - throws_ok { - $patron->lastseen('wrong_value')->store; + warning_like { + 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' ) ; # The table name is not always displayed, it depends on the DBMS version diff --git a/t/db_dependent/Koha/Schema/ExceptionMapper.t b/t/db_dependent/Koha/Schema/ExceptionMapper.t old mode 100644 new mode 100755 index 4c7b7615bb..eddade0437 --- a/t/db_dependent/Koha/Schema/ExceptionMapper.t +++ b/t/db_dependent/Koha/Schema/ExceptionMapper.t @@ -21,6 +21,7 @@ use Modern::Perl; use Test::NoWarnings; use Test::More tests => 11; use Test::Exception; +use Test::Warn; use Koha::Database; use Koha::Schema::ExceptionMapper; @@ -185,7 +186,7 @@ subtest 'exception_action integration' => sub { }; subtest 'Object::store integration' => sub { - plan tests => 1; + plan tests => 2; $schema->storage->txn_begin; @@ -195,10 +196,13 @@ subtest 'Object::store integration' => sub { # Force a duplicate cardnumber $patron2->cardnumber( $patron->cardnumber ); - throws_ok { - $patron2->store(); + warning_like { + 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; }; diff --git a/t/db_dependent/Koha/Suggestions.t b/t/db_dependent/Koha/Suggestions.t index 76a8e12212..1def7414ad 100755 --- a/t/db_dependent/Koha/Suggestions.t +++ b/t/db_dependent/Koha/Suggestions.t @@ -202,7 +202,7 @@ subtest 'constraints' => sub { 'Koha::Exceptions::Object::FKConstraint', '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" } ); $suggestion->managedby( $manager->borrowernumber )->store; @@ -222,7 +222,7 @@ subtest 'constraints' => sub { 'Koha::Exceptions::Object::FKConstraint', '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" } ); $suggestion->acceptedby( $acceptor->borrowernumber )->store; @@ -242,7 +242,7 @@ subtest 'constraints' => sub { 'Koha::Exceptions::Object::FKConstraint', '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" } ); $suggestion->rejectedby( $rejecter->borrowernumber )->store; @@ -260,7 +260,7 @@ subtest 'constraints' => sub { 'Koha::Exceptions::Object::FKConstraint', '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" } ); $suggestion->budgetid( $fund->id )->store; diff --git a/t/db_dependent/Suggestions.t b/t/db_dependent/Suggestions.t index 50e0496c94..207cc89d8e 100755 --- a/t/db_dependent/Suggestions.t +++ b/t/db_dependent/Suggestions.t @@ -346,7 +346,7 @@ warning_like( sub { ModSuggestion($mod_suggestion4); }, - qr{Incorrect date value: 'invalid date!' for column .*manageddate} + qr/Invalid value/ ); $messages = C4::Letters::GetQueuedMessages( { borrowernumber => $borrowernumber2 } );