Attachment 131498 Details for Bug 30194 – Bug 30194: Adapt Koha to the latest JSON::Validator version

[patch] Bug 30194: Adapt Koha to the latest JSON::Validator version

Bug-30194-Adapt-Koha-to-the-latest-JSONValidator-v.patch (text/plain), 9.44 KB, created by Tomás Cohen Arazi (tcohen) on 2022-03-08 19:16:21 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Tomás Cohen Arazi (tcohen)

Created: 2022-03-08 19:16:21 UTC

Size: 9.44 KB

patch

obsolete

>From 4b51db2ad15e9a8027d2d5f3cdafcd78d72253d6 Mon Sep 17 00:00:00 2001
>From: Tomas Cohen Arazi <tomascohen@theke.io>
>Date: Tue, 8 Mar 2022 12:56:48 -0300
>Subject: [PATCH] Bug 30194: Adapt Koha to the latest JSON::Validator version
>
>This patch makes V1.pm and PluginRoutes.pm work with the
>JSON::Validator breaking changes introduced after v4.
>
>JSON::Validator got stricter, and it seems there's some bug in how it
>resolves references, so I propose we bundle the YAML spec into a single
>file using a gulp task. This will save load time too, so has its
>advantages too.
>
>To test:
>1. Edit /etc/apt/sources.list.d/koha.list
>2. Duplicate the existing line below, replacing 'dev' for 'exp'
>3. Upgrade the packages
>   $ apt update ; apt dist-upgrade -y
>4. Run:
>   $ kshell
>  k$ prove t/db_dependent/Koha/REST/Plugin/PluginRoutes.t \
>           t/db_dependent/api/v1
>=> SUCCESS: Tests pass!
>5. Restart plack and check the logs:
>   $ koha-plack --restart kohadev ; tail -f /var/log/koha/kohadev/*.log
>=> SUCCESS: Nothing seems broken, things work
>6. Point your browser to http://localhost:8081/api/v1/
>=> SUCCESS: The API is loaded
>7. Sign off :-D
>---
> Koha/REST/Plugin/PluginRoutes.pm              | 33 +++++--------
> Koha/REST/V1.pm                               | 49 +++++++------------
> .../Koha/REST/Plugin/PluginRoutes.t           | 16 +++---
> t/db_dependent/Koha/REST/V1.t                 |  2 -
> 4 files changed, 39 insertions(+), 61 deletions(-)
>
>diff --git a/Koha/REST/Plugin/PluginRoutes.pm b/Koha/REST/Plugin/PluginRoutes.pm
>index cf7eea882b..e97a29c764 100644
>--- a/Koha/REST/Plugin/PluginRoutes.pm
>+++ b/Koha/REST/Plugin/PluginRoutes.pm
>@@ -24,6 +24,7 @@ use Koha::Plugins;
> use Koha::Logger;
> 
> use Clone qw( clone );
>+use JSON::Validator::Schema::OpenAPIv2;
> use Try::Tiny qw( catch try );
> 
> =head1 NAME
>@@ -41,8 +42,8 @@ Koha::REST::Plugin::PluginRoutes
> sub register {
>     my ( $self, $app, $config ) = @_;
> 
>-    my $spec      = $config->{spec};
>-    my $validator = $config->{validator};
>+    my $spec     = $config->{spec};
>+    my $validate = $config->{validate};
> 
>     my @plugins;
> 
>@@ -58,7 +59,7 @@ sub register {
>         );
> 
>         foreach my $plugin ( @plugins ) {
>-            $spec = $self->inject_routes( $spec, $plugin, $validator );
>+            $spec = $self->inject_routes( $spec, $plugin, $validate );
>         }
> 
>     }
>@@ -71,14 +72,14 @@ sub register {
> =cut
> 
> sub inject_routes {
>-    my ( $self, $spec, $plugin, $validator ) = @_;
>+    my ( $self, $spec, $plugin, $validate ) = @_;
> 
>-    return merge_spec( $spec, $plugin ) unless $validator;
>+    return merge_spec( $spec, $plugin ) unless $validate;
> 
>     return try {
> 
>         my $backup_spec = merge_spec( clone($spec), $plugin );
>-        if ( $self->spec_ok( $backup_spec, $validator ) ) {
>+        if ( $self->spec_ok( $backup_spec, $validate ) ) {
>             $spec = merge_spec( $spec, $plugin );
>         }
>         else {
>@@ -145,23 +146,15 @@ sub merge_spec {
> =cut
> 
> sub spec_ok {
>-    my ( $self, $spec, $validator ) = @_;
>+    my ( $self, $spec ) = @_;
> 
>-    my $schema = $self->{'swagger-v2-schema'};
>+    my $schema = JSON::Validator::Schema::OpenAPIv2->new( $spec );
> 
>-    return try {
>-        $validator->load_and_validate_schema(
>-            $spec,
>-            {
>-                allow_invalid_ref => 1,
>-                schema => ( $schema ) ? $schema : undef,
>-            }
>-        );
>-        return 1;
>-    }
>-    catch {
>-        return 0;
>+    if ($schema->is_invalid) {
>+        warn $schema->errors->[0];
>     }
>+
>+    return !$schema->is_invalid;
> }
> 
> 1;
>diff --git a/Koha/REST/V1.pm b/Koha/REST/V1.pm
>index 1c22a769d7..e64bf13cbf 100644
>--- a/Koha/REST/V1.pm
>+++ b/Koha/REST/V1.pm
>@@ -22,7 +22,8 @@ use Mojo::Base 'Mojolicious';
> use C4::Context;
> use Koha::Logger;
> 
>-use JSON::Validator::OpenAPI::Mojolicious;
>+use JSON::Validator::Schema::OpenAPIv2;
>+
> use Try::Tiny qw( catch try );
> 
> =head1 NAME
>@@ -68,25 +69,23 @@ sub startup {
>         $self->secrets([$secret_passphrase]);
>     }
> 
>-    my $validator = JSON::Validator::OpenAPI::Mojolicious->new;
>+    my $spec_file = $self->home->rel_file("api/v1/swagger/swagger.json");
> 
>     push @{$self->routes->namespaces}, 'Koha::Plugin';
> 
>     # Try to load and merge all schemas first and validate the result just once.
>-    my $spec;
>-    my $swagger_schema = $self->home->rel_file("api/swagger-v2-schema.json");
>     try {
>-        $spec = $validator->bundle(
>-            {
>-                replace => 1,
>-                schema => $self->home->rel_file("api/v1/swagger/swagger.yaml")
>-            }
>-        );
>+
>+        my $schema = JSON::Validator::Schema::OpenAPIv2->new;
>+
>+        $schema->resolve( $spec_file );
>+
>+        my $spec = $schema->bundle->data;
> 
>         $self->plugin(
>             'Koha::REST::Plugin::PluginRoutes' => {
>-                spec               => $spec,
>-                validator          => undef
>+                spec     => $spec,
>+                validate => 0,
>             }
>         ) unless C4::Context->needs_install; # load only if Koha is installed
> 
>@@ -94,11 +93,6 @@ sub startup {
>             OpenAPI => {
>                 spec  => $spec,
>                 route => $self->routes->under('/api/v1')->to('Auth#under'),
>-                schema => ( $swagger_schema ) ? $swagger_schema : undef,
>-                allow_invalid_ref =>
>-                1,    # required by our spec because $ref directly under
>-                        # Paths-, Parameters-, Definitions- & Info-object
>-                        # is not allowed by the OpenAPI specification.
>             }
>         );
>     }
>@@ -111,19 +105,16 @@ sub startup {
>         $logger->error("Warning: Could not load REST API spec bundle: " . $_);
> 
>         try {
>-            $validator->load_and_validate_schema(
>-                $self->home->rel_file("api/v1/swagger/swagger.yaml"),
>-                {
>-                    allow_invalid_ref  => 1,
>-                    schema => ( $swagger_schema ) ? $swagger_schema : undef,
>-                }
>-            );
> 
>-            $spec = $validator->schema->data;
>+            my $schema = JSON::Validator::Schema::OpenAPIv2->new;
>+            $schema->resolve( $spec_file );
>+
>+            my $spec = $schema->bundle->data;
>+
>             $self->plugin(
>                 'Koha::REST::Plugin::PluginRoutes' => {
>-                    spec      => $spec,
>-                    validator => $validator
>+                    spec     => $spec,
>+                    validate => 1
>                 }
>             )  unless C4::Context->needs_install; # load only if Koha is installed
> 
>@@ -131,10 +122,6 @@ sub startup {
>                 OpenAPI => {
>                     spec  => $spec,
>                     route => $self->routes->under('/api/v1')->to('Auth#under'),
>-                    allow_invalid_ref =>
>-                    1,    # required by our spec because $ref directly under
>-                            # Paths-, Parameters-, Definitions- & Info-object
>-                            # is not allowed by the OpenAPI specification.
>                 }
>             );
>         }
>diff --git a/t/db_dependent/Koha/REST/Plugin/PluginRoutes.t b/t/db_dependent/Koha/REST/Plugin/PluginRoutes.t
>index 9c6db7b89b..1bd91c9015 100755
>--- a/t/db_dependent/Koha/REST/Plugin/PluginRoutes.t
>+++ b/t/db_dependent/Koha/REST/Plugin/PluginRoutes.t
>@@ -25,8 +25,6 @@ use Test::MockModule;
> use File::Basename;
> use t::lib::Mocks;
> 
>-use JSON::Validator::OpenAPI::Mojolicious;
>-
> # Dummy app for testing the plugin
> use Mojolicious::Lite;
> 
>@@ -72,8 +70,9 @@ subtest 'Bad plugins tests' => sub {
>     my $t;
>     warning_like
>         { $t = Test::Mojo->new('Koha::REST::V1'); }
>-        [qr{Could not load REST API spec bundle: Invalid JSON specification},
>-        qr{The resulting spec is invalid. Skipping Bad API Route Plugin},],
>+        [qr{Could not load REST API spec bundle: /paths/~0001contrib~0001badass},
>+        qr{bother_wrong/put/parameters/0: /oneOf/1 Properties not allowed:},
>+        qr{Plugin Koha::Plugin::BadAPIRoute route injection failed: The resulting spec is invalid. Skipping Bad API Route Plugin},],
>         'Bad plugins raise warning';
> 
>     my $routes = get_defined_routes($t);
>@@ -153,8 +152,9 @@ subtest 'Permissions and access to plugin routes tests' => sub {
>     my $t;
>     warning_like
>         { $t = Test::Mojo->new('Koha::REST::V1'); }
>-        [qr{Could not load REST API spec bundle: Invalid JSON specification},
>-        qr{The resulting spec is invalid. Skipping Bad API Route Plugin},],
>+        [qr{Could not load REST API spec bundle: /paths/~0001contrib~0001badass},
>+        qr{bother_wrong/put/parameters/0: /oneOf/1 Properties not allowed:},
>+        qr{Plugin Koha::Plugin::BadAPIRoute route injection failed: The resulting spec is invalid. Skipping Bad API Route Plugin},],
>         'Bad plugins raise warning';
> 
>     my $routes = get_defined_routes($t);
>@@ -262,8 +262,8 @@ sub traverse_routes {
>     my $path = $route->pattern->unparsed || '/';
> 
>     # Methods
>-    my $via = $route->via;
>-    my $verb = !$via ? '*' : uc join ',', @$via;
>+    my $methods = $route->methods // [];
>+    my $verb = !$methods ? '*' : uc join ',', @$methods;
>     $routes->{$path}->{$verb} = 1;
> 
>     $depth++;
>diff --git a/t/db_dependent/Koha/REST/V1.t b/t/db_dependent/Koha/REST/V1.t
>index faddea9da3..fc333a0814 100755
>--- a/t/db_dependent/Koha/REST/V1.t
>+++ b/t/db_dependent/Koha/REST/V1.t
>@@ -21,8 +21,6 @@ use Test::More tests => 1;
> use Test::Mojo;
> use Test::Warn;
> 
>-use JSON::Validator::OpenAPI::Mojolicious;
>-
> subtest 'Type definition tests' => sub {
> 
>     plan tests => 4;
>-- 
>2.35.1

Actions: View | Diff | Splinter Review

Attachments on bug 30194: 131374 | 131486 | 131487 | 131488 | 131497 | 131498 | 131499 | 131500 | 131501 | 131502 | 131547 | 131548 | 131549 | 131550 | 131551 | 131552 | 132069 | 132070 | 132071 | 132072 | 132073 | 132097 | 132098 | 132099 | 132100 | 132102 | 132112 | 132116 | 132118 | 132119 | 132120 | 132121 | 132122 | 132123 | 132124 | 132125 | 132126 | 132151 | 132152 | 132252 | 132253 | 132254 | 132334 | 132335 | 132336 | 132337 | 132338 | 132339 | 132340 | 132442 | 132449