OneOf inhabitability#1211
Conversation
✅ Deploy Preview for graphql-spec-draft ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
Excellent catch! I think we can and should fast track this. Once you have the GraphQL.js implementation in place either add it to the next GraphQL WG agenda or let me know if you’d rather I represent it on your behalf. |
benjie
left a comment
benjie
left a comment
There was a problem hiding this comment.
Let's change the named type to an assertion for clarity
viaduct.arbitrary.graphql is a suite of generators used to property test Viaduct and other internal airbnb systems. The generators in this package use a model for GraphQL values, which can be applied to GraphQL objects that require them. Examples of this include the default values for a graphql argument definition, the arguments/variables used in a GraphQL document, or the result of executing a selection set. This value model has evolved over time -- the initial `RawValue` family of types has been gradually replaced with an `IR.Value` model. Currently, both models exist in the code base and are used for slightly different jobs. This PR completely removes the `RawValue` model and updates all systems to use `IR.Value`. Along the way it also updates most components to operate on a ViaductSchema rather than graphql-java's GraphQLSchema. **Better Edgecase Coverage** Before this change, the schema generator would assign default values and applied directives as it generated each type. This created complexity in other parts of the system (eg value generators had to know how to not generate a value for a type that hadn't been defined yet), and made it impossible for the system to generate some kinds of edge cases. This PR changes this behavior to insert default values and applied directives as additional passes, after all types have been generated. This simplifies value generation and allows for more kinds of edge cases to be generated. This ability to generate more edge cases has surfaced some novel issues, such as this potential spec issue with OneOf types: graphql/graphql-spec#1211 Github-Change-Id: 998226 GitOrigin-RevId: 6ae10823b358faf51f883d593d33d6767933ee8b
reframe the issue as as general class of input object habitation, not just a special edge case around pure OneOf trees. This allows handling the case of mixed OneOf/non-OneOf types, like: ```graphql input A @OneOf { b: B } input B { a: A! } ``` To support this, I've also rewritten the previously-proposed `OneOfInputObjectCanBeProvidedAFiniteValue` function as `InputObjectCanBeProvidedAFiniteValue`, which handles both pure inputs, pure OneOfs, and mixed input/OneOfs validation.
|
@benjie after looking at this more, I realized that my previous proposal only dealt with closed OneOf trees, but that one can also define uninhabited types by mixing OneOf and non-OneOfs. For example, this cycle is considered valid by both the current spec and my original proposal: input A @oneOf { b:B }
input B { a:A! }I've updated this PR to fold the overly-specific OneOf rules I'd originally written into the Cyclic References section, capturing the general shape of the problem for both flavors of input objects. This is implemented in graphql-js PR graphql/graphql-js#4564 I was going to plan on being at the graphql-wg call on March 5th, though with the slightly wider scope of this PR I'd be fine to catch a different meeting. |
benjie
left a comment
benjie
left a comment
There was a problem hiding this comment.
Great work! For Non-Null type we normally say "Let {innerType} be the inner type of {...}" or "Let {nullableType} be the unwrapped nullable type of", but these are just editorial nits really - the structure looks correct to me.
| - Let {innerType} be the type wrapped by {fieldType}. | ||
| - Return {FieldTypeCanBeProvidedAFiniteValue(innerType, visited)}. | ||
| - Assert: {fieldType} is a named type. | ||
| - If {fieldType} is not an Input Object type: |
There was a problem hiding this comment.
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.
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.
Nah, I also don’t think it should be there, but I think it might warrant discussion.
There was a problem hiding this comment.
Discussed in the 2026-07-02 graphql-wg.
There were no strong reasons for or against adding this language. I weakly believe leaving this language out is unambiguous so I slightly lean toward making no other edits here.
We agreed we'd leave this open to see if other reasons turn up for or against.
leebyron
added
💡 Proposal (RFC 1)
|
@jbellenger @benjie I have pre-emptively added this to the May working group. Feel free to put your name in front of the agenda item, else I'll be happy to present. |
The viaduct-arbitrary package generates graphql values which can be used in places where graphql literals are allowed, such as schema default values or field arguments. This generator has an issue when generating values for some kinds of OneOf types. For example, consider these types: > input A @OneOf { b:B, c:C } > input B { a:A! } > input C { as: [A!]! } In this schema, the A/B/C types form a nearly [uninhabited](graphql/graphql-spec#1211) graph (ie it is almost a cycle that would require values of this type to be infinite), however the field `C.as` allows exiting infinite recursion by its ability to be set to an empty list. The value generator has some optimizations for OneOfs that occur in cycles, but they are simple and don't know about recursion escapes like `C.as` that might be multiple steps away from the generation site. This can cause a stack overflow, as seen here: https://scans.gradle.com/s/xpr4udwzekcys/tests/task/:core:shared:arbitrary:test/details/viaduct.arbitrary.graphql.ViaductSchemasTest?top-execution=1 This PR fixes this issue by using a strongly connected component graph to select a field outside of the cycle when generating a value for a OneOf when the generator is past its maximum depth. Github-Change-Id: 1039560 GitOrigin-RevId: 8f3e0f825e61ef2975bf5e5f1d94584c9b5a98d8
This PR implements the validation described at graphql/graphql-spec#1211 More discussion is at the link above, but the core issue is around uninhabited OneOf types -- the property that a type can be defined in a way that a value cannot be generated for it. The simplest example of an uninhabited type is this recursive OneOf: ``` input A @OneOf { a:A } ``` While this type definition is currently valid by the rules of OneOf types and circular input types, it violates the spirit of the [Circular References](https://spec.graphql.org/draft/#sec-Input-Objects.Circular-References) because it does not allow construction of a finite value. This PR updates the existing cycle validation to consider OneOf types.
|
Update: implementation at graphql/graphql-js#4564 has been merged. |
leebyron
added
📄 Draft (RFC 2)
6 participants
OneOf types can form graphs for which finite values cannot be created.
The simplest example of this is a self-recursing OneOf type:
While this is the simplest form, OneOfs can also be combined with non-OneOf input object types to create other uninhabited types:
These kinds of types are considered valid on two fronts today:
While this follows the letter of the Circular References part of the spec, it violates the spirit of the spec in that there is no way to create a finite, legal value for these types. I propose that these topologies be declared invalid.
I took a stab at reworking the Circular References section to consider OneOfs and added a more formal specification framed around finite values.