Bug 16212 - Swagger specification separation and minification
Summary: Swagger specification separation and minification
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Web services (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Bugs List
QA Contact: Testopia
URL:
Keywords:
Depends on: 15126
Blocks: 16699 17102 17432
  Show dependency treegraph
 
Reported: 2016-04-06 11:04 UTC by Julian Maurice
Modified: 2017-06-14 22:15 UTC (History)
5 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Text to go in the release notes:
Version(s) released in:


Attachments
Newest Swagger-spec validated 100% and pushed to production yay! Includes the swaggerMinifier.pl (15.82 KB, application/gzip)
2016-04-06 11:05 UTC, Julian Maurice
Details
Bug 16212: Add script minifySwagger.pl (3.83 KB, patch)
2016-04-06 11:17 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 16212: Move from misc/devel to api/v1 (709 bytes, patch)
2016-06-14 12:48 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 16212: Use swagger.min.json insted of swagger.json (1.35 KB, patch)
2016-06-14 13:18 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 16212: Automatically minify swagger.json (6.10 KB, patch)
2016-06-20 12:22 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 16212: Automatically minify swagger.json (7.53 KB, patch)
2016-06-20 13:58 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 16212: Automatically minify swagger.json (7.55 KB, patch)
2016-06-20 14:00 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 16212: Automatically minify swagger.json (8.50 KB, patch)
2016-06-20 15:13 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 16212: Automatically minify swagger.json (8.53 KB, patch)
2016-06-21 13:33 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 16212: Add script minifySwagger.pl (4.13 KB, patch)
2016-06-30 10:44 UTC, Olli-Antti Kivilahti
Details | Diff | Splinter Review
Bug 16212: Move from misc/devel to api/v1 (1.00 KB, patch)
2016-06-30 10:45 UTC, Olli-Antti Kivilahti
Details | Diff | Splinter Review
Bug 16212: Use swagger.min.json insted of swagger.json (1.66 KB, patch)
2016-06-30 10:49 UTC, Olli-Antti Kivilahti
Details | Diff | Splinter Review
Bug 16212: Automatically minify swagger.json (8.84 KB, patch)
2016-06-30 10:49 UTC, Olli-Antti Kivilahti
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 16212: Add script minifySwagger.pl (4.41 KB, patch)
2016-06-30 12:56 UTC, Olli-Antti Kivilahti
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 16212: Add script minifySwagger.pl (4.41 KB, patch)
2016-06-30 13:27 UTC, Johanna Räisä
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 16212: Move from misc/devel to api/v1 (1.28 KB, patch)
2016-06-30 13:29 UTC, Johanna Räisä
Details | Diff | Splinter Review
Bug 16212: Use swagger.min.json insted of swagger.json (1.93 KB, patch)
2016-06-30 13:30 UTC, Johanna Räisä
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 16212: Use swagger.min.json insted of swagger.json (1.94 KB, patch)
2016-06-30 13:31 UTC, Johanna Räisä
Details | Diff | Splinter Review
Bug 16212: Automatically minify swagger.json (9.11 KB, patch)
2016-06-30 13:33 UTC, Johanna Räisä
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 16212: Add script minifySwagger.pl (4.41 KB, patch)
2016-06-30 13:50 UTC, Johanna Räisä
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 16212: Move from misc/devel to api/v1 (1.28 KB, patch)
2016-06-30 13:51 UTC, Johanna Räisä
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 16212: Use swagger.min.json insted of swagger.json (1.94 KB, patch)
2016-06-30 13:51 UTC, Johanna Räisä
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 16212: Automatically minify swagger.json (9.12 KB, patch)
2016-06-30 13:51 UTC, Johanna Räisä
Details | Diff | Splinter Review
Bug 16212: Add script minifySwagger.pl (4.47 KB, patch)
2016-08-11 08:58 UTC, Benjamin Rokseth
Details | Diff | Splinter Review
Bug 16212: Move from misc/devel to api/v1 (1.34 KB, patch)
2016-08-11 08:58 UTC, Benjamin Rokseth
Details | Diff | Splinter Review
Bug 16212: Use swagger.min.json insted of swagger.json (2.00 KB, patch)
2016-08-11 08:59 UTC, Benjamin Rokseth
Details | Diff | Splinter Review
Bug 16212: Add script minifySwagger.pl (4.86 KB, patch)
2016-08-25 13:29 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 16212: Use swagger.min.json insted of swagger.json (2.05 KB, patch)
2016-08-25 13:31 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 16212: Add script minifySwagger.pl (4.86 KB, patch)
2016-08-25 13:32 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 16212: Use swagger.min.json insted of swagger.json (2.05 KB, patch)
2016-08-25 13:32 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 Julian Maurice 2016-04-06 11:04:38 UTC
We should discuss how to split the Swagger spec, and whether or not we should minify it (and how).

For more details, see the following comments:
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15126#c5
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15126#c6
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15126#c26
Comment 1 Julian Maurice 2016-04-06 11:05:26 UTC
Created attachment 49949 [details]
Newest Swagger-spec validated 100% and pushed to production yay! Includes the swaggerMinifier.pl
Comment 2 Julian Maurice 2016-04-06 11:17:15 UTC Comment hidden (obsolete)
Comment 3 Lari Taskula 2016-06-14 12:48:03 UTC Comment hidden (obsolete)
Comment 4 Lari Taskula 2016-06-14 12:51:56 UTC
If the change above is OK, I think also all Swagger-related stuff could be in api/v1/swagger. I'm working on Bug 16699 so maybe this can be done there.
Comment 5 Lari Taskula 2016-06-14 13:18:58 UTC Comment hidden (obsolete)
Comment 6 Lari Taskula 2016-06-17 08:35:29 UTC
The minification is problematic at the moment, because it has to be executed manually. It could be assigned to execute automatically at Koha::REST::V1->startup(), but since this is executed on each request to the REST API, there will be a lot of useless computation. A dirty trick would be to make a copy of swagger.json and compare it to the actual swagger.json in startup(), and run swaggerMinifier.pl in case the two specification files are different, and finally replacing the copy with actual swagger.json.
Comment 7 Lari Taskula 2016-06-20 12:22:13 UTC Comment hidden (obsolete)
Comment 8 Lari Taskula 2016-06-20 13:58:39 UTC Comment hidden (obsolete)
Comment 9 Lari Taskula 2016-06-20 14:00:49 UTC Comment hidden (obsolete)
Comment 10 Lari Taskula 2016-06-20 15:13:21 UTC Comment hidden (obsolete)
Comment 11 Lari Taskula 2016-06-21 13:33:08 UTC Comment hidden (obsolete)
Comment 12 Olli-Antti Kivilahti 2016-06-30 10:44:12 UTC Comment hidden (obsolete)
Comment 13 Olli-Antti Kivilahti 2016-06-30 10:45:13 UTC Comment hidden (obsolete)
Comment 14 Olli-Antti Kivilahti 2016-06-30 10:49:02 UTC Comment hidden (obsolete)
Comment 15 Olli-Antti Kivilahti 2016-06-30 10:49:30 UTC Comment hidden (obsolete)
Comment 16 Olli-Antti Kivilahti 2016-06-30 12:56:51 UTC Comment hidden (obsolete)
Comment 17 Johanna Räisä 2016-06-30 13:27:30 UTC Comment hidden (obsolete)
Comment 18 Johanna Räisä 2016-06-30 13:29:23 UTC Comment hidden (obsolete)
Comment 19 Johanna Räisä 2016-06-30 13:30:49 UTC Comment hidden (obsolete)
Comment 20 Johanna Räisä 2016-06-30 13:31:23 UTC Comment hidden (obsolete)
Comment 21 Johanna Räisä 2016-06-30 13:33:05 UTC Comment hidden (obsolete)
Comment 22 Johanna Räisä 2016-06-30 13:50:52 UTC Comment hidden (obsolete)
Comment 23 Johanna Räisä 2016-06-30 13:51:06 UTC Comment hidden (obsolete)
Comment 24 Johanna Räisä 2016-06-30 13:51:17 UTC Comment hidden (obsolete)
Comment 25 Johanna Räisä 2016-06-30 13:51:28 UTC Comment hidden (obsolete)
Comment 26 Benjamin Rokseth 2016-07-08 08:57:16 UTC
Hi,
as much as I would like to sign off this patch, I'm afraid I cannot agree on the notion of automatic minifying. It is an anti-pattern of production code, and requires too much quirks to my liking.

a) It allows for inconsistency between apache/plack and minified/not minified
b) It requires cgi/apache rights to write to swagger definitions, not a good idea.
c) Plack also needs restarting/cache invalidation, since it caches swagger.json
d) What is the problem of minifying before deploy anyways?

