diff --git a/src/validation/__tests__/OverlappingFieldsCanBeMergedRule-test.ts b/src/validation/__tests__/OverlappingFieldsCanBeMergedRule-test.ts index 52c2deb1a0..932840d5b9 100644 --- a/src/validation/__tests__/OverlappingFieldsCanBeMergedRule-test.ts +++ b/src/validation/__tests__/OverlappingFieldsCanBeMergedRule-test.ts @@ -1049,6 +1049,137 @@ describe('Validate: Overlapping fields can be merged', () => { ); }); + it('allows non-conflicting overlapping required statuses', () => { + expectValidationErrorsWithSchema( + schema, + OverlappingFieldsCanBeMergedRule, + ` + { + someBox { + ... on IntBox { + scalar: unrelatedField! + } + ... on StringBox { + scalar! + } + } + } + `, + { + experimentalClientControlledNullability: true, + }, + ); + + expectValidationErrorsWithSchema( + schema, + OverlappingFieldsCanBeMergedRule, + ` + { + someBox { + ... on IntBox { + scalar: unrelatedField? + } + ... on StringBox { + scalar? + } + } + } + `, + { + experimentalClientControlledNullability: true, + }, + ); + }); + + it('disallows conflicting overlapping required statuses', () => { + expectValidationErrorsWithSchema( + schema, + OverlappingFieldsCanBeMergedRule, + ` + { + someBox { + ... on IntBox { + scalar: unrelatedField! + } + ... on StringBox { + scalar + } + } + } + `, + { + experimentalClientControlledNullability: true, + }, + ).toDeepEqual([ + { + message: + 'Fields "scalar" conflict because they have conflicting nullability designators "!" and "". Use different aliases on the fields to fetch both if this was intentional.', + locations: [ + { line: 5, column: 17 }, + { line: 8, column: 17 }, + ], + }, + ]); + + expectValidationErrorsWithSchema( + schema, + OverlappingFieldsCanBeMergedRule, + ` + { + someBox { + ... on IntBox { + scalar: unrelatedField! + } + ... on StringBox { + scalar? + } + } + } + `, + { + experimentalClientControlledNullability: true, + }, + ).toDeepEqual([ + { + message: + 'Fields "scalar" conflict because they have conflicting nullability designators "!" and "?". Use different aliases on the fields to fetch both if this was intentional.', + locations: [ + { line: 5, column: 17 }, + { line: 8, column: 17 }, + ], + }, + ]); + + expectValidationErrorsWithSchema( + schema, + OverlappingFieldsCanBeMergedRule, + ` + { + someBox { + ... on IntBox { + scalar: unrelatedField + } + ... on StringBox { + scalar? + } + } + } + `, + { + experimentalClientControlledNullability: true, + }, + ).toDeepEqual([ + { + message: + 'Fields "scalar" conflict because they have conflicting nullability designators "" and "?". Use different aliases on the fields to fetch both if this was intentional.', + locations: [ + { line: 5, column: 17 }, + { line: 8, column: 17 }, + ], + }, + ]); + }); + it('same wrapped scalar return types', () => { expectValidWithSchema( schema, diff --git a/src/validation/__tests__/RequiredStatusOnFieldMatchesDefinitionRule-test.ts b/src/validation/__tests__/RequiredStatusOnFieldMatchesDefinitionRule-test.ts new file mode 100644 index 0000000000..53f9f4b6ec --- /dev/null +++ b/src/validation/__tests__/RequiredStatusOnFieldMatchesDefinitionRule-test.ts @@ -0,0 +1,83 @@ +import { describe, it } from 'mocha'; + +import { buildSchema } from '../../utilities/buildASTSchema.js'; + +import { RequiredStatusOnFieldMatchesDefinitionRule } from '../rules/RequiredStatusOnFieldMatchesDefinitionRule.js'; + +import { expectValidationErrorsWithSchema } from './harness.js'; + +function expectErrors(queryStr: string) { + return expectValidationErrorsWithSchema( + testSchema, + RequiredStatusOnFieldMatchesDefinitionRule, + queryStr, + { experimentalClientControlledNullability: true }, + ); +} + +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: 3, column: 13 }], + }, + { + message: 'List nullability modifier is too shallow.', + locations: [{ line: 4, column: 24 }], + }, + ]); + }); +}); diff --git a/src/validation/__tests__/harness.ts b/src/validation/__tests__/harness.ts index 682932d897..076c1b39c5 100644 --- a/src/validation/__tests__/harness.ts +++ b/src/validation/__tests__/harness.ts @@ -2,6 +2,7 @@ import { expectJSON } from '../../__testUtils__/expectJSON.js'; import type { Maybe } from '../../jsutils/Maybe.js'; +import type { ParseOptions } from '../../language/parser.js'; import { parse } from '../../language/parser.js'; import type { GraphQLSchema } from '../../type/schema.js'; @@ -122,8 +123,9 @@ export function expectValidationErrorsWithSchema( schema: GraphQLSchema, rule: ValidationRule, queryStr: string, + parseOptions?: ParseOptions, ): any { - const doc = parse(queryStr); + const doc = parse(queryStr, parseOptions); const errors = validate(schema, doc, [rule]); return expectJSON(errors); } @@ -131,8 +133,14 @@ export function expectValidationErrorsWithSchema( export function expectValidationErrors( rule: ValidationRule, queryStr: string, + parseOptions?: ParseOptions, ): any { - return expectValidationErrorsWithSchema(testSchema, rule, queryStr); + return expectValidationErrorsWithSchema( + testSchema, + rule, + queryStr, + parseOptions, + ); } export function expectSDLValidationErrors( diff --git a/src/validation/rules/OverlappingFieldsCanBeMergedRule.ts b/src/validation/rules/OverlappingFieldsCanBeMergedRule.ts index e2444047c6..d0ecad7a59 100644 --- a/src/validation/rules/OverlappingFieldsCanBeMergedRule.ts +++ b/src/validation/rules/OverlappingFieldsCanBeMergedRule.ts @@ -617,17 +617,40 @@ 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) { + // Two fields have different types + if (doTypesConflict(type1, type2)) { + return [ + [ + responseName, + `they return conflicting types "${inspect(type1)}" and "${inspect( + type2, + )}"`, + ], + [node1], + [node2], + ]; + } + + // Two fields have different required operators + if (node1.nullabilityAssertion !== node2.nullabilityAssertion) { + return [ + [ + responseName, + `they have conflicting nullability designators "${ + node1.nullabilityAssertion === undefined + ? '' + : print(node1.nullabilityAssertion) + }" and "${ + node2.nullabilityAssertion === undefined + ? '' + : print(node2.nullabilityAssertion) + }"`, + ], + [node1], + [node2], + ]; + } } // 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..549383e939 --- /dev/null +++ b/src/validation/rules/RequiredStatusOnFieldMatchesDefinitionRule.ts @@ -0,0 +1,93 @@ +import { GraphQLError } from '../../error/GraphQLError.js'; + +import type { + FieldNode, + ListNullabilityOperatorNode, + NullabilityAssertionNode, +} from '../../language/ast.js'; +import type { ASTReducer, ASTVisitor } from '../../language/visitor.js'; +import { visit } from '../../language/visitor.js'; + +import type { GraphQLOutputType } from '../../type/definition.js'; +import { + assertListType, + getNullableType, + isListType, +} from '../../type/definition.js'; + +import type { ValidationContext } from '../ValidationContext.js'; + +/** + * 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.nullabilityAssertion; + 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.', { + nodes: node.nullabilityAssertion, + }), + ); + } else if (typeDepth < designatorDepth) { + context.reportError( + new GraphQLError('List nullability modifier is too deep.', { + nodes: node.nullabilityAssertion, + }), + ); + } + } + }, + }; + + 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: ListNullabilityOperatorNode | NullabilityAssertionNode, + ): number { + const getDepth: ASTReducer = { + NonNullAssertion: { + leave({ nullabilityAssertion }) { + return nullabilityAssertion ?? 0; + }, + }, + + ErrorBoundary: { + leave({ nullabilityAssertion }) { + return nullabilityAssertion ?? 0; + }, + }, + + ListNullabilityOperator: { + leave({ nullabilityAssertion }) { + return (nullabilityAssertion ?? 0) + 1; + }, + }, + }; + + return visit(designator, getDepth); + } +}