-
Notifications
You must be signed in to change notification settings - Fork 1.1k
OneOf inhabitability #1211
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
base: main
Are you sure you want to change the base?
OneOf inhabitability #1211
Changes from 3 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -1594,9 +1594,9 @@ Input Objects are allowed to reference other Input Objects as field types. A | |||||||||||||||||||
| circular reference occurs when an Input Object references itself either directly | ||||||||||||||||||||
| or through referenced Input Objects. | ||||||||||||||||||||
|
|
||||||||||||||||||||
| Circular references are generally allowed, however they may not be defined as an | ||||||||||||||||||||
| unbroken chain of Non-Null singular fields. Such Input Objects are invalid | ||||||||||||||||||||
| because there is no way to provide a legal value for them. | ||||||||||||||||||||
| Circular references are generally allowed, however they may not form cycles such | ||||||||||||||||||||
| that no finite value can be provided. Such Input Objects are invalid because | ||||||||||||||||||||
| there is no way to provide a legal value for them. | ||||||||||||||||||||
|
|
||||||||||||||||||||
| This example of a circularly-referenced input type is valid as the field `self` | ||||||||||||||||||||
| may be omitted or the value {null}. | ||||||||||||||||||||
|
|
@@ -1642,6 +1642,22 @@ input Second { | |||||||||||||||||||
| } | ||||||||||||||||||||
| ``` | ||||||||||||||||||||
|
|
||||||||||||||||||||
| Because _OneOf Input Objects_ require exactly one field to be set and non-null, | ||||||||||||||||||||
| nullable fields do not allow a finite value to be provided as they do in other | ||||||||||||||||||||
| Input Objects. This example is invalid because providing a value for `First` | ||||||||||||||||||||
| requires a non-null `Second`, and constructing a `Second` requires a non-null | ||||||||||||||||||||
| `First`: | ||||||||||||||||||||
|
||||||||||||||||||||
| Because _OneOf Input Objects_ require exactly one field to be set and non-null, | |
| nullable fields do not allow a finite value to be provided as they do in other | |
| Input Objects. This example is invalid because providing a value for `First` | |
| requires a non-null `Second`, and constructing a `Second` requires a non-null | |
| `First`: | |
| _OneOf Input Object_ require exactly one field be provided, and that field | |
| cannot be `null`. This example is invalid because providing a value for `First` | |
| requires a non-null `Second`, and constructing a `Second` requires a non-null | |
| `First`: |
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.
Thanks @benjie !
In applying this change, I re-read the nearby Circular References section and saw some opportunities to simplify:
- Counter Example No 84 uses a different order of
value/selffields from other examples - The
valuefield in these examples isn't discussed in the prose and isn't material to this section.
My inclination is to simplify these examples by removing the value fields, which I think would reduce some noise in these examples. Would you support including this change in this PR?
As a counter-argument, the value fields make these examples more realistic, rather than degenerate examples of recursion. Keeping them implies that the example I added around OneOf should probably also have them.
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 have no preference either way on the end result (that's more a question for @leebyron) but if you want to rework the existing examples please do so via a separate PR not this one.
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.
Scratch that I do have a preference: leave the existing circular references with the value since it shows its nullability doesn't matter to the problem (there's no branching). Leave your current examples with only one field, because they demonstrate the problem clearly without confusing the reader with combinatorics (because there is branching depending on which field you choose).
Changing the order of value/self in example 84 to be consistent would be a fine separate editorial PR if you were so inclined.
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.
Great! I spun this out into another PR at #1214
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.
Not sure if we need this added for clarity... To make it clear that OneOf Input Objects are included?
| - If {fieldType} is not an Input Object type: | |
| - If {fieldType} is not an Input Object type (including variants): |
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 this has the potential to add more confusion than it clarifies. "Input Object type", without an 'all variants' qualifier, is used consistently to describe OneOf and non-OneOf types. Adding "(including variants)" only here might make readers wonder whether the unqualified uses elsewhere are intentionally exclusive.
That said, you know the spec's editorial voice better than I do, so happy to add it if you think it helps.
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.
Nah, I also don’t think it should be there, but I think it might warrant discussion.
Uh oh!
There was an error while loading. Please reload this page.
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 understand the issue and the counter-example below, but am having trouble parsing/understanding the meaning of this sentence "nullable fields do not allow a finite value to be provided as they do in other Input Objects". Maybe it could be rephrased or maybe it's just me :)
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.
That's fair -- I was trying to describe that while a nullable field would normally provide an escape from infinite recursion, the field-value-may-not-be-null requirement of a OneOf prevents using this escape.