Bug 18137

Summary: Migrate from Mojolicious::Plugin::Swagger2 to Mojolicious::Plugin::OpenAPI
Product: Koha Reporter: Lari Taskula <lari.taskula>
Component: REST APIAssignee: Lari Taskula <lari.taskula>
Status: CLOSED FIXED QA Contact: Tomás Cohen Arazi <tomascohen>
Severity: enhancement    
Priority: P5 - low CC: dcook, fridolin.somers, jonathan.druart, josef.moravec, katrin.fischer, mirko, mtj, tomascohen
Version: masterKeywords: dependency
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: Medium patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Bug Depends on:    
Bug Blocks: 17424, 17479, 17505, 18407, 19037, 11897, 16497, 16825, 18120, 18205, 18282, 18290, 18731, 18890, 19072, 19196, 20595    
Attachments: Bug 18137: List Mojolicious::Plugin::OpenAPI as Perl dependency
Bug 18137: Add useful Koha::Exceptions
Bug 18137: Migrate from Swagger2 to Mojolicious::Plugin::OpenAPI
Bug 18137: Make /patrons Mojolicious::Plugin::OpenAPI compatible
Bug 18137: Make /cities Mojolicious::Plugin::OpenAPI compatible
Bug 18137: Make /holds Mojolicious::Plugin::OpenAPI compatible
Bug 18137: Remove x-mojo-around-action
Bug 18137: List Mojolicious::Plugin::OpenAPI as Perl dependency
Bug 18137: Add useful Koha::Exceptions
Bug 18137: Migrate from Swagger2 to Mojolicious::Plugin::OpenAPI
Bug 18137: Make /patrons Mojolicious::Plugin::OpenAPI compatible
Bug 18137: Make /cities Mojolicious::Plugin::OpenAPI compatible
Bug 18137: Make /holds Mojolicious::Plugin::OpenAPI compatible
Bug 18137: Remove x-mojo-around-action
Bug 18137: (follow-up) Fix handling DBIx::Class::Exception messages
Bug 18137: (QA-follow-up) Fix pod fail
Bug 18137: (follow-up) operationId must be unique
Bug 18137: (follow-up) Sort definitions.json
Bug 18137: List Mojolicious::Plugin::OpenAPI as Perl dependency
Bug 18137: Add useful Koha::Exceptions
Bug 18137: Migrate from Swagger2 to Mojolicious::Plugin::OpenAPI
Bug 18137: Make /patrons Mojolicious::Plugin::OpenAPI compatible
Bug 18137: Make /cities Mojolicious::Plugin::OpenAPI compatible
Bug 18137: Make /holds Mojolicious::Plugin::OpenAPI compatible
Bug 18137: Remove x-mojo-around-action
Bug 18137: (follow-up) Fix handling DBIx::Class::Exception messages
Bug 18137: (QA-follow-up) Fix pod fail
Bug 18137: (follow-up) Sort definitions.json
Bug 18137: List Mojolicious::Plugin::OpenAPI and JSON::Validator as dependencies
[SIGNED-OFF] Bug 18137: Add useful Koha::Exceptions
[SIGNED-OFF] Bug 18137: Migrate from Swagger2 to Mojolicious::Plugin::OpenAPI
[SIGNED-OFF] Bug 18137: Make /patrons Mojolicious::Plugin::OpenAPI compatible
[SIGNED-OFF] Bug 18137: Make /cities Mojolicious::Plugin::OpenAPI compatible
[SIGNED-OFF] Bug 18137: Make /holds Mojolicious::Plugin::OpenAPI compatible
[SIGNED-OFF] Bug 18137: Remove x-mojo-around-action
[SIGNED-OFF] Bug 18137: (follow-up) Fix handling DBIx::Class::Exception messages
[SIGNED-OFF] Bug 18137: (QA-follow-up) Fix pod fail
[SIGNED-OFF] Bug 18137: (follow-up) Sort definitions.json
[SIGNED-OFF] Bug 18137: List Mojolicious::Plugin::OpenAPI and JSON::Validator as dependencies
Bug 18137: Add useful Koha::Exceptions
Bug 18137: Migrate from Swagger2 to Mojolicious::Plugin::OpenAPI
Bug 18137: Make /patrons Mojolicious::Plugin::OpenAPI compatible
Bug 18137: Make /cities Mojolicious::Plugin::OpenAPI compatible
Bug 18137: Make /holds Mojolicious::Plugin::OpenAPI compatible
Bug 18137: Remove x-mojo-around-action
Bug 18137: (follow-up) Fix handling DBIx::Class::Exception messages
Bug 18137: (QA-follow-up) Fix pod fail
Bug 18137: (follow-up) Sort definitions.json
Bug 18137: List Mojolicious::Plugin::OpenAPI and JSON::Validator as dependencies
Bug 18137: Add useful Koha::Exceptions
Bug 18137: Migrate from Swagger2 to Mojolicious::Plugin::OpenAPI
Bug 18137: Make /patrons Mojolicious::Plugin::OpenAPI compatible
Bug 18137: Make /cities Mojolicious::Plugin::OpenAPI compatible
Bug 18137: Make /holds Mojolicious::Plugin::OpenAPI compatible
Bug 18137: Remove x-mojo-around-action
Bug 18137: (follow-up) Fix handling DBIx::Class::Exception messages
Bug 18137: (QA-follow-up) Fix pod fail
Bug 18137: (follow-up) Sort definitions.json
Bug 18137: List Mojolicious::Plugin::OpenAPI and JSON::Validator as dependencies
Bug 18137: List Mojolicious::Plugin::OpenAPI and JSON::Validator as dependencies
Bug 18137: Add useful Koha::Exceptions
Bug 18137: Migrate from Swagger2 to Mojolicious::Plugin::OpenAPI
Bug 18137: Make /patrons Mojolicious::Plugin::OpenAPI compatible
Bug 18137: Make /cities Mojolicious::Plugin::OpenAPI compatible
Bug 18137: Make /holds Mojolicious::Plugin::OpenAPI compatible
Bug 18137: Remove x-mojo-around-action
Bug 18137: (follow-up) Fix handling DBIx::Class::Exception messages
Bug 18137: (QA-follow-up) Fix pod fail
Bug 18137: (follow-up) Sort definitions.json
Bug 18137: List Mojolicious::Plugin::OpenAPI and JSON::Validator as dependencies
Bug 18137: List Mojolicious::Plugin::OpenAPI and JSON::Validator as dependencies
Bug 18137: (QA followup) Make sure the session exists and is expired on expiration tests
Bug 18137: (QA followup) Make sure the session exists and is expired on expiration tests
Bug 18137: Make /holds Mojolicious::Plugin::OpenAPI compatible
Bug 18137: Remove x-mojo-around-action
Bug 18137: (follow-up) Fix handling DBIx::Class::Exception messages
Bug 18137: (QA-follow-up) Fix pod fail
Bug 18137: (follow-up) Sort definitions.json
Bug 18137: List Mojolicious::Plugin::OpenAPI and JSON::Validator as dependencies
Bug 18137: (QA followup) Make sure the session exists and is expired on expiration tests
Bug 18137: Add useful Koha::Exceptions
Bug 18137: Migrate from Swagger2 to Mojolicious::Plugin::OpenAPI
Bug 18137: Make /patrons Mojolicious::Plugin::OpenAPI compatible
Bug 18137: Make /cities Mojolicious::Plugin::OpenAPI compatible
Bug 18137: Make /holds Mojolicious::Plugin::OpenAPI compatible
Bug 18137: Remove x-mojo-around-action
Bug 18137: (follow-up) Fix handling DBIx::Class::Exception messages
Bug 18137: (QA-follow-up) Fix pod fail
Bug 18137: (follow-up) Sort definitions.json
Bug 18137: List Mojolicious::Plugin::OpenAPI and JSON::Validator as dependencies
Bug 18137: (QA followup) Make sure the session exists and is expired on expiration tests

