Bug 28561 - Order_by triggers a DBIx warning Unable to properly collapse has_many results
Summary: Order_by triggers a DBIx warning Unable to properly collapse has_many results
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Marcel de Rooy
QA Contact: Nick Clemens
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2021-06-14 11:55 UTC by Marcel de Rooy
Modified: 2022-06-06 20:25 UTC (History)
6 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:
21.11.00,21.05.02,20.11.09


Attachments
Bug 28561: Resolve DBIx's Unable to collapse results-warning (2.04 KB, patch)
2021-06-14 13:45 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 28561: Fix noisy warning about $direction too (1.55 KB, patch)
2021-06-14 13:45 UTC, Marcel de Rooy
Details | Diff | Splinter Review
[ALTERNATIVE PATCH] Bug 28561: Remove DBIC warning in opac-shelves (993 bytes, patch)
2021-06-18 10:36 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 28561: Remove DBIC warning in opac-shelves (1.06 KB, patch)
2021-06-21 10:00 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 28561: Fix noisy warning about $direction too (1.57 KB, patch)
2021-07-09 13:19 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 28561: Remove DBIC warning in opac-shelves (1.08 KB, patch)
2021-07-09 13:19 UTC, Nick Clemens
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Marcel de Rooy 2021-06-14 11:55:30 UTC
Make a list, few biblios with multiple items.
Check the plack-opac-error.log

[2021/06/14 11:51:45] [WARN] Use of uninitialized value $direction in string ne at /usr/share/koha/opac/opac-shelves.pl line 265.
[2021/06/14 11:51:45] [WARN] Use of uninitialized value $direction in string ne at /usr/share/koha/opac/opac-shelves.pl line 265.
[2021/06/14 11:51:45] [WARN] DBIx::Class::ResultSet::_construct_results(): Unable to properly collapse has_many results in iterator mode due to order criteria - performed an eager cursor slurp underneath. Consider using ->all() instead at /usr/share/koha/Koha/Objects.pm line 335

Koha/Objects Line 335 says:
    my $result = $self->_resultset()->next();

The warning has to do with the sorting criteria. Disabling the order_by clause removes the warning.
Comment 1 Marcel de Rooy 2021-06-14 11:57:03 UTC
This query in opac/opac-shelves.pl contains the crux:

            my $contents = $shelf->get_contents->search(
                {},
                {
                    prefetch => [ { 'biblionumber' => { 'biblioitems' => 'items' } } ],
                    page     => $page,
                    rows     => $rows,
                    order_by => { "-$direction" => $order_by },
                }
            );
Comment 2 Marcel de Rooy 2021-06-14 12:35:59 UTC
The code in opac-shelves is far from optimal. The prefetch in the query selects biblio, biblioitems and items. But in the loop starting at L308 we are calling GetBiblioData, GetMarcBiblio, Biblios->find, GetItemsLocationInfo, $biblio->items. Horrible ;)

DBIx says about prefetching and collapse the following:
If an "order_by" is already declared, and orders the resultset in a way that makes collapsing as described above impossible (e.g. ORDER BY has_many_rel.column or ORDER BY RANDOM()), DBIC will automatically switch to "eager" mode and slurp the entire resultset before constructing the first object returned by "next".

So if we are 'sorting' by itemcallnumber in an "has many" table, we will trigger the warning. In that case we can remove the sort anyway.

This is a bit lazy 'solution' but this report was not about refactoring opac-shelves.
Comment 3 Marcel de Rooy 2021-06-14 13:45:50 UTC
Created attachment 121912 [details] [review]
Bug 28561: Resolve DBIx's Unable to collapse results-warning

[WARN] DBIx::Class::ResultSet::_construct_results(): Unable to properly collapse has_many results in iterator mode due to order criteria - performed an eager cursor slurp underneath. Consider using ->all() instead at /usr/share/koha/Koha/Objects.pm line 335

