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

propagate validateApiSpec: false to ajv #543

Closed

Conversation

aaronluman
Copy link
Contributor

@aaronluman aaronluman commented Feb 25, 2021

resolves issue 541

For context. We have a somewhat large spec file and would like to avoid the overhead of validating the spec on every request. There is also a concern that concurrent merges to our deploy could interject duplicate enum values, etc which would effectively kill the api. I created a test in our system specifically for validating the spec.

There is possibly a better way to do this and I am open to any suggestions that you might have.

For example, we could change the validateApiSpec key to an object|boolean (see OpenApiValidatorOpts.validateResponses) so that users of this package can decide how deeply they want to disable the validation.

Also, there is probably a cleaner way to pass the validateSchema flag to ajv than to override the options on each validator.

I didn't add any new tests yet but it is not breaking any existing tests and manually verified that ajv/lib/ajv.js:validateSchema does not get called with this override

-- edit --

I updated the branch so that it allows for passing in boolean or validation suppression options and added validateSchema to the baseOptions response

@aaronluman aaronluman force-pushed the 541-disable-ajv-validateSchema branch from 3f1f660 to 9facb84 Compare February 26, 2021 23:51
@cdimascio
Copy link
Owner

cdimascio commented Feb 27, 2021

@aaronluman thanks for the PR and tackling disabling spec validation on startup. You also mentioned validation per request. can you describe in detail what is being validated on every request that you do not expect? Thanks!

@cdimascio
Copy link
Owner

cdimascio commented Feb 28, 2021

@aaronluman please have a look at #544, I borrowed a number of your ideas but removed some of the configuration.

I believe it addresses this use case.

I'm interested to get your thoughts/comments. thanks!

@aaronluman
Copy link
Contributor Author

@cdimascio I think that your version handles the use case, thanks for the attention. I'll pull your branch into my project this evening after I put the kids to bed and double check but at first glance it looks like it will.

The 'per request' issue is probably not something that most users will run into. Basically, our project is too large to attach this to all of our endpoints at once, so our rollout plan is to attach it to a few endpoints at a time. While testing I found that having:

...
app.post('/example-one/id', postExampleOneHandler); // not ready to implement validation
...
app.get('/example-one', OpenApiValidator.middleware(config), getExampleOneHandler);
app.put('/example-one/id', OpenApiValidator.middleware(config), putExampleOneHandler);
...

will cause the the openapi spec file to load / parse / validate multiple times. I resolved this internally by wrapping the OpenApiValidator.middleware(config) with a singleton so that only loads once

@cdimascio
Copy link
Owner

#544 incorporates key elements from this PR.
closing this out in favor of #544

@cdimascio cdimascio closed this Feb 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

Successfully merging this pull request may close these issues.

2 participants