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
Created attachment 49949 [details] Newest Swagger-spec validated 100% and pushed to production yay! Includes the swaggerMinifier.pl
Created attachment 49950 [details] [review] Bug 16212: Add script minifySwagger.pl
Created attachment 52368 [details] [review] Bug 16212: Move from misc/devel to api/v1 Move minifySwagger.pl from misc/devel to api/v1 since it is directly related to swagger.json and the usage is simplified when it is in the same location with swagger.json.
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.
Created attachment 52369 [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.
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.
Created attachment 52576 [details] [review] Bug 16212: Automatically minify swagger.json This patch adds automatic minification of swagger.json. Since startup subroutine is executed on each call to REST API, we need to be careful to not do useless computation by minificating swagger.json when not required. Therefore, we need some algorithm to determine whether minification is needed. In this patch, minification is executed in two cases: 1. swagger.min.json does not exist 2. swagger.json is modified more recently than swagger.min.json Includes tests.
Created attachment 52589 [details] [review] Bug 16212: Automatically minify swagger.json This patch adds automatic minification of swagger.json. Since startup subroutine is executed on each call to REST API, we need to be careful to not do useless computation by minificating swagger.json when not required. Therefore, we need some algorithm to determine whether minification is needed. In this patch, minification is executed in two cases: 1. swagger.min.json does not exist 2. any of the specification files is modified more recently than swagger.min.json Includes tests.
Created attachment 52590 [details] [review] Bug 16212: Automatically minify swagger.json This patch adds automatic minification of swagger.json. Since startup subroutine is executed on each call to REST API, we need to be careful to not do useless computation by minificating swagger.json when not required. Therefore, we need some algorithm to determine whether minification is needed. In this patch, minification is executed in two cases: 1. swagger.min.json does not exist 2. any of the specification files is modified more recently than swagger.min.json Includes tests.
Created attachment 52601 [details] [review] Bug 16212: Automatically minify swagger.json This patch adds automatic minification of swagger.json. Since startup subroutine is executed on each call to REST API, we need to be careful to not do useless computation by minificating swagger.json when not required. Therefore, we need some algorithm to determine whether minification is needed. In this patch, minification is executed in two cases: 1. swagger.min.json does not exist 2. any of the specification files is modified more recently than swagger.min.json Includes tests. This feature requires Apache user to have write permissions to swagger.min.json, otherwise we cannot minify the specification automatically. Another solution is to run the minifySwagger.pl manually after each modification or create a daemon that listens to file modifications via "inotify" and executes minification when needed.
Created attachment 52651 [details] [review] Bug 16212: Automatically minify swagger.json This patch adds automatic minification of swagger.json. Since startup subroutine is executed on each call to REST API, we need to be careful to not do useless computation by minificating swagger.json when not required. Therefore, we need some algorithm to determine whether minification is needed. In this patch, minification is executed in two cases: 1. swagger.min.json does not exist 2. any of the specification files is modified more recently than swagger.min.json Includes tests. This feature requires Apache user to have write permissions to swagger.min.json, otherwise we cannot minify the specification automatically. Another solution is to run the minifySwagger.pl manually after each modification or create a daemon that listens to file modifications via "inotify" and executes minification when needed.
Created attachment 52952 [details] [review] Bug 16212: Add script minifySwagger.pl 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
Created attachment 52953 [details] [review] Bug 16212: Move from misc/devel to api/v1 Move minifySwagger.pl from misc/devel to api/v1 since it is directly related to swagger.json and the usage is simplified when it is in the same location with swagger.json. 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
Created attachment 52954 [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
Created attachment 52955 [details] [review] Bug 16212: Automatically minify swagger.json This patch adds automatic minification of swagger.json. Since startup subroutine is executed on each call to REST API, we need to be careful to not do useless computation by minificating swagger.json when not required. Therefore, we need some algorithm to determine whether minification is needed. In this patch, minification is executed in two cases: 1. swagger.min.json does not exist 2. any of the specification files is modified more recently than swagger.min.json Includes tests. This feature requires Apache user to have write permissions to swagger.min.json, otherwise we cannot minify the specification automatically. Another solution is to run the minifySwagger.pl manually after each modification or create a daemon that listens to file modifications via "inotify" and executes minification when needed. 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
Created attachment 52968 [details] [review] [SIGNED-OFF] Bug 16212: Add script minifySwagger.pl 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.
Created attachment 52969 [details] [review] [SIGNED-OFF] Bug 16212: Add script minifySwagger.pl 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.
Created attachment 52970 [details] [review] [SIGNED-OFF] Bug 16212: Move from misc/devel to api/v1 Move minifySwagger.pl from misc/devel to api/v1 since it is directly related to swagger.json and the usage is simplified when it is in the same location with swagger.json. 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.
Created attachment 52971 [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.
Created attachment 52972 [details] [review] [SIGNED-OFF] 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.
Created attachment 52973 [details] [review] Bug 16212: Automatically minify swagger.json This patch adds automatic minification of swagger.json. Since startup subroutine is executed on each call to REST API, we need to be careful to not do useless computation by minificating swagger.json when not required. Therefore, we need some algorithm to determine whether minification is needed. In this patch, minification is executed in two cases: 1. swagger.min.json does not exist 2. any of the specification files is modified more recently than swagger.min.json Includes tests. This feature requires Apache user to have write permissions to swagger.min.json, otherwise we cannot minify the specification automatically. Another solution is to run the minifySwagger.pl manually after each modification or create a daemon that listens to file modifications via "inotify" and executes minification when needed. 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.
Created attachment 52982 [details] [review] [SIGNED-OFF] Bug 16212: Add script minifySwagger.pl 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.
Created attachment 52983 [details] [review] [SIGNED-OFF] Bug 16212: Move from misc/devel to api/v1 Move minifySwagger.pl from misc/devel to api/v1 since it is directly related to swagger.json and the usage is simplified when it is in the same location with swagger.json. 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.
Created attachment 52984 [details] [review] [SIGNED-OFF] 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.
Created attachment 52985 [details] [review] [SIGNED-OFF] Bug 16212: Automatically minify swagger.json This patch adds automatic minification of swagger.json. Since startup subroutine is executed on each call to REST API, we need to be careful to not do useless computation by minificating swagger.json when not required. Therefore, we need some algorithm to determine whether minification is needed. In this patch, minification is executed in two cases: 1. swagger.min.json does not exist 2. any of the specification files is modified more recently than swagger.min.json Includes tests. This feature requires Apache user to have write permissions to swagger.min.json, otherwise we cannot minify the specification automatically. Another solution is to run the minifySwagger.pl manually after each modification or create a daemon that listens to file modifications via "inotify" and executes minification when needed. 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.
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.
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
(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
(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.
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.
(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!
(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.
Created attachment 54286 [details] [review] Bug 16212: Add script minifySwagger.pl 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>
Created attachment 54287 [details] [review] Bug 16212: Move from misc/devel to api/v1 Move minifySwagger.pl from misc/devel to api/v1 since it is directly related to swagger.json and the usage is simplified when it is in the same location with swagger.json. 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>
Created attachment 54288 [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>
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 {}
Created attachment 54870 [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>
Created attachment 54871 [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>
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>
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>
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
Pushed to master for 16.11, thanks Julian, Lari!