From 6eef54e9407dbf7e65a05022fbaa8fe09bf859d3 Mon Sep 17 00:00:00 2001 From: Ere Maijala Date: Wed, 29 Jan 2020 13:24:15 +0200 Subject: [PATCH] [SIGNED-OFF] Bug 22522: [19.11.x] Add support for current Mojolicious and related packages This patch allows tests to succeed with the following versions: JSON::Validator 3.18 Mojolicious 8.32 Mojolicious::Plugin::OpenAPI 2.21 Also Mojolicious::Plugin::OpenAPI version 1.17 and later 1.x versions now work. Calling valid_input in under() would cause ' Use of uninitialized value $_[2] ' in more recent OpenAPI plugins, so that was changed too. As far as I can see this does not affect authorization. Signed-off-by: Martin Renvoize Signed-off-by: Jonathan Druart Signed-off-by: Aleisha Amohia --- Koha/REST/V1/Auth.pm | 11 +- t/db_dependent/Koha/REST/Plugin/Objects.t | 111 ++++-------------- .../Koha/REST/Plugin/PluginRoutes.t | 16 ++- 3 files changed, 43 insertions(+), 95 deletions(-) diff --git a/Koha/REST/V1/Auth.pm b/Koha/REST/V1/Auth.pm index 8bffc977da..26ffd1d8f6 100644 --- a/Koha/REST/V1/Auth.pm +++ b/Koha/REST/V1/Auth.pm @@ -55,14 +55,14 @@ This subroutine is called before every request to API. =cut sub under { - my $c = shift->openapi->valid_input or return;; + my ( $c ) = @_; my $status = 0; try { # /api/v1/{namespace} - my $namespace = $c->req->url->to_abs->path->[2]; + my $namespace = $c->req->url->to_abs->path->[2] // ''; if ( $namespace eq 'public' and !C4::Context->preference('RESTPublicAPI') ) @@ -136,7 +136,11 @@ sub authenticate_api_request { my $user; - my $spec = $c->match->endpoint->pattern->defaults->{'openapi.op_spec'}; + # The following supports retrieval of spec with Mojolicious::Plugin::OpenAPI@1.17 and later (first one) + # and older versions (second one). + # TODO: remove the latter 'openapi.op_spec' if minimum version is bumped to at least 1.17. + my $spec = $c->openapi->spec || $c->match->endpoint->pattern->defaults->{'openapi.op_spec'}; + my $authorization = $spec->{'x-koha-authorization'}; my $authorization_header = $c->req->headers->authorization; @@ -202,7 +206,6 @@ sub authenticate_api_request { if ($status eq "ok") { my $session = get_session($sessionID); $user = Koha::Patrons->find($session->param('number')); - # $c->stash('koha.user' => $user); } elsif ($status eq "maintenance") { Koha::Exceptions::UnderMaintenance->throw( diff --git a/t/db_dependent/Koha/REST/Plugin/Objects.t b/t/db_dependent/Koha/REST/Plugin/Objects.t index 63e2123725..13f9a648ea 100644 --- a/t/db_dependent/Koha/REST/Plugin/Objects.t +++ b/t/db_dependent/Koha/REST/Plugin/Objects.t @@ -109,14 +109,15 @@ my $t = Test::Mojo->new; subtest 'objects.search helper' => sub { - plan tests => 90; + plan tests => 38; $schema->storage->txn_begin; - # Remove existing cities to have more control on the search restuls + # Remove existing cities to have more control on the search results Koha::Cities->delete; - # Create two sample patrons that match the query + # Create three sample cities that match the query. This makes sure we + # always have a "next" link regardless of Mojolicious::Plugin::OpenAPI version. $builder->build_object({ class => 'Koha::Cities', value => { @@ -129,10 +130,16 @@ subtest 'objects.search helper' => sub { city_name => 'Manuela' } }); + $builder->build_object({ + class => 'Koha::Cities', + value => { + city_name => 'Manuelab' + } + }); $t->get_ok('/cities?city_name=manuel&_per_page=1&_page=1') ->status_is(200) - ->header_like( 'Link' => qr/; rel="next",/ ) + ->header_like( 'Link' => qr/; rel="next",/ ) ->json_has('/0') ->json_hasnt('/1') ->json_is('/0/city_name' => 'Manuel'); @@ -145,53 +152,18 @@ subtest 'objects.search helper' => sub { }); # _match=starts_with - $t->get_ok('/cities?city_name=manuel&_per_page=3&_page=1&_match=starts_with') - ->status_is(200) - ->json_has('/0') - ->json_has('/1') - ->json_hasnt('/2') - ->json_is('/0/city_name' => 'Manuel') - ->json_is('/1/city_name' => 'Manuela'); - - # _match=ends_with - $t->get_ok('/cities?city_name=manuel&_per_page=3&_page=1&_match=ends_with') - ->status_is(200) - ->json_has('/0') - ->json_has('/1') - ->json_hasnt('/2') - ->json_is('/0/city_name' => 'Manuel') - ->json_is('/1/city_name' => 'Emanuel'); - - # _match=exact - $t->get_ok('/cities?city_name=manuel&_per_page=3&_page=1&_match=exact') - ->status_is(200) - ->json_has('/0') - ->json_hasnt('/1') - ->json_is('/0/city_name' => 'Manuel'); - - # _match=contains - $t->get_ok('/cities?city_name=manuel&_per_page=3&_page=1&_match=contains') + $t->get_ok('/cities?name=manuel&_per_page=4&_page=1&_match=starts_with') ->status_is(200) ->json_has('/0') ->json_has('/1') ->json_has('/2') ->json_hasnt('/3') - ->json_is('/0/city_name' => 'Manuel') - ->json_is('/1/city_name' => 'Manuela') - ->json_is('/2/city_name' => 'Emanuel'); - - ## _to_model tests - # _match=starts_with - $t->get_ok('/cities_to_model?nombre=manuel&_per_page=3&_page=1&_match=starts_with') - ->status_is(200) - ->json_has('/0') - ->json_has('/1') - ->json_hasnt('/2') - ->json_is('/0/city_name' => 'Manuel') - ->json_is('/1/city_name' => 'Manuela'); + ->json_is('/0/name' => 'Manuel') + ->json_is('/1/name' => 'Manuela') + ->json_is('/2/name' => 'Manuelab'); # _match=ends_with - $t->get_ok('/cities_to_model?nombre=manuel&_per_page=3&_page=1&_match=ends_with') + $t->get_ok('/cities?name=manuel&_per_page=4&_page=1&_match=ends_with') ->status_is(200) ->json_has('/0') ->json_has('/1') @@ -200,59 +172,24 @@ subtest 'objects.search helper' => sub { ->json_is('/1/city_name' => 'Emanuel'); # _match=exact - $t->get_ok('/cities_to_model?nombre=manuel&_per_page=3&_page=1&_match=exact') + $t->get_ok('/cities?name=manuel&_per_page=4&_page=1&_match=exact') ->status_is(200) ->json_has('/0') ->json_hasnt('/1') ->json_is('/0/city_name' => 'Manuel'); # _match=contains - $t->get_ok('/cities_to_model?nombre=manuel&_per_page=3&_page=1&_match=contains') - ->status_is(200) - ->json_has('/0') - ->json_has('/1') - ->json_has('/2') - ->json_hasnt('/3') - ->json_is('/0/city_name' => 'Manuel') - ->json_is('/1/city_name' => 'Manuela') - ->json_is('/2/city_name' => 'Emanuel'); - - ## _to_model && _to_api tests - # _match=starts_with - $t->get_ok('/cities_to_model_to_api?nombre=manuel&_per_page=3&_page=1&_match=starts_with') - ->status_is(200) - ->json_has('/0') - ->json_has('/1') - ->json_hasnt('/2') - ->json_is('/0/nombre' => 'Manuel') - ->json_is('/1/nombre' => 'Manuela'); - - # _match=ends_with - $t->get_ok('/cities_to_model_to_api?nombre=manuel&_per_page=3&_page=1&_match=ends_with') - ->status_is(200) - ->json_has('/0') - ->json_has('/1') - ->json_hasnt('/2') - ->json_is('/0/nombre' => 'Manuel') - ->json_is('/1/nombre' => 'Emanuel'); - - # _match=exact - $t->get_ok('/cities_to_model_to_api?nombre=manuel&_per_page=3&_page=1&_match=exact') - ->status_is(200) - ->json_has('/0') - ->json_hasnt('/1') - ->json_is('/0/nombre' => 'Manuel'); - - # _match=contains - $t->get_ok('/cities_to_model_to_api?nombre=manuel&_per_page=3&_page=1&_match=contains') + $t->get_ok('/cities?name=manuel&_per_page=4&_page=1&_match=contains') ->status_is(200) ->json_has('/0') ->json_has('/1') ->json_has('/2') - ->json_hasnt('/3') - ->json_is('/0/nombre' => 'Manuel') - ->json_is('/1/nombre' => 'Manuela') - ->json_is('/2/nombre' => 'Emanuel'); + ->json_has('/3') + ->json_hasnt('/4') + ->json_is('/0/name' => 'Manuel') + ->json_is('/1/name' => 'Manuela') + ->json_is('/2/name' => 'Manuelab') + ->json_is('/3/name' => 'Emanuel'); $schema->storage->txn_rollback; }; diff --git a/t/db_dependent/Koha/REST/Plugin/PluginRoutes.t b/t/db_dependent/Koha/REST/Plugin/PluginRoutes.t index 197a28d016..6444d8bc64 100644 --- a/t/db_dependent/Koha/REST/Plugin/PluginRoutes.t +++ b/t/db_dependent/Koha/REST/Plugin/PluginRoutes.t @@ -50,6 +50,8 @@ subtest 'Bad plugins tests' => sub { t::lib::Mocks::mock_config( 'enable_plugins', 1 ); t::lib::Mocks::mock_preference( 'UseKohaPlugins', 1 ); + # remove any existing plugins that might interfere + Koha::Plugins::Methods->search->delete; my $plugins = Koha::Plugins->new; $plugins->InstallPlugins; @@ -66,8 +68,10 @@ subtest 'Bad plugins tests' => sub { 'Bad plugins raise warning'; my $routes = get_defined_routes($t); - ok( !exists $routes->{'/contrib/badass/patrons/(:patron_id)/bother_wrong'}, 'Route doesn\'t exist' ); - ok( exists $routes->{'/contrib/testplugin/patrons/(:patron_id)/bother'}, 'Route exists' ); + # Support placeholders () and <> (latter style used starting with Mojolicious::Plugin::OpenAPI@1.28) + # TODO: remove () if minimum version is bumped to at least 1.28. + ok( !exists $routes->{'/contrib/badass/patrons/(:patron_id)/bother_wrong'} && !exists $routes->{'/contrib/badass/patrons/<:patron_id>/bother_wrong'}, 'Route doesn\'t exist' ); + ok( exists $routes->{'/contrib/testplugin/patrons/(:patron_id>)/bother'} || exists $routes->{'/contrib/testplugin/patrons/<:patron_id>/bother'}, 'Route exists' ); $schema->storage->txn_rollback; }; @@ -98,7 +102,9 @@ subtest 'Disabled plugins tests' => sub { my $t = Test::Mojo->new('Koha::REST::V1'); my $routes = get_defined_routes($t); - ok( !exists $routes->{'/contrib/testplugin/patrons/(:patron_id)/bother'}, + # Support placeholders () and <> (latter style used starting with Mojolicious::Plugin::OpenAPI@1.28) + # TODO: remove () if minimum version is bumped to at least 1.28. + ok( !exists $routes->{'/contrib/testplugin/patrons/(:patron_id)/bother'} && !exists $routes->{'/contrib/testplugin/patrons/<:patron_id>/bother'}, 'Plugin disabled, route not defined' ); $good_plugin->enable; @@ -106,7 +112,9 @@ subtest 'Disabled plugins tests' => sub { $t = Test::Mojo->new('Koha::REST::V1'); $routes = get_defined_routes($t); - ok( exists $routes->{'/contrib/testplugin/patrons/(:patron_id)/bother'}, + # Support placeholders () and <> (latter style used starting with Mojolicious::Plugin::OpenAPI@1.28) + # TODO: remove () if minimum version is bumped to at least 1.28. + ok( exists $routes->{'/contrib/testplugin/patrons/(:patron_id)/bother'} || exists $routes->{'/contrib/testplugin/patrons/<:patron_id>/bother'}, 'Plugin enabled, route defined' ); $schema->storage->txn_rollback; -- 2.39.5