Bug 33145 - Invalid specification for ERM routes
Summary: Invalid specification for ERM routes
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: REST API (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low major (vote)
Assignee: Martin Renvoize
QA Contact: Tomás Cohen Arazi
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2023-03-06 10:01 UTC by Martin Renvoize
Modified: 2023-12-28 20:43 UTC (History)
5 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
23.05.00,22.11.04


Attachments
Bug 33145: Fix ERM swagger specs (3.88 KB, patch)
2023-03-06 10:04 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 33145: Fix ERM swagger specs (3.97 KB, patch)
2023-03-06 10:48 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 33145: Fix ERM swagger specs (4.10 KB, patch)
2023-03-06 12:41 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 Martin Renvoize 2023-03-06 10:01:03 UTC
I'm not sure when this crept in, but the {provider} path parameter isn't properly defined in most ERM endpoints and thus the swagger spec isn't valid as it stands.

You can test with using he 'swagger-cli' not module as such:

```
cd api/v1/swagger
swagger-cli validate swagger.yaml
```
Comment 1 Martin Renvoize 2023-03-06 10:04:34 UTC
Created attachment 147777 [details] [review]
Bug 33145: Fix ERM swagger specs

This patch adds the missing 'Provider name' parameter definition to the
various ERM paths that were missing it.

Test plan
1) Install swagger-cli using npm
2) cd api/v1/swagger
3) Run `swagger-cli validate swagger.yaml`
4) Confirm validation fails
5) Apply the patch here
6) Re-run the swagger-cli test above and confirm it now passes.
Comment 2 Martin Renvoize 2023-03-06 10:39:23 UTC
I must admit.. I thought we had a unit test to catch invalid specs generally.. so surprised this crept in.
Comment 3 Marcel de Rooy 2023-03-06 10:48:44 UTC
Created attachment 147780 [details] [review]
Bug 33145: Fix ERM swagger specs

This patch adds the missing 'Provider name' parameter definition to the
various ERM paths that were missing it.

Test plan
1) Install swagger-cli using npm
2) cd api/v1/swagger
3) Run `swagger-cli validate swagger.yaml`
4) Confirm validation fails
5) Apply the patch here
6) Re-run the swagger-cli test above and confirm it now passes.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 4 Marcel de Rooy 2023-03-06 10:49:16 UTC
Tomas: Please have a look here
Comment 5 Tomás Cohen Arazi 2023-03-06 12:25:18 UTC
I run this for validating, installing swagger-cli is failing for me in ktd:

docker run --rm -v $PWD/api/v1/swagger:/swagger jeanberu/swagger-cli swagger-cli validate swagger/swagger.yaml
Comment 6 Tomás Cohen Arazi 2023-03-06 12:41:48 UTC
Created attachment 147782 [details] [review]
Bug 33145: Fix ERM swagger specs

This patch adds the missing 'Provider name' parameter definition to the
various ERM paths that were missing it.

Test plan
1) Run:
   docker run --rm -v $PWD/api/v1/swagger:/swagger \
          jeanberu/swagger-cli \
          swagger-cli validate swagger/swagger.yaml
=> FAIL: The spec is not valid!
2) Apply this patch
3) Repeat 1
=> SUCCESS: Tests pass!

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Edit: tcohen edited the test plan because of the failure to install
swagger-cli cleanly
Comment 7 Tomás Cohen Arazi 2023-03-06 12:49:14 UTC
Pushed to master for 23.05.

Nice work everyone, thanks!
Comment 8 Jonathan Druart 2023-03-07 10:47:32 UTC
Don't we need a regression test?
Comment 9 Jacob O'Mara 2023-03-07 18:28:19 UTC
Nice work, thanks everyone!

Pushed to 22.11.x for the next release.
Comment 10 Lucas Gass 2023-03-10 16:28:12 UTC
Not needed in 22.05.x