Implement OneOf Input Objects via @oneOf directive
#3513
Conversation
This adds the @OneOf directive as a default directive to indicate Objects and Input Objects as OneOf Objects/Input Objects which ensure there is exactly one non-null entry. Future commits will work to implement this directive.
This exposes a new boolean `oneOf` field on `__Type` that indicates if a type is `oneOf`. We implement by checking if the AST node for the object contains a `@oneOf` directive.
This validates that all fields of oneOf objects are nullable and that all fields of oneOf input object are nullable and do not have a default value.
This ensures that OneOf Input Objects have exactly one field provided with the correct non-null, type for that field. Additionally, a variable used in a OneOf Input Object must be non-nullable.
This ensures that we enforce the rules for OneOf Input Objects at execution time.
This ensures the OneOf Objects resolve to an object with exactly one non-null entry.
❌ Deploy Preview for compassionate-pike-271cb3 failed.
|
There was a problem hiding this comment.
Thank you for putting the time into writing this, and sorry it has taken me so long to get around to reviewing it.
From reviewing the work you've done, it's clear that I need to put some significant time into specifying oneOf objects, at the moment the implementations in here don't align with what I'm imagining, and they're inconsistent with how input objects are specified. Given the questions your work raises, I think the best approach is for me to figure out the answers before any more work is done towards supporting oneOf objects.
As for oneOf input objects, this is all the spec PR addresses currently; would it make sense to cut down this PR to only deal with oneOf input objects to get this closer to review?
We need to more fully specify how oneOf objects should behave, so we are removing them for now.
Thank you for taking a look! I agree that focusing on oneOf input objects makes sense, so I've stripped out everything around oneOf objects. |
erikkessler1
changed the title
@oneOf directive@oneOf directive
|
Odd, all my comments seem to have been duplicated; I've deleted the duplicates. |
There was a problem hiding this comment.
This is looking good 👍
My main concern right now is very JS-specific, and it's that we've not tested undefined and I think that might cause a whole host of issues with the Object.keys strategy. Though GraphQL typically uses JSON (which drops undefined) this is not always the case; I'm concerned that variables: { a: "abc", b: undefined } which should be valid might not be seen as such. Please add some tests for this so we can check 🙏
|
I've added some test coverage for those cases including the |
erikkessler1
force-pushed
the
oneof-implementation-objects
branch
from
301ba6f
to
6f7f8d5
Compare
There was a problem hiding this comment.
Looks good to me - excellent work @erikkessler1! Will need a proper review from someone more familiar with GraphQL.js than I, but it feels like this implements the behaviour in the oneof spec accurately.
GraphQL.js reviewer: please pay special attention to the error messages, I'm not sure of the conventions here. Feel free to ping me (here or via DM) to discuss if needed.
|
Pushed a commit that hopefully deals with the code coverage issue 👍 |
|
Merged main; CI passing now 🙌 |
|
Great feedback, thanks @IvanGoncharov! |
|
@graphql/graphql-js-reviewers I've merged in the latest @IvanGoncharov Can you dismiss your stale review, please? |
|
@IvanGoncharov any chance you can dismiss the review @benjie mentioned above? I know this has been a long awaited feature for many |
|
@IvanGoncharov It looks like the changes requested from your review have been addressed. Just kindly wanted to remind you about this as the review is stale at this point. @benjie Do you anticipate any further changes expected to be made on this PR? I was just curious about the status of this feature and what else needs to be done. |
|
I'm not aware of any expected changes. |
There was a problem hiding this comment.
It's a partial review. I will find time for other files soon.
Also, please update these files to have the new type with @oneOf: https://github.com/graphql/graphql-js/blob/e17a0897f67305626c6090ce0174f101b7a96fc4/src/__testUtils__/kitchenSinkSDL.ts
IvanGoncharov
added
spec RFC
|
Sorry missed that spec proposal in stage 2, so this PR can actually be merged. @erikkessler1 If you don't mind, I will address stylistic changes by just committing on top of your PR. |
@IvanGoncharov that sounds good to me! |
|
@erikkessler1 Thanks for addressing my review comments, and sorry for the delay 🙇 |
|
@erikkessler1 Sorry, I didn't find time for a quality review. |
Follow up from graphql#3513. I think it is fine for the spec text in graphql/graphql-spec#825 to refer to `OneOf Input Object`, as the spec explains what that is in another section. In a directive definition, that explanation by itself does not make much sense without prior knowledge.
Co-authored-by: Benjie Gillam <benjie@jemjie.com> Closes graphql/graphql-wg#648
This PR implements graphql/graphql-spec#825 to add a
@oneOfdirective that indicatesan Object is a OneOf Object oran Input Object is a OneOf Input Object. We then validateOneOf Objects andOneOf Input Objects conform to the spec during the validation and execution phases.Closes graphql/graphql-wg#648