Bugzilla – Attachment 186646 Details for
Bug 19871
Use new exceptions Koha::Exceptions::Object::DuplicateID and FKConstraint
Home
|
New
|
Browse
|
Search
|
[?]
|
Reports
|
Help
|
New Account
|
Log In
[x]
|
Forgot Password
Login:
[x]
[patch]
Bug 19871: Move FK constraint deletion handling to centralized exception translation
Bug-19871-Move-FK-constraint-deletion-handling-to-.patch (text/plain), 12.68 KB, created by
Martin Renvoize (ashimema)
on 2025-09-21 15:14:56 UTC
(
hide
)
Description:
Bug 19871: Move FK constraint deletion handling to centralized exception translation
Filename:
MIME Type:
Creator:
Martin Renvoize (ashimema)
Created:
2025-09-21 15:14:56 UTC
Size:
12.68 KB
patch
obsolete
>From e496751eed40147f23f67e3db341689e878c17dc Mon Sep 17 00:00:00 2001 >From: Martin Renvoize <martin.renvoize@openfifth.co.uk> >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 \(\`(?<database>.*?)\`\.\`(?<table>.*?)\`, CONSTRAINT \`(?<constraint>.*?)\` FOREIGN KEY \(\`(?<fk>.*?)\`\) REFERENCES \`.*\` \(\`(?<column>.*?)\`\)/ >- ) >- { >- 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 \(`(?<column>.*?)`\)/ ) { > 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 \(\`(?<database>.*?)\`\.\`(?<table>.*?)\`, CONSTRAINT \`(?<constraint>.*?)\` FOREIGN KEY \(\`(?<fk>.*?)\`\) REFERENCES \`.*\` \(\`(?<column>.*?)\`\)/ >+ ) >+ { >+ Koha::Exceptions::Object::FKConstraintDeletion->throw( >+ column => $+{column}, >+ constraint => $+{constraint}, >+ fk => $+{fk}, >+ table => $+{table}, >+ ); >+ } >+ > # Duplicate key violations > elsif ( $msg =~ /Duplicate entry '(.*?)' for key '(?<key>.*?)'/ ) { > Koha::Exceptions::Object::DuplicateID->throw( >@@ -104,8 +119,10 @@ sub translate_exception { > duplicate_id => $+{key} > ); > } >+ > # Invalid data type values > elsif ( $msg =~ /Incorrect (?<type>\w+) value: '(?<value>.*)' for column \W?(?<property>\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?(?<property>\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
You cannot view the attachment while viewing its details because your browser does not support IFRAMEs.
View the attachment on a separate page
.
View Attachment As Diff
View Attachment As Raw
Actions:
View
|
Diff
|
Splinter Review
Attachments on
bug 19871
:
186634
|
186635
|
186644
|
186645
| 186646 |
186647
|
186648
|
186649
|
186650