-
Notifications
You must be signed in to change notification settings - Fork 533
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
Added flag to avoid aggregation of anyOf/allOf/oneOf schemas while using resolveFully() #568
Conversation
…lving fully the spec
@webron can you accept this pr? I need that for some bugs inside vertx-web (vert-x3/vertx-web#733 and vert-x3/vertx-web#728) |
There are too many diff. just because of space. Can you please just have your changes visible as part of diff. |
I've reverted back some whitespaces removed automatically by my ide. Major changes involve in particular:
|
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.
Hi! @slinkydeveloper, I think it looks good, what got me confused for a while was the test, because it checks that the $ref are resolved in the Components->Schemas when aggregateCombinators: false
, and not resolved when aggregateCombinators: true
, but I think we could add to the test what happens with the PathItem that uses the AllOfSchema, e.g the withInvalidComposedModel
post:
operationId: withInvalidComposedModel
x-swagger-router-controller: TestController
requestBody:
content:
"application/json":
schema:
"$ref": "#/components/schemas/ExtendedAddress"
required: false
responses:
'200':
description: success!
The test should show that when aggregateCombinators is false
the properties inside the AllOfSchema (in the pathItem) remain separated. Right now the test is showing that the schema inside components is being resolved or not.
Thanks for this interesting option to the parser!
I've modified a bit the test. Now it checks also the path @gracekarina suggested 😄 |
|
||
assertEquals(allOf.getAllOf().size(), 2); | ||
|
||
assertTrue(allOf.getAllOf().get(0).getProperties().containsKey("street")); |
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.
this should be schema.getAllOf...
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.
Ops, brutal copy paste 😳
Thanks @slinkydeveloper |
Some users don't want to aggregate anyOf/allOf/oneOf schemas but simply wants all refs solved.
This solution adds a flag inside the
ResolverFully
and when schemas are processed It checks this flag. In case user don't want to aggregate, It generates a new list of subschemas solved and saves it to the composedSchema object.I've also added a flag in
ParseOptions
thatOpenAPIV3Parser
reads duringreadContents()
and I've wrote a test. For default the parser aggregate, as is now