Skip to content
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

Add OAS3 parser configuration for strict reference validation on schema load #343

Closed
makdad opened this issue Dec 16, 2021 · 2 comments
Closed

Comments

@makdad
Copy link
Contributor

makdad commented Dec 16, 2021

Per the recent merge in openapi_parser to solve ota42y/openapi_parser#29 - it is now possible for the OAS3 schema parser to validate the schema's references on load.

In the future, we want this to be the default option on openapi_parser, which means there will be a behavior change in Committee as well (and I'm not sure if there is even a use case to allow broken references -- would love to hear input on this topic).

If we want to "upgrade" Committee users, we need to pass a parameter:

openapi = OpenAPIParser.parse_with_filepath(hash, schema_path)
:

openapi = OpenAPIParser.parse_with_filepath(hash, schema_path)

becomes

openapi = OpenAPIParser.parse_with_filepath(hash, schema_path, { strict_reference_validation: true })

We are not changing the interface of OpenAPI Parser, and prior to openapi_parser 1.0.0.beta1, this configuration did not exist -- so passing the config is a "NOOP" if the Committee user isn't using the latest beta -- so we don't have to worry about backward compatibility with the openapi_parser Gem versions.

Doing what I suggest above will break some Committee users who have unknown (or known) invalid references in their OAS3 files. So I think it may be better to offer this strict validation as a configuration on Committee as well (eventually defaulting to "strict", or at least leaving it to the user to decide).

OR, another thought would be to leave the openapi_parser Gem version specified by Committee at an older version -- and make the code change above (relying on the fact it will be ignored by older parsers) -- and then when we decide to make this mandatory on Committee, just bump the openapi_parser referenced Gem version.

Before I attempt a draft implementation on Committee, I'd like to collect feedback from the maintainers.

@ota42y
Copy link
Member

ota42y commented Dec 19, 2021

Thanks for the good suggestion!
This problem is a vexing one 🤔

The committee provides painless updates to users for now. Specifically, when you update a major version, you update to the last minor version of the previous major version, and you can avoid breaking changes by removing all committee warnings.

Also, the committee is now unaware of the OpenAPI Parser for users.

use Committee::Middleware::RequestValidation, schema_path: 'open_api_3/schema.yaml'

So if you set the default to true on the committee side, it will suddenly break when you upgrade and users cannot avoid it based on the committee's warnings.

Therefore, I think we should provide the same options in the committee as in the OpenAPI Parser, and set them to false with a warning if they are not set, otherwise we should pass the set values directly to the OpenAPI Parser.
This way, users can avoid breaking changes based on the committee's warning.

However, since this is an option that probably shouldn't be false, I think the committee should warn people who set it to false from two versions ahead and encourage them to remove the option from three versions ahead and always set it to true.

makdad added a commit to makdad/committee that referenced this issue Dec 23, 2021
makdad added a commit to makdad/committee that referenced this issue Dec 23, 2021
makdad added a commit to makdad/committee that referenced this issue Dec 23, 2021
makdad added a commit to makdad/committee that referenced this issue Dec 28, 2021
ota42y added a commit that referenced this issue Dec 28, 2021
Add OpenAPI3 strict references option to Committee (#343)
@makdad
Copy link
Contributor Author

makdad commented Dec 28, 2021

Closing thanks to merge on above.

@makdad makdad closed this as completed Dec 28, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants