diff --git a/src/__testUtils__/kitchenSinkQuery.ts b/src/__testUtils__/kitchenSinkQuery.ts index 9ed9a7e983..8c66af347e 100644 --- a/src/__testUtils__/kitchenSinkQuery.ts +++ b/src/__testUtils__/kitchenSinkQuery.ts @@ -10,6 +10,17 @@ query queryName($foo: ComplexType, $site: Site = MOBILE) @onQuery { ...frag @onFragmentSpread } } + field3! + requiredField4: field4! + field5? + optionalField6: field6? + unsetListItemsRequiredList: listField[]! + requiredListItemsUnsetList: listField[!] + requiredListItemsRequiredList: listField[!]! + unsetListItemsOptionalList: listField[]? + optionalListItemsUnsetList: listField[?] + optionalListItemsOptionalList: listField[?]? + multidimensionalList: listField[[[!]!]!]! } ... @skip(unless: $foo) { id diff --git a/src/execution/__tests__/executor-test.ts b/src/execution/__tests__/executor-test.ts index fa59d32fd0..5732007861 100644 --- a/src/execution/__tests__/executor-test.ts +++ b/src/execution/__tests__/executor-test.ts @@ -21,6 +21,11 @@ import { } from '../../type/definition'; import { execute, executeSync } from '../execute'; +import { modifiedOutputType } from '../../utilities/applyRequiredStatus'; +import type { + NullabilityModifierNode, + SupportArrayNode, +} from '../../language/ast'; describe('Execute: Handles basic execution tasks', () => { it('throws if no document is provided', () => { @@ -1269,4 +1274,422 @@ describe('Execute: Handles basic execution tasks', () => { expect(result).to.deep.equal({ data: { foo: { bar: 'bar' } } }); expect(possibleTypes).to.deep.equal([fooObject]); }); + + describe('deals with ! and ?', () => { + const schema = new GraphQLSchema({ + query: new GraphQLObjectType({ + name: 'Query', + fields: { + food: { + type: new GraphQLObjectType({ + name: 'Food', + fields: { + name: { type: GraphQLString }, + calories: { type: GraphQLInt }, + }, + }), + resolve() { + return { + name: null, + calories: 10, + }; + }, + }, + lists: { + type: new GraphQLObjectType({ + name: 'Lists', + fields: { + list: { type: new GraphQLList(GraphQLInt) }, + twoDList: { + type: new GraphQLList(new GraphQLList(GraphQLInt)), + }, + threeDList: { + type: new GraphQLList( + new GraphQLList(new GraphQLList(GraphQLInt)), + ), + }, + nonNullThreeDList: { + // [[[!]!]!]! + type: new GraphQLNonNull( + new GraphQLList( + new GraphQLNonNull( + new GraphQLList( + new GraphQLNonNull( + new GraphQLList(new GraphQLNonNull(GraphQLInt)), + ), + ), + ), + ), + ), + }, + mixedThreeDList: { + // [[[?]!]!]? + type: new GraphQLList( + new GraphQLNonNull( + new GraphQLList( + new GraphQLNonNull(new GraphQLList(GraphQLInt)), + ), + ), + ), + }, + }, + }), + resolve() { + return { + list: [null, 1, 2, null], + twoDList: [ + [null, 1, 2, null], + [1, 2, 3], + ], + threeDList: [ + [[null], [null]], + [[null], [null]], + ], + nonNullThreeDList: [ + [[null], [null]], + [[null], [null]], + ], + mixedThreeDList: [ + [[null], [null]], + [[null], [null]], + ], + }; + }, + }, + }, + }), + }); + + it('smoke test', () => { + const plainDocument = parse(` + query { + food { + name + calories + } + } + `); + const plainResult = executeSync({ schema, document: plainDocument }); + + expect(plainResult).to.deep.equal({ + data: { food: { name: null, calories: 10 } }, + }); + }); + + it('null propagates when field that returns null is required', () => { + const singleNonNullOnNullValueDocument = parse(` + query { + food { + name! + calories + } + } + `); + const singleNonNullOnNullValueResult = executeSync({ + schema, + document: singleNonNullOnNullValueDocument, + }); + + expectJSON(singleNonNullOnNullValueResult).toDeepEqual({ + data: { food: null }, + errors: [ + { + locations: [{ column: 13, line: 4 }], + message: 'Cannot return null for non-nullable field Food.name.', + path: ['food', 'name'], + }, + ], + }); + }); + + it('null propagates when field that returns null and field that does not are both required', () => { + const bothNonNullOnNullValueDocument = parse(` + query { + food { + name! + calories! + } + } + `); + const bothNonNullOnNullValueResult = executeSync({ + schema, + document: bothNonNullOnNullValueDocument, + }); + + expectJSON(bothNonNullOnNullValueResult).toDeepEqual({ + data: { food: null }, + errors: [ + { + locations: [{ column: 13, line: 4 }], + message: 'Cannot return null for non-nullable field Food.name.', + path: ['food', 'name'], + }, + ], + }); + }); + + it('null does not propagate up when field that returns does not return null is required', () => { + const singleNonNullOnNonNullValueDocument = parse(` + query { + food { + calories! + } + } + `); + const singleNonNullOnNonNullValueResult = executeSync({ + schema, + document: singleNonNullOnNonNullValueDocument, + }); + + expect(singleNonNullOnNonNullValueResult).to.deep.equal({ + data: { food: { calories: 10 } }, + }); + }); + + it('null propagates when field that returns null is aliased and required', () => { + const nonNullAliasOnNullValueDocument = parse(` + query { + food { + theNameOfTheFood: name! + } + } + `); + const nonNullAliasOnNullValueResult = executeSync({ + schema, + document: nonNullAliasOnNullValueDocument, + }); + + expectJSON(nonNullAliasOnNullValueResult).toDeepEqual({ + data: { food: null }, + errors: [ + { + locations: [{ column: 13, line: 4 }], + message: 'Cannot return null for non-nullable field Food.name.', + path: ['food', 'theNameOfTheFood'], + }, + ], + }); + }); + + it('null propagates when field that returns null is required and inside an inline fragment', () => { + const nonNullInFragmentDocument = parse(` + query { + food { + ... on Food { + name! + } + } + } + `); + const nonNullInFragmentResult = executeSync({ + schema, + document: nonNullInFragmentDocument, + }); + + expectJSON(nonNullInFragmentResult).toDeepEqual({ + data: { food: null }, + errors: [ + { + locations: [{ column: 15, line: 5 }], + message: 'Cannot return null for non-nullable field Food.name.', + path: ['food', 'name'], + }, + ], + }); + }); + + it('null propagates when field that returns null is required, but other aliased value is unaffected', () => { + const aliasedNullAndNonNull = parse(` + query { + nonNullable: food { + name! + calories! + } + + nullable: food { + name + calories! + } + } + `); + const aliasedNullAndNonNullResult = executeSync({ + schema, + document: aliasedNullAndNonNull, + }); + + expectJSON(aliasedNullAndNonNullResult).toDeepEqual({ + data: { nonNullable: null, nullable: { calories: 10, name: null } }, + errors: [ + { + locations: [{ column: 13, line: 4 }], + message: 'Cannot return null for non-nullable field Food.name.', + path: ['nonNullable', 'name'], + }, + ], + }); + }); + + it('lists with incorrect depth fail to execute', () => { + const listsQuery = parse(` + query { + lists { + list[[!]] + } + } + `); + const listsQueryResult = executeSync({ + schema, + document: listsQuery, + }); + + expectJSON(listsQueryResult).toDeepEqual({ + data: { lists: null }, + errors: [ + { + locations: [{ column: 13, line: 4 }], + message: + 'Syntax Error: Something is wrong with the nullability designator. Is the correct list depth being used?', + path: ['lists', 'list'], + }, + ], + }); + }); + + it('lists with required element propagates null to top level list', () => { + const listsQuery = parse(` + query { + lists { + mixedThreeDList[[[!]]] + } + } + `); + const listsQueryResult = executeSync({ + schema, + document: listsQuery, + }); + + expectJSON(listsQueryResult).toDeepEqual({ + data: { lists: { mixedThreeDList: null } }, + errors: [ + { + locations: [{ column: 13, line: 4 }], + message: + 'Cannot return null for non-nullable field Lists.mixedThreeDList.', + path: ['lists', 'mixedThreeDList', 0, 0, 0], + }, + ], + }); + }); + + it('lists with required element propagates null to first error boundary', () => { + const listsQuery = parse(` + query { + lists { + mixedThreeDList[[[!]]?] + } + } + `); + const listsQueryResult = executeSync({ + schema, + document: listsQuery, + }); + + expectJSON(listsQueryResult).toDeepEqual({ + data: { lists: { mixedThreeDList: [null, null] } }, + errors: [ + { + locations: [{ column: 13, line: 4 }], + message: + 'Cannot return null for non-nullable field Lists.mixedThreeDList.', + path: ['lists', 'mixedThreeDList', 0, 0, 0], + }, + { + locations: [{ column: 13, line: 4 }], + message: + 'Cannot return null for non-nullable field Lists.mixedThreeDList.', + path: ['lists', 'mixedThreeDList', 1, 0, 0], + }, + ], + }); + }); + + it('modifiedOutputType produces correct output types with no overrides', () => { + // [[[!]]!]! + const type = new GraphQLNonNull( + new GraphQLList( + new GraphQLNonNull( + new GraphQLList(new GraphQLList(new GraphQLNonNull(GraphQLInt))), + ), + ), + ); + + // [[[]]] + const nullabilityNode: NullabilityModifierNode | SupportArrayNode = { + kind: Kind.LIST_NULLABILITY, + element: { + kind: Kind.LIST_NULLABILITY, + element: { + kind: Kind.LIST_NULLABILITY, + element: undefined, + }, + }, + }; + + const outputType = modifiedOutputType(type, nullabilityNode); + // [[[!]]!]! + const expectedOutputType = new GraphQLNonNull( + new GraphQLList( + new GraphQLNonNull( + new GraphQLList(new GraphQLList(new GraphQLNonNull(GraphQLInt))), + ), + ), + ); + + expect(outputType).to.deep.equal(expectedOutputType); + }); + + it('modifiedOutputType produces correct output types with overrides', () => { + // [[[!]]!]! + const type = new GraphQLNonNull( + new GraphQLList( + new GraphQLNonNull( + new GraphQLList(new GraphQLList(new GraphQLNonNull(GraphQLInt))), + ), + ), + ); + + // [[[]]] + const nullabilityNode: NullabilityModifierNode | SupportArrayNode = { + // kind: Kind.REQUIRED_DESIGNATOR, + // element: { + kind: Kind.LIST_NULLABILITY, + element: { + // kind: Kind.REQUIRED_DESIGNATOR, + // element: { + kind: Kind.LIST_NULLABILITY, + element: { + // kind: Kind.REQUIRED_DESIGNATOR, + // element: { + kind: Kind.LIST_NULLABILITY, + element: undefined, + // } + }, + // }, + }, + // }, + }; + + const outputType = modifiedOutputType(type, nullabilityNode); + // [[[!]]!]! + const expectedOutputType = new GraphQLNonNull( + new GraphQLList( + new GraphQLNonNull( + new GraphQLList(new GraphQLList(new GraphQLNonNull(GraphQLInt))), + ), + ), + ); + + expect(outputType).to.deep.equal(expectedOutputType); + }); + }); }); diff --git a/src/execution/execute.ts b/src/execution/execute.ts index 3dbb6c5cdd..179c0bf9d9 100644 --- a/src/execution/execute.ts +++ b/src/execution/execute.ts @@ -52,7 +52,10 @@ import { isNonNullType, } from '../type/definition'; +import { modifiedOutputType } from '../utilities/applyRequiredStatus'; + import { getVariableValues, getArgumentValues } from './values'; + import { collectFields, collectSubfields as _collectSubfields, @@ -489,7 +492,26 @@ function executeField( return; } - const returnType = fieldDef.type; + let returnType: GraphQLOutputType; + try { + returnType = modifiedOutputType(fieldDef.type, fieldNodes[0].required); + } catch (error) { + const location = fieldNodes[0]?.loc; + let starts: ReadonlyArray | undefined = []; + + /* istanbul ignore next (branch where location is undefined is difficult to test) */ + if (location !== undefined) { + starts = [location.start]; + } + throw new GraphQLError( + 'Syntax Error: Something is wrong with the nullability designator. Is the correct list depth being used?', + undefined, + fieldNodes[0].loc?.source, + starts, + pathToArray(path), + ); + } + const resolveFn = fieldDef.resolve ?? exeContext.fieldResolver; const info = buildResolveInfo( @@ -594,7 +616,7 @@ function handleFieldError( /** * Implements the instructions for completeValue as defined in the - * "Value Completion" section of the spec. + * "Value completion" section of the spec. * * If the field type is Non-Null, then this recursively completes the value * for the inner type. It throws a field error if that completion returns null, diff --git a/src/index.ts b/src/index.ts index 844ecaf5a2..c3125ec4ea 100644 --- a/src/index.ts +++ b/src/index.ts @@ -372,6 +372,7 @@ export { /** Custom validation rules */ NoDeprecatedCustomRule, NoSchemaIntrospectionCustomRule, + RequiredStatusOnFieldMatchesDefinitionRule, } from './validation/index'; export type { ValidationRule } from './validation/index'; @@ -451,6 +452,7 @@ export { DangerousChangeType, findBreakingChanges, findDangerousChanges, + modifiedOutputType, } from './utilities/index'; export type { diff --git a/src/language/__tests__/lexer-test.ts b/src/language/__tests__/lexer-test.ts index e7b247fd2a..89ef5a09cc 100644 --- a/src/language/__tests__/lexer-test.ts +++ b/src/language/__tests__/lexer-test.ts @@ -936,6 +936,13 @@ describe('Lexer', () => { value: undefined, }); + expect(lexOne('?')).to.contain({ + kind: TokenKind.QUESTION_MARK, + start: 0, + end: 1, + value: undefined, + }); + expect(lexOne('$')).to.contain({ kind: TokenKind.DOLLAR, start: 0, @@ -1181,6 +1188,7 @@ describe('isPunctuatorTokenKind', () => { it('returns true for punctuator tokens', () => { expect(isPunctuatorToken('!')).to.equal(true); + expect(isPunctuatorToken('?')).to.equal(true); expect(isPunctuatorToken('$')).to.equal(true); expect(isPunctuatorToken('&')).to.equal(true); expect(isPunctuatorToken('(')).to.equal(true); diff --git a/src/language/__tests__/parser-test.ts b/src/language/__tests__/parser-test.ts index 10b8c6f2ba..ea1f8c1eaf 100644 --- a/src/language/__tests__/parser-test.ts +++ b/src/language/__tests__/parser-test.ts @@ -224,6 +224,492 @@ describe('Parser', () => { ).to.not.throw(); }); + it('parses required field', () => { + const result = parse(dedent` + query { + requiredField! + } + `); + + expectJSON(result).toDeepEqual({ + kind: Kind.DOCUMENT, + loc: { start: 0, end: 26 }, + definitions: [ + { + kind: Kind.OPERATION_DEFINITION, + loc: { start: 0, end: 26 }, + operation: 'query', + name: undefined, + variableDefinitions: [], + directives: [], + selectionSet: { + kind: Kind.SELECTION_SET, + loc: { start: 6, end: 26 }, + selections: [ + { + kind: Kind.FIELD, + loc: { start: 10, end: 24 }, + alias: undefined, + name: { + kind: Kind.NAME, + loc: { start: 10, end: 23 }, + value: 'requiredField', + }, + arguments: [], + directives: [], + selectionSet: undefined, + required: { + kind: Kind.REQUIRED_DESIGNATOR, + element: undefined, + loc: { start: 25, end: 24 }, + }, + }, + ], + }, + }, + ], + }); + }); + + it('parses optional field', () => { + expect(() => + parse(` + query { + optionalField? + } + `), + ).to.not.throw(); + }); + + it('does not parse field with multiple designators', () => { + expect(() => + parse(` + query { + optionalField?! + } + `), + ).to.throw('Syntax Error: Expected Name, found "!".'); + + expect(() => + parse(` + query { + optionalField!? + } + `), + ).to.throw('Syntax Error: Expected Name, found "?".'); + }); + + it('parses required with alias', () => { + expect(() => + parse(` + query { + requiredField: field! + } + `), + ).to.not.throw(); + }); + + it('parses optional with alias', () => { + expect(() => + parse(` + query { + requiredField: field? + } + `), + ).to.not.throw(); + }); + + it('does not parse aliased field with bang on left of colon', () => { + expect(() => + parse(` + query { + requiredField!: field + } + `), + ).to.throw(); + }); + + it('does not parse aliased field with question mark on left of colon', () => { + expect(() => + parse(` + query { + requiredField?: field + } + `), + ).to.throw(); + }); + + it('does not parse aliased field with bang on left and right of colon', () => { + expect(() => + parse(` + query { + requiredField!: field! + } + `), + ).to.throw(); + }); + + it('does not parse aliased field with question mark on left and right of colon', () => { + expect(() => + parse(` + query { + requiredField?: field? + } + `), + ).to.throw(); + }); + + it('parses required within fragment', () => { + expect(() => + parse(` + fragment MyFragment on Query { + field! + } + `), + ).to.not.throw(); + }); + + it('parses optional within fragment', () => { + expect(() => + parse(` + fragment MyFragment on Query { + field? + } + `), + ).to.not.throw(); + }); + + it('parses field with required list elements', () => { + const result = parse(dedent` + { + field[!] + } + `); + + expectJSON(result).toDeepEqual({ + kind: Kind.DOCUMENT, + loc: { start: 0, end: 14 }, + definitions: [ + { + kind: Kind.OPERATION_DEFINITION, + loc: { start: 0, end: 14 }, + operation: 'query', + name: undefined, + variableDefinitions: [], + directives: [], + selectionSet: { + kind: Kind.SELECTION_SET, + loc: { start: 0, end: 14 }, + selections: [ + { + kind: Kind.FIELD, + loc: { start: 4, end: 12 }, + alias: undefined, + name: { + kind: Kind.NAME, + loc: { start: 4, end: 9 }, + value: 'field', + }, + arguments: [], + directives: [], + required: { + kind: Kind.LIST_NULLABILITY, + loc: { start: 9, end: 12 }, + element: { + kind: Kind.REQUIRED_DESIGNATOR, + loc: { start: 11, end: 11 }, + element: undefined, + }, + }, + selectionSet: undefined, + }, + ], + }, + }, + ], + }); + }); + + it('parses field with optional list elements', () => { + const result = parse(dedent` + { + field[?] + } + `); + + expectJSON(result).toDeepEqual({ + kind: Kind.DOCUMENT, + loc: { start: 0, end: 14 }, + definitions: [ + { + kind: Kind.OPERATION_DEFINITION, + loc: { start: 0, end: 14 }, + operation: 'query', + name: undefined, + variableDefinitions: [], + directives: [], + selectionSet: { + kind: Kind.SELECTION_SET, + loc: { start: 0, end: 14 }, + selections: [ + { + kind: Kind.FIELD, + loc: { start: 4, end: 12 }, + alias: undefined, + name: { + kind: Kind.NAME, + loc: { start: 4, end: 9 }, + value: 'field', + }, + arguments: [], + directives: [], + required: { + kind: Kind.LIST_NULLABILITY, + loc: { start: 9, end: 12 }, + element: { + kind: Kind.OPTIONAL_DESIGNATOR, + loc: { start: 11, end: 11 }, + element: undefined, + }, + }, + selectionSet: undefined, + }, + ], + }, + }, + ], + }); + }); + + it('parses field with required list', () => { + const result = parse(dedent` + { + field[]! + } + `); + + expectJSON(result).toDeepEqual({ + kind: Kind.DOCUMENT, + loc: { start: 0, end: 14 }, + definitions: [ + { + kind: Kind.OPERATION_DEFINITION, + loc: { start: 0, end: 14 }, + operation: 'query', + name: undefined, + variableDefinitions: [], + directives: [], + selectionSet: { + kind: Kind.SELECTION_SET, + loc: { start: 0, end: 14 }, + selections: [ + { + kind: Kind.FIELD, + loc: { start: 4, end: 12 }, + alias: undefined, + name: { + kind: Kind.NAME, + loc: { start: 4, end: 9 }, + value: 'field', + }, + arguments: [], + directives: [], + selectionSet: undefined, + required: { + kind: Kind.REQUIRED_DESIGNATOR, + element: { + kind: Kind.LIST_NULLABILITY, + element: undefined, + loc: { start: 9, end: 11 }, + }, + loc: { start: 13, end: 12 }, + }, + }, + ], + }, + }, + ], + }); + }); + + it('parses field with optional list', () => { + const result = parse(dedent` + { + field[]? + } + `); + + expectJSON(result).toDeepEqual({ + kind: Kind.DOCUMENT, + loc: { start: 0, end: 14 }, + definitions: [ + { + kind: Kind.OPERATION_DEFINITION, + loc: { start: 0, end: 14 }, + operation: 'query', + name: undefined, + variableDefinitions: [], + directives: [], + selectionSet: { + kind: Kind.SELECTION_SET, + loc: { start: 0, end: 14 }, + selections: [ + { + kind: Kind.FIELD, + loc: { start: 4, end: 12 }, + alias: undefined, + name: { + kind: Kind.NAME, + loc: { start: 4, end: 9 }, + value: 'field', + }, + arguments: [], + directives: [], + required: { + kind: Kind.OPTIONAL_DESIGNATOR, + element: { + kind: Kind.LIST_NULLABILITY, + loc: { start: 9, end: 11 }, + element: undefined, + }, + loc: { start: 13, end: 12 }, + }, + selectionSet: undefined, + }, + ], + }, + }, + ], + }); + }); + + it('parses multidimensional field with mixed list elements', () => { + const result = parse(dedent` + { + field[[[?]!]]! + } + `); + + expectJSON(result).toDeepEqual({ + kind: Kind.DOCUMENT, + loc: { start: 0, end: 20 }, + definitions: [ + { + kind: Kind.OPERATION_DEFINITION, + loc: { start: 0, end: 20 }, + operation: 'query', + name: undefined, + variableDefinitions: [], + directives: [], + selectionSet: { + kind: Kind.SELECTION_SET, + loc: { start: 0, end: 20 }, + selections: [ + { + kind: Kind.FIELD, + loc: { start: 4, end: 18 }, + alias: undefined, + name: { + kind: Kind.NAME, + loc: { start: 4, end: 9 }, + value: 'field', + }, + arguments: [], + directives: [], + required: { + kind: Kind.REQUIRED_DESIGNATOR, + loc: { start: 19, end: 18 }, + element: { + kind: Kind.LIST_NULLABILITY, + loc: { start: 9, end: 17 }, + element: { + kind: Kind.LIST_NULLABILITY, + loc: { start: 10, end: 16 }, + element: { + kind: Kind.REQUIRED_DESIGNATOR, + loc: { start: 15, end: 15 }, + element: { + kind: Kind.LIST_NULLABILITY, + loc: { start: 11, end: 14 }, + element: { + kind: Kind.OPTIONAL_DESIGNATOR, + loc: { start: 13, end: 13 }, + element: undefined, + }, + }, + }, + }, + }, + }, + selectionSet: undefined, + }, + ], + }, + }, + ], + }); + }); + + it('does not parse field with unbalanced brackets', () => { + expect(() => + parse(` + query { + field[[] + } + `), + ).to.throw('Syntax Error: Expected "]", found "}".'); + + expect(() => + parse(` + query { + field[]] + } + `), + ).to.throw('Syntax Error: Expected Name, found "]".'); + + expect(() => + parse(` + query { + field] + } + `), + ).to.throw('Syntax Error: Expected Name, found "]".'); + + expect(() => + parse(` + query { + field[ + } + `), + ).to.throw('Syntax Error: Expected "]", found "}".'); + }); + + it('does not parse field with assorted invalid nullability designators', () => { + expect(() => + parse(` + query { + field[][] + } + `), + ).to.throw('Syntax Error: Expected Name, found "[".'); + + expect(() => + parse(` + query { + field[!!] + } + `), + ).to.throw('Syntax Error: Expected "]", found "!".'); + + expect(() => + parse(` + query { + field[]?! + } + `), + ).to.throw('Syntax Error: Expected Name, found "!".'); + }); + it('creates ast', () => { const result = parse(dedent` { @@ -275,6 +761,7 @@ describe('Parser', () => { }, ], directives: [], + required: undefined, selectionSet: { kind: Kind.SELECTION_SET, loc: { start: 16, end: 38 }, @@ -290,6 +777,7 @@ describe('Parser', () => { }, arguments: [], directives: [], + required: undefined, selectionSet: undefined, }, { @@ -303,6 +791,7 @@ describe('Parser', () => { }, arguments: [], directives: [], + required: undefined, selectionSet: undefined, }, ], @@ -350,6 +839,7 @@ describe('Parser', () => { }, arguments: [], directives: [], + required: undefined, selectionSet: { kind: Kind.SELECTION_SET, loc: { start: 15, end: 27 }, @@ -365,6 +855,7 @@ describe('Parser', () => { }, arguments: [], directives: [], + required: undefined, selectionSet: undefined, }, ], diff --git a/src/language/__tests__/printer-test.ts b/src/language/__tests__/printer-test.ts index 9c2fcf9c5f..39068b77e1 100644 --- a/src/language/__tests__/printer-test.ts +++ b/src/language/__tests__/printer-test.ts @@ -13,6 +13,7 @@ describe('Printer: Query document', () => { const ast = { kind: Kind.FIELD, name: { kind: Kind.NAME, value: 'foo' }, + required: undefined, } as const; expect(print(ast)).to.equal('foo'); }); @@ -165,6 +166,17 @@ describe('Printer: Query document', () => { ...frag @onFragmentSpread } } + field3! + requiredField4: field4! + field5? + optionalField6: field6? + unsetListItemsRequiredList: listField[]! + requiredListItemsUnsetList: listField[!] + requiredListItemsRequiredList: listField[!]! + unsetListItemsOptionalList: listField[]? + optionalListItemsUnsetList: listField[?] + optionalListItemsOptionalList: listField[?]? + multidimensionalList: listField[[[!]!]!]! } ... @skip(unless: $foo) { id diff --git a/src/language/__tests__/visitor-test.ts b/src/language/__tests__/visitor-test.ts index 903b80d959..7d0ca2d441 100644 --- a/src/language/__tests__/visitor-test.ts +++ b/src/language/__tests__/visitor-test.ts @@ -654,6 +654,118 @@ describe('Visitor', () => { ['leave', 'Field', 1, undefined], ['leave', 'SelectionSet', 'selectionSet', 'Field'], ['leave', 'Field', 0, undefined], + ['enter', 'Field', 1, undefined], + ['enter', 'Name', 'name', 'Field'], + ['leave', 'Name', 'name', 'Field'], + ['enter', 'RequiredDesignator', 'required', 'Field'], + ['leave', 'RequiredDesignator', 'required', 'Field'], + ['leave', 'Field', 1, undefined], + ['enter', 'Field', 2, undefined], + ['enter', 'Name', 'alias', 'Field'], + ['leave', 'Name', 'alias', 'Field'], + ['enter', 'Name', 'name', 'Field'], + ['leave', 'Name', 'name', 'Field'], + ['enter', 'RequiredDesignator', 'required', 'Field'], + ['leave', 'RequiredDesignator', 'required', 'Field'], + ['leave', 'Field', 2, undefined], + ['enter', 'Field', 3, undefined], + ['enter', 'Name', 'name', 'Field'], + ['leave', 'Name', 'name', 'Field'], + ['enter', 'OptionalDesignator', 'required', 'Field'], + ['leave', 'OptionalDesignator', 'required', 'Field'], + ['leave', 'Field', 3, undefined], + ['enter', 'Field', 4, undefined], + ['enter', 'Name', 'alias', 'Field'], + ['leave', 'Name', 'alias', 'Field'], + ['enter', 'Name', 'name', 'Field'], + ['leave', 'Name', 'name', 'Field'], + ['enter', 'OptionalDesignator', 'required', 'Field'], + ['leave', 'OptionalDesignator', 'required', 'Field'], + ['leave', 'Field', 4, undefined], + ['enter', 'Field', 5, undefined], + ['enter', 'Name', 'alias', 'Field'], + ['leave', 'Name', 'alias', 'Field'], + ['enter', 'Name', 'name', 'Field'], + ['leave', 'Name', 'name', 'Field'], + ['enter', 'RequiredDesignator', 'required', 'Field'], + ['enter', 'ListNullabilityDesignator', 'element', 'RequiredDesignator'], + ['leave', 'ListNullabilityDesignator', 'element', 'RequiredDesignator'], + ['leave', 'RequiredDesignator', 'required', 'Field'], + ['leave', 'Field', 5, undefined], + ['enter', 'Field', 6, undefined], + ['enter', 'Name', 'alias', 'Field'], + ['leave', 'Name', 'alias', 'Field'], + ['enter', 'Name', 'name', 'Field'], + ['leave', 'Name', 'name', 'Field'], + ['enter', 'ListNullabilityDesignator', 'required', 'Field'], + ['enter', 'RequiredDesignator', 'element', 'ListNullabilityDesignator'], + ['leave', 'RequiredDesignator', 'element', 'ListNullabilityDesignator'], + ['leave', 'ListNullabilityDesignator', 'required', 'Field'], + ['leave', 'Field', 6, undefined], + ['enter', 'Field', 7, undefined], + ['enter', 'Name', 'alias', 'Field'], + ['leave', 'Name', 'alias', 'Field'], + ['enter', 'Name', 'name', 'Field'], + ['leave', 'Name', 'name', 'Field'], + ['enter', 'RequiredDesignator', 'required', 'Field'], + ['enter', 'ListNullabilityDesignator', 'element', 'RequiredDesignator'], + ['enter', 'RequiredDesignator', 'element', 'ListNullabilityDesignator'], + ['leave', 'RequiredDesignator', 'element', 'ListNullabilityDesignator'], + ['leave', 'ListNullabilityDesignator', 'element', 'RequiredDesignator'], + ['leave', 'RequiredDesignator', 'required', 'Field'], + ['leave', 'Field', 7, undefined], + ['enter', 'Field', 8, undefined], + ['enter', 'Name', 'alias', 'Field'], + ['leave', 'Name', 'alias', 'Field'], + ['enter', 'Name', 'name', 'Field'], + ['leave', 'Name', 'name', 'Field'], + ['enter', 'OptionalDesignator', 'required', 'Field'], + ['enter', 'ListNullabilityDesignator', 'element', 'OptionalDesignator'], + ['leave', 'ListNullabilityDesignator', 'element', 'OptionalDesignator'], + ['leave', 'OptionalDesignator', 'required', 'Field'], + ['leave', 'Field', 8, undefined], + ['enter', 'Field', 9, undefined], + ['enter', 'Name', 'alias', 'Field'], + ['leave', 'Name', 'alias', 'Field'], + ['enter', 'Name', 'name', 'Field'], + ['leave', 'Name', 'name', 'Field'], + ['enter', 'ListNullabilityDesignator', 'required', 'Field'], + ['enter', 'OptionalDesignator', 'element', 'ListNullabilityDesignator'], + ['leave', 'OptionalDesignator', 'element', 'ListNullabilityDesignator'], + ['leave', 'ListNullabilityDesignator', 'required', 'Field'], + ['leave', 'Field', 9, undefined], + ['enter', 'Field', 10, undefined], + ['enter', 'Name', 'alias', 'Field'], + ['leave', 'Name', 'alias', 'Field'], + ['enter', 'Name', 'name', 'Field'], + ['leave', 'Name', 'name', 'Field'], + ['enter', 'OptionalDesignator', 'required', 'Field'], + ['enter', 'ListNullabilityDesignator', 'element', 'OptionalDesignator'], + ['enter', 'OptionalDesignator', 'element', 'ListNullabilityDesignator'], + ['leave', 'OptionalDesignator', 'element', 'ListNullabilityDesignator'], + ['leave', 'ListNullabilityDesignator', 'element', 'OptionalDesignator'], + ['leave', 'OptionalDesignator', 'required', 'Field'], + ['leave', 'Field', 10, undefined], + ['enter', 'Field', 11, undefined], + ['enter', 'Name', 'alias', 'Field'], + ['leave', 'Name', 'alias', 'Field'], + ['enter', 'Name', 'name', 'Field'], + ['leave', 'Name', 'name', 'Field'], + ['enter', 'RequiredDesignator', 'required', 'Field'], + ['enter', 'ListNullabilityDesignator', 'element', 'RequiredDesignator'], + ['enter', 'RequiredDesignator', 'element', 'ListNullabilityDesignator'], + ['enter', 'ListNullabilityDesignator', 'element', 'RequiredDesignator'], + ['enter', 'RequiredDesignator', 'element', 'ListNullabilityDesignator'], + ['enter', 'ListNullabilityDesignator', 'element', 'RequiredDesignator'], + ['enter', 'RequiredDesignator', 'element', 'ListNullabilityDesignator'], + ['leave', 'RequiredDesignator', 'element', 'ListNullabilityDesignator'], + ['leave', 'ListNullabilityDesignator', 'element', 'RequiredDesignator'], + ['leave', 'RequiredDesignator', 'element', 'ListNullabilityDesignator'], + ['leave', 'ListNullabilityDesignator', 'element', 'RequiredDesignator'], + ['leave', 'RequiredDesignator', 'element', 'ListNullabilityDesignator'], + ['leave', 'ListNullabilityDesignator', 'element', 'RequiredDesignator'], + ['leave', 'RequiredDesignator', 'required', 'Field'], + ['leave', 'Field', 11, undefined], ['leave', 'SelectionSet', 'selectionSet', 'InlineFragment'], ['leave', 'InlineFragment', 1, undefined], ['enter', 'InlineFragment', 2, undefined], diff --git a/src/language/ast.ts b/src/language/ast.ts index 0b30366df0..635e802d6f 100644 --- a/src/language/ast.ts +++ b/src/language/ast.ts @@ -1,5 +1,6 @@ import type { Kind } from './kinds'; import type { Source } from './source'; + import type { TokenKind } from './tokenKind'; /** @@ -179,7 +180,10 @@ export type ASTNode = | InterfaceTypeExtensionNode | UnionTypeExtensionNode | EnumTypeExtensionNode - | InputObjectTypeExtensionNode; + | InputObjectTypeExtensionNode + | RequiredModifierNode + | OptionalModifierNode + | SupportArrayNode; /** * Utility type listing all nodes indexed by their kind. @@ -206,7 +210,17 @@ export const QueryDocumentKeys: { VariableDefinition: ['variable', 'type', 'defaultValue', 'directives'], Variable: ['name'], SelectionSet: ['selections'], - Field: ['alias', 'name', 'arguments', 'directives', 'selectionSet'], + Field: [ + 'alias', + 'name', + 'arguments', + 'directives', + 'selectionSet', + 'required', + ], + ListNullabilityDesignator: ['element'], + RequiredDesignator: ['element'], + OptionalDesignator: ['element'], Argument: ['name', 'value'], FragmentSpread: ['name', 'directives'], @@ -360,6 +374,30 @@ export interface FieldNode { readonly arguments?: ReadonlyArray; readonly directives?: ReadonlyArray; readonly selectionSet?: SelectionSetNode; + readonly required?: SupportArrayNode | NullabilityModifierNode; +} + +export interface RequiredModifierNode { + readonly kind: Kind.REQUIRED_DESIGNATOR; + readonly loc?: Location; + readonly element?: SupportArrayNode; +} + +export interface OptionalModifierNode { + readonly kind: Kind.OPTIONAL_DESIGNATOR; + readonly loc?: Location; + readonly element?: SupportArrayNode; +} + +// modifiers can be !, ? or [] +export type NullabilityModifierNode = + | RequiredModifierNode + | OptionalModifierNode; + +export interface SupportArrayNode { + readonly kind: Kind.LIST_NULLABILITY; + readonly loc?: Location; + readonly element?: NullabilityModifierNode | SupportArrayNode; } export interface ArgumentNode { diff --git a/src/language/kinds.ts b/src/language/kinds.ts index 39b2a8e675..3751ec1076 100644 --- a/src/language/kinds.ts +++ b/src/language/kinds.ts @@ -12,6 +12,9 @@ export enum Kind { SELECTION_SET = 'SelectionSet', FIELD = 'Field', ARGUMENT = 'Argument', + LIST_NULLABILITY = 'ListNullabilityDesignator', + REQUIRED_DESIGNATOR = 'RequiredDesignator', + OPTIONAL_DESIGNATOR = 'OptionalDesignator', /** Fragments */ FRAGMENT_SPREAD = 'FragmentSpread', diff --git a/src/language/lexer.ts b/src/language/lexer.ts index a28c10843c..709ec1a449 100644 --- a/src/language/lexer.ts +++ b/src/language/lexer.ts @@ -91,6 +91,7 @@ export class Lexer { export function isPunctuatorTokenKind(kind: TokenKind): boolean { return ( kind === TokenKind.BANG || + kind === TokenKind.QUESTION_MARK || kind === TokenKind.DOLLAR || kind === TokenKind.AMP || kind === TokenKind.PAREN_L || @@ -281,6 +282,13 @@ function readNextToken(lexer: Lexer, start: number): Token { return createToken(lexer, TokenKind.PIPE, position, position + 1); case 0x007d: // } return createToken(lexer, TokenKind.BRACE_R, position, position + 1); + case 0x003f: // ? + return createToken( + lexer, + TokenKind.QUESTION_MARK, + position, + position + 1, + ); // StringValue case 0x0022: // " if ( diff --git a/src/language/parser.ts b/src/language/parser.ts index 1b0bc75f92..1b66712aa2 100644 --- a/src/language/parser.ts +++ b/src/language/parser.ts @@ -3,6 +3,8 @@ import type { Maybe } from '../jsutils/Maybe'; import type { GraphQLError } from '../error/GraphQLError'; import { syntaxError } from '../error/syntaxError'; +import { TokenKind } from './tokenKind'; + import type { Token, NameNode, @@ -59,10 +61,15 @@ import type { UnionTypeExtensionNode, EnumTypeExtensionNode, InputObjectTypeExtensionNode, + SupportArrayNode, + NullabilityModifierNode, + OptionalModifierNode, + RequiredModifierNode, } from './ast'; -import { Kind } from './kinds'; + import { Location, OperationTypeNode } from './ast'; -import { TokenKind } from './tokenKind'; + +import { Kind } from './kinds'; import { Source, isSource } from './source'; import { DirectiveLocation } from './directiveLocation'; import { Lexer, isPunctuatorTokenKind } from './lexer'; @@ -409,7 +416,7 @@ export class Parser { } /** - * Field : Alias? Name Arguments? Directives? SelectionSet? + * Field : Alias? Name Nullability? Arguments? Directives? SelectionSet? * * Alias : Name : */ @@ -419,6 +426,7 @@ export class Parser { const nameOrAlias = this.parseName(); let alias; let name; + if (this.expectOptionalToken(TokenKind.COLON)) { alias = nameOrAlias; name = this.parseName(); @@ -435,9 +443,77 @@ export class Parser { selectionSet: this.peek(TokenKind.BRACE_L) ? this.parseSelectionSet() : undefined, + required: this.parseRequiredStatus(), }); } + /** + * Nullability : + * - ! + * - ? + * + */ + parseRequiredStatus(): + | NullabilityModifierNode + | SupportArrayNode + | undefined { + const list = this.parseListNullability(); + const nullabilityStatus = this.parseNullabilityModifierNode(list); + + return nullabilityStatus ?? list; + } + + parseListNullability(): SupportArrayNode | undefined { + const start = this._lexer.token; + + if (this.expectOptionalToken(TokenKind.BRACKET_L)) { + const child = this.parseRequiredStatus(); + this.expectToken(TokenKind.BRACKET_R); + + return this.node(start, { + kind: Kind.LIST_NULLABILITY, + element: child, + }); + } + } + + /** + * Nullability : + * - ! + * - ? + * + */ + parseNullabilityModifierNode( + childElement?: SupportArrayNode, + ): NullabilityModifierNode | undefined { + return ( + this.parseRequiredModifierNode(childElement) ?? + this.parseOptionalModifierNode(childElement) + ); + } + + parseRequiredModifierNode( + childElement?: SupportArrayNode, + ): RequiredModifierNode | undefined { + if (this.expectOptionalToken(TokenKind.BANG)) { + return this.node(this._lexer.token, { + kind: Kind.REQUIRED_DESIGNATOR, + element: childElement, + }); + } + } + + parseOptionalModifierNode( + childElement?: SupportArrayNode, + ): OptionalModifierNode | undefined { + if (this.expectOptionalToken(TokenKind.QUESTION_MARK)) { + return this.node(this._lexer.token, { + kind: Kind.OPTIONAL_DESIGNATOR, + element: childElement, + }); + } + } + /** * Arguments[Const] : ( Argument[?Const]+ ) */ diff --git a/src/language/printer.ts b/src/language/printer.ts index b6df7d80f9..fff2caf7a5 100644 --- a/src/language/printer.ts +++ b/src/language/printer.ts @@ -55,8 +55,16 @@ const printDocASTReducer: ASTReducer = { SelectionSet: { leave: ({ selections }) => block(selections) }, Field: { - leave({ alias, name, arguments: args, directives, selectionSet }) { - const prefix = wrap('', alias, ': ') + name; + leave({ + alias, + name, + arguments: args, + directives, + selectionSet, + required, + }) { + const prefix = join([wrap('', alias, ': '), name, required], ''); + let argsLine = prefix + wrap('(', join(args, ', '), ')'); if (argsLine.length > MAX_LINE_LENGTH) { @@ -67,6 +75,24 @@ const printDocASTReducer: ASTReducer = { }, }, + RequiredDesignator: { + leave({ element }) { + return (element ?? '') + '!'; + }, + }, + + OptionalDesignator: { + leave({ element }) { + return (element ?? '') + '?'; + }, + }, + + ListNullabilityDesignator: { + leave({ element }) { + return '[' + (element ?? '') + ']'; + }, + }, + Argument: { leave: ({ name, value }) => name + ': ' + value }, // Fragments diff --git a/src/language/tokenKind.ts b/src/language/tokenKind.ts index 4878d697b0..b8d79af142 100644 --- a/src/language/tokenKind.ts +++ b/src/language/tokenKind.ts @@ -25,6 +25,7 @@ export enum TokenKind { STRING = 'String', BLOCK_STRING = 'BlockString', COMMENT = 'Comment', + QUESTION_MARK = '?', } /** diff --git a/src/utilities/__tests__/TypeInfo-test.ts b/src/utilities/__tests__/TypeInfo-test.ts index 6008e1d927..fd54bf7dc7 100644 --- a/src/utilities/__tests__/TypeInfo-test.ts +++ b/src/utilities/__tests__/TypeInfo-test.ts @@ -268,6 +268,7 @@ describe('visitWithTypeInfo', () => { { kind: 'Field', name: { kind: 'Name', value: '__typename' }, + required: undefined, }, ], }, diff --git a/src/utilities/applyRequiredStatus.ts b/src/utilities/applyRequiredStatus.ts new file mode 100644 index 0000000000..8952f81f50 --- /dev/null +++ b/src/utilities/applyRequiredStatus.ts @@ -0,0 +1,107 @@ +import type { GraphQLOutputType } from '../type/definition'; +import { + getNullableType, + GraphQLNonNull, + isNonNullType, + assertListType, + GraphQLList, + isListType, +} from '../type/definition'; +import type { + SupportArrayNode, + NullabilityModifierNode, +} from '../language/ast'; +import type { ASTReducer } from '../language/visitor'; +import { visit } from '../language/visitor'; +import { GraphQLError } from '../error/GraphQLError'; + +/** + * Implements the "Accounting For Client Controlled Nullability Designators" + * section of the spec. In particular, this function figures out the true return + * type of a field by taking into account both the nullability listed in the + * schema, and the nullability providing by an operation. + */ +export function modifiedOutputType( + type: GraphQLOutputType, + nullabilityNode?: SupportArrayNode | NullabilityModifierNode, +): GraphQLOutputType { + const typeStack: [GraphQLOutputType] = [type]; + + while (isListType(getNullableType(typeStack[typeStack.length - 1]))) { + const list = assertListType( + getNullableType(typeStack[typeStack.length - 1]), + ); + const elementType = list.ofType as GraphQLOutputType; + typeStack.push(elementType); + } + + const applyStatusReducer: ASTReducer = { + RequiredDesignator: { + leave({ element }) { + if (element) { + return new GraphQLNonNull(getNullableType(element)); + } + + // We're working with the inner-most type + const nextType = typeStack.pop(); + + // There's no way for nextType to be null if both type and nullabilityNode are valid + // eslint-disable-next-line @typescript-eslint/no-non-null-assertion + return new GraphQLNonNull(getNullableType(nextType!)); + }, + }, + OptionalDesignator: { + leave({ element }) { + if (element) { + return getNullableType(element); + } + + // We're working with the inner-most type + const nextType = typeStack.pop(); + + // There's no way for nextType to be null if both type and nullabilityNode are valid + // eslint-disable-next-line @typescript-eslint/no-non-null-assertion + return getNullableType(nextType!); + }, + }, + ListNullabilityDesignator: { + leave({ element }) { + let listType = typeStack.pop(); + // Skip to the inner-most list + if (!isListType(getNullableType(listType))) { + listType = typeStack.pop(); + } + + if (!listType) { + throw new GraphQLError( + 'List nullability modifier is too deep.', + nullabilityNode, + ); + } + const isRequired = isNonNullType(listType); + if (element) { + return isRequired + ? new GraphQLNonNull(new GraphQLList(element)) + : new GraphQLList(element); + } + + // We're working with the inner-most list + return listType; + }, + }, + }; + + if (nullabilityNode) { + const modified = visit(nullabilityNode, applyStatusReducer); + // modifiers must be exactly the same depth as the field type + if (typeStack.length > 0) { + throw new GraphQLError( + 'List nullability modifier is too shallow.', + nullabilityNode, + ); + } + return modified; + } + + return type; +} diff --git a/src/utilities/index.ts b/src/utilities/index.ts index a1411f508e..8c846686e0 100644 --- a/src/utilities/index.ts +++ b/src/utilities/index.ts @@ -106,3 +106,5 @@ export type { BreakingChange, DangerousChange } from './findBreakingChanges'; /** Wrapper type that contains DocumentNode and types that can be deduced from it. */ export type { TypedQueryDocumentNode } from './typedQueryDocumentNode'; + +export { modifiedOutputType } from './applyRequiredStatus'; diff --git a/src/validation/__tests__/OverlappingFieldsCanBeMergedRule-test.ts b/src/validation/__tests__/OverlappingFieldsCanBeMergedRule-test.ts index f9f4456b5e..15544842dd 100644 --- a/src/validation/__tests__/OverlappingFieldsCanBeMergedRule-test.ts +++ b/src/validation/__tests__/OverlappingFieldsCanBeMergedRule-test.ts @@ -167,6 +167,24 @@ describe('Validate: Overlapping fields can be merged', () => { ]); }); + it('different nullability', () => { + expectErrors(` + fragment conflictingArgs on Dog { + doesKnowCommand + doesKnowCommand! + } + `).toDeepEqual([ + { + message: + 'Fields "doesKnowCommand" conflict because they return conflicting types "Boolean" and "Boolean!". Use different aliases on the fields to fetch both if this was intentional.', + locations: [ + { line: 3, column: 9 }, + { line: 4, column: 9 }, + ], + }, + ]); + }); + it('different args, second missing an argument', () => { expectErrors(` fragment conflictingArgs on Dog { @@ -236,6 +254,94 @@ describe('Validate: Overlapping fields can be merged', () => { `); }); + describe('disallows different nullability status even if the types are mutually exclusive', () => { + it('for required and unset', () => { + expectErrors(` + fragment conflictingNullability on Pet { + ... on Dog { + name! + } + ... on Cat { + name + } + } + `).toDeepEqual([ + { + message: + 'Fields "name" conflict because they return conflicting types "String!" and "String". Use different aliases on the fields to fetch both if this was intentional.', + locations: [ + { line: 4, column: 13 }, + { line: 7, column: 13 }, + ], + }, + ]); + }); + + it('for required and optional', () => { + expectErrors(` + fragment conflictingNullability on Pet { + ... on Dog { + name! + } + ... on Cat { + name? + } + } + `).toDeepEqual([ + { + message: + 'Fields "name" conflict because they return conflicting types "String!" and "String". Use different aliases on the fields to fetch both if this was intentional.', + locations: [ + { line: 4, column: 13 }, + { line: 7, column: 13 }, + ], + }, + ]); + }); + }); + + describe('allows same nullability on mutually exclusive types', () => { + it('for optional and unset', () => { + // this should be valid because name is nullable by default + expectValid(` + fragment conflictingNullability on Pet { + ... on Dog { + name? + } + ... on Cat { + name + } + } + `); + }); + + it('for optional and optional', () => { + expectValid(` + fragment conflictingNullability on Pet { + ... on Dog { + name? + } + ... on Cat { + name? + } + } + `); + }); + + it('for unset and unset', () => { + expectValid(` + fragment conflictingNullability on Pet { + ... on Dog { + name + } + ... on Cat { + name + } + } + `); + }); + }); + it('encounters conflict in fragments', () => { expectErrors(` { @@ -576,9 +682,16 @@ describe('Validate: Overlapping fields can be merged', () => { name: String } + type Lists { + list: [Int] + requiredList: [Int!] + mixedThreeDList: [[[Int]!]!] + } + type Query { someBox: SomeBox connection: Connection + lists: Lists } `); @@ -613,6 +726,709 @@ describe('Validate: Overlapping fields can be merged', () => { ]); }); + describe('Nullable field on types which potentially overlap', () => { + it('matching required status', () => { + expectValidWithSchema( + schema, + ` + { + someBox { + ...on SomeBox { + unrelatedField! + } + ...on IntBox { + unrelatedField! + } + } + } + `, + ); + }); + + it('matching optional status', () => { + expectValidWithSchema( + schema, + ` + { + someBox { + ...on SomeBox { + unrelatedField? + } + ...on IntBox { + unrelatedField? + } + } + } + `, + ); + }); + + it('matching unset status', () => { + expectValidWithSchema( + schema, + ` + { + someBox { + ...on SomeBox { + unrelatedField + } + ...on IntBox { + unrelatedField + } + } + } + `, + ); + }); + + it('matching optional and unset (optional) status', () => { + expectValidWithSchema( + schema, + ` + { + someBox { + ...on SomeBox { + unrelatedField? + } + ...on IntBox { + unrelatedField + } + } + } + `, + ); + }); + + it('conflicting required and unset (optional) status', () => { + expectErrorsWithSchema( + schema, + ` + { + someBox { + ...on SomeBox { + unrelatedField + } + ...on IntBox { + unrelatedField! + } + } + } + `, + ).toDeepEqual([ + { + message: + 'Fields "unrelatedField" conflict because they return conflicting types "String" and "String!". Use different aliases on the fields to fetch both if this was intentional.', + locations: [ + { line: 5, column: 19 }, + { line: 8, column: 19 }, + ], + }, + ]); + }); + + it('conflicting required and optional status', () => { + expectErrorsWithSchema( + schema, + ` + { + someBox { + ...on SomeBox { + unrelatedField? + } + ...on IntBox { + unrelatedField! + } + } + } + `, + ).toDeepEqual([ + { + message: + 'Fields "unrelatedField" conflict because they return conflicting types "String" and "String!". Use different aliases on the fields to fetch both if this was intentional.', + locations: [ + { line: 5, column: 19 }, + { line: 8, column: 19 }, + ], + }, + ]); + }); + + it('conflicting required and unset (optional) status with aliases', () => { + expectValidWithSchema( + schema, + ` + { + someBox { + ...on SomeBox { + nullable: unrelatedField + } + ...on IntBox { + nonNullable: unrelatedField! + } + } + } + `, + ); + }); + + it('conflicting required and optional status with aliases', () => { + expectValidWithSchema( + schema, + ` + { + someBox { + ...on SomeBox { + nullable: unrelatedField! + } + ...on IntBox { + nonNullable: unrelatedField? + } + } + } + `, + ); + }); + + it('matching optional and unset (optional) status with aliases', () => { + expectValidWithSchema( + schema, + ` + { + someBox { + ...on SomeBox { + unset: unrelatedField + } + ...on IntBox { + nonNullable: unrelatedField? + } + } + } + `, + ); + }); + + it('matching required and required status with aliases', () => { + expectValidWithSchema( + schema, + ` + { + someBox { + ...on SomeBox { + otherNonNullable: unrelatedField! + } + ...on IntBox { + nonNullable: unrelatedField! + } + } + } + `, + ); + }); + + it('matching optional and optional status with aliases', () => { + expectValidWithSchema( + schema, + ` + { + someBox { + ...on SomeBox { + nullable: unrelatedField? + } + ...on IntBox { + otherNullable: unrelatedField? + } + } + } + `, + ); + }); + + it('matching unset and unset status with aliases', () => { + expectValidWithSchema( + schema, + ` + { + someBox { + ...on SomeBox { + unset: unrelatedField + } + ...on IntBox { + otherUnset: unrelatedField + } + } + } + `, + ); + }); + }); + + describe('Non-nullable field on types which potentially overlap', () => { + it('matching required status', () => { + expectValidWithSchema( + schema, + ` + { + someBox { + ...on NonNullStringBox1 { + scalar! + } + ...on NonNullStringBox1Impl { + scalar! + } + } + } + `, + ); + }); + + it('matching required and unset (required) nullability status', () => { + expectValidWithSchema( + schema, + ` + { + someBox { + ...on NonNullStringBox1 { + scalar! + } + ...on NonNullStringBox1Impl { + scalar + } + } + } + `, + ); + }); + + it('matching optional and optional nullability status', () => { + expectValidWithSchema( + schema, + ` + { + someBox { + ...on NonNullStringBox1 { + scalar? + } + ...on NonNullStringBox1Impl { + scalar? + } + } + } + `, + ); + }); + + it('matching unset and unset nullability status', () => { + expectValidWithSchema( + schema, + ` + { + someBox { + ...on NonNullStringBox1 { + scalar + } + ...on NonNullStringBox1Impl { + scalar + } + } + } + `, + ); + }); + + it('conflicting optional and required nullability status', () => { + expectErrorsWithSchema( + schema, + ` + { + someBox { + ...on NonNullStringBox1 { + scalar? + } + ...on NonNullStringBox1Impl { + scalar! + } + } + } + `, + ).toDeepEqual([ + { + message: + 'Fields "scalar" conflict because they return conflicting types "String" and "String!". Use different aliases on the fields to fetch both if this was intentional.', + locations: [ + { line: 5, column: 19 }, + { line: 8, column: 19 }, + ], + }, + ]); + }); + + it('conflicting optional and unset (required) nullability status', () => { + expectErrorsWithSchema( + schema, + ` + { + someBox { + ...on NonNullStringBox1 { + scalar? + } + ...on NonNullStringBox1Impl { + scalar + } + } + } + `, + ).toDeepEqual([ + { + message: + 'Fields "scalar" conflict because they return conflicting types "String" and "String!". Use different aliases on the fields to fetch both if this was intentional.', + locations: [ + { line: 5, column: 19 }, + { line: 8, column: 19 }, + ], + }, + ]); + }); + + it('conflicting required and optional nullability status with aliases', () => { + expectValidWithSchema( + schema, + ` + { + someBox { + ...on NonNullStringBox1 { + nonNullable: scalar! + } + ...on NonNullStringBox1Impl { + nullable: scalar? + } + } + } + `, + ); + }); + + it('matching optional and optional nullability status with aliases', () => { + expectValidWithSchema( + schema, + ` + { + someBox { + ...on NonNullStringBox1 { + alsoNullable: scalar? + } + ...on NonNullStringBox1Impl { + nullable: scalar? + } + } + } + `, + ); + }); + + it('matching required and required nullability status with aliases', () => { + expectValidWithSchema( + schema, + ` + { + someBox { + ...on NonNullStringBox1 { + nonNullable: scalar! + } + ...on NonNullStringBox1Impl { + alsoNonNullable: scalar! + } + } + } + `, + ); + }); + + it('conflicting required and unset (required) nullability status with aliases', () => { + expectValidWithSchema( + schema, + ` + { + someBox { + ...on NonNullStringBox1 { + nonNullable: scalar! + } + ...on NonNullStringBox1Impl { + nullable: scalar + } + } + } + `, + ); + }); + + it('matching optional and unset (required) nullability status with aliases', () => { + expectValidWithSchema( + schema, + ` + { + someBox { + ...on NonNullStringBox1 { + nullable: scalar? + } + ...on NonNullStringBox1Impl { + nonNullable: scalar + } + } + } + `, + ); + }); + + it('matching unset (required) and unset (required) nullability status with aliases', () => { + expectValidWithSchema( + schema, + ` + { + someBox { + ...on NonNullStringBox1 { + nonNullable: scalar + } + ...on NonNullStringBox1Impl { + alsoNonNullable: scalar + } + } + } + `, + ); + }); + }); + + describe('Exclusive types without aliases', () => { + it('conflicting optional and required nullability status', () => { + expectErrorsWithSchema( + schema, + ` + { + someBox { + ... on IntBox { + scalar! + } + ... on StringBox { + scalar? + } + } + } + `, + ).toDeepEqual([ + { + message: + 'Fields "scalar" conflict because they return conflicting types "Int!" and "String". Use different aliases on the fields to fetch both if this was intentional.', + locations: [ + { line: 5, column: 19 }, + { line: 8, column: 19 }, + ], + }, + ]); + }); + + it('matching optional and optional nullability status', () => { + expectErrorsWithSchema( + schema, + ` + { + someBox { + ... on IntBox { + scalar? + } + ... on StringBox { + scalar? + } + } + } + `, + ).toDeepEqual([ + { + message: + 'Fields "scalar" conflict because they return conflicting types "Int" and "String". Use different aliases on the fields to fetch both if this was intentional.', + locations: [ + { line: 5, column: 19 }, + { line: 8, column: 19 }, + ], + }, + ]); + }); + + it('matching required and required nullability status', () => { + expectErrorsWithSchema( + schema, + ` + { + someBox { + ... on IntBox { + scalar! + } + ... on StringBox { + scalar! + } + } + } + `, + ).toDeepEqual([ + { + message: + 'Fields "scalar" conflict because they return conflicting types "Int!" and "String!". Use different aliases on the fields to fetch both if this was intentional.', + locations: [ + { line: 5, column: 19 }, + { line: 8, column: 19 }, + ], + }, + ]); + }); + + it('conflicting unset (optional) and required nullability status', () => { + expectErrorsWithSchema( + schema, + ` + { + someBox { + ... on IntBox { + scalar + } + ... on StringBox { + scalar! + } + } + } + `, + ).toDeepEqual([ + { + message: + 'Fields "scalar" conflict because they return conflicting types "Int" and "String!". Use different aliases on the fields to fetch both if this was intentional.', + locations: [ + { line: 5, column: 19 }, + { line: 8, column: 19 }, + ], + }, + ]); + }); + + it('matching unset (optional) and optional nullability status', () => { + expectErrorsWithSchema( + schema, + ` + { + someBox { + ... on IntBox { + scalar + } + ... on StringBox { + scalar? + } + } + } + `, + ).toDeepEqual([ + { + message: + 'Fields "scalar" conflict because they return conflicting types "Int" and "String". Use different aliases on the fields to fetch both if this was intentional.', + locations: [ + { line: 5, column: 19 }, + { line: 8, column: 19 }, + ], + }, + ]); + }); + + it('matching unset (optional) and unset (optional) nullability status', () => { + expectErrorsWithSchema( + schema, + ` + { + someBox { + ... on IntBox { + scalar + } + ... on StringBox { + scalar + } + } + } + `, + ).toDeepEqual([ + { + message: + 'Fields "scalar" conflict because they return conflicting types "Int" and "String". Use different aliases on the fields to fetch both if this was intentional.', + locations: [ + { line: 5, column: 19 }, + { line: 8, column: 19 }, + ], + }, + ]); + }); + }); + + describe('Varying required statuses on lists', () => { + it('matching fields merge', () => { + expectValidWithSchema( + schema, + ` + { + lists { + list[] + list[] + } + } + `, + ); + }); + + it('conflicting fields do not merge', () => { + expectErrorsWithSchema( + schema, + ` + { + lists { + list[!]! + list[] + } + } + `, + ).toDeepEqual([ + { + message: + 'Fields "list" conflict because they return conflicting types "[Int!]!" and "[Int]". Use different aliases on the fields to fetch both if this was intentional.', + locations: [ + { column: 17, line: 4 }, + { column: 17, line: 5 }, + ], + }, + ]); + }); + + it('matching fields with different nullability indicators merge', () => { + expectValidWithSchema( + schema, + ` + { + lists { + list[]! + list! + } + } + `, + ); + }); + + it('matching matching required list fields merge', () => { + expectValidWithSchema( + schema, + ` + { + lists { + requiredList[] + requiredList[] + } + } + `, + ); + }); + }); + it('compatible return shapes on different return types', () => { // In this case `deepBox` returns `SomeBox` in the first usage, and // `StringBox` in the second usage. These return types are not the same! @@ -638,6 +1454,43 @@ describe('Validate: Overlapping fields can be merged', () => { ); }); + it('nested conflicting nullability status which potentially overlap', () => { + // This is invalid since an object could potentially be both the Object + // type IntBox and the interface type NonNullStringBox1. While that + // condition does not exist in the current schema, the schema could + // expand in the future to allow this. Thus it is invalid. + expectErrorsWithSchema( + schema, + ` + { + someBox { + ... on SomeBox { + deepBox { + unrelatedField + } + } + ... on StringBox { + deepBox { + unrelatedField! + } + } + } + } + `, + ).toDeepEqual([ + { + message: + 'Fields "deepBox" conflict because subfields "unrelatedField" conflict because they return conflicting types "String" and "String!". Use different aliases on the fields to fetch both if this was intentional.', + locations: [ + { line: 5, column: 17 }, + { line: 6, column: 19 }, + { line: 10, column: 17 }, + { line: 11, column: 19 }, + ], + }, + ]); + }); + it('disallows differing return types despite no overlap', () => { expectErrorsWithSchema( schema, @@ -753,6 +1606,57 @@ describe('Validate: Overlapping fields can be merged', () => { ]); }); + it('allows the nullability status based on ! operator', () => { + expectValidWithSchema( + schema, + ` + { + someBox { + ... on NonNullStringBox1 { + scalar + } + ... on StringBox { + scalar! + } + } + } + `, + ); + }); + + it('disallows conflicting nullability statuses based on ! operator', () => { + expectErrorsWithSchema( + schema, + ` + { + someBox { + ... on IntBox { + scalar + } + ... on StringBox { + scalar! + } + } + } + `, + ).toDeepEqual([ + { + locations: [ + { + column: 17, + line: 5, + }, + { + column: 17, + line: 8, + }, + ], + message: + 'Fields "scalar" conflict because they return conflicting types "Int" and "String!". Use different aliases on the fields to fetch both if this was intentional.', + }, + ]); + }); + it('disallows differing return type list despite no overlap', () => { expectErrorsWithSchema( schema, diff --git a/src/validation/__tests__/RequiredStatusOnFieldMatchesDefinitionRule-test.ts b/src/validation/__tests__/RequiredStatusOnFieldMatchesDefinitionRule-test.ts new file mode 100644 index 0000000000..8e43ea296a --- /dev/null +++ b/src/validation/__tests__/RequiredStatusOnFieldMatchesDefinitionRule-test.ts @@ -0,0 +1,83 @@ +import { describe, it } from 'mocha'; + +import { buildSchema } from '../../utilities/buildASTSchema'; + +import { RequiredStatusOnFieldMatchesDefinitionRule } from '../rules/RequiredStatusOnFieldMatchesDefinitionRule'; + +import { expectValidationErrorsWithSchema } from './harness'; + +function expectErrors(queryStr: string) { + return expectValidationErrorsWithSchema( + testSchema, + RequiredStatusOnFieldMatchesDefinitionRule, + queryStr, + ); +} + +function expectValid(queryStr: string) { + expectErrors(queryStr).toDeepEqual([]); +} + +const testSchema = buildSchema(` + type Lists { + nonList: Int + list: [Int] + requiredList: [Int]! + mixedThreeDList: [[[Int]]] + } + + type Query { + lists: Lists + } +`); + +describe('Validate: Field uses correct list depth', () => { + it('Fields are valid', () => { + expectValid(` + fragment listFragment on Lists { + list[!] + nonList! + nonList? + mixedThreeDList[[[!]!]!]! + requiredList[] + unmodifiedList: list + } + `); + }); + + it('reports errors when list depth is too high', () => { + expectErrors(` + fragment listFragment on Lists { + notAList: nonList[!] + list[[]] + } + `).toDeepEqual([ + { + message: 'List nullability modifier is too deep.', + locations: [{ line: 3, column: 26 }], + }, + { + message: 'List nullability modifier is too deep.', + locations: [{ line: 4, column: 13 }], + }, + ]); + }); + + it('reports errors when list depth is too low', () => { + expectErrors(` + fragment listFragment on Lists { + list! + mixedThreeDList[[]!]! + } + `).toDeepEqual([ + { + message: 'List nullability modifier is too shallow.', + locations: [{ line: 4, column: 9 }], + }, + { + message: 'List nullability modifier is too shallow.', + locations: [{ line: 5, column: 7 }], + }, + ]); + }); +}); diff --git a/src/validation/index.ts b/src/validation/index.ts index 584faf3440..f5aa69d708 100644 --- a/src/validation/index.ts +++ b/src/validation/index.ts @@ -84,6 +84,8 @@ export { VariablesAreInputTypesRule } from './rules/VariablesAreInputTypesRule'; /** Spec Section: "All Variable Usages Are Allowed" */ export { VariablesInAllowedPositionRule } from './rules/VariablesInAllowedPositionRule'; +export { RequiredStatusOnFieldMatchesDefinitionRule } from './rules/RequiredStatusOnFieldMatchesDefinitionRule'; + /** SDL-specific validation rules */ export { LoneSchemaDefinitionRule } from './rules/LoneSchemaDefinitionRule'; export { UniqueOperationTypesRule } from './rules/UniqueOperationTypesRule'; diff --git a/src/validation/rules/OverlappingFieldsCanBeMergedRule.ts b/src/validation/rules/OverlappingFieldsCanBeMergedRule.ts index e76b32f97c..32e8e06ef1 100644 --- a/src/validation/rules/OverlappingFieldsCanBeMergedRule.ts +++ b/src/validation/rules/OverlappingFieldsCanBeMergedRule.ts @@ -19,6 +19,7 @@ import type { GraphQLOutputType, GraphQLField, } from '../../type/definition'; + import { getNamedType, isNonNullType, @@ -33,6 +34,7 @@ import { typeFromAST } from '../../utilities/typeFromAST'; import type { Maybe } from '../../jsutils/Maybe'; import type { ValidationContext } from '../ValidationContext'; +import { modifiedOutputType } from '../../utilities/applyRequiredStatus'; function reasonMessage(reason: ConflictReasonMessage): string { if (Array.isArray(reason)) { @@ -590,17 +592,29 @@ function findConflict( const type1 = def1?.type; const type2 = def2?.type; - if (type1 && type2 && doTypesConflict(type1, type2)) { - return [ - [ - responseName, - `they return conflicting types "${inspect(type1)}" and "${inspect( - type2, - )}"`, - ], - [node1], - [node2], - ]; + if (type1 && type2) { + // Errors will have already been handled by RequiredStatusOnFieldMatchesDefinitionRule + // so there's no need to do anything here in the event that modifiedOutputType throws + // an error. + try { + const modifiedType1 = modifiedOutputType(type1, node1.required); + const modifiedType2 = modifiedOutputType(type2, node2.required); + + if (doTypesConflict(modifiedType1, modifiedType2)) { + return [ + [ + responseName, + `they return conflicting types "${inspect( + modifiedType1, + )}" and "${inspect(modifiedType2)}"`, + ], + [node1], + [node2], + ]; + } + } catch { + /* Do nothing. See above comment. */ + } } // Collect and compare sub-fields. Use the same "visited fragment names" list diff --git a/src/validation/rules/RequiredStatusOnFieldMatchesDefinitionRule.ts b/src/validation/rules/RequiredStatusOnFieldMatchesDefinitionRule.ts new file mode 100644 index 0000000000..896ba83528 --- /dev/null +++ b/src/validation/rules/RequiredStatusOnFieldMatchesDefinitionRule.ts @@ -0,0 +1,95 @@ +import type { + FieldNode, + NullabilityModifierNode, + SupportArrayNode, +} from '../../language/ast'; + +import type { ASTReducer, ASTVisitor } from '../../language/visitor'; +import { visit } from '../../language/visitor'; + +import type { ValidationContext } from '../ValidationContext'; +import type { GraphQLOutputType } from '../../type/definition'; +import { + assertListType, + getNullableType, + isListType, +} from '../../type/definition'; + +import { GraphQLError } from '../../error/GraphQLError'; + +/** + * List element nullability designators need to use a depth that is the same as or less than the + * type of the field it's applied to. + * + * Otherwise the GraphQL document is invalid. + * + * See https://spec.graphql.org/draft/#sec-Field-Selections + */ +export function RequiredStatusOnFieldMatchesDefinitionRule( + context: ValidationContext, +): ASTVisitor { + return { + Field(node: FieldNode) { + const fieldDef = context.getFieldDef(); + const requiredNode = node.required; + if (fieldDef && requiredNode) { + const typeDepth = getTypeDepth(fieldDef.type); + const designatorDepth = getDesignatorDepth(requiredNode); + + if (typeDepth > designatorDepth) { + context.reportError( + new GraphQLError( + 'List nullability modifier is too shallow.', + node.required, + ), + ); + } else if (typeDepth < designatorDepth) { + context.reportError( + new GraphQLError( + 'List nullability modifier is too deep.', + node.required, + ), + ); + } + } + }, + }; + + function getTypeDepth(type: GraphQLOutputType): number { + let currentType = type; + let depthCount = 0; + while (isListType(getNullableType(currentType))) { + const list = assertListType(getNullableType(currentType)); + const elementType = list.ofType as GraphQLOutputType; + currentType = elementType; + depthCount += 1; + } + return depthCount; + } + + function getDesignatorDepth( + designator: SupportArrayNode | NullabilityModifierNode, + ): number { + const getDepth: ASTReducer = { + RequiredDesignator: { + leave({ element }) { + return element ?? 0; + }, + }, + + OptionalDesignator: { + leave({ element }) { + return element ?? 0; + }, + }, + + ListNullabilityDesignator: { + leave({ element }) { + return (element ?? 0) + 1; + }, + }, + }; + + return visit(designator, getDepth); + } +} diff --git a/src/validation/specifiedRules.ts b/src/validation/specifiedRules.ts index 930e9f0f58..5a11d0dd24 100644 --- a/src/validation/specifiedRules.ts +++ b/src/validation/specifiedRules.ts @@ -84,6 +84,8 @@ import { OverlappingFieldsCanBeMergedRule } from './rules/OverlappingFieldsCanBe // Spec Section: "Input Object Field Uniqueness" import { UniqueInputFieldNamesRule } from './rules/UniqueInputFieldNamesRule'; +import { RequiredStatusOnFieldMatchesDefinitionRule } from './rules/RequiredStatusOnFieldMatchesDefinitionRule'; + // SDL-specific validation rules import { LoneSchemaDefinitionRule } from './rules/LoneSchemaDefinitionRule'; import { UniqueOperationTypesRule } from './rules/UniqueOperationTypesRule'; @@ -125,6 +127,7 @@ export const specifiedRules: ReadonlyArray = Object.freeze([ ValuesOfCorrectTypeRule, ProvidedRequiredArgumentsRule, VariablesInAllowedPositionRule, + RequiredStatusOnFieldMatchesDefinitionRule, OverlappingFieldsCanBeMergedRule, UniqueInputFieldNamesRule, ]);