Description Lari Taskula 2017-02-17 11:16:22 UTC
As "the Swagger2 distribution is no longer actively maintained", migrate from Mojolicious::Plugin::Swagger2 to Mojolicious::Plugin::OpenAPI.
Comment 1 Lari Taskula 2017-02-21 13:09:49 UTC
Created attachment 60490 [details] [review]
Bug 18137: List Mojolicious::Plugin::OpenAPI as Perl dependency

This patch also upgrades Mojolicious version requirement to 6.40 as it is the
minimum requirement set by Mojolicious::Plugin::OpenAPI.

To test:
1. sudo cpanm Mojolicious::Plugin::OpenAPI@1.10
Comment 2 Lari Taskula 2017-02-21 13:10:12 UTC
Created attachment 60491 [details] [review]
Bug 18137: Add useful Koha::Exceptions
Comment 3 Lari Taskula 2017-02-21 13:10:39 UTC
Created attachment 60492 [details] [review]
Bug 18137: Migrate from Swagger2 to Mojolicious::Plugin::OpenAPI

This patch migrates from Swagger2 to Mojolicious::Plugin::OpenAPI as Swagger2 is
no longer actively maintained.

This migration involves some minor changes to our Swagger specification documents
and to controllers. Each operation is migrated in following patches separately.
Please see Mojolicious::Plugin::OpenAPI and its tutorial for more documentation.

The patch also refactors some API authentication -related code by taking advantage
of Koha::Exceptions. Authentication is now handled via Mojolicious's "under->to"
functionality. The actual authentication & authorization checks are moved to
Koha::REST::V1::Auth. Added a HTTP 503 response for when database update is
required, instead of returning an authentication failure as before.

To test:
1. prove t/db_dependent/api/v1/auth.t
Comment 4 Lari Taskula 2017-02-21 13:10:53 UTC
Created attachment 60493 [details] [review]
Bug 18137: Make /patrons Mojolicious::Plugin::OpenAPI compatible

Also:
- adding some missing and new response definitions into Swagger spec.
- fixing failing test due to Bug 17932's change of boolean values

To test:
1. prove t/db_dependent/api/v1/patrons.t
Comment 5 Lari Taskula 2017-02-21 13:11:07 UTC
Created attachment 60494 [details] [review]
Bug 18137: Make /cities Mojolicious::Plugin::OpenAPI compatible

Also:
- adding some missing and new response definitions into Swagger spec.

To test:
1. prove t/db_dependent/api/v1/cities.t
Comment 6 Lari Taskula 2017-02-21 13:11:26 UTC
Created attachment 60495 [details] [review]
Bug 18137: Make /holds Mojolicious::Plugin::OpenAPI compatible

Also
- adding some missing and new response definitions into Swagger spec.
- fixing failing tests due to Bug 17932's change of boolean values

To test:
1. prove t/db_dependent/api/v1/holds.t
Comment 7 Lari Taskula 2017-02-21 13:19:29 UTC
Mojolicious::Plugin::OpenAPI is at the time of this post at version 1.09 in CPAN, however I am having problems with the CPAN version, as follows: 

Could not load document from /home/ubuntu/kohaclone/api/v1/swagger/../parameters.json: Can't open file "/home/ubuntu/kohaclone/api/v1/swagger/../parameters.json": No such file or directory at /usr/local/share/perl/5.18.2/JSON/Validator.pm line 140.

Fortunately, it looks like Mojolicious::Plugin::OpenAPI version 1.10 (currently at unreleased status in GitHub) fixes this issue. So if you are testing this before 1.10 is released, I would suggest using the source provided directly in GitHub. https://github.com/jhthorsen/mojolicious-plugin-openapi
Comment 8 Lari Taskula 2017-02-21 17:56:58 UTC
Created attachment 60533 [details] [review]
Bug 18137: Remove x-mojo-around-action

