[Bug 27034] New: $c->objects->search shouldn't use path parameters
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27034 Bug ID: 27034 Summary: $c->objects->search shouldn't use path parameters Change sponsored?: --- Product: Koha Version: master Hardware: All OS: All Status: NEW Severity: normal Priority: P5 - low Component: REST API Assignee: koha-bugs@lists.koha-community.org Reporter: tomascohen@gmail.com The objects.search Mojolicious helper incorrectly adds path parameters to the query for filtering. This works, trivially for plat routes like: GET /patrons/:patron_id or for routes that really have links like (fictional example): GET /patrons/:patron_id/holds In the latter, we already know there's a link borrowers<->reserves so searching holds and filtering by the borrowernumber will work. About this there are two things to consider: - That's not how we are coding, we are actually picking the patron_id manually and using object methods to access the resultset: my $patron = Koha::Patrons->find( $c->validation->param('patron_id') ); ... my $holds_rs = $patron->holds; my $holds = $c->objects->search({ resultset => $holds_rs }); - If we have more tricky examples like: GET /holds/:hold_id/pickup_locations it is obvious that hold_id doesn't make any sense for Koha::Libraries, so filtering on it actually causes an exception. One option is to manually remove the problematic path parameters in the controller: my $hold_id = $c->validation->param('hold_id'); my $hold = Koha::Holds->find( $hold_id ); delete $c->validation->output->{'hold_id'}; my $pickup_locations_rs = $hold->pickup_locations; my $pickup_locations = $c->objects->search( $pickup_locations_rs ); My feeling is that we should be following the current pattern, and not really use the path parameters in objects.search. If we later need some handling in the generic objects->search helper, we can reintroduce it, but we need a real use case, which cannot be found in the codebase yet. It all points towards not needing them there, and just getting in the middle with no gain (just the opposite). -- 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=27034 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Assignee|koha-bugs@lists.koha-commun |tomascohen@gmail.com |ity.org | Status|NEW |In Discussion CC| |agustinmoyano@theke.io, | |jonathan.druart@bugs.koha-c | |ommunity.org, | |julian.maurice@biblibre.com | |, | |kyle@bywatersolutions.com, | |martin.renvoize@ptfs-europe | |.com, | |nick@bywatersolutions.com -- 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=27034 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also| |https://bugs.koha-community | |.org/bugzilla3/show_bug.cgi | |?id=27015 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27034 --- Comment #1 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- That took a few re-reads, but I think I generally agree with you.. lets clean it up for now and remove it. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27034 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Patch complexity|--- |Trivial patch Status|In Discussion |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27034 --- Comment #2 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Created attachment 114777 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=114777&action=edit Bug 27034: Do not use path parameters on building the query This patch simply removes the use of path parameters on building the query in $c->objects->search. The sample usage in the original tests considers a specific use case in which the 'nested' resultset has the path param in its attributes: GET /patrons/:patron_id/holds In this case, $c->objects->search would be passed a Koha::Holds resultset into which it would build a query. As the patron_id param can be mapped into a Koha::Hold attribute, this works. We don't actually use this approach in code. What we do is searching for the patron, and properly return a 404 if the patron doesn't exist [1]. If it exists, we actually call $patron->holds to get the related resultset, so there's no gain in having the path param on the query afterwards. That said, if we wanted to build: GET /holds/:hold_id/pickup_locations as the latter is a calculated value, from a resultset that doesn't actually include a (mapped to some attribute) hold_id attribute, there's no way to use it if it will automatically add the hold_id to the pickup_locations resultset. It will give errors about hold_id not being an attribute of Koha::Library (Branches.pm actually). So this patch removes the automatic use of path parameters in $c->objects->search. We can extract them in the controller and add them to the passed resultset if required. But this patch removes a constraint we have for building routes controllers right now. [1] If we didn't return the 404, and we just passed a fresh Koha::Holds resultset to be feeded with the patron_id, we would always return an empty array in cases where the patron doesn't exist. This would be misleading, but I'm open to discuss it. Design-wise. 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=27034 --- Comment #3 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Patch "Bug 27015: Make pickup locations searchable" is going to add a FIXME about this. This patch must remove it then. +++ b/Koha/REST/V1/Holds.pm @@ -445,6 +445,8 @@ sub pickup_locations { my $c = shift->openapi->valid_input or return; my $hold_id = $c->validation->param('hold_id'); + # FIXME: We should really skip the path params in $c->objects->search + delete $c->validation->output->{hold_id}; -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27034 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Depends on| |27015 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27015 [Bug 27015] Add filtering options to the pickup_locations routes -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27034 --- Comment #4 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Created attachment 114806 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=114806&action=edit Bug 27034: (follow-up) Remove unneeded params tweak As $c->objects->search doesn't consider path params anymore, there's no need to manually tweak the query parameters. To test: 1. Run: $ kshell k$ prove t/db_dependent/api/v1/holds.t => SUCCESS: Tests pass! 2. Apply this patch 3. Repeat 1 => SUCCESS: Tests still pass! 4. 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=27034 --- Comment #5 from Tomás Cohen Arazi <tomascohen@gmail.com> --- (In reply to Jonathan Druart from comment #3)
Patch "Bug 27015: Make pickup locations searchable" is going to add a FIXME about this.
This patch must remove it then.
+++ b/Koha/REST/V1/Holds.pm @@ -445,6 +445,8 @@ sub pickup_locations { my $c = shift->openapi->valid_input or return;
my $hold_id = $c->validation->param('hold_id'); + # FIXME: We should really skip the path params in $c->objects->search + delete $c->validation->output->{hold_id};
Thanks for taking the time to review at this point! -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27034 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |27352 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27352 [Bug 27352] Add GET /biblios/:biblio_id/items -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27034 David Nind <david@davidnind.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |david@davidnind.com --- Comment #6 from David Nind <david@davidnind.com> --- Is running these tests and getting no fails sufficient for sign off? - prove t/db_dependent/Koha/REST/Plugin/Objects.t - prove t/db_dependent/api/v1/holds.t Both tests passed before and after the bug was applied. If so, I can sign off. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27034 --- Comment #7 from Tomás Cohen Arazi <tomascohen@gmail.com> --- (In reply to David Nind from comment #6)
Is running these tests and getting no fails sufficient for sign off?
- prove t/db_dependent/Koha/REST/Plugin/Objects.t - prove t/db_dependent/api/v1/holds.t
Both tests passed before and after the bug was applied.
If so, I can sign off.
That's exactly the test plan, so.. yes! -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27034 --- Comment #8 from David Nind <david@davidnind.com> --- (In reply to Tomás Cohen Arazi from comment #7)
That's exactly the test plan, so.. yes!
Cool bananas! Sign off on its way... -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27034 David Nind <david@davidnind.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27034 David Nind <david@davidnind.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #114777|0 |1 is obsolete| | --- Comment #9 from David Nind <david@davidnind.com> --- Created attachment 115796 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=115796&action=edit Bug 27034: Do not use path parameters on building the query This patch simply removes the use of path parameters on building the query in $c->objects->search. The sample usage in the original tests considers a specific use case in which the 'nested' resultset has the path param in its attributes: GET /patrons/:patron_id/holds In this case, $c->objects->search would be passed a Koha::Holds resultset into which it would build a query. As the patron_id param can be mapped into a Koha::Hold attribute, this works. We don't actually use this approach in code. What we do is searching for the patron, and properly return a 404 if the patron doesn't exist [1]. If it exists, we actually call $patron->holds to get the related resultset, so there's no gain in having the path param on the query afterwards. That said, if we wanted to build: GET /holds/:hold_id/pickup_locations as the latter is a calculated value, from a resultset that doesn't actually include a (mapped to some attribute) hold_id attribute, there's no way to use it if it will automatically add the hold_id to the pickup_locations resultset. It will give errors about hold_id not being an attribute of Koha::Library (Branches.pm actually). So this patch removes the automatic use of path parameters in $c->objects->search. We can extract them in the controller and add them to the passed resultset if required. But this patch removes a constraint we have for building routes controllers right now. [1] If we didn't return the 404, and we just passed a fresh Koha::Holds resultset to be feeded with the patron_id, we would always return an empty array in cases where the patron doesn't exist. This would be misleading, but I'm open to discuss it. Design-wise. Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: David Nind <david@davidnind.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27034 David Nind <david@davidnind.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #114806|0 |1 is obsolete| | --- Comment #10 from David Nind <david@davidnind.com> --- Created attachment 115797 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=115797&action=edit Bug 27034: (follow-up) Remove unneeded params tweak As $c->objects->search doesn't consider path params anymore, there's no need to manually tweak the query parameters. To test: 1. Run: $ kshell k$ prove t/db_dependent/api/v1/holds.t => SUCCESS: Tests pass! 2. Apply this patch 3. Repeat 1 => SUCCESS: Tests still pass! 4. Sign off :-D Signed-off-by: David Nind <david@davidnind.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27034 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #115796|0 |1 is obsolete| | --- Comment #11 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 115805 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=115805&action=edit Bug 27034: Do not use path parameters on building the query This patch simply removes the use of path parameters on building the query in $c->objects->search. The sample usage in the original tests considers a specific use case in which the 'nested' resultset has the path param in its attributes: GET /patrons/:patron_id/holds In this case, $c->objects->search would be passed a Koha::Holds resultset into which it would build a query. As the patron_id param can be mapped into a Koha::Hold attribute, this works. We don't actually use this approach in code. What we do is searching for the patron, and properly return a 404 if the patron doesn't exist [1]. If it exists, we actually call $patron->holds to get the related resultset, so there's no gain in having the path param on the query afterwards. That said, if we wanted to build: GET /holds/:hold_id/pickup_locations as the latter is a calculated value, from a resultset that doesn't actually include a (mapped to some attribute) hold_id attribute, there's no way to use it if it will automatically add the hold_id to the pickup_locations resultset. It will give errors about hold_id not being an attribute of Koha::Library (Branches.pm actually). So this patch removes the automatic use of path parameters in $c->objects->search. We can extract them in the controller and add them to the passed resultset if required. But this patch removes a constraint we have for building routes controllers right now. [1] If we didn't return the 404, and we just passed a fresh Koha::Holds resultset to be feeded with the patron_id, we would always return an empty array in cases where the patron doesn't exist. This would be misleading, but I'm open to discuss it. Design-wise. Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: David Nind <david@davidnind.com> 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=27034 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #115797|0 |1 is obsolete| | --- Comment #12 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 115806 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=115806&action=edit Bug 27034: (follow-up) Remove unneeded params tweak As $c->objects->search doesn't consider path params anymore, there's no need to manually tweak the query parameters. To test: 1. Run: $ kshell k$ prove t/db_dependent/api/v1/holds.t => SUCCESS: Tests pass! 2. Apply this patch 3. Repeat 1 => SUCCESS: Tests still pass! 4. Sign off :-D Signed-off-by: David Nind <david@davidnind.com> 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=27034 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Passed QA --- Comment #13 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Changes all make sense to me and the code works well. Qa scripts are happy and unit tests are updated. Passing QA -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27034 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Passed QA |Pushed to master Version(s)| |21.05.00 released in| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27034 --- Comment #14 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Pushed to master for 21.05, thanks to everybody involved! -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27034 Fridolin Somers <fridolin.somers@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |fridolin.somers@biblibre.co | |m --- Comment #15 from Fridolin Somers <fridolin.somers@biblibre.com> --- Depends on Bug 27015 not in 20.11.x -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27034 Caroline Cyr La Rose <caroline.cyr-la-rose@inlibro.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |caroline.cyr-la-rose@inlibr | |o.com -- You are receiving this mail because: You are watching all bug changes.
participants (1)
-
bugzilla-daemon＠bugs.koha-community.org