Bug 25296 - Add a way to force an empty Koha::Objects resultset
Summary: Add a way to force an empty Koha::Objects resultset
Status: Pushed to master
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Tomás Cohen Arazi
QA Contact: Jonathan Druart
URL:
Keywords:
Depends on:
Blocks: 25297
  Show dependency treegraph
 
Reported: 2020-04-27 21:31 UTC by Tomás Cohen Arazi
Modified: 2020-05-08 22:42 UTC (History)
6 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:
20.05.00


Attachments
Bug 25296: Unit tests (1.31 KB, patch)
2020-04-27 21:37 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 25296: Add a way to force an empty Koha::Objects resultset (1.38 KB, patch)
2020-04-27 21:37 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 25296: Unit tests (1.37 KB, patch)
2020-04-28 14:01 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 25296: Add a way to force an empty Koha::Objects resultset (1.45 KB, patch)
2020-04-28 14:01 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 25296: Tests for uninstantiated behaviour (1.20 KB, patch)
2020-04-29 12:31 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 25296: Make ->empty work for uninstantiated calls (1.27 KB, patch)
2020-04-29 12:31 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 25296: Unit tests (1.44 KB, patch)
2020-04-29 13:09 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 25296: Add a way to force an empty Koha::Objects resultset (1.52 KB, patch)
2020-04-29 13:09 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 25296: Tests for uninstantiated behaviour (1.27 KB, patch)
2020-04-29 13:09 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 25296: Make ->empty work for uninstantiated calls (1.34 KB, patch)
2020-04-29 13:09 UTC, Jonathan Druart
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 2020-04-27 21:31:37 UTC
In some scenarios, it would be handy for consistency to just return an empty resultset (instead of undef, for example).

Take the following example:

sub current_item_level_holds {
    my ($self) = @_;

    my $items_rs     = $self->_result->aqorders_items;
    my @item_numbers = $items_rs->get_column('itemnumber')->all;

    return unless @item_numbers;

    my $biblio = $self->biblio;
    return unless $biblio;

    return $biblio->current_holds->search(
        {
            itemnumber => {
                -in => \@item_numbers
            }
        }
    );
}

if we wanted to always return a Koha::Holds iterator (as ->current_holds does) we could do:

sub current_item_level_holds {
    my ($self) = @_;

    my $items_rs     = $self->_result->aqorders_items;
    my @item_numbers = $items_rs->get_column('itemnumber')->all;
    my $biblio = $self->biblio;

    unless ( $biblio and @itemnumbers ) {
        return Koha::Holds->new->empty;
    }

    return $biblio->current_holds->search(
        {
            itemnumber => {
                -in => \@item_numbers
            }
        }
    );
}
Comment 1 Tomás Cohen Arazi 2020-04-27 21:37:11 UTC
Created attachment 103811 [details] [review]
Bug 25296: Unit tests

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 2 Tomás Cohen Arazi 2020-04-27 21:37:15 UTC
Created attachment 103812 [details] [review]
Bug 25296: Add a way to force an empty Koha::Objects resultset

This patch adds a new ->empty method to Koha::Objects, that can be used
to make the underlying DBIC resultset empty. This way, we can have
consistency in our method's return values without the need to build
a query that we know in advance that will be empty. No need to hit the
DB at all.

To test:
1. Apply this patches
2. Notice the tests cover what is expected
3. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/Objects.t
=> SUCCESS: Tests pass! Yay!
4. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 3 Frédéric Demians 2020-04-28 04:48:36 UTC
How to sign-off such a patch? And who has the knowledge/authority to do it? Reading the code and the reasoning behind this patch, I could say that it's clear and clean, but is it enough to sign-off?
Comment 4 Frédéric Demians 2020-04-28 05:28:06 UTC
Coming back from bug 25297, I have a question: Is it necessary to explicitly call:

  Koha::Objects->new->empty;

