Bug 16699 - Swagger: Split parameters and paths, and specify required permissions for resource
Summary: Swagger: Split parameters and paths, and specify required permissions for res...
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: 16212 16271
Blocks: 16652 13895 14868 16330
  Show dependency treegraph
 
Reported: 2016-06-09 13:59 UTC by Lari Taskula
Modified: 2017-12-07 22:17 UTC (History)
3 users (show)

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


Attachments
Bug 16699: Split parameters and paths in Swagger (19.62 KB, patch)
2016-06-14 15:01 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 16699: Add borrowernumberQueryParam for reusability (2.22 KB, patch)
2016-06-14 15:01 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 16699: Fix mixed-up indentation from 2-4 spaces to 2 spaces (23.67 KB, patch)
2016-06-14 15:02 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 16699: Support multiple types in primitive definitions (2.66 KB, patch)
2016-06-14 15:02 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 16699: Reference new x-primitives in currently defined objects (3.70 KB, patch)
2016-06-14 15:03 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 16699: Move Swagger-related files to api/v1/swagger (4.71 KB, patch)
2016-06-14 15:03 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 16699: Move Swagger-related files to api/v1/swagger (5.65 KB, patch)
2016-06-20 12:37 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 16699: Move Swagger-related files to api/v1/swagger (5.65 KB, patch)
2016-06-20 12:42 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 16699: Split parameters and paths in Swagger (20.15 KB, patch)
2016-06-30 11:19 UTC, Olli-Antti Kivilahti
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 16699: Split parameters and paths in Swagger (20.16 KB, patch)
2016-06-30 11:22 UTC, Olli-Antti Kivilahti
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 16699: Add borrowernumberQueryParam for reusability (2.77 KB, patch)
2016-06-30 11:23 UTC, Olli-Antti Kivilahti
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 16699: Fix mixed-up indentation from 2-4 spaces to 2 spaces (24.21 KB, patch)
2016-06-30 11:23 UTC, Olli-Antti Kivilahti
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 16699: Support multiple types in primitive definitions (3.21 KB, patch)
2016-06-30 11:24 UTC, Olli-Antti Kivilahti
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 16699: Reference new x-primitives in currently defined objects (4.24 KB, patch)
2016-06-30 11:24 UTC, Olli-Antti Kivilahti
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 16699: Move Swagger-related files to api/v1/swagger (6.19 KB, patch)
2016-06-30 11:26 UTC, Olli-Antti Kivilahti
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 16699: Split parameters and paths in Swagger (20.34 KB, patch)
2016-06-30 13:33 UTC, Johanna Räisä
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 16699: Add borrowernumberQueryParam for reusability (3.04 KB, patch)
2016-06-30 13:34 UTC, Johanna Räisä
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 16699: Fix mixed-up indentation from 2-4 spaces to 2 spaces (24.48 KB, patch)
2016-06-30 13:34 UTC, Johanna Räisä
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 16699: Support multiple types in primitive definitions (3.47 KB, patch)
2016-06-30 13:35 UTC, Johanna Räisä
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 16699: Reference new x-primitives in currently defined objects (4.51 KB, patch)
2016-06-30 13:37 UTC, Johanna Räisä
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 16699: Move Swagger-related files to api/v1/swagger (6.46 KB, patch)
2016-06-30 13:39 UTC, Johanna Räisä
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 16699: Split parameters and paths in Swagger (20.34 KB, patch)
2016-06-30 13:51 UTC, Johanna Räisä
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 16699: Add borrowernumberQueryParam for reusability (3.04 KB, patch)
2016-06-30 13:51 UTC, Johanna Räisä
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 16699: Fix mixed-up indentation from 2-4 spaces to 2 spaces (24.48 KB, patch)
2016-06-30 13:52 UTC, Johanna Räisä
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 16699: Support multiple types in primitive definitions (3.47 KB, patch)
2016-06-30 13:52 UTC, Johanna Räisä
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 16699: Reference new x-primitives in currently defined objects (4.51 KB, patch)
2016-06-30 13:52 UTC, Johanna Räisä
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 16699: Move Swagger-related files to api/v1/swagger (6.46 KB, patch)
2016-06-30 13:53 UTC, Johanna Räisä
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 16699: Split parameters and paths in Swagger (25.87 KB, patch)
2016-08-11 10:37 UTC, Benjamin Rokseth
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 16699: Add borrowernumberQueryParam for reusability (3.03 KB, patch)
2016-08-11 10:37 UTC, Benjamin Rokseth
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 16699: Fix mixed-up indentation from 2-4 spaces to 2 spaces (25.73 KB, patch)
2016-08-11 10:37 UTC, Benjamin Rokseth
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 16699: Support multiple types in primitive definitions (3.54 KB, patch)
2016-08-11 10:37 UTC, Benjamin Rokseth
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 16699: Reference new x-primitives in currently defined objects (4.58 KB, patch)
2016-08-11 10:37 UTC, Benjamin Rokseth
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 16699: Move Swagger-related files to api/v1/swagger (5.59 KB, patch)
2016-08-11 10:37 UTC, Benjamin Rokseth
Details | Diff | Splinter Review
Bug 16699: Remove requirement from borrowernumberQueryParam (1.04 KB, patch)
2016-08-11 13:01 UTC, Lari Taskula
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 16699: Remove requirement from borrowernumberQueryParam (1.11 KB, patch)
2016-08-12 12:05 UTC, Benjamin Rokseth
Details | Diff | Splinter Review
Bug 16699: Split parameters and paths in Swagger (25.91 KB, patch)
2016-08-25 18:19 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 16699: Add borrowernumberQueryParam for reusability (3.07 KB, patch)
2016-08-25 18:19 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 16699: Fix mixed-up indentation from 2-4 spaces to 2 spaces (25.77 KB, patch)
2016-08-25 18:19 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 16699: Support multiple types in primitive definitions (3.58 KB, patch)
2016-08-25 18:19 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 16699: Reference new x-primitives in currently defined objects (4.62 KB, patch)
2016-08-25 18:20 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 16699: Move Swagger-related files to api/v1/swagger (5.36 KB, patch)
2016-08-25 18:20 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 16699: Remove requirement from borrowernumberQueryParam (1.16 KB, patch)
2016-08-25 18:20 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 16699: (QA followup) Move minified swagger file into the swagger/ dir (633 bytes, patch)
2016-08-25 18:21 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 16699: (QA followup) Move minified swagger file into the swagger/ dir (704 bytes, patch)
2016-08-25 21:23 UTC, Benjamin Rokseth
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Lari Taskula 2016-06-09 13:59:37 UTC
It's great that Bug 15126 splits Swagger specification over multiple files. However, the problem of still exists as swagger.json grows larger with each new path. We should do the same split as Bug 15126 did, but for paths and parameters. I propose the following structure for paths:

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

