Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
3.1.1 discriminator improvements #2618
3.1.1 discriminator improvements #2618
Changes from 9 commits
4707ffc
bde8f97
314a756
19ea81c
4708f4a
0ae0968
a76933b
a9873a4
1924bf0
6ea52f2
eb5ddee
2d940c5
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 is the main part that's controversial. In Swagger 2.0, the references were strongly implied to be scoped to document "A", and many tools implement that behavior. There are other component name connections that have similar ambiguities and we should address them all at once. (FWIW, I strongly agree with what you've written here, but it would be a de-facto breaking change for some and we recently decided not to do "clarifications" that are de-facto breaking changes).
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.
My understanding is that the
"allOf"
form specifically exists to avoid having to list the subschemas in a"oneOf"
or"anyOf"
, so this should not be changed. I don't personally understand how this even works, and I'm not the only one. But it's related to the controversial thing- if all schema names resolve to the Components Object of what we now call the "entry document", you can just search that one list. I guess? Apparently we need to figure that out.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.
The problem I was trying to solve here is that the example payloads never say which schema is being used to evaluate those payloads. Based on my understanding of the
allOf
usage, none of those schemas could produce the results the documentation says it does, so I introduced something that could.At this point I can only assume that the "Pet" schema is what these payloads are intended to be evaluated against and the behavior described is correct in that case. So, I've reverted my change other than to specify that "Pet" is the schema these payloads are evaluated against. Let me know if that's the right thing to do. I can't see how that makes sense, so I wouldn't be surprised if it's still not correct. Let me know.
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 would need to be removed if the
oneOf
addition to the example is removed.