Bug 24965 - Koha::Object->to_api should handle undef counts
Summary: Koha::Object->to_api should handle undef counts
Status: RESOLVED DUPLICATE of bug 25297
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: Main
Hardware: All All
: P5 - low normal (vote)
Assignee: Tomás Cohen Arazi
QA Contact: Testopia
URL:
Keywords:
Depends on: 24528
Blocks:
  Show dependency treegraph
 
Reported: 2020-03-24 12:21 UTC by Tomás Cohen Arazi
Modified: 2020-04-27 22:05 UTC (History)
2 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 24965: Regression tests (1.53 KB, patch)
2020-03-24 13:04 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 24965: Make Koha::Object->to_api handle undef counts (1.86 KB, patch)
2020-03-24 13:04 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 24965: Regression tests (1.58 KB, patch)
2020-03-31 19:29 UTC, David Nind
Details | Diff | Splinter Review
Bug 24965: Make Koha::Object->to_api handle undef counts (1.90 KB, patch)
2020-03-31 19:29 UTC, David Nind
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-03-24 12:21:05 UTC
If to_api is requested to embed the count on a relation that might be undef (for example, if the relation is not just a FK on another table but linking table:

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
            }
        }
    );
}

it is possible to get exceptions for calling ->count on an undefined object. We should catch this situation and return zero.
Comment 1 Tomás Cohen Arazi 2020-03-24 13:04:27 UTC
Created attachment 101560 [details] [review]
Bug 24965: Regression tests

To test:
1. Apply this patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/Object.t
=> FAIL: Tests fail with "Can't call method "count" on an undefined
value..."

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 2 Tomás Cohen Arazi 2020-03-24 13:04:30 UTC
Created attachment 101561 [details] [review]
Bug 24965: Make Koha::Object->to_api handle undef counts

In some cases where we write a class method that is not directly tied to
a DBIC relationship (maybe it should) we are following the following
pattern:

   my $related_object = Koha::RelatedObjects->find($params,$attributes);
   return unless $related_object;
   return $related_object->some_iterator_on_other_class;

While this is not highly consistent (design-wise, because it would be
great if we could return an empty iterator instead of undef) we use this
broadly and consistently.

The to_api method +count handling is not failsafe to those undef cases.
And we should make it return 0 instead of trying to get ->count on
undefined objects.

This patch makes to_api fallback to 0 when it is requested to embed a
count.

To test:
1. Apply the regression test patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/Object.t
=> FAIL: Test fails with an error about ->count on undefined variable.
3. Apply this patch
4. Repeat 2
=> SUCCESS: The patch adds the simple fallback to 0 on counts
5. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 3 David Nind 2020-03-31 19:29:36 UTC
Created attachment 102154 [details] [review]
Bug 24965: Regression tests

To test:
1. Apply this patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/Object.t
=> FAIL: Tests fail with "Can't call method "count" on an undefined
value..."

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

Signed-off-by: David Nind <david@davidnind.com>
Comment 4 David Nind 2020-03-31 19:29:39 UTC
Created attachment 102155 [details] [review]
Bug 24965: Make Koha::Object->to_api handle undef counts

In some cases where we write a class method that is not directly tied to
a DBIC relationship (maybe it should) we are following the following
pattern:

   my $related_object = Koha::RelatedObjects->find($params,$attributes);
   return unless $related_object;
   return $related_object->some_iterator_on_other_class;

While this is not highly consistent (design-wise, because it would be
great if we could return an empty iterator instead of undef) we use this
broadly and consistently.

The to_api method +count handling is not failsafe to those undef cases.
And we should make it return 0 instead of trying to get ->count on
undefined objects.

This patch makes to_api fallback to 0 when it is requested to embed a
count.

To test:
1. Apply the regression test patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/Object.t
=> FAIL: Test fails with an error about ->count on undefined variable.
3. Apply this patch
4. Repeat 2
=> SUCCESS: The patch adds the simple fallback to 0 on counts
5. Sign off :-D

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

Signed-off-by: David Nind <david@davidnind.com>
Comment 5 Jonathan Druart 2020-04-08 08:04:56 UTC
What is the problematic situation in current_item_level_holds?

no @item_numbers, or no $biblio?

The 'return unless @item_numbers' can be removed, you will get an empty iterator in that case.


Do you have other examples?
Comment 6 Jonathan Druart 2020-04-15 10:22:49 UTC
Waiting for an answer.
Comment 7 Tomás Cohen Arazi 2020-04-27 20:58:27 UTC
(In reply to Jonathan Druart from comment #6)
> Waiting for an answer.

In my opinion it feels dirty. We should have a way to effectively build an 'empty iterator' instead of actually performing a search to the DB, knowing it needs to return 0 results.

Damn you make me work extra all the time
Comment 8 Tomás Cohen Arazi 2020-04-27 22:02:31 UTC

*** This bug has been marked as a duplicate of bug 25297 ***