Bug 29387 - BatchUpdateBiblio does not handle exception correctly
Summary: BatchUpdateBiblio does not handle exception correctly
Status: Pushed to master
Alias: None
Product: Koha
Classification: Unclassified
Component: Tools (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low major (vote)
Assignee: Jonathan Druart
QA Contact: Marcel de Rooy
URL:
Keywords:
Depends on: 22417
Blocks:
  Show dependency treegraph
 
Reported: 2021-11-02 09:42 UTC by Jonathan Druart
Modified: 2021-11-22 10:54 UTC (History)
5 users (show)

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


Attachments
Bug 29387: Stringify exception when logging error during a batch mod biblio (3.95 KB, patch)
2021-11-02 09:48 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 29387: Stringify exception when logging error during a batch mod biblio (4.56 KB, patch)
2021-11-02 10:44 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 29387: Strinfigy exceptions for other background job modules (2.71 KB, patch)
2021-11-08 09:23 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 29387: Stringify exception when logging error during a batch mod biblio (4.74 KB, patch)
2021-11-12 18:13 UTC, Joonas Kylmälä
Details | Diff | Splinter Review
Bug 29387: Strinfigy exceptions for other background job modules (2.85 KB, patch)
2021-11-12 18:13 UTC, Joonas Kylmälä
Details | Diff | Splinter Review
Bug 29387: Stringify exception when logging error during a batch mod biblio (4.84 KB, patch)
2021-11-19 07:43 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 29387: Stringify exceptions for other background job modules (2.94 KB, patch)
2021-11-19 07:43 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 29387: (QA follow-up) Fix modules in test (1.20 KB, patch)
2021-11-19 07:43 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 29387: [21.05] Stringify exception when logging error during a batch mod biblio (5.99 KB, patch)
2021-11-22 10:53 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Druart 2021-11-02 09:42:54 UTC
If an exception is raised by one the methods/subroutines called when batch biblio update, the job will fail in the middle and some records won't be processed.
Comment 1 Jonathan Druart 2021-11-02 09:48:09 UTC
Created attachment 127205 [details] [review]
Bug 29387: Stringify exception when logging error during a batch mod biblio

If an exception is raised by one the methods/subroutines called when batch biblio update, the job will fail in the middle and some records won't be processed.

Test plan:
0. Don't apply the patch
In KTD you can run the a batch mod biblio on the 100 first records and
the worker will fail with
"""
countered object 'C4::Biblio::_koha_modify_biblioitem_nonmarc(): DBI Exception: DBD::mysql::st execute failed: Data too long for column 'lccn' at row 1 at /kohadevbox/koha/C4/Biblio.pm line 423
', but neither allow_blessed, convert_blessed nor allow_tags settings are enabled (or TO_JSON/FREEZE method missing) at /kohadevbox/koha/Koha/BackgroundJob/BatchUpdateBiblio.pm line 120.
"""
The UI is not saying anything about this problem.

Generate 100 biblionumbers: perl -e 'for (1..100){print "$_\n"}'
1. Apply the patch and confirm than now you see the detail of the batch
update and that biblionumber 72 is marked as not been modified with the
correct error (raw DBI error)
Comment 2 Jonathan Druart 2021-11-02 10:44:25 UTC
Created attachment 127207 [details] [review]
Bug 29387: Stringify exception when logging error during a batch mod biblio

If an exception is raised by one the methods/subroutines called when batch biblio update, the job will fail in the middle and some records won't be processed.

Test plan:
0. Don't apply the patch
In KTD you can run the a batch mod biblio on the 100 first records and
the worker will fail with
"""
countered object 'C4::Biblio::_koha_modify_biblioitem_nonmarc(): DBI Exception: DBD::mysql::st execute failed: Data too long for column 'lccn' at row 1 at /kohadevbox/koha/C4/Biblio.pm line 423
', but neither allow_blessed, convert_blessed nor allow_tags settings are enabled (or TO_JSON/FREEZE method missing) at /kohadevbox/koha/Koha/BackgroundJob/BatchUpdateBiblio.pm line 120.
"""
The UI is not saying anything about this problem.

Generate 100 biblionumbers: perl -e 'for (1..100){print "$_\n"}'
1. Apply the patch and confirm than now you see the detail of the batch
update and that biblionumber 72 is marked as not been modified with the
correct error (raw DBI error)
Comment 3 Joonas Kylmälä 2021-11-06 12:48:14 UTC
This only fixes the bug from one place, these need the same fix:

./Koha/BackgroundJob/BatchDeleteAuthority.pm60:                error => ($@ ? $@ : 0),
./Koha/BackgroundJob/BatchUpdateAuthority.pm93:                error => ($@ ? $@ : 0),
./Koha/BackgroundJob/BatchCancelHold.pm97:                ? ( $@ ? $@ : 0 )
./Koha/BackgroundJob/BatchDeleteBiblio.pm139:                error => ($@ ? $@ : $error),
Comment 4 Jonathan Druart 2021-11-08 09:23:40 UTC
Created attachment 127431 [details] [review]
Bug 29387: Strinfigy exceptions for other background job modules
Comment 5 Fridolin Somers 2021-11-12 07:11:08 UTC
We have two behaviors :
  error => "$@"
  error => ($@ ? "$@" : 0)
Shouldn't it be only one here ?
Comment 6 Joonas Kylmälä 2021-11-12 18:13:52 UTC
Created attachment 127594 [details] [review]
Bug 29387: Stringify exception when logging error during a batch mod biblio

If an exception is raised by one the methods/subroutines called when batch biblio update, the job will fail in the middle and some records won't be processed.

Test plan:
0. Don't apply the patch
In KTD you can run the a batch mod biblio on the 100 first records and
the worker will fail with
"""
countered object 'C4::Biblio::_koha_modify_biblioitem_nonmarc(): DBI Exception: DBD::mysql::st execute failed: Data too long for column 'lccn' at row 1 at /kohadevbox/koha/C4/Biblio.pm line 423
', but neither allow_blessed, convert_blessed nor allow_tags settings are enabled (or TO_JSON/FREEZE method missing) at /kohadevbox/koha/Koha/BackgroundJob/BatchUpdateBiblio.pm line 120.
"""
The UI is not saying anything about this problem.

Generate 100 biblionumbers: perl -e 'for (1..100){print "$_\n"}'
1. Apply the patch and confirm than now you see the detail of the batch
update and that biblionumber 72 is marked as not been modified with the
correct error (raw DBI error)

JK: add executable bit to BatchUpdateBiblio.t
Signed-off-by: Joonas Kylmälä <joonas.kylmala@iki.fi>
Comment 7 Joonas Kylmälä 2021-11-12 18:13:56 UTC
Created attachment 127595 [details] [review]
Bug 29387: Strinfigy exceptions for other background job modules

Signed-off-by: Joonas Kylmälä <joonas.kylmala@iki.fi>
Comment 8 Joonas Kylmälä 2021-11-12 18:16:29 UTC
(In reply to Fridolin Somers from comment #5)
> We have two behaviors :
>   error => "$@"
>   error => ($@ ? "$@" : 0)
> Shouldn't it be only one here ?

In the   error => "$@" cases there is already the if else check in the outer "if else" block so no need for this.
Comment 9 Marcel de Rooy 2021-11-19 07:20:46 UTC
QAing
Comment 10 Marcel de Rooy 2021-11-19 07:41:21 UTC
Did we consider the allow_blessed/convert_blessed route in JSON?
Comment 11 Marcel de Rooy 2021-11-19 07:43:36 UTC
Created attachment 127825 [details] [review]
Bug 29387: Stringify exception when logging error during a batch mod biblio

If an exception is raised by one the methods/subroutines called when batch biblio update, the job will fail in the middle and some records won't be processed.

Test plan:
0. Don't apply the patch
In KTD you can run the a batch mod biblio on the 100 first records and
the worker will fail with
"""
countered object 'C4::Biblio::_koha_modify_biblioitem_nonmarc(): DBI Exception: DBD::mysql::st execute failed: Data too long for column 'lccn' at row 1 at /kohadevbox/koha/C4/Biblio.pm line 423
', but neither allow_blessed, convert_blessed nor allow_tags settings are enabled (or TO_JSON/FREEZE method missing) at /kohadevbox/koha/Koha/BackgroundJob/BatchUpdateBiblio.pm line 120.
"""
The UI is not saying anything about this problem.

Generate 100 biblionumbers: perl -e 'for (1..100){print "$_\n"}'
1. Apply the patch and confirm than now you see the detail of the batch
update and that biblionumber 72 is marked as not been modified with the
correct error (raw DBI error)

JK: add executable bit to BatchUpdateBiblio.t
Signed-off-by: Joonas Kylmälä <joonas.kylmala@iki.fi>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 12 Marcel de Rooy 2021-11-19 07:43:39 UTC
Created attachment 127826 [details] [review]
Bug 29387: Stringify exceptions for other background job modules

Signed-off-by: Joonas Kylmälä <joonas.kylmala@iki.fi>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 13 Marcel de Rooy 2021-11-19 07:43:43 UTC
Created attachment 127827 [details] [review]
Bug 29387: (QA follow-up) Fix modules in test

Can't locate object method "new" via package "Koha::BackgroundJob::BatchUpdateBiblio" (perhaps you forgot to load "Koha::BackgroundJob::BatchUpdateBiblio"?) at t/db_dependent/Koha/BackgroundJobs/BatchUpdateBiblio.t line 45.

How did it pass at signoff?

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 14 Jonathan Druart 2021-11-19 09:41:46 UTC
(In reply to Marcel de Rooy from comment #13)
> Created attachment 127827 [details] [review] [review]
> Bug 29387: (QA follow-up) Fix modules in test
> 
> Can't locate object method "new" via package
> "Koha::BackgroundJob::BatchUpdateBiblio" (perhaps you forgot to load
> "Koha::BackgroundJob::BatchUpdateBiblio"?) at
> t/db_dependent/Koha/BackgroundJobs/BatchUpdateBiblio.t line 45.
> 
> How did it pass at signoff?
> 
> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Thanks for the follow-up. It got broken by bug 28413 that was pushed in the meanwhile.

(In reply to Marcel de Rooy from comment #10)
> Did we consider the allow_blessed/convert_blessed route in JSON?

I didn't, we could but I wouldn't introduce such changes for 21.11.00
Comment 15 Jonathan Druart 2021-11-19 14:15:42 UTC
Pushed to master for 21.11, thanks to everybody involved!
Comment 16 Kyle M Hall 2021-11-19 16:57:30 UTC
Doesn't apply to 21.05.x. Please rebase and let me know if this is needed in 21.05!
Comment 17 Jonathan Druart 2021-11-22 10:53:47 UTC
Created attachment 127924 [details] [review]
Bug 29387: [21.05] Stringify exception when logging error during a batch mod biblio

If an exception is raised by one the methods/subroutines called when batch biblio update, the job will fail in the middle and some records won't be processed.

Test plan:
0. Don't apply the patch
In KTD you can run the a batch mod biblio on the 100 first records and
the worker will fail with
"""
countered object 'C4::Biblio::_koha_modify_biblioitem_nonmarc(): DBI Exception: DBD::mysql::st execute failed: Data too long for column 'lccn' at row 1 at /kohadevbox/koha/C4/Biblio.pm line 423
', but neither allow_blessed, convert_blessed nor allow_tags settings are enabled (or TO_JSON/FREEZE method missing) at /kohadevbox/koha/Koha/BackgroundJob/BatchUpdateBiblio.pm line 120.
"""
The UI is not saying anything about this problem.

Generate 100 biblionumbers: perl -e 'for (1..100){print "$_\n"}'
1. Apply the patch and confirm than now you see the detail of the batch
update and that biblionumber 72 is marked as not been modified with the
correct error (raw DBI error)

JK: add executable bit to BatchUpdateBiblio.t
Signed-off-by: Joonas Kylmälä <joonas.kylmala@iki.fi>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Bug 29387: Stringify exceptions for other background job modules

Signed-off-by: Joonas Kylmälä <joonas.kylmala@iki.fi>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Bug 29387: (QA follow-up) Fix modules in test

Can't locate object method "new" via package "Koha::BackgroundJob::BatchUpdateBiblio" (perhaps you forgot to load "Koha::BackgroundJob::BatchUpdateBiblio"?) at t/db_dependent/Koha/BackgroundJobs/BatchUpdateBiblio.t line 45.

How did it pass at signoff?

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 18 Jonathan Druart 2021-11-22 10:54:50 UTC
(In reply to Kyle M Hall from comment #16)
> Doesn't apply to 21.05.x. Please rebase and let me know if this is needed in
> 21.05!

Yes it's needed, I rebased and squashed the patches.