[Bug 28871] New: Koha::Objects->empty doesn't work correctly in list context
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28871 Bug ID: 28871 Summary: Koha::Objects->empty doesn't work correctly in list context Change sponsored?: --- Product: Koha Version: unspecified Hardware: All OS: All Status: NEW Severity: enhancement Priority: P5 - low Component: Architecture, internals, and plumbing Assignee: koha-bugs@lists.koha-community.org Reporter: tomascohen@gmail.com QA Contact: testopia@bugs.koha-community.org 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... ] -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28871 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Severity|enhancement |normal Assignee|koha-bugs@lists.koha-commun |tomascohen@gmail.com |ity.org | -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28871 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |kyle@bywatersolutions.com, | |martin.renvoize@ptfs-europe | |.com -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28871 --- Comment #1 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Created attachment 123876 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=123876&action=edit Bug 28871: Regression test -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28871 --- Comment #2 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Created attachment 123877 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=123877&action=edit 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). -- You are receiving this mail because: You are watching all bug changes.
SET GLOBAL general_log=1; SET GLOBAL general_log_file='mariadb.log'; \q m$ tail -f /var/lib/mysql/mariadb.log
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28871 --- Comment #3 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Created attachment 123878 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=123878&action=edit 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 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28871 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |Needs Signoff CC| |tomascohen@gmail.com Patch complexity|--- |Trivial patch -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28871 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #123877|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28871 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #123878|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28871 --- Comment #4 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Created attachment 123879 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=123879&action=edit 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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28871 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |24857 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24857 [Bug 24857] Add ability to create "Volumes" for records, and assign items to volumes -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28871 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #123876|0 |1 is obsolete| | --- Comment #5 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 123925 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=123925&action=edit Bug 28871: Regression test Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28871 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #123879|0 |1 is obsolete| | --- Comment #6 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 123926 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=123926&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28871 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off --- Comment #7 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- No obvious regressions found, nice simple patch. Signing off. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28871 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also| |https://bugs.koha-community | |.org/bugzilla3/show_bug.cgi | |?id=28883 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28871 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- QA Contact|testopia@bugs.koha-communit |m.de.rooy@rijksmuseum.nl |y.org | CC| |m.de.rooy@rijksmuseum.nl Status|Signed Off |BLOCKED --- Comment #8 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- QAing -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28871 --- Comment #9 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- 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([]); -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28871 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|BLOCKED |Passed QA -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28871 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #123925|0 |1 is obsolete| | --- Comment #10 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 123987 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=123987&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28871 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #123926|0 |1 is obsolete| | --- Comment #11 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 123988 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=123988&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28871 --- Comment #12 from Tomás Cohen Arazi <tomascohen@gmail.com> --- (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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28871 Jonathan Druart <jonathan.druart+koha@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Passed QA |BLOCKED CC| |jonathan.druart+koha@gmail. | |com --- Comment #13 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- Blocked by the discussion on bug 28883. IF we decide to remove wantarray it does not make sense to have this patch. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28871 --- Comment #14 from Tomás Cohen Arazi <tomascohen@gmail.com> --- This fix is too simple and is correct regarding the current codebase. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28871 --- Comment #15 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- (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? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28871 --- Comment #16 from Tomás Cohen Arazi <tomascohen@gmail.com> --- (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... -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28871 --- Comment #17 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- (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? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28871 --- Comment #18 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- In other words, please show me a better example where it does not work now in list context. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28871 --- Comment #19 from Tomás Cohen Arazi <tomascohen@gmail.com> --- (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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28871 --- Comment #20 from Tomás Cohen Arazi <tomascohen@gmail.com> --- (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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28871 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Severity|normal |major CC| |fridolin.somers@biblibre.co | |m --- Comment #21 from Tomás Cohen Arazi <tomascohen@gmail.com> --- 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... -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28871 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also| |https://bugs.koha-community | |.org/bugzilla3/show_bug.cgi | |?id=29804 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28871 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also| |https://bugs.koha-community | |.org/bugzilla3/show_bug.cgi | |?id=29806 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28871 --- Comment #22 from Tomás Cohen Arazi <tomascohen@gmail.com> --- (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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28871 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|BLOCKED |RESOLVED Resolution|--- |WORKSFORME --- Comment #23 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Let's close this one, and focus on removing the use of wantarray in Koha::Objects instead. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28871 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks|24857 | Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24857 [Bug 24857] Add ability to group items for records -- You are receiving this mail because: You are watching all bug changes.
participants (1)
-
bugzilla-daemon＠bugs.koha-community.org