From 6b5db123ec91874bd368e601ea930384ff67a869 Mon Sep 17 00:00:00 2001
From: Ere Maijala <ere.maijala@helsinki.fi>
Date: Wed, 29 Jan 2020 13:24:15 +0200
Subject: [PATCH] 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 <martin.renvoize@ptfs-europe.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
---
 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/<http:\/\/.*\?.*&_page=2.*>; rel="next",/ )
+        ->header_like( 'Link' => qr/<http:\/\/.*[\?&]_page=2.*>; 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.20.1