From 03a68cc99c73847e0b956b713fd1ccc75e35e1cf Mon Sep 17 00:00:00 2001 From: twof Date: Sat, 2 Jul 2022 11:33:27 -0700 Subject: [PATCH 1/8] validation code moved over and updated to meet new naming conventions. test coverage not passing. --- src/validation/__tests__/harness.ts | 14 ++++++-- .../rules/OverlappingFieldsCanBeMergedRule.ts | 32 +++++++++++-------- 2 files changed, 30 insertions(+), 16 deletions(-) diff --git a/src/validation/__tests__/harness.ts b/src/validation/__tests__/harness.ts index 682932d897..4423c48312 100644 --- a/src/validation/__tests__/harness.ts +++ b/src/validation/__tests__/harness.ts @@ -2,7 +2,8 @@ import { expectJSON } from '../../__testUtils__/expectJSON.js'; import type { Maybe } from '../../jsutils/Maybe.js'; -import { parse } from '../../language/parser.js'; +import type { ParseOptions } from '../../language/parser'; +import { parse } from '../../language/parser'; 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..6fd8d16d76 100644 --- a/src/validation/rules/OverlappingFieldsCanBeMergedRule.ts +++ b/src/validation/rules/OverlappingFieldsCanBeMergedRule.ts @@ -29,8 +29,9 @@ import { isObjectType, } from '../../type/definition.js'; -import { sortValueNode } from '../../utilities/sortValueNode.js'; -import { typeFromAST } from '../../utilities/typeFromAST.js'; +import { applyRequiredStatus } from '../../utilities/applyRequiredStatus'; +import { sortValueNode } from '../../utilities/sortValueNode'; +import { typeFromAST } from '../../utilities/typeFromAST'; import type { ValidationContext } from '../ValidationContext.js'; @@ -617,17 +618,22 @@ 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) { + const modifiedType1 = applyRequiredStatus(type1, node1.nullabilityAssertion); + const modifiedType2 = applyRequiredStatus(type2, node2.nullabilityAssertion); + + if (doTypesConflict(modifiedType1, modifiedType2)) { + return [ + [ + responseName, + `they return conflicting types "${inspect( + modifiedType1, + )}" and "${inspect(modifiedType2)}"`, + ], + [node1], + [node2], + ]; + } } // Collect and compare sub-fields. Use the same "visited fragment names" list From 2b5048d063d228c864f6b6b1fd773115caf8fa96 Mon Sep 17 00:00:00 2001 From: twof Date: Sat, 2 Jul 2022 13:42:06 -0700 Subject: [PATCH 2/8] new files --- .../__tests__/applyRequiredStatus-test.ts | 79 +++++++++++ src/utilities/applyRequiredStatus.ts | 123 ++++++++++++++++++ ...StatusOnFieldMatchesDefinitionRule-test.ts | 83 ++++++++++++ ...uiredStatusOnFieldMatchesDefinitionRule.ts | 93 +++++++++++++ 4 files changed, 378 insertions(+) create mode 100644 src/utilities/__tests__/applyRequiredStatus-test.ts create mode 100644 src/utilities/applyRequiredStatus.ts create mode 100644 src/validation/__tests__/RequiredStatusOnFieldMatchesDefinitionRule-test.ts create mode 100644 src/validation/rules/RequiredStatusOnFieldMatchesDefinitionRule.ts diff --git a/src/utilities/__tests__/applyRequiredStatus-test.ts b/src/utilities/__tests__/applyRequiredStatus-test.ts new file mode 100644 index 0000000000..64e2ab4e52 --- /dev/null +++ b/src/utilities/__tests__/applyRequiredStatus-test.ts @@ -0,0 +1,79 @@ +import { expect } from 'chai'; +import { describe, it } from 'mocha'; + +import { Parser, parseType } from '../../language/parser'; +import { TokenKind } from '../../language/tokenKind'; + +import { assertOutputType } from '../../type/definition'; +import { GraphQLInt } from '../../type/scalars'; +import { GraphQLSchema } from '../../type/schema'; + +import { applyRequiredStatus } from '../applyRequiredStatus'; +import { typeFromAST } from '../typeFromAST'; + +function applyRequiredStatusTest( + typeStr: string, + nullabilityStr: string, +): string { + const schema = new GraphQLSchema({ types: [GraphQLInt] }); + const type = assertOutputType(typeFromAST(schema, parseType(typeStr))); + + const parser = new Parser(nullabilityStr, { + experimentalClientControlledNullability: true, + }); + + parser.expectToken(TokenKind.SOF); + const nullabilityNode = parser.parseNullabilityModifier(); + parser.expectToken(TokenKind.EOF); + + const outputType = applyRequiredStatus(type, nullabilityNode); + return outputType.toString(); +} + +describe('applyRequiredStatus', () => { + it('applyRequiredStatus smoke test', () => { + expect(applyRequiredStatusTest('Int', '')).to.equal('Int'); + }); + + it('applyRequiredStatus produces correct output types with no overrides', () => { + expect(applyRequiredStatusTest('[[[Int!]]!]!', '[[[]]]')).to.equal( + '[[[Int!]]!]!', + ); + }); + + it('applyRequiredStatus produces correct output types with required overrides', () => { + expect(applyRequiredStatusTest('[[[Int!]]!]!', '[[[!]!]!]!')).to.equal( + '[[[Int!]!]!]!', + ); + }); + + it('applyRequiredStatus produces correct output types with optional overrides', () => { + expect(applyRequiredStatusTest('[[[Int!]]!]!', '[[[?]?]?]?')).to.equal( + '[[[Int]]]', + ); + }); + + it('applyRequiredStatus throws error when modifier is too deep', () => { + expect(() => { + applyRequiredStatusTest('[[[Int!]]!]!', '[[[[]]]]'); + }).to.throw('List nullability modifier is too deep.'); + }); + + it('applyRequiredStatus throws error when modifier is too shallow', () => { + expect(() => { + applyRequiredStatusTest('[[[Int!]]!]!', '[[]]'); + }).to.throw('List nullability modifier is too shallow.'); + }); + + it('applyRequiredStatus with required designator functions when list syntax is excluded', () => { + expect(applyRequiredStatusTest('[[[Int!]]!]', '!')).to.equal( + '[[[Int!]]!]!', + ); + }); + + it('applyRequiredStatus with optional designator functions when list syntax is excluded', () => { + expect(applyRequiredStatusTest('[[[Int!]]!]!', '?')).to.equal( + '[[[Int!]]!]', + ); + }); +}); \ No newline at end of file diff --git a/src/utilities/applyRequiredStatus.ts b/src/utilities/applyRequiredStatus.ts new file mode 100644 index 0000000000..e8456782e6 --- /dev/null +++ b/src/utilities/applyRequiredStatus.ts @@ -0,0 +1,123 @@ +import { GraphQLError } from '../error/GraphQLError'; + +import type { NullabilityAssertionNode } from '../language/ast'; +import { Kind } from '../language/kinds'; +import type { ASTReducer } from '../language/visitor'; +import { visit } from '../language/visitor'; + +import type { GraphQLOutputType } from '../type/definition'; +import { + assertListType, + getNullableType, + GraphQLList, + GraphQLNonNull, + isListType, + isNonNullType, +} from '../type/definition'; + +/** + * 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. + * + * @internal + */ +export function applyRequiredStatus( + type: GraphQLOutputType, + nullabilityNode: NullabilityAssertionNode | undefined, +): GraphQLOutputType { + // If the field is marked with 0 or 1 nullability designator + // short-circuit + if (nullabilityNode === undefined) { + return type; + } else if (nullabilityNode?.nullabilityAssertion === undefined) { + if (nullabilityNode?.kind === Kind.NON_NULL_ASSERTION) { + return new GraphQLNonNull(getNullableType(type)); + } else if (nullabilityNode?.kind === Kind.ERROR_BOUNDARY) { + return getNullableType(type); + } + } + + const typeStack: [GraphQLOutputType] = [type]; + + // Load the nullable version each type in the type definition to typeStack + while (isListType(getNullableType(typeStack[typeStack.length - 1]))) { + const list = assertListType( + getNullableType(typeStack[typeStack.length - 1]), + ); + const elementType = list.ofType as GraphQLOutputType; + typeStack.push(elementType); + } + + // Re-apply nullability to each level of the list from the outside in + const applyStatusReducer: ASTReducer = { + NonNullAssertion: { + leave({ nullabilityAssertion }) { + if (nullabilityAssertion) { + return new GraphQLNonNull(getNullableType(nullabilityAssertion)); + } + + // 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!)); + }, + }, + ErrorBoundary: { + leave({ nullabilityAssertion }) { + if (nullabilityAssertion) { + return getNullableType(nullabilityAssertion); + } + + // 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!); + }, + }, + ListNullabilityOperator: { + leave({ nullabilityAssertion }) { + 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.', + { + nodes: nullabilityNode + }, + ); + } + const isRequired = isNonNullType(listType); + if (nullabilityAssertion) { + return isRequired + ? new GraphQLNonNull(new GraphQLList(nullabilityAssertion)) + : new GraphQLList(nullabilityAssertion); + } + + // We're working with the inner-most list + return listType; + }, + }, + }; + + 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.', + { + nodes: nullabilityNode + }, + ); + } + return modified; +} diff --git a/src/validation/__tests__/RequiredStatusOnFieldMatchesDefinitionRule-test.ts b/src/validation/__tests__/RequiredStatusOnFieldMatchesDefinitionRule-test.ts new file mode 100644 index 0000000000..96718ac4cb --- /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, + { 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/rules/RequiredStatusOnFieldMatchesDefinitionRule.ts b/src/validation/rules/RequiredStatusOnFieldMatchesDefinitionRule.ts new file mode 100644 index 0000000000..6925d7d2d3 --- /dev/null +++ b/src/validation/rules/RequiredStatusOnFieldMatchesDefinitionRule.ts @@ -0,0 +1,93 @@ +import { GraphQLError } from '../../error/GraphQLError'; + +import type { + FieldNode, + ListNullabilityOperatorNode, + NullabilityAssertionNode, +} from '../../language/ast'; +import type { ASTReducer, ASTVisitor } from '../../language/visitor'; +import { visit } from '../../language/visitor'; + +import type { GraphQLOutputType } from '../../type/definition'; +import { + assertListType, + getNullableType, + isListType, +} from '../../type/definition'; + +import type { ValidationContext } from '../ValidationContext'; + +/** + * 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); + } +} From 7036ee7e682cfef2b2fb8d6e6bf3152565a51484 Mon Sep 17 00:00:00 2001 From: twof Date: Sat, 2 Jul 2022 13:55:01 -0700 Subject: [PATCH 3/8] removed reliance on applyRequiredStatus --- src/utilities/applyRequiredStatus.ts | 123 ------------------ .../rules/OverlappingFieldsCanBeMergedRule.ts | 31 ++++- 2 files changed, 24 insertions(+), 130 deletions(-) delete mode 100644 src/utilities/applyRequiredStatus.ts diff --git a/src/utilities/applyRequiredStatus.ts b/src/utilities/applyRequiredStatus.ts deleted file mode 100644 index e8456782e6..0000000000 --- a/src/utilities/applyRequiredStatus.ts +++ /dev/null @@ -1,123 +0,0 @@ -import { GraphQLError } from '../error/GraphQLError'; - -import type { NullabilityAssertionNode } from '../language/ast'; -import { Kind } from '../language/kinds'; -import type { ASTReducer } from '../language/visitor'; -import { visit } from '../language/visitor'; - -import type { GraphQLOutputType } from '../type/definition'; -import { - assertListType, - getNullableType, - GraphQLList, - GraphQLNonNull, - isListType, - isNonNullType, -} from '../type/definition'; - -/** - * 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. - * - * @internal - */ -export function applyRequiredStatus( - type: GraphQLOutputType, - nullabilityNode: NullabilityAssertionNode | undefined, -): GraphQLOutputType { - // If the field is marked with 0 or 1 nullability designator - // short-circuit - if (nullabilityNode === undefined) { - return type; - } else if (nullabilityNode?.nullabilityAssertion === undefined) { - if (nullabilityNode?.kind === Kind.NON_NULL_ASSERTION) { - return new GraphQLNonNull(getNullableType(type)); - } else if (nullabilityNode?.kind === Kind.ERROR_BOUNDARY) { - return getNullableType(type); - } - } - - const typeStack: [GraphQLOutputType] = [type]; - - // Load the nullable version each type in the type definition to typeStack - while (isListType(getNullableType(typeStack[typeStack.length - 1]))) { - const list = assertListType( - getNullableType(typeStack[typeStack.length - 1]), - ); - const elementType = list.ofType as GraphQLOutputType; - typeStack.push(elementType); - } - - // Re-apply nullability to each level of the list from the outside in - const applyStatusReducer: ASTReducer = { - NonNullAssertion: { - leave({ nullabilityAssertion }) { - if (nullabilityAssertion) { - return new GraphQLNonNull(getNullableType(nullabilityAssertion)); - } - - // 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!)); - }, - }, - ErrorBoundary: { - leave({ nullabilityAssertion }) { - if (nullabilityAssertion) { - return getNullableType(nullabilityAssertion); - } - - // 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!); - }, - }, - ListNullabilityOperator: { - leave({ nullabilityAssertion }) { - 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.', - { - nodes: nullabilityNode - }, - ); - } - const isRequired = isNonNullType(listType); - if (nullabilityAssertion) { - return isRequired - ? new GraphQLNonNull(new GraphQLList(nullabilityAssertion)) - : new GraphQLList(nullabilityAssertion); - } - - // We're working with the inner-most list - return listType; - }, - }, - }; - - 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.', - { - nodes: nullabilityNode - }, - ); - } - return modified; -} diff --git a/src/validation/rules/OverlappingFieldsCanBeMergedRule.ts b/src/validation/rules/OverlappingFieldsCanBeMergedRule.ts index 6fd8d16d76..73c6243523 100644 --- a/src/validation/rules/OverlappingFieldsCanBeMergedRule.ts +++ b/src/validation/rules/OverlappingFieldsCanBeMergedRule.ts @@ -29,7 +29,6 @@ import { isObjectType, } from '../../type/definition.js'; -import { applyRequiredStatus } from '../../utilities/applyRequiredStatus'; import { sortValueNode } from '../../utilities/sortValueNode'; import { typeFromAST } from '../../utilities/typeFromAST'; @@ -619,16 +618,34 @@ function findConflict( const type2 = def2?.type; if (type1 && type2) { - const modifiedType1 = applyRequiredStatus(type1, node1.nullabilityAssertion); - const modifiedType2 = applyRequiredStatus(type2, node2.nullabilityAssertion); + // Two fields have different types + if (doTypesConflict(type1, type2)) { + return [ + [ + responseName, + `they return conflicting types "${inspect(type1)}" and "${inspect( + type2, + )}"`, + ], + [node1], + [node2], + ]; + } - if (doTypesConflict(modifiedType1, modifiedType2)) { + // Two fields have different required operators + if (node1.nullabilityAssertion !== node2.nullabilityAssertion) { return [ [ responseName, - `they return conflicting types "${inspect( - modifiedType1, - )}" and "${inspect(modifiedType2)}"`, + `they have conflicting nullability designators "${ + node1.nullabilityAssertion === undefined + ? '' + : print(node1.nullabilityAssertion) + }" and "${ + node2.nullabilityAssertion === undefined + ? '' + : print(node2.nullabilityAssertion) + }"`, ], [node1], [node2], From e19360b84ca0bd037ce4ddb911b5f7a165cea3f3 Mon Sep 17 00:00:00 2001 From: twof Date: Sat, 2 Jul 2022 13:59:59 -0700 Subject: [PATCH 4/8] npm test passing --- .../OverlappingFieldsCanBeMergedRule-test.ts | 133 ++++++++++++++++++ 1 file changed, 133 insertions(+) diff --git a/src/validation/__tests__/OverlappingFieldsCanBeMergedRule-test.ts b/src/validation/__tests__/OverlappingFieldsCanBeMergedRule-test.ts index 52c2deb1a0..0fc8079d4b 100644 --- a/src/validation/__tests__/OverlappingFieldsCanBeMergedRule-test.ts +++ b/src/validation/__tests__/OverlappingFieldsCanBeMergedRule-test.ts @@ -1049,6 +1049,139 @@ 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, + }, + ); + // TODO: need one with list designators + }); + + 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 }, + ], + }, + ]); + // TODO: need one with list designators + }); + it('same wrapped scalar return types', () => { expectValidWithSchema( schema, From 30a07d73bb583e43c3f2f52d1e5e745ed40db2ac Mon Sep 17 00:00:00 2001 From: twof Date: Sat, 2 Jul 2022 14:06:41 -0700 Subject: [PATCH 5/8] cleanup --- .../__tests__/OverlappingFieldsCanBeMergedRule-test.ts | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/validation/__tests__/OverlappingFieldsCanBeMergedRule-test.ts b/src/validation/__tests__/OverlappingFieldsCanBeMergedRule-test.ts index 0fc8079d4b..932840d5b9 100644 --- a/src/validation/__tests__/OverlappingFieldsCanBeMergedRule-test.ts +++ b/src/validation/__tests__/OverlappingFieldsCanBeMergedRule-test.ts @@ -1089,7 +1089,6 @@ describe('Validate: Overlapping fields can be merged', () => { experimentalClientControlledNullability: true, }, ); - // TODO: need one with list designators }); it('disallows conflicting overlapping required statuses', () => { @@ -1179,7 +1178,6 @@ describe('Validate: Overlapping fields can be merged', () => { ], }, ]); - // TODO: need one with list designators }); it('same wrapped scalar return types', () => { From 749586a8d2a281d5d236b39733e7149401961df5 Mon Sep 17 00:00:00 2001 From: twof Date: Mon, 4 Jul 2022 11:50:09 -0600 Subject: [PATCH 6/8] remove dead test --- .../__tests__/applyRequiredStatus-test.ts | 79 ------------------- 1 file changed, 79 deletions(-) delete mode 100644 src/utilities/__tests__/applyRequiredStatus-test.ts diff --git a/src/utilities/__tests__/applyRequiredStatus-test.ts b/src/utilities/__tests__/applyRequiredStatus-test.ts deleted file mode 100644 index 64e2ab4e52..0000000000 --- a/src/utilities/__tests__/applyRequiredStatus-test.ts +++ /dev/null @@ -1,79 +0,0 @@ -import { expect } from 'chai'; -import { describe, it } from 'mocha'; - -import { Parser, parseType } from '../../language/parser'; -import { TokenKind } from '../../language/tokenKind'; - -import { assertOutputType } from '../../type/definition'; -import { GraphQLInt } from '../../type/scalars'; -import { GraphQLSchema } from '../../type/schema'; - -import { applyRequiredStatus } from '../applyRequiredStatus'; -import { typeFromAST } from '../typeFromAST'; - -function applyRequiredStatusTest( - typeStr: string, - nullabilityStr: string, -): string { - const schema = new GraphQLSchema({ types: [GraphQLInt] }); - const type = assertOutputType(typeFromAST(schema, parseType(typeStr))); - - const parser = new Parser(nullabilityStr, { - experimentalClientControlledNullability: true, - }); - - parser.expectToken(TokenKind.SOF); - const nullabilityNode = parser.parseNullabilityModifier(); - parser.expectToken(TokenKind.EOF); - - const outputType = applyRequiredStatus(type, nullabilityNode); - return outputType.toString(); -} - -describe('applyRequiredStatus', () => { - it('applyRequiredStatus smoke test', () => { - expect(applyRequiredStatusTest('Int', '')).to.equal('Int'); - }); - - it('applyRequiredStatus produces correct output types with no overrides', () => { - expect(applyRequiredStatusTest('[[[Int!]]!]!', '[[[]]]')).to.equal( - '[[[Int!]]!]!', - ); - }); - - it('applyRequiredStatus produces correct output types with required overrides', () => { - expect(applyRequiredStatusTest('[[[Int!]]!]!', '[[[!]!]!]!')).to.equal( - '[[[Int!]!]!]!', - ); - }); - - it('applyRequiredStatus produces correct output types with optional overrides', () => { - expect(applyRequiredStatusTest('[[[Int!]]!]!', '[[[?]?]?]?')).to.equal( - '[[[Int]]]', - ); - }); - - it('applyRequiredStatus throws error when modifier is too deep', () => { - expect(() => { - applyRequiredStatusTest('[[[Int!]]!]!', '[[[[]]]]'); - }).to.throw('List nullability modifier is too deep.'); - }); - - it('applyRequiredStatus throws error when modifier is too shallow', () => { - expect(() => { - applyRequiredStatusTest('[[[Int!]]!]!', '[[]]'); - }).to.throw('List nullability modifier is too shallow.'); - }); - - it('applyRequiredStatus with required designator functions when list syntax is excluded', () => { - expect(applyRequiredStatusTest('[[[Int!]]!]', '!')).to.equal( - '[[[Int!]]!]!', - ); - }); - - it('applyRequiredStatus with optional designator functions when list syntax is excluded', () => { - expect(applyRequiredStatusTest('[[[Int!]]!]!', '?')).to.equal( - '[[[Int!]]!]', - ); - }); -}); \ No newline at end of file From fc0c01e0ad6e5ee40be2584de4be6fcbfac9d6fa Mon Sep 17 00:00:00 2001 From: twof Date: Fri, 25 Nov 2022 13:57:02 -0800 Subject: [PATCH 7/8] import naming convention --- src/validation/__tests__/harness.ts | 4 ++-- src/validation/rules/OverlappingFieldsCanBeMergedRule.ts | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/validation/__tests__/harness.ts b/src/validation/__tests__/harness.ts index 4423c48312..076c1b39c5 100644 --- a/src/validation/__tests__/harness.ts +++ b/src/validation/__tests__/harness.ts @@ -2,8 +2,8 @@ import { expectJSON } from '../../__testUtils__/expectJSON.js'; import type { Maybe } from '../../jsutils/Maybe.js'; -import type { ParseOptions } from '../../language/parser'; -import { parse } from '../../language/parser'; +import type { ParseOptions } from '../../language/parser.js'; +import { parse } from '../../language/parser.js'; import type { GraphQLSchema } from '../../type/schema.js'; diff --git a/src/validation/rules/OverlappingFieldsCanBeMergedRule.ts b/src/validation/rules/OverlappingFieldsCanBeMergedRule.ts index 73c6243523..d0ecad7a59 100644 --- a/src/validation/rules/OverlappingFieldsCanBeMergedRule.ts +++ b/src/validation/rules/OverlappingFieldsCanBeMergedRule.ts @@ -29,8 +29,8 @@ import { isObjectType, } from '../../type/definition.js'; -import { sortValueNode } from '../../utilities/sortValueNode'; -import { typeFromAST } from '../../utilities/typeFromAST'; +import { sortValueNode } from '../../utilities/sortValueNode.js'; +import { typeFromAST } from '../../utilities/typeFromAST.js'; import type { ValidationContext } from '../ValidationContext.js'; From abe402ab08783cc5ba905c9f268a989c8d5eef93 Mon Sep 17 00:00:00 2001 From: twof Date: Fri, 25 Nov 2022 14:31:05 -0800 Subject: [PATCH 8/8] more imports --- ...uiredStatusOnFieldMatchesDefinitionRule-test.ts | 6 +++--- .../RequiredStatusOnFieldMatchesDefinitionRule.ts | 14 +++++++------- 2 files changed, 10 insertions(+), 10 deletions(-) diff --git a/src/validation/__tests__/RequiredStatusOnFieldMatchesDefinitionRule-test.ts b/src/validation/__tests__/RequiredStatusOnFieldMatchesDefinitionRule-test.ts index 96718ac4cb..53f9f4b6ec 100644 --- a/src/validation/__tests__/RequiredStatusOnFieldMatchesDefinitionRule-test.ts +++ b/src/validation/__tests__/RequiredStatusOnFieldMatchesDefinitionRule-test.ts @@ -1,10 +1,10 @@ import { describe, it } from 'mocha'; -import { buildSchema } from '../../utilities/buildASTSchema'; +import { buildSchema } from '../../utilities/buildASTSchema.js'; -import { RequiredStatusOnFieldMatchesDefinitionRule } from '../rules/RequiredStatusOnFieldMatchesDefinitionRule'; +import { RequiredStatusOnFieldMatchesDefinitionRule } from '../rules/RequiredStatusOnFieldMatchesDefinitionRule.js'; -import { expectValidationErrorsWithSchema } from './harness'; +import { expectValidationErrorsWithSchema } from './harness.js'; function expectErrors(queryStr: string) { return expectValidationErrorsWithSchema( diff --git a/src/validation/rules/RequiredStatusOnFieldMatchesDefinitionRule.ts b/src/validation/rules/RequiredStatusOnFieldMatchesDefinitionRule.ts index 6925d7d2d3..549383e939 100644 --- a/src/validation/rules/RequiredStatusOnFieldMatchesDefinitionRule.ts +++ b/src/validation/rules/RequiredStatusOnFieldMatchesDefinitionRule.ts @@ -1,21 +1,21 @@ -import { GraphQLError } from '../../error/GraphQLError'; +import { GraphQLError } from '../../error/GraphQLError.js'; import type { FieldNode, ListNullabilityOperatorNode, NullabilityAssertionNode, -} from '../../language/ast'; -import type { ASTReducer, ASTVisitor } from '../../language/visitor'; -import { visit } from '../../language/visitor'; +} from '../../language/ast.js'; +import type { ASTReducer, ASTVisitor } from '../../language/visitor.js'; +import { visit } from '../../language/visitor.js'; -import type { GraphQLOutputType } from '../../type/definition'; +import type { GraphQLOutputType } from '../../type/definition.js'; import { assertListType, getNullableType, isListType, -} from '../../type/definition'; +} from '../../type/definition.js'; -import type { ValidationContext } from '../ValidationContext'; +import type { ValidationContext } from '../ValidationContext.js'; /** * List element nullability designators need to use a depth that is the same as or less than the