Bug 19871

Summary: Use new exceptions Koha::Exceptions::Object::DuplicateID and FKConstraint
Product: Koha Reporter: Josef Moravec <josef.moravec>
Component: Architecture, internals, and plumbingAssignee: Martin Renvoize (ashimema) <martin.renvoize>
Status: Needs Signoff --- QA Contact: Tomás Cohen Arazi (tcohen) <tomascohen>
Severity: enhancement    
Priority: P5 - low CC: jonathan.druart, martin.renvoize, nick, tomascohen
Version: Main   
Hardware: All   
OS: All   
GIT URL: Initiative type: ---
Sponsorship status: --- Comma delimited list of Sponsors:
Crowdfunding goal: 0 Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Circulation function:
Bug Depends on: 19828    
Bug Blocks: 40445    
Attachments: Bug 19871: Add centralized DBIx::Class exception translation system
Bug 19871: Use centralized exception translation in Koha::Item::add_to_bundle
Bug 19871: Add centralized DBIx::Class exception translation system
Bug 19871: Use centralized exception translation in Koha::Item::add_to_bundle
Bug 19871: Move FK constraint deletion handling to centralized exception translation
Bug 19871: Update virtualshelf templates to use Koha exceptions instead of DBIx::Class::Exception
Bug 19871: Update Koha::Patron::Modification to use centralized exception handling
Bug 19871: Move enum data truncation handling to centralized exception translation
Bug 19871: (QA follow-up) Replace symbolic refs with proper mock object class
Bug 19871: Add centralized DBIx::Class exception translation system
Bug 19871: Use centralized exception translation in Koha::Item::add_to_bundle
Bug 19871: Move FK constraint deletion handling to centralized exception translation
Bug 19871: Update virtualshelf templates to use Koha exceptions instead of DBIx::Class::Exception
Bug 19871: Update Koha::Patron::Modification to use centralized exception handling
Bug 19871: Move enum data truncation handling to centralized exception translation
Bug 19871: (QA follow-up) Replace symbolic refs with proper mock object class
Bug 19871: Make exception handling more explicit in add_to_bundle

Description Josef Moravec 2017-12-22 07:50:46 UTC
There are some reference of DBIx::Class:Exception in our code, they should be updated to use new Koha::Exceptions from bug 19828.
Comment 1 Josef Moravec 2017-12-22 07:51:31 UTC
Affected files:

Koha/Patron/Modification.pm
Koha/REST/V1/Acquisitions/Vendors.pm
Koha/REST/V1/Cities.pm
koha-tmpl/intranet-tmpl/prog/en/modules/virtualshelves/shelves.tt
koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-shelves.tt
Comment 2 Martin Renvoize (ashimema) 2025-09-21 11:54:21 UTC
Created attachment 186634 [details] [review]
Bug 19871: Add centralized DBIx::Class exception translation system

This introduces a reusable exception translation utility that eliminates
code duplication and provides consistent handling of DBIx::Class exceptions
throughout the codebase.

New Components:
- Koha::Schema::Util::ExceptionTranslator: Core utility class for translating
  DBIx::Class exceptions to Koha-specific exceptions
- Koha::Schema: Enhanced with safe_do() and translate_exception() methods
  for convenient access to centralized exception handling
- Comprehensive unit tests covering all translation scenarios

Benefits:
- Eliminates ~90 lines of duplicated exception handling code
- Centralizes maintenance of exception translation logic
- Provides consistent error messages and exception types
- Makes it easier to extend support for additional database engines
- Follows the pattern suggested in existing FIXME comments

Exception Types Handled:
- Foreign key constraint violations → Koha::Exceptions::Object::FKConstraint
- Duplicate key violations → Koha::Exceptions::Object::DuplicateID
- Invalid data type values → Koha::Exceptions::Object::BadValue
- Enum data truncation → Koha::Exceptions::Object::BadValue
Comment 3 Martin Renvoize (ashimema) 2025-09-21 11:54:23 UTC
Created attachment 186635 [details] [review]
Bug 19871: Use centralized exception translation in Koha::Item::add_to_bundle

