Bug 36797 - Record with 1000+ holds and unique priorities causes a 500 error
Summary: Record with 1000+ holds and unique priorities causes a 500 error
Status: Pushed to stable
Alias: None
Product: Koha
Classification: Unclassified
Component: Hold requests (show other bugs)
Version: Main
Hardware: All All
: P5 - low normal (vote)
Assignee: Lucas Gass
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2024-05-06 22:38 UTC by Lucas Gass
Modified: 2024-05-30 19:14 UTC (History)
3 users (show)

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


Attachments
Bug 36797: Change WHILE loop into FOREACH (3.07 KB, patch)
2024-05-07 16:06 UTC, Lucas Gass
Details | Diff | Splinter Review
Bug 36797: Change WHILE loop into FOREACH (3.14 KB, patch)
2024-05-07 17:57 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 36797: Change WHILE loop into FOREACH (3.19 KB, patch)
2024-05-10 19:05 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Lucas Gass 2024-05-06 22:38:21 UTC
To recreate:
1. Find a record that has 1000 non-waiting holds with different priorities. 
2. Try loading the reserve page for that record ( /reserve/request.pl?biblionumber=X )
4. Error: Template process failed: undef error - WHILE loop terminated (> 1000 iterations)


This looks to be because of TT 'While' loop in holds_table.inc:
    [% BLOCK priority_dropdown %]
        [%- SET loop_priority = 1 -%] 
        [%- WHILE loop_priority <= last_priority -%] 
            [%- IF this_priority == loop_priority -%] 
                <option value="[% loop_priority | html %]" selected="selected">[% loop_priority | html %]</option>
            [%- ELSE -%] 
                <option value="[% loop_priority | html %]">[% loop_priority | html %]</option>
            [%- END -%] 
            [%- loop_priority = loop_priority + 1- %]
        [%- END -%] 
    [% END %]


TT documentation states that it will terminate a WHILE loop larger than 1000 to prevent runaways:

https://template-toolkit.org/docs/manual/Directives.html#section_WHILE
Comment 1 Lucas Gass 2024-05-06 22:51:19 UTC
If you're using k-t-d to test you can use this script:

https://github.com/ricofreak/handy_koha_scripts/blob/main/make_holds.pl

After you run it and have at least 1000 holds I then did the following inside k-t-d:

sudo koha-mysql kohadev 
update reserves set priority = reserve_id;
Comment 2 Lucas Gass 2024-05-07 16:02:48 UTC
Maybe this can just be a FOREACH loop? It won't die after 1000 iterations.
Comment 3 Lucas Gass 2024-05-07 16:06:04 UTC
Created attachment 166302 [details] [review]
Bug 36797: Change WHILE loop into FOREACH

To test:
1. Find a record that has 1000 non-waiting holds with different priorities.
2. Try loading the reserve page for that record ( /reserve/request.pl?biblionumber=X )
3. Error: Template process failed: undef error - WHILE loop terminated (> 1000 iterations)
4. APPLY PATCH
5. Try again, this time the page should load.
6. Try testing on a record with less than 1000 holds, making sure the priorities are still set right.
7. Make sure you can change your priorities and everything works right.
Comment 4 Martin Renvoize 2024-05-07 17:57:37 UTC
Created attachment 166327 [details] [review]
Bug 36797: Change WHILE loop into FOREACH

To test:
1. Find a record that has 1000 non-waiting holds with different priorities.
2. Try loading the reserve page for that record ( /reserve/request.pl?biblionumber=X )
3. Error: Template process failed: undef error - WHILE loop terminated (> 1000 iterations)
4. APPLY PATCH
5. Try again, this time the page should load.
6. Try testing on a record with less than 1000 holds, making sure the priorities are still set right.
7. Make sure you can change your priorities and everything works right.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 5 Martin Renvoize 2024-05-07 17:58:03 UTC
Simple patch converting WHILE to FOREACH.. works as expected.

Passing QA
Comment 6 Martin Renvoize 2024-05-07 17:58:18 UTC
oof.. Signing off I mean ;)
Comment 7 Nick Clemens (kidclamp) 2024-05-10 19:05:59 UTC
Created attachment 166607 [details] [review]
Bug 36797: Change WHILE loop into FOREACH

To test:
1. Find a record that has 1000 non-waiting holds with different priorities.
2. Try loading the reserve page for that record ( /reserve/request.pl?biblionumber=X )
3. Error: Template process failed: undef error - WHILE loop terminated (> 1000 iterations)
4. APPLY PATCH
5. Try again, this time the page should load.
6. Try testing on a record with less than 1000 holds, making sure the priorities are still set right.
7. Make sure you can change your priorities and everything works right.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 8 Katrin Fischer 2024-05-13 08:42:28 UTC
Pushed for 24.05!

Well done everyone, thank you!
Comment 9 Fridolin Somers 2024-05-29 09:25:17 UTC
Pushed to 23.11.x for 23.11.06
Comment 10 Lucas Gass 2024-05-30 19:14:01 UTC
This is going to require some rebasing for 23.05.x, I'll come back to do so ASAP.