From 4b1ae0338845a4c701a1ea93530cf5c229590296 Mon Sep 17 00:00:00 2001 From: Matt Mahoney Date: Thu, 9 Feb 2023 16:09:43 -0500 Subject: [PATCH 1/4] Fragment Arguments: Only Syntax Changes --- src/language/kinds.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/src/language/kinds.ts b/src/language/kinds.ts index d606c12cbe..8d9098ed4e 100644 --- a/src/language/kinds.ts +++ b/src/language/kinds.ts @@ -22,6 +22,7 @@ export enum Kind { FRAGMENT_SPREAD = 'FragmentSpread', INLINE_FRAGMENT = 'InlineFragment', FRAGMENT_DEFINITION = 'FragmentDefinition', + FRAGMENT_ARGUMENT_DEFINITION = 'FragmentArgumentDefinition', /** Values */ VARIABLE = 'Variable', From e9fbda2a0702c22781550adb0a9915e48d8ba484 Mon Sep 17 00:00:00 2001 From: mjmahone Date: Thu, 19 Jan 2023 13:14:27 -0500 Subject: [PATCH 2/4] RFC: Fragment Arguments --- src/execution/__tests__/variables-test.ts | 250 +++++++++ src/execution/collectFields.ts | 28 +- src/utilities/TypeInfo.ts | 82 ++- src/utilities/__tests__/TypeInfo-test.ts | 104 ++++ src/utilities/keyForFragmentSpread.ts | 23 + src/utilities/substituteFragmentArguments.ts | 78 +++ src/validation/ValidationContext.ts | 15 +- .../__tests__/KnownDirectivesRule-test.ts | 1 + .../NoUndefinedVariablesRule-test.ts | 63 +++ .../NoUnusedFragmentVariablesRule-test.ts | 120 +++++ .../__tests__/NoUnusedVariablesRule-test.ts | 22 + .../OverlappingFieldsCanBeMergedRule-test.ts | 108 ++++ .../ProvidedRequiredArgumentsRule-test.ts | 108 ++++ .../__tests__/ValuesOfCorrectTypeRule-test.ts | 31 ++ .../VariablesAreInputTypesRule-test.ts | 27 + .../VariablesInAllowedPositionRule-test.ts | 104 ++++ src/validation/index.ts | 3 + .../rules/NoUndefinedVariablesRule.ts | 5 +- .../rules/NoUnusedFragmentVariablesRule.ts | 40 ++ src/validation/rules/NoUnusedVariablesRule.ts | 5 +- .../rules/OverlappingFieldsCanBeMergedRule.ts | 487 +++++++++++------- .../rules/ProvidedRequiredArgumentsRule.ts | 40 +- .../rules/VariablesInAllowedPositionRule.ts | 9 +- src/validation/specifiedRules.ts | 3 + 24 files changed, 1547 insertions(+), 209 deletions(-) create mode 100644 src/utilities/keyForFragmentSpread.ts create mode 100644 src/utilities/substituteFragmentArguments.ts create mode 100644 src/validation/__tests__/NoUnusedFragmentVariablesRule-test.ts create mode 100644 src/validation/rules/NoUnusedFragmentVariablesRule.ts diff --git a/src/execution/__tests__/variables-test.ts b/src/execution/__tests__/variables-test.ts index 6e4b39e810..10d88b8a5c 100644 --- a/src/execution/__tests__/variables-test.ts +++ b/src/execution/__tests__/variables-test.ts @@ -104,6 +104,13 @@ function fieldWithInputArg( }; } +const NestedType: GraphQLObjectType = new GraphQLObjectType({ + name: 'NestedType', + fields: { + echo: fieldWithInputArg({ type: GraphQLString }), + }, +}); + const TestType = new GraphQLObjectType({ name: 'TestType', fields: { @@ -129,6 +136,10 @@ const TestType = new GraphQLObjectType({ defaultValue: 'Hello World', }), list: fieldWithInputArg({ type: new GraphQLList(GraphQLString) }), + nested: { + type: NestedType, + resolve: () => ({}), + }, nnList: fieldWithInputArg({ type: new GraphQLNonNull(new GraphQLList(GraphQLString)), }), @@ -1066,6 +1077,245 @@ describe('Execute: Handles inputs', () => { }); }); + describe('using fragment arguments', () => { + it('when there are no fragment arguments', () => { + const result = executeQuery(` + query { + ...a + } + fragment a on TestType { + fieldWithNonNullableStringInput(input: "A") + } + `); + expect(result).to.deep.equal({ + data: { + fieldWithNonNullableStringInput: '"A"', + }, + }); + }); + + it('when a value is required and provided', () => { + const result = executeQuery(` + query { + ...a(value: "A") + } + fragment a($value: String!) on TestType { + fieldWithNonNullableStringInput(input: $value) + } + `); + expect(result).to.deep.equal({ + data: { + fieldWithNonNullableStringInput: '"A"', + }, + }); + }); + + it('when a value is required and not provided', () => { + const result = executeQuery(` + query { + ...a + } + fragment a($value: String!) on TestType { + fieldWithNullableStringInput(input: $value) + } + `); + expect(result).to.deep.equal({ + data: { + fieldWithNullableStringInput: null, + }, + }); + }); + + it('when the definition has a default and is provided', () => { + const result = executeQuery(` + query { + ...a(value: "A") + } + fragment a($value: String! = "B") on TestType { + fieldWithNonNullableStringInput(input: $value) + } + `); + expect(result).to.deep.equal({ + data: { + fieldWithNonNullableStringInput: '"A"', + }, + }); + }); + + it('when the definition has a default and is not provided', () => { + const result = executeQuery(` + query { + ...a + } + fragment a($value: String! = "B") on TestType { + fieldWithNonNullableStringInput(input: $value) + } + `); + expect(result).to.deep.equal({ + data: { + fieldWithNonNullableStringInput: '"B"', + }, + }); + }); + + it('when the definition has a non-nullable default and is provided null', () => { + const result = executeQuery(` + query { + ...a(value: null) + } + fragment a($value: String! = "B") on TestType { + fieldWithNullableStringInput(input: $value) + } + `); + expect(result).to.deep.equal({ + data: { + fieldWithNullableStringInput: 'null', + }, + }); + }); + + it('when the definition has no default and is not provided', () => { + const result = executeQuery(` + query { + ...a + } + fragment a($value: String) on TestType { + fieldWithNonNullableStringInputAndDefaultArgumentValue(input: $value) + } + `); + expect(result).to.deep.equal({ + data: { + fieldWithNonNullableStringInputAndDefaultArgumentValue: + '"Hello World"', + }, + }); + }); + + it('when an argument is shadowed by an operation variable', () => { + const result = executeQuery(` + query($x: String! = "A") { + ...a(x: "B") + } + fragment a($x: String) on TestType { + fieldWithNullableStringInput(input: $x) + } + `); + expect(result).to.deep.equal({ + data: { + fieldWithNullableStringInput: '"B"', + }, + }); + }); + + it('when a nullable argument with a field default is not provided and shadowed by an operation variable', () => { + const result = executeQuery(` + query($x: String = "A") { + ...a + } + fragment a($x: String) on TestType { + fieldWithNonNullableStringInputAndDefaultArgumentValue(input: $x) + } + `); + expect(result).to.deep.equal({ + data: { + fieldWithNonNullableStringInputAndDefaultArgumentValue: + '"Hello World"', + }, + }); + }); + + it('when a fragment is used with different args', () => { + const result = executeQuery(` + query($x: String = "Hello") { + a: nested { + ...a(x: "a") + } + b: nested { + ...a(x: "b", b: true) + } + hello: nested { + ...a(x: $x) + } + } + fragment a($x: String, $b: Boolean = false) on NestedType { + a: echo(input: $x) @skip(if: $b) + b: echo(input: $x) @include(if: $b) + } + `); + expect(result).to.deep.equal({ + data: { + a: { + a: '"a"', + }, + b: { + b: '"b"', + }, + hello: { + a: '"Hello"', + }, + }, + }); + }); + + it('when the argument variable is nested in a complex type', () => { + const result = executeQuery(` + query { + ...a(value: "C") + } + fragment a($value: String) on TestType { + list(input: ["A", "B", $value, "D"]) + } + `); + expect(result).to.deep.equal({ + data: { + list: '["A", "B", "C", "D"]', + }, + }); + }); + + it('when argument variables are used recursively', () => { + const result = executeQuery(` + query { + ...a(aValue: "C") + } + fragment a($aValue: String) on TestType { + ...b(bValue: $aValue) + } + fragment b($bValue: String) on TestType { + list(input: ["A", "B", $bValue, "D"]) + } + `); + expect(result).to.deep.equal({ + data: { + list: '["A", "B", "C", "D"]', + }, + }); + }); + + it('when argument passed in as list', () => { + const result = executeQuery(` + query Q($opValue: String = "op") { + ...a(aValue: "A") + } + fragment a($aValue: String, $bValue: String) on TestType { + ...b(aValue: [$aValue, "B"], bValue: [$bValue, $opValue]) + } + fragment b($aValue: [String], $bValue: [String], $cValue: String) on TestType { + aList: list(input: $aValue) + bList: list(input: $bValue) + cList: list(input: [$cValue]) + } + `); + expect(result).to.deep.equal({ + data: { + aList: '["A", "B"]', + bList: '[null, "op"]', + cList: '[null]', + }, + }); + }); + }); + describe('getVariableValues: limit maximum number of coercion errors', () => { const doc = parse(` query ($input: [String!]) { diff --git a/src/execution/collectFields.ts b/src/execution/collectFields.ts index 17468b791f..a5634023ad 100644 --- a/src/execution/collectFields.ts +++ b/src/execution/collectFields.ts @@ -22,6 +22,8 @@ import { } from '../type/directives.js'; import type { GraphQLSchema } from '../type/schema.js'; +import { keyForFragmentSpread } from '../utilities/keyForFragmentSpread.js'; +import { substituteFragmentArguments } from '../utilities/substituteFragmentArguments.js'; import { typeFromAST } from '../utilities/typeFromAST.js'; import { getDirectiveValues } from './values.js'; @@ -124,7 +126,7 @@ function collectFieldsImpl( selectionSet: SelectionSetNode, fields: AccumulatorMap, patches: Array, - visitedFragmentNames: Set, + visitedFragmentKeys: Set, ): void { for (const selection of selectionSet.selections) { switch (selection.kind) { @@ -156,7 +158,7 @@ function collectFieldsImpl( selection.selectionSet, patchFields, patches, - visitedFragmentNames, + visitedFragmentKeys, ); patches.push({ label: defer.label, @@ -172,24 +174,24 @@ function collectFieldsImpl( selection.selectionSet, fields, patches, - visitedFragmentNames, + visitedFragmentKeys, ); } break; } case Kind.FRAGMENT_SPREAD: { - const fragName = selection.name.value; + const fragmentKey = keyForFragmentSpread(selection); if (!shouldIncludeNode(variableValues, selection)) { continue; } const defer = getDeferValues(operation, variableValues, selection); - if (visitedFragmentNames.has(fragName) && !defer) { + if (visitedFragmentKeys.has(fragmentKey) && !defer) { continue; } - const fragment = fragments[fragName]; + const fragment = fragments[selection.name.value]; if ( !fragment || !doesFragmentConditionMatch(schema, fragment, runtimeType) @@ -198,9 +200,13 @@ function collectFieldsImpl( } if (!defer) { - visitedFragmentNames.add(fragName); + visitedFragmentKeys.add(fragmentKey); } + const fragmentSelectionSet = substituteFragmentArguments( + fragment, + selection, + ); if (defer) { const patchFields = new AccumulatorMap(); collectFieldsImpl( @@ -209,10 +215,10 @@ function collectFieldsImpl( variableValues, operation, runtimeType, - fragment.selectionSet, + fragmentSelectionSet, patchFields, patches, - visitedFragmentNames, + visitedFragmentKeys, ); patches.push({ label: defer.label, @@ -225,10 +231,10 @@ function collectFieldsImpl( variableValues, operation, runtimeType, - fragment.selectionSet, + fragmentSelectionSet, fields, patches, - visitedFragmentNames, + visitedFragmentKeys, ); } break; diff --git a/src/utilities/TypeInfo.ts b/src/utilities/TypeInfo.ts index fe8ea1fbab..2563a20660 100644 --- a/src/utilities/TypeInfo.ts +++ b/src/utilities/TypeInfo.ts @@ -1,6 +1,12 @@ import type { Maybe } from '../jsutils/Maybe.js'; +import type { ObjMap } from '../jsutils/ObjMap.js'; -import type { ASTNode, FieldNode } from '../language/ast.js'; +import type { + ASTNode, + FieldNode, + FragmentDefinitionNode, + FragmentSpreadNode, +} from '../language/ast.js'; import { isNode } from '../language/ast.js'; import { Kind } from '../language/kinds.js'; import type { ASTVisitor } from '../language/visitor.js'; @@ -31,6 +37,7 @@ import type { GraphQLDirective } from '../type/directives.js'; import type { GraphQLSchema } from '../type/schema.js'; import { typeFromAST } from './typeFromAST.js'; +import { valueFromAST } from './valueFromAST.js'; /** * TypeInfo is a utility class which, given a GraphQL schema, can keep track @@ -39,6 +46,7 @@ import { typeFromAST } from './typeFromAST.js'; */ export class TypeInfo { private _schema: GraphQLSchema; + private _typeStack: Array>; private _parentTypeStack: Array>; private _inputTypeStack: Array>; @@ -47,6 +55,8 @@ export class TypeInfo { private _directive: Maybe; private _argument: Maybe; private _enumValue: Maybe; + private _fragmentSpread: Maybe; + private _fragmentDefinitions: ObjMap; private _getFieldDef: GetFieldDefFn; constructor( @@ -69,6 +79,8 @@ export class TypeInfo { this._directive = null; this._argument = null; this._enumValue = null; + this._fragmentSpread = null; + this._fragmentDefinitions = Object.create(null); this._getFieldDef = getFieldDefFn ?? getFieldDef; if (initialType) { if (isInputType(initialType)) { @@ -142,6 +154,17 @@ export class TypeInfo { // checked before continuing since TypeInfo is used as part of validation // which occurs before guarantees of schema and document validity. switch (node.kind) { + case Kind.DOCUMENT: { + // A document's fragment definitions are type signatures + // referenced via fragment spreads. Ensure we can use definitions + // before visiting their call sites. + for (const astNode of node.definitions) { + if (astNode.kind === Kind.FRAGMENT_DEFINITION) { + this._fragmentDefinitions[astNode.name.value] = astNode; + } + } + break; + } case Kind.SELECTION_SET: { const namedType: unknown = getNamedType(this.getType()); this._parentTypeStack.push( @@ -180,6 +203,10 @@ export class TypeInfo { this._typeStack.push(isOutputType(outputType) ? outputType : undefined); break; } + case Kind.FRAGMENT_SPREAD: { + this._fragmentSpread = node; + break; + } case Kind.VARIABLE_DEFINITION: { const inputType: unknown = typeFromAST(schema, node.type); this._inputTypeStack.push( @@ -190,15 +217,50 @@ export class TypeInfo { case Kind.ARGUMENT: { let argDef; let argType: unknown; - const fieldOrDirective = this.getDirective() ?? this.getFieldDef(); - if (fieldOrDirective) { - argDef = fieldOrDirective.args.find( - (arg) => arg.name === node.name.value, + const directive = this.getDirective(); + const fragmentSpread = this._fragmentSpread; + const fieldDef = this.getFieldDef(); + if (directive) { + argDef = directive.args.find((arg) => arg.name === node.name.value); + } else if (fragmentSpread) { + const fragmentDef = + this._fragmentDefinitions[fragmentSpread.name.value]; + const fragVarDef = fragmentDef?.variableDefinitions?.find( + (varDef) => varDef.variable.name.value === node.name.value, ); - if (argDef) { - argType = argDef.type; + if (fragVarDef) { + const fragVarType = typeFromAST(schema, fragVarDef.type); + if (isInputType(fragVarType)) { + const fragVarDefault = fragVarDef.defaultValue + ? valueFromAST(fragVarDef.defaultValue, fragVarType) + : undefined; + + // Minor hack: transform the FragmentArgDef + // into a schema Argument definition, to + // enable visiting identically to field/directive args + const schemaArgDef: GraphQLArgument = { + name: fragVarDef.variable.name.value, + type: fragVarType, + defaultValue: fragVarDefault, + description: undefined, + deprecationReason: undefined, + extensions: {}, + astNode: { + ...fragVarDef, + kind: Kind.INPUT_VALUE_DEFINITION, + name: fragVarDef.variable.name, + }, + }; + argDef = schemaArgDef; + } } + } else if (fieldDef) { + argDef = fieldDef.args.find((arg) => arg.name === node.name.value); + } + if (argDef) { + argType = argDef.type; } + this._argument = argDef; this._defaultValueStack.push(argDef ? argDef.defaultValue : undefined); this._inputTypeStack.push(isInputType(argType) ? argType : undefined); @@ -248,6 +310,9 @@ export class TypeInfo { leave(node: ASTNode) { switch (node.kind) { + case Kind.DOCUMENT: + this._fragmentDefinitions = Object.create(null); + break; case Kind.SELECTION_SET: this._parentTypeStack.pop(); break; @@ -263,6 +328,9 @@ export class TypeInfo { case Kind.FRAGMENT_DEFINITION: this._typeStack.pop(); break; + case Kind.FRAGMENT_SPREAD: + this._fragmentSpread = null; + break; case Kind.VARIABLE_DEFINITION: this._inputTypeStack.pop(); break; diff --git a/src/utilities/__tests__/TypeInfo-test.ts b/src/utilities/__tests__/TypeInfo-test.ts index 971316f8b4..00053fe669 100644 --- a/src/utilities/__tests__/TypeInfo-test.ts +++ b/src/utilities/__tests__/TypeInfo-test.ts @@ -515,4 +515,108 @@ describe('visitWithTypeInfo', () => { ['leave', 'SelectionSet', null, 'Human', 'Human'], ]); }); + + it('supports traversals of fragment arguments', () => { + const typeInfo = new TypeInfo(testSchema); + + const ast = parse(` + query { + ...Foo(x: 4) + } + + fragment Foo( + $x: ID! + ) on QueryRoot { + human(id: $x) { name } + } + `); + + const visited: Array = []; + visit( + ast, + visitWithTypeInfo(typeInfo, { + enter(node) { + const type = typeInfo.getType(); + const inputType = typeInfo.getInputType(); + visited.push([ + 'enter', + node.kind, + node.kind === 'Name' ? node.value : null, + String(type), + String(inputType), + ]); + }, + leave(node) { + const type = typeInfo.getType(); + const inputType = typeInfo.getInputType(); + visited.push([ + 'leave', + node.kind, + node.kind === 'Name' ? node.value : null, + String(type), + String(inputType), + ]); + }, + }), + ); + + expect(visited).to.deep.equal([ + ['enter', 'Document', null, 'undefined', 'undefined'], + ['enter', 'OperationDefinition', null, 'QueryRoot', 'undefined'], + ['enter', 'SelectionSet', null, 'QueryRoot', 'undefined'], + ['enter', 'FragmentSpread', null, 'QueryRoot', 'undefined'], + ['enter', 'Name', 'Foo', 'QueryRoot', 'undefined'], + ['leave', 'Name', 'Foo', 'QueryRoot', 'undefined'], + ['enter', 'Argument', null, 'QueryRoot', 'ID!'], + ['enter', 'Name', 'x', 'QueryRoot', 'ID!'], + ['leave', 'Name', 'x', 'QueryRoot', 'ID!'], + ['enter', 'IntValue', null, 'QueryRoot', 'ID!'], + ['leave', 'IntValue', null, 'QueryRoot', 'ID!'], + ['leave', 'Argument', null, 'QueryRoot', 'ID!'], + ['leave', 'FragmentSpread', null, 'QueryRoot', 'undefined'], + ['leave', 'SelectionSet', null, 'QueryRoot', 'undefined'], + ['leave', 'OperationDefinition', null, 'QueryRoot', 'undefined'], + ['enter', 'FragmentDefinition', null, 'QueryRoot', 'undefined'], + ['enter', 'Name', 'Foo', 'QueryRoot', 'undefined'], + ['leave', 'Name', 'Foo', 'QueryRoot', 'undefined'], + ['enter', 'VariableDefinition', null, 'QueryRoot', 'ID!'], + ['enter', 'Variable', null, 'QueryRoot', 'ID!'], + ['enter', 'Name', 'x', 'QueryRoot', 'ID!'], + ['leave', 'Name', 'x', 'QueryRoot', 'ID!'], + ['leave', 'Variable', null, 'QueryRoot', 'ID!'], + ['enter', 'NonNullType', null, 'QueryRoot', 'ID!'], + ['enter', 'NamedType', null, 'QueryRoot', 'ID!'], + ['enter', 'Name', 'ID', 'QueryRoot', 'ID!'], + ['leave', 'Name', 'ID', 'QueryRoot', 'ID!'], + ['leave', 'NamedType', null, 'QueryRoot', 'ID!'], + ['leave', 'NonNullType', null, 'QueryRoot', 'ID!'], + ['leave', 'VariableDefinition', null, 'QueryRoot', 'ID!'], + ['enter', 'NamedType', null, 'QueryRoot', 'undefined'], + ['enter', 'Name', 'QueryRoot', 'QueryRoot', 'undefined'], + ['leave', 'Name', 'QueryRoot', 'QueryRoot', 'undefined'], + ['leave', 'NamedType', null, 'QueryRoot', 'undefined'], + ['enter', 'SelectionSet', null, 'QueryRoot', 'undefined'], + ['enter', 'Field', null, 'Human', 'undefined'], + ['enter', 'Name', 'human', 'Human', 'undefined'], + ['leave', 'Name', 'human', 'Human', 'undefined'], + ['enter', 'Argument', null, 'Human', 'ID'], + ['enter', 'Name', 'id', 'Human', 'ID'], + ['leave', 'Name', 'id', 'Human', 'ID'], + ['enter', 'Variable', null, 'Human', 'ID'], + ['enter', 'Name', 'x', 'Human', 'ID'], + ['leave', 'Name', 'x', 'Human', 'ID'], + ['leave', 'Variable', null, 'Human', 'ID'], + ['leave', 'Argument', null, 'Human', 'ID'], + ['enter', 'SelectionSet', null, 'Human', 'undefined'], + ['enter', 'Field', null, 'String', 'undefined'], + ['enter', 'Name', 'name', 'String', 'undefined'], + ['leave', 'Name', 'name', 'String', 'undefined'], + ['leave', 'Field', null, 'String', 'undefined'], + ['leave', 'SelectionSet', null, 'Human', 'undefined'], + ['leave', 'Field', null, 'Human', 'undefined'], + ['leave', 'SelectionSet', null, 'QueryRoot', 'undefined'], + ['leave', 'FragmentDefinition', null, 'QueryRoot', 'undefined'], + ['leave', 'Document', null, 'undefined', 'undefined'], + ]); + }); }); diff --git a/src/utilities/keyForFragmentSpread.ts b/src/utilities/keyForFragmentSpread.ts new file mode 100644 index 0000000000..95b2d9e6cc --- /dev/null +++ b/src/utilities/keyForFragmentSpread.ts @@ -0,0 +1,23 @@ +import type { FragmentSpreadNode } from '../language/ast.js'; +import { print } from '../language/printer.js'; + +/** + * Create a key that uniquely identifies common fragment spreads. + * Treats the fragment spread as the source of truth for the key: it + * does not bother to look up the argument definitions to de-duplicate default-variable args. + * + * Using the fragment definition to more accurately de-duplicate common spreads + * is a potential performance win, but in practice it seems unlikely to be common. + */ +export function keyForFragmentSpread(fragmentSpread: FragmentSpreadNode) { + const fragmentName = fragmentSpread.name.value; + const fragmentArguments = fragmentSpread.arguments; + if (fragmentArguments == null || fragmentArguments.length === 0) { + return fragmentName; + } + + const printedArguments: Array = fragmentArguments + .map(print) + .sort((a, b) => a.localeCompare(b)); + return fragmentName + '(' + printedArguments.join(',') + ')'; +} diff --git a/src/utilities/substituteFragmentArguments.ts b/src/utilities/substituteFragmentArguments.ts new file mode 100644 index 0000000000..1f0e0d8597 --- /dev/null +++ b/src/utilities/substituteFragmentArguments.ts @@ -0,0 +1,78 @@ +import type { Maybe } from '../jsutils/Maybe.js'; +import type { ObjMap } from '../jsutils/ObjMap.js'; + +import type { + ArgumentNode, + FragmentDefinitionNode, + FragmentSpreadNode, + SelectionSetNode, + ValueNode, + VariableDefinitionNode, +} from '../language/ast.js'; +import { Kind } from '../language/kinds.js'; +import { visit } from '../language/visitor.js'; + +/** + * Replaces all fragment argument values with non-fragment-scoped values. + * + * NOTE: fragment arguments are scoped to the fragment they're defined on. + * Therefore, after we apply the passed-in arguments, all remaining variables + * must be either operation defined variables or explicitly unset. + */ +export function substituteFragmentArguments( + def: FragmentDefinitionNode, + fragmentSpread: FragmentSpreadNode, +): SelectionSetNode { + const argumentDefinitions = def.variableDefinitions; + if (argumentDefinitions == null || argumentDefinitions.length === 0) { + return def.selectionSet; + } + const argumentValues = fragmentArgumentSubstitutions( + argumentDefinitions, + fragmentSpread.arguments, + ); + return visit(def.selectionSet, { + Variable(node) { + return argumentValues[node.name.value]; + }, + }); +} + +export function fragmentArgumentSubstitutions( + variableDefinitions: ReadonlyArray, + argumentValues: Maybe>, +): ObjMap { + const substitutions: ObjMap = {}; + if (argumentValues) { + for (const argument of argumentValues) { + substitutions[argument.name.value] = argument.value; + } + } + + for (const variableDefinition of variableDefinitions) { + const argumentName = variableDefinition.variable.name.value; + if (substitutions[argumentName]) { + continue; + } + + const defaultValue = variableDefinition.defaultValue; + if (defaultValue) { + substitutions[argumentName] = defaultValue; + } else { + // We need a way to allow unset arguments without accidentally + // replacing an unset fragment argument with an operation + // variable value. Fragment arguments must always have LOCAL scope. + // + // To remove this hack, we need to either: + // - include fragment argument scope when evaluating fields + // - make unset fragment arguments invalid + // Requiring the spread to pass all non-default-defined arguments is nice, + // but makes field argument default values impossible to use. + substitutions[argumentName] = { + kind: Kind.VARIABLE, + name: { kind: Kind.NAME, value: '__UNSET' }, + }; + } + } + return substitutions; +} diff --git a/src/validation/ValidationContext.ts b/src/validation/ValidationContext.ts index b0c5524fa7..b271c232f0 100644 --- a/src/validation/ValidationContext.ts +++ b/src/validation/ValidationContext.ts @@ -9,6 +9,7 @@ import type { FragmentSpreadNode, OperationDefinitionNode, SelectionSetNode, + VariableDefinitionNode, VariableNode, } from '../language/ast.js'; import { Kind } from '../language/kinds.js'; @@ -26,13 +27,15 @@ import type { import type { GraphQLDirective } from '../type/directives.js'; import type { GraphQLSchema } from '../type/schema.js'; -import { TypeInfo, visitWithTypeInfo } from '../utilities/TypeInfo.js'; +import type { TypeInfo } from '../utilities/TypeInfo.js'; +import { visitWithTypeInfo } from '../utilities/TypeInfo.js'; type NodeWithSelectionSet = OperationDefinitionNode | FragmentDefinitionNode; interface VariableUsage { readonly node: VariableNode; readonly type: Maybe; readonly defaultValue: Maybe; + readonly fragmentVarDef: Maybe; } /** @@ -199,16 +202,24 @@ export class ValidationContext extends ASTValidationContext { let usages = this._variableUsages.get(node); if (!usages) { const newUsages: Array = []; - const typeInfo = new TypeInfo(this._schema); + const typeInfo = this._typeInfo; + const localVariableDefinitions = + node.kind === Kind.FRAGMENT_DEFINITION + ? node.variableDefinitions + : undefined; visit( node, visitWithTypeInfo(typeInfo, { VariableDefinition: () => false, Variable(variable) { + const fragmentVarDef = localVariableDefinitions?.find( + (varDef) => varDef.variable.name.value === variable.name.value, + ); newUsages.push({ node: variable, type: typeInfo.getInputType(), defaultValue: typeInfo.getDefaultValue(), + fragmentVarDef, }); }, }), diff --git a/src/validation/__tests__/KnownDirectivesRule-test.ts b/src/validation/__tests__/KnownDirectivesRule-test.ts index c27155c9b2..cbcace2ca0 100644 --- a/src/validation/__tests__/KnownDirectivesRule-test.ts +++ b/src/validation/__tests__/KnownDirectivesRule-test.ts @@ -41,6 +41,7 @@ const schemaWithDirectives = buildSchema(` directive @onSubscription on SUBSCRIPTION directive @onField on FIELD directive @onFragmentDefinition on FRAGMENT_DEFINITION + directive @onFragmentVariableDefinition on VARIABLE_DEFINITION directive @onFragmentSpread on FRAGMENT_SPREAD directive @onFragmentVariableDefinition on FRAGMENT_VARIABLE_DEFINITION directive @onInlineFragment on INLINE_FRAGMENT diff --git a/src/validation/__tests__/NoUndefinedVariablesRule-test.ts b/src/validation/__tests__/NoUndefinedVariablesRule-test.ts index c6ed758cad..9d53ea8c2e 100644 --- a/src/validation/__tests__/NoUndefinedVariablesRule-test.ts +++ b/src/validation/__tests__/NoUndefinedVariablesRule-test.ts @@ -404,4 +404,67 @@ describe('Validate: No undefined variables', () => { }, ]); }); + + it('fragment defined arguments are not undefined variables', () => { + expectValid(` + query Foo { + ...FragA + } + fragment FragA($a: String) on Type { + field1(a: $a) + } + `); + }); + + it('defined variables used as fragment arguments are not undefined variables', () => { + expectValid(` + query Foo($b: String) { + ...FragA(a: $b) + } + fragment FragA($a: String) on Type { + field1 + } + `); + }); + + it('variables used as fragment arguments may be undefined variables', () => { + expectErrors(` + query Foo { + ...FragA(a: $a) + } + fragment FragA($a: String) on Type { + field1 + } + `).toDeepEqual([ + { + message: 'Variable "$a" is not defined by operation "Foo".', + locations: [ + { line: 3, column: 21 }, + { line: 2, column: 7 }, + ], + }, + ]); + }); + + it('variables shadowed by parent fragment arguments are still undefined variables', () => { + expectErrors(` + query Foo { + ...FragA + } + fragment FragA($a: String) on Type { + ...FragB + } + fragment FragB on Type { + field1(a: $a) + } + `).toDeepEqual([ + { + message: 'Variable "$a" is not defined by operation "Foo".', + locations: [ + { line: 9, column: 19 }, + { line: 2, column: 7 }, + ], + }, + ]); + }); }); diff --git a/src/validation/__tests__/NoUnusedFragmentVariablesRule-test.ts b/src/validation/__tests__/NoUnusedFragmentVariablesRule-test.ts new file mode 100644 index 0000000000..016a00582c --- /dev/null +++ b/src/validation/__tests__/NoUnusedFragmentVariablesRule-test.ts @@ -0,0 +1,120 @@ +import { describe, it } from 'mocha'; + +import { NoUnusedFragmentVariablesRule } from '../rules/NoUnusedFragmentVariablesRule.js'; + +import { expectValidationErrors } from './harness.js'; + +function expectErrors(queryStr: string) { + return expectValidationErrors(NoUnusedFragmentVariablesRule, queryStr); +} + +function expectValid(queryStr: string) { + expectErrors(queryStr).toDeepEqual([]); +} + +describe('Validate: No unused fragment variables', () => { + it('uses all variables', () => { + expectValid(` + fragment Foo($a: String, $b: String, $c: String) on Type { + field(a: $a, b: $b, c: $c) + } + `); + }); + + it('uses all variables deeply', () => { + expectValid(` + fragment Foo($a: String, $b: String, $c: String) on Type { + field(a: $a) { + field(b: $b) { + field(c: $c) + } + } + } + `); + }); + + it('uses all variables deeply in inline fragments', () => { + expectValid(` + fragment Foo($a: String, $b: String, $c: String) on Type { + ... on Type { + field(a: $a) { + field(b: $b) { + ... on Type { + field(c: $c) + } + } + } + } + } + `); + }); + + it('variable not used', () => { + expectErrors(` + fragment Foo($a: String, $b: String, $c: String) on Type { + field(a: $a, b: $b) + } + `).toDeepEqual([ + { + message: 'Variable "$c" is never used in fragment "Foo".', + locations: [{ line: 2, column: 44 }], + }, + ]); + }); + + it('query passes argument for unused variable', () => { + expectErrors(` + query Q($c: String) { + type { + ...Foo(a: "", b: "", c: $c) + } + } + fragment Foo($a: String, $b: String, $c: String) on Type { + field(a: $a, b: $b) + } + `).toDeepEqual([ + { + message: 'Variable "$c" is never used in fragment "Foo".', + locations: [{ line: 7, column: 44 }], + }, + ]); + }); + + it('child fragment uses a variable of the same name', () => { + expectErrors(` + query Q($a: String) { + type { + ...Foo + } + } + fragment Foo($a: String) on Type { + ...Bar + } + fragment Bar on Type { + field(a: $a) + } + `).toDeepEqual([ + { + message: 'Variable "$a" is never used in fragment "Foo".', + locations: [{ line: 7, column: 20 }], + }, + ]); + }); + + it('multiple variables not used', () => { + expectErrors(` + fragment Foo($a: String, $b: String, $c: String) on Type { + field(b: $b) + } + `).toDeepEqual([ + { + message: 'Variable "$a" is never used in fragment "Foo".', + locations: [{ line: 2, column: 20 }], + }, + { + message: 'Variable "$c" is never used in fragment "Foo".', + locations: [{ line: 2, column: 44 }], + }, + ]); + }); +}); diff --git a/src/validation/__tests__/NoUnusedVariablesRule-test.ts b/src/validation/__tests__/NoUnusedVariablesRule-test.ts index 47dac39c99..d08d547931 100644 --- a/src/validation/__tests__/NoUnusedVariablesRule-test.ts +++ b/src/validation/__tests__/NoUnusedVariablesRule-test.ts @@ -230,4 +230,26 @@ describe('Validate: No unused variables', () => { }, ]); }); + + it('fragment defined arguments are not unused variables', () => { + expectValid(` + query Foo { + ...FragA + } + fragment FragA($a: String) on Type { + field1(a: $a) + } + `); + }); + + it('defined variables used as fragment arguments are not unused variables', () => { + expectValid(` + query Foo($b: String) { + ...FragA(a: $b) + } + fragment FragA($a: String) on Type { + field1 + } + `); + }); }); diff --git a/src/validation/__tests__/OverlappingFieldsCanBeMergedRule-test.ts b/src/validation/__tests__/OverlappingFieldsCanBeMergedRule-test.ts index 52c2deb1a0..2656900f6e 100644 --- a/src/validation/__tests__/OverlappingFieldsCanBeMergedRule-test.ts +++ b/src/validation/__tests__/OverlappingFieldsCanBeMergedRule-test.ts @@ -1246,4 +1246,112 @@ describe('Validate: Overlapping fields can be merged', () => { }, ]); }); + + describe('fragment arguments must produce fields that can be merged', () => { + it('allows conflicting spreads at different depths', () => { + expectValid(` + query ValidDifferingFragmentArgs($command1: DogCommand, $command2: DogCommand) { + dog { + ...DoesKnowCommand(command: $command1) + mother { + ...DoesKnowCommand(command: $command2) + } + } + } + + fragment DoesKnowCommand($command: DogCommand) on Dog { + doesKnowCommand(dogCommand: $command) + } + `); + }); + + it('encounters conflict in fragments', () => { + expectErrors(` + { + ...WithArgs(x: 3) + ...WithArgs(x: 4) + } + fragment WithArgs($x: Int) on Type { + a(x: $x) + } + `).toDeepEqual([ + { + message: + 'Spreads "WithArgs" conflict because WithArgs(x: 3) and WithArgs(x: 4) have different fragment arguments.', + locations: [ + { line: 3, column: 11 }, + { line: 4, column: 11 }, + ], + }, + ]); + }); + + it('encounters nested field conflict in fragments that could otherwise merge', () => { + expectErrors(` + query ValidDifferingFragmentArgs($command1: DogCommand, $command2: DogCommand) { + dog { + ...DoesKnowCommandNested(command: $command1) + mother { + ...DoesKnowCommandNested(command: $command2) + } + } + } + + fragment DoesKnowCommandNested($command: DogCommand) on Dog { + doesKnowCommand(dogCommand: $command) + mother { + doesKnowCommand(dogCommand: $command) + } + } + `).toDeepEqual([ + { + message: + 'Fields "mother" conflict because subfields "doesKnowCommand" conflict because they have differing arguments. Use different aliases on the fields to fetch both if this was intentional.', + locations: [ + { line: 5, column: 13 }, + { line: 14, column: 13 }, + { line: 13, column: 11 }, + { line: 12, column: 11 }, + ], + }, + ]); + }); + + it('encounters nested conflict in fragments', () => { + expectErrors(` + { + connection { + edges { + ...WithArgs(x: 3) + } + } + ...Connection + } + + fragment Connection on Type { + connection { + edges { + ...WithArgs(x: 4) + } + } + } + fragment WithArgs($x: Int) on Type { + a(x: $x) + } + `).toDeepEqual([ + { + message: + 'Fields "connection" conflict because subfields "edges" conflict because child spreads "WithArgs" conflict because WithArgs(x: 3) and WithArgs(x: 4) have different fragment arguments. Use different aliases on the fields to fetch both if this was intentional.', + locations: [ + { line: 3, column: 11 }, + { line: 4, column: 13 }, + { line: 5, column: 15 }, + { line: 12, column: 11 }, + { line: 13, column: 13 }, + { line: 14, column: 15 }, + ], + }, + ]); + }); + }); }); diff --git a/src/validation/__tests__/ProvidedRequiredArgumentsRule-test.ts b/src/validation/__tests__/ProvidedRequiredArgumentsRule-test.ts index 6f0d223c15..66545ddb59 100644 --- a/src/validation/__tests__/ProvidedRequiredArgumentsRule-test.ts +++ b/src/validation/__tests__/ProvidedRequiredArgumentsRule-test.ts @@ -356,4 +356,112 @@ describe('Validate: Provided required arguments', () => { ]); }); }); + + describe('Fragment required arguments', () => { + it('ignores unknown arguments', () => { + expectValid(` + { + ...Foo(unknownArgument: true) + } + fragment Foo on Query { + dog + } + `); + }); + + // Query: should this be allowed? + // We could differentiate between required/optional (i.e. no default value) + // vs. nullable/non-nullable (i.e. no !), whereas now they are conflated. + // So today: + // $x: Int! `x:` is required and must not be null (NOT a nullable variable) + // $x: Int! = 3 `x:` is not required and must not be null (MAY BE a nullable variable) + // $x: Int `x:` is not required and may be null + // $x: Int = 3 `x:` is not required and may be null + // + // It feels weird to collapse the nullable cases but not the non-nullable ones. + // Whereas all four feel like they ought to mean something explicitly different. + // + // Potential proposal: + // $x: Int! `x:` is required and must not be null (NOT a nullable variable) + // $x: Int! = 3 `x:` is not required and must not be null (NOT a nullable variable) + // $x: Int `x:` is required and may be null + // $x: Int = 3 `x:` is not required and may be null + // + // Required then is whether there's a default value, + // and nullable is whether there's a ! + it('Missing nullable argument with default is allowed', () => { + expectValid(` + { + ...F + + } + fragment F($x: Int = 3) on Query { + foo + } + `); + }); + // Above proposal: this should be an error + it('Missing nullable argument is allowed', () => { + expectValid(` + { + ...F + + } + fragment F($x: Int) on Query { + foo + } + `); + }); + it('Missing non-nullable argument with default is allowed', () => { + expectValid(` + { + ...F + + } + fragment F($x: Int! = 3) on Query { + foo + } + `); + }); + it('Missing non-nullable argument is not allowed', () => { + expectErrors(` + { + ...F + + } + fragment F($x: Int!) on Query { + foo + } + `).toDeepEqual([ + { + message: + 'Fragment "F" argument "x" of type "{ kind: "NonNullType", type: { kind: "NamedType", name: [Object], loc: [Object] }, loc: [Object] }" is required, but it was not provided.', + locations: [ + { line: 3, column: 13 }, + { line: 6, column: 22 }, + ], + }, + ]); + }); + + it('Supplies required variables', () => { + expectValid(` + { + ...F(x: 3) + + } + fragment F($x: Int!) on Query { + foo + } + `); + }); + + it('Skips missing fragments', () => { + expectValid(` + { + ...Missing(x: 3) + } + `); + }); + }); }); diff --git a/src/validation/__tests__/ValuesOfCorrectTypeRule-test.ts b/src/validation/__tests__/ValuesOfCorrectTypeRule-test.ts index f0b7dfa57e..d843139d89 100644 --- a/src/validation/__tests__/ValuesOfCorrectTypeRule-test.ts +++ b/src/validation/__tests__/ValuesOfCorrectTypeRule-test.ts @@ -1198,4 +1198,35 @@ describe('Validate: Values of correct type', () => { ]); }); }); + + describe('Fragment argument values', () => { + it('list variables with invalid item', () => { + expectErrors(` + fragment InvalidItem($a: [String] = ["one", 2]) on Query { + dog { name } + } + `).toDeepEqual([ + { + message: 'String cannot represent a non string value: 2', + locations: [{ line: 2, column: 53 }], + }, + ]); + }); + + it('fragment spread with invalid argument value', () => { + expectErrors(` + fragment GivesString on Query { + ...ExpectsInt(a: "three") + } + fragment ExpectsInt($a: Int) on Query { + dog { name } + } + `).toDeepEqual([ + { + message: 'Int cannot represent non-integer value: "three"', + locations: [{ line: 3, column: 28 }], + }, + ]); + }); + }); }); diff --git a/src/validation/__tests__/VariablesAreInputTypesRule-test.ts b/src/validation/__tests__/VariablesAreInputTypesRule-test.ts index 8027a35826..eddd202df8 100644 --- a/src/validation/__tests__/VariablesAreInputTypesRule-test.ts +++ b/src/validation/__tests__/VariablesAreInputTypesRule-test.ts @@ -18,6 +18,9 @@ describe('Validate: Variables are input types', () => { query Foo($a: Unknown, $b: [[Unknown!]]!) { field(a: $a, b: $b) } + fragment Bar($a: Unknown, $b: [[Unknown!]]!) on Query { + field(a: $a, b: $b) + } `); }); @@ -26,6 +29,9 @@ describe('Validate: Variables are input types', () => { query Foo($a: String, $b: [Boolean!]!, $c: ComplexInput) { field(a: $a, b: $b, c: $c) } + fragment Bar($a: String, $b: [Boolean!]!, $c: ComplexInput) on Query { + field(a: $a, b: $b, c: $c) + } `); }); @@ -49,4 +55,25 @@ describe('Validate: Variables are input types', () => { }, ]); }); + + it('output types on fragment arguments are invalid', () => { + expectErrors(` + fragment Bar($a: Dog, $b: [[CatOrDog!]]!, $c: Pet) on Query { + field(a: $a, b: $b, c: $c) + } + `).toDeepEqual([ + { + locations: [{ line: 2, column: 24 }], + message: 'Variable "$a" cannot be non-input type "Dog".', + }, + { + locations: [{ line: 2, column: 33 }], + message: 'Variable "$b" cannot be non-input type "[[CatOrDog!]]!".', + }, + { + locations: [{ line: 2, column: 53 }], + message: 'Variable "$c" cannot be non-input type "Pet".', + }, + ]); + }); }); diff --git a/src/validation/__tests__/VariablesInAllowedPositionRule-test.ts b/src/validation/__tests__/VariablesInAllowedPositionRule-test.ts index 16467741bb..d0da8f5305 100644 --- a/src/validation/__tests__/VariablesInAllowedPositionRule-test.ts +++ b/src/validation/__tests__/VariablesInAllowedPositionRule-test.ts @@ -356,5 +356,109 @@ describe('Validate: Variables are in allowed positions', () => { dog @include(if: $boolVar) }`); }); + + it('undefined in directive with default value with option', () => { + expectValid(` + { + dog @include(if: $x) + }`); + }); + }); + + describe('Fragment arguments are validated', () => { + it('Boolean => Boolean', () => { + expectValid(` + query Query($booleanArg: Boolean) + { + complicatedArgs { + ...A(b: $booleanArg) + } + } + fragment A($b: Boolean) on ComplicatedArgs { + booleanArgField(booleanArg: $b) + } + `); + }); + + it('Boolean => Boolean with default value', () => { + expectValid(` + query Query($booleanArg: Boolean) + { + complicatedArgs { + ...A(b: $booleanArg) + } + } + fragment A($b: Boolean = true) on ComplicatedArgs { + booleanArgField(booleanArg: $b) + } + `); + }); + + it('Boolean => Boolean!', () => { + expectErrors(` + query Query($ab: Boolean) + { + complicatedArgs { + ...A(b: $ab) + } + } + fragment A($b: Boolean!) on ComplicatedArgs { + booleanArgField(booleanArg: $b) + } + `).toDeepEqual([ + { + message: + 'Variable "$ab" of type "Boolean" used in position expecting type "Boolean!".', + locations: [ + { line: 2, column: 21 }, + { line: 5, column: 21 }, + ], + }, + ]); + }); + + it('Int => Int! fails when variable provides null default value', () => { + expectErrors(` + query Query($intVar: Int = null) { + complicatedArgs { + ...A(i: $intVar) + } + } + fragment A($i: Int!) on ComplicatedArgs { + nonNullIntArgField(nonNullIntArg: $i) + } + `).toDeepEqual([ + { + message: + 'Variable "$intVar" of type "Int" used in position expecting type "Int!".', + locations: [ + { line: 2, column: 21 }, + { line: 4, column: 21 }, + ], + }, + ]); + }); + + it('Int fragment arg => Int! field arg fails even when shadowed by Int! variable', () => { + expectErrors(` + query Query($intVar: Int!) { + complicatedArgs { + ...A(i: $intVar) + } + } + fragment A($intVar: Int) on ComplicatedArgs { + nonNullIntArgField(nonNullIntArg: $intVar) + } + `).toDeepEqual([ + { + message: + 'Variable "$intVar" of type "Int" used in position expecting type "Int!".', + locations: [ + { line: 7, column: 20 }, + { line: 8, column: 45 }, + ], + }, + ]); + }); }); }); diff --git a/src/validation/index.ts b/src/validation/index.ts index b0cc754490..54615bdeee 100644 --- a/src/validation/index.ts +++ b/src/validation/index.ts @@ -48,6 +48,9 @@ export { NoUndefinedVariablesRule } from './rules/NoUndefinedVariablesRule.js'; // Spec Section: "Fragments must be used" export { NoUnusedFragmentsRule } from './rules/NoUnusedFragmentsRule.js'; +// Spec Section: "All Fragment Variables Used" +export { NoUnusedFragmentVariablesRule } from './rules/NoUnusedFragmentVariablesRule.js'; + // Spec Section: "All Variables Used" export { NoUnusedVariablesRule } from './rules/NoUnusedVariablesRule.js'; diff --git a/src/validation/rules/NoUndefinedVariablesRule.ts b/src/validation/rules/NoUndefinedVariablesRule.ts index d1672ecd0b..dea99da3e9 100644 --- a/src/validation/rules/NoUndefinedVariablesRule.ts +++ b/src/validation/rules/NoUndefinedVariablesRule.ts @@ -22,7 +22,10 @@ export function NoUndefinedVariablesRule( ); const usages = context.getRecursiveVariableUsages(operation); - for (const { node } of usages) { + for (const { node, fragmentVarDef } of usages) { + if (fragmentVarDef) { + continue; + } const varName = node.name.value; if (!variableNameDefined.has(varName)) { context.reportError( diff --git a/src/validation/rules/NoUnusedFragmentVariablesRule.ts b/src/validation/rules/NoUnusedFragmentVariablesRule.ts new file mode 100644 index 0000000000..3012d7b996 --- /dev/null +++ b/src/validation/rules/NoUnusedFragmentVariablesRule.ts @@ -0,0 +1,40 @@ +import { GraphQLError } from '../../error/GraphQLError.js'; + +import type { ASTVisitor } from '../../language/visitor.js'; + +import type { ValidationContext } from '../ValidationContext.js'; + +/** + * No unused variables + * + * A GraphQL fragment is only valid if all arguments defined by it + * are used within the same fragment. + * + * See https://spec.graphql.org/draft/#sec-All-Variables-Used + */ +export function NoUnusedFragmentVariablesRule( + context: ValidationContext, +): ASTVisitor { + return { + FragmentDefinition(fragment) { + const usages = context.getVariableUsages(fragment); + const argumentNameUsed = new Set( + usages.map(({ node }) => node.name.value), + ); + // FIXME: https://github.com/graphql/graphql-js/issues/2203 + /* c8 ignore next */ + const variableDefinitions = fragment.variableDefinitions ?? []; + for (const varDef of variableDefinitions) { + const argName = varDef.variable.name.value; + if (!argumentNameUsed.has(argName)) { + context.reportError( + new GraphQLError( + `Variable "$${argName}" is never used in fragment "${fragment.name.value}".`, + { nodes: varDef }, + ), + ); + } + } + }, + }; +} diff --git a/src/validation/rules/NoUnusedVariablesRule.ts b/src/validation/rules/NoUnusedVariablesRule.ts index 7a0660cce0..44d39d067b 100644 --- a/src/validation/rules/NoUnusedVariablesRule.ts +++ b/src/validation/rules/NoUnusedVariablesRule.ts @@ -17,7 +17,10 @@ export function NoUnusedVariablesRule(context: ValidationContext): ASTVisitor { OperationDefinition(operation) { const usages = context.getRecursiveVariableUsages(operation); const variableNameUsed = new Set( - usages.map(({ node }) => node.name.value), + usages + // Skip variables used as fragment arguments + .filter(({ fragmentVarDef }) => !fragmentVarDef) + .map(({ node }) => node.name.value), ); // FIXME: https://github.com/graphql/graphql-js/issues/2203 diff --git a/src/validation/rules/OverlappingFieldsCanBeMergedRule.ts b/src/validation/rules/OverlappingFieldsCanBeMergedRule.ts index e2444047c6..d46aae5a6f 100644 --- a/src/validation/rules/OverlappingFieldsCanBeMergedRule.ts +++ b/src/validation/rules/OverlappingFieldsCanBeMergedRule.ts @@ -8,6 +8,7 @@ import type { DirectiveNode, FieldNode, FragmentDefinitionNode, + FragmentSpreadNode, ObjectValueNode, SelectionSetNode, } from '../../language/ast.js'; @@ -29,7 +30,9 @@ import { isObjectType, } from '../../type/definition.js'; +import { keyForFragmentSpread } from '../../utilities/keyForFragmentSpread.js'; import { sortValueNode } from '../../utilities/sortValueNode.js'; +import { substituteFragmentArguments } from '../../utilities/substituteFragmentArguments.js'; import { typeFromAST } from '../../utilities/typeFromAST.js'; import type { ValidationContext } from '../ValidationContext.js'; @@ -38,17 +41,25 @@ import type { ValidationContext } from '../ValidationContext.js'; // This file contains a lot of such errors but we plan to refactor it anyway // so just disable it for entire file. -function reasonMessage(reason: ConflictReasonMessage): string { - if (Array.isArray(reason)) { - return reason - .map( - ([responseName, subReason]) => - `subfields "${responseName}" conflict because ` + - reasonMessage(subReason), - ) +function reasonMessage(message: ConflictReasonMessage): string { + if (Array.isArray(message)) { + return message + .map((subC) => { + const subConflict = subC; + if (subConflict.kind === 'FIELD') { + return ( + `subfields "${subConflict.responseName}" conflict because ` + + reasonMessage(subConflict.reasonMessage) + ); + } + return ( + `child spreads "${subConflict.fragmentName}" conflict because ` + + reasonMessage(subConflict.reasonMessage) + ); + }) .join(' and '); } - return reason; + return message; } /** @@ -68,36 +79,62 @@ export function OverlappingFieldsCanBeMergedRule( // dramatically improve the performance of this validator. const comparedFragmentPairs = new PairSet(); - // A cache for the "field map" and list of fragment names found in any given + // A cache for the "field map" and list of fragment spreads found in any given // selection set. Selection sets may be asked for this information multiple // times, so this improves the performance of this validator. - const cachedFieldsAndFragmentNames = new Map(); + const cachedFieldsAndFragmentSpreads = new Map(); return { SelectionSet(selectionSet) { const conflicts = findConflictsWithinSelectionSet( context, - cachedFieldsAndFragmentNames, + cachedFieldsAndFragmentSpreads, comparedFragmentPairs, context.getParentType(), selectionSet, ); - for (const [[responseName, reason], fields1, fields2] of conflicts) { - const reasonMsg = reasonMessage(reason); - context.reportError( - new GraphQLError( - `Fields "${responseName}" conflict because ${reasonMsg}. Use different aliases on the fields to fetch both if this was intentional.`, - { nodes: fields1.concat(fields2) }, - ), - ); + for (const { reason, selectionPath1, selectionPath2 } of conflicts) { + const reasonMsg = reasonMessage(reason.reasonMessage); + const errorNodes = { nodes: selectionPath1.concat(selectionPath2) }; + if (reason.kind === 'FIELD') { + context.reportError( + new GraphQLError( + `Fields "${reason.responseName}" conflict because ${reasonMsg}. Use different aliases on the fields to fetch both if this was intentional.`, + errorNodes, + ), + ); + } else { + // FRAGMENT_SPREAD + context.reportError( + new GraphQLError( + // Fragments can't be aliased, so there's no easy way to resolve these conflicts today. + `Spreads "${reason.fragmentName}" conflict because ${reasonMsg}.`, + errorNodes, + ), + ); + } } }, }; } -type Conflict = [ConflictReason, Array, Array]; +interface Conflict { + reason: ConflictReason; + selectionPath1: Array; + selectionPath2: Array; +} // Field name and reason. -type ConflictReason = [string, ConflictReasonMessage]; +type ConflictReason = FieldConflictReason | FragmentSpreadConflictReason; +interface FieldConflictReason { + kind: 'FIELD'; + responseName: string; + reasonMessage: ConflictReasonMessage; +} +interface FragmentSpreadConflictReason { + kind: 'FRAGMENT_SPREAD'; + fragmentName: string; + reasonMessage: ConflictReasonMessage; +} // Reason is a string, or a nested list of conflicts. type ConflictReasonMessage = string | Array; // Tuple defining a field node in a context. @@ -108,8 +145,11 @@ type NodeAndDef = [ ]; // Map of array of those. type NodeAndDefCollection = ObjMap>; -type FragmentNames = ReadonlyArray; -type FieldsAndFragmentNames = readonly [NodeAndDefCollection, FragmentNames]; +type FragmentSpreadsByKey = ObjMap; +type FieldsAndFragmentSpreads = readonly [ + NodeAndDefCollection, + FragmentSpreadsByKey, +]; /** * Algorithm: @@ -171,58 +211,60 @@ type FieldsAndFragmentNames = readonly [NodeAndDefCollection, FragmentNames]; // GraphQL Document. function findConflictsWithinSelectionSet( context: ValidationContext, - cachedFieldsAndFragmentNames: Map, + cachedFieldsAndFragmentSpreads: Map< + SelectionSetNode, + FieldsAndFragmentSpreads + >, comparedFragmentPairs: PairSet, parentType: Maybe, selectionSet: SelectionSetNode, ): Array { const conflicts: Array = []; - const [fieldMap, fragmentNames] = getFieldsAndFragmentNames( + const [fieldMap, spreadCollection] = getFieldsAndFragmentSpreads( context, - cachedFieldsAndFragmentNames, + cachedFieldsAndFragmentSpreads, parentType, selectionSet, ); - // (A) Find find all conflicts "within" the fields of this selection set. + // (A) First find all conflicts "within" the fields and spreads of this selection set. // Note: this is the *only place* `collectConflictsWithin` is called. collectConflictsWithin( context, conflicts, - cachedFieldsAndFragmentNames, + cachedFieldsAndFragmentSpreads, comparedFragmentPairs, fieldMap, ); - if (fragmentNames.length !== 0) { - // (B) Then collect conflicts between these fields and those represented by - // each spread fragment name found. - for (let i = 0; i < fragmentNames.length; i++) { - collectConflictsBetweenFieldsAndFragment( + // (B) Then collect conflicts between these fields and those represented by + // each spread fragment name found. + const fragmentSpreads = Object.values(spreadCollection); + for (let i = 0; i < fragmentSpreads.length; i++) { + collectConflictsBetweenFieldsAndFragment( + context, + conflicts, + cachedFieldsAndFragmentSpreads, + comparedFragmentPairs, + false, + fieldMap, + fragmentSpreads[i], + ); + // (C) Then compare this fragment with all other fragments found in this + // selection set to collect conflicts between fragments spread together. + // This compares each item in the list of fragment names to every other + // item in that same list (except for itself). + for (let j = i + 1; j < fragmentSpreads.length; j++) { + collectConflictsBetweenFragments( context, conflicts, - cachedFieldsAndFragmentNames, + cachedFieldsAndFragmentSpreads, comparedFragmentPairs, false, - fieldMap, - fragmentNames[i], + fragmentSpreads[i], + fragmentSpreads[j], ); - // (C) Then compare this fragment with all other fragments found in this - // selection set to collect conflicts between fragments spread together. - // This compares each item in the list of fragment names to every other - // item in that same list (except for itself). - for (let j = i + 1; j < fragmentNames.length; j++) { - collectConflictsBetweenFragments( - context, - conflicts, - cachedFieldsAndFragmentNames, - comparedFragmentPairs, - false, - fragmentNames[i], - fragmentNames[j], - ); - } } } return conflicts; @@ -233,22 +275,28 @@ function findConflictsWithinSelectionSet( function collectConflictsBetweenFieldsAndFragment( context: ValidationContext, conflicts: Array, - cachedFieldsAndFragmentNames: Map, + cachedFieldsAndFragmentSpreads: Map< + SelectionSetNode, + FieldsAndFragmentSpreads + >, comparedFragmentPairs: PairSet, areMutuallyExclusive: boolean, fieldMap: NodeAndDefCollection, - fragmentName: string, + fragmentSpread: FragmentSpreadNode, ): void { - const fragment = context.getFragment(fragmentName); - if (!fragment) { + const fragmentName = fragmentSpread.name.value; + const fragmentDef = context.getFragment(fragmentName); + if (!fragmentDef) { return; } - const [fieldMap2, referencedFragmentNames] = - getReferencedFieldsAndFragmentNames( + const fragmentKey = keyForFragmentSpread(fragmentSpread); + const [fieldMap2, referencedFragmentSpreads] = + getReferencedFieldsAndFragmentSpreads( context, - cachedFieldsAndFragmentNames, - fragment, + cachedFieldsAndFragmentSpreads, + fragmentDef, + fragmentSpread, ); // Do not compare a fragment's fieldMap to itself. @@ -261,7 +309,7 @@ function collectConflictsBetweenFieldsAndFragment( collectConflictsBetween( context, conflicts, - cachedFieldsAndFragmentNames, + cachedFieldsAndFragmentSpreads, comparedFragmentPairs, areMutuallyExclusive, fieldMap, @@ -270,31 +318,34 @@ function collectConflictsBetweenFieldsAndFragment( // (E) Then collect any conflicts between the provided collection of fields // and any fragment names found in the given fragment. - for (const referencedFragmentName of referencedFragmentNames) { + for (const [ + referencedFragmentKey, + referencedFragmentSpread, + ] of Object.entries(referencedFragmentSpreads)) { // Memoize so two fragments are not compared for conflicts more than once. if ( comparedFragmentPairs.has( - referencedFragmentName, - fragmentName, + referencedFragmentKey, + fragmentKey, areMutuallyExclusive, ) ) { continue; } comparedFragmentPairs.add( - referencedFragmentName, - fragmentName, + referencedFragmentKey, + fragmentKey, areMutuallyExclusive, ); collectConflictsBetweenFieldsAndFragment( context, conflicts, - cachedFieldsAndFragmentNames, + cachedFieldsAndFragmentSpreads, comparedFragmentPairs, areMutuallyExclusive, fieldMap, - referencedFragmentName, + referencedFragmentSpread, ); } } @@ -304,46 +355,64 @@ function collectConflictsBetweenFieldsAndFragment( function collectConflictsBetweenFragments( context: ValidationContext, conflicts: Array, - cachedFieldsAndFragmentNames: Map, + cachedFieldsAndFragmentSpreads: Map< + SelectionSetNode, + FieldsAndFragmentSpreads + >, comparedFragmentPairs: PairSet, areMutuallyExclusive: boolean, - fragmentName1: string, - fragmentName2: string, + fragmentSpread1: FragmentSpreadNode, + fragmentSpread2: FragmentSpreadNode, ): void { + const fragmentKey1 = keyForFragmentSpread(fragmentSpread1); + const fragmentKey2 = keyForFragmentSpread(fragmentSpread2); // No need to compare a fragment to itself. - if (fragmentName1 === fragmentName2) { + if (fragmentKey1 === fragmentKey2) { return; } // Memoize so two fragments are not compared for conflicts more than once. if ( - comparedFragmentPairs.has( - fragmentName1, - fragmentName2, - areMutuallyExclusive, - ) + comparedFragmentPairs.has(fragmentKey1, fragmentKey2, areMutuallyExclusive) ) { return; } - comparedFragmentPairs.add(fragmentName1, fragmentName2, areMutuallyExclusive); + comparedFragmentPairs.add(fragmentKey1, fragmentKey2, areMutuallyExclusive); + + // Two unique fragment spreads reference the same fragment, + // which is a conflict + if (fragmentSpread1.name.value === fragmentSpread2.name.value) { + conflicts.push({ + reason: { + kind: 'FRAGMENT_SPREAD', + fragmentName: fragmentSpread1.name.value, + reasonMessage: `${fragmentKey1} and ${fragmentKey2} have different fragment arguments`, + }, + selectionPath1: [fragmentSpread1], + selectionPath2: [fragmentSpread2], + }); + return; + } - const fragment1 = context.getFragment(fragmentName1); - const fragment2 = context.getFragment(fragmentName2); - if (!fragment1 || !fragment2) { + const fragmentDef1 = context.getFragment(fragmentSpread1.name.value); + const fragmentDef2 = context.getFragment(fragmentSpread2.name.value); + if (!fragmentDef1 || !fragmentDef2) { return; } - const [fieldMap1, referencedFragmentNames1] = - getReferencedFieldsAndFragmentNames( + const [fieldMap1, referencedFragmentSpreads1] = + getReferencedFieldsAndFragmentSpreads( context, - cachedFieldsAndFragmentNames, - fragment1, + cachedFieldsAndFragmentSpreads, + fragmentDef1, + fragmentSpread1, ); - const [fieldMap2, referencedFragmentNames2] = - getReferencedFieldsAndFragmentNames( + const [fieldMap2, referencedFragmentSpreads2] = + getReferencedFieldsAndFragmentSpreads( context, - cachedFieldsAndFragmentNames, - fragment2, + cachedFieldsAndFragmentSpreads, + fragmentDef2, + fragmentSpread2, ); // (F) First, collect all conflicts between these two collections of fields @@ -351,7 +420,7 @@ function collectConflictsBetweenFragments( collectConflictsBetween( context, conflicts, - cachedFieldsAndFragmentNames, + cachedFieldsAndFragmentSpreads, comparedFragmentPairs, areMutuallyExclusive, fieldMap1, @@ -360,29 +429,33 @@ function collectConflictsBetweenFragments( // (G) Then collect conflicts between the first fragment and any nested // fragments spread in the second fragment. - for (const referencedFragmentName2 of referencedFragmentNames2) { + for (const referencedFragmentSpread2 of Object.values( + referencedFragmentSpreads2, + )) { collectConflictsBetweenFragments( context, conflicts, - cachedFieldsAndFragmentNames, + cachedFieldsAndFragmentSpreads, comparedFragmentPairs, areMutuallyExclusive, - fragmentName1, - referencedFragmentName2, + fragmentSpread1, + referencedFragmentSpread2, ); } // (G) Then collect conflicts between the second fragment and any nested // fragments spread in the first fragment. - for (const referencedFragmentName1 of referencedFragmentNames1) { + for (const referencedFragmentSpread1 of Object.values( + referencedFragmentSpreads1, + )) { collectConflictsBetweenFragments( context, conflicts, - cachedFieldsAndFragmentNames, + cachedFieldsAndFragmentSpreads, comparedFragmentPairs, areMutuallyExclusive, - referencedFragmentName1, - fragmentName2, + referencedFragmentSpread1, + fragmentSpread2, ); } } @@ -392,7 +465,10 @@ function collectConflictsBetweenFragments( // between the sub-fields of two overlapping fields. function findConflictsBetweenSubSelectionSets( context: ValidationContext, - cachedFieldsAndFragmentNames: Map, + cachedFieldsAndFragmentSpreads: Map< + SelectionSetNode, + FieldsAndFragmentSpreads + >, comparedFragmentPairs: PairSet, areMutuallyExclusive: boolean, parentType1: Maybe, @@ -402,15 +478,15 @@ function findConflictsBetweenSubSelectionSets( ): Array { const conflicts: Array = []; - const [fieldMap1, fragmentNames1] = getFieldsAndFragmentNames( + const [fieldMap1, fragmentSpreads1] = getFieldsAndFragmentSpreads( context, - cachedFieldsAndFragmentNames, + cachedFieldsAndFragmentSpreads, parentType1, selectionSet1, ); - const [fieldMap2, fragmentNames2] = getFieldsAndFragmentNames( + const [fieldMap2, fragmentSpreads2] = getFieldsAndFragmentSpreads( context, - cachedFieldsAndFragmentNames, + cachedFieldsAndFragmentSpreads, parentType2, selectionSet2, ); @@ -419,54 +495,56 @@ function findConflictsBetweenSubSelectionSets( collectConflictsBetween( context, conflicts, - cachedFieldsAndFragmentNames, + cachedFieldsAndFragmentSpreads, comparedFragmentPairs, areMutuallyExclusive, fieldMap1, fieldMap2, ); + // (H.1) Collect all conflicts between the two sets of fragment spreads + // (I) Then collect conflicts between the first collection of fields and - // those referenced by each fragment name associated with the second. - for (const fragmentName2 of fragmentNames2) { + // those referenced by each fragment spread associated with the second. + for (const fragmentSpread2 of Object.values(fragmentSpreads2)) { collectConflictsBetweenFieldsAndFragment( context, conflicts, - cachedFieldsAndFragmentNames, + cachedFieldsAndFragmentSpreads, comparedFragmentPairs, areMutuallyExclusive, fieldMap1, - fragmentName2, + fragmentSpread2, ); } // (I) Then collect conflicts between the second collection of fields and - // those referenced by each fragment name associated with the first. - for (const fragmentName1 of fragmentNames1) { + // those referenced by each fragment spread associated with the first. + for (const fragmentSpread1 of Object.values(fragmentSpreads1)) { collectConflictsBetweenFieldsAndFragment( context, conflicts, - cachedFieldsAndFragmentNames, + cachedFieldsAndFragmentSpreads, comparedFragmentPairs, areMutuallyExclusive, fieldMap2, - fragmentName1, + fragmentSpread1, ); } - // (J) Also collect conflicts between any fragment names by the first and - // fragment names by the second. This compares each item in the first set of - // names to each item in the second set of names. - for (const fragmentName1 of fragmentNames1) { - for (const fragmentName2 of fragmentNames2) { + // (J) Also collect conflicts between any fragment spreads by the first and + // fragment spreads by the second. This compares each item in the first set of + // spreads to each item in the second set of spreads. + for (const fragmentSpread1 of Object.values(fragmentSpreads1)) { + for (const fragmentSpread2 of Object.values(fragmentSpreads2)) { collectConflictsBetweenFragments( context, conflicts, - cachedFieldsAndFragmentNames, + cachedFieldsAndFragmentSpreads, comparedFragmentPairs, areMutuallyExclusive, - fragmentName1, - fragmentName2, + fragmentSpread1, + fragmentSpread2, ); } } @@ -477,7 +555,10 @@ function findConflictsBetweenSubSelectionSets( function collectConflictsWithin( context: ValidationContext, conflicts: Array, - cachedFieldsAndFragmentNames: Map, + cachedFieldsAndFragmentSpreads: Map< + SelectionSetNode, + FieldsAndFragmentSpreads + >, comparedFragmentPairs: PairSet, fieldMap: NodeAndDefCollection, ): void { @@ -492,9 +573,9 @@ function collectConflictsWithin( if (fields.length > 1) { for (let i = 0; i < fields.length; i++) { for (let j = i + 1; j < fields.length; j++) { - const conflict = findConflict( + const conflict = findFieldConflict( context, - cachedFieldsAndFragmentNames, + cachedFieldsAndFragmentSpreads, comparedFragmentPairs, false, // within one collection is never mutually exclusive responseName, @@ -518,7 +599,10 @@ function collectConflictsWithin( function collectConflictsBetween( context: ValidationContext, conflicts: Array, - cachedFieldsAndFragmentNames: Map, + cachedFieldsAndFragmentSpreads: Map< + SelectionSetNode, + FieldsAndFragmentSpreads + >, comparedFragmentPairs: PairSet, parentFieldsAreMutuallyExclusive: boolean, fieldMap1: NodeAndDefCollection, @@ -534,9 +618,9 @@ function collectConflictsBetween( if (fields2) { for (const field1 of fields1) { for (const field2 of fields2) { - const conflict = findConflict( + const conflict = findFieldConflict( context, - cachedFieldsAndFragmentNames, + cachedFieldsAndFragmentSpreads, comparedFragmentPairs, parentFieldsAreMutuallyExclusive, responseName, @@ -554,9 +638,12 @@ function collectConflictsBetween( // Determines if there is a conflict between two particular fields, including // comparing their sub-fields. -function findConflict( +function findFieldConflict( context: ValidationContext, - cachedFieldsAndFragmentNames: Map, + cachedFieldsAndFragmentSpreads: Map< + SelectionSetNode, + FieldsAndFragmentSpreads + >, comparedFragmentPairs: PairSet, parentFieldsAreMutuallyExclusive: boolean, responseName: string, @@ -585,20 +672,28 @@ function findConflict( const name1 = node1.name.value; const name2 = node2.name.value; if (name1 !== name2) { - return [ - [responseName, `"${name1}" and "${name2}" are different fields`], - [node1], - [node2], - ]; + return { + reason: { + kind: 'FIELD', + responseName, + reasonMessage: `"${name1}" and "${name2}" are different fields`, + }, + selectionPath1: [node1], + selectionPath2: [node2], + }; } // Two field calls must have the same arguments. if (stringifyArguments(node1) !== stringifyArguments(node2)) { - return [ - [responseName, 'they have differing arguments'], - [node1], - [node2], - ]; + return { + reason: { + kind: 'FIELD', + responseName, + reasonMessage: 'they have differing arguments', + }, + selectionPath1: [node1], + selectionPath2: [node2], + }; } } @@ -606,11 +701,15 @@ function findConflict( const directives1 = /* c8 ignore next */ node1.directives ?? []; const directives2 = /* c8 ignore next */ node2.directives ?? []; if (!sameStreams(directives1, directives2)) { - return [ - [responseName, 'they have differing stream directives'], - [node1], - [node2], - ]; + return { + reason: { + kind: 'FIELD', + responseName, + reasonMessage: 'they have differing stream directives', + }, + selectionPath1: [node1], + selectionPath2: [node2], + }; } // The return type for each field. @@ -618,16 +717,17 @@ function findConflict( const type2 = def2?.type; if (type1 && type2 && doTypesConflict(type1, type2)) { - return [ - [ + return { + reason: { + kind: 'FIELD', responseName, - `they return conflicting types "${inspect(type1)}" and "${inspect( - type2, - )}"`, - ], - [node1], - [node2], - ]; + reasonMessage: `they return conflicting types "${inspect( + type1, + )}" and "${inspect(type2)}"`, + }, + selectionPath1: [node1], + selectionPath2: [node2], + }; } // Collect and compare sub-fields. Use the same "visited fragment names" list @@ -638,7 +738,7 @@ function findConflict( if (selectionSet1 && selectionSet2) { const conflicts = findConflictsBetweenSubSelectionSets( context, - cachedFieldsAndFragmentNames, + cachedFieldsAndFragmentSpreads, comparedFragmentPairs, areMutuallyExclusive, getNamedType(type1), @@ -720,58 +820,73 @@ function doTypesConflict( // Given a selection set, return the collection of fields (a mapping of response // name to field nodes and definitions) as well as a list of fragment names // referenced via fragment spreads. -function getFieldsAndFragmentNames( +function getFieldsAndFragmentSpreads( context: ValidationContext, - cachedFieldsAndFragmentNames: Map, + cachedFieldsAndFragmentSpreads: Map< + SelectionSetNode, + FieldsAndFragmentSpreads + >, parentType: Maybe, selectionSet: SelectionSetNode, -): FieldsAndFragmentNames { - const cached = cachedFieldsAndFragmentNames.get(selectionSet); +): FieldsAndFragmentSpreads { + const cached = cachedFieldsAndFragmentSpreads.get(selectionSet); if (cached) { return cached; } const nodeAndDefs: NodeAndDefCollection = Object.create(null); - const fragmentNames = new Set(); - _collectFieldsAndFragmentNames( + const fragmentSpreads: ObjMap = {}; + _collectFieldsAndFragmentSpreads( context, parentType, selectionSet, nodeAndDefs, - fragmentNames, + fragmentSpreads, ); - const result = [nodeAndDefs, [...fragmentNames]] as const; - cachedFieldsAndFragmentNames.set(selectionSet, result); + const result = [nodeAndDefs, { ...fragmentSpreads }] as const; + cachedFieldsAndFragmentSpreads.set(selectionSet, result); return result; } // Given a reference to a fragment, return the represented collection of fields -// as well as a list of nested fragment names referenced via fragment spreads. -function getReferencedFieldsAndFragmentNames( +// as well as a list of nested referenced fragment spreads. +function getReferencedFieldsAndFragmentSpreads( context: ValidationContext, - cachedFieldsAndFragmentNames: Map, - fragment: FragmentDefinitionNode, + cachedFieldsAndFragmentSpreads: Map< + SelectionSetNode, + FieldsAndFragmentSpreads + >, + fragmentDef: FragmentDefinitionNode, + fragmentSpread: FragmentSpreadNode, ) { + const fragmentSelectionSet = substituteFragmentArguments( + fragmentDef, + fragmentSpread, + ); + // Short-circuit building a type from the node if possible. - const cached = cachedFieldsAndFragmentNames.get(fragment.selectionSet); + const cached = cachedFieldsAndFragmentSpreads.get(fragmentSelectionSet); if (cached) { return cached; } - const fragmentType = typeFromAST(context.getSchema(), fragment.typeCondition); - return getFieldsAndFragmentNames( + const fragmentType = typeFromAST( + context.getSchema(), + fragmentDef.typeCondition, + ); + return getFieldsAndFragmentSpreads( context, - cachedFieldsAndFragmentNames, + cachedFieldsAndFragmentSpreads, fragmentType, - fragment.selectionSet, + fragmentSelectionSet, ); } -function _collectFieldsAndFragmentNames( +function _collectFieldsAndFragmentSpreads( context: ValidationContext, parentType: Maybe, selectionSet: SelectionSetNode, nodeAndDefs: NodeAndDefCollection, - fragmentNames: Set, + fragmentSpreads: FragmentSpreadsByKey, ): void { for (const selection of selectionSet.selections) { switch (selection.kind) { @@ -791,19 +906,19 @@ function _collectFieldsAndFragmentNames( break; } case Kind.FRAGMENT_SPREAD: - fragmentNames.add(selection.name.value); + fragmentSpreads[keyForFragmentSpread(selection)] = selection; break; case Kind.INLINE_FRAGMENT: { const typeCondition = selection.typeCondition; const inlineFragmentType = typeCondition ? typeFromAST(context.getSchema(), typeCondition) : parentType; - _collectFieldsAndFragmentNames( + _collectFieldsAndFragmentSpreads( context, inlineFragmentType, selection.selectionSet, nodeAndDefs, - fragmentNames, + fragmentSpreads, ); break; } @@ -820,11 +935,21 @@ function subfieldConflicts( node2: FieldNode, ): Maybe { if (conflicts.length > 0) { - return [ - [responseName, conflicts.map(([reason]) => reason)], - [node1, ...conflicts.map(([, fields1]) => fields1).flat()], - [node2, ...conflicts.map(([, , fields2]) => fields2).flat()], - ]; + return { + reason: { + kind: 'FIELD', + responseName, + reasonMessage: conflicts.map((conflict) => conflict.reason), + }, + selectionPath1: [ + node1, + ...conflicts.map((subConflict) => subConflict.selectionPath1).flat(), + ], + selectionPath2: [ + node2, + ...conflicts.map((subConflict) => subConflict.selectionPath2).flat(), + ], + }; } } diff --git a/src/validation/rules/ProvidedRequiredArgumentsRule.ts b/src/validation/rules/ProvidedRequiredArgumentsRule.ts index 350264496f..348504676c 100644 --- a/src/validation/rules/ProvidedRequiredArgumentsRule.ts +++ b/src/validation/rules/ProvidedRequiredArgumentsRule.ts @@ -4,7 +4,10 @@ import type { ObjMap } from '../../jsutils/ObjMap.js'; import { GraphQLError } from '../../error/GraphQLError.js'; -import type { InputValueDefinitionNode } from '../../language/ast.js'; +import type { + InputValueDefinitionNode, + VariableDefinitionNode, +} from '../../language/ast.js'; import { Kind } from '../../language/kinds.js'; import { print } from '../../language/printer.js'; import type { ASTVisitor } from '../../language/visitor.js'; @@ -56,6 +59,37 @@ export function ProvidedRequiredArgumentsRule( } }, }, + FragmentSpread: { + // Validate on leave to allow for directive errors to appear first. + leave(spreadNode) { + const fragmentDef = context.getFragment(spreadNode.name.value); + if (!fragmentDef) { + return false; + } + + const providedArgs = new Set( + // FIXME: https://github.com/graphql/graphql-js/issues/2203 + /* c8 ignore next */ + spreadNode.arguments?.map((arg) => arg.name.value), + ); + // FIXME: https://github.com/graphql/graphql-js/issues/2203 + /* c8 ignore next */ + for (const varDef of fragmentDef.variableDefinitions ?? []) { + if ( + !providedArgs.has(varDef.variable.name.value) && + isRequiredArgumentNode(varDef) + ) { + const argTypeStr = inspect(varDef.type); + context.reportError( + new GraphQLError( + `Fragment "${spreadNode.name.value}" argument "${varDef.variable.name.value}" of type "${argTypeStr}" is required, but it was not provided.`, + { nodes: [spreadNode, varDef] }, + ), + ); + } + } + }, + }, }; } @@ -122,6 +156,8 @@ export function ProvidedRequiredArgumentsOnDirectivesRule( }; } -function isRequiredArgumentNode(arg: InputValueDefinitionNode): boolean { +function isRequiredArgumentNode( + arg: InputValueDefinitionNode | VariableDefinitionNode, +): boolean { return arg.type.kind === Kind.NON_NULL_TYPE && arg.defaultValue == null; } diff --git a/src/validation/rules/VariablesInAllowedPositionRule.ts b/src/validation/rules/VariablesInAllowedPositionRule.ts index 4039540eba..52cb2fbb69 100644 --- a/src/validation/rules/VariablesInAllowedPositionRule.ts +++ b/src/validation/rules/VariablesInAllowedPositionRule.ts @@ -1,9 +1,10 @@ import { inspect } from '../../jsutils/inspect.js'; import type { Maybe } from '../../jsutils/Maybe.js'; +import type { ObjMap } from '../../jsutils/ObjMap.js'; import { GraphQLError } from '../../error/GraphQLError.js'; -import type { ValueNode } from '../../language/ast.js'; +import type { ValueNode, VariableDefinitionNode } from '../../language/ast.js'; import { Kind } from '../../language/kinds.js'; import type { ASTVisitor } from '../../language/visitor.js'; @@ -26,7 +27,7 @@ import type { ValidationContext } from '../ValidationContext.js'; export function VariablesInAllowedPositionRule( context: ValidationContext, ): ASTVisitor { - let varDefMap = Object.create(null); + let varDefMap: ObjMap = Object.create(null); return { OperationDefinition: { @@ -36,9 +37,9 @@ export function VariablesInAllowedPositionRule( leave(operation) { const usages = context.getRecursiveVariableUsages(operation); - for (const { node, type, defaultValue } of usages) { + for (const { node, type, defaultValue, fragmentVarDef } of usages) { const varName = node.name.value; - const varDef = varDefMap[varName]; + const varDef = fragmentVarDef ?? varDefMap[varName]; if (varDef && type) { // A var type is allowed if it is the same or more strict (e.g. is // a subtype of) than the expected type. It can be more strict if diff --git a/src/validation/specifiedRules.ts b/src/validation/specifiedRules.ts index 60c967f8f0..59d81e3a83 100644 --- a/src/validation/specifiedRules.ts +++ b/src/validation/specifiedRules.ts @@ -31,6 +31,8 @@ import { NoFragmentCyclesRule } from './rules/NoFragmentCyclesRule.js'; import { NoUndefinedVariablesRule } from './rules/NoUndefinedVariablesRule.js'; // Spec Section: "Fragments must be used" import { NoUnusedFragmentsRule } from './rules/NoUnusedFragmentsRule.js'; +// Spec Section: "All Fragment Variables Used" +import { NoUnusedFragmentVariablesRule } from './rules/NoUnusedFragmentVariablesRule.js'; // Spec Section: "All Variables Used" import { NoUnusedVariablesRule } from './rules/NoUnusedVariablesRule.js'; // Spec Section: "Field Selection Merging" @@ -99,6 +101,7 @@ export const specifiedRules: ReadonlyArray = Object.freeze([ UniqueVariableNamesRule, NoUndefinedVariablesRule, NoUnusedVariablesRule, + NoUnusedFragmentVariablesRule, KnownDirectivesRule, UniqueDirectivesPerLocationRule, DeferStreamDirectiveOnRootFieldRule, From bfd80a6dcbcf974c18f757df848d854c43b8a9db Mon Sep 17 00:00:00 2001 From: Matt Mahoney Date: Wed, 8 Feb 2023 19:52:25 -0500 Subject: [PATCH 3/4] Add flag for parser --- src/__testUtils__/kitchenSinkQuery.ts | 2 +- src/execution/__tests__/variables-test.ts | 2 +- src/utilities/__tests__/TypeInfo-test.ts | 23 +++++++++++++---------- 3 files changed, 15 insertions(+), 12 deletions(-) diff --git a/src/__testUtils__/kitchenSinkQuery.ts b/src/__testUtils__/kitchenSinkQuery.ts index ff989d4b46..46628b6556 100644 --- a/src/__testUtils__/kitchenSinkQuery.ts +++ b/src/__testUtils__/kitchenSinkQuery.ts @@ -10,7 +10,7 @@ query queryName($foo: ComplexType, $site: Site = MOBILE) @onQuery { ...frag @onFragmentSpread } } - + field3! field4? requiredField5: field5! diff --git a/src/execution/__tests__/variables-test.ts b/src/execution/__tests__/variables-test.ts index 10d88b8a5c..b21c1ed9b6 100644 --- a/src/execution/__tests__/variables-test.ts +++ b/src/execution/__tests__/variables-test.ts @@ -160,7 +160,7 @@ function executeQuery( query: string, variableValues?: { [variable: string]: unknown }, ) { - const document = parse(query); + const document = parse(query, { experimentalFragmentArguments: true }); return executeSync({ schema, document, variableValues }); } diff --git a/src/utilities/__tests__/TypeInfo-test.ts b/src/utilities/__tests__/TypeInfo-test.ts index 00053fe669..552b57938b 100644 --- a/src/utilities/__tests__/TypeInfo-test.ts +++ b/src/utilities/__tests__/TypeInfo-test.ts @@ -519,17 +519,20 @@ describe('visitWithTypeInfo', () => { it('supports traversals of fragment arguments', () => { const typeInfo = new TypeInfo(testSchema); - const ast = parse(` - query { - ...Foo(x: 4) - } + const ast = parse( + ` + query { + ...Foo(x: 4) + } - fragment Foo( - $x: ID! - ) on QueryRoot { - human(id: $x) { name } - } - `); + fragment Foo( + $x: ID! + ) on QueryRoot { + human(id: $x) { name } + } + `, + { experimentalFragmentArguments: true }, + ); const visited: Array = []; visit( From 7fe0733aa2782a0def998365678e11b2876e2187 Mon Sep 17 00:00:00 2001 From: Matt Mahoney Date: Thu, 9 Feb 2023 17:07:14 -0500 Subject: [PATCH 4/4] Split PRs, no functional changes --- src/__testUtils__/kitchenSinkQuery.ts | 2 +- src/language/kinds.ts | 1 - src/validation/__tests__/KnownDirectivesRule-test.ts | 1 - 3 files changed, 1 insertion(+), 3 deletions(-) diff --git a/src/__testUtils__/kitchenSinkQuery.ts b/src/__testUtils__/kitchenSinkQuery.ts index 46628b6556..ff989d4b46 100644 --- a/src/__testUtils__/kitchenSinkQuery.ts +++ b/src/__testUtils__/kitchenSinkQuery.ts @@ -10,7 +10,7 @@ query queryName($foo: ComplexType, $site: Site = MOBILE) @onQuery { ...frag @onFragmentSpread } } - + field3! field4? requiredField5: field5! diff --git a/src/language/kinds.ts b/src/language/kinds.ts index 8d9098ed4e..d606c12cbe 100644 --- a/src/language/kinds.ts +++ b/src/language/kinds.ts @@ -22,7 +22,6 @@ export enum Kind { FRAGMENT_SPREAD = 'FragmentSpread', INLINE_FRAGMENT = 'InlineFragment', FRAGMENT_DEFINITION = 'FragmentDefinition', - FRAGMENT_ARGUMENT_DEFINITION = 'FragmentArgumentDefinition', /** Values */ VARIABLE = 'Variable', diff --git a/src/validation/__tests__/KnownDirectivesRule-test.ts b/src/validation/__tests__/KnownDirectivesRule-test.ts index cbcace2ca0..c27155c9b2 100644 --- a/src/validation/__tests__/KnownDirectivesRule-test.ts +++ b/src/validation/__tests__/KnownDirectivesRule-test.ts @@ -41,7 +41,6 @@ const schemaWithDirectives = buildSchema(` directive @onSubscription on SUBSCRIPTION directive @onField on FIELD directive @onFragmentDefinition on FRAGMENT_DEFINITION - directive @onFragmentVariableDefinition on VARIABLE_DEFINITION directive @onFragmentSpread on FRAGMENT_SPREAD directive @onFragmentVariableDefinition on FRAGMENT_VARIABLE_DEFINITION directive @onInlineFragment on INLINE_FRAGMENT