From e496751eed40147f23f67e3db341689e878c17dc Mon Sep 17 00:00:00 2001 From: Martin Renvoize Date: Sun, 21 Sep 2025 15:34:34 +0100 Subject: [PATCH] Bug 19871: Move FK constraint deletion handling to centralized exception translation Move the FK constraint deletion exception handling from Koha::Object::delete to the centralized ExceptionTranslator utility. This consolidates all DBIx::Class exception translation logic in one place and simplifies the Object delete method. Changes: - Add FK constraint deletion pattern matching to ExceptionTranslator - Remove manual exception handling from Koha::Object::delete - Add test coverage for FK constraint deletion translation - Simplify Object delete method to use centralized translation The centralized system now handles both FK constraint scenarios: - Child row constraint failures (add/update operations) - Parent row constraint failures (delete operations) --- Koha/Object.pm | 20 +--- Koha/Schema/Util/ExceptionTranslator.pm | 31 ++++-- .../Koha/Schema/Util/ExceptionTranslator.t | 95 +++++++++++-------- 3 files changed, 88 insertions(+), 58 deletions(-) diff --git a/Koha/Object.pm b/Koha/Object.pm index 3e0bc01e924..b35658411f2 100644 --- a/Koha/Object.pm +++ b/Koha/Object.pm @@ -173,6 +173,7 @@ sub store { try { return $self->_result()->update_or_insert() ? $self : undef; } catch { + # Use centralized exception translation warn $_->{msg} if ref($_) eq 'DBIx::Class::Exception'; @@ -192,7 +193,7 @@ sub store { } # Delegate to centralized exception translation - $self->_result->result_source->schema->translate_exception($_, $columns_info); + $self->_result->result_source->schema->translate_exception( $_, $columns_info ); } } @@ -238,20 +239,9 @@ sub delete { try { $deleted = $self->_result()->delete; } catch { - if ( ref($_) eq 'DBIx::Class::Exception' ) { - if ( $_->{msg} =~ - /Cannot delete or update a parent row\: a foreign key constraint fails \(\`(?.*?)\`\.\`(?.*?)\`, CONSTRAINT \`(?.*?)\` FOREIGN KEY \(\`(?.*?)\`\) REFERENCES \`.*\` \(\`(?.*?)\`\)/ - ) - { - Koha::Exceptions::Object::FKConstraintDeletion->throw( - column => $+{column}, - constraint => $+{constraint}, - fk => $+{fk}, - table => $+{table}, - ); - } - $_->rethrow(); - } + + # Delegate to centralized exception translation + $self->_result->result_source->schema->translate_exception($_); }; if ( ref $deleted ) { diff --git a/Koha/Schema/Util/ExceptionTranslator.pm b/Koha/Schema/Util/ExceptionTranslator.pm index 929aced4f66..22586f7c76a 100644 --- a/Koha/Schema/Util/ExceptionTranslator.pm +++ b/Koha/Schema/Util/ExceptionTranslator.pm @@ -89,6 +89,7 @@ sub translate_exception { # Foreign key constraint failures if ( $msg =~ /Cannot add or update a child row: a foreign key constraint fails/ ) { + # FIXME: MySQL error, if we support more DB engines we should implement this for each if ( $msg =~ /FOREIGN KEY \(`(?.*?)`\)/ ) { Koha::Exceptions::Object::FKConstraint->throw( @@ -97,6 +98,20 @@ sub translate_exception { ); } } + + # Foreign key constraint deletion failures (parent row deletion blocked) + elsif ( $msg =~ + /Cannot delete or update a parent row\: a foreign key constraint fails \(\`(?.*?)\`\.\`(?
.*?)\`, CONSTRAINT \`(?.*?)\` FOREIGN KEY \(\`(?.*?)\`\) REFERENCES \`.*\` \(\`(?.*?)\`\)/ + ) + { + Koha::Exceptions::Object::FKConstraintDeletion->throw( + column => $+{column}, + constraint => $+{constraint}, + fk => $+{fk}, + table => $+{table}, + ); + } + # Duplicate key violations elsif ( $msg =~ /Duplicate entry '(.*?)' for key '(?.*?)'/ ) { Koha::Exceptions::Object::DuplicateID->throw( @@ -104,8 +119,10 @@ sub translate_exception { duplicate_id => $+{key} ); } + # Invalid data type values elsif ( $msg =~ /Incorrect (?\w+) value: '(?.*)' for column \W?(?\S+)/ ) { + # The optional \W in the regex might be a quote or backtick my $type = $+{type}; my $value = $+{value}; @@ -116,12 +133,14 @@ sub translate_exception { type => $type, value => $value, property => $property =~ /(\w+\.\w+)$/ - ? $1 - : $property, # results in table.column without quotes or backticks + ? $1 + : $property, # results in table.column without quotes or backticks ); } + # Data truncation for enum columns elsif ( $msg =~ /Data truncated for column \W?(?\w+)/ ) { + # The optional \W in the regex might be a quote or backtick my $property = $+{property}; @@ -132,9 +151,9 @@ sub translate_exception { Koha::Exceptions::Object::BadValue->throw( type => 'enum', property => $property =~ /(\w+\.\w+)$/ - ? $1 - : $property, # results in table.column without quotes or backticks - value => 'Invalid enum value', # We don't have access to the object here + ? $1 + : $property, # results in table.column without quotes or backticks + value => 'Invalid enum value', # We don't have access to the object here ); } } @@ -164,4 +183,4 @@ Koha Development Team =cut -1; \ No newline at end of file +1; diff --git a/t/db_dependent/Koha/Schema/Util/ExceptionTranslator.t b/t/db_dependent/Koha/Schema/Util/ExceptionTranslator.t index c2c8d5e149e..967808ffbc9 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 => 7; +use Test::More tests => 8; use Test::Exception; use Koha::Database; @@ -36,13 +36,15 @@ subtest 'foreign_key_constraint_translation' => sub { $schema->storage->txn_begin; # Create a mock DBIx::Class::Exception for FK constraint - my $exception = bless { - msg => "Cannot add or update a child row: a foreign key constraint fails (`koha`.`items`, CONSTRAINT `items_ibfk_1` FOREIGN KEY (`biblionumber`) REFERENCES `biblio` (`biblionumber`))" - }, 'DBIx::Class::Exception'; + my $exception = + bless { msg => + "Cannot add or update a child row: a foreign key constraint fails (`koha`.`items`, CONSTRAINT `items_ibfk_1` FOREIGN KEY (`biblionumber`) REFERENCES `biblio` (`biblionumber`))" + }, 'DBIx::Class::Exception'; throws_ok { Koha::Schema::Util::ExceptionTranslator->translate_exception($exception); - } 'Koha::Exceptions::Object::FKConstraint', 'FK constraint exception is properly translated'; + } + 'Koha::Exceptions::Object::FKConstraint', 'FK constraint exception is properly translated'; $schema->storage->txn_rollback; }; @@ -53,13 +55,13 @@ subtest 'duplicate_key_translation' => sub { $schema->storage->txn_begin; # Create a mock DBIx::Class::Exception for duplicate key - my $exception = bless { - msg => "Duplicate entry 'test\@example.com' for key 'borrowers.email'" - }, 'DBIx::Class::Exception'; + my $exception = bless { msg => "Duplicate entry 'test\@example.com' for key 'borrowers.email'" }, + 'DBIx::Class::Exception'; throws_ok { Koha::Schema::Util::ExceptionTranslator->translate_exception($exception); - } 'Koha::Exceptions::Object::DuplicateID', 'Duplicate key exception is properly translated'; + } + 'Koha::Exceptions::Object::DuplicateID', 'Duplicate key exception is properly translated'; $schema->storage->txn_rollback; }; @@ -70,13 +72,13 @@ subtest 'bad_value_translation' => sub { $schema->storage->txn_begin; # Create a mock DBIx::Class::Exception for bad value - my $exception = bless { - msg => "Incorrect datetime value: '2025-13-45' for column 'date_due' at row 1" - }, 'DBIx::Class::Exception'; + my $exception = bless { msg => "Incorrect datetime value: '2025-13-45' for column 'date_due' at row 1" }, + 'DBIx::Class::Exception'; throws_ok { Koha::Schema::Util::ExceptionTranslator->translate_exception($exception); - } 'Koha::Exceptions::Object::BadValue', 'Bad value exception is properly translated'; + } + 'Koha::Exceptions::Object::BadValue', 'Bad value exception is properly translated'; $schema->storage->txn_rollback; }; @@ -87,17 +89,14 @@ subtest 'enum_truncation_translation' => sub { $schema->storage->txn_begin; # Create a mock DBIx::Class::Exception for enum truncation - my $exception = bless { - msg => "Data truncated for column 'status' at row 1" - }, 'DBIx::Class::Exception'; + my $exception = bless { msg => "Data truncated for column 'status' at row 1" }, 'DBIx::Class::Exception'; - my $columns_info = { - status => { data_type => 'enum' } - }; + my $columns_info = { status => { data_type => 'enum' } }; throws_ok { - Koha::Schema::Util::ExceptionTranslator->translate_exception($exception, $columns_info); - } 'Koha::Exceptions::Object::BadValue', 'Enum truncation exception is properly translated'; + Koha::Schema::Util::ExceptionTranslator->translate_exception( $exception, $columns_info ); + } + 'Koha::Exceptions::Object::BadValue', 'Enum truncation exception is properly translated'; $schema->storage->txn_rollback; }; @@ -108,20 +107,39 @@ subtest 'non_dbix_exception_passthrough' => sub { $schema->storage->txn_begin; # Create a regular exception (not DBIx::Class::Exception) - my $exception = bless { - msg => "Some other error" - }, 'Some::Other::Exception'; + my $exception = bless { msg => "Some other error" }, 'Some::Other::Exception'; # Mock the rethrow method $exception->{rethrown} = 0; { + package Some::Other::Exception; sub rethrow { $_[0]->{rethrown} = 1; die $_[0]; } } throws_ok { Koha::Schema::Util::ExceptionTranslator->translate_exception($exception); - } qr/Some::Other::Exception/, 'Non-DBIx::Class exceptions are rethrown unchanged'; + } + qr/Some::Other::Exception/, 'Non-DBIx::Class exceptions are rethrown unchanged'; + + $schema->storage->txn_rollback; +}; + +subtest 'fk_constraint_deletion_translation' => sub { + plan tests => 1; + + $schema->storage->txn_begin; + + # Create a mock DBIx::Class::Exception for FK constraint deletion + my $exception = + bless { msg => + "Cannot delete or update a parent row: a foreign key constraint fails (`koha`.`items`, CONSTRAINT `items_ibfk_1` FOREIGN KEY (`biblionumber`) REFERENCES `biblio` (`biblionumber`))" + }, 'DBIx::Class::Exception'; + + throws_ok { + Koha::Schema::Util::ExceptionTranslator->translate_exception($exception); + } + 'Koha::Exceptions::Object::FKConstraintDeletion', 'FK constraint deletion exception is properly translated'; $schema->storage->txn_rollback; }; @@ -132,23 +150,26 @@ subtest 'schema_safe_do_method' => sub { $schema->storage->txn_begin; # Test successful operation - my $result = $schema->safe_do(sub { - return "success"; - }); + my $result = $schema->safe_do( + sub { + return "success"; + } + ); is( $result, "success", 'safe_do returns result on success' ); # Test exception translation throws_ok { - $schema->safe_do(sub { - # Create a mock DBIx::Class::Exception - my $exception = bless { - msg => "Duplicate entry 'test' for key 'primary'" - }, 'DBIx::Class::Exception'; - die $exception; - }); - } 'Koha::Exceptions::Object::DuplicateID', 'safe_do translates exceptions properly'; + $schema->safe_do( + sub { + # Create a mock DBIx::Class::Exception + my $exception = bless { msg => "Duplicate entry 'test' for key 'primary'" }, 'DBIx::Class::Exception'; + die $exception; + } + ); + } + 'Koha::Exceptions::Object::DuplicateID', 'safe_do translates exceptions properly'; $schema->storage->txn_rollback; }; -1; \ No newline at end of file +1; -- 2.51.0