Implement OneOf Input Objects via `@oneOf` directive #3513

erikkessler1 · 2022-03-18T14:19:31Z

This PR implements graphql/graphql-spec#825 to add a @oneOf directive that indicates ~~an Object is a OneOf Object or~~ an Input Object is a OneOf Input Object. We then validate ~~OneOf Objects and~~ OneOf Input Objects conform to the spec during the validation and execution phases.

Closes graphql/graphql-wg#648

@OneOf

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.

linux-foundation-easycla · 2022-03-18T14:19:36Z

The committers listed above are authorized under a signed CLA.

✅ login: erikkessler1 / name: Erik Kessler (99189d9, 534b243, 25928e3, 24f0d95, 15e39f5, 50bb968)

netlify · 2022-03-18T14:19:38Z

❌ Deploy Preview for compassionate-pike-271cb3 failed.

Name	Link
🔨 Latest commit	`cfc68cd`
🔍 Latest deploy log	https://app.netlify.com/sites/compassionate-pike-271cb3/deploys/64677c014a3b3100086e8c6d

benjie

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?

src/execution/__tests__/oneof-test.ts

We need to more fully specify how oneOf objects should behave, so we are removing them for now.

erikkessler1 · 2022-04-02T13:12:20Z

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?

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.

benjie · 2022-04-04T11:40:16Z

Odd, all my comments seem to have been duplicated; I've deleted the duplicates.

benjie

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 🙏

src/execution/__tests__/oneof-test.ts

src/type/__tests__/introspection-test.ts

src/utilities/__tests__/valueFromAST-test.ts

src/utilities/coerceInputValue.ts

erikkessler1 · 2022-04-08T15:39:28Z

I've added some test coverage for those cases including the variables: { a: "abc", b: undefined } case.

benjie

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.

benjie · 2022-05-23T13:49:59Z

Pushed a commit that hopefully deals with the code coverage issue 👍

benjie · 2022-05-23T14:39:36Z

Merged main; CI passing now 🙌

src/type/introspection.ts

benjie · 2022-05-25T09:45:28Z

Great feedback, thanks @IvanGoncharov!

saltman424 · 2023-04-21T16:31:34Z

@IvanGoncharov any chance you can dismiss the review @benjie mentioned above? I know this has been a long awaited feature for many

yalshekerchi · 2023-05-11T14:09:41Z

@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.

benjie · 2023-05-11T17:17:19Z

I'm not aware of any expected changes.

src/execution/__tests__/oneof-test.ts

IvanGoncharov

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

src/execution/__tests__/oneof-test.ts

src/type/__tests__/introspection-test.ts

IvanGoncharov · 2023-05-18T15:04:50Z

Sorry missed that spec proposal in stage 2, so this PR can actually be merged.
Also missed all the comments (my inbox is in bad shape atm).
@benjie Thank you for the ping 👍

@erikkessler1 If you don't mind, I will address stylistic changes by just committing on top of your PR.
I think it would be faster that way, it takes a lot of time to comment on a lot of tiny changes.

erikkessler1 · 2023-05-18T16:03:19Z

If you don't mind, I will address stylistic changes by just committing on top of your PR.
I think it would be faster that way, it takes a lot of time to comment on a lot of tiny changes.

@IvanGoncharov that sounds good to me!

IvanGoncharov · 2023-05-31T15:50:54Z

@erikkessler1 Thanks for addressing my review comments, and sorry for the delay 🙇
I'm still planning to review/merge this PR, and I will try to find time this weekend.

IvanGoncharov · 2023-06-27T11:49:52Z

@erikkessler1 Sorry, I didn't find time for a quality review.
That said, the last time I reviewed it, all issues were non-critical and could be fixed later.
Since there were no other objections I will merge it as is.

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 <[email protected]> Closes graphql/graphql-wg#648

erikkessler1 added 6 commits March 18, 2022 10:01

Add oneOf directive

99189d9

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.

Add __Type.oneOf

534b243

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.

Validate oneOf types

25928e3

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.

Enforce oneOf rules during the validation phase

24f0d95

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.

Validate oneOf input objects at execution time

15e39f5

This ensures that we enforce the rules for OneOf Input Objects at execution time.

Validate oneOf objects at execution time

50bb968

This ensures the OneOf Objects resolve to an object with exactly one non-null entry.

erikkessler1 mentioned this pull request Mar 18, 2022

Directive for Oneof Objects and Oneof Input Objects erikkessler1/graphql-js#1

Open

benjie requested changes Mar 31, 2022

View reviewed changes

Remove oneOf objects

36f44d3

We need to more fully specify how oneOf objects should behave, so we are removing them for now.

erikkessler1 changed the title ~~Implement OneOf Input Objects and OneOf Objects via @oneOf directive~~ Implement OneOf Input Objects via @oneOf directive Apr 2, 2022

benjie requested changes Apr 4, 2022

View reviewed changes

src/execution/__tests__/oneof-test.ts Show resolved Hide resolved

src/type/__tests__/introspection-test.ts Show resolved Hide resolved

src/utilities/__tests__/valueFromAST-test.ts Show resolved Hide resolved

src/utilities/coerceInputValue.ts Show resolved Hide resolved

graphql deleted a comment from github-actions bot Apr 4, 2022

Add test coverage

6f7f8d5

erikkessler1 force-pushed the oneof-implementation-objects branch from 301ba6f to 6f7f8d5 Compare April 8, 2022 15:40

benjie approved these changes Apr 11, 2022

View reviewed changes

Add another test to increase coverage

