-
Notifications
You must be signed in to change notification settings - Fork 9.1k
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
Clarify discriminator + oneOf/anyOf/allOf usage (3.1.1 modified port of #3822) #3846
Conversation
This moves some guidance up to the fixed fields section where it is more obvious, and explicitly designates other configurations as having undefined behavior. It also creates subsections to organize the different topics, pulls key guidance out of the examples and up into those sections, and provides clarification on the ambiguity of names and URIs. Finally, it incorporates ideas from @jdesrosiers regarding the ambiguous `mapping` syntax submitted in a prior PR, but does so in a way that meets our compatibility requirements for patch releases. For the same compatibility reasons, the MUST wording for requiring the named discriminator property in the schema was (regrettably) weakened to a "SHOULD but otherwise undefined", as we have done for other problematic ambiguities. Co-authored-by: Jason Desrosiers <[email protected]>
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.
+1, minor nits
No thanks, you go ahead. But, I appreciate you asking and the recognition. Thanks for getting this work over the finish line. |
I found the "discriminator value" language confusing, because `discriminator` is the field name, and in most cases the value of a field is the value in the OpenAPI Description. Reviewers did not like "discriminator property value", but "discriminating value" makes it clear that this is about the value that actually causes schema selection, and not about the value of the discriminator keyword. Also removed placeholder in favor of just having the headings, as has been done in several other PRs that overlap with new section PRs for internal linking.
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.
Looks good
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.
Looks good. 👍
I commented on an empty H4. If that was not intended it should be fixed but everything else looked fine.
#### <a name="resolvingImplicitConnections"></a>Resolving Implicit Connections | ||
|
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 looks like an empty H4 ... is that what was intended? It looks very odd.
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.
It's to keep the markdown validator from complaining about broken links. That section is being added in another PR. I have like 6 different PRs that cross-reference each other and there's no good way to do this. Of the three ways I've tried, this was the only one that @ralfhandl didn't object to.
It will all get sorted as we merge and resolve conflicts.
@handrews Do you want to merge now? |
Backport (3.0.4) of merge resolution from #3846
Clarify discriminator + oneOf/anyOf/allOf usage (3.2.0 port of #3846)
This is not a direct port of #3822 (although it is close), for two reasons:
allOf
-using schemas is not needed in 3.1.1I have tried to merge the improvements as best I can. There were two notable changes to the improved text:
propertyName
property sadly has to be weakened to a "SHOULD but the behavior is otherwise undefined" due to our tightened compatibility requirements for patch releases - this is how we have handled other problematic ambiguities – in practice, this is essentially as strong since disregarding the SHOULD is clearly not interoperable (see PR Define "undefined" and "implementation-defined" (3.1.1 port of #3449) #3805 for the meaning of "undefined" and "implementation-defined"I also revived some of @jdesrosiers 's ideas that were taken out for compatibility reasons and included a co-author credit; see the commit message below.
Once this PR is accepted, I will backport @jdesrosiers 's changes, as merged here, to 3.0.4 and include a co-author credit (unless you'd rather do it yourself, Jason).
Fixes:
discriminator
usage onallOf
schemas. #3424Commit message:
This moves some guidance up to the fixed fields section where
it is more obvious, and explicitly designates other configurations
as having undefined behavior.
It also creates subsections to organize the different topics, pulls
key guidance out of the examples and up into those sections,
and provides clarification on the ambiguity of names and URIs.
Finally, it incorporates ideas from @jdesrosiers regarding
the ambiguous
mapping
syntax submitted in a prior PR, butdoes so in a way that meets our compatibility requirements
for patch releases.
For the same compatibility reasons, the MUST wording for
requiring the named discriminator property in the schema
was (regrettably) weakened to a "SHOULD but otherwise undefined",
as we have done for other problematic ambiguities.
Co-authored-by: Jason Desrosiers [email protected]