Bug 42391: (follow-up) Add NotNull, NotInStorage and UnhandledDBError translations

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 <david@davidnind.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>

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 {

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);
         }
     );

Koha/Schema/Util/ExceptionTranslator.pm

@@ -120,6 +120,20 @@ sub translate_exception {
         );
     }
 
+    # NOT NULL violations
+    elsif ($msg =~ /Column '(?<property>\w+)' cannot be null/
+        || $msg =~ /'(?<property>\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;
 }

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;

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');
 

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;

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;
 };

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';