Created attachment 44386 [details]
Dependency tree for Swagger2 0.60

Hi!

I think your solution looks robust and extendable. I would push the separation of definitions a bit further.
This is a good article about how to split the Swagger2-spec:
    http://azimi.me/2015/07/16/split-swagger-into-smaller-files.html
and it is pretty much the same as you did.

How about we modified your suggestion so the structure looks like this:

.
├── swagger.json
├── info
│   └── index.json
├── definitions
│   └── index.json
│   └── error.json
│   └── patron.json
├── parameters
│   └── index.json
│   └── patron.json
└── paths
    ├── index.json
    └── patrons.json

where definitions/patron.json looks like this:
    "borrower": {
      "type": "object",
      "properties": {
        "borrowernumber": {
          "$ref": "#/definitions/borrowernumber"
        },
        "cardnumber": {
          "$ref": "#/definitions/cardnumber"
        },
        "surname": {
          "$ref": "#/definitions/surname"
        },
        "firstname": {
          "$ref": "#/definitions/firstname"
        },
        ...
      }
    },
    "cardnumber": {
      "description": "Library assigned ID number for Borrowers",
      "type": "string"
    },
    "borrowernumber": {
      "type": "integer",
      "description": "Borrower internal identifier"
    },
    "surname": {
      "description": "borrower's last name",
      "type": "string"
    },
    "surname": {
      "description": "borrower's first name",
      "type": "string"
    }

Now that there is a clear separation of logic (and release from insanely obscure git conflict resolutions with swagger.json), we should enforce that we define all parameters and definitions globally under #/parameters and #/definitions instead of defining them under object/path definitions.
This way we maximize code reuse.
In some cases it might not make much sense (if we only focus on today), like for "surname" and "firstname" in the above example, but some day in the near future somebody might want to reuse those components and then he would have to refactor those changes (or more likely just duplicate code because we are lazy). So as a future-proof solution I feel global definitions are the way to go.

We could even push this a bit further, by instead of defining these REST API with the restrictions of the Koha DB, we could use commonly acknowledged linked data schemas, like schema.org to define our objects.
https://schema.org/Person
This would make it much easier to share the same records, for example between multiple Koha installations. Mainly because of the unique identifier in these objects, like {"url": "https://renki.vaarakirjastot.fi/api/v1/patrons/43441"}
This is actually something which is easily enabled due to our awesome REST API and helps build more linked data systems between various API consumers.
So if somebody would take our Patron-object, it would have a undeniably unique identifier and wouldn't conflict with Patron-objects originating from other systems.

But for now we can focus on splitting our Swagger-spec.

Also one thing regarding the structure of the api/v1/-directory:

Currently our api/v1/ looks like this:

.
├── hypnotoad.conf
├── hypnotoad.pid
├── app.cgi
├── swagger.json
├── doc
│   └── #Swagger-UI API discovery tool
│
├── auth_examples #REST API authentication code for different languages
│   │             #Discoverable from the Swagger-UI
│   └── ruby_example.rb.txt
│   └── ajax_example.sj.txt
#
#└── swagger  ##Could we add this?##
#    └── #swagger.json and subfolders and files

If you start adding the Swagger-spec linked files and subfolders here. The directory structure will get very confusing. How about we add a directory where to put the "swagger"-specification files?
I understand that the Swagger-UI might not be something which is aimed to be included in the official Koha, but somebody will certainly want to run some real API browsing/discovery service and this is a prefect place to do that. However Swagger-UI works really well and it is improving all the time with a lot of committers in already.
We could name the directory "swagger", or even better "OpenAPI", which is the new name of the Swagger2-specification
    https://github.com/OAI/OpenAPI-Specification

