-
-
Notifications
You must be signed in to change notification settings - Fork 6.7k
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
[Java][jersey2] fix oneOf, anyOf documentation #6695
Conversation
## oneOf schemas | ||
{{#oneOf}} | ||
* [{{{.}}}]({{{.}}}.md) | ||
{{/oneOf}} |
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 about when we combine allOf and oneOf together?
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.
Let's step back. What does "combine allOf and oneOf together" really mean? Can you give an example of use case on that? What does the payload look like?
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.
Sure. A use case for this is when a user extracts common shared properties into a schema and they choose to have those properties stored in the oneOf schema as additionalProperties, not through allOf inheritance.
Let's look at an example:
EdibleInterface:
type: object
properties:
calories:
type: integer
Apple:
type: object
properties:
color:
type: string
varietal:
type: string
ComposedSchema:
allOf:
- $ref: '#/components/schemas/EdibleInterface'
oneOf:
- $ref: '#/components/schemas/Apple'
And a sample payload is:
{"calories": 110, "color": "yellow", "varietal": "golden delicious"}
At the ComposedSchema level, allOf validates + oneOf validates, and if we chose to return an instance of Apple when deserializing ComposedSchema, Apple includes the property calories as an additionalProperty.
Aren't json schema definitions additive?
ComposedSchema says that a payload must validate both EdibleInterface + Apple, schemas together, correct?
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.
Let's focus on "ComposedSchema" and please forget about my implementation for the time being. Given the following:
ComposedSchema:
allOf:
- $ref: '#/components/schemas/EdibleInterface'
oneOf:
- $ref: '#/components/schemas/Apple'
oneOf and allOf are defined at the same level. Your understanding is that the properties defined in the oneOf schema are automatically unfolded and included in the object "ComposedSchema" (same level as the properties defined in the allOf schema), right?
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.
Yes, that's what I am trying to convey with the question
ComposedSchema says that a payload must validate both EdibleInterface + Apple, schemas together, correct?
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.
Assuming the answer to your question is yes, then the following is also valid, right?
ComposedSchema:
allOf:
- $ref: '#/components/schemas/EdibleInterface'
oneOf:
- type: string
- type: integer
Does it make sense to you? If it does, what does the payload (e.g. JSON) look like?
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.
I think that it can only be combined when they are of the same type. What do you think?
Because your example asks us to do this
3 + {"calories": 3}
Also, your example also applies to allof of different types which is not possible:
allOf:
- Apple
- integer
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.
I think that it can only be combined when they are of the same type. What do you think?
My view is that it's not up to me (or you) whether there's such a rule to ensure the same type. It comes down to the definition in JSON schema. I may have missed it but I don't see such a rule explicitly (or implicitly) defined.
One more example. What about the following:
ComposedSchema:
allOf:
- $ref: '#/components/schemas/EdibleInterface'
oneOf:
- $ref: '#/components/schemas/Apple'
- $ref: '#/components/schemas/Banana'
oneOf:
- $ref: '#/components/schemas/Dog'
- $ref: '#/components/schemas/Cat'
I assume it's also valid. Do you agree?
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.
Hmm isn't oneof a key? I thought that it was and that you can't have two of the same key.
Based on that thought, I think that it is not valid.
I will need to look at the spec to see what it says. Yup it is not up to us, I am asking about your understanding and interpretation.
Is allOf of mixed types valid to you? If so, what payload can meet those requirements?
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.
From the spec allof is defined as:
The OpenAPI Specification allows combining and extending model definitions using the allOf property of JSON Schema, in effect offering model composition. allOf takes an array of object definitions that are validated independently but together compose a single object.
{{/isEnum}} | ||
{{^isEnum}} | ||
{{^oneOf.isEmpty}} | ||
{{>model_oneof_doc}} |
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 about when properties are set in the composed schema and we have oneOf or anyOf?
@spacether as discussed via Slack, my implementation has limitations that I'll address later. Since this change only updates the documentation, I'll go ahead to merge it. |
Fix oneOf, anyOf auto-generated documentation (.md)
FYI @bbdouglas (2017/07) @sreeshas (2017/08) @jfiala (2017/08) @lukoyanov (2017/09) @cbornet (2017/09) @jeff9finger (2018/01) @karismann (2019/03) @Zomzog (2019/04) @lwlee2608 (2019/10) @bkabrda (2020/01)
PR checklist
./bin/generate-samples.sh
to update all Petstore samples related to your fix. This is important, as CI jobs will verify all generator outputs of your HEAD commit as it would merge with master. These must match the expectations made by your contribution. You may regenerate an individual generator by passing the relevant config(s) as an argument to the script, for example./bin/generate-samples.sh bin/config/java*
. For Windows users, please run the script in Git BASH.master