DBIx tells:
If an "order_by" is already declared, and orders the resultset in a way that makes collapsing as described above impossible (e.g. ORDER BY has_many_rel.column or ORDER BY RANDOM()), DBIC will automatically switch to "eager" mode and slurp the entire resultset before constructing the first object returned by "next".

The query in opac-shelves here prefetches biblio, biblioitems and items.
May order by itemcallnumber (so that is a has_many column). Note that
ordering biblio records (having multiple items) by itemcallnumber
is formally impossible.

This patch pragmatically removes the order_by clause, but I am open
for a better solution ;)

Test plan:
Pick a list with biblios having multiple items.
Show the list in OPAC. Change the sortfield.
Check your plack-opac-error.log.
Comment 4 Marcel de Rooy 2021-06-14 13:45:53 UTC
Created attachment 121913 [details] [review]
Bug 28561: Fix noisy warning about $direction too

Use of uninitialized value $direction in string ne at /usr/share/koha/opac/opac-shelves.pl line 265.

Bonus:
Use of uninitialized value $sortfield in string eq at /usr/share/koha/opac/opac-shelves.pl line 264.

Test plan:
While testing patch 1, check the logs for these warnings with and
without this patch.
Comment 5 Marcel de Rooy 2021-06-14 13:58:41 UTC
NOTE:
Although this patch significantly reduces the number of warnings on the collapse. I still had some when I sorted on title too. It seems that it does not occur on the other fields as author etc. Not sure why. Caching?

[2021/06/14 13:06:59] [WARN] L272title-title at /usr/share/koha/opac/opac-shelves.pl line 272.
[2021/06/14 13:06:59] [WARN] DBIx::Class::ResultSet::_construct_results(): Unable to properly collapse has_many results in iterator mode due to order criteria - performed an eager cursor slurp underneath. Consider using ->all() instead at /usr/share/koha/Koha/Objects.pm line 335
Comment 6 Marcel de Rooy 2021-06-15 11:06:27 UTC
Still hesitant if we should proceed like that.. What do you think?
Comment 7 Jonathan Druart 2021-06-18 10:36:29 UTC
Created attachment 122119 [details] [review]
[ALTERNATIVE PATCH] Bug 28561: Remove DBIC warning in opac-shelves
Comment 8 Jonathan Druart 2021-06-18 10:37:22 UTC
We don't actually need to prefetch as we are only using the biblionumber.
Using join and distinct seems to remove the warning and produce the same results.
Comment 9 Marcel de Rooy 2021-06-21 10:00:46 UTC
Created attachment 122215 [details] [review]
Bug 28561: Remove DBIC warning in opac-shelves

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 10 Marcel de Rooy 2021-06-21 10:03:32 UTC
(In reply to Jonathan Druart from comment #8)
> We don't actually need to prefetch as we are only using the biblionumber.
> Using join and distinct seems to remove the warning and produce the same
> results.

So simple..
Comment 11 Nick Clemens 2021-07-09 13:19:15 UTC
Created attachment 122743 [details] [review]
Bug 28561: Fix noisy warning about $direction too

Use of uninitialized value $direction in string ne at /usr/share/koha/opac/opac-shelves.pl line 265.

Bonus:
Use of uninitialized value $sortfield in string eq at /usr/share/koha/opac/opac-shelves.pl line 264.

Test plan:
While testing patch 1, check the logs for these warnings with and
without this patch.

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 12 Nick Clemens 2021-07-09 13:19:19 UTC
Created attachment 122744 [details] [review]
Bug 28561: Remove DBIC warning in opac-shelves

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

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 13 Jonathan Druart 2021-07-12 13:49:49 UTC
Pushed to master for 21.11, thanks to everybody involved!
Comment 14 Kyle M Hall 2021-07-16 12:16:44 UTC
Pushed to 21.05.x for 21.05.02
Comment 15 Fridolin Somers 2021-08-02 21:13:49 UTC
Pushed to 20.11.x for 20.11.09
Comment 16 Victor Grousset/tuxayo 2021-08-16 18:28:57 UTC
Not backported to oldoldstable (20.05.x). Feel free to ask if it's needed.