Bug 33227 - OpenAPI validation is failing for paths/biblios.yaml
Summary: OpenAPI validation is failing for paths/biblios.yaml
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: REST API (show other bugs)
Version: Main
Hardware: All All
: P5 - low normal (vote)
Assignee: Tomás Cohen Arazi
QA Contact:
URL:
Keywords:
Depends on: 31800
Blocks: 33250 33328
  Show dependency treegraph
 
Reported: 2023-03-14 15:42 UTC by Pedro Amorim
Modified: 2023-12-28 20:44 UTC (History)
4 users (show)

See Also:
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


Attachments
Bug 32227: Unit tests (1.46 KB, patch)
2023-03-15 14:33 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 33227: Unit tests (1.40 KB, patch)
2023-03-15 14:34 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 33227: Unit tests (1.40 KB, patch)
2023-03-16 15:13 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 33227: Remove invalid spec and adjust the code accordingly (8.05 KB, patch)
2023-03-16 15:13 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 33227: Unit tests (1.46 KB, patch)
2023-03-20 11:01 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 33227: Remove invalid spec and adjust the code accordingly (8.11 KB, patch)
2023-03-20 11:01 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 33227: (follow-up) Improve docs (2.05 KB, patch)
2023-03-20 11:55 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 33227: (follow-up) Improve docs (2.17 KB, patch)
2023-03-20 12:13 UTC, Martin Renvoize
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
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.