Bug 28871 - Koha::Objects->empty doesn't work correctly in list context
Summary: Koha::Objects->empty doesn't work correctly in list context
Status: CLOSED WORKSFORME
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low major (vote)
Assignee: Tomás Cohen Arazi
QA Contact: Marcel de Rooy
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2021-08-17 14:27 UTC by Tomás Cohen Arazi
Modified: 2022-12-12 21:24 UTC (History)
6 users (show)

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


Attachments
Bug 28871: Regression test (1.01 KB, patch)
2021-08-17 15:01 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 28871: Make Koha::Objects->empty work on list context (1.25 KB, patch)
2021-08-17 15:01 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 28871: (follow-up) Remove caching (1.23 KB, patch)
2021-08-17 15:01 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 28871: Make Koha::Objects->empty work on list context (954 bytes, patch)
2021-08-17 15:21 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 28871: Regression test (1.08 KB, patch)
2021-08-18 08:50 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 28871: Make Koha::Objects->empty work on list context (1020 bytes, patch)
2021-08-18 08:50 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 28871: Regression test (1.17 KB, patch)
2021-08-20 07:06 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 28871: Make Koha::Objects->empty work on list context (1.09 KB, patch)
2021-08-20 07:06 UTC, Marcel de Rooy
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Tomás Cohen Arazi 2021-08-17 14:27:40 UTC
When in list context, it assigns an array with an interator inside.

To test:
my @patrons = Koha::Patrons->new->empty;
Dumper(@patrons);

=> FAIL: prints [ Koha::Patrons... ]
Comment 1 Tomás Cohen Arazi 2021-08-17 15:01:51 UTC
Created attachment 123876 [details] [review]
Bug 28871: Regression test
Comment 2 Tomás Cohen Arazi 2021-08-17 15:01:55 UTC
Created attachment 123877 [details] [review]
Bug 28871: Make Koha::Objects->empty work on list context

This patch makes Koha::Objects->empty work on list context, by making it
return the $self->search call.

To test:
1. Apply the regression test patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/Objects.t
=> FAIL: Tests fail!
3. Apply this patch
4. Repeat 2
=> SUCCESS: Tests pass!
5. Sign off :-D

Note: this patch has the side effect of making Koha::Objects->new->empty
to perform a search on the DB. I'm not sure how to solve this because
the cache needs to be set after the resultset is generated. It feels
like a cleaner approach (i.e. not using the set_cache method).
Comment 3 Tomás Cohen Arazi 2021-08-17 15:01:59 UTC
Created attachment 123878 [details] [review]
Bug 28871: (follow-up) Remove caching

This patch removes the use of set_cache, as it gets ineffective with
this patchset.

To test:
1. Without any patches applied do:
   $ docker exec -ti koha_db_1 bash
  m$ mysql -ppassword
   > SET GLOBAL general_log=1;
   > SET GLOBAL general_log_file='mariadb.log';
   > \q
  m$ tail -f /var/lib/mysql/mariadb.log
2. On a separate terminal run:
   $ kshell
  k$ prove t/db_dependent/Koha/Objects.t
=> Notice that before the DELETE tests are run, there are some queries
about borrowers and 0 = 1
3. Apply this patches
4. Repeat 2
=> Notice there's an extra query

This is only to highlight the side-effect of this change.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 4 Tomás Cohen Arazi 2021-08-17 15:21:56 UTC
Created attachment 123879 [details] [review]
Bug 28871: Make Koha::Objects->empty work on list context

This patch makes Koha::Objects->empty work on list context, by making it
return the $self->search call.

To test:
1. Apply the regression test patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/Objects.t
=> FAIL: Tests fail!
3. Apply this patch
4. Repeat 2
=> SUCCESS: Tests pass!
5. Sign off :-D
Comment 5 Martin Renvoize 2021-08-18 08:50:17 UTC
Created attachment 123925 [details] [review]
Bug 28871: Regression test

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 6 Martin Renvoize 2021-08-18 08:50:20 UTC
Created attachment 123926 [details] [review]
Bug 28871: Make Koha::Objects->empty work on list context

