Bug 33227

Summary: OpenAPI validation is failing for paths/biblios.yaml
Product: Koha Reporter: Pedro Amorim <pedro.amorim>
Component: REST APIAssignee: Tomás Cohen Arazi <tomascohen>
Status: RESOLVED FIXED QA Contact:
Severity: normal    
Priority: P5 - low CC: agustinmoyano, lucas, martin.renvoize, tomascohen
Version: Main   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: Trivial patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
23.05.00,22.11.04
Bug Depends on: 31800    
Bug Blocks: 33250, 33328    
Attachments: Bug 32227: Unit tests
Bug 33227: Unit tests
Bug 33227: Unit tests
Bug 33227: Remove invalid spec and adjust the code accordingly
Bug 33227: Unit tests
Bug 33227: Remove invalid spec and adjust the code accordingly
Bug 33227: (follow-up) Improve docs
Bug 33227: (follow-up) Improve docs

Description Pedro Amorim 2023-03-14 15:42:54 UTC
Running
swagger-cli validate swagger.yaml

we get:

Validation failed. /paths/biblios/post/parameters/Body has an invalid type (string,object)
Comment 1 Tomás Cohen Arazi 2023-03-15 14:33:19 UTC Comment hidden (obsolete)
Comment 2 Tomás Cohen Arazi 2023-03-15 14:34:04 UTC
Created attachment 148214 [details] [review]
Bug 33227: Unit tests

This patch adds a unit test that launches the swagger-cli validator
against our spec.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 3 Martin Renvoize 2023-03-16 09:26:16 UTC
This is great!  It'll be a vast improvement to have our spec validated regularly like this :)
Comment 4 Martin Renvoize 2023-03-16 09:27:09 UTC
Confirmed working from this end so feel free to add my stamp if you're ready, Tomas.  Otherwise, I'll wait for the bug to go NSO.
Comment 5 Tomás Cohen Arazi 2023-03-16 15:13:31 UTC
Created attachment 148268 [details] [review]
Bug 33227: Unit tests

This patch adds a unit test that launches the swagger-cli validator
against our spec.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 6 Tomás Cohen Arazi 2023-03-16 15:13:34 UTC
Created attachment 148269 [details] [review]
Bug 33227: Remove invalid spec and adjust the code accordingly

The spec contains a non-standard definition for the body param. Removing
it from the spec makes us need to handle the 'body' manually in the case
of JSON data.

This patch basically does that. It also changes the uses of
$c->validation, which are discouraged by the
Mojolicious::Plugin::OpenAPI dev/maintainer. I do it to highlight what
we must do in other places and the fact that there's no behavior change.

To test:
1. Apply this patch
2. Run:
   $ ktd --shell
  k$ prove t/db_dependent/api/v1/biblios.t \
           xt/api.t
=> SUCCESS: Tests pass!
3. Sign off :-D
Comment 7 Martin Renvoize 2023-03-20 11:01:04 UTC
Created attachment 148407 [details] [review]
Bug 33227: Unit tests

This patch adds a unit test that launches the swagger-cli validator
against our spec.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 8 Martin Renvoize 2023-03-20 11:01:07 UTC
Created attachment 148408 [details] [review]
Bug 33227: Remove invalid spec and adjust the code accordingly

The spec contains a non-standard definition for the body param. Removing
it from the spec makes us need to handle the 'body' manually in the case
of JSON data.

This patch basically does that. It also changes the uses of
$c->validation, which are discouraged by the
Mojolicious::Plugin::OpenAPI dev/maintainer. I do it to highlight what
we must do in other places and the fact that there's no behavior change.

To test:
1. Apply this patch
2. Run:
   $ ktd --shell
  k$ prove t/db_dependent/api/v1/biblios.t \
           xt/api.t
=> SUCCESS: Tests pass!
3. Sign off :-D

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 9 Martin Renvoize 2023-03-20 11:06:34 UTC
This resolves the issues presented and causes now regressions.

However, I'd love to see some documentation added somewhere to the API docs that states we expect MARC records to be passed (in various formats).. at the moment, the docs aren't all that clear.

Anyways.. Passing QA to get us back on spec
Comment 10 Tomás Cohen Arazi 2023-03-20 11:55:56 UTC
Created attachment 148410 [details] [review]
Bug 33227: (follow-up) Improve docs

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 11 Martin Renvoize 2023-03-20 12:13:20 UTC
Created attachment 148411 [details] [review]
Bug 33227: (follow-up) Improve docs

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 12 Tomás Cohen Arazi 2023-03-20 12:43:20 UTC
Pushed to master for 23.05.

Nice work everyone, thanks!
Comment 13 Matt Blenkinsop 2023-03-21 16:41:54 UTC
Nice work everyone!

Pushed to stable for 22.11.x
Comment 14 Lucas Gass 2023-04-24 16:50:16 UTC
Doesn't apply cleanly in 22.05.x, if needed please rebase.