ac4bb46

benjie added 2 commits May 23, 2022 15:28

Merge branch 'main' into oneof-implementation-objects

a5b4a47

Upgrade GraphQLError calls

a081f85

IvanGoncharov requested changes May 24, 2022

View reviewed changes

src/type/introspection.ts Outdated Show resolved Hide resolved

src/type/introspection.ts Show resolved Hide resolved

benjie added 2 commits May 25, 2022 10:44

Rename __Type.oneOf to __Type.isOneOf

dc58476

Remove unnecessary return

ec88066

freaz mentioned this pull request Mar 29, 2023

[Feature Request] Missing support for unions superfaceai/one-service#5

Open

IvanGoncharov reviewed May 18, 2023

View reviewed changes

src/execution/__tests__/oneof-test.ts Show resolved Hide resolved

IvanGoncharov requested changes May 18, 2023

View reviewed changes

IvanGoncharov added spec RFC Implementation of a proposed change to the GraphQL specification PR: feature 🚀 requires increase of "minor" version number labels May 18, 2023

erikkessler1 added 3 commits May 19, 2023 08:12

Merge branch 'main' into oneof-implementation-objects

903555d

Fix lint violations

84a87ee

Address PR feedback

cfc68cd

IvanGoncharov merged commit 8cfa3de into graphql:main Jun 27, 2023

erikkessler1 deleted the oneof-implementation-objects branch June 27, 2023 12:16

dondonz mentioned this pull request Jul 11, 2023

Implement @oneOf directive graphql-java/graphql-java#3266

Closed

justlevine mentioned this pull request Jul 16, 2023

Polymorphic input types webonyx/graphql-php#619

Open

spawnia mentioned this pull request Jul 17, 2023

Improve description for @oneOf directive #3937

Merged

jasonbahl mentioned this pull request Aug 11, 2023

Create and update mutations should be created with post types or field groups wp-graphql/wpgraphql-acf#75

Closed

mengledowl mentioned this pull request Oct 10, 2023

@oneOf directive support MichalLytek/type-graphql#1542

Open

mfulton26 mentioned this pull request Dec 9, 2023

directive examples sikanhe/gqtx#75

Open

jasonbahl mentioned this pull request Feb 20, 2024

Introduce support for the @oneOf GraphQL Directive wp-graphql/wp-graphql#3050

Open

yaacovCR mentioned this pull request Mar 22, 2024

Input Value Validation #3813

Merged

benjie pushed a commit that referenced this pull request Jun 21, 2024

Implement OneOf Input Objects via @oneOf directive (#3513)

003f4a9

Co-authored-by: Benjie Gillam <[email protected]> Closes graphql/graphql-wg#648

benjie mentioned this pull request Jun 21, 2024

backport[v16]: Implement OneOf Input Objects via @oneOf directive #4124

Merged

yaacovCR mentioned this pull request Sep 6, 2024

Support @oneOf directive spec graphql/graphiql#3768

Open

7 tasks

jasonkuhrt mentioned this pull request Oct 29, 2024

refactor: treeshakable kind enum #4270

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement OneOf Input Objects via `@oneOf` directive #3513

Implement OneOf Input Objects via `@oneOf` directive #3513

erikkessler1 commented Mar 18, 2022 •

edited

Loading

linux-foundation-easycla bot commented Mar 18, 2022 •

edited

Loading

netlify bot commented Mar 18, 2022 •

edited

Loading

benjie left a comment

erikkessler1 commented Apr 2, 2022

benjie commented Apr 4, 2022

benjie left a comment

erikkessler1 commented Apr 8, 2022

benjie left a comment

benjie commented May 23, 2022

benjie commented May 23, 2022

benjie commented May 25, 2022

saltman424 commented Apr 21, 2023

yalshekerchi commented May 11, 2023

benjie commented May 11, 2023

IvanGoncharov left a comment

IvanGoncharov commented May 18, 2023

erikkessler1 commented May 18, 2023

IvanGoncharov commented May 31, 2023

IvanGoncharov commented Jun 27, 2023

Implement OneOf Input Objects via @oneOf directive #3513

Implement OneOf Input Objects via @oneOf directive #3513

Conversation

erikkessler1 commented Mar 18, 2022 • edited Loading

linux-foundation-easycla bot commented Mar 18, 2022 • edited Loading

netlify bot commented Mar 18, 2022 • edited Loading

❌ Deploy Preview for compassionate-pike-271cb3 failed.

benjie left a comment

Choose a reason for hiding this comment

erikkessler1 commented Apr 2, 2022

benjie commented Apr 4, 2022

benjie left a comment

Choose a reason for hiding this comment

erikkessler1 commented Apr 8, 2022

benjie left a comment

Choose a reason for hiding this comment

benjie commented May 23, 2022

benjie commented May 23, 2022

benjie commented May 25, 2022

saltman424 commented Apr 21, 2023

yalshekerchi commented May 11, 2023

benjie commented May 11, 2023

IvanGoncharov left a comment

Choose a reason for hiding this comment

IvanGoncharov commented May 18, 2023

erikkessler1 commented May 18, 2023

IvanGoncharov commented May 31, 2023

IvanGoncharov commented Jun 27, 2023

Implement OneOf Input Objects via `@oneOf` directive #3513

Implement OneOf Input Objects via `@oneOf` directive #3513

erikkessler1 commented Mar 18, 2022 •

edited

Loading

linux-foundation-easycla bot commented Mar 18, 2022 •

edited

Loading

netlify bot commented Mar 18, 2022 •

edited

Loading