Mojolicious::Plugin::OpenAPI does not support x-mojo-around action. This patch
removes it from our specification document.
Comment 9 Lari Taskula 2017-02-21 18:02:12 UTC
(In reply to Lari Taskula from comment #7)
> Mojolicious::Plugin::OpenAPI is at the time of this post at version 1.09 in
> CPAN, however I am having problems with the CPAN version, as follows: 
> 
> Could not load document from
> /home/ubuntu/kohaclone/api/v1/swagger/../parameters.json: Can't open file
> "/home/ubuntu/kohaclone/api/v1/swagger/../parameters.json": No such file or
> directory at /usr/local/share/perl/5.18.2/JSON/Validator.pm line 140.
> 
> Fortunately, it looks like Mojolicious::Plugin::OpenAPI version 1.10
> (currently at unreleased status in GitHub) fixes this issue. So if you are
> testing this before 1.10 is released, I would suggest using the source
> provided directly in GitHub.
> https://github.com/jhthorsen/mojolicious-plugin-openapi
It appears v 1.10 was released right after I posted this. So cpanm Mojolicious::Plugin::OpenAPI@1.10 should be fine.
Comment 10 Lari Taskula 2017-03-01 14:59:06 UTC
I would like to point out that I am experiencing an issue with JSON::Validator, where $refs are sometimes left unresolved in our spec and it causes some error messages and crashes. This issue is being tracked at https://github.com/jhthorsen/json-validator/pull/52.
Comment 11 Olli-Antti Kivilahti 2017-03-06 11:42:57 UTC
Created attachment 60837 [details] [review]
Bug 18137: List Mojolicious::Plugin::OpenAPI as Perl dependency

This patch also upgrades Mojolicious version requirement to 6.40 as it is the
minimum requirement set by Mojolicious::Plugin::OpenAPI.

To test:
1. sudo cpanm Mojolicious::Plugin::OpenAPI@1.10

Signed-off-by: Olli-Antti Kivilahti <olli-antti.kivilahti@jns.fi>
Comment 12 Olli-Antti Kivilahti 2017-03-06 11:43:21 UTC
Created attachment 60838 [details] [review]
Bug 18137: Add useful Koha::Exceptions

Signed-off-by: Olli-Antti Kivilahti <olli-antti.kivilahti@jns.fi>
Comment 13 Olli-Antti Kivilahti 2017-03-06 11:43:40 UTC
Created attachment 60839 [details] [review]
Bug 18137: Migrate from Swagger2 to Mojolicious::Plugin::OpenAPI

This patch migrates from Swagger2 to Mojolicious::Plugin::OpenAPI as Swagger2 is
no longer actively maintained.

This migration involves some minor changes to our Swagger specification documents
and to controllers. Each operation is migrated in following patches separately.
Please see Mojolicious::Plugin::OpenAPI and its tutorial for more documentation.

The patch also refactors some API authentication -related code by taking advantage
of Koha::Exceptions. Authentication is now handled via Mojolicious's "under->to"
functionality. The actual authentication & authorization checks are moved to
Koha::REST::V1::Auth. Added a HTTP 503 response for when database update is
required, instead of returning an authentication failure as before.

To test:
1. prove t/db_dependent/api/v1/auth.t

Signed-off-by: Olli-Antti Kivilahti <olli-antti.kivilahti@jns.fi>
Comment 14 Olli-Antti Kivilahti 2017-03-06 11:44:13 UTC
Created attachment 60840 [details] [review]
Bug 18137: Make /patrons Mojolicious::Plugin::OpenAPI compatible

Also:
- adding some missing and new response definitions into Swagger spec.
- fixing failing test due to Bug 17932's change of boolean values

To test:
1. prove t/db_dependent/api/v1/patrons.t

Signed-off-by: Olli-Antti Kivilahti <olli-antti.kivilahti@jns.fi>
Comment 15 Olli-Antti Kivilahti 2017-03-06 11:44:44 UTC
Created attachment 60841 [details] [review]
Bug 18137: Make /cities Mojolicious::Plugin::OpenAPI compatible

Also:
- adding some missing and new response definitions into Swagger spec.

To test:
1. prove t/db_dependent/api/v1/cities.t

Signed-off-by: Olli-Antti Kivilahti <olli-antti.kivilahti@jns.fi>
Comment 16 Olli-Antti Kivilahti 2017-03-06 11:45:06 UTC
Created attachment 60842 [details] [review]
Bug 18137: Make /holds Mojolicious::Plugin::OpenAPI compatible

Also
- adding some missing and new response definitions into Swagger spec.
- fixing failing tests due to Bug 17932's change of boolean values

To test:
1. prove t/db_dependent/api/v1/holds.t

Signed-off-by: Olli-Antti Kivilahti <olli-antti.kivilahti@jns.fi>
Comment 17 Olli-Antti Kivilahti 2017-03-06 11:45:29 UTC
Created attachment 60843 [details] [review]
Bug 18137: Remove x-mojo-around-action

Mojolicious::Plugin::OpenAPI does not support x-mojo-around action. This patch
removes it from our specification document.

Signed-off-by: Olli-Antti Kivilahti <olli-antti.kivilahti@jns.fi>
Comment 18 Lari Taskula 2017-03-06 11:53:41 UTC
(In reply to Lari Taskula from comment #10)
> I would like to point out that I am experiencing an issue with
> JSON::Validator, where $refs are sometimes left unresolved in our spec and
> it causes some error messages and crashes. This issue is being tracked at
> https://github.com/jhthorsen/json-validator/pull/52.

The developer of JSON::Validator has been working hard on it and it now seems to have been fixed. The fix will appear in version 0.96 of JSON::Validator. That should probably be added as dependency once it is released.
Comment 19 Josef Moravec 2017-03-07 09:56:48 UTC
Just tested, looks good for me, just a minor issue found by qa test tool:

 FAIL	Koha/REST/V1/Auth.pm
   FAIL	  pod
		 in file Koha/REST/V1/Auth.pm
		*** WARNING: 
		
		=head3 without preceding higher level
Comment 20 Lari Taskula 2017-03-07 12:00:08 UTC
Created attachment 60866 [details] [review]
Bug 18137: (follow-up) Fix handling DBIx::Class::Exception messages

- DBIx::Class::Exception should use ->{msg}
Comment 21 Lari Taskula 2017-03-07 12:00:25 UTC
Created attachment 60867 [details] [review]
Bug 18137: (QA-follow-up) Fix pod fail
Comment 22 Tomás Cohen Arazi 2017-03-08 13:41:45 UTC
I like it, and I'm testing it. My only concern is dependencies. I tried to install Mojolicious 6.40 along with Mojolicious::Plugin::OpenAPI 1.10 and it wouldn't install. 1.10 needs newer Mojo (7.something).
Comment 23 Tomás Cohen Arazi 2017-03-08 23:21:24 UTC
Pleaes provide feedback about the dependencies so I can test it.
Comment 24 Mirko Tietgen 2017-03-09 10:28:14 UTC
I submitted Mojolicious::Plugin::OpenAPI to Debian a few days ago (1.10). 1.13 is out by now. And both versions depend on versions of libjson-validator-perl not yet in Debian unstable, and the tests in the latest version are broken.

Can't build at the moment, but it seemed possible to get everything working in Jessie (including Mojolicious 7.x). I'll give an update when I know more.
Comment 25 Lari Taskula 2017-03-13 15:54:46 UTC
(In reply to Mirko Tietgen from comment #24)
> I submitted Mojolicious::Plugin::OpenAPI to Debian a few days ago (1.10).
> 1.13 is out by now. And both versions depend on versions of
> libjson-validator-perl not yet in Debian unstable, and the tests in the
> latest version are broken.
Thanks for looking at this Mirko! 

I had a look at JSON::Validator version 0.96. There indeed seemed to be some issues with the tests in the latest version. I noticed there is a commit to json-validator in GitHub as well as some IRC discussion in #swagger @irc.perl.org suggesting that tests are broken on an older version of Test::More. Using the latest version of Test::More passes the tests. I tested an older version of Test::More (1.001014) and tests are indeed failing:

$ sudo cpanm Test::More@1.001014
--> Working on Test::More
Fetching http://backpan.perl.org/authors/id/E/EX/EXODIST/Test-Simple-1.001014.tar.gz ... OK
Configuring Test-Simple-1.001014 ... OK
Building and testing Test-Simple-1.001014 ... OK
Successfully installed Test-Simple-1.001014 (downgraded from 1.302078)
1 distribution installed
$ prove t
..
..
Test Summary Report
-------------------
t/acceptance.t                       (Wstat: 0 Tests: 265 Failed: 0)
  TODO passed:   45-47, 49-52, 56-57
t/jv-formats.t                       (Wstat: 256 Tests: 28 Failed: 1)
  Failed test:  27
  Non-zero exit status: 1
t/jv-object.t                        (Wstat: 256 Tests: 19 Failed: 1)
  Failed test:  18
  Non-zero exit status: 1
t/openapi-formats.t                  (Wstat: 512 Tests: 24 Failed: 2)
  Failed tests:  12, 22
  Non-zero exit status: 2
t/openapi-response.t                 (Wstat: 0 Tests: 7 Failed: 0)
  TODO passed:   1
t/openapi.t                          (Wstat: 0 Tests: 8 Failed: 0)
  TODO passed:   1
Files=44, Tests=558, 11 wallclock secs ( 0.24 usr  0.06 sys +  9.82 cusr  1.04 csys = 11.16 CPU)
Result: FAIL

Whereas the new version passes:

$ sudo cpanm Test::More
--> Working on Test::More
Fetching http://www.cpan.org/authors/id/E/EX/EXODIST/Test-Simple-1.302078.tar.gz ... OK
Configuring Test-Simple-1.302078 ... OK
Building and testing Test-Simple-1.302078 ... OK
Successfully installed Test-Simple-1.302078 (upgraded from 1.001014)
1 distribution installed
$ prove t
..
..
All tests successful.

Test Summary Report
-------------------
t/acceptance.t                       (Wstat: 0 Tests: 265 Failed: 0)
  TODO passed:   45-47, 49-52, 56-57
t/jv-object.t                        (Wstat: 0 Tests: 19 Failed: 0)
  TODO passed:   9
t/openapi-formats.t                  (Wstat: 0 Tests: 24 Failed: 0)
  TODO passed:   7, 10-11
t/openapi-response.t                 (Wstat: 0 Tests: 7 Failed: 0)
  TODO passed:   1
t/openapi.t                          (Wstat: 0 Tests: 8 Failed: 0)
  TODO passed:   1
Files=44, Tests=558, 12 wallclock secs ( 0.23 usr  0.08 sys + 10.66 cusr  1.12 csys = 12.09 CPU)
Result: PASS

Could this be related to the issue you were having with the tests?

Anyway, as a side note to comment #18, Mojolicious::Plugin::OpenAPI 1.13 has a requirement for JSON::Validator 0.95, which is not enough for us. We need JSON::Validator 0.96 or newer. It will be a requirement in currently unreleased Mojolicious::Plugin::OpenAPI version 1.14. Should we wait for that or explicitly define requirement for JSON::Validator 0.96 in PerlDependencies.pm?

What comes to Mojolicious - I've attempted to run our REST API on Mojolicious 7.26 and so far it seems ok. Tests are also passing for me.
Comment 26 Lari Taskula 2017-03-16 11:58:12 UTC
Created attachment 61163 [details] [review]
Bug 18137: (follow-up) operationId must be unique

operationId has the following documentation:
 "Unique string used to identify the operation. The id MUST be unique among all
  operations described in the API."

This patch modifies operationIds to be unique accross our API operations.
Comment 27 Lari Taskula 2017-03-16 12:25:58 UTC
Created attachment 61166 [details] [review]
Bug 18137: (follow-up) Sort definitions.json

Before this file grows, we should sort it alphabetically.

To test:
1. prove t/db_dependent/api/v1
Comment 28 Mirko Tietgen 2017-03-21 08:23:09 UTC
(In reply to Lari Taskula from comment #25)
> Anyway, as a side note to comment #18, Mojolicious::Plugin::OpenAPI 1.13 has
> a requirement for JSON::Validator 0.95, which is not enough for us. We need
> JSON::Validator 0.96 or newer. It will be a requirement in currently
> unreleased Mojolicious::Plugin::OpenAPI version 1.14. Should we wait for
> that or explicitly define requirement for JSON::Validator 0.96 in
> PerlDependencies.pm?

We can define it now and maybe have to adjust in case it changes. Mojolicious::Plugin::OpenAPI 1.14 is not released yet, I can't work further on the Debian package until it is.
Comment 29 Lari Taskula 2017-04-27 14:35:18 UTC
Created attachment 62780 [details] [review]
Bug 18137: List Mojolicious::Plugin::OpenAPI as Perl dependency

This patch also upgrades Mojolicious version requirement to 6.40 as it is the
minimum requirement set by Mojolicious::Plugin::OpenAPI.

To test:
1. sudo cpanm Mojolicious::Plugin::OpenAPI@1.10

Signed-off-by: Olli-Antti Kivilahti <olli-antti.kivilahti@jns.fi>
Comment 30 Lari Taskula 2017-04-27 14:35:30 UTC
Created attachment 62781 [details] [review]
Bug 18137: Add useful Koha::Exceptions

Signed-off-by: Olli-Antti Kivilahti <olli-antti.kivilahti@jns.fi>
Comment 31 Lari Taskula 2017-04-27 14:35:42 UTC
Created attachment 62782 [details] [review]
Bug 18137: Migrate from Swagger2 to Mojolicious::Plugin::OpenAPI

This patch migrates from Swagger2 to Mojolicious::Plugin::OpenAPI as Swagger2 is
no longer actively maintained.

This migration involves some minor changes to our Swagger specification documents
and to controllers. Each operation is migrated in following patches separately.
Please see Mojolicious::Plugin::OpenAPI and its tutorial for more documentation.

The patch also refactors some API authentication -related code by taking advantage
of Koha::Exceptions. Authentication is now handled via Mojolicious's "under->to"
functionality. The actual authentication & authorization checks are moved to
Koha::REST::V1::Auth. Added a HTTP 503 response for when database update is
required, instead of returning an authentication failure as before.

To test:
1. prove t/db_dependent/api/v1/auth.t

Signed-off-by: Olli-Antti Kivilahti <olli-antti.kivilahti@jns.fi>
Comment 32 Lari Taskula 2017-04-27 14:35:55 UTC
Created attachment 62783 [details] [review]
Bug 18137: Make /patrons Mojolicious::Plugin::OpenAPI compatible

Also:
- adding some missing and new response definitions into Swagger spec.
- fixing failing test due to Bug 17932's change of boolean values

To test:
1. prove t/db_dependent/api/v1/patrons.t

Signed-off-by: Olli-Antti Kivilahti <olli-antti.kivilahti@jns.fi>
Comment 33 Lari Taskula 2017-04-27 14:36:08 UTC
Created attachment 62784 [details] [review]
Bug 18137: Make /cities Mojolicious::Plugin::OpenAPI compatible

Also:
- adding some missing and new response definitions into Swagger spec.

To test:
1. prove t/db_dependent/api/v1/cities.t

Signed-off-by: Olli-Antti Kivilahti <olli-antti.kivilahti@jns.fi>
Comment 34 Lari Taskula 2017-04-27 14:36:20 UTC
Created attachment 62785 [details] [review]
Bug 18137: Make /holds Mojolicious::Plugin::OpenAPI compatible

Also
- adding some missing and new response definitions into Swagger spec.
- fixing failing tests due to Bug 17932's change of boolean values

To test:
1. prove t/db_dependent/api/v1/holds.t

Signed-off-by: Olli-Antti Kivilahti <olli-antti.kivilahti@jns.fi>
Comment 35 Lari Taskula 2017-04-27 14:36:33 UTC
Created attachment 62786 [details] [review]
Bug 18137: Remove x-mojo-around-action

Mojolicious::Plugin::OpenAPI does not support x-mojo-around action. This patch
removes it from our specification document.

Signed-off-by: Olli-Antti Kivilahti <olli-antti.kivilahti@jns.fi>
Comment 36 Lari Taskula 2017-04-27 14:36:46 UTC
Created attachment 62787 [details] [review]
Bug 18137: (follow-up) Fix handling DBIx::Class::Exception messages

- DBIx::Class::Exception should use ->{msg}
Comment 37 Lari Taskula 2017-04-27 14:36:59 UTC
Created attachment 62788 [details] [review]
Bug 18137: (QA-follow-up) Fix pod fail
Comment 38 Lari Taskula 2017-04-27 14:37:11 UTC
Created attachment 62789 [details] [review]
Bug 18137: (follow-up) Sort definitions.json

Before this file grows, we should sort it alphabetically.

To test:
1. prove t/db_dependent/api/v1
Comment 39 Lari Taskula 2017-04-27 14:43:35 UTC
Just rebasing these on master and squashing some forgotten "use"s into Koha::REST::V1::Auth.
Comment 40 Mirko Tietgen 2017-04-28 07:04:21 UTC
(In reply to Mirko Tietgen from comment #28)

> Mojolicious::Plugin::OpenAPI 1.14 is not released yet, I can't work further
> on the Debian package until it is.

@ Lari:

JSON::Validator 0.97 has been released.
Mojolicious::Plugin::OpenAPI 1.14 has not been released.

If I understand correctly, Mojolicious::Plugin::OpenAPI 1.14 is not a requirement for us, just JSON::Validator 0.96?.Then please specify 0.96 as a dependency and I will check if I can build JSON::Validator 0.97 and Mojolicious::Plugin::OpenAPI 1.13 now after the update.
Comment 41 Lari Taskula 2017-04-28 12:22:10 UTC
(In reply to Mirko Tietgen from comment #40)
> (In reply to Mirko Tietgen from comment #28)
> 
> > Mojolicious::Plugin::OpenAPI 1.14 is not released yet, I can't work further
> > on the Debian package until it is.
> 
> @ Lari:
> 
> JSON::Validator 0.97 has been released.
> Mojolicious::Plugin::OpenAPI 1.14 has not been released.
> 
> If I understand correctly, Mojolicious::Plugin::OpenAPI 1.14 is not a
> requirement for us, just JSON::Validator 0.96?.Then please specify 0.96 as a
> dependency and I will check if I can build JSON::Validator 0.97 and
> Mojolicious::Plugin::OpenAPI 1.13 now after the update.
(In reply to Mirko Tietgen from comment #40)
> (In reply to Mirko Tietgen from comment #28)
> 
> > Mojolicious::Plugin::OpenAPI 1.14 is not released yet, I can't work further
> > on the Debian package until it is.
> 
> @ Lari:
> 
> JSON::Validator 0.97 has been released.
> Mojolicious::Plugin::OpenAPI 1.14 has not been released.
> 
> If I understand correctly, Mojolicious::Plugin::OpenAPI 1.14 is not a
> requirement for us, just JSON::Validator 0.96?.Then please specify 0.96 as a
> dependency and I will check if I can build JSON::Validator 0.97 and
> Mojolicious::Plugin::OpenAPI 1.13 now after the update.

Thanks for checking! I'll do that! Also, Tomas experienced issues in installing Mojolicious 6.40 together with M::P::OpenAPI 1.10 - I couldn't reproduce this issue, but perhaps it is a safe bet to upgrade Mojolicious at the same time. We have been successfully running Koha REST API on Mojolicious versions between 7.26-7.29, and Koha's REST tests are also passing on the latest release 7.31.

This upgrade will fail test t/db_dependent/api/v1/swagger/definitions.t. I will try to patch that in another Bug.
Comment 42 Lari Taskula 2017-04-28 12:23:41 UTC
Created attachment 62816 [details] [review]
Bug 18137: List Mojolicious::Plugin::OpenAPI and JSON::Validator as dependencies

Signed-off-by: Olli-Antti Kivilahti <olli-antti.kivilahti@jns.fi>
Comment 43 Lari Taskula 2017-04-28 12:44:05 UTC
(In reply to Lari Taskula from comment #41)
> This upgrade will fail test t/db_dependent/api/v1/swagger/definitions.t. I
> will try to patch that in another Bug.
Bug 18508
Comment 44 Josef Moravec 2017-05-03 16:00:44 UTC
Created attachment 63025 [details] [review]
[SIGNED-OFF] Bug 18137: Add useful Koha::Exceptions

Signed-off-by: Olli-Antti Kivilahti <olli-antti.kivilahti@jns.fi>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 45 Josef Moravec 2017-05-03 16:00:56 UTC
Created attachment 63026 [details] [review]
[SIGNED-OFF] Bug 18137: Migrate from Swagger2 to Mojolicious::Plugin::OpenAPI

This patch migrates from Swagger2 to Mojolicious::Plugin::OpenAPI as Swagger2 is
no longer actively maintained.

This migration involves some minor changes to our Swagger specification documents
and to controllers. Each operation is migrated in following patches separately.
Please see Mojolicious::Plugin::OpenAPI and its tutorial for more documentation.

The patch also refactors some API authentication -related code by taking advantage
of Koha::Exceptions. Authentication is now handled via Mojolicious's "under->to"
functionality. The actual authentication & authorization checks are moved to
Koha::REST::V1::Auth. Added a HTTP 503 response for when database update is
required, instead of returning an authentication failure as before.

To test:
1. prove t/db_dependent/api/v1/auth.t

Signed-off-by: Olli-Antti Kivilahti <olli-antti.kivilahti@jns.fi>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 46 Josef Moravec 2017-05-03 16:01:02 UTC
Created attachment 63027 [details] [review]
[SIGNED-OFF] Bug 18137: Make /patrons Mojolicious::Plugin::OpenAPI compatible

Also:
- adding some missing and new response definitions into Swagger spec.
- fixing failing test due to Bug 17932's change of boolean values

To test:
1. prove t/db_dependent/api/v1/patrons.t

Signed-off-by: Olli-Antti Kivilahti <olli-antti.kivilahti@jns.fi>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 47 Josef Moravec 2017-05-03 16:01:09 UTC
Created attachment 63028 [details] [review]
[SIGNED-OFF] Bug 18137: Make /cities Mojolicious::Plugin::OpenAPI compatible

Also:
- adding some missing and new response definitions into Swagger spec.

To test:
1. prove t/db_dependent/api/v1/cities.t

Signed-off-by: Olli-Antti Kivilahti <olli-antti.kivilahti@jns.fi>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 48 Josef Moravec 2017-05-03 16:01:14 UTC
Created attachment 63029 [details] [review]
[SIGNED-OFF] Bug 18137: Make /holds Mojolicious::Plugin::OpenAPI compatible

Also
- adding some missing and new response definitions into Swagger spec.
- fixing failing tests due to Bug 17932's change of boolean values

To test:
1. prove t/db_dependent/api/v1/holds.t

Signed-off-by: Olli-Antti Kivilahti <olli-antti.kivilahti@jns.fi>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 49 Josef Moravec 2017-05-03 16:01:19 UTC
Created attachment 63030 [details] [review]
[SIGNED-OFF] Bug 18137: Remove x-mojo-around-action

Mojolicious::Plugin::OpenAPI does not support x-mojo-around action. This patch
removes it from our specification document.

Signed-off-by: Olli-Antti Kivilahti <olli-antti.kivilahti@jns.fi>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 50 Josef Moravec 2017-05-03 16:01:25 UTC
Created attachment 63031 [details] [review]
[SIGNED-OFF] Bug 18137: (follow-up) Fix handling DBIx::Class::Exception messages

- DBIx::Class::Exception should use ->{msg}

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 51 Josef Moravec 2017-05-03 16:01:30 UTC
Created attachment 63032 [details] [review]
[SIGNED-OFF] Bug 18137: (QA-follow-up) Fix pod fail

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 52 Josef Moravec 2017-05-03 16:01:36 UTC
Created attachment 63033 [details] [review]
[SIGNED-OFF] Bug 18137: (follow-up) Sort definitions.json

Before this file grows, we should sort it alphabetically.

To test:
1. prove t/db_dependent/api/v1

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 53 Josef Moravec 2017-05-03 16:01:41 UTC
Created attachment 63034 [details] [review]
[SIGNED-OFF] Bug 18137: List Mojolicious::Plugin::OpenAPI and JSON::Validator as dependencies

Signed-off-by: Olli-Antti Kivilahti <olli-antti.kivilahti@jns.fi>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 54 Tomás Cohen Arazi 2017-05-05 16:03:48 UTC
Ok, I tested this successfully using:

$ sudo cpanm Mojolicious@7.31
$ sudo cpanm JSON::Validator@0.96
$ sudo cpanm Mojolicious::Plugin::OpenAPI@1.13

Tests pass:
kohadev-koha@kohadevbox:/home/vagrant/kohaclone$ prove t/db_dependent/api/v1/
t/db_dependent/api/v1/auth.t ..... Use of uninitialized value $lasttime in numeric lt (<) at /home/vagrant/kohaclone/C4/Auth.pm line 1673.
t/db_dependent/api/v1/auth.t ..... ok   
t/db_dependent/api/v1/cities.t ... ok   
t/db_dependent/api/v1/holds.t .... ok   
t/db_dependent/api/v1/patrons.t .. ok     
All tests successful.
Files=4, Tests=31, 10 wallclock secs ( 0.05 usr  0.00 sys +  8.52 cusr  1.26 csys =  9.83 CPU)
Result: PASS

I also tested the API using Firefox's HttpRequester, with excellent results.

I'm signing this patchset, but will wait for Mirko to tell about the deps, because otherwise we could end up breaking the stable release due to unpackaged dependencies.
Comment 55 Tomás Cohen Arazi 2017-05-05 16:06:22 UTC
Created attachment 63177 [details] [review]
Bug 18137: Add useful Koha::Exceptions

Signed-off-by: Olli-Antti Kivilahti <olli-antti.kivilahti@jns.fi>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 56 Tomás Cohen Arazi 2017-05-05 16:06:33 UTC
Created attachment 63178 [details] [review]
Bug 18137: Migrate from Swagger2 to Mojolicious::Plugin::OpenAPI

This patch migrates from Swagger2 to Mojolicious::Plugin::OpenAPI as Swagger2 is
no longer actively maintained.

This migration involves some minor changes to our Swagger specification documents
and to controllers. Each operation is migrated in following patches separately.
Please see Mojolicious::Plugin::OpenAPI and its tutorial for more documentation.

The patch also refactors some API authentication -related code by taking advantage
of Koha::Exceptions. Authentication is now handled via Mojolicious's "under->to"
functionality. The actual authentication & authorization checks are moved to
Koha::REST::V1::Auth. Added a HTTP 503 response for when database update is
required, instead of returning an authentication failure as before.

To test:
1. prove t/db_dependent/api/v1/auth.t

Signed-off-by: Olli-Antti Kivilahti <olli-antti.kivilahti@jns.fi>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 57 Tomás Cohen Arazi 2017-05-05 16:06:45 UTC
Created attachment 63179 [details] [review]
Bug 18137: Make /patrons Mojolicious::Plugin::OpenAPI compatible

Also:
- adding some missing and new response definitions into Swagger spec.
- fixing failing test due to Bug 17932's change of boolean values

To test:
1. prove t/db_dependent/api/v1/patrons.t

Signed-off-by: Olli-Antti Kivilahti <olli-antti.kivilahti@jns.fi>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 58 Tomás Cohen Arazi 2017-05-05 16:06:55 UTC
Created attachment 63180 [details] [review]
Bug 18137: Make /cities Mojolicious::Plugin::OpenAPI compatible

Also:
- adding some missing and new response definitions into Swagger spec.

To test:
1. prove t/db_dependent/api/v1/cities.t

Signed-off-by: Olli-Antti Kivilahti <olli-antti.kivilahti@jns.fi>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 59 Tomás Cohen Arazi 2017-05-05 16:07:11 UTC
Created attachment 63181 [details] [review]
Bug 18137: Make /holds Mojolicious::Plugin::OpenAPI compatible

Also
- adding some missing and new response definitions into Swagger spec.
- fixing failing tests due to Bug 17932's change of boolean values

To test:
1. prove t/db_dependent/api/v1/holds.t

Signed-off-by: Olli-Antti Kivilahti <olli-antti.kivilahti@jns.fi>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 60 Tomás Cohen Arazi 2017-05-05 16:07:21 UTC
Created attachment 63182 [details] [review]
Bug 18137: Remove x-mojo-around-action

Mojolicious::Plugin::OpenAPI does not support x-mojo-around action. This patch
removes it from our specification document.

Signed-off-by: Olli-Antti Kivilahti <olli-antti.kivilahti@jns.fi>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 61 Tomás Cohen Arazi 2017-05-05 16:07:32 UTC
Created attachment 63183 [details] [review]
Bug 18137: (follow-up) Fix handling DBIx::Class::Exception messages

- DBIx::Class::Exception should use ->{msg}

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 62 Tomás Cohen Arazi 2017-05-05 16:07:43 UTC
Created attachment 63184 [details] [review]
Bug 18137: (QA-follow-up) Fix pod fail

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 63 Tomás Cohen Arazi 2017-05-05 16:07:53 UTC
Created attachment 63185 [details] [review]
Bug 18137: (follow-up) Sort definitions.json

Before this file grows, we should sort it alphabetically.

To test:
1. prove t/db_dependent/api/v1

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 64 Tomás Cohen Arazi 2017-05-05 16:08:05 UTC
Created attachment 63186 [details] [review]
Bug 18137: List Mojolicious::Plugin::OpenAPI and JSON::Validator as dependencies

Signed-off-by: Olli-Antti Kivilahti <olli-antti.kivilahti@jns.fi>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 65 Tomás Cohen Arazi 2017-05-05 16:11:12 UTC
I messed up with the packages/obsoletes, re-submitting.
Comment 66 Tomás Cohen Arazi 2017-05-05 16:11:33 UTC
Created attachment 63187 [details] [review]
Bug 18137: Add useful Koha::Exceptions

Signed-off-by: Olli-Antti Kivilahti <olli-antti.kivilahti@jns.fi>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 67 Tomás Cohen Arazi 2017-05-05 16:11:39 UTC
Created attachment 63188 [details] [review]
Bug 18137: Migrate from Swagger2 to Mojolicious::Plugin::OpenAPI

This patch migrates from Swagger2 to Mojolicious::Plugin::OpenAPI as Swagger2 is
no longer actively maintained.

This migration involves some minor changes to our Swagger specification documents
and to controllers. Each operation is migrated in following patches separately.
Please see Mojolicious::Plugin::OpenAPI and its tutorial for more documentation.

The patch also refactors some API authentication -related code by taking advantage
of Koha::Exceptions. Authentication is now handled via Mojolicious's "under->to"
functionality. The actual authentication & authorization checks are moved to
Koha::REST::V1::Auth. Added a HTTP 503 response for when database update is
required, instead of returning an authentication failure as before.

To test:
1. prove t/db_dependent/api/v1/auth.t

Signed-off-by: Olli-Antti Kivilahti <olli-antti.kivilahti@jns.fi>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 68 Tomás Cohen Arazi 2017-05-05 16:11:44 UTC
Created attachment 63189 [details] [review]
Bug 18137: Make /patrons Mojolicious::Plugin::OpenAPI compatible

Also:
- adding some missing and new response definitions into Swagger spec.
- fixing failing test due to Bug 17932's change of boolean values

To test:
1. prove t/db_dependent/api/v1/patrons.t

Signed-off-by: Olli-Antti Kivilahti <olli-antti.kivilahti@jns.fi>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 69 Tomás Cohen Arazi 2017-05-05 16:11:49 UTC
Created attachment 63190 [details] [review]
Bug 18137: Make /cities Mojolicious::Plugin::OpenAPI compatible

Also:
- adding some missing and new response definitions into Swagger spec.

To test:
1. prove t/db_dependent/api/v1/cities.t

Signed-off-by: Olli-Antti Kivilahti <olli-antti.kivilahti@jns.fi>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 70 Tomás Cohen Arazi 2017-05-05 16:11:54 UTC
Created attachment 63191 [details] [review]
Bug 18137: Make /holds Mojolicious::Plugin::OpenAPI compatible

Also
- adding some missing and new response definitions into Swagger spec.
- fixing failing tests due to Bug 17932's change of boolean values

To test:
1. prove t/db_dependent/api/v1/holds.t

Signed-off-by: Olli-Antti Kivilahti <olli-antti.kivilahti@jns.fi>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 71 Tomás Cohen Arazi 2017-05-05 16:11:59 UTC
Created attachment 63192 [details] [review]
Bug 18137: Remove x-mojo-around-action

Mojolicious::Plugin::OpenAPI does not support x-mojo-around action. This patch
removes it from our specification document.

Signed-off-by: Olli-Antti Kivilahti <olli-antti.kivilahti@jns.fi>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 72 Tomás Cohen Arazi 2017-05-05 16:12:03 UTC
Created attachment 63193 [details] [review]
Bug 18137: (follow-up) Fix handling DBIx::Class::Exception messages

- DBIx::Class::Exception should use ->{msg}

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 73 Tomás Cohen Arazi 2017-05-05 16:12:08 UTC
Created attachment 63194 [details] [review]
Bug 18137: (QA-follow-up) Fix pod fail

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 74 Tomás Cohen Arazi 2017-05-05 16:12:13 UTC
Created attachment 63195 [details] [review]
Bug 18137: (follow-up) Sort definitions.json

Before this file grows, we should sort it alphabetically.

To test:
1. prove t/db_dependent/api/v1

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 75 Tomás Cohen Arazi 2017-05-05 16:12:18 UTC
Created attachment 63196 [details] [review]
Bug 18137: List Mojolicious::Plugin::OpenAPI and JSON::Validator as dependencies

Signed-off-by: Olli-Antti Kivilahti <olli-antti.kivilahti@jns.fi>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 76 Lari Taskula 2017-05-09 12:16:27 UTC
Created attachment 63283 [details] [review]
Bug 18137: List Mojolicious::Plugin::OpenAPI and JSON::Validator as dependencies

Signed-off-by: Olli-Antti Kivilahti <olli-antti.kivilahti@jns.fi>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 77 Lari Taskula 2017-05-09 12:18:57 UTC
(In reply to Lari Taskula from comment #76)
> Created attachment 63283 [details] [review] [review]
> Bug 18137: List Mojolicious::Plugin::OpenAPI and JSON::Validator as
> dependencies
> 
> Signed-off-by: Olli-Antti Kivilahti <olli-antti.kivilahti@jns.fi>
> 
> Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Rebased on master. This patch now maintains requirement as defined by Bug 17190.
Comment 78 Mirko Tietgen 2017-05-17 11:30:36 UTC
I have uploaded libmojolicious-perl_7.21+dfsg-1~kohadev1 to the unstable repository.
Comment 80 Mirko Tietgen 2017-05-18 10:18:16 UTC

I have uploaded preliminary versions of

libjson-validator-perl_0.97-1~kohadev1_all.deb
libmojolicious-plugin-openapi-perl_1.15-1~kohadev1_all.deb

to the unstable repository
Comment 81 Mirko Tietgen 2017-05-25 17:18:15 UTC
According to Tomás, the newer versions break dev setups, so we are back to libmojolicious-perl 6.15 and libjson-validator-perl 0.67 for the time being.
Comment 82 Tomás Cohen Arazi 2017-07-19 16:34:23 UTC
Created attachment 65106 [details] [review]
Bug 18137: Add useful Koha::Exceptions

Signed-off-by: Olli-Antti Kivilahti <olli-antti.kivilahti@jns.fi>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 83 Tomás Cohen Arazi 2017-07-19 16:34:28 UTC
Created attachment 65107 [details] [review]
Bug 18137: Migrate from Swagger2 to Mojolicious::Plugin::OpenAPI

This patch migrates from Swagger2 to Mojolicious::Plugin::OpenAPI as Swagger2 is
no longer actively maintained.

This migration involves some minor changes to our Swagger specification documents
and to controllers. Each operation is migrated in following patches separately.
Please see Mojolicious::Plugin::OpenAPI and its tutorial for more documentation.

The patch also refactors some API authentication -related code by taking advantage
of Koha::Exceptions. Authentication is now handled via Mojolicious's "under->to"
functionality. The actual authentication & authorization checks are moved to
Koha::REST::V1::Auth. Added a HTTP 503 response for when database update is
required, instead of returning an authentication failure as before.

To test:
1. prove t/db_dependent/api/v1/auth.t

Signed-off-by: Olli-Antti Kivilahti <olli-antti.kivilahti@jns.fi>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 84 Tomás Cohen Arazi 2017-07-19 16:34:33 UTC
Created attachment 65108 [details] [review]
Bug 18137: Make /patrons Mojolicious::Plugin::OpenAPI compatible

Also:
- adding some missing and new response definitions into Swagger spec.
- fixing failing test due to Bug 17932's change of boolean values

To test:
1. prove t/db_dependent/api/v1/patrons.t

Signed-off-by: Olli-Antti Kivilahti <olli-antti.kivilahti@jns.fi>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 85 Tomás Cohen Arazi 2017-07-19 16:34:39 UTC
Created attachment 65109 [details] [review]
Bug 18137: Make /cities Mojolicious::Plugin::OpenAPI compatible

Also:
- adding some missing and new response definitions into Swagger spec.

To test:
1. prove t/db_dependent/api/v1/cities.t

Signed-off-by: Olli-Antti Kivilahti <olli-antti.kivilahti@jns.fi>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 86 Tomás Cohen Arazi 2017-07-19 16:34:45 UTC
Created attachment 65110 [details] [review]
Bug 18137: Make /holds Mojolicious::Plugin::OpenAPI compatible

Also
- adding some missing and new response definitions into Swagger spec.
- fixing failing tests due to Bug 17932's change of boolean values

To test:
1. prove t/db_dependent/api/v1/holds.t

Signed-off-by: Olli-Antti Kivilahti <olli-antti.kivilahti@jns.fi>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 87 Tomás Cohen Arazi 2017-07-19 16:34:51 UTC
Created attachment 65111 [details] [review]
Bug 18137: Remove x-mojo-around-action

Mojolicious::Plugin::OpenAPI does not support x-mojo-around action. This patch
removes it from our specification document.

Signed-off-by: Olli-Antti Kivilahti <olli-antti.kivilahti@jns.fi>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 88 Tomás Cohen Arazi 2017-07-19 16:34:56 UTC
Created attachment 65112 [details] [review]
Bug 18137: (follow-up) Fix handling DBIx::Class::Exception messages

- DBIx::Class::Exception should use ->{msg}

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 89 Tomás Cohen Arazi 2017-07-19 16:35:02 UTC
Created attachment 65113 [details] [review]
Bug 18137: (QA-follow-up) Fix pod fail

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 90 Tomás Cohen Arazi 2017-07-19 16:35:10 UTC
Created attachment 65114 [details] [review]
Bug 18137: (follow-up) Sort definitions.json

Before this file grows, we should sort it alphabetically.

To test:
1. prove t/db_dependent/api/v1

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 91 Tomás Cohen Arazi 2017-07-19 16:35:16 UTC
Created attachment 65115 [details] [review]
Bug 18137: List Mojolicious::Plugin::OpenAPI and JSON::Validator as dependencies

Signed-off-by: Olli-Antti Kivilahti <olli-antti.kivilahti@jns.fi>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 92 Jonathan Druart 2017-07-19 17:29:41 UTC
The QA script reports POD coverage issues.
Comment 93 Tomás Cohen Arazi 2017-08-01 17:42:36 UTC
Created attachment 65404 [details] [review]
Bug 18137: List Mojolicious::Plugin::OpenAPI and JSON::Validator as dependencies

Edit (tcohen): I've changed the version numbers to match those Mirko has already
successfully packaged and are known to work for this patchset.

Signed-off-by: Olli-Antti Kivilahti <olli-antti.kivilahti@jns.fi>
Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 94 Jonathan Druart 2017-08-08 21:12:26 UTC
I get a warning and a failure from tests:

t/db_dependent/api/v1/auth.t ................. Use of uninitialized value $lasttime in numeric lt (<) at /home/vagrant/kohaclone/C4/Auth.pm line 1679.
   
t/db_dependent/api/v1/swagger/definitions.t .. Can't locate object method "cache_dir" via package "Swagger2::SchemaValidator" at /usr/share/perl5/Swagger2.pm line 41.
Comment 95 Lari Taskula 2017-08-09 10:24:55 UTC
(In reply to Jonathan Druart from comment #94)
> I get a warning and a failure from tests:
> 
> t/db_dependent/api/v1/auth.t ................. Use of uninitialized value
> $lasttime in numeric lt (<) at /home/vagrant/kohaclone/C4/Auth.pm line 1679.
>    
> t/db_dependent/api/v1/swagger/definitions.t .. Can't locate object method
> "cache_dir" via package "Swagger2::SchemaValidator" at
> /usr/share/perl5/Swagger2.pm line 41.

See Bug 18508
Comment 96 Tomás Cohen Arazi 2017-08-09 12:11:34 UTC
I missed definitions.t on testing. And confirm this is fixed by Lari's patch.

Note: I'm all for removing definitions.t BTW, just wanted to note it.
Comment 97 Tomás Cohen Arazi 2017-08-09 14:11:42 UTC
Created attachment 65713 [details] [review]
Bug 18137: (QA followup) Make sure the session exists and is expired on expiration tests

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 98 Lari Taskula 2017-08-09 14:36:49 UTC
Created attachment 65714 [details] [review]
Bug 18137: (QA followup) Make sure the session exists and is expired on expiration tests

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Lari Taskula <lari.taskula@jns.fi>
Comment 99 Martin Renvoize 2017-08-23 12:31:17 UTC
Comment on attachment 65107 [details] [review]
Bug 18137: Migrate from Swagger2 to Mojolicious::Plugin::OpenAPI

Review of attachment 65107 [details] [review]:
-----------------------------------------------------------------

::: Koha/REST/V1/Auth.pm
@@ +57,5 @@
> +                json => { error => 'Something went wrong, check the logs.' }
> +            );
> +        }
> +        if ($_->isa('Koha::Exceptions::UnderMaintenance')) {
> +            return $c->render(status => 503, json => { error => $_->error });

Should these calls to render not all be 'openapi => { error => $_->error }` as aposed to `json => { error => $_->error }`.. else none of these responses are getting validated.
Comment 100 Tomás Cohen Arazi 2017-08-23 22:14:32 UTC
(In reply to Martin Renvoize from comment #99)
> Comment on attachment 65107 [details] [review] [review]
> Bug 18137: Migrate from Swagger2 to Mojolicious::Plugin::OpenAPI
> 
> Review of attachment 65107 [details] [review] [review]:
> -----------------------------------------------------------------
> 
> ::: Koha/REST/V1/Auth.pm
> @@ +57,5 @@
> > +                json => { error => 'Something went wrong, check the logs.' }
> > +            );
> > +        }
> > +        if ($_->isa('Koha::Exceptions::UnderMaintenance')) {
> > +            return $c->render(status => 503, json => { error => $_->error });
> 
> Should these calls to render not all be 'openapi => { error => $_->error }`
> as aposed to `json => { error => $_->error }`.. else none of these responses
> are getting validated.

That's because the auth check is done using ->under, and as the check itself is calling ->render, it is breaking the dispatch chain (not breaking like in 'stop working' but like short-circuiting to another way of returning the error. Clarifying to avoid possible idiomatic issues).

I think we should leave it as-is and deal with this problem on a separate bug, or leave it. Because it could involve a big re-engineering to make the check let the chain continue while propagating the exception.

I can't even imagine how to do it without adding irrelevant checks and repetitive code to controller classes.
Comment 101 Tomás Cohen Arazi 2017-09-04 13:53:41 UTC Comment hidden (obsolete)
Comment 102 Tomás Cohen Arazi 2017-09-04 13:53:54 UTC Comment hidden (obsolete)
Comment 103 Tomás Cohen Arazi 2017-09-04 13:54:06 UTC Comment hidden (obsolete)
Comment 104 Tomás Cohen Arazi 2017-09-04 13:54:21 UTC Comment hidden (obsolete)
Comment 105 Tomás Cohen Arazi 2017-09-04 13:54:33 UTC Comment hidden (obsolete)
Comment 106 Tomás Cohen Arazi 2017-09-04 13:54:42 UTC Comment hidden (obsolete)
Comment 107 Tomás Cohen Arazi 2017-09-04 13:54:50 UTC Comment hidden (obsolete)
Comment 108 Tomás Cohen Arazi 2017-09-04 13:55:30 UTC Comment hidden (obsolete)
Comment 109 Tomás Cohen Arazi 2017-09-04 13:55:39 UTC Comment hidden (obsolete)
Comment 110 Tomás Cohen Arazi 2017-09-04 13:55:48 UTC Comment hidden (obsolete)
Comment 111 Tomás Cohen Arazi 2017-09-04 13:55:55 UTC Comment hidden (obsolete)
Comment 112 Tomás Cohen Arazi 2017-09-04 13:56:02 UTC Comment hidden (obsolete)
Comment 113 Tomás Cohen Arazi 2017-09-04 13:56:10 UTC Comment hidden (obsolete)
Comment 114 Tomás Cohen Arazi 2017-09-04 13:56:18 UTC Comment hidden (obsolete)
Comment 115 Tomás Cohen Arazi 2017-09-04 13:56:26 UTC Comment hidden (obsolete)
Comment 116 Tomás Cohen Arazi 2017-09-04 13:56:34 UTC Comment hidden (obsolete)
Comment 117 Tomás Cohen Arazi 2017-09-04 13:56:42 UTC Comment hidden (obsolete)
Comment 118 Tomás Cohen Arazi 2017-09-04 13:56:54 UTC Comment hidden (obsolete)
Comment 119 Tomás Cohen Arazi 2017-09-04 14:03:56 UTC
Rebased against master, which changed Koha::REST::V1::Hold a bit.
Comment 120 Jonathan Druart 2017-09-21 15:49:36 UTC
Pushed to master for 17.11, thanks to everybody involved!
Comment 121 Fridolin Somers 2017-10-27 11:50:16 UTC
Enhancement not pushed to 17.05.x