(In reply to Olli-Antti Kivilahti from comment #11)
> Created attachment 47844 [details] [review] [review]
> Swagger Minifier

This patch won't apply as it delete a file which doesn't exist in any other (non-obsoleted) patch. Please reformat your patch.

Also, if "Big splitted swagger2 spec. Example of the planned usage pattern." patch is not to be pushed, please include "[DO NOT PUSH]" in front of the patch title to make it obvious

Thanks for taking a look!

What I am curious of is whether or not the spliced swagger2-spec I posted makes sense?

I haven't looked at it in detail, but having a filesystem structure that match swagger spec structure would certainly make sense

I just tested minifySwagger.pl and something bothers me : it resolves all "$ref", meaning the "minified" file will be larger than the sum of all files it comes from if the same "$ref" is used at least twice.

Is it possible resolve only "$ref" that point to different files ? So we keep the idea of a (small) single minified file and let the client resolve the other refs.

Comment on attachment 44373 [details] [review]
Bug 15126: x-mojo-controller deprecation

>From a1267ff354633ef7f491704f81d9ffe1590672a7 Mon Sep 17 00:00:00 2001
>From: Martin Renvoize <martin.renvoize@ptfs-europe.com>
>Date: Wed, 7 Oct 2015 14:49:17 +0000
>Subject: [PATCH] Bug 15126: x-mojo-controller deprecation
>
>Remove the use of soon to be deprecated x-mojo-controller from our
>specification and replace with the recommended operationId format.
>---
> Koha/REST/V1/{Patrons.pm => Patron.pm} | 6 +++---
> api/v1/swagger.json                    | 2 --
> 2 files changed, 3 insertions(+), 5 deletions(-)
> rename Koha/REST/V1/{Patrons.pm => Patron.pm} (96%)
>
>diff --git a/Koha/REST/V1/Patrons.pm b/Koha/REST/V1/Patron.pm
>similarity index 96%
>rename from Koha/REST/V1/Patrons.pm
>rename to Koha/REST/V1/Patron.pm
>index 6468bba..fee6c31 100644
>--- a/Koha/REST/V1/Patrons.pm
>+++ b/Koha/REST/V1/Patron.pm
>@@ -1,4 +1,4 @@
>-package Koha::REST::V1::Patrons;
>+package Koha::REST::V1::Patron;
> 
> # This file is part of Koha.
> #
>@@ -22,7 +22,7 @@ use Mojo::Base 'Mojolicious::Controller';
> use C4::Auth qw( haspermission );
> use Koha::Patrons;
> 
>-sub list_patrons {
>+sub list {
>     my ($c, $args, $cb) = @_;
> 
>     my $user = $c->stash('koha.user');
>@@ -35,7 +35,7 @@ sub list_patrons {
>     $c->$cb($patrons->unblessed, 200);
> }
> 
>-sub get_patron {
>+sub get {
>     my ($c, $args, $cb) = @_;
> 
>     my $user = $c->stash('koha.user');
>diff --git a/api/v1/swagger.json b/api/v1/swagger.json
>index 8f32781..e821c44 100644
>--- a/api/v1/swagger.json
>+++ b/api/v1/swagger.json
>@@ -16,7 +16,6 @@
>   "paths": {
>     "/patrons": {
>       "get": {
>-        "x-mojo-controller": "Koha::REST::V1::Patrons",
>         "operationId": "listPatrons",
>         "tags": ["patrons"],
>         "produces": [
>@@ -43,7 +42,6 @@
>     },
>     "/patrons/{borrowernumber}": {
>       "get": {
>-        "x-mojo-controller": "Koha::REST::V1::Patrons",
>         "operationId": "getPatron",
>         "tags": ["patrons"],
>         "parameters": [
>-- 
>1.9.1

Sorry for the spamming, I rebased the patches against master, since some changes were already pushed to master by bug #13799.

(In reply to Julian Maurice from comment #17)
> I just tested minifySwagger.pl and something bothers me : it resolves all
> "$ref", meaning the "minified" file will be larger than the sum of all files
> it comes from if the same "$ref" is used at least twice.
> 
> Is it possible resolve only "$ref" that point to different files ? So we
> keep the idea of a (small) single minified file and let the client resolve
> the other refs.

There are some issue with this approach. Firstly, if you want to have reusable reference contents, they probably must reside under the "Definitions Object", and the "Definitions Object" only accepts Schemas/Objects, so we cannot define reusable primitive data types, such as biblionumber.
{ type => 'String',
  description => 'descr'
}

Atleast I didn't figure out how to do it.

Maybe you could add an extra level of nesting under the "Definitions Object"
"x-primitive-definitions": {
  "biblionumber": {
    type => 'Integer',
    description => 'descr'
  }
}

but that might break any automatic Swagger2-tools.

Also the way references are dereferenced is part of the core Mojolicious::Plugin::Swagger2-functionality and I am not so sure how the original author will like that change. I guess it would be nice to have and even doable, but it might be a really difficult algorithm to implement, which makes a complex system even more complex.

The reason I implemented this approach is that external Swagger2-tools (like Swagger2 UI) doesn't resolve those references well, or was it that they dont reference at all (at the point of writing).

I am not concerned about any repetition in the Swagger2-definition in Koha, tho you never know how it might affect us in the future.


Anyway. I am currently working on implementing CORS-support for the Swagger2-API, because we need it now. This certainly is a challenge :)


Also. We moved the
/definitions/index.json
to
/definitions.json

When you reference a definition, you had to write the reference like this:
"$ref": "/definitions/index.json#/borrowernumber"

now you can write the reference more easy:
"$ref": "/definitions.json#/borrowernumber"

Anyway you can find our newest Swagger2-definition from here:
https://github.com/KohaSuomi/kohasuomi/tree/kohasuomi3.16/api/v1/swagger
https://renki.vaarakirjastot.fi/api/v1/doc/


It took us quite some time to refactor to that model + a lot of testing :)
But we are more interested in following the community model. However our API is a product of months of work and continuous self-improvement so I can say with confidence that we are taking the right route.

(In reply to Benjamin Rokseth from comment #24)
> Created attachment 49846 [details] [review] [review]
> Bug 15126: x-mojo-controller deprecation
> 
> Remove the use of soon to be deprecated x-mojo-controller from our
> specification and replace with the recommended operationId format.
> 
> Signed-off-by: Benjamin Rokseth <benjamin.rokseth@kul.oslo.kommune.no>

Also be advised that the new model of inferring the correct controller-subroutine -pair from the operationId is not without it's problems. Tho if you are willing to accept some clunkiness with the operationId, you can get by without the x-mojo-controller.

One issue I found out is that the operationId "getStory" doesn't translate to Stories::Get, but it translates to something like Storys::Get, which might be rather confusing.

This is not a stopper tho
Just saying, be careful.
use a lot of
export SWAGGER2_DEBUG=1
:)

Olli, can we move the "Newest Swagger-spec validated 100% and pushed to production yay! Includes the swaggerMinifier.pl" attachment to another bug? This way we can continue the discussion on swagger-minification, while letting the patches here pass into master

Sure.

Tho I stress that you should replace

definitions/index.json

with

definitions.json

Comment on attachment 48226 [details]
Newest Swagger-spec validated 100% and pushed to production yay! Includes the swaggerMinifier.pl

Moved to bug 16212

Please provide an updated patch.

Did someone try to package Mojolicious v6 and Swagger2 v0.59 for Jessie?

(In reply to Olli-Antti Kivilahti from comment #29)
> Sure.
> 
> Tho I stress that you should replace
> 
> definitions/index.json
> 
> with
> 
> definitions.json

What about that suggestion?

(In reply to Jonathan Druart from comment #34)
> What about that suggestion?

Olli said:
> When you reference a definition, you had to write the reference like this:
> "$ref": "/definitions/index.json#/borrowernumber"
> 
> now you can write the reference more easy:
> "$ref": "/definitions.json#/borrowernumber"

But you can also write (Olli please correct me if I'm wrong)
  "$ref": "/definitions/borrowernumber.json"

So I don't see any benefit in moving index.json

(In reply to Jonathan Druart from comment #33)
> Did someone try to package Mojolicious v6 and Swagger2 v0.59 for Jessie?

They are both packaged in Debian testing, so it shouldn't be too hard to make a backport for Jessie. But I'll let Galen answer that.
Adding Galen to the CC list.

(In reply to Julian Maurice from comment #36)
> (In reply to Jonathan Druart from comment #33)
> > Did someone try to package Mojolicious v6 and Swagger2 v0.59 for Jessie?
> 
> They are both packaged in Debian testing, so it shouldn't be too hard to
> make a backport for Jessie. But I'll let Galen answer that.
> Adding Galen to the CC list.

Any news on this one?

(In reply to Julian Maurice from comment #32)
> Created attachment 50142 [details] [review] [review]
> Bug 15126: Update patron definition

Julian, I think we need tests to make sure the swagger file will be always up-to-date with the DB structure. Any ideas how we could do that?

(In reply to Jonathan Druart from comment #38)
> (In reply to Julian Maurice from comment #32)
> > Created attachment 50142 [details] [review] [review] [review]
> > Bug 15126: Update patron definition
> 
> Julian, I think we need tests to make sure the swagger file will be always
> up-to-date with the DB structure. Any ideas how we could do that?

An easy solution would be to parse the spec file with Swagger module, and for each path check that the keys are the same that the corresponding DB table.
But what if some paths return more (or less, or different) keys than the corresponding DB table (or if there is no corresponding DB table) ?
A proper solution would be to call each path and check that the keys in the response match with the spec, but it has to be done each time a path is added

(In reply to Jonathan Druart from comment #38)
> (In reply to Julian Maurice from comment #32)
> > Created attachment 50142 [details] [review] [review] [review]
> > Bug 15126: Update patron definition
> 
> Julian, I think we need tests to make sure the swagger file will be always
> up-to-date with the DB structure. Any ideas how we could do that?

Maybe that should be a coding guideline/QA requisite, that every endpoint has a proper set of unit tests, including this check on the swagger files. I'd vote for discussing this on the next dev meeting to gather broader opinions.

I verified that most of these changes work correctly in the process of testing bug 13903, so I'd be happy to put my Passed QA stamp on this, but I'm just now coming into this discussion and not sure where things stand. If Galen has a packages for the new Swagger today or tomorrow, are the other concerns on this bug resolved?

Created attachment 50916 [details] [review]
Bug 15126: Refactor spec file

Created attachment 50917 [details] [review]
Bug 15126: x-mojo-controller deprecation

Created attachment 50918 [details] [review]
Bug 15126: Update dependencies

Created attachment 50919 [details] [review]
Bug 15126: Update patron definition

Still needs input from Package Manager about dependencies.

I have uploaded to the squeeze-dev/unstable slot the following backported packages:

libswagger2-perl 0.77-1~kohadev+1
libmojolicious-perl 6.15+dfsg-1~kohadev+1
libjson-validator-perl 0.67+dfsg-1~kohadev+1
libio-socket-socks-perl 0.67-1~kohadev+1

Please give it a whirl.

Data point: seems to work for me on a Jessie box.

This is kinda the reason I was always a bit against using the Swagger2 and Mojolicious combination. Don't get me wrong, I love the combination for developeing against, but both are still fast moving projects, especially the swagger2 one which in the grand scheme of things is still itself fairly infant in it's release lifetime.

Since reporting this bug at Swagger 0.59.. we're now at 0.77 and still moving forward fairly quickly.

Still, I don't see any reason not to push this one.. but expect a new one opening up soon for the 0.77 release.. and another for the 0.80 and so on and so on.. ;)

Just to my pence

Pushed to master for Koha 16.05, thanks Martin and Julian!

(In reply to Galen Charlton from comment #47)
> I have uploaded to the squeeze-dev/unstable slot the following backported
> packages:
> 
> libswagger2-perl 0.77-1~kohadev+1
> libmojolicious-perl 6.15+dfsg-1~kohadev+1
> libjson-validator-perl 0.67+dfsg-1~kohadev+1
> libio-socket-socks-perl 0.67-1~kohadev+1
> 
> Please give it a whirl.

I confirm that it works with Swagger 2 0.77 and Mojolicious 6.15. Should we update PerlDependencies.pm ?
libjson-validator-perl was installed automatically as a dependency, but libio-socket-socks-perl was not and it seems not needed. Why is it packaged ?

(In reply to Julian Maurice from comment #51)
> libio-socket-socks-perl was not and it seems not needed. Why is it packaged ?

It's one of the recommends for libmojolicious-perl and I tossed it in, but it's not required as far as I can tell (and thus doesn't need to be added as a Koha perldep).

(In reply to Tomás Cohen Arazi from comment #40)
> (In reply to Jonathan Druart from comment #38)
> > (In reply to Julian Maurice from comment #32)
> > > Created attachment 50142 [details] [review] [review] [review] [review]
> > > Bug 15126: Update patron definition
> > 
> > Julian, I think we need tests to make sure the swagger file will be always
> > up-to-date with the DB structure. Any ideas how we could do that?
> 
> Maybe that should be a coding guideline/QA requisite, that every endpoint
> has a proper set of unit tests, including this check on the swagger files.
> I'd vote for discussing this on the next dev meeting to gather broader
> opinions.

This has never been done and the json file is not up-to-date: checkprevcheckout and updated_on are missing.

(In reply to Jonathan Druart from comment #53)
> (In reply to Tomás Cohen Arazi from comment #40)
> > (In reply to Jonathan Druart from comment #38)
> > > (In reply to Julian Maurice from comment #32)
> > > > Created attachment 50142 [details] [review] [review] [review] [review] [review]
> > > > Bug 15126: Update patron definition
> > > 
> > > Julian, I think we need tests to make sure the swagger file will be always
> > > up-to-date with the DB structure. Any ideas how we could do that?
> > 
> > Maybe that should be a coding guideline/QA requisite, that every endpoint
> > has a proper set of unit tests, including this check on the swagger files.
> > I'd vote for discussing this on the next dev meeting to gather broader
> > opinions.
> 
> This has never been done and the json file is not up-to-date:
> checkprevcheckout and updated_on are missing.

Will you fill a bug? :-D

(In reply to Tomás Cohen Arazi from comment #54)
> (In reply to Jonathan Druart from comment #53)
> > (In reply to Tomás Cohen Arazi from comment #40)
> > > (In reply to Jonathan Druart from comment #38)
> > > > (In reply to Julian Maurice from comment #32)
> > > > > Created attachment 50142 [details] [review] [review] [review] [review] [review] [review]
> > > > > Bug 15126: Update patron definition
> > > > 
> > > > Julian, I think we need tests to make sure the swagger file will be always
> > > > up-to-date with the DB structure. Any ideas how we could do that?
> > > 
> > > Maybe that should be a coding guideline/QA requisite, that every endpoint
> > > has a proper set of unit tests, including this check on the swagger files.
> > > I'd vote for discussing this on the next dev meeting to gather broader
> > > opinions.
> > 
> > This has never been done and the json file is not up-to-date:
> > checkprevcheckout and updated_on are missing.
> 
> Will you fill a bug? :-D

See bug 17032, already pushed now.