Koha-community/Koha
13
7
Fork 0
Code Issues Releases Wiki Activity

Bug 27034: Do not use path parameters on building the query

Browse source 
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>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
This commit is contained in:
Tomás Cohen Arazi 2021-01-04 07:55:09 -03:00 committed by Jonathan Druart
parent b0bdaab666
commit 1ddd0ef3cb
2 changed files with 1 additions and 57 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

10
Koha/REST/Plugin/Objects.pm
View file

@ -95,16 +95,6 @@ sub register {
$filtered_params = $c->build_query_params( $filtered_params, $reserved_params );
}
if ( defined $path_params ) {
# Apply the mapping function to the passed params
$filtered_params //= {};
$path_params = $result_set->attributes_from_api($path_params);
foreach my $param (keys %{$path_params}) {
$filtered_params->{$param} = $path_params->{$param};
}
}
if( defined $reserved_params->{q} || defined $reserved_params->{query} || defined $reserved_params->{'x-koha-query'}) {
$filtered_params //={};
my @query_params_array;

48
t/db_dependent/Koha/REST/Plugin/Objects.t
View file

@ -19,7 +19,6 @@ use Modern::Perl;
use Koha::Acquisition::Orders;
use Koha::Cities;
use Koha::Holds;
use Koha::Biblios;
# Dummy app for testing the plugin
@ -46,16 +45,6 @@ get '/orders' => sub {
$c->render( status => 200, json => $orders );
};
get '/patrons/:patron_id/holds' => sub {
my $c = shift;
my $params = $c->req->params->to_hash;
$params->{patron_id} = $c->stash("patron_id");
$c->validation->output($params);
my $holds_set = Koha::Holds->new;
my $holds = $c->objects->search( $holds_set );
$c->render( status => 200, json => {count => scalar(@$holds)} );
};
get '/biblios' => sub {
my $c = shift;
my $output = $c->req->params->to_hash;
@ -75,9 +64,8 @@ get '/biblios' => sub {
$c->render( status => 200, json => {count => scalar(@$biblios), biblios => $biblios} );
};
# The tests
use Test::More tests => 11;
use Test::More tests => 10;
use Test::Mojo;
use t::lib::Mocks;
@ -309,40 +297,6 @@ subtest 'objects.search helper, embed' => sub {
$schema->storage->txn_rollback;
};
subtest 'objects.search helper, with path parameters and _match' => sub {
plan tests => 8;
$schema->storage->txn_begin;
Koha::Holds->search()->delete;
my $patron = Koha::Patrons->find(10);
$patron->delete if $patron;
$patron = $builder->build_object( { class => "Koha::Patrons" } );
$patron->borrowernumber(10)->store;
$builder->build_object(
{
class => "Koha::Holds",
value => { borrowernumber => $patron->borrowernumber }
}
);
my $t = Test::Mojo->new;
$t->get_ok('/patrons/1/holds?_match=exact')
->json_is('/count' => 0, 'there should be no holds for borrower 1 with _match=exact');
$t->get_ok('/patrons/1/holds?_match=contains')
->json_is('/count' => 0, 'there should be no holds for borrower 1 with _match=contains');
$t->get_ok('/patrons/10/holds?_match=exact')
->json_is('/count' => 1, 'there should be 1 hold for borrower 10 with _match=exact');
$t->get_ok('/patrons/10/holds?_match=contains')
->json_is('/count' => 1, 'there should be 1 hold for borrower 10 with _match=contains');
$schema->storage->txn_rollback;
};
subtest 'object.search helper with query parameter' => sub {
plan tests => 4;