When you assign a result:
 
  my $result = Koha::Holds->new;

Don't you have already $result->count == 0 ?

If this was the case, your code in bug 25297 could become something like that:

    my $result = Koha::Holds->new;
    my $biblio = $self->biblio;
    return $result unless $biblio;

    my $items_rs = $self->_result->aqorders_items;
    my @item_numbers = $items_rs->get_column('itemnumber')->all;
    return $result unless @item_numbers;
Comment 5 Tomás Cohen Arazi 2020-04-28 11:31:18 UTC
(In reply to Frédéric Demians from comment #4)
> Coming back from bug 25297, I have a question: Is it necessary to explicitly
> call:
> 
>   Koha::Objects->new->empty;
> 
> When you assign a result:
>  
>   my $result = Koha::Holds->new;
> 
> Don't you have already $result->count == 0 ?

I wrote the regression tests for the behavior so you can play with other options. The short answer is Koha::Holds->new refers to the whole table rows.
Comment 6 Tomás Cohen Arazi 2020-04-28 12:18:42 UTC
My conversation with one of the DBIx::Class authors:

[17:56:51]  <tcohen> weird question: if I know a query is going to return and empty resultset in advance, is there a way/helper to avoid performing the query, but returning something resultset-ish for consistency?
[17:57:17]  <mst> tcohen: yes
[17:57:23]  <mst> tcohen: $rs->set_cache([]);
[17:57:32]  <tcohen> oh!
[17:57:43]  <mst> tcohen: not weird at all, I designed that feature in somewhere over ten years ago and if it doesn't work for you, please ask again :D
Comment 7 Martin Renvoize 2020-04-28 14:01:36 UTC
Created attachment 103860 [details] [review]
Bug 25296: Unit tests

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 8 Martin Renvoize 2020-04-28 14:01:39 UTC
Created attachment 103861 [details] [review]
Bug 25296: Add a way to force an empty Koha::Objects resultset

This patch adds a new ->empty method to Koha::Objects, that can be used
to make the underlying DBIC resultset empty. This way, we can have
consistency in our method's return values without the need to build
a query that we know in advance that will be empty. No need to hit the
DB at all.

To test:
1. Apply this patches
2. Notice the tests cover what is expected
3. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/Objects.t
=> SUCCESS: Tests pass! Yay!
4. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 9 Martin Renvoize 2020-04-28 14:02:30 UTC
This makes a lot of sense in the context of bug 25297. Signing off as it works as expected and contains tests that pass.

We will need clear coding guidelines for when it should and should not be used.
Comment 10 Jonathan Druart 2020-04-29 09:04:34 UTC
use Koha::Patrons;
say Koha::Patrons->new->empty->count;
say Koha::Patrons->empty->count;

0
53


How could we avoid that?
Comment 11 Tomás Cohen Arazi 2020-04-29 12:13:28 UTC
(In reply to Jonathan Druart from comment #10)
> use Koha::Patrons;
> say Koha::Patrons->new->empty->count;
> say Koha::Patrons->empty->count;
> 
> 0
> 53
> 
> 
> How could we avoid that?

Do we need to?
Comment 12 Tomás Cohen Arazi 2020-04-29 12:31:40 UTC
Created attachment 103928 [details] [review]
Bug 25296: Tests for uninstantiated behaviour

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 13 Tomás Cohen Arazi 2020-04-29 12:31:44 UTC
Created attachment 103929 [details] [review]
Bug 25296: Make ->empty work for uninstantiated calls

In OO we would usually instantiate the class and then interact with it
like in:
    my $rs = Koha::Patrons->new;
    $rs->empty;

Koha's practice is to call things like Koha::Patrons->search without
instantiating the Koha::Patrons class. To keep consistency, this patch
instantiates the resultset object on behalf of the caller if required.

To test:
1. Apply the tests patch
2. Run:
   $ kshell
  $k prove t/db_dependent/Koha/Objects.t
=> FAIL: Tests fail because it is expected to have the class
instantiated
3. Apply this patch
4. Repeat 2.
=> SUCCESS: Tests pass, instantiation happens implicitly.
5. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 14 Tomás Cohen Arazi 2020-04-29 12:32:12 UTC
(In reply to Jonathan Druart from comment #10)
> use Koha::Patrons;
> say Koha::Patrons->new->empty->count;
> say Koha::Patrons->empty->count;
> 
> 0
> 53
> 
> 
> How could we avoid that?

Solved, you can now QA on this, please.
Comment 15 Jonathan Druart 2020-04-29 13:09:33 UTC
Created attachment 103934 [details] [review]
Bug 25296: Unit tests

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 16 Jonathan Druart 2020-04-29 13:09:36 UTC
Created attachment 103935 [details] [review]
Bug 25296: Add a way to force an empty Koha::Objects resultset

This patch adds a new ->empty method to Koha::Objects, that can be used
to make the underlying DBIC resultset empty. This way, we can have
consistency in our method's return values without the need to build
a query that we know in advance that will be empty. No need to hit the
DB at all.

To test:
1. Apply this patches
2. Notice the tests cover what is expected
3. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/Objects.t
=> SUCCESS: Tests pass! Yay!
4. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 17 Jonathan Druart 2020-04-29 13:09:40 UTC
Created attachment 103936 [details] [review]
Bug 25296: Tests for uninstantiated behaviour

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 18 Jonathan Druart 2020-04-29 13:09:43 UTC
Created attachment 103937 [details] [review]
Bug 25296: Make ->empty work for uninstantiated calls

In OO we would usually instantiate the class and then interact with it
like in:
    my $rs = Koha::Patrons->new;
    $rs->empty;

Koha's practice is to call things like Koha::Patrons->search without
instantiating the Koha::Patrons class. To keep consistency, this patch
instantiates the resultset object on behalf of the caller if required.

To test:
1. Apply the tests patch
2. Run:
   $ kshell
  $k prove t/db_dependent/Koha/Objects.t
=> FAIL: Tests fail because it is expected to have the class
instantiated
3. Apply this patch
4. Repeat 2.
=> SUCCESS: Tests pass, instantiation happens implicitly.
5. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 19 Jonathan Druart 2020-04-29 13:10:35 UTC
I am wondering if we should not force ->empty to be called only as a class method.

my $patrons = Koha::Patrons->search;
$patrons->empty; # Does not really make sense to me.
Comment 20 Martin Renvoize 2020-04-29 18:06:29 UTC
Nice work everyone!

Pushed to master for 20.05
Comment 21 Tomás Cohen Arazi 2020-04-29 19:30:48 UTC
(In reply to Jonathan Druart from comment #19)
> I am wondering if we should not force ->empty to be called only as a class
> method.
> 
> my $patrons = Koha::Patrons->search;
> $patrons->empty; # Does not really make sense to me.

It would be:

my $patrons_rs = Koha::Patrons->new->empty;

but I get the point.

In the context of bug 25297, it really looks like an unnecessary extra line.

I still feel uncomfortable with the magic instantiation that happens in ->search (and now in ->empty).

my $rs = Koha::Things->new;

$rs->search( $condition_1 ) if $condition_1;
$rs->search( $condition_2 ) if $condition_2;

while ( my $res = $rs->next ) {...}

it is easier to understand (conceptually, for me) than

my $rs = Koha::Things->search( $condition_1 );
$rs->search( $condition_2 ) if $condition_2;

while ( my $res = $rs->next ) {...}

even if you save one line of code (or a ->new on the same line even)...

In this case I opted to not be rigid on my POV and implemented both behaviours,  but I think it is worth discussing in a broader place, if you think some guideline needs to be generally adopted.
Comment 22 Joy Nelson 2020-05-08 22:42:20 UTC
not backported to 19.11