Bug 30194

*** Bug 26893 has been marked as a duplicate of this bug. ***

*** Bug 30193 has been marked as a duplicate of this bug. ***

Created attachment 131497 [details] [review]
Bug 30194: Update dependencies versions

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

Created attachment 131499 [details] [review]
Bug 30194: Fix invalid spec

This patch fixes a problem with the spec.

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

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>

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

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.

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...)

(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.

(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.

(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

(In reply to David Cook from comment #14)
> Sounds good to me.

Can you test it?

(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.)

Created attachment 131547 [details] [review]
Bug 30194: Update dependencies versions

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

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>

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>

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>

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>

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>

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

(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 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

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"?

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?

(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.

(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

(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.

The spec I was testing with is available here: https://gitlab.com/mrenvoize/koha-swagger

I think it's validly broken up

(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.

(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...

Created attachment 132069 [details] [review]
Bug 30194: Update dependencies versions

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

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>

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>

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>

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>

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.

This is ready y'all. Please test!

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>

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>

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>

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>

(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

(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

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);

Do we want this last patch?

(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.

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

Did we break the html view of the spec?

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);

(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.

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

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 :(.

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

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.

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>

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>

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>

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>

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>

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>

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>

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..

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..

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>

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>

Should we take advantage of this big diff in the specs to standardize (order+quotes) all the yaml spec files?

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>

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>

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>

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!

Reverting to SO, I thought Jonathan already stamped his signature on this. I added my signature to the correct and cool follow-ups.

(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.

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>

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>

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>

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>

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>

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>

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>

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

> 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!

(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.

(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

Created attachment 132442 [details] [review]
Bug 30194: Add missing additionalProperties to q_body

This gets xt/api.t passing again.

This last patch resolves the final outstanding issue Mason found :)

Pushed to master for 22.05, thanks to everybody involved ﷐[U+1F984]﷑

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>

Reverted "Bug 30194: Add missing additionalProperties to q_body"

Bug 30194: (follow-up) Fix xt/api.t
Pushed to master

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.

(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.

How long do we need the 'exp' workaround for dev environments ?

(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

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.

(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.

(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

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

Note that this introduces a regression/vulnerability that bug 23653 originally fixed. 

If json-schema.org is down, Koha's API won't work. 

See Comment 2 on bug 36721. I'll open a new bug to address this as well...