From 2b0bd07a2db8f4e5c255b6cace300a2d741eb08a Mon Sep 17 00:00:00 2001 From: Tomas Cohen Arazi Date: Wed, 5 Aug 2020 12:04:47 -0300 Subject: [PATCH] Bug 26143: Regression tests This patch introduces tests for the per_page=-1 handling use case. From now on per_page=-1 means 'all resources'. On writing this I noticed that we always paginate results no matter what, but there was a weird condition under which on pagination headers were sent back to the API consumer. This is highlighted in the precedent patch, which is not the -1 situation this one tries to tackle. Both pagination and searching are broken with per_page=-1, which is a standard, and we actually didn't explicitly set a way to request all resources. To verify this: 1. Apply the previous tests patch and this one 2. Run: $ kshell k$ prove t/Koha/REST/Plugin/Pagination.t \ t/db_dependent/Koha/REST/Plugin/Objects.t => FAIL: Things are damn broken Signed-off-by: Tomas Cohen Arazi Signed-off-by: Jonathan Druart (cherry picked from commit d20bc39d759bc9321b1b7accb952f90351a14caa) Signed-off-by: Lucas Gass (cherry picked from commit 9a4910812f03fade73d27db8ce975dc0a15fe9e7) Signed-off-by: Aleisha Amohia --- t/Koha/REST/Plugin/Pagination.t | 52 ++++++++++++++++++++++- t/db_dependent/Koha/REST/Plugin/Objects.t | 14 +++++- 2 files changed, 64 insertions(+), 2 deletions(-) diff --git a/t/Koha/REST/Plugin/Pagination.t b/t/Koha/REST/Plugin/Pagination.t index adf3001c4f..dff13fb770 100644 --- a/t/Koha/REST/Plugin/Pagination.t +++ b/t/Koha/REST/Plugin/Pagination.t @@ -70,6 +70,28 @@ get '/pagination_headers_without_page' => sub { $c->render( json => { ok => 1 }, status => 200 ); }; +get '/pagination_headers_with_minus_one' => sub { + my $c = shift; + $c->add_pagination_headers( + { + total => 10, + 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, + params => { page => 100, _per_page => -1, firstname => 'Jonathan' } + } + ); + $c->render( json => { ok => 1 }, status => 200 ); +}; + # The tests use Test::More tests => 2; @@ -79,7 +101,7 @@ use t::lib::Mocks; subtest 'add_pagination_headers() tests' => sub { - plan tests => 75; + plan tests => 101; my $t = Test::Mojo->new; @@ -164,6 +186,34 @@ subtest 'add_pagination_headers() tests' => sub { ->header_like( 'Link' => qr/; rel="last"/ ) ->header_like( 'Link' => qr/; rel="last"/ ) ->header_like( 'Link' => qr/; rel="last"/ ); + + $t->get_ok('/pagination_headers_with_minus_one') + ->status_is( 200 ) + ->header_is( 'X-Total-Count' => 10, 'X-Total-Count header present, with per_page=-1' ) + ->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_unlike( 'Link' => qr/; rel="next",/, 'No next page, all resources are fetched' ) + ->header_like( 'Link' => qr/; rel="first",/ ) + ->header_like( 'Link' => qr/; rel="first",/ ) + ->header_like( 'Link' => qr/; rel="first",/ ) + ->header_like( 'Link' => qr/; rel="last"/ ) + ->header_like( 'Link' => qr/; rel="last"/ ) + ->header_like( 'Link' => qr/; rel="last"/ ); + + $t->get_ok('/pagination_headers_with_minus_one_and_invalid_page') + ->status_is( 200 ) + ->header_is( 'X-Total-Count' => 10, 'X-Total-Count header present, with per_page=-1' ) + ->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_unlike( 'Link' => qr/; rel="next",/, 'No next page, all resources are fetched' ) + ->header_like( 'Link' => qr/; rel="first",/ ) + ->header_like( 'Link' => qr/; rel="first",/ ) + ->header_like( 'Link' => qr/; rel="first",/ ) + ->header_like( 'Link' => qr/; rel="last"/ ) + ->header_like( 'Link' => qr/; rel="last"/ ) + ->header_like( 'Link' => qr/; rel="last"/ ); }; subtest 'dbic_merge_pagination() tests' => sub { diff --git a/t/db_dependent/Koha/REST/Plugin/Objects.t b/t/db_dependent/Koha/REST/Plugin/Objects.t index e4e66c47b0..f23ab6d718 100644 --- a/t/db_dependent/Koha/REST/Plugin/Objects.t +++ b/t/db_dependent/Koha/REST/Plugin/Objects.t @@ -110,7 +110,7 @@ my $t = Test::Mojo->new; subtest 'objects.search helper' => sub { - plan tests => 44; + plan tests => 50; $schema->storage->txn_begin; @@ -211,6 +211,18 @@ subtest 'objects.search helper' => sub { $response_count = scalar @{ $t->tx->res->json }; is( $response_count, 5, 'RESTdefaultPageSize is honoured by default (5)' ); + $t->get_ok('/cities?_page=1&_per_page=-1') + ->status_is(200); + + $response_count = scalar @{ $t->tx->res->json }; + is( $response_count, 24, '_per_page=-1 means all resources' ); + + $t->get_ok('/cities?_page=100&_per_page=-1') + ->status_is(200); + + $response_count = scalar @{ $t->tx->res->json }; + is( $response_count, 24, 'When _per_page=-1 the page param is not considered' ); + $schema->storage->txn_rollback; }; -- 2.39.5