Bug 30194 - Update required JSON::Validator version
Summary: Update required JSON::Validator version
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: REST API (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Tomás Cohen Arazi
QA Contact: Jonathan Druart
URL:
Keywords: release-notes-needed
: 26893 30193 (view as bug list)
Depends on: 30193
Blocks: 25796 29810 30055 30408 30674
  Show dependency treegraph
 
Reported: 2022-02-28 11:02 UTC by Tomás Cohen Arazi
Modified: 2022-05-03 18:04 UTC (History)
11 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
This development adapts Koha so it works with the latest versions of: - Mojolicious - JSON::Validator - Mojolicious::Plugin::OpenAPI
Version(s) released in:
22.05.00


Attachments
Bug 30194: Adapt V1.pm to the latest JSON::Validator (3.60 KB, patch)
2022-03-04 12:18 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 30194: Adapt Koha to the latest JSON::Validator version (8.46 KB, patch)
2022-03-08 16:05 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 30194: Fix invalid spec (740 bytes, patch)
2022-03-08 16:05 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 30194: Add spec bundle pipeline (278.85 KB, patch)
2022-03-08 16:05 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 30194: Update dependencies versions (1.13 KB, patch)
2022-03-08 19:16 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 30194: Adapt Koha to the latest JSON::Validator version (9.44 KB, patch)
2022-03-08 19:16 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 30194: Fix invalid spec (740 bytes, patch)
2022-03-08 19:16 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 30194: Add spec bundle pipeline (278.88 KB, patch)
2022-03-08 19:16 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 30194: (follow-up) Remove invalid tests (2.49 KB, patch)
2022-03-08 19:16 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 30194: (26351 follow-up) Silence useless warning (1.38 KB, patch)
2022-03-08 19:16 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 30194: Update dependencies versions (1.19 KB, patch)
2022-03-10 12:22 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 30194: Adapt Koha to the latest JSON::Validator version (9.50 KB, patch)
2022-03-10 12:22 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 30194: Fix invalid spec (806 bytes, patch)
2022-03-10 12:22 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 30194: Add spec bundle pipeline (278.95 KB, patch)
2022-03-10 12:22 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 30194: (follow-up) Remove invalid tests (2.55 KB, patch)
2022-03-10 12:22 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 30194: (26351 follow-up) Silence useless warning (1.44 KB, patch)
2022-03-10 12:22 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 30194: Update dependencies versions (1.19 KB, patch)
2022-03-23 18:17 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 30194: Adapt Koha to the latest JSON::Validator version (9.50 KB, patch)
2022-03-23 18:17 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 30194: Fix invalid spec (9.90 KB, patch)
2022-03-23 18:17 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 30194: (follow-up) Remove invalid tests (2.55 KB, patch)
2022-03-23 18:17 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 30194: (26351 follow-up) Silence useless warning (1.44 KB, patch)
2022-03-23 18:17 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 30194: Adapt Koha to the latest JSON::Validator version (9.59 KB, patch)
2022-03-24 06:13 UTC, Mason James
Details | Diff | Splinter Review
Bug 30194: Fix invalid spec (9.99 KB, patch)
2022-03-24 06:14 UTC, Mason James
Details | Diff | Splinter Review
Bug 30194: (follow-up) Remove invalid tests (2.64 KB, patch)
2022-03-24 06:14 UTC, Mason James
Details | Diff | Splinter Review
Bug 30194: (26351 follow-up) Silence useless warning (1.53 KB, patch)
2022-03-24 06:14 UTC, Mason James
Details | Diff | Splinter Review
Bug 30194: Auto generate swagger.yaml (21.11 KB, patch)
2022-03-24 07:56 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 30194: Auto generate swagger.yaml (21.13 KB, patch)
2022-03-24 09:38 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 30194: Simply OpenAPI Specification (226.92 KB, patch)
2022-03-24 10:26 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 30194: (follow-up) Simply OpenAPI Specification (230.80 KB, patch)
2022-03-24 11:33 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 30194: (follow-up) Remove reference to swagger-v2-schema (1.21 KB, patch)
2022-03-24 11:33 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 30194: Adapt Koha to the latest JSON::Validator version (9.61 KB, patch)
2022-03-24 11:35 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 30194: Fix invalid spec (10.01 KB, patch)
2022-03-24 11:35 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 30194: (follow-up) Remove invalid tests (2.66 KB, patch)
2022-03-24 11:35 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 30194: (26351 follow-up) Silence useless warning (1.55 KB, patch)
2022-03-24 11:35 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 30194: Auto generate swagger.yaml (19.82 KB, patch)
2022-03-24 11:35 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 30194: (follow-up) Simply OpenAPI Specification (230.87 KB, patch)
2022-03-24 11:35 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 30194: (follow-up) Remove reference to swagger-v2-schema (1.27 KB, patch)
2022-03-24 11:35 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 30194: (follow-up) Simply OpenAPI Specification (230.93 KB, patch)
2022-03-24 13:30 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 30194: (follow-up) Remove reference to swagger-v2-schema (1.27 KB, patch)
2022-03-24 13:30 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 30194: Auto generate swagger.yaml (19.87 KB, patch)
2022-03-25 19:36 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 30194: (follow-up) Simply OpenAPI Specification (227.90 KB, patch)
2022-03-25 19:37 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 30194: (follow-up) Remove reference to swagger-v2-schema (1.33 KB, patch)
2022-03-25 19:37 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 30194: Adapt Koha to the latest JSON::Validator version (9.69 KB, patch)
2022-03-28 14:59 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 30194: Fix invalid spec (10.09 KB, patch)
2022-03-28 14:59 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 30194: (follow-up) Remove invalid tests (2.74 KB, patch)
2022-03-28 14:59 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 30194: (26351 follow-up) Silence useless warning (1.62 KB, patch)
2022-03-28 15:00 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 30194: Auto generate swagger.yaml (19.95 KB, patch)
2022-03-28 15:00 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 30194: (follow-up) Simply OpenAPI Specification (228.11 KB, patch)
2022-03-28 15:00 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 30194: (follow-up) Remove reference to swagger-v2-schema (1.40 KB, patch)
2022-03-28 15:01 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 30194: Add missing additionalProperties to q_body (731 bytes, patch)
2022-03-29 14:06 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 30194: (follow-up) Fix xt/api.t (1.17 KB, patch)
2022-03-29 15:13 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Tomás Cohen Arazi 2022-02-28 11:02:18 UTC

    
Comment 1 Tomás Cohen Arazi 2022-02-28 11:03:55 UTC
*** Bug 26893 has been marked as a duplicate of this bug. ***
Comment 2 Tomás Cohen Arazi 2022-03-04 12:18:54 UTC Comment hidden (obsolete)
Comment 3 Tomás Cohen Arazi 2022-03-08 16:05:33 UTC Comment hidden (obsolete)
Comment 4 Tomás Cohen Arazi 2022-03-08 16:05:38 UTC Comment hidden (obsolete)
Comment 5 Tomás Cohen Arazi 2022-03-08 16:05:44 UTC Comment hidden (obsolete)
Comment 6 Tomás Cohen Arazi 2022-03-08 17:58:33 UTC
*** Bug 30193 has been marked as a duplicate of this bug. ***
Comment 7 Tomás Cohen Arazi 2022-03-08 19:16:15 UTC
Created attachment 131497 [details] [review]
Bug 30194: Update dependencies versions
Comment 8 Tomás Cohen Arazi 2022-03-08 19:16:21 UTC
Created attachment 131498 [details] [review]
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
Comment 9 Tomás Cohen Arazi 2022-03-08 19:16:27 UTC
Created attachment 131499 [details] [review]
Bug 30194: Fix invalid spec

This patch fixes a problem with the spec.
Comment 10 Tomás Cohen Arazi 2022-03-08 19:16:33 UTC
Created attachment 131500 [details] [review]
Bug 30194: Add spec bundle pipeline

This patch introduces a gulp task for bundling the spec.

To use it, run:

$ yarn spec
Comment 11 Tomás Cohen Arazi 2022-03-08 19:16:40 UTC
Created attachment 131501 [details] [review]
Bug 30194: (follow-up) Remove invalid tests

This patch removes tests for the wrong 'Accept' header situation. It is
the only thing I found I could do.

The situation is: the JSON::Validator + Mojolicious::Plugin::OpenAPI is
trapping this case and returning its own error code (400) and it also
builds its own message! So no point testing it, as it can change again.

I post it as a separate patch just in case someone finds a better way to
handle this and obsoletes the patch :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 12 Tomás Cohen Arazi 2022-03-08 19:16:46 UTC
Created attachment 131502 [details] [review]
Bug 30194: (26351 follow-up) Silence useless warning

Every place in which Koha::Plugin::Test is loaded, and a patron is
updated, this hook needs to be silenced.

To reproduce:
1. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/REST/Plugin/PluginRoutes.t
=> FAIL: There's a warn about patron_barcode_transform
2. Apply this patch
3. Repeat 1
=> SUCCESS: Tests pass!
4. Sign off :-D
Comment 13 Tomás Cohen Arazi 2022-03-08 19:33:52 UTC
Notes about this patchset:

- Mason did a great job packaging the latest Mojolicious, JSON::Validator and  Mojolicious::Plugin::OpenAPI
- The 'exp' repository needs to be added, as detailed in the testing steps.
- It depends on us deprecating Debian 9, which is reasonable as it is EOL in June, so it is a good idea for removing its support for the 22.05 release already.
- JSON::Validator got stricter, and our spec is not kosher => problems. Some are easy and are fixed, others... see below.
- It adds a new step for bundling the spec into a single file. I really would've preferred not to do that. I did it on a separate patch so it can be ditched if we find another way. Basically, references break with JSON::Validator and all points to a bug on the library. But bundling is not the worst, as it will save startup time anyway, and any spec change (in development) already requires a plack restart, so maybe we need some alias that does both. If someone finds a way to fix this, I'll support that.

Looking forward to get some feedback here.
Comment 14 David Cook 2022-03-08 22:38:08 UTC
Sounds good to me.

I'm most excited about this:
https://github.com/jhthorsen/mojolicious-plugin-openapi/commit/673079d19f827ce8c8ab3a2943a4abc798fa1e18

I've found that Mojolicious::Plugin::OpenAPI causes multi-tenant Koha to startup slowly and consume a lot of CPU, so really looking forward to being able to use the "skip_validating_specification" option. (I figure we should validate it at least once, but really shouldn't need to re-validate it for every app startup...)
Comment 15 David Cook 2022-03-08 22:43:15 UTC
(In reply to Tomás Cohen Arazi from comment #13)
> - It adds a new step for bundling the spec into a single file. I really
> would've preferred not to do that. I did it on a separate patch so it can be
> ditched if we find another way. Basically, references break with
> JSON::Validator and all points to a bug on the library. But bundling is not
> the worst, as it will save startup time anyway, and any spec change (in
> development) already requires a plack restart, so maybe we need some alias
> that does both. If someone finds a way to fix this, I'll support that.
> 

It could be worthwhile asking Jan directly. When I made my observations about validation being prohibitively slow, he responded and patched quite quickly. I think we'd resolved the whole thing within 1 week.
Comment 16 Tomás Cohen Arazi 2022-03-08 22:56:08 UTC
(In reply to David Cook from comment #15)
> It could be worthwhile asking Jan directly. When I made my observations
> about validation being prohibitively slow, he responded and patched quite
> quickly. I think we'd resolved the whole thing within 1 week.

I've been chatting with him about this. I just ran out of fuel to write a regression test for JSON:: Validator. Might do it.

The thing is, the spec as-is needs some tweaking to be valid. He even tweaked it to make it valid. But then, even if online validators say it is ok, and also swagger-cli, the library has problems resolving external refs.

With this implementation, we still keep the splitted files to ease things. If we want to rollback because we suddenly can, it is an easy step.

I need help with that. It took me a couple days already to get here.
Comment 17 Tomás Cohen Arazi 2022-03-08 22:58:56 UTC
(In reply to David Cook from comment #14)
> Sounds good to me.
> 
> I'm most excited about this:
> https://github.com/jhthorsen/mojolicious-plugin-openapi/commit/
> 673079d19f827ce8c8ab3a2943a4abc798fa1e18
> 
> I've found that Mojolicious::Plugin::OpenAPI causes multi-tenant Koha to
> startup slowly and consume a lot of CPU, so really looking forward to being
> able to use the "skip_validating_specification" option. (I figure we should
> validate it at least once, but really shouldn't need to re-validate it for
> every app startup...)

I think something can be done with JSON::Validator::Store. My first implementation used that, but didn't have enough time to think of possible side effects (it clearly needs reviewing the JSON::Validator code just in case).

And for the record: the reason we validate the spec is so we are able to inject routes from plugins.

But maybe we are doing it twice hmmm
Comment 18 Tomás Cohen Arazi 2022-03-09 11:02:16 UTC
(In reply to David Cook from comment #14)
> Sounds good to me.

Can you test it?
Comment 19 David Cook 2022-03-09 22:33:32 UTC
(In reply to Tomás Cohen Arazi from comment #18)
> (In reply to David Cook from comment #14)
> > Sounds good to me.
> 
> Can you test it?

I'll see how I'm going for time at the end of the day. Looks like it should be pretty straightforward. (Reminding myself it's a SO not a QA.)
Comment 20 Martin Renvoize 2022-03-10 12:22:22 UTC
Created attachment 131547 [details] [review]
Bug 30194: Update dependencies versions

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 21 Martin Renvoize 2022-03-10 12:22:26 UTC
Created attachment 131548 [details] [review]
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

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 22 Martin Renvoize 2022-03-10 12:22:30 UTC
Created attachment 131549 [details] [review]
Bug 30194: Fix invalid spec

This patch fixes a problem with the spec.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 23 Martin Renvoize 2022-03-10 12:22:35 UTC
Created attachment 131550 [details] [review]
Bug 30194: Add spec bundle pipeline

This patch introduces a gulp task for bundling the spec.

To use it, run:

$ yarn spec

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 24 Martin Renvoize 2022-03-10 12:22:40 UTC
Created attachment 131551 [details] [review]
Bug 30194: (follow-up) Remove invalid tests

This patch removes tests for the wrong 'Accept' header situation. It is
the only thing I found I could do.

The situation is: the JSON::Validator + Mojolicious::Plugin::OpenAPI is
trapping this case and returning its own error code (400) and it also
builds its own message! So no point testing it, as it can change again.

I post it as a separate patch just in case someone finds a better way to
handle this and obsoletes the patch :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 25 Martin Renvoize 2022-03-10 12:22:44 UTC
Created attachment 131552 [details] [review]
Bug 30194: (26351 follow-up) Silence useless warning

Every place in which Koha::Plugin::Test is loaded, and a patron is
updated, this hook needs to be silenced.

To reproduce:
1. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/REST/Plugin/PluginRoutes.t
=> FAIL: There's a warn about patron_barcode_transform
2. Apply this patch
3. Repeat 1
=> SUCCESS: Tests pass!
4. Sign off :-D

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 26 Martin Renvoize 2022-03-10 12:34:56 UTC
This all works as expected, well done Tomas.  Looking through the bundle code, I'm baffled as to what's going wrong but it's certainly, in my opinion, in the library and not the Koha side.

Anywho, I think this approach is a happy median, Signing Off
Comment 27 Tomás Cohen Arazi 2022-03-15 19:00:51 UTC
(In reply to Martin Renvoize from comment #26)
> This all works as expected, well done Tomas.  Looking through the bundle
> code, I'm baffled as to what's going wrong but it's certainly, in my
> opinion, in the library and not the Koha side.
> 
> Anywho, I think this approach is a happy median, Signing Off

Thank you thank you thank you.
Comment 28 Jonathan Druart 2022-03-17 09:55:01 UTC
Comment on attachment 131547 [details] [review]
Bug 30194: Update dependencies versions

Review of attachment 131547 [details] [review]:
-----------------------------------------------------------------

::: cpanfile
@@ +54,4 @@
>  requires 'HTTP::Request::Common', '1.26';
>  requires 'IPC::Cmd', '0.46';
>  requires 'JSON', '2.07';
> +requires 'JSON::Validator', '5.05';

I am not getting 5.05

% pmvers JSON::Validator
5.03

% apt show libjson-validator-perl
Package: libjson-validator-perl
Version: 5.99+really5.03-1
Comment 29 Jonathan Druart 2022-03-17 10:18:41 UTC
1. gulp task could be "api_spec", "spec" it too generic IMO

2. Did you test with plugins injecting their own routes?

3. We need to document this on the wiki

4. We need a check in the release tools, or we will get things out of sync

5. Reference to api/swagger-v2-schema.json has been removed from Koha/REST/V1.pm but still in Koha/REST/Plugin/PluginRoutes.pm. Is that correct?

6. Can you clarify: what would be an option to not have the bundled version? How can we fix the "invalid refs"?
Comment 30 Jonathan Druart 2022-03-17 10:44:14 UTC
https://swagger.io/docs/specification/using-ref/

"""
Places Where $ref Can Be Used
A common misconception is that $ref is allowed anywhere in an OpenAPI specification file. Actually $ref is only allowed in places where the OpenAPI 3.0 Specification explicitly states that the value may be a reference. For example, $ref cannot be used in the info section and directly under paths:

    openapi: 3.0.0
    # Incorrect!
    info:
      $ref: info.yaml
    paths:
      $ref: paths.yaml

However, you can $ref individual paths, like so:

    paths:
      /users:
        $ref: '../resources/users.yaml'
      /users/{userId}:
        $ref: '../resources/users-by-id.yaml'
"""

The fix does not seem hard to implement, why aren't we doing that?
Comment 31 Tomás Cohen Arazi 2022-03-17 10:51:55 UTC
(In reply to Jonathan Druart from comment #30
> The fix does not seem hard to implement, why aren't we doing that?


We are not on V3, but what you say is correct. Martin and I, independently, tried that route. Embedding the top level YAML files in swagger.yaml
That would solve the first barrier and I can submit that.
But then there's the ref resolution problem... We didn't manage to make it work. And the error messages were misleading and pointing to the library mixing definitions and paths.

I'll try to do it to share the results.
Comment 32 Tomás Cohen Arazi 2022-03-17 10:54:06 UTC
(In reply to Jonathan Druart from comment #28)
> Comment on attachment 131547 [details] [review] [review]
> Bug 30194: Update dependencies versions
> 
> Review of attachment 131547 [details] [review] [review]:
> -----------------------------------------------------------------
> 
> ::: cpanfile
> @@ +54,4 @@
> >  requires 'HTTP::Request::Common', '1.26';
> >  requires 'IPC::Cmd', '0.46';
> >  requires 'JSON', '2.07';
> > +requires 'JSON::Validator', '5.05';
> 
> I am not getting 5.05
> 
> % pmvers JSON::Validator
> 5.03
> 
> % apt show libjson-validator-perl
> Package: libjson-validator-perl
> Version: 5.99+really5.03-1

Umm. You are right. I think my env had the latest when I wrote that follow-up, in my attempt to make refs work. We can package the latest out just keep 5.03. Either works
Comment 33 Tomás Cohen Arazi 2022-03-17 14:13:29 UTC
(In reply to Jonathan Druart from comment #29)
> 1. gulp task could be "api_spec", "spec" it too generic IMO

I agree. We can also do what you proposed on IRC, using a single bundled file altogether.

> 2. Did you test with plugins injecting their own routes?

I tested the INN-Reach plugin (several routes), and the Coverflow plugin (static assets routes).

> 3. We need to document this on the wiki

Probably. Once we reach a consensus on things I can volunteer.

> 4. We need a check in the release tools, or we will get things out of sync
> 
> 5. Reference to api/swagger-v2-schema.json has been removed from
> Koha/REST/V1.pm but still in Koha/REST/Plugin/PluginRoutes.pm. Is that
> correct?

I might have removed it because the spec is packaged my Mason on an openapi-specification

> 6. Can you clarify: what would be an option to not have the bundled version?
> How can we fix the "invalid refs"?

We just need to move the paths.yaml content into swagger.yaml, and probably the same for definitions and parameters. The latter are not actually used so can just be removed for now. Because we point to definitions.yaml and parameters.yaml directly from the paths. And x-koha-primitives... can just be removed as it doesn't help much.
Comment 34 Martin Renvoize 2022-03-17 14:17:06 UTC
The spec I was testing with is available here: https://gitlab.com/mrenvoize/koha-swagger

I think it's validly broken up
Comment 35 Jonathan Druart 2022-03-17 14:40:16 UTC
(In reply to Martin Renvoize from comment #34)
> The spec I was testing with is available here:
> https://gitlab.com/mrenvoize/koha-swagger
> 
> I think it's validly broken up

I've tested v3 on https://gitlab.com/joubu/Koha/-/commits/openapiv3
Same problem with refs.
Comment 36 Jonathan Druart 2022-03-17 14:43:07 UTC
(In reply to Jonathan Druart from comment #35)
> (In reply to Martin Renvoize from comment #34)
> > The spec I was testing with is available here:
> > https://gitlab.com/mrenvoize/koha-swagger
> > 
> > I think it's validly broken up
> 
> I've tested v3 on https://gitlab.com/joubu/Koha/-/commits/openapiv3
> Same problem with refs.

I was working on cities, and got the following error:
Warning: Could not load REST API spec bundle: /components/schemas/cities_yaml: /oneOf/0 Properties not allowed: get./components/schemas/cities_yaml/$ref: /oneOf/1 Missing property. at /usr/share/perl5/Mojolicious/Plugin/OpenAPI.pm line 27.

But "/components/schemas/cities_yaml" does not make sense
See the bundle file
2045   /cities:
2046     $ref: 'paths/cities.yaml' 
and I moved the "get:" route to api/v1/swagger/paths/cities.yaml

If I move it back to the bundle file, all is working correctly
And so I have no idea how to debug...
Comment 37 Tomás Cohen Arazi 2022-03-23 18:17:08 UTC
Created attachment 132069 [details] [review]
Bug 30194: Update dependencies versions

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 38 Tomás Cohen Arazi 2022-03-23 18:17:14 UTC
Created attachment 132070 [details] [review]
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

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 39 Tomás Cohen Arazi 2022-03-23 18:17:19 UTC
Created attachment 132071 [details] [review]
Bug 30194: Fix invalid spec

This patch fixes a problem with the spec.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 40 Tomás Cohen Arazi 2022-03-23 18:17:25 UTC
Created attachment 132072 [details] [review]
Bug 30194: (follow-up) Remove invalid tests

This patch removes tests for the wrong 'Accept' header situation. It is
the only thing I found I could do.

The situation is: the JSON::Validator + Mojolicious::Plugin::OpenAPI is
trapping this case and returning its own error code (400) and it also
builds its own message! So no point testing it, as it can change again.

I post it as a separate patch just in case someone finds a better way to
handle this and obsoletes the patch :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 41 Tomás Cohen Arazi 2022-03-23 18:17:31 UTC
Created attachment 132073 [details] [review]
Bug 30194: (26351 follow-up) Silence useless warning

Every place in which Koha::Plugin::Test is loaded, and a patron is
updated, this hook needs to be silenced.

To reproduce:
1. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/REST/Plugin/PluginRoutes.t
=> FAIL: There's a warn about patron_barcode_transform
2. Apply this patch
3. Repeat 1
=> SUCCESS: Tests pass!
4. Sign off :-D

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 42 Tomás Cohen Arazi 2022-03-23 18:20:22 UTC
JSON::Validator got finally patched (version 5.07) and we can now avoid bundling the spec. So I reorganized the patches and removed that bit.

The new JSON::Validator::Schema::OpenAPIv2 class includes the schema, so it doesn't need to be included anymore. Feel free to ask me to remove it if necessary.
Comment 43 Tomás Cohen Arazi 2022-03-23 18:24:33 UTC
This is ready y'all. Please test!
Comment 44 Mason James 2022-03-24 06:13:56 UTC
Created attachment 132097 [details] [review]
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

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Mason James <mtj@kohaaloha.com>
Comment 45 Mason James 2022-03-24 06:14:01 UTC
Created attachment 132098 [details] [review]
Bug 30194: Fix invalid spec

This patch fixes a problem with the spec.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Mason James <mtj@kohaaloha.com>
Comment 46 Mason James 2022-03-24 06:14:06 UTC
Created attachment 132099 [details] [review]
Bug 30194: (follow-up) Remove invalid tests

This patch removes tests for the wrong 'Accept' header situation. It is
the only thing I found I could do.

The situation is: the JSON::Validator + Mojolicious::Plugin::OpenAPI is
trapping this case and returning its own error code (400) and it also
builds its own message! So no point testing it, as it can change again.

I post it as a separate patch just in case someone finds a better way to
handle this and obsoletes the patch :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Mason James <mtj@kohaaloha.com>
Comment 47 Mason James 2022-03-24 06:14:11 UTC
Created attachment 132100 [details] [review]
Bug 30194: (26351 follow-up) Silence useless warning

Every place in which Koha::Plugin::Test is loaded, and a patron is
updated, this hook needs to be silenced.

To reproduce:
1. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/REST/Plugin/PluginRoutes.t
=> FAIL: There's a warn about patron_barcode_transform
2. Apply this patch
3. Repeat 1
=> SUCCESS: Tests pass!
4. Sign off :-D

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Mason James <mtj@kohaaloha.com>
Comment 48 Mason James 2022-03-24 06:20:31 UTC
(In reply to Tomás Cohen Arazi from comment #43)
> This is ready y'all. Please test!

this is working for me on ktd/master-d10, with json-validator v5.07

root@kohadevbox:koha(master)$ time  prove -r  t/db_dependent/Koha/REST
...
All tests successful.
Files=3, Tests=19, 82 wallclock secs ( 0.06 usr  0.04 sys + 69.18 cusr 12.53 csys = 81.81 CPU)
Result: PASS

real    1m22.692s
user    1m9.339s
sys     0m12.603s
Comment 49 Mason James 2022-03-24 06:23:55 UTC
(In reply to Tomás Cohen Arazi from comment #42)
> JSON::Validator got finally patched (version 5.07) and we can now avoid
> bundling the spec. So I reorganized the patches and removed that bit.

i've added json-val 5.07 to the following apt repo, for testing...
 deb http://debian.koha-community.org/koha-staging exp main

i'll add it to the following repo, once this patchset is pushed
 deb http://debian.koha-community.org/koha-staging dev main
Comment 50 Jonathan Druart 2022-03-24 07:56:11 UTC
Created attachment 132102 [details] [review]
Bug 30194: Auto generate swagger.yaml

It was not sorted correct and had inconsistencies.

use YAML;
use File::Slurp qw( write_file );
my $yaml = YAML::LoadFile('api/v1/swagger/swagger.yaml');
YAML::Bless($yaml)->keys([qw(swagger basePath definitions paths info tags )]);
$yaml = YAML::Dump($yaml);
$yaml =~ s|'|"|xmsg;
write_file('api/v1/swagger/swagger.yaml', $yaml);
Comment 51 Jonathan Druart 2022-03-24 07:56:32 UTC
Do we want this last patch?
Comment 52 Jonathan Druart 2022-03-24 08:06:37 UTC
(In reply to Tomás Cohen Arazi from comment #33)
> (In reply to Jonathan Druart from comment #29)
> > 5. Reference to api/swagger-v2-schema.json has been removed from
> > Koha/REST/V1.pm but still in Koha/REST/Plugin/PluginRoutes.pm. Is that
> > correct?
> 
> I might have removed it because the spec is packaged my Mason on an
> openapi-specification

This is still not clear for me.
It does not seem correct to embed it if it's brought by the package now.
Comment 53 Martin Renvoize 2022-03-24 08:27:31 UTC
Hmm, I'm not so sure about that last patch.. whilst I agree it's nice to sort the paths and definitions blocks.. I feel the like info block is better left in it's current sort order?

I also went a bit further in my spec cleaning.. not sure if it's better or worse, but it does get rid of a few more of our local odities.  https://gitlab.com/mrenvoize/koha-swagger/-/tree/clean
Comment 54 Martin Renvoize 2022-03-24 08:36:43 UTC
Did we break the html view of the spec?
Comment 55 Jonathan Druart 2022-03-24 09:38:45 UTC
Created attachment 132112 [details] [review]
Bug 30194: Auto generate swagger.yaml

It was not sorted correct and had inconsistencies.

use YAML;
use File::Slurp qw( write_file );
my $hash = YAML::LoadFile('api/v1/swagger/swagger.yaml');
YAML::Bless($hash)->keys([qw(swagger basePath definitions paths info tags)]);
my $info = $hash->{info};
YAML::Bless($info)->keys([qw(title version license contact description)]);
my $yaml= YAML::Dump($hash);
$yaml =~ s|'|"|xmsg;
write_file('api/v1/swagger/swagger.yaml', $yaml);
Comment 56 Jonathan Druart 2022-03-24 09:39:34 UTC
(In reply to Martin Renvoize from comment #53)
> Hmm, I'm not so sure about that last patch.. whilst I agree it's nice to
> sort the paths and definitions blocks.. I feel the like info block is better
> left in it's current sort order?

Adjusted.
Comment 57 Martin Renvoize 2022-03-24 10:26:17 UTC
Created attachment 132116 [details] [review]
Bug 30194: Simply OpenAPI Specification

This patch simplifies the specification directory further by doing the
following:

* Inlines paths.yml into swagger.yaml
* Inlines definitions.yaml into swagger.yaml
* Inlines parameters.yaml into swagger.yaml
* Drops x-primitives.yaml
Comment 58 Martin Renvoize 2022-03-24 11:22:03 UTC
OK, I think this still needs a few tweaks and I'm working on those.

I can see the spec structure as it stands causing some challenges in maintenance.

Right now, we have definitions.yaml as an index file AND that same index duplicated inside swagger.yaml.. same for paths and others.  The reasoning for this is that the files within paths/ refer up to definitions.yaml and back down into the definitions/ directory of files.. and we also require the index at the top swagger.yaml file to be swagger valid.

I created a patch that simplifies the spec correcting this, but it appears our plugin routes stuff fails using it :(.
Comment 59 Martin Renvoize 2022-03-24 11:33:28 UTC
Created attachment 132118 [details] [review]
Bug 30194: (follow-up) Simply OpenAPI Specification

This patch simplifies the specification directory further by doing the
following:

* Inlines paths.yml into swagger.yaml
* Inlines definitions.yaml into swagger.yaml
* Inlines parameters.yaml into swagger.yaml
* Drops x-primitives.yaml
Comment 60 Martin Renvoize 2022-03-24 11:33:33 UTC
Created attachment 132119 [details] [review]
Bug 30194: (follow-up) Remove reference to swagger-v2-schema

This file is now bundled directly an no longer needs to be refered to
separately.
Comment 61 Martin Renvoize 2022-03-24 11:35:17 UTC
Created attachment 132120 [details] [review]
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

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Mason James <mtj@kohaaloha.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 62 Martin Renvoize 2022-03-24 11:35:22 UTC
Created attachment 132121 [details] [review]
Bug 30194: Fix invalid spec

This patch fixes a problem with the spec.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Mason James <mtj@kohaaloha.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 63 Martin Renvoize 2022-03-24 11:35:27 UTC
Created attachment 132122 [details] [review]
Bug 30194: (follow-up) Remove invalid tests

This patch removes tests for the wrong 'Accept' header situation. It is
the only thing I found I could do.

The situation is: the JSON::Validator + Mojolicious::Plugin::OpenAPI is
trapping this case and returning its own error code (400) and it also
builds its own message! So no point testing it, as it can change again.

I post it as a separate patch just in case someone finds a better way to
handle this and obsoletes the patch :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Mason James <mtj@kohaaloha.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 64 Martin Renvoize 2022-03-24 11:35:32 UTC
Created attachment 132123 [details] [review]
Bug 30194: (26351 follow-up) Silence useless warning

Every place in which Koha::Plugin::Test is loaded, and a patron is
updated, this hook needs to be silenced.

To reproduce:
1. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/REST/Plugin/PluginRoutes.t
=> FAIL: There's a warn about patron_barcode_transform
2. Apply this patch
3. Repeat 1
=> SUCCESS: Tests pass!
4. Sign off :-D

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Mason James <mtj@kohaaloha.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 65 Martin Renvoize 2022-03-24 11:35:37 UTC
Created attachment 132124 [details] [review]
Bug 30194: Auto generate swagger.yaml

It was not sorted correct and had inconsistencies.

use YAML;
use File::Slurp qw( write_file );
my $hash = YAML::LoadFile('api/v1/swagger/swagger.yaml');
YAML::Bless($hash)->keys([qw(swagger basePath definitions parameters paths info tags)]);
my $info = $hash->{info};
YAML::Bless($info)->keys([qw(title version license contact description)]);
my $yaml= YAML::Dump($hash);
$yaml =~ s|'|"|xmsg;
write_file('api/v1/swagger/swagger.yaml', $yaml);

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 66 Martin Renvoize 2022-03-24 11:35:42 UTC
Created attachment 132125 [details] [review]
Bug 30194: (follow-up) Simply OpenAPI Specification

This patch simplifies the specification directory further by doing the
following:

* Inlines paths.yml into swagger.yaml
* Inlines definitions.yaml into swagger.yaml
* Inlines parameters.yaml into swagger.yaml
* Drops x-primitives.yaml

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 67 Martin Renvoize 2022-03-24 11:35:46 UTC
Created attachment 132126 [details] [review]
Bug 30194: (follow-up) Remove reference to swagger-v2-schema

This file is now bundled directly an no longer needs to be refered to
separately.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 68 Martin Renvoize 2022-03-24 11:37:48 UTC
So I fixed the issue mentioned in my last comment.. it was a bug after I sorted my updated swagger.yaml again using Jonathans mentioned quick script (I neglected to add the 'parameters' piece in).

All working now, tests run and pass, API endpoints tested and working, plugins tested and working :).

Only think I can't seem to work out is I get a 403 when trying to visit /api/v1/.html now.. so the HTML render of the spec does appear to be broken..
Comment 69 Martin Renvoize 2022-03-24 11:50:17 UTC
OK, the spec rendering works too.. looks like it's a local apache config thing on my dev env.. trying in ktd and it works nicely..
Comment 70 Martin Renvoize 2022-03-24 13:30:43 UTC
Created attachment 132151 [details] [review]
Bug 30194: (follow-up) Simply OpenAPI Specification

This patch simplifies the specification directory further by doing the
following:

* Inlines paths.yml into swagger.yaml
* Inlines definitions.yaml into swagger.yaml
* Inlines parameters.yaml into swagger.yaml
* Drops x-primitives.yaml

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 71 Martin Renvoize 2022-03-24 13:30:50 UTC
Created attachment 132152 [details] [review]
Bug 30194: (follow-up) Remove reference to swagger-v2-schema

This file is now bundled directly an no longer needs to be refered to
separately.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 72 Jonathan Druart 2022-03-25 14:08:04 UTC
Should we take advantage of this big diff in the specs to standardize (order+quotes) all the yaml spec files?
Comment 73 Tomás Cohen Arazi 2022-03-25 19:36:48 UTC
Created attachment 132252 [details] [review]
Bug 30194: Auto generate swagger.yaml

It was not sorted correct and had inconsistencies.

use YAML;
use File::Slurp qw( write_file );
my $hash = YAML::LoadFile('api/v1/swagger/swagger.yaml');
YAML::Bless($hash)->keys([qw(swagger basePath definitions parameters paths info tags)]);
my $info = $hash->{info};
YAML::Bless($info)->keys([qw(title version license contact description)]);
my $yaml= YAML::Dump($hash);
$yaml =~ s|'|"|xmsg;
write_file('api/v1/swagger/swagger.yaml', $yaml);

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 74 Tomás Cohen Arazi 2022-03-25 19:37:00 UTC
Created attachment 132253 [details] [review]
Bug 30194: (follow-up) Simply OpenAPI Specification

This patch simplifies the specification directory further by doing the
following:

* Inlines paths.yml into swagger.yaml
* Inlines definitions.yaml into swagger.yaml
* Inlines parameters.yaml into swagger.yaml
* Drops x-primitives.yaml

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 75 Tomás Cohen Arazi 2022-03-25 19:37:06 UTC
Created attachment 132254 [details] [review]
Bug 30194: (follow-up) Remove reference to swagger-v2-schema

This file is now bundled directly an no longer needs to be refered to
separately.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 76 Tomás Cohen Arazi 2022-03-25 19:38:14 UTC
Tested with:

JSON::Validator@5.08
Mojolicious::Plugin::OpenAPI@5.05

installed via cpanm for now.

All works, tests pass. Love the follow-ups! Go team!
Comment 77 Tomás Cohen Arazi 2022-03-25 19:43:45 UTC
Reverting to SO, I thought Jonathan already stamped his signature on this. I added my signature to the correct and cool follow-ups.
Comment 78 Jonathan Druart 2022-03-28 14:49:23 UTC
(In reply to Jonathan Druart from comment #72)
> Should we take advantage of this big diff in the specs to standardize
> (order+quotes) all the yaml spec files?

Opened bug 30385.
Comment 79 Jonathan Druart 2022-03-28 14:59:37 UTC
Created attachment 132334 [details] [review]
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

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Mason James <mtj@kohaaloha.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 80 Jonathan Druart 2022-03-28 14:59:51 UTC
Created attachment 132335 [details] [review]
Bug 30194: Fix invalid spec

This patch fixes a problem with the spec.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Mason James <mtj@kohaaloha.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 81 Jonathan Druart 2022-03-28 14:59:59 UTC
Created attachment 132336 [details] [review]
Bug 30194: (follow-up) Remove invalid tests

This patch removes tests for the wrong 'Accept' header situation. It is
the only thing I found I could do.

The situation is: the JSON::Validator + Mojolicious::Plugin::OpenAPI is
trapping this case and returning its own error code (400) and it also
builds its own message! So no point testing it, as it can change again.

I post it as a separate patch just in case someone finds a better way to
handle this and obsoletes the patch :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Mason James <mtj@kohaaloha.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 82 Jonathan Druart 2022-03-28 15:00:07 UTC
Created attachment 132337 [details] [review]
Bug 30194: (26351 follow-up) Silence useless warning

Every place in which Koha::Plugin::Test is loaded, and a patron is
updated, this hook needs to be silenced.

To reproduce:
1. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/REST/Plugin/PluginRoutes.t
=> FAIL: There's a warn about patron_barcode_transform
2. Apply this patch
3. Repeat 1
=> SUCCESS: Tests pass!
4. Sign off :-D

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Mason James <mtj@kohaaloha.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 83 Jonathan Druart 2022-03-28 15:00:22 UTC
Created attachment 132338 [details] [review]
Bug 30194: Auto generate swagger.yaml

It was not sorted correct and had inconsistencies.

use YAML;
use File::Slurp qw( write_file );
my $hash = YAML::LoadFile('api/v1/swagger/swagger.yaml');
YAML::Bless($hash)->keys([qw(swagger basePath definitions parameters paths info tags)]);
my $info = $hash->{info};
YAML::Bless($info)->keys([qw(title version license contact description)]);
my $yaml= YAML::Dump($hash);
$yaml =~ s|'|"|xmsg;
write_file('api/v1/swagger/swagger.yaml', $yaml);

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 84 Jonathan Druart 2022-03-28 15:00:53 UTC
Created attachment 132339 [details] [review]
Bug 30194: (follow-up) Simply OpenAPI Specification

This patch simplifies the specification directory further by doing the
following:

* Inlines paths.yml into swagger.yaml
* Inlines definitions.yaml into swagger.yaml
* Inlines parameters.yaml into swagger.yaml
* Drops x-primitives.yaml

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 85 Jonathan Druart 2022-03-28 15:01:01 UTC
Created attachment 132340 [details] [review]
Bug 30194: (follow-up) Remove reference to swagger-v2-schema

This file is now bundled directly an no longer needs to be refered to
separately.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 86 Mason James 2022-03-29 04:08:40 UTC
root@kohadevbox:koha(master)$ cat /etc/debian_version
bookworm/sid

root@kohadevbox:koha(master)$ dpkg -l | grep really
ii  libjson-validator-perl                 5.99+really5.08-koha1  
ii  libmojolicious-perl                    9.90+really8.12-koha1  
ii  libmojolicious-plugin-openapi-perl     5.99+really5.05-koha1  


root@kohadevbox:koha(master)$  prove t/db_dependent/Koha/REST/Plugin/PluginRoutes.t \
>            t/db_dependent/api/v1
t/db_dependent/Koha/REST/Plugin/PluginRoutes.t .........     # Looks like you planned 3 tests but ran 1.
t/db_dependent/Koha/REST/Plugin/PluginRoutes.t ......... 1/4
#   Failed test 'Bad plugins tests'
#   at t/db_dependent/Koha/REST/Plugin/PluginRoutes.t line 85.
Can't locate object method "methods" via package "Mojolicious::Routes::Route" at t/db_dependent/Koha/REST/Plugin/PluginRoutes.t line 269.
# Looks like your test exited with 255 just after 1.
t/db_dependent/Koha/REST/Plugin/PluginRoutes.t ......... Dubious, test returned 255 (wstat 65280, 0xff00)
Failed 4/4 subtests
t/db_dependent/api/v1/acquisitions_funds.t ............. ok
t/db_dependent/api/v1/acquisitions_orders.t ............ ok
t/db_dependent/api/v1/acquisitions_vendors.t ........... ok
t/db_dependent/api/v1/advanced_editor_macros.t ......... ok
t/db_dependent/api/v1/article_requests.t ............... ok
t/db_dependent/api/v1/auth.t ........................... 1/3
    #   Failed test '200 OK'
    #   at t/db_dependent/api/v1/auth.t line 133.
    #          got: '404'
    #     expected: '200'
    # Looks like you failed 1 test of 4.
t/db_dependent/api/v1/auth.t ........................... 3/3
#   Failed test 'spec retrieval tests'
#   at t/db_dependent/api/v1/auth.t line 138.
# Looks like you failed 1 test of 3.
t/db_dependent/api/v1/auth.t ........................... Dubious, test returned 1 (wstat 256, 0x100)
Failed 1/3 subtests
t/db_dependent/api/v1/auth_authenticate_api_request.t .. ok
t/db_dependent/api/v1/auth_basic.t ..................... ok
t/db_dependent/api/v1/biblios.t ........................ ok
t/db_dependent/api/v1/cashups.t ........................ ok
t/db_dependent/api/v1/checkouts.t ...................... ok
t/db_dependent/api/v1/cities.t ......................... ok
t/db_dependent/api/v1/clubs_holds.t .................... ok
t/db_dependent/api/v1/holds.t .......................... ok
t/db_dependent/api/v1/illrequests.t .................... ok
t/db_dependent/api/v1/import_batch_profiles.t .......... ok
t/db_dependent/api/v1/items.t .......................... ok
t/db_dependent/api/v1/libraries.t ...................... ok
t/db_dependent/api/v1/oauth.t .......................... ok
t/db_dependent/api/v1/pagination.t ..................... ok
t/db_dependent/api/v1/patrons.t ........................ ok
t/db_dependent/api/v1/patrons_accounts.t ............... ok
t/db_dependent/api/v1/patrons_extended_attributes.t .... ok
t/db_dependent/api/v1/patrons_holds.t .................. ok
t/db_dependent/api/v1/patrons_password.t ............... ok
t/db_dependent/api/v1/return_claims.t .................. ok
t/db_dependent/api/v1/smtp_servers.t ................... ok
t/db_dependent/api/v1/stockrotationstage.t ............. ok
t/db_dependent/api/v1/suggestions.t .................... ok
t/db_dependent/api/v1/transfer_limits.t ................ ok
t/db_dependent/api/v1/unhandled_exceptions.t ........... ok

Test Summary Report
-------------------
t/db_dependent/Koha/REST/Plugin/PluginRoutes.t       (Wstat: 65280 Tests: 1 Failed: 1)
  Failed test:  1
  Non-zero exit status: 255
  Parse errors: Bad plan.  You planned 4 tests but ran 1.
t/db_dependent/api/v1/auth.t                         (Wstat: 256 Tests: 3 Failed: 1)
  Failed test:  3
  Non-zero exit status: 1
Files=32, Tests=218, 396 wallclock secs ( 0.32 usr  0.30 sys + 313.25 cusr 75.65 csys = 389.52 CPU)
Result: FAIL
Comment 87 Mason James 2022-03-29 04:11:16 UTC
> prove -r t/db_dependent/Koha/REST/Plugin/PluginRoutes.t \
>          t/db_dependent/api/v1

hi folks, i get a FAIL on this patchset with the new packages, on deb12

...oops!
Comment 88 Tomás Cohen Arazi 2022-03-29 07:47:44 UTC
(In reply to Mason James from comment #87)
> > prove -r t/db_dependent/Koha/REST/Plugin/PluginRoutes.t \
> >          t/db_dependent/api/v1
> 
> hi folks, i get a FAIL on this patchset with the new packages, on deb12
> 
> ...oops!

It seems that the master-bookworm image still has Mojolicious 8.12. As soon as I added the 'exp' repo and updated it (now 9.22) tests pass as on the other distributions.
Comment 89 Mason James 2022-03-29 13:06:50 UTC
(In reply to Tomás Cohen Arazi from comment #88)
> (In reply to Mason James from comment #87)
> > > prove -r t/db_dependent/Koha/REST/Plugin/PluginRoutes.t \
> > >          t/db_dependent/api/v1
> > 
> > hi folks, i get a FAIL on this patchset with the new packages, on deb12
> > 
> > ...oops!
> 
> It seems that the master-bookworm image still has Mojolicious 8.12. As soon
> as I added the 'exp' repo and updated it (now 9.22) tests pass as on the
> other distributions.

thanks, great news!
Tomás, can you test the following please

$ prove xt/api.t
Comment 90 Martin Renvoize 2022-03-29 14:06:10 UTC
Created attachment 132442 [details] [review]
Bug 30194: Add missing additionalProperties to q_body

This gets xt/api.t passing again.
Comment 91 Martin Renvoize 2022-03-29 14:06:51 UTC
This last patch resolves the final outstanding issue Mason found :)
Comment 92 Fridolin Somers 2022-03-29 14:51:29 UTC
Pushed to master for 22.05, thanks to everybody involved 🦄
Comment 93 Tomás Cohen Arazi 2022-03-29 15:13:44 UTC
Created attachment 132449 [details] [review]
Bug 30194: (follow-up) Fix xt/api.t

The query object can have an arbitrary structure, and thus it doesn't
make sense to check for the additionalProperties presence. also, it is
not allowed by OpenAPIv2 :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 94 Fridolin Somers 2022-03-29 15:14:05 UTC
Reverted "Bug 30194: Add missing additionalProperties to q_body"
Comment 95 Fridolin Somers 2022-03-29 15:18:18 UTC
Bug 30194: (follow-up) Fix xt/api.t
Pushed to master
Comment 96 Ere Maijala 2022-04-04 09:54:40 UTC
I think the relnotes should be expanded a bit since this affects backwards-compatibility. At least the fact that the validation is now stricter and doesn't allow the invalid refs it used to should be mentioned, as these can have a direct effect on REST API plugins.
Comment 97 Tomás Cohen Arazi 2022-04-05 08:49:20 UTC
(In reply to Ere Maijala from comment #96)
> I think the relnotes should be expanded a bit since this affects
> backwards-compatibility. At least the fact that the validation is now
> stricter and doesn't allow the invalid refs it used to should be mentioned,
> as these can have a direct effect on REST API plugins.

Indeed. We will need to add technical release notes.
Comment 98 Marcel de Rooy 2022-04-07 06:15:41 UTC
How long do we need the 'exp' workaround for dev environments ?
Comment 99 Mason James 2022-04-08 06:08:20 UTC
(In reply to Marcel de Rooy from comment #98)
> How long do we need the 'exp' workaround for dev environments ?

we can remove the 'exp' repo now
Comment 100 Michael Hafen 2022-04-12 20:38:54 UTC
Please keep the exp repo until the dev repo has the newer packages for all the affected libraries.
Also, shouldn't the required module versions be updated on the About Koha page?
Also, I seem to have the wrong version of Mojolicious::Plugin::OpenAPI in my dev install.  It's complaining about JSON::Validator::Ref being missing.  I have 2.16 from the dev repo.
Comment 101 Tomás Cohen Arazi 2022-04-12 23:03:19 UTC
(In reply to Michael Hafen from comment #100)
> Please keep the exp repo until the dev repo has the newer packages for all
> the affected libraries.
> Also, shouldn't the required module versions be updated on the About Koha
> page?
> Also, I seem to have the wrong version of Mojolicious::Plugin::OpenAPI in my
> dev install.  It's complaining about JSON::Validator::Ref being missing.  I
> have 2.16 from the dev repo.

Please pull the latest docker images, and if it doesn't ship the right versions, report back with more info so we fix the repo.
Comment 102 Mason James 2022-04-13 02:17:58 UTC
(In reply to Michael Hafen from comment #100)
> Please keep the exp repo until the dev repo has the newer packages for all
> the affected libraries.
> Also, shouldn't the required module versions be updated on the About Koha
> page?
> Also, I seem to have the wrong version of Mojolicious::Plugin::OpenAPI in my
> dev install.  It's complaining about JSON::Validator::Ref being missing.  I
> have 2.16 from the dev repo.

hi Michael, if you use the 'koha-staging' repo, your problems should be resolved
Comment 103 Michael Hafen 2022-04-13 18:27:38 UTC
Yep, that fixed it.  Thanks.

(In reply to Mason James from comment #102)
> (In reply to Michael Hafen from comment #100)
> > Please keep the exp repo until the dev repo has the newer packages for all
> > the affected libraries.
> > Also, shouldn't the required module versions be updated on the About Koha
> > page?
> > Also, I seem to have the wrong version of Mojolicious::Plugin::OpenAPI in my
> > dev install.  It's complaining about JSON::Validator::Ref being missing.  I
> > have 2.16 from the dev repo.
> 
> hi Michael, if you use the 'koha-staging' repo, your problems should be
> resolved