Also Swagger should specify the required permissions for each resource. We can do this by adding "x-koha-permission"-field for each path, and using this field when checking for the needed permissions. This way required permissions are well-documented and anyone can instantly find out which permissions they need.
Comment 1 Benjamin Rokseth 2016-06-13 10:06:07 UTC
(In reply to Lari Taskula from comment #0)
> It's great that Bug 15126 splits Swagger specification over multiple files.
> However, the problem of still exists as swagger.json grows larger with each
> new path. We should do the same split as Bug 15126 did, but for paths and
> parameters. I propose the following structure for paths:
> 
> .
> ├── swagger.json
> ├── definitions
> │   └── index.json
> │   └── error.json
> │   └── patron.json
> ├── parameters
> │   └── index.json
> │   └── patron.json
> └── paths
>     ├── index.json
>     └── patrons.json
> 
> Also Swagger should specify the required permissions for each resource. We
> can do this by adding "x-koha-permission"-field for each path, and using
> this field when checking for the needed permissions. This way required
> permissions are well-documented and anyone can instantly find out which
> permissions they need.

Agree with this one, patching swagger.json for each api update/enhancement is cumbersome and prone for errors. As it seems you have already implemented this, please submit patch and I will review asap.
Comment 2 Lari Taskula 2016-06-14 10:48:07 UTC
Thanks for the interest! Unfortunately, I am having some issues with this approach. The specifications won't validate through Swagger-UI validator (online.swagger.io/validator/debug?url=url_to_your_swagger.json). I read a little about the issue, and to my understanding the structure I described above does not seem to meet Swagger2 specification. It seems that the Paths Object does not take a $ref, but Path Item Object, which can take a $ref. This means that we would not meet the Swagger2 specification if we attempted something like this in swagger.json:

..
"paths": {
  "$ref": "paths/index.json"
},
..

In fact the same error happens when running the current master version (the definitions/index.json in Bug 15126) against online.swagger.io-validator. This is the error message I get with the online.swagger.io-validator:

{"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"}}]}

