Skip to content

[all] Adds strict spec option #2783

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
May 3, 2019
Merged

[all] Adds strict spec option #2783

merged 3 commits into from
May 3, 2019

Conversation

jimschubert
Copy link
Member

@jimschubert jimschubert commented May 1, 2019

Introduces an option to allow user customization of strict specification
behaviors. For instance, OpenAPI 3.x requires a path object name to be
prefixed with '/' so we append any missing '/', but this may not be
desirable to some users or generators. In this commit, this fix specifically is
the only modification affected.

PR checklist

  • Read the contribution guidelines.
  • Ran the shell script under ./bin/ to update Petstore sample so that CIs can verify the change. (For instance, only need to run ./bin/{LANG}-petstore.sh, ./bin/openapi3/{LANG}-petstore.sh if updating the {LANG} (e.g. php, ruby, python, etc) code generator or {LANG} client's mustache templates). Windows batch files can be found in .\bin\windows\.
  • Filed the PR against the correct branch: master, 3.4.x, 4.0.x. Default: master.
  • Copied the technical committee to review the pull request if your PR is targeting a particular programming language.

Description of the PR

Adds an option which allows users to disable any strict specification behaviors applied during generation. For example, #1034 introduces a modification which prefixes all paths missing a beginning slash to better fit "MUST" definitions in the OpenAPI Specification 3.x. Ideally, this behavior would be rolled up into the parser (i.e. if we've validated via the parser, we shouldn't need to fix the spec document).

Such a switch is necessary because, as discussed in #2702, users may not want the input document to be modified to match strict behaviors against the specification.

Use of this option to disable strict behaviors, however, does mean that one or more generators may not work as expected. For example, if the url part of a document is: http://localhost/api and the path is resource/{id}, this may result in a generated URL such as http://localhost/apiresource/{id}.

Note that there seems to be a discrepancy or error in the OpenAPI 3.0.x specification. A server url object default is a server with relative url /. The url requirement is defined as:

REQUIRED. A URL to the target host. This URL supports Server Variables and MAY be relative, to indicate that the host location is relative to the location where the OpenAPI document is being served. Variable substitutions will be made when a variable is named in {brackets}.

But, URLs with and without trailing / are both valid.

The field name for a paths object is defined as:

A relative path to an individual endpoint. The field name MUST begin with a forward slash (/). The path is appended (no relative URL resolution) to the expanded URL from the Server Object's url field in order to construct the full URL. Path templating is allowed. When matching URLs, concrete (non-templated) paths would be matched before their templated counterparts. Templated paths with the same hierarchy but different templated names MUST NOT exist as they are identical. In case of ambiguous matching, it's up to the tooling to decide which one to use.

So, strictly following the spec here, it seems that when server.url = / and paths["/api"] exist, the desired outcome (no relative URL resolution, appending path to URL) is //api, which obviously isn't correct as this means the protocol-relative hostname api.

This PR does not attempt to resolve any concerns with the above scenario; it is only provided as an example of why we follow strict behaviors by default and need to sometimes modify what a user provides (even when it successfully validates against the spec).

/cc @OpenAPITools/generator-core-team

TODO

Once merged, the option will need to be applied to the gradle plugin. It seems to always resolve against maven with greater precedence over mavenLocal, and I'm hesitant to shift repository settings around and introduce any issues with the build pipeline.

Introduces an option to allow user customization of strict specification
behaviors. For instance, OpenAPI 3.x requires a path object name to be
prefixed with '/' so we append any missing '/', but this may not be
desirable to some users or generators. In this commit, this fix specifically is
the only modification affected.
@@ -214,6 +214,12 @@
description = "Skips the default behavior of validating an input specification.")
private Boolean skipValidateSpec;

@Option(name = {"--strict-spec"},
title = "true/false strict behavior",
description = "'MUST' and 'SHALL' wording in OpenAPI spec is strictly adhered to, e.g. no fixes will be applied to documents which pass validation but don't follow the spec.",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/, e.g. no/. When false, no/

@@ -57,6 +57,7 @@ mvn clean compile
- `logToStderr` - write all log messages (not just errors) to STDOUT
- `enablePostProcessFile` - enable file post-processing hook
- `skipValidateSpec` - Whether or not to skip validating the input spec prior to generation. By default, invalid specifications will result in an error.
- `strictSpec` - Whether or not to treat an input document strictly against the spec. 'MUST' and 'SHALL' wording in OpenAPI spec is strictly adhered to, e.g. no fixes will be applied to documents which pass validation but don't follow the spec.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/, e.g. no/. When false, no/

Copy link
Contributor

@dkliban dkliban left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I left a couple of comments about the docs, but the rest of this looks right to me. Thanks for putting this together!

@jimschubert
Copy link
Member Author

Thanks for the review, it made me realize I had missed documenting the CLI option in a couple places.

Copy link
Member

@wing328 wing328 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@jimschubert jimschubert merged commit a2fb88c into master May 3, 2019
@jimschubert jimschubert deleted the strict-spec-option branch May 3, 2019 14:57
@jimschubert
Copy link
Member Author

Forgot to assign from CodegenConfigurator to CodegenConfig. Opened #2814 with a fix for this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants