Bug 29087 - Holds to pull list can crash with a SQL::Abstract puke
Summary: Holds to pull list can crash with a SQL::Abstract puke
Status: Signed Off
Alias: None
Product: Koha
Classification: Unclassified
Component: Hold requests (show other bugs)
Version: Main
Hardware: All All
: P5 - low major with 1 vote (vote)
Assignee: Bugs List
QA Contact: Testopia
URL:
Keywords:
Depends on: 3142
Blocks:
  Show dependency treegraph
 
Reported: 2021-09-22 21:16 UTC by Galen Charlton
Modified: 2024-05-10 10:03 UTC (History)
13 users (show)

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


Attachments
Bug 29087: Prevent filter_by_for_hold to crash if default holdallowed is not_allowed (3.47 KB, patch)
2021-11-19 12:28 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 29087: Add unit tests (6.74 KB, patch)
2024-04-16 13:25 UTC, Matt Blenkinsop
Details | Diff | Splinter Review
Bug 29087: Prevent filter_by_for_hold to crash if default holdallowed is not_allowed (6.60 KB, patch)
2024-05-08 08:23 UTC, Matt Blenkinsop
Details | Diff | Splinter Review
Bug 29087: Add unit tests (6.74 KB, patch)
2024-05-08 08:23 UTC, Matt Blenkinsop
Details | Diff | Splinter Review
Bug 29087: Prevent filter_by_for_hold to crash if default holdallowed is not_allowed (6.63 KB, patch)
2024-05-08 08:33 UTC, David Nind
Details | Diff | Splinter Review
Bug 29087: Add unit tests (6.77 KB, patch)
2024-05-08 08:33 UTC, David Nind
Details | Diff | Splinter Review
Bug 29087: Prevent filter_by_for_hold to crash if default holdallowed is not_allowed (2.82 KB, patch)
2024-05-10 10:01 UTC, Matt Blenkinsop
Details | Diff | Splinter Review
Bug 29087: Add unit tests (3.18 KB, patch)
2024-05-10 10:02 UTC, Matt Blenkinsop
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Galen Charlton 2021-09-22 21:16:14 UTC
Opening the holds to pull list page can crash with the following logged error:

  SQL::Abstract::puke(): [SQL::Abstract::__ANON__] Fatal: SQL::Abstract before v1.75 used to generate incorrect SQL when the -NOT IN operator was given an undef-containing list: !!!AUDIT YOUR CODE AND DATA!!! (the upcoming Data::Query-based version of SQL::Abstract will emit the logically correct SQL instead of raising this exception) at /usr/share/koha/lib/Koha/Objects.pm line 145

I've traced this to Koha::Holds->get_items_that_can_fill(). Among other things, this routine fetches a list of item types that cannot fill holds, i.e., cases where the 'holdallowed' rule value is 'not_allowed'.

However, if your default rule is to not allow holds, the @hold_not_allowed_itypes list will end up including an entry whose value is undef. When passed to the item query, doing the -not_in filter on itype will result in the crash listed above.

A quick fix might be to filter out undef from that list. That will fix the crash, but could lead to incorrect results if the default policy is to deny holds for all item types except for ones that are going to be specifically permitted.
Comment 1 Jonathan Druart 2021-11-19 12:28:00 UTC
Created attachment 127859 [details] [review]
Bug 29087: Prevent filter_by_for_hold to crash if default holdallowed is not_allowed
Comment 2 Jonathan Druart 2021-11-19 12:29:01 UTC
This seems to be the way to go, but I don't think it's ready for integration. We should have the logic moved to Koha::ItemTypes and add test coverage. Will try and come back to this later.
Comment 3 Jonathan Druart 2021-11-19 12:31:02 UTC
Galen, thanks for reporting this bug!
Are you seeing this in master or stable?
Comment 4 Ashley Johnson 2021-12-23 01:05:43 UTC
Our library system is seeing this bug as well. Had a lot of staff reporting the error message to me today.
Comment 5 Liz Rea 2022-03-25 17:53:13 UTC
We have some libraries seeing this now as well, the rule mentioned here doesn't seem to be the culprit, but the error is the same.
Comment 6 Jonathan Druart 2022-03-25 18:42:38 UTC
Which version? Did you try the patch?
Comment 7 David Roberts 2022-10-21 02:21:53 UTC
I've tried patching a customer site with this and it works OK. Happy to sign off (even though the bug isn't requesting it yet....)
Comment 8 Katrin Fischer 2023-06-24 18:21:14 UTC
Should we move this to NSO or SO even?
Comment 9 Martin Renvoize 2023-09-19 15:29:40 UTC
We see this occasionally on customer sites too.. we've even got it backported in a couple of cases I think.
Comment 10 Jonathan Druart 2023-09-20 07:13:12 UTC
(In reply to Katrin Fischer from comment #8)
> Should we move this to NSO or SO even?

No, see comment 2.
Comment 11 Matt Blenkinsop 2024-04-16 13:25:03 UTC
Created attachment 164936 [details] [review]
Bug 29087: Add unit tests

prove t/db_dependent/Koha/Items.t
Comment 12 Matt Blenkinsop 2024-04-16 13:27:09 UTC
We're seeing this again on customer sites. Looking at the patch attached I'm not sure the code belongs in ItemTypes as the error is coming from the default circulation rules at its root. I've had a go at some unit tests for the patch Joubu did earlier. Would be good to get this resolved and pushed
Comment 13 David Nind 2024-04-29 09:53:53 UTC
Not sure from the comments if this is ready for sign off, or if more discussion is required.
Comment 14 Jonathan Druart 2024-05-07 13:04:06 UTC
(In reply to David Nind from comment #13)
> Not sure from the comments if this is ready for sign off, or if more
> discussion is required.

Yes, ready for signoff, we can move to a module later if needed.
Comment 15 David Nind 2024-05-08 08:20:14 UTC
The patch no longer applies:

git bz apply 29087

Bug 29087 - Holds to pull list can crash with a SQL::Abstract puke

127859 - Bug 29087: Prevent filter_by_for_hold to crash if default holdallowed is not_allowed
164936 - Bug 29087: Add unit tests

Apply? [(y)es, (n)o, (i)nteractive] y
Applying: Bug 29087: Prevent filter_by_for_hold to crash if default holdallowed is not_allowed
Using index info to reconstruct a base tree...
M	Koha/Items.pm
M	t/db_dependent/Koha/Holds.t
M	t/db_dependent/Koha/Items.t
Falling back to patching base and 3-way merge...
Auto-merging t/db_dependent/Koha/Items.t
CONFLICT (content): Merge conflict in t/db_dependent/Koha/Items.t
Auto-merging t/db_dependent/Koha/Holds.t
Auto-merging Koha/Items.pm
error: Failed to merge in the changes.
Patch failed at 0001 Bug 29087: Prevent filter_by_for_hold to crash if default holdallowed is not_allowed
Comment 16 Matt Blenkinsop 2024-05-08 08:23:55 UTC
Created attachment 166354 [details] [review]
Bug 29087: Prevent filter_by_for_hold to crash if default holdallowed is not_allowed
Comment 17 Matt Blenkinsop 2024-05-08 08:23:58 UTC
Created attachment 166355 [details] [review]
Bug 29087: Add unit tests

prove t/db_dependent/Koha/Items.t
Comment 18 David Nind 2024-05-08 08:33:27 UTC
Created attachment 166359 [details] [review]
Bug 29087: Prevent filter_by_for_hold to crash if default holdallowed is not_allowed

Signed-off-by: David Nind <david@davidnind.com>
Comment 19 David Nind 2024-05-08 08:33:30 UTC
Created attachment 166360 [details] [review]
Bug 29087: Add unit tests

prove t/db_dependent/Koha/Items.t

Signed-off-by: David Nind <david@davidnind.com>
Comment 20 David Nind 2024-05-08 08:36:58 UTC
Thanks Matt!

Testing notes (using KTD):

1. Tests pass before after the patches. 

2. Hope that is sufficient for sign off, otherwise please change the status back to needs sign off.
Comment 21 Jonathan Druart 2024-05-08 12:46:39 UTC
There is something wrong with the first patch. My initial patch did not add that many tests.
Comment 22 Matt Blenkinsop 2024-05-10 09:07:47 UTC
Will be a rebase error on my part, having a look now
Comment 23 Matt Blenkinsop 2024-05-10 10:01:58 UTC
Created attachment 166527 [details] [review]
Bug 29087: Prevent filter_by_for_hold to crash if default holdallowed is not_allowed

Signed-off-by: David Nind <david@davidnind.com>
Comment 24 Matt Blenkinsop 2024-05-10 10:02:01 UTC
Created attachment 166528 [details] [review]
Bug 29087: Add unit tests

prove t/db_dependent/Koha/Items.t

Signed-off-by: David Nind <david@davidnind.com>
Comment 25 Matt Blenkinsop 2024-05-10 10:03:03 UTC
Rebased so that the first commit matches the original, except for changes in Items.t which are now covered in the second commit