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).
That took a few re-reads, but I think I generally agree with you.. lets clean it up for now and remove it.
Created attachment 114777 [details] [review] 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>
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};
Created attachment 114806 [details] [review] 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
(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!
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.
(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!
(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...
Created attachment 115796 [details] [review] 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>
Created attachment 115797 [details] [review] 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>
Created attachment 115805 [details] [review] 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>
Created attachment 115806 [details] [review] 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>
Changes all make sense to me and the code works well. Qa scripts are happy and unit tests are updated. Passing QA
Pushed to master for 21.05, thanks to everybody involved!
Depends on Bug 27015 not in 20.11.x