Attachment 156304 Details for Bug 33503 – Bug 33503 - Plugin OpenAPI2.0 specification schema fragments are not resolved. Merge full schema definitions.

[patch] Bug 33503 - Plugin OpenAPI2.0 specification schema fragments are not resolved. Merge full schema definitions.

Bug-33503---Plugin-OpenAPI20-specification-schema-.patch (text/plain), 13.18 KB, created by Olli-Antti Kivilahti on 2023-09-27 17:39:08 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Olli-Antti Kivilahti

Created: 2023-09-27 17:39:08 UTC

Size: 13.18 KB

patch

obsolete

>From 9cfd1f6f210f0ff0741848df1e9acba90944b6fa Mon Sep 17 00:00:00 2001
>From: Olli-Antti Kivilahti <olli-antti.kivilahti@hypernova.fi>
>Date: Wed, 12 Apr 2023 12:52:41 +0300
>Subject: [PATCH] Bug 33503 - Plugin OpenAPI2.0 specification schema fragments
> are not resolved. Merge full schema definitions.
>
>Koha::REST::Plugin::PluginRoutes.pm simply injects a plugin's OpenAPI2.0 route
>definitions into the /paths -object.
>Possibly after Bug 30194, dereferencing OpenAPI2.0 schema fragments/components
>has started to fail.
>This plugin:
>    https://github.com/Hypernova-Oy/koha-plugin-self-service/releases/tag/v1.0.11
>used to get it's REST API paths injected and dereferenced/bundled correctly,
>for Koha versions 22.05 and before, but in the new Koha version the same schema
>definitions no longer work.
>
>This patch adds a new plugin hook,
>    api_spec
>which injects a full OpenAPI2.0 compatible schema into the full Koha schema,
>avoiding to overwrite any existing definitions/parameters/info/paths/etc.
>This way we maintain backwards compatibility with existing plugins, which are
>not broken, and maintain updated plugins' ability to use complex schemas.
>We need to inject full schema definitions, because of the way how the
>new OpenAPI validator does dereferencing,
>by creating /parameters and /definitions -objects inside the plugin's OpenAPI
>schema object. The api_route()-plugin hook then proceeds to strip away only the
>paths/routes and loses the internal data structure references to the schema
>fragments/components.
>
>If the api_spec is defined in the plugin, the api_routes-hook is never called.
>
>TEST PLAN:
>
>a) Run the modified test.
>or
>b) Install this plugin, and observe the routes are not loaded into the REST API.
>   https://github.com/Hypernova-Oy/koha-plugin-self-service/releases/tag/v1.0.11
>b1) Deploying this patch doesnt fix the plugin, as the whole subsystem to load
>    OpenAPI2 spec has changed, and the plugins updated with workarounds.
>    Plugins can be updated to support api_spec once it has been pushed.
>---
> Koha/REST/Plugin/PluginRoutes.pm              | 77 ++++++++++++++++++-
> .../Koha/REST/Plugin/PluginRoutes.t           |  3 +-
> t/lib/plugins/Koha/Plugin/BundledApiSpec.pm   | 46 +++++++++++
> .../Koha/Plugin/BundledApiSpec/openapi.yaml   | 69 +++++++++++++++++
> .../BundledApiSpec/openapi/definitions.json   |  5 ++
> .../openapi/definitions/finger.json           | 12 +++
> .../BundledApiSpec/openapi/parameters.json    | 18 +++++
> 7 files changed, 226 insertions(+), 4 deletions(-)
> create mode 100644 t/lib/plugins/Koha/Plugin/BundledApiSpec.pm
> create mode 100644 t/lib/plugins/Koha/Plugin/BundledApiSpec/openapi.yaml
> create mode 100644 t/lib/plugins/Koha/Plugin/BundledApiSpec/openapi/definitions.json
> create mode 100644 t/lib/plugins/Koha/Plugin/BundledApiSpec/openapi/definitions/finger.json
> create mode 100644 t/lib/plugins/Koha/Plugin/BundledApiSpec/openapi/parameters.json
>
>diff --git a/Koha/REST/Plugin/PluginRoutes.pm b/Koha/REST/Plugin/PluginRoutes.pm
>index c275f2578c..fac7d48be1 100644
>--- a/Koha/REST/Plugin/PluginRoutes.pm
>+++ b/Koha/REST/Plugin/PluginRoutes.pm
>@@ -102,11 +102,13 @@ sub inject_routes {
> sub merge_spec {
>     my ( $spec, $plugin ) = @_;
> 
>-    if($plugin->can('api_routes')) {
>+    # Prefer the new api_spec() to inject a full OpenAPI2 spec instead of just the route definitions, if both are defined, to prevent duplicate routes.
>+    # This way we can use the same plugin release for the new and the old versions of Koha.
>+    if($plugin->can('api_routes') && not($plugin->can('api_spec'))) { # Deprecated
>         my $plugin_spec = $plugin->api_routes;
> 
>         foreach my $route ( keys %{ $plugin_spec } ) {
>-            my $THE_route = '/contrib/' . $plugin->api_namespace . $route;
>+            my $THE_route = get_normalized_plugin_path($plugin->api_namespace, $route);
>             if ( exists $spec->{ $THE_route } ) {
>                 # Route exists, overwriting is forbidden
>                 Koha::Exceptions::Plugin::ForbiddenAction->throw(
>@@ -118,12 +120,57 @@ sub merge_spec {
>         }
>     }
> 
>+    if($plugin->can('api_spec')) {
>+        my $plugin_namespace = $plugin->api_namespace;
>+        my $plugin_spec = $plugin->api_spec;
>+        my @errors;
>+        if ($plugin_spec->{definitions}) {
>+            while (my ($k, $v) = each %{$plugin_spec->{definitions}}) {
>+                if ($spec->{definitions}->{$k}) {
>+                    push(@errors, "Duplicate OpenAPI definition '$k'. Plugin needs to rename one of it's definitions.");
>+                } else {
>+                    $spec->{definitions}->{$k} = $v;
>+                }
>+            }
>+        }
>+        if ($plugin_spec->{parameters}) {
>+            while (my ($k, $v) = each %{$plugin_spec->{parameters}}) {
>+                if ($spec->{parameters}->{$k}) {
>+                    push(@errors, "Duplicate OpenAPI parameter '$k'. Plugin needs to rename one of it's parameters.");
>+                } else {
>+                    $spec->{parameters}->{$k} = $v;
>+                }
>+            }
>+        }
>+        if ($plugin_spec->{tags}) {
>+            if ($spec->{tags}) {
>+                push(@{$spec->{tags}}, @{$plugin_spec->{tags}});
>+            } else {
>+                $spec->{tags} = $plugin_spec->{tags};
>+            }
>+        }
>+        if ($plugin_spec->{paths}) {
>+            while (my ($k, $v) = each %{$plugin_spec->{paths}}) {
>+                if ($spec->{paths}->{$k}) {
>+                    push(@errors, "Duplicate OpenAPI parameter '$k'. Plugin needs to rename one of it's paths.");
>+                } else {
>+                    $spec->{paths}->{get_normalized_plugin_path($plugin_namespace, $k)} = $v;
>+                }
>+            }
>+        }
>+        if (@errors) {
>+            Koha::Exceptions::Plugin::ForbiddenAction->throw(
>+                "Plugin '".$plugin->{class}."' had the following errors while merging OpenAPI specs:\n".join(', ', @errors)
>+            );
>+        }
>+    }
>+
>     if($plugin->can('static_routes')) {
>         my $plugin_spec = $plugin->static_routes;
> 
>         foreach my $route ( keys %{ $plugin_spec } ) {
> 
>-            my $THE_route = '/contrib/' . $plugin->api_namespace . '/static'.$route;
>+            my $THE_route = get_normalized_plugin_static_path($plugin->api_namespace, $route);
>             if ( exists $spec->{ $THE_route } ) {
>                 # Route exists, overwriting is forbidden
>                 Koha::Exceptions::Plugin::ForbiddenAction->throw(
>@@ -153,4 +200,28 @@ sub spec_ok {
>     return !$schema->is_invalid;
> }
> 
>+=head3 get_normalized_plugin_path
>+
>+public static subroutine
>+Normalizes a plugin's route path to /contrib/<plugin-namespace>/<plugin-route-path>
>+
>+=cut
>+
>+sub get_normalized_plugin_path {
>+    my ($plugin_api_namespace, $plugin_path_name) = @_;
>+    return '/contrib/' . $plugin_api_namespace . $plugin_path_name;
>+}
>+
>+=head3 get_normalized_plugin_static_path
>+
>+public static subroutine
>+Normalizes a plugin's static route path to /contrib/<plugin-namespace>/static/<plugin-route-path>
>+
>+=cut
>+
>+sub get_normalized_plugin_static_path {
>+    my ($plugin_api_namespace, $plugin_path_name) = @_;
>+    return '/contrib/' . $plugin_api_namespace . '/static' . $plugin_path_name;
>+}
>+
> 1;
>diff --git a/t/db_dependent/Koha/REST/Plugin/PluginRoutes.t b/t/db_dependent/Koha/REST/Plugin/PluginRoutes.t
>index 66746508ed..98e7138a32 100755
>--- a/t/db_dependent/Koha/REST/Plugin/PluginRoutes.t
>+++ b/t/db_dependent/Koha/REST/Plugin/PluginRoutes.t
>@@ -129,7 +129,7 @@ subtest 'Disabled plugins tests' => sub {
> 
> subtest 'Permissions and access to plugin routes tests' => sub {
> 
>-    plan tests => 16;
>+    plan tests => 17;
> 
>     $schema->storage->txn_begin;
> 
>@@ -164,6 +164,7 @@ subtest 'Permissions and access to plugin routes tests' => sub {
>     my $routes = get_defined_routes($t);
>     ok( exists $routes->{'/contrib/testplugin/patrons/bother'}, 'Route exists' );
>     ok( exists $routes->{'/contrib/testplugin/public/patrons/bother'}, 'Route exists' );
>+    ok( exists $routes->{'/contrib/bundledass/patrons/<:patron_id>/finger'}, 'Bundled Route exists' );
> 
>     C4::Context->set_preference( 'RESTPublicAnonymousRequests', 0 );
> 
>diff --git a/t/lib/plugins/Koha/Plugin/BundledApiSpec.pm b/t/lib/plugins/Koha/Plugin/BundledApiSpec.pm
>new file mode 100644
>index 0000000000..368d8609e8
>--- /dev/null
>+++ b/t/lib/plugins/Koha/Plugin/BundledApiSpec.pm
>@@ -0,0 +1,46 @@
>+package Koha::Plugin::BundledApiSpec;
>+
>+use Modern::Perl;
>+
>+use base qw(Koha::Plugins::Base);
>+
>+our $VERSION = 0.01;
>+our $metadata = {
>+    name            => 'Bundled OpenAPI Spec Plugin',
>+    author          => 'Olli-Antti Kivilahti',
>+    description     => 'Test plugin for testing the bundling of OpenAPI schema fragments',
>+    date_authored   => '2023-',
>+    date_updated    => '2023-04-06',
>+    minimum_version => '21.11',
>+    maximum_version => undef,
>+    version         => $VERSION,
>+    my_example_tag  => 'bundled_api_spec',
>+};
>+
>+sub new {
>+    my ( $class, $args ) = @_;
>+    $args->{'metadata'} = $metadata;
>+    my $self = $class->SUPER::new($args);
>+    return $self;
>+}
>+
>+sub api_namespace {
>+    return "bundledass";
>+}
>+
>+# One should never define these both, but only the other.
>+#sub api_route {
>+#    my ( $self, $args ) = @_;
>+#    return JSON::Validator->new->schema($self->mbf_dir() . "/openapi.yaml")->schema->{data};
>+#}
>+
>+# One should never define these both, but only the other. Automated test suite should disable the other for testing.
>+sub api_spec {
>+    my ( $self, $args ) = @_;
>+
>+    my $schema2 = JSON::Validator::Schema::OpenAPIv2->new;
>+    $schema2->resolve($self->mbf_dir() . "/openapi.yaml");
>+    return $schema2->bundle->data;
>+}
>+
>+1;
>diff --git a/t/lib/plugins/Koha/Plugin/BundledApiSpec/openapi.yaml b/t/lib/plugins/Koha/Plugin/BundledApiSpec/openapi.yaml
>new file mode 100644
>index 0000000000..1cf2132118
>--- /dev/null
>+++ b/t/lib/plugins/Koha/Plugin/BundledApiSpec/openapi.yaml
>@@ -0,0 +1,69 @@
>+---
>+
>+swagger: "2.0"
>+info:
>+  title: API Title
>+  version: "1.0"
>+
>+paths:
>+  /patrons/{patron_id}/finger:
>+    delete:
>+      x-mojo-to: Koha::Plugin::BundledApiSpec#finger_delete
>+      operationId: finger_delete
>+      tags:
>+        - plugin
>+      produces:
>+        - application/json
>+      parameters:
>+        - $ref: openapi/parameters.json#/patron_idPathParam
>+      x-koha-authorization:
>+        permissions:
>+          borrowers: finger
>+      responses:
>+        "200":
>+          description: Deleted ok
>+          schema:
>+            type: object
>+            required:
>+              - deleted_count
>+            properties:
>+              deleted_count:
>+                description: How many Block-objects have been deleted in the DB
>+                type: integer
>+    get:
>+      x-mojo-to: Koha::Plugin::BundledApiSpec#finger_get
>+      operationId: finger_get
>+      tags:
>+        - plugin
>+      produces:
>+        - application/json
>+      parameters:
>+        - $ref: openapi/parameters.json#/patron_idPathParam
>+      x-koha-authorization:
>+        permissions:
>+          borrowers: finger
>+      responses:
>+        "200":
>+          description: List of fingers
>+          schema:
>+            type: array
>+            items:
>+              $ref: openapi/definitions.json#/finger
>+    post:
>+      x-mojo-to: Koha::Plugin::BundledApiSpec#finger_post
>+      operationId: finger_post
>+      tags:
>+        - plugin
>+      produces:
>+        - application/json
>+      parameters:
>+        - $ref: openapi/parameters.json#/patron_idPathParam
>+        - $ref: openapi/parameters.json#/finger
>+      x-koha-authorization:
>+        permissions:
>+          borrowers: finger
>+      responses:
>+        "200":
>+          description: The created self-service block
>+          schema:
>+            $ref: openapi/definitions.json#/finger
>diff --git a/t/lib/plugins/Koha/Plugin/BundledApiSpec/openapi/definitions.json b/t/lib/plugins/Koha/Plugin/BundledApiSpec/openapi/definitions.json
>new file mode 100644
>index 0000000000..2ee34eac20
>--- /dev/null
>+++ b/t/lib/plugins/Koha/Plugin/BundledApiSpec/openapi/definitions.json
>@@ -0,0 +1,5 @@
>+{
>+  "finger": {
>+    "$ref": "./definitions/finger.json"
>+  }
>+}
>\ No newline at end of file
>diff --git a/t/lib/plugins/Koha/Plugin/BundledApiSpec/openapi/definitions/finger.json b/t/lib/plugins/Koha/Plugin/BundledApiSpec/openapi/definitions/finger.json
>new file mode 100644
>index 0000000000..9adc5fb6fe
>--- /dev/null
>+++ b/t/lib/plugins/Koha/Plugin/BundledApiSpec/openapi/definitions/finger.json
>@@ -0,0 +1,12 @@
>+{
>+  "type": "object",
>+  "properties": {
>+    "patron_id": {
>+      "type": "integer",
>+      "description": "The user that has the finger"
>+    },
>+    "type": {
>+      "type": "string"
>+    }
>+  }
>+}
>\ No newline at end of file
>diff --git a/t/lib/plugins/Koha/Plugin/BundledApiSpec/openapi/parameters.json b/t/lib/plugins/Koha/Plugin/BundledApiSpec/openapi/parameters.json
>new file mode 100644
>index 0000000000..9beb538348
>--- /dev/null
>+++ b/t/lib/plugins/Koha/Plugin/BundledApiSpec/openapi/parameters.json
>@@ -0,0 +1,18 @@
>+{
>+  "patron_idPathParam": {
>+    "name": "patron_id",
>+    "in": "path",
>+    "description": "Internal Patron identifier",
>+    "required": true,
>+    "type": "integer"
>+  },
>+  "finger": {
>+    "name": "finger",
>+    "description": "Finger",
>+    "in": "body",
>+    "required": true,
>+    "schema": {
>+      "$ref": "./definitions.json#/finger"
>+    }
>+  }
>+}
>\ No newline at end of file
>-- 
>2.34.1

Actions: View | Diff | Splinter Review

Attachments on bug 33503: 149520 | 149533 | 156304 | 156312