Directive for Oneof Objects and Oneof Input Objects #1

erikkessler1 · 2022-01-18T15:10:40Z

This sets the groundwork for an implementation of graphql/graphql-spec#825 for Objects and Input Objects (Oneof Fields omitted for now) by adding @oneOf as a built-in directive and exposing __Type.oneOf during schema introspection.

dfreeman · 2022-01-18T15:22:06Z

src/utilities/__tests__/buildASTSchema-test.ts

    expect(schema.getDirective('skip')).to.not.equal(undefined);
    expect(schema.getDirective('include')).to.not.equal(undefined);
    expect(schema.getDirective('deprecated')).to.not.equal(undefined);
    expect(schema.getDirective('specifiedBy')).to.not.equal(undefined);
+    expect(schema.getDirective('include')).to.not.equal(undefined);


Should this be oneOf?

Whoops yeah. Thanks!

dfreeman · 2022-01-18T15:22:50Z

src/type/directives.ts

+new GraphQLDirective({
+  name: 'oneOf',
+  description: 'Indicates an Object is a Oneof Object or an Input Object is a Oneof Input Object.',
+  locations: [DirectiveLocation.OBJECT, DirectiveLocation.INPUT_OBJECT],
+  args: {},
+});


Suggested change

new GraphQLDirective({

name: 'oneOf',

description: 'Indicates an Object is a Oneof Object or an Input Object is a Oneof Input Object.',

locations: [DirectiveLocation.OBJECT, DirectiveLocation.INPUT_OBJECT],

args: {},

});

new GraphQLDirective({

name: 'oneOf',

description: 'Indicates an Object is a Oneof Object or an Input Object is a Oneof Input Object.',

locations: [DirectiveLocation.OBJECT, DirectiveLocation.INPUT_OBJECT],

args: {},

});

dfreeman · 2022-01-18T15:24:39Z

src/type/validate.ts

+    if (isObjectType(type) && type.isOneOf) {
+      validateOneofObjectField(type, field, context);


It seems like the "of" sometimes gets capitalized (e.g. isOneOf) and sometimes doesn't (e.g. validateOneofObjectField)?

validateOneOfObjectField probably make more sense, but I was confused myself what the capitalization should be. In the RFC they are called "Oneof Input Objects" but maybe that is just the titleized version?

He also just writes "oneof" with no caps at all in a comment or two, so I'm not sure there's much consistency to be had ¯\_(ツ)_/¯

Either way it's probably not a big deal, I just happened to notice as I was reading through

jturkel

Looks great (although I don't know much about graphql-js internals)! A few questions:

Can oneOf be applied to interfaces since this is prototyping support in both inputs and outputs?
Can oneOf be applied to type extensions? Is there additional validation required either way?
This is intentionally not implementing one fields as described here right?

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

erikkessler1 · 2022-01-19T14:54:11Z

Can oneOf be applied to interfaces since this is prototyping support in both inputs and outputs?

I imagine it would support interfaces. I'll have to come back to that.

Can oneOf be applied to type extensions? Is there additional validation required either way?

The spec talks about how they apply to extensions here https://github.com/graphql/graphql-spec/pull/825/files#diff-30a69c5a5eded8e1aea52e53dad1181e6ec8f549ca2c50570b035153e2de1c43R1712-R1715 so I'll have to come back and make sure I've captured that part of the spec.

This is intentionally not implementing one fields as described here right?

That's correct, Benjie seemed more concerned with getting objects and input objects implemented:

I think it's fine to drop field arguments ("Oneof Fields" in the spec edit) and go with just object types and input object types.

This adds `oneof` as a valid word so `Oneof` isn't flagged as a typo. I wasn't sure what the correct capitalization should be since the RFC styles it as "Oneof Input Objects" but in the schema it is `__Type.oneOf`. I tried to use "Oneof" in prose and `oneOf` in code.

benjie · 2022-01-28T09:26:08Z

I've only looked at this in GitHub (not checked it out and tried it/etc) but it looks good to me 👍

Capitalisation of oneof vs oneOf is an open question. I prefer oneOf because I think it's clearer - especially for non-native speakers. It seems casing is mixed across different languages, e.g. Swagger & JSON Schema use oneOf but protobuf uses oneof. Let's go with oneOf for now 👍

erikkessler1 · 2022-01-28T16:51:58Z

Capitalisation of oneof vs oneOf is an open question. I prefer oneOf because I think it's clearer - especially for non-native speakers. It seems casing is mixed across different languages, e.g. Swagger & JSON Schema use oneOf but protobuf uses oneof. Let's go with oneOf for now 👍

That makes sense. Do would you imagine that we style it as OneOf when titleized? For example, for the description of the directive, would you imagine it be:

Indicates an Object is a Oneof Object or an Input Object is a Oneof Input Object.

or

Indicates an Object is a OneOf Object or an Input Object is a OneOf Input Object.

benjie · 2022-02-01T07:54:05Z

I think OneOf for consistency.

erikkessler1 · 2022-03-18T17:00:50Z

I've opened this as a PR on the upstream repository graphql#3513

dfreeman reviewed Jan 18, 2022

View reviewed changes

jturkel reviewed Jan 18, 2022

View reviewed changes

erikkessler1 added 3 commits January 19, 2022 09:40

Add oneOf directive

5005148

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

ba9049a

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

a9c4479

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.

erikkessler1 force-pushed the oneof-directive branch from 80262f5 to a9c4479 Compare January 19, 2022 14:43

erikkessler1 mentioned this pull request Jan 21, 2022

[2021-03-04] Implement Oneof Input Objects in GraphQL.js graphql/graphql-wg#648

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Directive for Oneof Objects and Oneof Input Objects #1

Directive for Oneof Objects and Oneof Input Objects #1

erikkessler1 commented Jan 18, 2022

dfreeman Jan 18, 2022

erikkessler1 Jan 18, 2022

dfreeman Jan 18, 2022

dfreeman Jan 18, 2022

erikkessler1 Jan 18, 2022

dfreeman Jan 18, 2022

jturkel left a comment

erikkessler1 commented Jan 19, 2022

benjie commented Jan 28, 2022

erikkessler1 commented Jan 28, 2022

benjie commented Feb 1, 2022

erikkessler1 commented Mar 18, 2022

		if (isObjectType(type) && type.isOneOf) {
		validateOneofObjectField(type, field, context);

Directive for Oneof Objects and Oneof Input Objects #1

Are you sure you want to change the base?

Directive for Oneof Objects and Oneof Input Objects #1

Conversation

erikkessler1 commented Jan 18, 2022

dfreeman Jan 18, 2022

Choose a reason for hiding this comment

erikkessler1 Jan 18, 2022

Choose a reason for hiding this comment

dfreeman Jan 18, 2022

Choose a reason for hiding this comment

dfreeman Jan 18, 2022

Choose a reason for hiding this comment

erikkessler1 Jan 18, 2022

Choose a reason for hiding this comment

dfreeman Jan 18, 2022

Choose a reason for hiding this comment

jturkel left a comment

Choose a reason for hiding this comment

erikkessler1 commented Jan 19, 2022

benjie commented Jan 28, 2022

erikkessler1 commented Jan 28, 2022

benjie commented Feb 1, 2022

erikkessler1 commented Mar 18, 2022