Replace duplicated DBIx::Class exception handling code with the new
centralized exception translation system introduced in the previous commit.

This change:
- Eliminates ~40 lines of duplicated exception handling code
- Uses the new Koha::Schema::translate_exception() method
- Maintains identical exception behavior and error messages
- Addresses the FIXME comment requesting this refactoring

The add_to_bundle method now uses consistent exception translation
with the rest of the codebase, reducing maintenance burden and
ensuring consistent error handling patterns.
Comment 4 Martin Renvoize (ashimema) 2025-09-21 15:14:38 UTC
Created attachment 186644 [details] [review]
Bug 19871: Add centralized DBIx::Class exception translation system

This introduces a reusable exception translation utility that eliminates
code duplication and provides consistent handling of DBIx::Class exceptions
throughout the codebase.

New Components:
- Koha::Schema::Util::ExceptionTranslator: Core utility class for translating
  DBIx::Class exceptions to Koha-specific exceptions
- Koha::Schema: Enhanced with safe_do() and translate_exception() methods
  for convenient access to centralized exception handling
- Comprehensive unit tests covering all translation scenarios

Benefits:
- Eliminates ~90 lines of duplicated exception handling code
- Centralizes maintenance of exception translation logic
- Provides consistent error messages and exception types
- Makes it easier to extend support for additional database engines
- Follows the pattern suggested in existing FIXME comments

Exception Types Handled:
- Foreign key constraint violations → Koha::Exceptions::Object::FKConstraint
- Duplicate key violations → Koha::Exceptions::Object::DuplicateID
- Invalid data type values → Koha::Exceptions::Object::BadValue
- Enum data truncation → Koha::Exceptions::Object::BadValue
Comment 5 Martin Renvoize (ashimema) 2025-09-21 15:14:47 UTC
Created attachment 186645 [details] [review]
Bug 19871: Use centralized exception translation in Koha::Item::add_to_bundle

Replace duplicated DBIx::Class exception handling code with the new
centralized exception translation system introduced in the previous commit.

This change:
- Eliminates ~40 lines of duplicated exception handling code
- Uses the new Koha::Schema::translate_exception() method
- Maintains identical exception behavior and error messages
- Addresses the FIXME comment requesting this refactoring

The add_to_bundle method now uses consistent exception translation
with the rest of the codebase, reducing maintenance burden and
ensuring consistent error handling patterns.
Comment 6 Martin Renvoize (ashimema) 2025-09-21 15:14:56 UTC
Created attachment 186646 [details] [review]
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)
Comment 7 Martin Renvoize (ashimema) 2025-09-21 15:15:06 UTC
Created attachment 186647 [details] [review]
Bug 19871: Update virtualshelf templates to use Koha exceptions instead of DBIx::Class::Exception

Replace DBIx::Class::Exception handling in shelf templates with specific
Koha exception types that are now provided by the centralized exception
translation system.

Template changes:
- Replace 'DBIx::Class::Exception' case with specific Koha exception handlers
- Add user-friendly error messages for FKConstraint, DuplicateID, and BadValue
- Improve error presentation with descriptive text instead of raw database errors

This provides better user experience by showing meaningful error messages
instead of technical database exception details.
Comment 8 Martin Renvoize (ashimema) 2025-09-21 15:15:11 UTC
Created attachment 186648 [details] [review]
Bug 19871: Update Koha::Patron::Modification to use centralized exception handling

Replace manual DBIx::Class::Exception handling in the approve() method with
the centralized exception translation system while preserving domain-specific
exception behavior.

Changes:
- Use $schema->safe_do() for database operations with automatic exception translation
- Wrap safe_do in try/catch to convert any exception to domain-specific Patron::Modification exception
- Simplify exception handling logic while maintaining the same API contract
- Remove manual DBIx::Class::Exception type checking

