Bug 28189 - Move the base swagger file to YAML
Summary: Move the base swagger file to YAML
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: REST API (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement
Assignee: Tomás Cohen Arazi (tcohen)
QA Contact:
URL:
Keywords:
Depends on: 28157
Blocks: 17314 29072
  Show dependency treegraph
 
Reported: 2021-04-21 17:18 UTC by Tomás Cohen Arazi (tcohen)
Modified: 2022-06-06 20:29 UTC (History)
5 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
21.05.00
Circulation function:


Attachments
Bug 28189: Move swagger file to YAML format (3.16 KB, patch)
2021-04-21 17:32 UTC, Tomás Cohen Arazi (tcohen)
Details | Diff | Splinter Review
Bug 28189: Move swagger file to YAML format (3.21 KB, patch)
2021-04-22 13:37 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
Bug 28189: Move swagger file to YAML format (3.29 KB, patch)
2021-04-22 14:07 UTC, Kyle M Hall (khall)
Details | Diff | Splinter Review
Bug 28189: (QA folloq-up) Fix fallback case (874 bytes, patch)
2021-05-10 11:36 UTC, Tomás Cohen Arazi (tcohen)
Details | Diff | Splinter Review
Bug 28189: Fix PluginRoutes.t (1.60 KB, patch)
2021-05-17 14:09 UTC, Jonathan Druart
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 (tcohen) 2021-04-21 17:18:18 UTC
In order to be able to add better documentation on the API, we should move the base file to YAML, so we can add multi-line Markdown in it. It renders correctly with tools like Redoc.
Comment 1 Tomás Cohen Arazi (tcohen) 2021-04-21 17:19:41 UTC
I decided to make this dependent on bug 28157, which would really benefit from adding some documentation like this. So it is a good first addition.
Comment 2 Tomás Cohen Arazi (tcohen) 2021-04-21 17:32:16 UTC
Created attachment 119963 [details] [review]
Bug 28189: Move swagger file to YAML format

This patch changes the base OpenAPI file (swagger.json) into YAML. The
motivation for this, is adding more documentation, in Markdown.

JSON doesn't accept multiline strings, so this seems like a good move
for readability.

To test:
1. Verify your API is functional
2. Apply this patch
3. Repeat 1
=> SUCCESS: No changes, really
4. Point your browser to /api/v1/.html
=> SUCCESS: Some nicely formatted description of the API can be seen. It
includes information about x-koha-library.
5. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 3 David Cook 2021-04-22 06:52:41 UTC
No argument here. YAML is way nicer to work with as a human.
Comment 4 Martin Renvoize (ashimema) 2021-04-22 13:37:59 UTC
Created attachment 120004 [details] [review]
Bug 28189: Move swagger file to YAML format

This patch changes the base OpenAPI file (swagger.json) into YAML. The
motivation for this, is adding more documentation, in Markdown.

JSON doesn't accept multiline strings, so this seems like a good move
for readability.

To test:
1. Verify your API is functional
2. Apply this patch
3. Repeat 1
=> SUCCESS: No changes, really
4. Point your browser to /api/v1/.html
=> SUCCESS: Some nicely formatted description of the API can be seen. It
includes information about x-koha-library.
5. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 5 Martin Renvoize (ashimema) 2021-04-22 13:38:37 UTC
Tested the api, and api docs and they both work as expected..

A positive change.. going straight for QA.

Passed
Comment 6 Kyle M Hall (khall) 2021-04-22 14:07:40 UTC
Created attachment 120005 [details] [review]
Bug 28189: Move swagger file to YAML format

This patch changes the base OpenAPI file (swagger.json) into YAML. The
motivation for this, is adding more documentation, in Markdown.

JSON doesn't accept multiline strings, so this seems like a good move
for readability.

To test:
1. Verify your API is functional
2. Apply this patch
3. Repeat 1
=> SUCCESS: No changes, really
4. Point your browser to /api/v1/.html
=> SUCCESS: Some nicely formatted description of the API can be seen. It
includes information about x-koha-library.
5. Sign off :-D

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

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 7 Jonathan Druart 2021-04-30 07:46:33 UTC
Koha/REST/V1.pm:                $self->home->rel_file("api/v1/swagger/swagger.json"),


Not this one?
Comment 8 Martin Renvoize (ashimema) 2021-04-30 08:07:32 UTC
(In reply to Jonathan Druart from comment #7)
> Koha/REST/V1.pm:               
> $self->home->rel_file("api/v1/swagger/swagger.json"),
> 
> 
> Not this one?

That's interesting.. I didn't look deep into the code when I tested.. I just tested that routes continued to work and that the docs route continued to work.. and in both cases they worked fine.. seems the plugin doesn't care about the file extension.. that's surprising..

I agree though.. it should be corrected here.. well spotted.
Comment 9 David Cook 2021-05-02 23:11:22 UTC
I'm not sure I'm following the flow but I'm guessing Jonathan was referring to th swagger.json line in the fallback section?
Comment 10 Tomás Cohen Arazi (tcohen) 2021-05-02 23:25:39 UTC
(In reply to David Cook from comment #9)
> I'm not sure I'm following the flow but I'm guessing Jonathan was referring
> to th swagger.json line in the fallback section?

Yes, it was a busy Friday. Will fix it tomorrow.
Comment 11 Tomás Cohen Arazi (tcohen) 2021-05-10 11:36:14 UTC
Created attachment 120766 [details] [review]
Bug 28189: (QA folloq-up) Fix fallback case

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 12 Tomás Cohen Arazi (tcohen) 2021-05-10 11:36:58 UTC
Sorry for the delay.
Comment 13 Jonathan Druart 2021-05-12 15:47:09 UTC
Pushed to master for 21.05, thanks to everybody involved!
Comment 14 Jonathan Druart 2021-05-17 10:22:19 UTC
t/db_dependent/Koha/REST/Plugin/PluginRoutes.t is failing, please fix ASAP.
Comment 15 Jonathan Druart 2021-05-17 13:21:28 UTC
In my understanding the tests are telling us that we are doing something terribly wrong here. We are removing support for routes injected by plugin (as they are written in json). Is that correct?
Comment 16 Jonathan Druart 2021-05-17 14:09:50 UTC
Created attachment 121053 [details] [review]
Bug 28189: Fix PluginRoutes.t

So, this one was hidden.

The failures were:
    #   Failed test 'Bad plugins raise warning'
    #   at t/db_dependent/Koha/REST/Plugin/PluginRoutes.t line 75.
    # found warning: Warning: Could not load REST API spec bundle: Invalid JSON specification HASH(0x556972b22da0):
    # found warning: Warning: Could not load REST API spec bundle: Invalid JSON specification HASH(0x5569735b8368):
    # expected to find warning: (?^u:Could not load REST API spec bundle: Invalid JSON specification)
    # expected to find warning: (?^u:The resulting spec is invalid. Skipping Bad API Route Plugin)

And the correct error was (after a debug warn in JSON::Validator):
Warning: Could not load REST API spec bundle: Invalid JSON specification HASH(0x55fd0c3d3160):
- /info/version: Expected string - got number. at /usr/share/perl5/JSON/Validator.pm line 165.

So this patch fixes it, but I don't understand why it's only failing for
plugin routes however.
Comment 17 Jonathan Druart 2021-05-17 14:20:22 UTC
Follow-up pushed to master.
Comment 18 Fridolin Somers 2021-05-18 10:00:16 UTC
Enhancement not pushed to 20.11.x