OneOf Inhabitability#4564
Conversation
|
@jbellenger is attempting to deploy a commit to the The GraphQL Foundation Team on Vercel. A member of the Team first needs to authorize it. |
jbellenger
force-pushed
the
jbellenger-oneof-inhabitability
branch
from
64e8899 to
bd3e1bf
Compare
benjie
left a comment
benjie
left a comment
There was a problem hiding this comment.
Great tests! (It's 10:30pm so I've not tried to review the code... 😅)
jbellenger
force-pushed
the
jbellenger-oneof-inhabitability
branch
from
bd3e1bf to
ba19546
Compare
benjie
added
the
spec RFC
| for (const type of typesWithUnbreakableCycles) { | ||
| const cyclePath = traceUnbreakableCycle(type, typesWithUnbreakableCycles); | ||
| const pathStr = cyclePath.map((p) => p.fieldStr).join(', '); | ||
| context.reportError( | ||
| `Input Object ${type} references itself via the required fields: ${pathStr}.`, | ||
| cyclePath.map((p) => p.astNode), |
There was a problem hiding this comment.
Out of curiosity, why move the generation of the error message to a second traversal vs building it while traversing the type before?
There was a problem hiding this comment.
Okay. Guessing this is problematic for stuff like this:
input A @oneOf {
b: B
c: C
}
input B {
a: A!
}
input C {
a: A!
}Current errors are:
Input Object A references itself via the required fields: A.b, B.a.
Input Object B references itself via the required fields: B.a, A.b.
Input Object C references itself via the required fields: C.a, A.b, B.a.
Which is a bit misleading as those fields aren't really required. Not 100% sure how to improve the diagnostics though.
There was a problem hiding this comment.
This is a good callout, in particular the validation error for C is not very helpful here.
My gut feeling is that this kind of validation is a property of a graph of types, rather than individual types, and that this will probably be easier to understand if reported as a single error that describes the participating types and at least one illegal path.
Something like (just riffing):
"Input Objects A, B, and C cannot be provided finite values because they form an unbreakable cycle through fields: A.b, A.c, B.a, and C.a."
wdyt?
There was a problem hiding this comment.
Agreed, feels like something like SCC.
This is a diagnostics issue and this PR already provides a fix. We could merge this one as is and make a second one down the road.
There was a problem hiding this comment.
I took a closer look at the existing behavior. Contrary to what we discussed in graphql-wg, todays behavior is not exactly 1 error per type in a cycle -- it's closer to 1 error per distinct cycle.
The most representative test case for this behavior is this one. I've updated the validator in this PR to follow this same pattern.
There is also a case of overlapping cycles, which today get reported as separate errors (test).
For example, the schema you posted above is a good illustration of 2 distinct cycles that overlap through type A:
input A @oneOf { b:B, c:C }
input B { a:A! }
input C { a:A! }While the validator is already halfway to SCC, going all the way would, I think, probably result in reporting errors for this example as a single error (rather than 2), since all 3 types are strongly connected.
So in the latest revision, this graph produces these 2 errors, which match the existing behavior:
Input Object A cannot be provided a finite value because it references itself through fields: A.b, B.a.
Input Object A cannot be provided a finite value because it references itself through fields: A.c, C.a.
Rework the circular references validation to detect input object types that cannot be provided a finite value. This covers: - Self-recursive OneOf types (e.g. input A @OneOf { a: A }) - Mixed OneOf/non-OneOf cycles with no escape path - Standard non-null circular references (existing behavior preserved) Rename algorithms to match spec terminology: - InputObjectHasUnbreakableCycle (was InputObjectCanBeProvidedAFiniteValue) - InputFieldTypeHasUnbreakableCycle (was FieldTypeCanBeProvidedAFiniteValue)
yaacovCR
force-pushed
the
jbellenger-oneof-inhabitability
branch
from
ba19546 to
c5a600f
Compare
- add memoization to improve validation performance - graceful handling of empty oneof defintions - add missing test coverage
Update error reporting for uninhabited cycles to match the previous implementation, which would report 1 error per distinct cycle. Also, update the errors to use the spec language "cannot be provided a finite value"
4 participants
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:
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 because it does not allow construction of a finite value.
This PR updates the existing cycle validation to consider OneOf types. I've tried to follow the existing conventions, though I'm also new here and might have missed something. Any feedback on style or substance is welcome.