From 50051483ce3b0ed059e3c91c034086b2b2e67181 Mon Sep 17 00:00:00 2001 From: Erik Kessler Date: Fri, 14 Jan 2022 12:17:09 -0500 Subject: [PATCH 1/4] Add `oneOf` directive 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. --- src/index.ts | 1 + src/type/__tests__/introspection-test.ts | 6 ++++++ src/type/directives.ts | 12 ++++++++++++ src/type/index.ts | 1 + src/utilities/__tests__/buildASTSchema-test.ts | 13 +++++++++---- src/utilities/__tests__/findBreakingChanges-test.ts | 2 ++ src/utilities/__tests__/printSchema-test.ts | 5 +++++ 7 files changed, 36 insertions(+), 4 deletions(-) diff --git a/src/index.ts b/src/index.ts index bde23ad563..c76b29c56d 100644 --- a/src/index.ts +++ b/src/index.ts @@ -64,6 +64,7 @@ export { GraphQLSkipDirective, GraphQLDeprecatedDirective, GraphQLSpecifiedByDirective, + GraphQLOneOfDirective, // "Enum" of Type Kinds TypeKind, // Constant Deprecation Reason diff --git a/src/type/__tests__/introspection-test.ts b/src/type/__tests__/introspection-test.ts index 741d9c0076..de1344901d 100644 --- a/src/type/__tests__/introspection-test.ts +++ b/src/type/__tests__/introspection-test.ts @@ -989,6 +989,12 @@ describe('Introspection', () => { }, ], }, + { + name: 'oneOf', + isRepeatable: false, + locations: ['OBJECT', 'INPUT_OBJECT'], + args: [], + }, ], }, }, diff --git a/src/type/directives.ts b/src/type/directives.ts index bb3e441a43..89b0897a45 100644 --- a/src/type/directives.ts +++ b/src/type/directives.ts @@ -209,6 +209,17 @@ export const GraphQLSpecifiedByDirective: GraphQLDirective = }, }); +/** + * Used to declare an Input Object as a Oneof Input Objects and an Object as a Oneof Object. + */ +export const GraphQLOneOfDirective: GraphQLDirective = 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: {}, +}); + /** * The full list of specified directives. */ @@ -218,6 +229,7 @@ export const specifiedDirectives: ReadonlyArray = GraphQLSkipDirective, GraphQLDeprecatedDirective, GraphQLSpecifiedByDirective, + GraphQLOneOfDirective, ]); export function isSpecifiedDirective(directive: GraphQLDirective): boolean { diff --git a/src/type/index.ts b/src/type/index.ts index 270dd67d35..cf276d1e02 100644 --- a/src/type/index.ts +++ b/src/type/index.ts @@ -133,6 +133,7 @@ export { GraphQLSkipDirective, GraphQLDeprecatedDirective, GraphQLSpecifiedByDirective, + GraphQLOneOfDirective, // Constant Deprecation Reason DEFAULT_DEPRECATION_REASON, } from './directives'; diff --git a/src/utilities/__tests__/buildASTSchema-test.ts b/src/utilities/__tests__/buildASTSchema-test.ts index 7427ebb507..29280474ec 100644 --- a/src/utilities/__tests__/buildASTSchema-test.ts +++ b/src/utilities/__tests__/buildASTSchema-test.ts @@ -23,6 +23,7 @@ import { assertDirective, GraphQLDeprecatedDirective, GraphQLIncludeDirective, + GraphQLOneOfDirective, GraphQLSkipDirective, GraphQLSpecifiedByDirective, } from '../../type/directives'; @@ -222,7 +223,7 @@ describe('Schema Builder', () => { it('Maintains @include, @skip & @specifiedBy', () => { const schema = buildSchema('type Query'); - expect(schema.getDirectives()).to.have.lengthOf(4); + expect(schema.getDirectives()).to.have.lengthOf(5); expect(schema.getDirective('skip')).to.equal(GraphQLSkipDirective); expect(schema.getDirective('include')).to.equal(GraphQLIncludeDirective); expect(schema.getDirective('deprecated')).to.equal( @@ -231,6 +232,7 @@ describe('Schema Builder', () => { expect(schema.getDirective('specifiedBy')).to.equal( GraphQLSpecifiedByDirective, ); + expect(schema.getDirective('oneOf')).to.equal(GraphQLOneOfDirective); }); it('Overriding directives excludes specified', () => { @@ -239,9 +241,10 @@ describe('Schema Builder', () => { directive @include on FIELD directive @deprecated on FIELD_DEFINITION directive @specifiedBy on FIELD_DEFINITION + directive @oneOf on OBJECT `); - expect(schema.getDirectives()).to.have.lengthOf(4); + expect(schema.getDirectives()).to.have.lengthOf(5); expect(schema.getDirective('skip')).to.not.equal(GraphQLSkipDirective); expect(schema.getDirective('include')).to.not.equal( GraphQLIncludeDirective, @@ -252,18 +255,20 @@ describe('Schema Builder', () => { expect(schema.getDirective('specifiedBy')).to.not.equal( GraphQLSpecifiedByDirective, ); + expect(schema.getDirective('oneOf')).to.not.equal(GraphQLOneOfDirective); }); - it('Adding directives maintains @include, @skip & @specifiedBy', () => { + it('Adding directives maintains @include, @skip, @deprecated, @specifiedBy, and @oneOf', () => { const schema = buildSchema(` directive @foo(arg: Int) on FIELD `); - expect(schema.getDirectives()).to.have.lengthOf(5); + expect(schema.getDirectives()).to.have.lengthOf(6); 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('oneOf')).to.not.equal(undefined); }); it('Type modifiers', () => { diff --git a/src/utilities/__tests__/findBreakingChanges-test.ts b/src/utilities/__tests__/findBreakingChanges-test.ts index 5a7956ae5d..ba526deb48 100644 --- a/src/utilities/__tests__/findBreakingChanges-test.ts +++ b/src/utilities/__tests__/findBreakingChanges-test.ts @@ -4,6 +4,7 @@ import { describe, it } from 'mocha'; import { GraphQLDeprecatedDirective, GraphQLIncludeDirective, + GraphQLOneOfDirective, GraphQLSkipDirective, GraphQLSpecifiedByDirective, } from '../../type/directives'; @@ -802,6 +803,7 @@ describe('findBreakingChanges', () => { GraphQLSkipDirective, GraphQLIncludeDirective, GraphQLSpecifiedByDirective, + GraphQLOneOfDirective, ], }); diff --git a/src/utilities/__tests__/printSchema-test.ts b/src/utilities/__tests__/printSchema-test.ts index 84f30fc0e2..ad590af28a 100644 --- a/src/utilities/__tests__/printSchema-test.ts +++ b/src/utilities/__tests__/printSchema-test.ts @@ -656,6 +656,11 @@ describe('Type System Printer', () => { url: String! ) on SCALAR + """ + Indicates an Object is a Oneof Object or an Input Object is a Oneof Input Object. + """ + directive @oneOf on OBJECT | INPUT_OBJECT + """ A GraphQL Schema defines the capabilities of a GraphQL server. It exposes all available types and directives on the server, as well as the entry points for query, mutation, and subscription operations. """ From ba9049aa33a8f82f02968d23bc15448c97124a00 Mon Sep 17 00:00:00 2001 From: Erik Kessler Date: Fri, 14 Jan 2022 12:31:23 -0500 Subject: [PATCH 2/4] Add `__Type.oneOf` 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. --- src/type/__tests__/introspection-test.ts | 89 +++++++++++++++++++++ src/type/definition.ts | 7 ++ src/type/introspection.ts | 10 +++ src/utilities/__tests__/printSchema-test.ts | 1 + src/utilities/extendSchema.ts | 12 +++ 5 files changed, 119 insertions(+) diff --git a/src/type/__tests__/introspection-test.ts b/src/type/__tests__/introspection-test.ts index de1344901d..0ee64385ae 100644 --- a/src/type/__tests__/introspection-test.ts +++ b/src/type/__tests__/introspection-test.ts @@ -372,6 +372,17 @@ describe('Introspection', () => { isDeprecated: false, deprecationReason: null, }, + { + name: 'oneOf', + args: [], + type: { + kind: 'SCALAR', + name: 'Boolean', + ofType: null, + }, + isDeprecated: false, + deprecationReason: null, + }, ], inputFields: null, interfaces: [], @@ -1525,6 +1536,84 @@ describe('Introspection', () => { }); }); + it('identifies oneOf for objects', () => { + const schema = buildSchema(` + type SomeObject @oneOf { + a: String + } + + type AnotherObject { + a: String + b: String + } + + type Query { + someField: String + } + `); + + const source = ` + { + a: __type(name: "SomeObject") { + oneOf + } + b: __type(name: "AnotherObject") { + oneOf + } + } + `; + + expect(graphqlSync({ schema, source })).to.deep.equal({ + data: { + a: { + oneOf: true, + }, + b: { + oneOf: false, + }, + }, + }); + }); + + it('identifies oneOf for input objects', () => { + const schema = buildSchema(` + input SomeInputObject @oneOf { + a: String + } + + input AnotherInputObject { + a: String + b: String + } + + type Query { + someField(someArg: SomeInputObject): String + } + `); + + const source = ` + { + a: __type(name: "SomeInputObject") { + oneOf + } + b: __type(name: "AnotherInputObject") { + oneOf + } + } + `; + + expect(graphqlSync({ schema, source })).to.deep.equal({ + data: { + a: { + oneOf: true, + }, + b: { + oneOf: false, + }, + }, + }); + }); + it('fails as expected on the __type root field without an arg', () => { const schema = buildSchema(` type Query { diff --git a/src/type/definition.ts b/src/type/definition.ts index 090afa367f..07cfd4df2e 100644 --- a/src/type/definition.ts +++ b/src/type/definition.ts @@ -757,6 +757,7 @@ export class GraphQLObjectType { extensions: Readonly>; astNode: Maybe; extensionASTNodes: ReadonlyArray; + isOneOf: boolean; private _fields: ThunkObjMap>; private _interfaces: ThunkReadonlyArray; @@ -768,6 +769,7 @@ export class GraphQLObjectType { this.extensions = toObjMap(config.extensions); this.astNode = config.astNode; this.extensionASTNodes = config.extensionASTNodes ?? []; + this.isOneOf = config.isOneOf ?? false; this._fields = () => defineFieldMap(config); this._interfaces = () => defineInterfaces(config); @@ -936,6 +938,7 @@ export interface GraphQLObjectTypeConfig { extensions?: Maybe>>; astNode?: Maybe; extensionASTNodes?: Maybe>; + isOneOf?: boolean; } interface GraphQLObjectTypeNormalizedConfig @@ -1605,6 +1608,7 @@ export class GraphQLInputObjectType { extensions: Readonly; astNode: Maybe; extensionASTNodes: ReadonlyArray; + isOneOf: boolean; private _fields: ThunkObjMap; @@ -1614,6 +1618,7 @@ export class GraphQLInputObjectType { this.extensions = toObjMap(config.extensions); this.astNode = config.astNode; this.extensionASTNodes = config.extensionASTNodes ?? []; + this.isOneOf = config.isOneOf ?? false; this._fields = defineInputFieldMap.bind(undefined, config); } @@ -1646,6 +1651,7 @@ export class GraphQLInputObjectType { extensions: this.extensions, astNode: this.astNode, extensionASTNodes: this.extensionASTNodes, + isOneOf: this.isOneOf, }; } @@ -1691,6 +1697,7 @@ export interface GraphQLInputObjectTypeConfig { extensions?: Maybe>; astNode?: Maybe; extensionASTNodes?: Maybe>; + isOneOf?: boolean; } interface GraphQLInputObjectTypeNormalizedConfig diff --git a/src/type/introspection.ts b/src/type/introspection.ts index e5fce6f241..e2e6a7b503 100644 --- a/src/type/introspection.ts +++ b/src/type/introspection.ts @@ -323,6 +323,16 @@ export const __Type: GraphQLObjectType = new GraphQLObjectType({ type: __Type, resolve: (type) => ('ofType' in type ? type.ofType : undefined), }, + oneOf: { + type: GraphQLBoolean, + resolve: (type) => { + if (isInputObjectType(type) || isObjectType(type)) { + return type.isOneOf; + } + + return null; + }, + }, } as GraphQLFieldConfigMap), }); diff --git a/src/utilities/__tests__/printSchema-test.ts b/src/utilities/__tests__/printSchema-test.ts index ad590af28a..8c75d0722a 100644 --- a/src/utilities/__tests__/printSchema-test.ts +++ b/src/utilities/__tests__/printSchema-test.ts @@ -703,6 +703,7 @@ describe('Type System Printer', () => { enumValues(includeDeprecated: Boolean = false): [__EnumValue!] inputFields(includeDeprecated: Boolean = false): [__InputValue!] ofType: __Type + oneOf: Boolean } """An enum describing what kind of type a given \`__Type\` is.""" diff --git a/src/utilities/extendSchema.ts b/src/utilities/extendSchema.ts index 998847e9f1..bf666d08c7 100644 --- a/src/utilities/extendSchema.ts +++ b/src/utilities/extendSchema.ts @@ -66,6 +66,7 @@ import { import { GraphQLDeprecatedDirective, GraphQLDirective, + GraphQLOneOfDirective, GraphQLSpecifiedByDirective, } from '../type/directives'; import { introspectionTypes, isIntrospectionType } from '../type/introspection'; @@ -592,6 +593,7 @@ export function extendSchemaImpl( fields: () => buildFieldMap(allNodes), astNode, extensionASTNodes, + isOneOf: isOneOf(astNode), }); } case Kind.INTERFACE_TYPE_DEFINITION: { @@ -646,6 +648,7 @@ export function extendSchemaImpl( fields: () => buildInputFieldMap(allNodes), astNode, extensionASTNodes, + isOneOf: isOneOf(astNode), }); } } @@ -682,3 +685,12 @@ function getSpecifiedByURL( // @ts-expect-error validated by `getDirectiveValues` return specifiedBy?.url; } + +/** + * Given an input object node, returns if the node should be OneOf. + */ +function isOneOf( + node: InputObjectTypeDefinitionNode | ObjectTypeDefinitionNode, +): boolean { + return Boolean(getDirectiveValues(GraphQLOneOfDirective, node)); +} From a9c44796719247041f9cf70e1a032fafa6e76145 Mon Sep 17 00:00:00 2001 From: Erik Kessler Date: Sat, 15 Jan 2022 10:59:45 -0500 Subject: [PATCH 3/4] Validate oneof types 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. --- src/type/__tests__/validation-test.ts | 65 ++++++++++++++++++++++++++- src/type/validate.ts | 42 +++++++++++++++++ 2 files changed, 106 insertions(+), 1 deletion(-) diff --git a/src/type/__tests__/validation-test.ts b/src/type/__tests__/validation-test.ts index 629e0b8c3c..ca8fce5b88 100644 --- a/src/type/__tests__/validation-test.ts +++ b/src/type/__tests__/validation-test.ts @@ -336,7 +336,7 @@ describe('Type System: A Schema must have Object root types', () => { ]); }); - it('rejects a schema extended with invalid root types', () => { + it('rejects a Schema extended with invalid root types', () => { let schema = buildSchema(` input SomeInputObject { test: String @@ -1663,6 +1663,69 @@ describe('Type System: Input Object fields must have input types', () => { }); }); +describe('Type System: Oneof Object fields must be nullable', () => { + it('rejects non-nullable fields', () => { + const schema = buildSchema(` + type Query { + test: SomeObject + } + + type SomeObject @oneOf { + a: String + b: String! + } + `); + expectJSON(validateSchema(schema)).toDeepEqual([ + { + message: + 'Field SomeObject.b must be nullable as it is part of a Oneof Type.', + locations: [{ line: 8, column: 12 }], + }, + ]); + }); +}); + +describe('Type System: Oneof Input Object fields must be nullable', () => { + it('rejects non-nullable fields', () => { + const schema = buildSchema(` + type Query { + test(arg: SomeInputObject): String + } + + input SomeInputObject @oneOf { + a: String + b: String! + } + `); + expectJSON(validateSchema(schema)).toDeepEqual([ + { + message: 'Oneof input field SomeInputObject.b must be nullable.', + locations: [{ line: 8, column: 12 }], + }, + ]); + }); + + it('rejects fields with default values', () => { + const schema = buildSchema(` + type Query { + test(arg: SomeInputObject): String + } + + input SomeInputObject @oneOf { + a: String + b: String = "foo" + } + `); + expectJSON(validateSchema(schema)).toDeepEqual([ + { + message: + 'Oneof input field SomeInputObject.b cannot have a default value.', + locations: [{ line: 8, column: 9 }], + }, + ]); + }); +}); + describe('Objects must adhere to Interface they implement', () => { it('accepts an Object which implements an Interface', () => { const schema = buildSchema(` diff --git a/src/type/validate.ts b/src/type/validate.ts index 92f7078757..fdc0f90b1f 100644 --- a/src/type/validate.ts +++ b/src/type/validate.ts @@ -20,6 +20,7 @@ import { isEqualType, isTypeSubTypeOf } from '../utilities/typeComparators'; import type { GraphQLEnumType, + GraphQLField, GraphQLInputField, GraphQLInputObjectType, GraphQLInterfaceType, @@ -308,6 +309,23 @@ function validateFields( ); } } + + if (isObjectType(type) && type.isOneOf) { + validateOneOfObjectField(type, field, context); + } + } +} + +function validateOneOfObjectField( + type: GraphQLObjectType, + field: GraphQLField, + context: SchemaValidationContext, +): void { + if (isNonNullType(field.type)) { + context.reportError( + `Field ${type.name}.${field.name} must be nullable as it is part of a Oneof Type.`, + field.astNode?.type, + ); } } @@ -531,6 +549,30 @@ function validateInputFields( [getDeprecatedDirectiveNode(field.astNode), field.astNode?.type], ); } + + if (inputObj.isOneOf) { + validateOneOfInputObjectField(inputObj, field, context); + } + } +} + +function validateOneOfInputObjectField( + type: GraphQLInputObjectType, + field: GraphQLInputField, + context: SchemaValidationContext, +): void { + if (isNonNullType(field.type)) { + context.reportError( + `Oneof input field ${type.name}.${field.name} must be nullable.`, + field.astNode?.type, + ); + } + + if (field.defaultValue) { + context.reportError( + `Oneof input field ${type.name}.${field.name} cannot have a default value.`, + field.astNode, + ); } } From 4f07bfb58b7c4b22ca0ad11c0df08226ca79f302 Mon Sep 17 00:00:00 2001 From: Erik Kessler Date: Fri, 21 Jan 2022 13:13:09 -0500 Subject: [PATCH 4/4] Add oneof as a word 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. --- cspell.yml | 1 + 1 file changed, 1 insertion(+) diff --git a/cspell.yml b/cspell.yml index 5b4e41d576..ac1aec6693 100644 --- a/cspell.yml +++ b/cspell.yml @@ -20,6 +20,7 @@ words: - graphiql - sublinks - instanceof + - oneof # Different names used inside tests - Skywalker