Bug 27034 - $c->objects->search shouldn't use path parameters
Summary: $c->objects->search shouldn't use path parameters
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: REST API (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Tomás Cohen Arazi
QA Contact:
URL:
Keywords:
Depends on: 27015
Blocks: 27352
  Show dependency treegraph
 
Reported: 2020-11-16 15:22 UTC by Tomás Cohen Arazi
Modified: 2021-06-14 21:34 UTC (History)
9 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Trivial patch
Text to go in the release notes:
Version(s) released in:
21.05.00


Attachments
Bug 27034: Do not use path parameters on building the query (5.56 KB, patch)
2021-01-04 11:17 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 27034: (follow-up) Remove unneeded params tweak (1.09 KB, patch)
2021-01-04 15:26 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 27034: Do not use path parameters on building the query (5.61 KB, patch)
2021-01-25 23:40 UTC, David Nind
Details | Diff | Splinter Review
Bug 27034: (follow-up) Remove unneeded params tweak (1.14 KB, patch)
2021-01-25 23:40 UTC, David Nind
Details | Diff | Splinter Review
Bug 27034: Do not use path parameters on building the query (5.67 KB, patch)
2021-01-26 12:31 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 27034: (follow-up) Remove unneeded params tweak (1.20 KB, patch)
2021-01-26 12:31 UTC, Martin Renvoize
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-11-16 15:22:44 UTC
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).
Comment 1 Martin Renvoize 2020-12-15 13:23:44 UTC
That took a few re-reads, but I think I generally agree with you.. lets clean it up for now and remove it.
Comment 2 Tomás Cohen Arazi 2021-01-04 11:17:58 UTC
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>
Comment 3 Jonathan Druart 2021-01-04 15:17:33 UTC
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};
Comment 4 Tomás Cohen Arazi 2021-01-04 15:26:43 UTC
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
Comment 5 Tomás Cohen Arazi 2021-01-05 12:44:07 UTC
(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!
Comment 6 David Nind 2021-01-25 22:57:07 UTC
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.
Comment 7 Tomás Cohen Arazi 2021-01-25 23:10:22 UTC
(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!
Comment 8 David Nind 2021-01-25 23:38:57 UTC
(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...
Comment 9 David Nind 2021-01-25 23:40:31 UTC
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>
Comment 10 David Nind 2021-01-25 23:40:35 UTC
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>
Comment 11 Martin Renvoize 2021-01-26 12:31:40 UTC
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>
Comment 12 Martin Renvoize 2021-01-26 12:31:44 UTC
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>
Comment 13 Martin Renvoize 2021-01-26 12:32:18 UTC
Changes all make sense to me and the code works well.  Qa scripts are happy and unit tests are updated.

Passing QA
Comment 14 Jonathan Druart 2021-01-29 07:59:52 UTC
Pushed to master for 21.05, thanks to everybody involved!
Comment 15 Fridolin Somers 2021-02-05 15:33:45 UTC
Depends on Bug 27015 not in 20.11.x