Bug 28561

Summary: Order_by triggers a DBIx warning Unable to properly collapse has_many results
Product: Koha Reporter: Marcel de Rooy <m.de.rooy>
Component: Architecture, internals, and plumbingAssignee: Marcel de Rooy <m.de.rooy>
Status: CLOSED FIXED QA Contact: Nick Clemens <nick>
Severity: normal    
Priority: P5 - low CC: fridolin.somers, jonathan.druart, kyle, m.de.rooy, nick, victor
Version: Main   
Hardware: All   
OS: All   
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
Bug 28561: Fix noisy warning about $direction too
[ALTERNATIVE PATCH] Bug 28561: Remove DBIC warning in opac-shelves
Bug 28561: Remove DBIC warning in opac-shelves
Bug 28561: Fix noisy warning about $direction too
Bug 28561: Remove DBIC warning in opac-shelves

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.