Bug 19828 - Koha::Object->store should catch DBIC exceptions and raise Koha::Exceptions
Summary: Koha::Object->store should catch DBIC exceptions and raise Koha::Exceptions
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Tomás Cohen Arazi
QA Contact: Josef Moravec
URL:
Keywords:
Depends on:
Blocks: 19871 16330 22051
  Show dependency treegraph
 
Reported: 2017-12-18 16:19 UTC by Tomás Cohen Arazi
Modified: 2018-12-28 14:15 UTC (History)
8 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 19828: Unit tests (1.93 KB, patch)
2017-12-18 16:54 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 19828: Make Koha::Object->store translate DBIC exceptions into Koha exceptions (2.83 KB, patch)
2017-12-18 16:54 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 19828: Unit tests (3.19 KB, patch)
2017-12-19 13:34 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 19828: Make Koha::Object->store translate DBIC exceptions into Koha exceptions (3.14 KB, patch)
2017-12-19 13:34 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 19828: Unit tests (3.37 KB, patch)
2017-12-20 19:11 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 19828: Make Koha::Object->store translate DBIC exceptions into Koha exceptions (3.14 KB, patch)
2017-12-20 19:11 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 19828: Fix tests that expect DBD errors instead of the exception message (981 bytes, patch)
2017-12-20 19:11 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 19828: Unit tests (3.42 KB, patch)
2017-12-21 15:07 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 19828: Make Koha::Object->store translate DBIC exceptions into Koha exceptions (3.18 KB, patch)
2017-12-21 15:07 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 19828: Fix tests that expect DBD errors instead of the exception message (1.01 KB, patch)
2017-12-21 15:07 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 19828: Unit tests (3.47 KB, patch)
2017-12-22 07:46 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 19828: Make Koha::Object->store translate DBIC exceptions into Koha exceptions (3.23 KB, patch)
2017-12-22 07:46 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 19828: Fix tests that expect DBD errors instead of the exception message (1.06 KB, patch)
2017-12-22 07:46 UTC, Josef Moravec
Details | Diff | Splinter 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 (940 bytes, patch)
2017-12-22 07:46 UTC, Josef Moravec
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
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!