Bug 19828

Summary: Koha::Object->store should catch DBIC exceptions and raise Koha::Exceptions
Product: Koha Reporter: Tomás Cohen Arazi <tomascohen>
Component: Architecture, internals, and plumbingAssignee: Tomás Cohen Arazi <tomascohen>
Status: CLOSED FIXED QA Contact: Josef Moravec <josef.moravec>
Severity: enhancement    
Priority: P5 - low CC: benjamin.rokseth, jonathan.druart, josef.moravec, kyle, lari.taskula, m.de.rooy, nick, tomascohen
Version: Main   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: Small patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Bug Depends on:    
Bug Blocks: 19871, 16330, 22051    
Attachments: Bug 19828: Unit tests
Bug 19828: Make Koha::Object->store translate DBIC exceptions into Koha exceptions
Bug 19828: Unit tests
Bug 19828: Make Koha::Object->store translate DBIC exceptions into Koha exceptions
Bug 19828: Unit tests
Bug 19828: Make Koha::Object->store translate DBIC exceptions into Koha exceptions
Bug 19828: Fix tests that expect DBD errors instead of the exception message
Bug 19828: Unit tests
Bug 19828: Make Koha::Object->store translate DBIC exceptions into Koha exceptions
Bug 19828: Fix tests that expect DBD errors instead of the exception message
Bug 19828: Unit tests
Bug 19828: Make Koha::Object->store translate DBIC exceptions into Koha exceptions
Bug 19828: Fix tests that expect DBD errors instead of the exception message
Bug 19828: (QA follow-up) Fix rethrowing exception and move rethrowing out of 'if' statement just in case the exception does not came from DBIx::Class

Description Tomás Cohen Arazi 2017-12-18 16:19:17 UTC
DBIC exceptions are not suitable for reporting end users and API consumers the problems we find. They need to be parsed to find out the root cause of the problems. This could be done in Koha::Object-based classes (see bug 16330 for a discussion on this), but it should be handled more generically, in Koha::Object->store.
Comment 1 Tomás Cohen Arazi 2017-12-18 16:54:04 UTC Comment hidden (obsolete)
Comment 2 Tomás Cohen Arazi 2017-12-18 16:54:09 UTC Comment hidden (obsolete)
Comment 3 Tomás Cohen Arazi 2017-12-19 13:34:51 UTC
Created attachment 69907 [details] [review]
Bug 19828: Unit tests

This patch introduces unit tests for the changes this bug introduces to
Koha::Object->store.

To test:
- Apply this patch
- Run:
  $ kshell
 k$ prove t/db_dependent/Koha/Object.t
=> FAIL: Tests should fail because the changes are not implemented on this patch
Comment 4 Tomás Cohen Arazi 2017-12-19 13:34:57 UTC
Created attachment 69908 [details] [review]
Bug 19828: Make Koha::Object->store translate DBIC exceptions into Koha exceptions

This patch introduces a try/catch block in store() and parses the error
when an exceptional situation takes place.

It only deals with FK constraint violations and duplicate IDs. The rest of the DBIC exceptions
are rethrown.

To test:

- Apply this patch
- Run:
  $ kshell
 k$ prove t/db_dependent/Koha/Object.t
=> SUCCESS: Tests pass!
- Sign off :-D
Comment 5 Tomás Cohen Arazi 2017-12-19 13:35:38 UTC
I added the duplicate key use case.
Comment 6 Tomás Cohen Arazi 2017-12-20 19:11:17 UTC
Created attachment 69963 [details] [review]
Bug 19828: Unit tests

This patch introduces unit tests for the changes this bug introduces to
Koha::Object->store.

To test:
- Apply this patch
- Run:
  $ kshell
 k$ prove t/db_dependent/Koha/Object.t
=> FAIL: Tests should fail because the changes are not implemented on this patch
Comment 7 Tomás Cohen Arazi 2017-12-20 19:11:23 UTC
Created attachment 69964 [details] [review]
Bug 19828: Make Koha::Object->store translate DBIC exceptions into Koha exceptions

This patch introduces a try/catch block in store() and parses the error
when an exceptional situation takes place.

It only deals with FK constraint violations and duplicate IDs. The rest of the DBIC exceptions
are rethrown.

To test:

- Apply this patch
- Run:
  $ kshell
 k$ prove t/db_dependent/Koha/Object.t
=> SUCCESS: Tests pass!
- Sign off :-D
Comment 8 Tomás Cohen Arazi 2017-12-20 19:11:36 UTC
Created attachment 69965 [details] [review]
Bug 19828: Fix tests that expect DBD errors instead of the exception message
Comment 9 Kyle M Hall 2017-12-21 15:07:39 UTC
Created attachment 70048 [details] [review]
Bug 19828: Unit tests

This patch introduces unit tests for the changes this bug introduces to
Koha::Object->store.

To test:
- Apply this patch
- Run:
  $ kshell
 k$ prove t/db_dependent/Koha/Object.t
=> FAIL: Tests should fail because the changes are not implemented on this patch

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 10 Kyle M Hall 2017-12-21 15:07:46 UTC
Created attachment 70049 [details] [review]
Bug 19828: Make Koha::Object->store translate DBIC exceptions into Koha exceptions

This patch introduces a try/catch block in store() and parses the error
when an exceptional situation takes place.

It only deals with FK constraint violations and duplicate IDs. The rest of the DBIC exceptions
are rethrown.

To test:

- Apply this patch
- Run:
  $ kshell
 k$ prove t/db_dependent/Koha/Object.t
=> SUCCESS: Tests pass!
- Sign off :-D

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 11 Kyle M Hall 2017-12-21 15:07:49 UTC
Created attachment 70050 [details] [review]
Bug 19828: Fix tests that expect DBD errors instead of the exception message

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 12 Josef Moravec 2017-12-22 07:46:33 UTC
Created attachment 70097 [details] [review]
Bug 19828: Unit tests

This patch introduces unit tests for the changes this bug introduces to
Koha::Object->store.

To test:
- Apply this patch
- Run:
  $ kshell
 k$ prove t/db_dependent/Koha/Object.t
=> FAIL: Tests should fail because the changes are not implemented on this patch

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 13 Josef Moravec 2017-12-22 07:46:36 UTC
Created attachment 70098 [details] [review]
Bug 19828: Make Koha::Object->store translate DBIC exceptions into Koha exceptions

This patch introduces a try/catch block in store() and parses the error
when an exceptional situation takes place.

It only deals with FK constraint violations and duplicate IDs. The rest of the DBIC exceptions
are rethrown.

To test:

- Apply this patch
- Run:
  $ kshell
 k$ prove t/db_dependent/Koha/Object.t
=> SUCCESS: Tests pass!
- Sign off :-D

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 14 Josef Moravec 2017-12-22 07:46:39 UTC
Created attachment 70099 [details] [review]
Bug 19828: Fix tests that expect DBD errors instead of the exception message

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 15 Josef Moravec 2017-12-22 07:46:43 UTC
Created attachment 70100 [details] [review]
Bug 19828: (QA follow-up) Fix rethrowing exception and move rethrowing out of 'if' statement just in case the exception does not came from DBIx::Class

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 16 Jonathan Druart 2017-12-22 16:17:35 UTC
Pushed to master for 18.05, thanks to everybody involved!
Comment 17 Nick Clemens 2018-01-16 12:14:19 UTC
Enhancement, skipping for 17.11.x.
Awesome work everybody!