Attachment 115805 Details for Bug 27034 – Bug 27034: Do not use path parameters on building the query

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

Bug-27034-Do-not-use-path-parameters-on-building-t.patch (text/plain), 5.67 KB, created by Martin Renvoize (ashimema) on 2021-01-26 12:31:40 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Martin Renvoize (ashimema)

Created: 2021-01-26 12:31:40 UTC

Size: 5.67 KB

patch

obsolete

>From 8cabd72884f69194d8b995d474851079cddd3404 Mon Sep 17 00:00:00 2001
>From: Tomas Cohen Arazi <tomascohen@theke.io>
>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 <tomascohen@theke.io>
>Signed-off-by: David Nind <david@davidnind.com>
>Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
>---
> 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 0f08a2d37a..fec1fe794a 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 cbec228ce9..119dc79f2c 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 => 10;
>+use Test::More tests => 9;
> use Test::Mojo;
> 
> use t::lib::Mocks;
>@@ -274,40 +262,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;
> 
>-- 
>2.20.1

Actions: View | Diff | Splinter Review

Attachments on bug 27034: 114777 | 114806 | 115796 | 115797 | 115805 | 115806