Bug 28189

Summary: Move the base swagger file to YAML
Product: Koha Reporter: Tomás Cohen Arazi (tcohen) <tomascohen>
Component: REST APIAssignee: Tomás Cohen Arazi (tcohen) <tomascohen>
Status: CLOSED FIXED QA Contact:
Severity: enhancement    
Priority: P5 - low CC: dcook, fridolin.somers, jonathan.druart, kyle, martin.renvoize
Version: Main   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
21.05.00
Circulation function:
Bug Depends on: 28157    
Bug Blocks: 17314, 29072    
Attachments: Bug 28189: Move swagger file to YAML format
Bug 28189: Move swagger file to YAML format
Bug 28189: Move swagger file to YAML format
Bug 28189: (QA folloq-up) Fix fallback case
Bug 28189: Fix PluginRoutes.t

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