gitea
/
Koha-clms
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

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 <martin.renvoize@ptfs-europe.com>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
(cherry picked from commit ad83d2e07d)
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
This commit is contained in:
Tomas Cohen Arazi 2023-03-03 09:28:14 -03:00 committed by Martin Renvoize
parent d1f3c6e806
commit 5455fcd371
No known key found for this signature in database
GPG Key ID: 422B469130441A0F
3 changed files with 141 additions and 117 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

53
Koha/REST/Plugin/Objects.pm
View File

@ -44,7 +44,7 @@ sub register {
Performs a database search using given Koha::Objects object and the $id.
Returns I<undef> if no object is found or the I<API representation> 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<API representations> 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;
}
);

79
Koha/REST/Plugin/Pagination.pm
View File

@ -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 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.
Adds RFC5988 compliant I<Link> headers for pagination to the response message carried
by our controller.
If page size is omitted, it defaults to the value of the RESTdefaultPageSize syspref.
Additionally, it also adds the customer I<X-Total-Count> header containing the total results
count, and I<X-Base-Total-Count> 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<search_rs> helper.
=over
=item base_total
Total records for the search domain (e.g. all patron records, filtered only by visibility)
=item page
The requested page number, usually extracted from the request query.
See I<objects.search_rs> for more information.
=item per_page
The requested maximum results per page, usually extracted from the request query.
See I<objects.search_rs> for more information.
=item query_params
The request query, usually extracted from the request query and used to build the I<Link> headers.
See I<objects.search_rs> 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;
}
);

126
t/Koha/REST/Plugin/Pagination.t → t/db_dependent/Koha/REST/Plugin/Pagination.t
View File

@ -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/<http:\/\/.*\?.*_per_page=3.*>; rel="prev",/ )
->header_like( 'Link' => qr/<http:\/\/.*\?.*_page=1.*>; rel="prev",/ )
->header_like( 'Link' => qr/<http:\/\/.*\?.*firstname=Jonathan.*>; rel="prev",/ )
@ -130,10 +120,10 @@ subtest 'add_pagination_headers() tests' => sub {
->header_like( 'Link' => qr/<http:\/\/.*\?.*_page=4.*>; rel="last"/ )
->header_like( 'Link' => qr/<http:\/\/.*\?.*firstname=Jonathan.*>; 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/<http:\/\/.*\?.*>; rel="prev",/ )
->header_like( 'Link' => qr/<http:\/\/.*\?.*_per_page=3.*>; rel="next",/ )
->header_like( 'Link' => qr/<http:\/\/.*\?.*_page=2.*>; rel="next",/ )
@ -145,10 +135,10 @@ subtest 'add_pagination_headers() tests' => sub {
->header_like( 'Link' => qr/<http:\/\/.*\?.*_page=4.*>; rel="last"/ )
->header_like( 'Link' => qr/<http:\/\/.*\?.*firstname=Jonathan.*>; 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/<http:\/\/.*\?.*_per_page=3.*>; rel="prev",/ )
->header_like( 'Link' => qr/<http:\/\/.*\?.*_page=3.*>; rel="prev",/ )
->header_like( 'Link' => qr/<http:\/\/.*\?.*firstname=Jonathan.*>; rel="prev",/ )
@ -161,10 +151,10 @@ subtest 'add_pagination_headers() tests' => sub {
->header_like( 'Link' => qr/<http:\/\/.*\?.*firstname=Jonathan.*>; 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/<http:\/\/.*\?.*per_page=3.*>; rel="prev",/ )
->header_like( 'Link' => qr/<http:\/\/.*\?.*page=1.*>; rel="prev",/ )
->header_like( 'Link' => qr/<http:\/\/.*\?.*firstname=Jonathan.*>; rel="prev",/ )
@ -178,15 +168,15 @@ subtest 'add_pagination_headers() tests' => sub {
->header_like( 'Link' => qr/<http:\/\/.*\?.*page=4.*>; rel="last"/ )
->header_like( 'Link' => qr/<http:\/\/.*\?.*firstname=Jonathan.*>; 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/<http:\/\/.*\?.*per_page=3.*>; rel="prev",/, 'First page, no previous' )
->header_unlike( 'Link' => qr/<http:\/\/.*\?.*page=1.*>; rel="prev",/, 'First page, no previous' )
->header_unlike( 'Link' => qr/<http:\/\/.*\?.*firstname=Jonathan.*>; rel="prev",/, 'First page, no previous' )
->header_like( 'Link' => qr/<http:\/\/.*\?.*per_page=3.*>; rel="next",/ )
->header_like( 'Link' => qr/<http:\/\/.*\?.*page=3.*>; rel="next",/ )
->header_like( 'Link' => qr/<http:\/\/.*\?.*page=2.*>; rel="next",/ )
->header_like( 'Link' => qr/<http:\/\/.*\?.*firstname=Jonathan.*>; rel="next",/ )
->header_like( 'Link' => qr/<http:\/\/.*\?.*per_page=3.*>; rel="first",/ )
->header_like( 'Link' => qr/<http:\/\/.*\?.*page=1.*>; rel="first",/ )
@ -195,10 +185,10 @@ subtest 'add_pagination_headers() tests' => sub {
->header_like( 'Link' => qr/<http:\/\/.*\?.*page=4.*>; rel="last"/ )
->header_like( 'Link' => qr/<http:\/\/.*\?.*firstname=Jonathan.*>; 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/<http:\/\/.*\?.*per_page=-1.*>; rel="prev",/, 'First page, no previous' )
->header_unlike( 'Link' => qr/<http:\/\/.*\?.*page=1.*>; rel="prev",/, 'First page, no previous' )
->header_unlike( 'Link' => qr/<http:\/\/.*\?.*firstname=Jonathan.*>; rel="prev",/, 'First page, no previous' )
@ -210,10 +200,10 @@ subtest 'add_pagination_headers() tests' => sub {
->header_like( 'Link' => qr/<http:\/\/.*\?.*page=1.*>; rel="last"/ )
->header_like( 'Link' => qr/<http:\/\/.*\?.*firstname=Jonathan.*>; 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/<http:\/\/.*\?.*per_page=-1.*>; rel="prev",/, 'First page, no previous' )
->header_unlike( 'Link' => qr/<http:\/\/.*\?.*page=1.*>; rel="prev",/, 'First page, no previous' )
->header_unlike( 'Link' => qr/<http:\/\/.*\?.*firstname=Jonathan.*>; rel="prev",/, 'First page, no previous' )