Attachment 143132 Details for Bug 26635 – Bug 26635: Move expand syntax to x-koha-embed

[patch] Bug 26635: Move expand syntax to x-koha-embed

Bug-26635-Move-expand-syntax-to-x-koha-embed.patch (text/plain), 12.21 KB, created by Kyle M Hall (khall) on 2022-11-03 15:22:51 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Kyle M Hall (khall)

Created: 2022-11-03 15:22:51 UTC

Size: 12.21 KB

patch

obsolete

>From 88eb3eebfadfa885eb8558450f466d95bceb5da9 Mon Sep 17 00:00:00 2001
>From: Tomas Cohen Arazi <tomascohen@theke.io>
>Date: Wed, 2 Nov 2022 12:48:29 -0300
>Subject: [PATCH] Bug 26635: Move expand syntax to x-koha-embed
>MIME-Version: 1.0
>Content-Type: text/plain; charset=UTF-8
>Content-Transfer-Encoding: 8bit
>
>Instead of a global av-expand flag (through a header) this patch
>proposes to allow specifying +av_expand at the x-koha-embed header
>level. This allows a more fine-grained control on what objects get avs
>expanded. e.g.:
>
>GET /patrons/123
>x-koha-expand: +av_expand,checkouts.item+av_expand
>=>
>{
>    "_str": {
>        "city": { "str": "CÃ³rdoba", ... }
>    },
>    ...
>    "checkouts": [
>        {
>            ...,
>            "item": {
>                "_str": {
>                    "not_for_loan_status": { "str": "Reference material", ... },
>                    ...
>                },
>                ...
>            }
>        }
>    ]
>}
>
>To test:
>1. Run:
>   $ kshell
>  k$ prove t/db_dependent/Koha/Object.t \
>           t/Koha/REST/Plugin/Query.t \
>           t/db_dependent/Koha/REST/Plugin/Objects.t
>=> SUCCESS: Tests pass!
>2. Sign off :-D
>
>Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
>
>Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
>---
> Koha/Object.pm                            | 22 ++++++++++------
> Koha/REST/Plugin/Objects.pm               |  4 +--
> Koha/REST/Plugin/Query.pm                 | 10 ++++++-
> t/Koha/REST/Plugin/Query.t                | 32 ++++++++++++++++++-----
> t/db_dependent/Koha/Object.t              | 26 ++++++++++++++++--
> t/db_dependent/Koha/REST/Plugin/Objects.t | 11 ++++----
> 6 files changed, 80 insertions(+), 25 deletions(-)
>
>diff --git a/Koha/Object.pm b/Koha/Object.pm
>index 3b2b80407b..187ba00769 100644
>--- a/Koha/Object.pm
>+++ b/Koha/Object.pm
>@@ -553,9 +553,17 @@ sub to_api {
>     my ( $self, $params ) = @_;
>     my $json_object = $self->TO_JSON;
> 
>+    # Make sure we duplicate the $params variable to avoid
>+    # breaking calls in a loop (Koha::Objects->to_api)
>+    $params = defined $params ? {%$params} : {};
>+
>+    # children should be able to handle without
>+    my $embeds    = delete $params->{embed};
>+    my $av_expand = delete $params->{av_expand};
>+
>     # coded values handling
>     my $avs = {};
>-    if ( $params->{av_expand} and $self->can('api_av_mapping') ) {
>+    if ( $av_expand and $self->can('api_av_mapping') ) {
>         $avs = $self->api_av_mapping($params);
>     }
> 
>@@ -565,7 +573,7 @@ sub to_api {
>             delete $json_object->{$field} unless any { $_ eq $field } @{ $self->public_read_list };
>         }
> 
>-        if ( $params->{av_expand} ) {
>+        if ( $av_expand ) {
>             foreach my $field (keys %{$avs}) {
>                 delete $avs->{$field}
>                     unless any { $_ eq $field } @{ $self->public_read_list };
>@@ -598,12 +606,7 @@ sub to_api {
>     }
> 
>     $json_object->{_str} = $avs
>-      if $params->{av_expand};
>-
>-    # Make sure we duplicate the $params variable to avoid
>-    # breaking calls in a loop (Koha::Objects->to_api)
>-    $params    = {%$params};
>-    my $embeds = delete $params->{embed};
>+      if $av_expand;
> 
>     if ($embeds) {
>         foreach my $embed ( keys %{$embeds} ) {
>@@ -617,6 +620,9 @@ sub to_api {
>                 my $curr = $embed;
>                 my $next = $embeds->{$curr}->{children};
> 
>+                $params->{av_expand} = 1
>+                  if $embeds->{$embed}->{av_expand};
>+
>                 my $children = $self->$curr;
> 
>                 if ( defined $children and ref($children) eq 'ARRAY' ) {
>diff --git a/Koha/REST/Plugin/Objects.pm b/Koha/REST/Plugin/Objects.pm
>index c9a2a26106..ccc0a08942 100644
>--- a/Koha/REST/Plugin/Objects.pm
>+++ b/Koha/REST/Plugin/Objects.pm
>@@ -54,7 +54,7 @@ the requested object. It passes through any embeds if specified.
> 
>             # Look for embeds
>             my $embed     = $c->stash('koha.embed');
>-            my $av_expand = $c->req->headers->header('x-koha-av-expand');
>+            my $av_expand = $c->stash('koha.av_expand');
> 
>             # Generate prefetches for embedded stuff
>             $c->dbic_merge_prefetch(
>@@ -100,7 +100,7 @@ shouldn't be called twice in it.
>             my $is_public = $c->stash('is_public');
>             # Look for embeds
>             my $embed     = $c->stash('koha.embed');
>-            my $av_expand = $c->req->headers->header('x-koha-av-expand');
>+            my $av_expand = $c->stash('koha.av_expand');
> 
>             # Merge sorting into query attributes
>             $c->dbic_merge_sorting(
>diff --git a/Koha/REST/Plugin/Query.pm b/Koha/REST/Plugin/Query.pm
>index b1418d6c19..f75d284625 100644
>--- a/Koha/REST/Plugin/Query.pm
>+++ b/Koha/REST/Plugin/Query.pm
>@@ -242,7 +242,11 @@ Unwraps and stashes the x-koha-embed headers for use later query construction
>             if ($embed_header) {
>                 my $THE_embed = {};
>                 foreach my $embed_req ( split /\s*,\s*/, $embed_header ) {
>-                    _merge_embed( _parse_embed($embed_req), $THE_embed );
>+                    if ( $embed_req eq '+av_expand' ) {    # special case
>+                        $c->stash( 'koha.av_expand' => 1 );
>+                    } else {
>+                        _merge_embed( _parse_embed($embed_req), $THE_embed );
>+                    }
>                 }
> 
>                 $c->stash( 'koha.embed' => $THE_embed )
>@@ -364,6 +368,10 @@ sub _parse_embed {
>             my $key = $+{relation} . "_count";
>             $result->{$key} = { is_count => 1 };
>         }
>+        elsif ( $curr =~ m/^(?<relation>.*)\+av_expand/ ) {
>+            my $key = $+{relation};
>+            $result->{$key} = { av_expand => 1 };
>+        }
>         else {
>             $result->{$curr} = {};
>         }
>diff --git a/t/Koha/REST/Plugin/Query.t b/t/Koha/REST/Plugin/Query.t
>index 5a3774599f..509f4f9ff6 100755
>--- a/t/Koha/REST/Plugin/Query.t
>+++ b/t/Koha/REST/Plugin/Query.t
>@@ -210,11 +210,15 @@ get '/stash_embed' => sub {
>     my $c = shift;
> 
>     $c->stash_embed();
>-    my $embed = $c->stash('koha.embed');
>+    my $embed     = $c->stash('koha.embed');
>+    my $av_expand = $c->stash('koha.av_expand');
> 
>     $c->render(
>         status => 200,
>-        json   => $embed
>+        json   => {
>+            av_expand => $av_expand,
>+            embed     => $embed,
>+        }
>     );
> };
> 
>@@ -429,24 +433,38 @@ subtest '_build_query_params_from_api' => sub {
> 
> subtest 'stash_embed() tests' => sub {
> 
>-    plan tests => 8;
>+    plan tests => 14;
> 
>     my $t = Test::Mojo->new;
> 
>     $t->get_ok( '/stash_embed' => { 'x-koha-embed' => 'checkouts,checkouts.item' } )
>-      ->json_is( { checkouts => { children => { item => { } } } } );
>+      ->json_is( '/embed' => { checkouts => { children => { item => { } } } } );
> 
>     $t->get_ok( '/stash_embed' => { 'x-koha-embed' => 'checkouts,checkouts.item,library' } )
>-      ->json_is( { checkouts => { children => { item => {} } }, library => {} } );
>+      ->json_is( '/embed' => { checkouts => { children => { item => {} } }, library => {} } );
> 
>     $t->get_ok( '/stash_embed' => { 'x-koha-embed' => 'holds+count' } )
>-      ->json_is( { holds_count => { is_count => 1 } } );
>+      ->json_is( '/embed' => { holds_count => { is_count => 1 } } );
> 
>     $t->get_ok( '/stash_embed' => { 'x-koha-embed' => 'checkouts,checkouts.item,patron' } )
>-      ->json_is({
>+      ->json_is( '/embed' => {
>             checkouts => { children => { item => {} } },
>             patron    => {}
>         });
>+
>+    $t->get_ok( '/stash_embed' => { 'x-koha-embed' => 'checkouts,checkouts.item+av_expand,patron+av_expand' } )
>+      ->json_is( '/embed' => {
>+            checkouts => { children => { item => { av_expand => 1 } } },
>+            patron    => { av_expand => 1 }
>+        })
>+      ->json_is( '/av_expand' => undef );
>+
>+    $t->get_ok( '/stash_embed' => { 'x-koha-embed' => 'checkouts+av_expand,checkouts.item,patron,+av_expand' } )
>+      ->json_is( '/embed' => {
>+            checkouts => { children => { item => { } }, av_expand => 1 },
>+            patron    => { }
>+        })
>+      ->json_is( '/av_expand' => 1 );
> };
> 
> subtest 'stash_overrides() tests' => sub {
>diff --git a/t/db_dependent/Koha/Object.t b/t/db_dependent/Koha/Object.t
>index 6e7d8a1a9f..47346c5850 100755
>--- a/t/db_dependent/Koha/Object.t
>+++ b/t/db_dependent/Koha/Object.t
>@@ -226,7 +226,7 @@ subtest 'TO_JSON tests' => sub {
> 
> subtest "to_api() tests" => sub {
> 
>-    plan tests => 30;
>+    plan tests => 31;
> 
>     $schema->storage->txn_begin;
> 
>@@ -303,14 +303,36 @@ subtest "to_api() tests" => sub {
>     is($biblio_api->{items}->[0]->{holds}->[0]->{hold_id}, $hold->reserve_id, 'Hold matches');
>     is_deeply($biblio_api->{biblioitem}, $biblio->biblioitem->to_api, 'More than one root');
> 
>+    my $_str = {
>+        location => {
>+            category => 'ASD',
>+            str      => 'Estante alto',
>+            type     => 'av'
>+        }
>+    };
>+
>+    # mock Koha::Item so it implements 'api_av_mapping'
>+    my $item_mock = Test::MockModule->new('Koha::Item');
>+    $item_mock->mock(
>+        'api_av_mapping',
>+        sub {
>+            return $_str;
>+        }
>+    );
>+
>     my $hold_api = $hold->to_api(
>         {
>-            embed => { 'item' => {} }
>+            embed => { 'item' => { av_expand => 1 } }
>         }
>     );
> 
>     is( ref($hold_api->{item}), 'HASH', 'Single nested object works correctly' );
>     is( $hold_api->{item}->{item_id}, $item->itemnumber, 'Object embedded correctly' );
>+    is_deeply(
>+        $hold_api->{item}->{_str},
>+        $_str,
>+        '_str correctly added to nested embed'
>+    );
> 
>     # biblio with no items
>     my $new_biblio = $builder->build_sample_biblio;
>diff --git a/t/db_dependent/Koha/REST/Plugin/Objects.t b/t/db_dependent/Koha/REST/Plugin/Objects.t
>index 21f89f0639..487b9f6380 100755
>--- a/t/db_dependent/Koha/REST/Plugin/Objects.t
>+++ b/t/db_dependent/Koha/REST/Plugin/Objects.t
>@@ -38,6 +38,7 @@ plugin 'Koha::REST::Plugin::Pagination';
> get '/cities' => sub {
>     my $c = shift;
>     $c->validation->output($c->req->params->to_hash);
>+    $c->stash_embed;
>     my $cities = $c->objects->search(Koha::Cities->new);
>     $c->render( status => 200, json => $cities );
> };
>@@ -45,6 +46,7 @@ get '/cities' => sub {
> get '/cities/:city_id' => sub {
>     my $c = shift;
>     my $id = $c->stash("city_id");
>+    $c->stash_embed;
>     my $city = $c->objects->find(Koha::Cities->new, $id);
>     $c->render( status => 200, json => $city );
> };
>@@ -712,7 +714,7 @@ subtest 'objects.find helper with expanded authorised values' => sub {
>         }
>     );
> 
>-    $t->get_ok( '/cities/' . $manuel->id => { 'x-koha-av-expand' => 1 } )
>+    $t->get_ok( '/cities/' . $manuel->id => { 'x-koha-embed' => '+av_expand' } )
>       ->status_is(200)->json_is( '/name' => 'Manuel' )
>       ->json_has('/_str')
>       ->json_is( '/_str/country/type'     => 'av' )
>@@ -723,7 +725,7 @@ subtest 'objects.find helper with expanded authorised values' => sub {
>       ->status_is(200)->json_is( '/name' => 'Manuel' )
>       ->json_hasnt('/_str');
> 
>-    $t->get_ok( '/cities/' . $manuela->id => { 'x-koha-av-expand' => 1 } )
>+    $t->get_ok( '/cities/' . $manuela->id => { 'x-koha-embed' => '+av_expand' } )
>       ->status_is(200)->json_is( '/name' => 'Manuela' )
>       ->json_has('/_str')
>       ->json_is( '/_str/country/type'     => 'av' )
>@@ -827,7 +829,7 @@ subtest 'objects.search helper with expanded authorised values' => sub {
>     );
> 
>     $t->get_ok( '/cities?name=manuel&_per_page=4&_page=1&_match=starts_with' =>
>-          { 'x-koha-av-expand' => 1 } )->status_is(200)
>+          { 'x-koha-embed' => '+av_expand' } )->status_is(200)
>       ->json_has('/0')->json_has('/1')->json_hasnt('/2')
>       ->json_is( '/0/name' => 'Manuel' )
>       ->json_has('/0/_str')
>@@ -840,8 +842,7 @@ subtest 'objects.search helper with expanded authorised values' => sub {
>       ->json_is( '/1/_str/country/type'     => 'av' )
>       ->json_is( '/1/_str/country/category' => $cat->category_name );
> 
>-    $t->get_ok( '/cities?name=manuel&_per_page=4&_page=1&_match=starts_with' =>
>-          { 'x-koha-av-expand' => 0 } )->status_is(200)
>+    $t->get_ok( '/cities?name=manuel&_per_page=4&_page=1&_match=starts_with' )->status_is(200)
>       ->json_has('/0')->json_has('/1')->json_hasnt('/2')
>       ->json_is( '/0/name' => 'Manuel' )->json_hasnt('/0/_str')
>       ->json_is( '/1/name' => 'Manuela' )->json_hasnt('/1/_str');
>-- 
>2.30.2

Actions: View | Diff | Splinter Review

Attachments on bug 26635: 111400 | 111401 | 111402 | 111411 | 111412 | 111413 | 111414 | 112538 | 112539 | 116232 | 116233 | 116234 | 116235 | 116592 | 116593 | 120698 | 120699 | 120700 | 124268 | 142761 | 142762 | 142763 | 142776 | 142792 | 142793 | 142794 | 142795 | 142796 | 142801 | 142802 | 142803 | 142804 | 142805 | 143039 | 143040 | 143041 | 143042 | 143043 | 143044 | 143073 | 143074 | 143075 | 143076 | 143077 | 143078 | 143079 | 143080 | 143127 | 143128 | 143129 | 143130 | 143131 | 143132 | 143133 | 143134 | 143276 | 143354 | 143355 | 143356 | 143357 | 143358 | 143359 | 143360 | 143361 | 143362