From 033cfd772c77e1256868d069bf57f7d69a768908 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tom=C3=A1s=20Cohen=20Arazi?= Date: Wed, 15 Apr 2026 16:56:05 -0300 Subject: [PATCH] Bug 42391: (follow-up) Add NotNull, NotInStorage and UnhandledDBError translations MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Extend the ExceptionTranslator to handle two additional MySQL error patterns: - NOT NULL violations ('Column X cannot be null' and 'X doesn't have a default value') → Koha::Exceptions::Object::NotNull - Not in database errors → Koha::Exceptions::Object::NotInStorage Add Koha::Exceptions::Object::NotNull exception class with a property field identifying the offending column. Add Koha::Exceptions::Object::UnhandledDBError as a catch-all for DBI errors that don't match any known pattern. This closes the gap where unmatched MySQL errors could leak as DBIx::Class::Exception to callers (e.g. templates that no longer catch that type). The exception_action handler now distinguishes three cases: - Koha exception objects (from txn_do re-throws): passed through via 'die $msg' to preserve the original exception type. Without this, txn_do's rollback-and-rethrow would stringify the Koha exception and wrap it in UnhandledDBError, losing the specific type. - DBI errors (prefixed 'DBI Exception:'): translated if possible, otherwise wrapped in UnhandledDBError. - DBIC internal errors (e.g. 'Not in database'): translated if possible, otherwise thrown as DBIx::Class::Exception. Note: NotInStorage translation works for update operations but not for delete-on-detached-Row where DBIC bypasses the Schema's exception_action. Those cases remain DBIx::Class::Exception. Update tests: - OAIServers.t, Z3950Servers.t: assert NotNull with property check - Hold.t: assert NotNull, remove warn suppression hack - Objects.t: update-not-in-storage cases assert NotInStorage - Upload.t: delete-not-in-storage stays DBIx::Class::Exception Signed-off-by: David Nind Signed-off-by: Kyle M Hall Signed-off-by: Tomás Cohen Arazi Signed-off-by: Pedro Amorim --- Koha/Exceptions/Object.pm | 9 ++++ Koha/Schema.pm | 11 ++++- Koha/Schema/Util/ExceptionTranslator.pm | 14 ++++++ t/db_dependent/Koha/Hold.t | 3 +- t/db_dependent/Koha/OAIServers.t | 8 ++-- t/db_dependent/Koha/Objects.t | 6 +-- .../Koha/Schema/Util/ExceptionTranslator.t | 44 ++++++++++++++++--- t/db_dependent/Koha/Z3950Servers.t | 12 ++--- 8 files changed, 85 insertions(+), 22 deletions(-) diff --git a/Koha/Exceptions/Object.pm b/Koha/Exceptions/Object.pm index 85181d9f90..20226cabaa 100644 --- a/Koha/Exceptions/Object.pm +++ b/Koha/Exceptions/Object.pm @@ -69,6 +69,15 @@ use Exception::Class ( isa => 'Koha::Exceptions::Object', description => 'The object is not in storage yet', }, + 'Koha::Exceptions::Object::NotNull' => { + isa => 'Koha::Exceptions::Object', + description => 'A required column value was not provided', + fields => ['property'], + }, + 'Koha::Exceptions::Object::UnhandledDBError' => { + isa => 'Koha::Exceptions::Object', + description => 'An unhandled database error occurred', + }, ); sub full_message { diff --git a/Koha/Schema.pm b/Koha/Schema.pm index 677ae29a7a..0041f3062f 100644 --- a/Koha/Schema.pm +++ b/Koha/Schema.pm @@ -38,10 +38,19 @@ sub connection { sub { my ($msg) = @_; + # Don't re-wrap Koha exceptions that DBIC is re-throwing (e.g. from txn_do) + die $msg if ref($msg); + # Attempt translation — throws a Koha exception on match + # Handles both DBI errors and DBIC internal errors (e.g. "Not in database") Koha::Schema::Util::ExceptionTranslator->translate_exception($msg); - # No match — let DBIC throw its default DBIx::Class::Exception + # No match — wrap DBI errors so DBIx::Class::Exception never leaks + if ( $msg =~ /^DBI Exception:/ ) { + Koha::Exceptions::Object::UnhandledDBError->throw( error => $msg ); + } + + # Non-DBI DBIC internal errors: let DBIC handle normally DBIx::Class::Exception->throw($msg); } ); diff --git a/Koha/Schema/Util/ExceptionTranslator.pm b/Koha/Schema/Util/ExceptionTranslator.pm index 1881cd0615..4593a2ddff 100644 --- a/Koha/Schema/Util/ExceptionTranslator.pm +++ b/Koha/Schema/Util/ExceptionTranslator.pm @@ -120,6 +120,20 @@ sub translate_exception { ); } + # NOT NULL violations + elsif ($msg =~ /Column '(?\w+)' cannot be null/ + || $msg =~ /'(?\w+)' doesn't have a default value/ ) + { + Koha::Exceptions::Object::NotNull->throw( + property => $+{property}, + ); + } + + # Object not in storage (fires for update, but not for delete on detached Rows) + elsif ( $msg =~ /Not in database/ ) { + Koha::Exceptions::Object::NotInStorage->throw(); + } + # No match — return without throwing so DBIC's default handling proceeds return; } diff --git a/t/db_dependent/Koha/Hold.t b/t/db_dependent/Koha/Hold.t index 8504786346..a1ba2bf1b9 100755 --- a/t/db_dependent/Koha/Hold.t +++ b/t/db_dependent/Koha/Hold.t @@ -86,9 +86,8 @@ subtest 'biblio() tests' => sub { } ); - local $SIG{__WARN__} = sub { warn $_[0] unless $_[0] =~ /cannot be null/ }; throws_ok { $hold->biblionumber(undef)->store; } - 'DBIx::Class::Exception', + 'Koha::Exceptions::Object::NotNull', 'reserves.biblionumber cannot be null, exception thrown'; $schema->storage->txn_rollback; diff --git a/t/db_dependent/Koha/OAIServers.t b/t/db_dependent/Koha/OAIServers.t index fe9d03cc4c..e7f5618bc8 100755 --- a/t/db_dependent/Koha/OAIServers.t +++ b/t/db_dependent/Koha/OAIServers.t @@ -81,12 +81,12 @@ subtest 'Check NOT NULL without default values' => sub { } ); - throws_ok { $server->store } 'DBIx::Class::Exception', 'Exception on empty endpoint'; - like( $@->{msg}, qr/'endpoint' doesn't have a default value/, 'Verified that DBIx blamed endpoint' ); + throws_ok { $server->store } 'Koha::Exceptions::Object::NotNull', 'Exception on empty endpoint'; + is( $@->property, 'endpoint', 'Exception identifies the endpoint column' ); $server->endpoint('endpoint_added'); - throws_ok { $server->store } 'DBIx::Class::Exception', 'Exception on empty servername'; - like( $@->{msg}, qr/'servername' doesn't have a default value/, 'Verified that DBIx blamed servername' ); + throws_ok { $server->store } 'Koha::Exceptions::Object::NotNull', 'Exception on empty servername'; + is( $@->property, 'servername', 'Exception identifies the servername column' ); $server->servername('servername_added'); diff --git a/t/db_dependent/Koha/Objects.t b/t/db_dependent/Koha/Objects.t index 7e77c8a577..53bd4a6462 100755 --- a/t/db_dependent/Koha/Objects.t +++ b/t/db_dependent/Koha/Objects.t @@ -595,8 +595,7 @@ subtest 'Return same values as DBIx::Class' => sub { defined $e_us && defined $e_them, 'Delete an object that is not in storage should raise an exception' ); - is( ref($e_us), 'DBIx::Class::Exception' ) - ; # FIXME This needs adjustment, we want to throw a Koha::Exception + is( ref($e_us), 'DBIx::Class::Exception' ); }; @@ -725,8 +724,7 @@ subtest 'Return same values as DBIx::Class' => sub { defined $e_us && defined $e_them, 'Delete a patron that is not in storage should raise an exception' ); - is( ref($e_us), 'DBIx::Class::Exception' ) - ; # FIXME This needs adjustment, we want to throw a Koha::Exception + is( ref($e_us), 'DBIx::Class::Exception' ); # CASE 3 - Delete a patron that cannot be deleted (as a checkout) $patron = Koha::Patron->new($patron_data)->store; diff --git a/t/db_dependent/Koha/Schema/Util/ExceptionTranslator.t b/t/db_dependent/Koha/Schema/Util/ExceptionTranslator.t index c6b9053f1c..99b587eedf 100755 --- a/t/db_dependent/Koha/Schema/Util/ExceptionTranslator.t +++ b/t/db_dependent/Koha/Schema/Util/ExceptionTranslator.t @@ -19,7 +19,7 @@ use Modern::Perl; use Test::NoWarnings; -use Test::More tests => 9; +use Test::More tests => 11; use Test::Exception; use Koha::Database; @@ -130,22 +130,56 @@ subtest 'fk_constraint_deletion_translation' => sub { $schema->storage->txn_rollback; }; -subtest 'exception_action integration' => sub { +subtest 'not_null_translation' => sub { plan tests => 2; $schema->storage->txn_begin; + throws_ok { + Koha::Schema::Util::ExceptionTranslator->translate_exception("Column 'host' cannot be null"); + } + 'Koha::Exceptions::Object::NotNull', 'NOT NULL violation is properly translated'; + + is( $@->property, 'host', 'property is correctly extracted' ); + + $schema->storage->txn_rollback; +}; + +subtest 'not_in_database_translation' => sub { + plan tests => 1; + + $schema->storage->txn_begin; + + throws_ok { + Koha::Schema::Util::ExceptionTranslator->translate_exception("Not in database"); + } + 'Koha::Exceptions::Object::NotInStorage', 'Not in database is properly translated'; + + $schema->storage->txn_rollback; +}; + +subtest 'exception_action integration' => sub { + plan tests => 3; + + $schema->storage->txn_begin; + # Test that the schema's exception_action translates known errors throws_ok { $schema->throw_exception("Duplicate entry 'test' for key 'primary'"); } 'Koha::Exceptions::Object::DuplicateID', 'exception_action translates known errors'; - # Test that unknown errors still throw as DBIx::Class::Exception + # Test that unknown DBI errors throw as UnhandledDBError throws_ok { - $schema->throw_exception("Some unknown DBIC error"); + $schema->throw_exception("DBI Exception: DBD::mysql::st execute failed: Some unknown DB error"); } - 'DBIx::Class::Exception', 'exception_action preserves unknown errors as DBIx::Class::Exception'; + 'Koha::Exceptions::Object::UnhandledDBError', 'exception_action wraps unknown DBI errors as UnhandledDBError'; + + # Test that non-DBI DBIC internal errors remain DBIx::Class::Exception + throws_ok { + $schema->throw_exception("Some internal DBIC error"); + } + 'DBIx::Class::Exception', 'exception_action preserves DBIC internal errors'; $schema->storage->txn_rollback; }; diff --git a/t/db_dependent/Koha/Z3950Servers.t b/t/db_dependent/Koha/Z3950Servers.t index 66f1ef8a88..8cdec2eb05 100755 --- a/t/db_dependent/Koha/Z3950Servers.t +++ b/t/db_dependent/Koha/Z3950Servers.t @@ -89,16 +89,16 @@ subtest 'Host, syntax and encoding are NOT NULL now (BZ 30571)' => sub { } ); - throws_ok { $server->store } 'DBIx::Class::Exception', 'Exception on empty host'; - like( $@->{msg}, qr/'host' doesn't have a default value/, 'Verified that DBIx blamed host' ); + throws_ok { $server->store } 'Koha::Exceptions::Object::NotNull', 'Exception on empty host'; + is( $@->property, 'host', 'Exception identifies the host column' ); $server->host('host_added.nl'); - throws_ok { $server->store } 'DBIx::Class::Exception', 'Exception on empty syntax'; - like( $@->{msg}, qr/'syntax' doesn't have a default value/, 'Verified that DBIx blamed syntax' ); + throws_ok { $server->store } 'Koha::Exceptions::Object::NotNull', 'Exception on empty syntax'; + is( $@->property, 'syntax', 'Exception identifies the syntax column' ); $server->syntax('USMARC'); - throws_ok { $server->store } 'DBIx::Class::Exception', 'Exception on empty encoding'; - like( $@->{msg}, qr/'encoding' doesn't have a default value/, 'Verified that DBIx blamed encoding' ); + throws_ok { $server->store } 'Koha::Exceptions::Object::NotNull', 'Exception on empty encoding'; + is( $@->property, 'encoding', 'Exception identifies the encoding column' ); $server->encoding('utf8'); lives_ok { $server->store } 'No exceptions anymore';