From 1ddd0ef3cb8d9b36421672eb1a57f3b4c3c51434 Mon Sep 17 00:00:00 2001 From: Tomas Cohen Arazi Date: Mon, 4 Jan 2021 07:55:09 -0300 Subject: [PATCH] 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 Signed-off-by: David Nind Signed-off-by: Martin Renvoize Signed-off-by: Jonathan Druart --- Koha/REST/Plugin/Objects.pm | 10 ----- t/db_dependent/Koha/REST/Plugin/Objects.t | 48 +---------------------- 2 files changed, 1 insertion(+), 57 deletions(-) diff --git a/Koha/REST/Plugin/Objects.pm b/Koha/REST/Plugin/Objects.pm index 36f647833a..246aece704 100644 --- a/Koha/REST/Plugin/Objects.pm +++ b/Koha/REST/Plugin/Objects.pm @@ -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; diff --git a/t/db_dependent/Koha/REST/Plugin/Objects.t b/t/db_dependent/Koha/REST/Plugin/Objects.t index 35b16f9329..60c9f81192 100755 --- a/t/db_dependent/Koha/REST/Plugin/Objects.t +++ b/t/db_dependent/Koha/REST/Plugin/Objects.t @@ -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;