| Summary: | Koha::Object->to_api should handle undef counts | ||
|---|---|---|---|
| Product: | Koha | Reporter: | Tomás Cohen Arazi (tcohen) <tomascohen> |
| Component: | Architecture, internals, and plumbing | Assignee: | Tomás Cohen Arazi (tcohen) <tomascohen> |
| Status: | RESOLVED DUPLICATE | QA Contact: | Testopia <testopia> |
| Severity: | normal | ||
| Priority: | P5 - low | CC: | jonathan.druart, tomascohen |
| Version: | Main | ||
| Hardware: | All | ||
| OS: | All | ||
| Change sponsored?: | --- | Patch complexity: | Trivial patch |
| Documentation contact: | Documentation submission: | ||
| Text to go in the release notes: | Version(s) released in: | ||
| Circulation function: | |||
| Bug Depends on: | 24528 | ||
| Bug Blocks: | |||
| Attachments: |
Bug 24965: Regression tests
Bug 24965: Make Koha::Object->to_api handle undef counts Bug 24965: Regression tests Bug 24965: Make Koha::Object->to_api handle undef counts |
||
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> 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> 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> 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> 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? Waiting for an answer. (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 |
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.