Also I'm not sure of the need to minify swagger at all. If the only reason is for the online validation, I would argue that the online validation is the problem. More important is `mojo swagger2 validate`. Minifying swagger gives no extra performance opposed to http gzipped, probably the opposite. And it is unreadable.

Hate to be the party killer, but if you consider removing the patch "Automatically minify swagger.json" I will sign off. Certainly if it means the splitting of definition and paths can be signed off, which I consider a very good idea.

I don't consider the online validation an issue, but I do consider the problematic patching of large json files an issue.
Comment 27 Lari Taskula 2016-07-27 16:32:30 UTC
Hey Benjamin, thanks for the interest! I'm glad we share a desire to split our specification :)

(In reply to Benjamin Rokseth from comment #26)
> Also I'm not sure of the need to minify swagger at all. If the only reason
> is for the online validation, I would argue that the online validation is
> the problem. More important is `mojo swagger2 validate`.
Our spec in master is not a valid Swagger spec, even if `mojo swagger2 validate` says so. Swagger specification does not support a reference whole Definitions-object in swagger.json like this:

"definitions": {
  "$ref": "./definitions/index.json"
}

That is super annoying, but I think we also want to validate against the online validator because it spots this issue. `mojo swagger2 validate` works (for now), but what if the plugin's validator is updated to consider this type of $ref invalid (as Swagger specification says)? We would have to place all definitions and paths in swagger.json, and at maximum $ref individual paths and definitions to external files, not the whole Paths / Definitions-object.

I think Olli-Antti also encountered some other issues that this minifier fixes. Maybe he can comment more about it.

Btw, I have opened an issue to Swagger2 plugin's GitHub to initiate some more discussion: https://github.com/jhthorsen/swagger2/issues/89
Comment 28 Benjamin Rokseth 2016-07-28 00:59:06 UTC
(In reply to Lari Taskula from comment #27)
> Hey Benjamin, thanks for the interest! I'm glad we share a desire to split
> our specification :)
> 
> (In reply to Benjamin Rokseth from comment #26)
> > Also I'm not sure of the need to minify swagger at all. If the only reason
> > is for the online validation, I would argue that the online validation is
> > the problem. More important is `mojo swagger2 validate`.
> Our spec in master is not a valid Swagger spec, even if `mojo swagger2
> validate` says so. Swagger specification does not support a reference whole
> Definitions-object in swagger.json like this:
> 
> "definitions": {
>   "$ref": "./definitions/index.json"
> }
> 
> That is super annoying, but I think we also want to validate against the
> online validator because it spots this issue. `mojo swagger2 validate` works
> (for now), but what if the plugin's validator is updated to consider this
> type of $ref invalid (as Swagger specification says)? We would have to place
> all definitions and paths in swagger.json, and at maximum $ref individual
> paths and definitions to external files, not the whole Paths /
> Definitions-object.
> 
> I think Olli-Antti also encountered some other issues that this minifier
> fixes. Maybe he can comment more about it.
> 
> Btw, I have opened an issue to Swagger2 plugin's GitHub to initiate some
> more discussion: https://github.com/jhthorsen/swagger2/issues/89

Lari, I engaged in your discussion at github, but to be sure, are we talking about the same thing? I test the swagger definitions at the online swagger validator and it obviously fails at relative and URI, as they are not resolvable to the validator. This is to be expected, but does not mean the definition is invalid. The only way to validate online in any case would be to first resolve all refs to one single json document.

You refer to invalid refs in swagger specifications, but I only find this discussion in the swagger-ui
Comment 29 Lari Taskula 2016-08-04 09:47:59 UTC
(In reply to Benjamin Rokseth from comment #28)
> (In reply to Lari Taskula from comment #27)
> > Hey Benjamin, thanks for the interest! I'm glad we share a desire to split
> > our specification :)
> > 
> > (In reply to Benjamin Rokseth from comment #26)
> > > Also I'm not sure of the need to minify swagger at all. If the only reason
> > > is for the online validation, I would argue that the online validation is
> > > the problem. More important is `mojo swagger2 validate`.
> > Our spec in master is not a valid Swagger spec, even if `mojo swagger2
> > validate` says so. Swagger specification does not support a reference whole
> > Definitions-object in swagger.json like this:
> > 
> > "definitions": {
> >   "$ref": "./definitions/index.json"
> > }
> > 
> > That is super annoying, but I think we also want to validate against the
> > online validator because it spots this issue. `mojo swagger2 validate` works
> > (for now), but what if the plugin's validator is updated to consider this
> > type of $ref invalid (as Swagger specification says)? We would have to place
> > all definitions and paths in swagger.json, and at maximum $ref individual
> > paths and definitions to external files, not the whole Paths /
> > Definitions-object.
> > 
> > I think Olli-Antti also encountered some other issues that this minifier
> > fixes. Maybe he can comment more about it.
> > 
> > Btw, I have opened an issue to Swagger2 plugin's GitHub to initiate some
> > more discussion: https://github.com/jhthorsen/swagger2/issues/89
> 
> Lari, I engaged in your discussion at github, but to be sure, are we talking
> about the same thing? I test the swagger definitions at the online swagger
> validator and it obviously fails at relative and URI, as they are not
> resolvable to the validator. This is to be expected, but does not mean the
> definition is invalid. The only way to validate online in any case would be
> to first resolve all refs to one single json document.
> 
> You refer to invalid refs in swagger specifications, but I only find this
> discussion in the swagger-ui
I agree that the online swagger validator is unable to resolve relative references, but the original issue still exists. You can also write a single spec file with the problem in online validator, e.g.

{
  "swagger": "2.0",
  "info": { "version": "0.1", "title" : "ref all paths/definitions" },
  "paths": { "$ref": "#/x-def/paths" },
  "definitions": { "$ref": "#/x-def/definitions" },
  "x-def": {
    "definitions": {
      "patron": {
        "type": "object",
        "properties": {
          "borrowernumber": {
            "type": "string",
            "description": "internally assigned user identifier"
          }
        }
      }
    },
    "paths": {
      "/patrons": {
        "get": {
          "responses" : {
            "200": {
              "description": "A list of patrons",
              "schema": {
                "type": "array",
                "items": {
                  "$ref": "#/definitions/patron"
                }
              }
            }
          }
        }
      }
    }
  }
}
online.swagger.io validator will return:
"attribute paths.$ref is not of type `object`",
"attribute definitions.$ref is not of type `object`"

`mojo swagger2 validate` works because it resolves all $refs before validation. The validation never sees a "$ref". When adjusted to not resolve $refs in our own spec, I got
[JSON::Validator] type /paths _validate_type_object [/paths: Properties not allowed: $ref.]
[JSON::Validator] type /definitions/$ref _validate_type_object [/definitions/$ref: Expected object - got string.]

To make this spec pass validation (both online validator and adjusted case of JSON::Validator), we need to $ref individual paths and definitions instead, e.g.:
{
  "swagger": "2.0",
  "info": { "version": "0.1", "title" : "ref individual paths/definitions" },
  "paths": {
    "/patrons": { "$ref": "#/x-def/paths/patrons" }
  },
  "definitions": {
    "patron": { "$ref": "#/x-def/definitions/patron" }
  },
  "x-def": {
    "definitions": {
      "patron": {
        "type": "object",
        "properties": {
          "borrowernumber": {
            "type": "string",
            "description": "internally assigned user identifier"
          }
        }
      }
    },
    "paths": {
      "patrons": {
        "get": {
          "responses" : {
            "200": {
              "description": "A list of patrons",
              "schema": {
                "type": "array",
                "items": {
                  "$ref": "#/definitions/patron"
                }
              }
            }
          }
        }
      }
    }
  }
}

This issue is referred also elsewhere than swagger-ui, here are some links:
https://github.com/mission-liao/pyswagger/issues/53#issuecomment-168872441 (also points to OpenAPI Specification, "@webron: You can reference specific http verb operations, not full sets of paths.")
swagger-parser comment on the article about splitting spec: https://github.com/swagger-api/swagger-parser/issues/87#issuecomment-140280055

It's just some of the annoying features in Swagger. It also looks like the author of Swagger2/OpenAPI plugin has commited this change in the new OpenAPI plugin https://github.com/jhthorsen/mojolicious-plugin-openapi/commit/7d804c58eae51b42b3506badd59d4123fb723e8c and I guess this means that in the future our current spec will not validate without first resolving all the $refs with (for example) this minifier script.
Comment 30 Benjamin Rokseth 2016-08-10 09:55:19 UTC
So, I'm not able to grasp the problems in swagger splitting specifications. Since the specs seem to be maintained, it's probably best to minify before validation, so the $ref problem is gone. The main point is to keep the specs in an understandable (and patchable) structure.

Anyways, I still disagree on the automatic minifying. It has no place in production code, since the specs should be static there in any case.
Comment 31 Lari Taskula 2016-08-10 12:28:59 UTC
(In reply to Benjamin Rokseth from comment #30)
> The main point is to keep the specs in an understandable (and patchable) 
> structure.
Completely agree on this.

It's unfortunate that Swagger has limitations that make it more complicated for us.

> Anyways, I still disagree on the automatic minifying. It has no place in
> production code, since the specs should be static there in any case.
Perhaps it could be parametrized to be enabled / disabled, but for now I think the best action is to obsolete that patch.

Thanks for your comments and engagement in discussion both here and GitHub, Benjamin, I really appreciate it!
Comment 32 Lari Taskula 2016-08-10 12:38:49 UTC
(In reply to Lari Taskula from comment #31)
> > Anyways, I still disagree on the automatic minifying. It has no place in
> > production code, since the specs should be static there in any case.
> Perhaps it could be parametrized to be enabled / disabled, but for now I
> think the best action is to obsolete that patch.
Moved to Bug 17102.
Comment 33 Benjamin Rokseth 2016-08-11 08:58:07 UTC Comment hidden (obsolete)
Comment 34 Benjamin Rokseth 2016-08-11 08:58:18 UTC Comment hidden (obsolete)
Comment 35 Benjamin Rokseth 2016-08-11 08:59:02 UTC Comment hidden (obsolete)
Comment 36 Benjamin Rokseth 2016-08-11 09:03:45 UTC
Tests: 

swagger.json:

curl -X POST -H "accept:application/json" -d @swagger.json http://online.swagger.io/validator/debug

{"messages":["attribute definitions.$ref is not of type `object`"],"schemaValidationMessages":[{"level":"error","domain":"validation","keyword":"type","message":"instance type (string) does not match any allowed primitive type (allowed: [\"object\"])","schema":{"loadingURI":"http://swagger.io/v2/schema.json#","pointer":"/definitions/schema"},"instance":{"pointer":"/definitions/$ref"}}]}

swagger.min.json
curl -X POST -H "accept:application/json" -d @swagger.min.json http://online.swagger.io/validator/debug
{}
Comment 37 Tomás Cohen Arazi 2016-08-25 13:29:58 UTC Comment hidden (obsolete)
Comment 38 Tomás Cohen Arazi 2016-08-25 13:31:33 UTC Comment hidden (obsolete)
Comment 39 Tomás Cohen Arazi 2016-08-25 13:32:10 UTC
Created attachment 54872 [details] [review]
Bug 16212: Add script minifySwagger.pl

This patch introduces the misc/devel/minifySwagger.pl script that
loads the Swagger files, follows references and produces a compact
("minified") version of the swagger file which is suitable for
distribution.

The wiki page should be updated with instructions on how to regenerate
it so the Release Manager does it on each spec upgrade.

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

My name is Olli-Antti Kivilahti and I approve this commit.
We have been using the Swagger2.0-driven REST API on Mojolicious for 1 year now
in production and I am certain we have a pretty good idea on how to work with
the limitations of Swagger2.0

Signed-off-by: Johanna Raisa <johanna.raisa@gmail.com>

My name is Johanna Räisä and I approve this commit.
We have been using Swagger2.0-driven REST API in production successfully.

Signed-off-by: Benjamin Rokseth <benjamin.rokseth@kul.oslo.kommune.no>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 40 Tomás Cohen Arazi 2016-08-25 13:32:44 UTC
Created attachment 54873 [details] [review]
Bug 16212: Use swagger.min.json insted of swagger.json

Our current approach with splitting the Swagger specification brings problems.
For example, in swagger.json, if we split the whole "definitions" object with
$ref, validations will fail with online.swagger.io validator.

(See http://online.swagger.io/validator/debug?url=url_to_your_swagger_json)

The problem also occurs with "paths" (Paths Object), because simply $ref to all
paths for example in /paths/index.json does not meet the Swagger2 specification.

The problem is solved by using the minification script and the minified version
of swagger.json after which the swagger.min.json is valid Swagger2 specification,
because the minifier resolves the problematic $refs of swagger.json file in the
minified version.

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

My name is Olli-Antti Kivilahti and I approve this commit.
We have been using the Swagger2.0-driven REST API on Mojolicious for 1 year now
in production and I am certain we have a pretty good idea on how to work with
the limitations of Swagger2.0

Signed-off-by: Johanna Raisa <johanna.raisa@gmail.com>

My name is Johanna Räisä and I approve this commit.
We have been using Swagger2.0-driven REST API in production successfully.

Signed-off-by: Benjamin Rokseth <benjamin.rokseth@kul.oslo.kommune.no>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 41 Tomás Cohen Arazi 2016-08-25 13:35:25 UTC
I've moved the minifySwagger.pl script back to misc/devel as it is similar to how we handle the DBIx schema files.

@RM: Beware you need to create the minified file before pushing this patches, as it would otherwise break the REST api (which points to the minified version with this patchset):

$ misc/devel/minifySwagger.pl --source api/v1/swagger.json --destination api/v1/swagger.min.json
Comment 42 Kyle M Hall 2016-08-25 13:50:55 UTC
Pushed to master for 16.11, thanks Julian, Lari!