-
-
Notifications
You must be signed in to change notification settings - Fork 211
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
fix: observe validateApiSpec and avoid schema re-checks (performance) #544
Conversation
…press-openapi-validator into cmd/validation-skip
src/framework/ajv/options.ts
Outdated
} | ||
|
||
return { | ||
validateSchema: false, // only needed on startup |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you mean by "only needed on startup"?
With this set to false it blocks ajv validations from running regardless of the value of validateApiSpec. (the ajv check catches duplicate enum values, for example).
adding validateApiSpec to the const { ... } = this.options on line 48 and changing this line to validateSchema: validateApiSpec
allows for full validation suppression when validateApiSpec is set to false but still allows for full schema validation otherwise
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch...
Schema validation occurs here on startup https://github.com/cdimascio/express-openapi-validator/blob/master/src/framework/openapi.schema.validator.ts#L10
that said, ajv isn’t set up the same way there as it is for request and response validation. Perhaps it should be. It may make sense for the startup code above to create an ajv instance by calling the code below (but setting validateSchema true and skipping the req/res specific logic) https://github.com/cdimascio/express-openapi-validator/blob/master/src/framework/ajv/index.ts#L21.
And, in addition, the startup ajv instance will still need the draft schema and open api schema, etc setup.
Doing something like this may make it possible to do a complete validate on startup, which should make it okay to set validateSchema to false in all other cases
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
interesting. running the tests this morning everything appears to be working as expected. The startup check is catching the schema errors when I set validateApiSpec: true
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fantastic! i'll work to get this merged in. thanks for your help
@all-contributors add @aaronluman for code and test |
I've put up a pull request to add @aaronluman! 🎉 |
related to @aaronluman's #543
fixes:
behavior:
validateApiSpec
is observed. currently, setting it tofalse
is ignoredvalidateApiSpec
istrue
.validateApiSpec
isfalse
schema validation is skipped entirely which was the initial intended behavior