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

[Java][jersey2] fix oneOf, anyOf documentation #6695

Merged
merged 4 commits into from
Jun 23, 2020
Merged

Conversation

wing328
Copy link
Member

@wing328 wing328 commented Jun 17, 2020

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

  • Read the contribution guidelines.
  • If contributing template-only or documentation-only changes which will change sample output, build the project beforehand.
  • Run the shell script ./bin/generate-samples.shto 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.
  • File the PR against the correct branch: master
  • Copy the technical committee to review the pull request if your PR is targeting a particular programming language.

@wing328 wing328 added the WIP Work in Progress label Jun 17, 2020
## oneOf schemas
{{#oneOf}}
* [{{{.}}}]({{{.}}}.md)
{{/oneOf}}
Copy link
Contributor

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?

Copy link
Member Author

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?

Copy link
Contributor

@spacether spacether Jun 17, 2020

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?

Copy link
Member Author

@wing328 wing328 Jun 18, 2020

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?

Copy link
Contributor

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?

Copy link
Member Author

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?

Copy link
Contributor

@spacether spacether Jun 18, 2020

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

Copy link
Member Author

@wing328 wing328 Jun 18, 2020

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?

Copy link
Contributor

@spacether spacether Jun 18, 2020

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?

Copy link
Contributor

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}}
Copy link
Contributor

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?

@wing328
Copy link
Member Author

wing328 commented Jun 23, 2020

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants