-
-
Notifications
You must be signed in to change notification settings - Fork 222
fix: Remove all OpenAPI examples from spec before validating #1040
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: Remove all OpenAPI examples from spec before validating #1040
Conversation
Originally, this PR also ran If you want, I can create a separate PR to fix |
thanks for the PR @max-at-silverflow. currently, schema.preporcessor traverses the schema. as part of this process it attempts to remove examples (the logic is in schemaVisitor which calls removeExamples). given this PR, i suspect that is not actually remove all examples. can this PR be updated to leverage/fix the existing process. Note that the existing process attempts to clean up examples through a single schema traversal. this logic appears to add a second traversal. i'd like to ensure the single pass is maintained for performance. Pointers regarding the current remove examples logic: currently, the schema.preprocessor.ts exposes an entry point method this.traverseSchemas(schemaNodes, (parent, schema, opts) =>
this.schemaVisitor(parent, schema, opts),
); schema visitor is called during the schema walk. This visitor includes a call to the e.g. private schemaVisitor(
parent: SchemaObjectNode,
node: SchemaObjectNode,
opts: TraversalStates,
) {
const pschemas = [parent?.schema];
const nschemas = [node.schema];
// ...
// visit the node in both the request and response schema
for (let i = 0; i < nschemas.length; i++) {
// ...
this.removeExamples(pschema, nschema, options)
//.. ideally, this PR leverages this call path |
Hey @cdimascio! Thanks for the response, that makes a lot of sense! I will see that I can implement it in a compatible way! |
@max-at-silverflow fyi, i pushed the following change which should help with some example(s) removal |
Thanks a lot for the changes! This is a great start! Unfortunately, it does not resolve all my issue yet. In particular, duplicated ids in the I am currently trying to understand the pre-processor, hoping to improve the example removal even further. I will see if there is a reasonable way to achieve this. But thanks for your efforts! |
No problem. Thank for continuing to investigate. |
Alright, I made quite some progress on a more complete fix, I will update
this MR in the next few days!
…On Mon, 3 Mar 2025 at 13:39, Carmine DiMascio ***@***.***> wrote:
No problem. Thank for continuing to investigate.
—
Reply to this email directly, view it on GitHub
<#1040 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/BOLD6FVWMBPZNNYJWI7Z2JD2SRERDAVCNFSM6AAAAABVCWP3XOVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDMOJUGI2TAMJQHE>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
[image: cdimascio]*cdimascio* left a comment
(cdimascio/express-openapi-validator#1040)
<#1040 (comment)>
No problem. Thank for continuing to investigate.
—
Reply to this email directly, view it on GitHub
<#1040 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/BOLD6FVWMBPZNNYJWI7Z2JD2SRERDAVCNFSM6AAAAABVCWP3XOVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDMOJUGI2TAMJQHE>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
Okay, I have something. But it is quite a different fix, with lots more implications. So I made a new MR: #1051. I hope that is in your interest! We can probably close this MR in turn, but maybe have a look at the other approach, whether it is acceptable for you. |
closing this out given the variety of related fixes delivered. |
This PR was made to resolve issue #931. It introduces the following changes:
Commit 189d624:
stripExamples
function that takes anOpenAPIV3.DocumentV3
orOpenAPIV3.DocumentV3_1
, removing allexample
andexamples
properties. (Note: This only affects examples provided as part of the spec, not user-defined properties calledexample
orexamples
).stripExamples
in the spec loader to ensure examples are always discarded and do not affect validation in any way.stripExamples
call, this test fails).These changes are necessary to avoid the validator from erroring out when providing specs that include multiple examples with the same value for a property
id
. This is explained in more detail in issue #931, though the proposed solution does not cover all cases. This PR expands on the suggestions and strips all examples, instead of just the component examples.Note:
Originally, this PR also ran
prettier -c -w ./
to improve the code style of the repository (49ffda6). But I realized that were way too many changes, so I reverted them in 8dfdb94. Sorry for the back-and-forth.Closes #931