This maintains backward compatibility by always throwing
Koha::Exceptions::Patron::Modification exceptions as before, but leverages
the centralized system for consistent database exception handling.
Comment 9 Martin Renvoize (ashimema) 2025-09-21 15:15:13 UTC
Created attachment 186649 [details] [review]
Bug 19871: Move enum data truncation handling to centralized exception translation

Complete the migration of enum data truncation exception handling from
Koha::Object to the centralized ExceptionTranslator utility. This eliminates
the last remaining manual DBIx::Class exception handling in Object code.

Changes:
- Add optional object parameter to ExceptionTranslator->translate_exception()
- Update Schema wrapper methods to accept and pass object parameter
- Enhance enum truncation handling to extract actual property values when object is available
- Remove manual enum exception handling from Koha::Object::store()
- Add comprehensive test coverage for enum handling with and without object context
- Maintain contextual warning behavior for store() operations

The centralized system now handles ALL DBIx::Class exception scenarios:
- FK constraint failures (add/update and delete operations)
- Duplicate key violations
- Invalid data type values
- Enum data truncation with actual property values

This completes the centralization effort while providing better error reporting
than the previous manual approach.
Comment 10 Martin Renvoize (ashimema) 2025-09-21 15:15:16 UTC
Created attachment 186650 [details] [review]
Bug 19871: (QA follow-up) Replace symbolic refs with proper mock object class

Replace 'no strict refs' usage with a proper TestObject package to avoid
Perl::Critic violations while maintaining the same test functionality.

The mock object now uses standard OOP patterns instead of symbolic
references for method creation.
Comment 11 Tomás Cohen Arazi (tcohen) 2025-09-30 20:58:09 UTC
I really love this. Great job!

My only doubt is the explicit call in the catch in Koha::Item:

```
$schema->translate_exception($_);
```

I'd expect a `->rethrow` or similar instead.


This is great stuff!
Comment 12 Martin Renvoize (ashimema) 2025-11-07 12:46:02 UTC Comment hidden (obsolete)
Comment 13 Martin Renvoize (ashimema) 2025-11-07 12:46:05 UTC Comment hidden (obsolete)
Comment 14 Martin Renvoize (ashimema) 2025-11-07 12:46:07 UTC Comment hidden (obsolete)
Comment 15 Martin Renvoize (ashimema) 2025-11-07 12:46:09 UTC Comment hidden (obsolete)
Comment 16 Martin Renvoize (ashimema) 2025-11-07 12:46:11 UTC Comment hidden (obsolete)
Comment 17 Martin Renvoize (ashimema) 2025-11-07 12:46:13 UTC Comment hidden (obsolete)
Comment 18 Martin Renvoize (ashimema) 2025-11-07 12:46:16 UTC Comment hidden (obsolete)
Comment 19 Martin Renvoize (ashimema) 2025-11-07 12:46:18 UTC
Created attachment 189312 [details] [review]
Bug 19871: Make exception handling more explicit in add_to_bundle

Replace the implicit exception translation call with an explicit safe_do
pattern that clearly shows exceptions are rethrown.

Changes:
- Use $schema->safe_do() wrapper for automatic exception translation
- Add explicit $_->rethrow() in catch block for clarity
- Follow the same pattern used in Koha::Patron::Modification::approve()
- Make control flow more obvious to future maintainers

This change maintains identical behavior while improving code clarity
by making it explicit that the catch block never returns normally.
Comment 20 Chris Cormack 2025-11-21 00:32:59 UTC
Automatic patch test results:

â ï¸ The patch applies with fuzzy option (ignoring whitespace).
This may indicate minor conflicts that need to be resolved.

Details: Patch applies with fuzzy option (ignoring whitespace)

This is an automated message from the patch testing system.