This patch makes Koha::Objects->empty work on list context, by making it
return the $self->search call.

To test:
1. Apply the regression test patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/Objects.t
=> FAIL: Tests fail!
3. Apply this patch
4. Repeat 2
=> SUCCESS: Tests pass!
5. Sign off :-D

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 7 Martin Renvoize 2021-08-18 08:50:49 UTC
No obvious regressions found, nice simple patch.

Signing off.
Comment 8 Marcel de Rooy 2021-08-20 06:06:48 UTC
QAing
Comment 9 Marcel de Rooy 2021-08-20 07:03:31 UTC
BTW It seems to me that we do not even need to do a search, just create a resultset with new only. But this is merely a theoretical difference. There will be no db read since we set the cache.

#    $self = $self->search(\'0 = 1');
    $self->_resultset()->set_cache([]);
Comment 10 Marcel de Rooy 2021-08-20 07:06:10 UTC
Created attachment 123987 [details] [review]
Bug 28871: Regression test

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 11 Marcel de Rooy 2021-08-20 07:06:13 UTC
Created attachment 123988 [details] [review]
Bug 28871: Make Koha::Objects->empty work on list context

This patch makes Koha::Objects->empty work on list context, by making it
return the $self->search call.

To test:
1. Apply the regression test patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/Objects.t
=> FAIL: Tests fail!
3. Apply this patch
4. Repeat 2
=> SUCCESS: Tests pass!
5. Sign off :-D

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 12 Tomás Cohen Arazi 2021-08-20 10:08:36 UTC
(In reply to Marcel de Rooy from comment #9)
> BTW It seems to me that we do not even need to do a search, just create a
> resultset with new only. But this is merely a theoretical difference. There
> will be no db read since we set the cache.
> 
> #    $self = $self->search(\'0 = 1');
>     $self->_resultset()->set_cache([]);

That ->search call will not trigger a db search. That's correct. It was added so further chaining always returns ah empty set.
Comment 13 Jonathan Druart 2021-10-08 08:50:45 UTC
Blocked by the discussion on bug 28883.
IF we decide to remove wantarray it does not make sense to have this patch.
Comment 14 Tomás Cohen Arazi 2021-11-01 14:54:04 UTC
This fix is too simple and is correct regarding the current codebase.
Comment 15 Marcel de Rooy 2021-11-02 12:08:46 UTC
(In reply to Tomás Cohen Arazi from comment #0)
> To test:
> my @patrons = Koha::Patrons->new->empty;
> Dumper(@patrons);

What is the practical use of doing so?
I would prefer my @patrons;

Was there some intention to use empty with chaining?
Comment 16 Tomás Cohen Arazi 2021-11-02 12:32:12 UTC
(In reply to Marcel de Rooy from comment #15)
> (In reply to Tomás Cohen Arazi from comment #0)
> > To test:
> > my @patrons = Koha::Patrons->new->empty;
> > Dumper(@patrons);
> 
> What is the practical use of doing so?

You should git grep '\->empty'

The existence of ->empty is meant for consistency in results, and also to avoid querying the DB for empty results when we know in advance that we need an empty resultset.

> I would prefer my @patrons;

Right, but my example was too simple. Sometimes ->empty is called implicitly somewhere else, far from the code you're looking at.

> Was there some intention to use empty with chaining?

empty is there so you don't need to do things like:

if ( $result->count > 0 ) {
    $result = $result->filter_by_something_else;
}

you just chain your queries and know it will work...
Comment 17 Marcel de Rooy 2021-11-02 14:44:00 UTC
(In reply to Tomás Cohen Arazi from comment #16)

> You should git grep '\->empty'

Just say it :)
Koha/Acquisition/Order.pm:        return Koha::Holds->new->empty;
=> It returns a I<Koha::Holds> resultset. So that should be no longer the case in LIST context ?
Koha/Biblio.pm:    return Koha::Items->new->empty
*     return Koha::Items->new->empty      unless C4::Context->preference('EasyAnalyticalRecords');
* return Koha::Items->search( { itemnumber => { -in => \@itemnumbers } } );
=> Far fetched, but we might have empty list or empty result set ?
Koha/Item.pm:        return Koha::Libraries->new()->empty if $branchitemrule->{holdallowed} eq 'from_local_hold_group' && !$self->home_branch->validate_hold_sibling( {branchcode => $patron->branchcode} );
Koha/Item.pm:        return Koha::Libraries->new()->empty if $branchitemrule->{holdallowed} eq 'from_home_library' && $self->home_branch->branchcode ne $patron->branchcode;
=> So pickup locations will return () or result set that could be empty ?

Looks a bit inconsistent. Or do I miss something ;)


> you just chain your queries and know it will work...

Chain is scalar context. So no difference between current state and your patch?
Comment 18 Marcel de Rooy 2021-11-02 14:45:40 UTC
In other words, please show me a better example where it does not work now in list context.
Comment 19 Tomás Cohen Arazi 2021-11-03 11:40:35 UTC
(In reply to Marcel de Rooy from comment #18)
> In other words, please show me a better example where it does not work now
> in list context.

This sub:

sub items_fetch {
    return Koha::Items->search(...);
}

will respect list context and do the appropriate thing i.e.

      my @items = items_fetch();

will perform the DB query, and have @items be assigned a list of Koha::Item objects.

As ->empty is expected to be returned sometimes, and (why not) chained with other queries, it needs to behave the same as ->search. This is the ->empty counterpart of what I raised on bug 28883.

Look at t/db_dependendent/Koha/Biblio.t:419

the addition of ->as_list there, is a consequence of the possibility Koha::Libraries->new->empty is returned in some scenarios.

So Koha::Biblio->pickup_locations can honour list context sometimes, and sometimes not. With this patch the programmer can be sure ->pickup_locations has a predictable behavior and avoid coding for exceptional cases.
Comment 20 Tomás Cohen Arazi 2021-11-03 11:43:52 UTC
(In reply to Tomás Cohen Arazi from comment #19)
> So Koha::Biblio->pickup_locations can honour list context sometimes, and
> sometimes not. With this patch the programmer can be sure ->pickup_locations
> has a predictable behavior and avoid coding for exceptional cases.

That said, removing the use of wantarray entirely (with which I agree after the debate, and because of the tradeoffs, not for the behavior), would make this patch unnecessary. But that doesn't look like it is gonna happen soon.
Comment 21 Tomás Cohen Arazi 2022-01-06 10:06:47 UTC
Found in production:

[2021/12/09 10:20:29] [ERROR] PUT /api/v1/holds/499056/pickup_location: unhandled exception (Koha::Exceptions::Object::MethodNotCoveredByTests)<<The method Ko
ha::Libraries->branchcode is not covered by tests!

Please, allow this patches in, until we decide about wantarray. It is weird not to fix an obvious bug because there's a long term refactoring being discussed...
Comment 22 Tomás Cohen Arazi 2022-01-06 11:32:58 UTC
(In reply to Tomás Cohen Arazi from comment #21)
> Found in production:
> 
> [2021/12/09 10:20:29] [ERROR] PUT /api/v1/holds/499056/pickup_location:
> unhandled exception (Koha::Exceptions::Object::MethodNotCoveredByTests)<<The
> method Ko
> ha::Libraries->branchcode is not covered by tests!
> 
> Please, allow this patches in, until we decide about wantarray. It is weird
> not to fix an obvious bug because there's a long term refactoring being
> discussed...

I filed 29804 to avoid delays for the releases.
Comment 23 Tomás Cohen Arazi 2022-01-07 15:10:32 UTC
Let's close this one, and focus on removing the use of wantarray in Koha::Objects instead.