An option would be to keep the proposed structure for development, and use the minifySwagger.pl-script (Bug 16212) to have a valid Swagger specification in the minified swagger.min.json.
Comment 3 Lari Taskula 2016-06-14 15:01:28 UTC Comment hidden (obsolete)
Comment 4 Lari Taskula 2016-06-14 15:01:53 UTC Comment hidden (obsolete)
Comment 5 Lari Taskula 2016-06-14 15:02:13 UTC Comment hidden (obsolete)
Comment 6 Lari Taskula 2016-06-14 15:02:32 UTC Comment hidden (obsolete)
Comment 7 Lari Taskula 2016-06-14 15:03:21 UTC Comment hidden (obsolete)
Comment 8 Lari Taskula 2016-06-14 15:03:45 UTC Comment hidden (obsolete)
Comment 9 Lari Taskula 2016-06-14 15:12:17 UTC
The patches above with Bug 16212 is my proposal for splitting the Swagger spec. This passes online.swagger.io/validator validations and passes all current REST tests in master. I think it is essential to split the specification yet also maintain its validity against the swagger.io-validator. I'm open to ideas and criticism :)

I will soon provide the patch for defining the required permissions in Swagger paths spec.
Comment 10 Lari Taskula 2016-06-20 09:46:10 UTC
The second part of this Bug duplicated Bug 14868. Moving the permission definitions in Swagger spec to Bug 14868.
Comment 11 Lari Taskula 2016-06-20 12:37:44 UTC Comment hidden (obsolete)
Comment 12 Lari Taskula 2016-06-20 12:42:19 UTC Comment hidden (obsolete)
Comment 13 Olli-Antti Kivilahti 2016-06-30 11:19:05 UTC Comment hidden (obsolete)
Comment 14 Olli-Antti Kivilahti 2016-06-30 11:22:11 UTC Comment hidden (obsolete)
Comment 15 Olli-Antti Kivilahti 2016-06-30 11:23:17 UTC Comment hidden (obsolete)
Comment 16 Olli-Antti Kivilahti 2016-06-30 11:23:51 UTC Comment hidden (obsolete)
Comment 17 Olli-Antti Kivilahti 2016-06-30 11:24:19 UTC Comment hidden (obsolete)
Comment 18 Olli-Antti Kivilahti 2016-06-30 11:24:56 UTC Comment hidden (obsolete)
Comment 19 Olli-Antti Kivilahti 2016-06-30 11:26:04 UTC Comment hidden (obsolete)
Comment 20 Johanna Räisä 2016-06-30 13:33:30 UTC Comment hidden (obsolete)
Comment 21 Johanna Räisä 2016-06-30 13:34:11 UTC Comment hidden (obsolete)
Comment 22 Johanna Räisä 2016-06-30 13:34:34 UTC Comment hidden (obsolete)
Comment 23 Johanna Räisä 2016-06-30 13:35:15 UTC Comment hidden (obsolete)
Comment 24 Johanna Räisä 2016-06-30 13:37:39 UTC Comment hidden (obsolete)
Comment 25 Johanna Räisä 2016-06-30 13:39:13 UTC Comment hidden (obsolete)
Comment 26 Johanna Räisä 2016-06-30 13:51:39 UTC Comment hidden (obsolete)
Comment 27 Johanna Räisä 2016-06-30 13:51:54 UTC Comment hidden (obsolete)
Comment 28 Johanna Räisä 2016-06-30 13:52:16 UTC Comment hidden (obsolete)
Comment 29 Johanna Räisä 2016-06-30 13:52:34 UTC Comment hidden (obsolete)
Comment 30 Johanna Räisä 2016-06-30 13:52:45 UTC Comment hidden (obsolete)
Comment 31 Johanna Räisä 2016-06-30 13:53:04 UTC Comment hidden (obsolete)
Comment 32 Benjamin Rokseth 2016-07-08 09:02:48 UTC
> Currently it is not possible to define multiple types for primitive definitions
> in /definitions/*. If you try to use the following
> "firstname": {
>  "type": ["string", "null"],
>  "description": "patron's first name"

mojo swagger2 validate accepts this, and it works, so what is the problem?
Perhaps we should consider online swagger validator faulty?

If you remove agree on my objections to bug 16212 and remove the auto minifying I would be happy to sign off.
Comment 33 Benjamin Rokseth 2016-08-11 10:37:18 UTC Comment hidden (obsolete)
Comment 34 Benjamin Rokseth 2016-08-11 10:37:25 UTC Comment hidden (obsolete)
Comment 35 Benjamin Rokseth 2016-08-11 10:37:38 UTC Comment hidden (obsolete)
Comment 36 Benjamin Rokseth 2016-08-11 10:37:47 UTC Comment hidden (obsolete)
Comment 37 Benjamin Rokseth 2016-08-11 10:37:53 UTC Comment hidden (obsolete)
Comment 38 Benjamin Rokseth 2016-08-11 10:37:59 UTC Comment hidden (obsolete)
Comment 39 Benjamin Rokseth 2016-08-11 10:42:01 UTC
Rebased against 16271 which is now in master.

Removed patched t/api/v1/minifier.t which should be moved to bug #17102

Please test and get this moved further, as it is a pain to rebase swagger definitions as is today ...
Comment 40 Lari Taskula 2016-08-11 13:01:54 UTC Comment hidden (obsolete)
Comment 41 Lari Taskula 2016-08-11 13:08:11 UTC
(In reply to Benjamin Rokseth from comment #39)
> Rebased against 16271 which is now in master.
> 
> Removed patched t/api/v1/minifier.t which should be moved to bug #17102
> 
> Please test and get this moved further, as it is a pain to rebase swagger
> definitions as is today ...
Thanks Benjamin! And I agree, it can be surprisingly time consuming!
Comment 42 Benjamin Rokseth 2016-08-12 12:05:21 UTC Comment hidden (obsolete)
Comment 43 Tomás Cohen Arazi 2016-08-25 18:19:15 UTC
Created attachment 54884 [details] [review]
Bug 16699: Split parameters and paths in Swagger

Parameters and paths should be split in our Swagger specification, because
otherwise swagger.json would become messy with all the paths and their
further specification in the same file. Also parameters should be split
for the same reason.

Instead of using index.json for definitions, parameters and paths, we define
new files "definitions.json", "parameters.json" and "paths.json" in order to
simplify the references. If we kept using index.json and try to reference
"/definitions/error.json" from "/paths/holds.json", reference would be
"../definitions/index.json#/error" instead of now simplified version,
"../definitions.json#/error".

Here is the proposed structure:

.
├── swagger.json
├── definitions.json
├── paths.json
├── parameters.json
├── definitions
│   └── error.json
│   └── patron.json
├── parameters
│   └── patron.json
├── paths
│   └── patrons.json
├── minifySwagger.pl
└── swagger.min.js

The swagger.json paths, definitions and parameters will look as follows:
...
  "paths": {
    "$ref": "paths.json"
  },
  "definitions": {
    "$ref": "definitions.json"
  },
  "parameters": {
    "$ref": "parameters.json"
  }
...

A problem with splitting specification into multiple files directly from
swagger.json (e.g. "paths": { "$ref": "paths.json" }) is that it is not
following the Swagger specification and an error will be thrown by the
Swagger-UI default validator (online.swagger.io/validator).

To overcome this problem, we use the minifySwagger.pl script from Buug 16212.
This allows the developers to work with the structure introduced in this patch
thus allowing developers to split the specification nicely, and still have a
valid Swagger specification in the minified swagger.min.json.

To test:
-2: Apply the minifier-patch in Buug 16212.
-1: Make sure you can validate your specification with Swagger2 validator at
    online.swagger.io/validator/debug?url=url_to_swaggerjson, or install it
    locally from https://github.com/swagger-api/validator-badge.

1. Don't apply this patch yet, but first validate swagger.json
   with swagger.io-validator (or your local version, if you installed it)
2. Observe that validation errors are given
3. Run minifySwagger.pl
4. Validate swagger.min.json with the validator you used in step 1
5. Observe that validation passes and we overcame the invalid specification
   problem in swagger.min.json
6. Apply this patch
7. Run minifySwagger.pl
8. Repeat step 4
9. Observe that validation passes with new structure
10. Run REST tests at t/db_dependents/api/v1

(11. Study the new structure of our Swagger specifications :))

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
We participated in the development of the Mojolicious::Plugin::Swagger and know
it well. We have made an extension to the plugin to provide full CORS support
and have been building all our in-house features on the new REST API.

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 44 Tomás Cohen Arazi 2016-08-25 18:19:28 UTC
Created attachment 54885 [details] [review]
Bug 16699: Add borrowernumberQueryParam for reusability

The borrowernumber as a query parameter should be defined in parameters.json
in order to allow its reusability.

To test:
1. Apply patch
2. Run minifySwagger.pl
3. Validate swagger.min.json in online.swagger.io/validator/debug?url=url_to+
   _your_swagger_min_json or your local swagger-api/validator-badge validator
4. Observe that validation passes

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
We participated in the development of the Mojolicious::Plugin::Swagger and know
it well. We have made an extension to the plugin to provide full CORS support
and have been building all our in-house features on the new REST API.

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 45 Tomás Cohen Arazi 2016-08-25 18:19:37 UTC
Created attachment 54886 [details] [review]
Bug 16699: Fix mixed-up indentation from 2-4 spaces to 2 spaces

These definitions had indentation of 4 spaces, while rest of the specification
uses 2 spaces. This patch simply maintains the consistency in indentations and
provides no other modifications to code.

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
We participated in the development of the Mojolicious::Plugin::Swagger and know
it well. We have made an extension to the plugin to provide full CORS support
and have been building all our in-house features on the new REST API.

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 46 Tomás Cohen Arazi 2016-08-25 18:19:51 UTC
Created attachment 54887 [details] [review]
Bug 16699: Support multiple types in primitive definitions

Currently it is not possible to define multiple types for primitive definitions
in /definitions/*. If you try to use the following
"firstname": {
  "type": ["string", "null"],
  "description": "patron's first name"
}
in definitions.json, online.swagger.io validator will not validate it:
{"messages":["attribute definitions.firstname.type is not of type `string`"]}

One way to get around this issue is to extend definitions with custom
"x-primitives" object, where we will define all reusable primitive definitions.

To test:
1. Add the "firstname" example above to definitions.json
2. Run minifySwagger.pl
3. Validate your specification
4. Observe that error with description mentioned above is given
5. Apply patch
6. Repeat step 2 and 3
7. Observe that validation passes

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
We participated in the development of the Mojolicious::Plugin::Swagger and know
it well. We have made an extension to the plugin to provide full CORS support
and have been building all our in-house features on the new REST API.

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 47 Tomás Cohen Arazi 2016-08-25 18:20:02 UTC
Created attachment 54888 [details] [review]
Bug 16699: Reference new x-primitives in currently defined objects

Since we have defined some basic x-primitives in x-primitives.json, we can now
start to reuse them in our currently defined objects.

To test:
1. Apply patch
2. Run minifySwagger.pl
3. Validate your Swagger specifications
4. Observe that validation passes

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
We participated in the development of the Mojolicious::Plugin::Swagger and know
it well. We have made an extension to the plugin to provide full CORS support
and have been building all our in-house features on the new REST API.

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 48 Tomás Cohen Arazi 2016-08-25 18:20:13 UTC
Created attachment 54889 [details] [review]
Bug 16699: Move Swagger-related files to api/v1/swagger

This patch separates Swagger-specifications and the minifySwagger.pl from other
api-files by moving specifications & minifier into api/v1/swagger.

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
We participated in the development of the Mojolicious::Plugin::Swagger and know
it well. We have made an extension to the plugin to provide full CORS support
and have been building all our in-house features on the new REST API.

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 49 Tomás Cohen Arazi 2016-08-25 18:20:21 UTC
Created attachment 54890 [details] [review]
Bug 16699: Remove requirement from borrowernumberQueryParam

borrowernumberQueryParam shouldn't be required as also changed in Bug 16271.

To test:
1. Don't apply the patch yet, but first minify Swagger and run
   t/db_dependent/api/v1/holds.t
2. Observe that some tests fail with response code 400 when expecting 200.
3. Apply patch and minify Swagger
4. Run t/db_dependent/api/v1/holds.t
5. Observe that tests pass.

Signed-off-by: Benjamin Rokseth <benjamin.rokseth@kul.oslo.kommune.no>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 50 Tomás Cohen Arazi 2016-08-25 18:21:22 UTC Comment hidden (obsolete)
Comment 51 Benjamin Rokseth 2016-08-25 21:23:10 UTC
Created attachment 54892 [details] [review]
Bug 16699: (QA followup) Move minified swagger file into the swagger/ dir

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Benjamin Rokseth <benjamin.rokseth@kul.oslo.kommune.no>
Comment 52 Kyle M Hall 2016-08-26 13:10:59 UTC
Pushed to master for 16.11, thanks Lari!