From 5455fcd3718677b0a7fb33ece113d95fe5d9bc1e Mon Sep 17 00:00:00 2001 From: Tomas Cohen Arazi Date: Fri, 3 Mar 2023 09:28:14 -0300 Subject: [PATCH] Bug 33080: Allow pagination to be built from stashed values The way the old `objects.search` was build implied several totals were calculated there, and passed to the `$c->add_pagination_headers` helper. With the introduction of `objects.search_rs` and the ability of doing things to the resultset afterwards, it felt like out of place to have the pagination headers be implicitly set inside `objects.search_rs`. This patch makes the search_rs stash some required values (from the original request) and makes `add_pagination_headers` accept those values. This way other callers can still build their own pagination values, while allowing this simplified implementation. Full-stack tests still pass, and the helper tests are moved to the `db_dependent` section and are now more meaningful as well To test: 1. Apply this patch 2. Run: $ ktd --shell $ prove t/db_dependent/Koha/REST/Plugin/Pagination.t \ t/db_dependent/api/v1/ => SUCCESS: Tests pass! Nothing broken! 3. Sign off :-D Signed-off-by: Martin Renvoize Signed-off-by: Kyle M Hall Signed-off-by: Tomas Cohen Arazi (cherry picked from commit ad83d2e07d3c2c28e962b1a2e8d04b6f25a4a868) Signed-off-by: Martin Renvoize --- Koha/REST/Plugin/Objects.pm | 53 ++++---- Koha/REST/Plugin/Pagination.pm | 79 +++++++---- .../Koha/REST/Plugin/Pagination.t | 126 ++++++++---------- 3 files changed, 141 insertions(+), 117 deletions(-) rename t/{ => db_dependent}/Koha/REST/Plugin/Pagination.t (76%) diff --git a/Koha/REST/Plugin/Objects.pm b/Koha/REST/Plugin/Objects.pm index 462fdf97da..94baf03753 100644 --- a/Koha/REST/Plugin/Objects.pm +++ b/Koha/REST/Plugin/Objects.pm @@ -44,7 +44,7 @@ sub register { Performs a database search using given Koha::Objects object and the $id. Returns I if no object is found or the I of -the requested object. It passes through any embeds if specified. +the requested object including any embeds specified in the request. =cut @@ -65,10 +65,10 @@ the requested object. It passes through any embeds if specified. . . . return $c->render({ status => 200, openapi => $patron_rs->to_api }); -Returns the passed Koha::Objects resultset filtered to the passed $id and +Returns the passed Koha::Object result filtered to the passed $id and with any embeds requested by the api applied. -The resultset can then be used for further processing. +The result object can then be used for further processing. =cut @@ -102,8 +102,8 @@ The resultset can then be used for further processing. Performs a database search using given Koha::Objects object with any api query parameters applied. -Returns an arrayref of the hashrefs representing the resulting objects -for API rendering. +Returns an arrayref of I of the requested objects +including any embeds specified in the request. Warning: this helper adds pagination headers to the calling controller, and thus shouldn't be called twice in it. @@ -114,22 +114,28 @@ shouldn't be called twice in it. 'objects.search' => sub { my ( $c, $result_set ) = @_; - return $c->objects->to_api( $c->objects->search_rs($result_set) ); + # Generate the resultset using the HTTP request information + my $objects_rs = $c->objects->search_rs($result_set); + + # Add pagination headers + $c->add_pagination_headers(); + + return $c->objects->to_api($objects_rs); } ); =head3 objects.search_rs - my $patrons_rs = Koha::Patrons->new; - my $patrons_rs = $c->objects->search_rs( $patrons_rs ); + my $patrons_object = Koha::Patrons->new; + my $patrons_rs = $c->objects->search_rs( $patrons_object ); . . . return $c->render({ status => 200, openapi => $patrons_rs->to_api }); Returns the passed Koha::Objects resultset filtered as requested by the api query parameters and with requested embeds applied. -Warning: this helper adds pagination headers to the calling controller, and thus -shouldn't be called twice in it. +Warning: this helper stashes base values for the pagination headers to the calling +controller, and thus shouldn't be called twice in it. =cut @@ -155,9 +161,12 @@ shouldn't be called twice in it. # If no pagination parameters are passed, default $reserved_params->{_per_page} //= - C4::Context->preference('RESTdefaultPageSize'); + C4::Context->preference('RESTdefaultPageSize') // 20; $reserved_params->{_page} //= 1; + $c->stash('koha.pagination.page' => $reserved_params->{_page}); + $c->stash('koha.pagination.per_page' => $reserved_params->{_per_page}); + unless ( $reserved_params->{_per_page} == -1 ) { # Merge pagination into query attributes @@ -244,23 +253,15 @@ shouldn't be called twice in it. $result_set = $result_set->search_limited, if $result_set->can('search_limited'); - # Perform search - my $objects = $result_set->search( $filtered_params, $attributes ); - my $total = $result_set->search->count; + $c->stash('koha.pagination.base_total' => $result_set->count); + $c->stash('koha.pagination.query_params' => $args); - $c->add_pagination_headers( - { - total => ( - $objects->is_paged - ? $objects->pager->total_entries - : $objects->count - ), - base_total => $total, - params => $args, - } - ); + # Generate the resultset + my $objects_rs = $result_set->search( $filtered_params, $attributes ); + # Stash the page total if requires, total otherwise + $c->stash('koha.pagination.total' => $objects_rs->is_paged ? $objects_rs->pager->total_entries : $objects_rs->count); - return $objects; + return $objects_rs; } ); diff --git a/Koha/REST/Plugin/Pagination.pm b/Koha/REST/Plugin/Pagination.pm index 9873a7af57..cf5ec5c5d5 100644 --- a/Koha/REST/Plugin/Pagination.pm +++ b/Koha/REST/Plugin/Pagination.pm @@ -38,21 +38,52 @@ sub register { =head3 add_pagination_headers - my $patrons = Koha::Patrons->search( ... ); - $c->add_pagination_headers({ - total => $patrons->count, - params => { - _page => ... - _per_page => ... - ... + $c->add_pagination_headers(); + $c->add_pagination_headers( + { + base_total => $base_total, + page => $page, + per_page => $per_page, + query_params => $query_params, + total => $total, } - }); + ); + +Adds RFC5988 compliant I headers for pagination to the response message carried +by our controller. + +Additionally, it also adds the customer I header containing the total results +count, and I header containing the total of the non-filtered results count. + +Optionally accepts the any of the following parameters to override the values stored in the +stash by the I helper. + +=over + +=item base_total + +Total records for the search domain (e.g. all patron records, filtered only by visibility) -Adds a Link header to the response message $c carries, following RFC5988, including -the following relation types: 'prev', 'next', 'first' and 'last'. -It also adds X-Total-Count containing the total results count, and X-Base-Total-Count containing the total of the non-filtered results count. +=item page -If page size is omitted, it defaults to the value of the RESTdefaultPageSize syspref. +The requested page number, usually extracted from the request query. +See I for more information. + +=item per_page + +The requested maximum results per page, usually extracted from the request query. +See I for more information. + +=item query_params + +The request query, usually extracted from the request query and used to build the I headers. +See I for more information. + +=item total + +Total records for the search with filters applied. + +=back =cut @@ -60,11 +91,11 @@ If page size is omitted, it defaults to the value of the RESTdefaultPageSize sys 'add_pagination_headers' => sub { my ( $c, $args ) = @_; - my $total = $args->{total}; - my $base_total = $args->{base_total}; - my $req_page = $args->{params}->{_page} // 1; - my $per_page = $args->{params}->{_per_page} // - C4::Context->preference('RESTdefaultPageSize') // 20; + my $base_total = $args->{base_total} // $c->stash('koha.pagination.base_total'); + my $req_page = $args->{page} // $c->stash('koha.pagination.page') // 1; + my $per_page = $args->{per_page} // $c->stash('koha.pagination.per_page') // C4::Context->preference('RESTdefaultPageSize') // 20; + my $params = $args->{query_params} // $c->stash('koha.pagination.query_params'); + my $total = $args->{total} // $c->stash('koha.pagination.total'); my $pages; if ( $per_page == -1 ) { @@ -86,7 +117,7 @@ If page size is omitted, it defaults to the value of the RESTdefaultPageSize sys { page => $req_page - 1, per_page => $per_page, rel => 'prev', - params => $args->{params} + params => $params } ); } @@ -98,17 +129,17 @@ If page size is omitted, it defaults to the value of the RESTdefaultPageSize sys { page => $req_page + 1, per_page => $per_page, rel => 'next', - params => $args->{params} + params => $params } ); } push @links, _build_link( $c, - { page => 1, per_page => $per_page, rel => 'first', params => $args->{params} } ); + { page => 1, per_page => $per_page, rel => 'first', params => $params } ); push @links, _build_link( $c, - { page => $pages, per_page => $per_page, rel => 'last', params => $args->{params} } ); + { page => $pages, per_page => $per_page, rel => 'last', params => $params } ); # Add Link header foreach my $link (@links) { @@ -116,8 +147,10 @@ If page size is omitted, it defaults to the value of the RESTdefaultPageSize sys } # Add X-Total-Count header - $c->res->headers->add( 'X-Total-Count' => $total ); - $c->res->headers->add( 'X-Base-Total-Count' => $base_total ) if defined $base_total; + $c->res->headers->add( 'X-Total-Count' => $total ); + $c->res->headers->add( 'X-Base-Total-Count' => $base_total ) + if defined $base_total; + return $c; } ); diff --git a/t/Koha/REST/Plugin/Pagination.t b/t/db_dependent/Koha/REST/Plugin/Pagination.t similarity index 76% rename from t/Koha/REST/Plugin/Pagination.t rename to t/db_dependent/Koha/REST/Plugin/Pagination.t index e8d25e7360..5e8a4a3449 100755 --- a/t/Koha/REST/Plugin/Pagination.t +++ b/t/db_dependent/Koha/REST/Plugin/Pagination.t @@ -20,10 +20,28 @@ use Modern::Perl; # Dummy app for testing the plugin use Mojolicious::Lite; +use Koha::Patrons; + +use t::lib::TestBuilder; + app->log->level('error'); plugin 'Koha::REST::Plugin::Pagination'; +my $schema = Koha::Database->new->schema; +my $builder = t::lib::TestBuilder->new; + +$schema->storage->txn_begin; + +# Add 10 known 'Jonathan' patrons +my @patron_ids; +for my $i ( 1 .. 10 ) { + push @patron_ids, $builder->build_object({ class => 'Koha::Patrons', value => { firstname => 'Jonathan' } })->id; +} +# Add two non-Jonathans +push @patron_ids, $builder->build_object({ class => 'Koha::Patrons' })->id; +push @patron_ids, $builder->build_object({ class => 'Koha::Patrons' })->id; + # For add_pagination_headers() get '/empty' => sub { @@ -33,19 +51,27 @@ get '/empty' => sub { get '/pagination_headers' => sub { my $c = shift; - $c->add_pagination_headers({ total => 10, base_total => 12, params => { _page => 2, _per_page => 3, firstname => 'Jonathan' } }); - $c->render( json => { ok => 1 }, status => 200 ); -}; -get '/pagination_headers_first_page' => sub { - my $c = shift; - $c->add_pagination_headers({ total => 10, base_total => 12, params => { _page => 1, _per_page => 3, firstname => 'Jonathan' } }); - $c->render( json => { ok => 1 }, status => 200 ); -}; + my $args = $c->req->params->to_hash; + + my $result_set = Koha::Patrons->search( + { borrowernumber => \@patron_ids } + ); + + my $rows = ($args->{_per_page}) ? + ( $args->{_per_page} == -1 ) ? undef : $args->{_per_page} + : C4::Context->preference('RESTdefaultPageSize'); + + my $objects_rs = $result_set->search( { firstname => 'Jonathan' } ,{ page => $args->{_page} // 1, rows => $rows }); + + $c->stash('koha.pagination.page' => $args->{_page}); + $c->stash('koha.pagination.per_page' => $args->{_per_page}); + $c->stash('koha.pagination.base_total' => $result_set->count); + $c->stash('koha.pagination.query_params' => $args); + $c->stash('koha.pagination.total' => $objects_rs->is_paged ? $objects_rs->pager->total_entries : $objects_rs->count); + + $c->add_pagination_headers; -get '/pagination_headers_last_page' => sub { - my $c = shift; - $c->add_pagination_headers({ total => 10, base_total => 12, params => { _page => 4, _per_page => 3, firstname => 'Jonathan' } }); $c->render( json => { ok => 1 }, status => 200 ); }; @@ -58,42 +84,6 @@ get '/dbic_merge_pagination' => sub { $c->render( json => $filter, status => 200 ); }; -get '/pagination_headers_without_page_size' => sub { - my $c = shift; - $c->add_pagination_headers({ total => 10, base_total => 12, params => { _page => 2, firstname => 'Jonathan' } }); - $c->render( json => { ok => 1 }, status => 200 ); -}; - -get '/pagination_headers_without_page' => sub { - my $c = shift; - $c->add_pagination_headers({ total => 10, base_total => 12, params => { _per_page => 3, firstname => 'Jonathan' } }); - $c->render( json => { ok => 1 }, status => 200 ); -}; - -get '/pagination_headers_with_minus_one' => sub { - my $c = shift; - $c->add_pagination_headers( - { - total => 10, - base_total => 12, - params => { _per_page => -1, firstname => 'Jonathan' } - } - ); - $c->render( json => { ok => 1 }, status => 200 ); -}; - -get '/pagination_headers_with_minus_one_and_invalid_page' => sub { - my $c = shift; - $c->add_pagination_headers( - { - total => 10, - base_total => 12, - params => { page => 100, _per_page => -1, firstname => 'Jonathan' } - } - ); - $c->render( json => { ok => 1 }, status => 200 ); -}; - # The tests use Test::More tests => 2; @@ -109,14 +99,14 @@ subtest 'add_pagination_headers() tests' => sub { $t->get_ok('/empty') ->status_is( 200 ) - ->header_is( 'X-Total-Count' => undef, 'X-Total-Count is undefined' ) + ->header_is( 'X-Total-Count' => undef, 'X-Total-Count is undefined' ) ->header_is( 'X-Base-Total-Count' => undef, 'X-Base-Total-Count is undefined' ) - ->header_is( 'Link' => undef, 'Link is undefined' ); + ->header_is( 'Link' => undef, 'Link is undefined' ); - $t->get_ok('/pagination_headers') + $t->get_ok('/pagination_headers?firstname=Jonathan&_page=2&_per_page=3') ->status_is( 200 ) - ->header_is( 'X-Total-Count' => 10, 'X-Total-Count contains the passed value' ) - ->header_is( 'X-Base-Total-Count' => 12, 'X-Base-Total-Count contains the passed value' ) + ->header_is( 'X-Total-Count' => 10, 'X-Total-Count correctly set' ) + ->header_is( 'X-Base-Total-Count' => 12, 'X-Base-Total-Count correctly set' ) ->header_like( 'Link' => qr/; rel="prev",/ ) ->header_like( 'Link' => qr/; rel="prev",/ ) ->header_like( 'Link' => qr/; rel="prev",/ ) @@ -130,10 +120,10 @@ subtest 'add_pagination_headers() tests' => sub { ->header_like( 'Link' => qr/; rel="last"/ ) ->header_like( 'Link' => qr/; rel="last"/ ); - $t->get_ok('/pagination_headers_first_page') + $t->get_ok('/pagination_headers?firstname=Jonathan&_page=1&_per_page=3') ->status_is( 200 ) - ->header_is( 'X-Total-Count' => 10, 'X-Total-Count contains the passed value' ) - ->header_is( 'X-Base-Total-Count' => 12, 'X-Base-Total-Count contains the passed value' ) + ->header_is( 'X-Total-Count' => 10, 'X-Total-Count correctly set' ) + ->header_is( 'X-Base-Total-Count' => 12, 'X-Base-Total-Count correctly set' ) ->header_unlike( 'Link' => qr/; rel="prev",/ ) ->header_like( 'Link' => qr/; rel="next",/ ) ->header_like( 'Link' => qr/; rel="next",/ ) @@ -145,10 +135,10 @@ subtest 'add_pagination_headers() tests' => sub { ->header_like( 'Link' => qr/; rel="last"/ ) ->header_like( 'Link' => qr/; rel="last"/ ); - $t->get_ok('/pagination_headers_last_page') + $t->get_ok('/pagination_headers?firstname=Jonathan&_page=4&_per_page=3') ->status_is( 200 ) - ->header_is( 'X-Total-Count' => 10, 'X-Total-Count contains the passed value' ) - ->header_is( 'X-Base-Total-Count' => 12, 'X-Base-Total-Count contains the passed value' ) + ->header_is( 'X-Total-Count' => 10, 'X-Total-Count correctly set' ) + ->header_is( 'X-Base-Total-Count' => 12, 'X-Base-Total-Count correctly set' ) ->header_like( 'Link' => qr/; rel="prev",/ ) ->header_like( 'Link' => qr/; rel="prev",/ ) ->header_like( 'Link' => qr/; rel="prev",/ ) @@ -161,10 +151,10 @@ subtest 'add_pagination_headers() tests' => sub { ->header_like( 'Link' => qr/; rel="last"/ ); t::lib::Mocks::mock_preference('RESTdefaultPageSize', 3); - $t->get_ok('/pagination_headers_without_page_size') + $t->get_ok('/pagination_headers?firstname=Jonathan&_page=2') ->status_is( 200 ) - ->header_is( 'X-Total-Count' => 10, 'X-Total-Count contains the passed value' ) - ->header_is( 'X-Base-Total-Count' => 12, 'X-Base-Total-Count contains the passed value' ) + ->header_is( 'X-Total-Count' => 10, 'X-Total-Count correctly set' ) + ->header_is( 'X-Base-Total-Count' => 12, 'X-Base-Total-Count correctly set' ) ->header_like( 'Link' => qr/; rel="prev",/ ) ->header_like( 'Link' => qr/; rel="prev",/ ) ->header_like( 'Link' => qr/; rel="prev",/ ) @@ -178,15 +168,15 @@ subtest 'add_pagination_headers() tests' => sub { ->header_like( 'Link' => qr/; rel="last"/ ) ->header_like( 'Link' => qr/; rel="last"/ ); - $t->get_ok('/pagination_headers_without_page') + $t->get_ok('/pagination_headers?firstname=Jonathan&_per_page=3') ->status_is( 200 ) ->header_is( 'X-Total-Count' => 10, 'X-Total-Count header present, even without page param' ) - ->header_is( 'X-Base-Total-Count' => 12, 'X-Base-Total-Count contains the passed value' ) + ->header_is( 'X-Base-Total-Count' => 12, 'X-Base-Total-Count correctly set' ) ->header_unlike( 'Link' => qr/; rel="prev",/, 'First page, no previous' ) ->header_unlike( 'Link' => qr/; rel="prev",/, 'First page, no previous' ) ->header_unlike( 'Link' => qr/; rel="prev",/, 'First page, no previous' ) ->header_like( 'Link' => qr/; rel="next",/ ) - ->header_like( 'Link' => qr/; rel="next",/ ) + ->header_like( 'Link' => qr/; rel="next",/ ) ->header_like( 'Link' => qr/; rel="next",/ ) ->header_like( 'Link' => qr/; rel="first",/ ) ->header_like( 'Link' => qr/; rel="first",/ ) @@ -195,10 +185,10 @@ subtest 'add_pagination_headers() tests' => sub { ->header_like( 'Link' => qr/; rel="last"/ ) ->header_like( 'Link' => qr/; rel="last"/ ); - $t->get_ok('/pagination_headers_with_minus_one') + $t->get_ok('/pagination_headers?firstname=Jonathan&_per_page=-1') ->status_is( 200 ) ->header_is( 'X-Total-Count' => 10, 'X-Total-Count header present, with per_page=-1' ) - ->header_is( 'X-Base-Total-Count' => 12, 'X-Base-Total-Count contains the passed value' ) + ->header_is( 'X-Base-Total-Count' => 12, 'X-Base-Total-Count correctly set' ) ->header_unlike( 'Link' => qr/; rel="prev",/, 'First page, no previous' ) ->header_unlike( 'Link' => qr/; rel="prev",/, 'First page, no previous' ) ->header_unlike( 'Link' => qr/; rel="prev",/, 'First page, no previous' ) @@ -210,10 +200,10 @@ subtest 'add_pagination_headers() tests' => sub { ->header_like( 'Link' => qr/; rel="last"/ ) ->header_like( 'Link' => qr/; rel="last"/ ); - $t->get_ok('/pagination_headers_with_minus_one_and_invalid_page') + $t->get_ok('/pagination_headers?firstname=Jonathan&_per_page=-1&_page=100') ->status_is( 200 ) ->header_is( 'X-Total-Count' => 10, 'X-Total-Count header present, with per_page=-1' ) - ->header_is( 'X-Base-Total-Count' => 12, 'X-Base-Total-Count contains the passed value' ) + ->header_is( 'X-Base-Total-Count' => 12, 'X-Base-Total-Count correctly set' ) ->header_unlike( 'Link' => qr/; rel="prev",/, 'First page, no previous' ) ->header_unlike( 'Link' => qr/; rel="prev",/, 'First page, no previous' ) ->header_unlike( 'Link' => qr/; rel="prev",/, 'First page, no previous' ) -- 2.39.5