diff --git a/src/execution/__tests__/variables-test.ts b/src/execution/__tests__/variables-test.ts index 69b9e93fd9..04aaf5971b 100644 --- a/src/execution/__tests__/variables-test.ts +++ b/src/execution/__tests__/variables-test.ts @@ -1201,7 +1201,7 @@ describe('Execute: Handles inputs', () => { expect(result).to.have.property('errors'); expect(result.errors).to.have.length(1); - expect(result.errors?.[0]?.message).to.match( + expect(result.errors?.[0].message).to.match( /Argument "value" of required type "String!"/, ); }); @@ -1269,7 +1269,7 @@ describe('Execute: Handles inputs', () => { expect(result).to.have.property('errors'); expect(result.errors).to.have.length(1); - expect(result.errors?.[0]?.message).to.match( + expect(result.errors?.[0].message).to.match( /Argument "value" of non-null type "String!"/, ); }); @@ -1307,6 +1307,22 @@ describe('Execute: Handles inputs', () => { }); }); + it('when a nullable argument without a field default is not provided and shadowed by an operation variable', () => { + const result = executeQueryWithFragmentArguments(` + query($x: String = "A") { + ...a + } + fragment a($x: String) on TestType { + fieldWithNullableStringInput(input: $x) + } + `); + expect(result).to.deep.equal({ + data: { + fieldWithNullableStringInput: null, + }, + }); + }); + it('when a nullable argument with a field default is not provided and shadowed by an operation variable', () => { const result = executeQueryWithFragmentArguments(` query($x: String = "A") { @@ -1411,6 +1427,27 @@ describe('Execute: Handles inputs', () => { }); }); + it('when argument variables with the same name are used directly and recursively', () => { + const result = executeQueryWithFragmentArguments(` + query { + ...a(value: "A") + } + fragment a($value: String!) on TestType { + ...b(value: "B") + fieldInFragmentA: fieldWithNonNullableStringInput(input: $value) + } + fragment b($value: String!) on TestType { + fieldInFragmentB: fieldWithNonNullableStringInput(input: $value) + } + `); + expect(result).to.deep.equal({ + data: { + fieldInFragmentA: '"A"', + fieldInFragmentB: '"B"', + }, + }); + }); + it('when argument passed in as list', () => { const result = executeQueryWithFragmentArguments(` query Q($opValue: String = "op") { @@ -1433,5 +1470,33 @@ describe('Execute: Handles inputs', () => { }, }); }); + + it('when argument passed to a directive', () => { + const result = executeQueryWithFragmentArguments(` + query { + ...a(value: true) + } + fragment a($value: Boolean!) on TestType { + fieldWithNonNullableStringInput @skip(if: $value) + } + `); + expect(result).to.deep.equal({ + data: {}, + }); + }); + + it('when argument passed to a directive on a nested field', () => { + const result = executeQueryWithFragmentArguments(` + query { + ...a(value: true) + } + fragment a($value: Boolean!) on TestType { + nested { echo(input: "echo") @skip(if: $value) } + } + `); + expect(result).to.deep.equal({ + data: { nested: {} }, + }); + }); }); }); diff --git a/src/execution/collectFields.ts b/src/execution/collectFields.ts index 004a0337aa..04fd1fa510 100644 --- a/src/execution/collectFields.ts +++ b/src/execution/collectFields.ts @@ -17,13 +17,24 @@ import { } from '../type/directives'; import type { GraphQLSchema } from '../type/schema'; +import type { GraphQLVariableSignature } from '../utilities/getVariableSignature'; import { typeFromAST } from '../utilities/typeFromAST'; -import { getArgumentValuesFromSpread, getDirectiveValues } from './values'; +import { experimentalGetArgumentValues, getDirectiveValues } from './values'; + +export interface FragmentVariables { + signatures: ObjMap; + values: ObjMap; +} export interface FieldDetails { node: FieldNode; - fragmentVariableValues?: ObjMap | undefined; + fragmentVariables?: FragmentVariables | undefined; +} + +export interface FragmentDetails { + definition: FragmentDefinitionNode; + variableSignatures?: ObjMap | undefined; } /** @@ -37,7 +48,7 @@ export interface FieldDetails { */ export function collectFields( schema: GraphQLSchema, - fragments: ObjMap, + fragments: ObjMap, variableValues: { [variable: string]: unknown }, runtimeType: GraphQLObjectType, selectionSet: SelectionSetNode, @@ -68,7 +79,7 @@ export function collectFields( */ export function collectSubfields( schema: GraphQLSchema, - fragments: ObjMap, + fragments: ObjMap, variableValues: { [variable: string]: unknown }, returnType: GraphQLObjectType, fieldEntries: ReadonlyArray, @@ -85,6 +96,7 @@ export function collectSubfields( entry.node.selectionSet, subFieldEntries, visitedFragmentNames, + entry.fragmentVariables, ); } } @@ -93,19 +105,18 @@ export function collectSubfields( function collectFieldsImpl( schema: GraphQLSchema, - fragments: ObjMap, + fragments: ObjMap, variableValues: { [variable: string]: unknown }, runtimeType: GraphQLObjectType, selectionSet: SelectionSetNode, fields: Map>, visitedFragmentNames: Set, - localVariableValues?: { [variable: string]: unknown }, + fragmentVariables?: FragmentVariables, ): void { for (const selection of selectionSet.selections) { switch (selection.kind) { case Kind.FIELD: { - const vars = localVariableValues ?? variableValues; - if (!shouldIncludeNode(vars, selection)) { + if (!shouldIncludeNode(selection, variableValues, fragmentVariables)) { continue; } const name = getFieldEntryKey(selection); @@ -113,13 +124,13 @@ function collectFieldsImpl( if (fieldList !== undefined) { fieldList.push({ node: selection, - fragmentVariableValues: localVariableValues ?? undefined, + fragmentVariables, }); } else { fields.set(name, [ { node: selection, - fragmentVariableValues: localVariableValues ?? undefined, + fragmentVariables, }, ]); } @@ -127,7 +138,7 @@ function collectFieldsImpl( } case Kind.INLINE_FRAGMENT: { if ( - !shouldIncludeNode(variableValues, selection) || + !shouldIncludeNode(selection, variableValues, fragmentVariables) || !doesFragmentConditionMatch(schema, selection, runtimeType) ) { continue; @@ -140,6 +151,7 @@ function collectFieldsImpl( selection.selectionSet, fields, visitedFragmentNames, + fragmentVariables, ); break; } @@ -147,7 +159,7 @@ function collectFieldsImpl( const fragName = selection.name.value; if ( visitedFragmentNames.has(fragName) || - !shouldIncludeNode(variableValues, selection) + !shouldIncludeNode(selection, variableValues, fragmentVariables) ) { continue; } @@ -155,39 +167,34 @@ function collectFieldsImpl( const fragment = fragments[fragName]; if ( !fragment || - !doesFragmentConditionMatch(schema, fragment, runtimeType) + !doesFragmentConditionMatch(schema, fragment.definition, runtimeType) ) { continue; } - // We need to introduce a concept of shadowing: - // - // - when a fragment defines a variable that is in the parent scope but not given - // in the fragment-spread we need to look at this variable as undefined and check - // whether the definition has a defaultValue, if not remove it from the variableValues. - // - when a fragment does not define a variable we need to copy it over from the parent - // scope as that variable can still get used in spreads later on in the selectionSet. - // - when a value is passed in through the fragment-spread we need to copy over the key-value - // into our variable-values. - const fragmentVariableValues = fragment.variableDefinitions - ? getArgumentValuesFromSpread( + const fragmentVariableSignatures = fragment.variableSignatures; + let newFragmentVariables: FragmentVariables | undefined; + if (fragmentVariableSignatures) { + newFragmentVariables = { + signatures: fragmentVariableSignatures, + values: experimentalGetArgumentValues( selection, - schema, - fragment.variableDefinitions, + Object.values(fragmentVariableSignatures), variableValues, - localVariableValues, - ) - : undefined; + fragmentVariables, + ), + }; + } collectFieldsImpl( schema, fragments, variableValues, runtimeType, - fragment.selectionSet, + fragment.definition.selectionSet, fields, visitedFragmentNames, - fragmentVariableValues, + newFragmentVariables, ); break; } @@ -200,10 +207,16 @@ function collectFieldsImpl( * directives, where `@skip` has higher precedence than `@include`. */ function shouldIncludeNode( - variableValues: { [variable: string]: unknown }, node: FragmentSpreadNode | FieldNode | InlineFragmentNode, + variableValues: { [variable: string]: unknown }, + fragmentVariables: FragmentVariables | undefined, ): boolean { - const skip = getDirectiveValues(GraphQLSkipDirective, node, variableValues); + const skip = getDirectiveValues( + GraphQLSkipDirective, + node, + variableValues, + fragmentVariables, + ); if (skip?.if === true) { return false; } @@ -212,6 +225,7 @@ function shouldIncludeNode( GraphQLIncludeDirective, node, variableValues, + fragmentVariables, ); if (include?.if === false) { return false; diff --git a/src/execution/execute.ts b/src/execution/execute.ts index d11900976b..7b5288c388 100644 --- a/src/execution/execute.ts +++ b/src/execution/execute.ts @@ -4,6 +4,7 @@ import { invariant } from '../jsutils/invariant'; import { isIterableObject } from '../jsutils/isIterableObject'; import { isObjectLike } from '../jsutils/isObjectLike'; import { isPromise } from '../jsutils/isPromise'; +import { mapValue } from '../jsutils/mapValue'; import type { Maybe } from '../jsutils/Maybe'; import { memoize3 } from '../jsutils/memoize3'; import type { ObjMap } from '../jsutils/ObjMap'; @@ -20,7 +21,6 @@ import { locatedError } from '../error/locatedError'; import type { DocumentNode, FieldNode, - FragmentDefinitionNode, OperationDefinitionNode, } from '../language/ast'; import { OperationTypeNode } from '../language/ast'; @@ -52,12 +52,14 @@ import { import type { GraphQLSchema } from '../type/schema'; import { assertValidSchema } from '../type/validate'; -import type { FieldDetails } from './collectFields'; +import { getVariableSignature } from '../utilities/getVariableSignature'; + +import type { FieldDetails, FragmentDetails } from './collectFields'; import { collectFields, collectSubfields as _collectSubfields, } from './collectFields'; -import { getArgumentValues, getVariableValues } from './values'; +import { experimentalGetArgumentValues, getVariableValues } from './values'; /** * A memoized collection of relevant subfields with regard to the return @@ -107,7 +109,7 @@ const collectSubfields = memoize3( */ export interface ExecutionContext { schema: GraphQLSchema; - fragments: ObjMap; + fragments: ObjMap; rootValue: unknown; contextValue: unknown; operation: OperationDefinitionNode; @@ -290,7 +292,7 @@ export function buildExecutionContext( } = args; let operation: OperationDefinitionNode | undefined; - const fragments: ObjMap = Object.create(null); + const fragments: ObjMap = Object.create(null); for (const definition of document.definitions) { switch (definition.kind) { case Kind.OPERATION_DEFINITION: @@ -307,9 +309,18 @@ export function buildExecutionContext( operation = definition; } break; - case Kind.FRAGMENT_DEFINITION: - fragments[definition.name.value] = definition; + case Kind.FRAGMENT_DEFINITION: { + let variableSignatures; + if (definition.variableDefinitions) { + variableSignatures = Object.create(null); + for (const varDef of definition.variableDefinitions) { + const signature = getVariableSignature(schema, varDef); + variableSignatures[signature.name] = signature; + } + } + fragments[definition.name.value] = { definition, variableSignatures }; break; + } default: // ignore non-executable definitions } @@ -519,11 +530,11 @@ function executeField( // Build a JS object of arguments from the field.arguments AST, using the // variables scope to fulfill any variable references. // TODO: find a way to memoize, in case this field is within a List type. - const args = getArgumentValues( + const args = experimentalGetArgumentValues( fieldEntries[0].node, fieldDef.args, exeContext.variableValues, - fieldEntries[0].fragmentVariableValues, + fieldEntries[0].fragmentVariables, ); // The resolve function's optional third argument is a context value that @@ -598,7 +609,10 @@ export function buildResolveInfo( parentType, path, schema: exeContext.schema, - fragments: exeContext.fragments, + fragments: mapValue( + exeContext.fragments, + (fragment) => fragment.definition, + ), rootValue: exeContext.rootValue, operation: exeContext.operation, variableValues: exeContext.variableValues, diff --git a/src/execution/subscribe.ts b/src/execution/subscribe.ts index caf3c7617f..186b3a1d9e 100644 --- a/src/execution/subscribe.ts +++ b/src/execution/subscribe.ts @@ -240,11 +240,7 @@ async function executeSubscription( // Build a JS object of arguments from the field.arguments AST, using the // variables scope to fulfill any variable references. - const args = getArgumentValues( - fieldEntries[0].node, - fieldDef.args, - variableValues, - ); + const args = getArgumentValues(fieldDef, fieldEntries[0].node, variableValues); // The resolve function's optional third argument is a context value that // is provided to every resolve function within an execution. It is commonly diff --git a/src/execution/values.ts b/src/execution/values.ts index c1050cc6ef..7500619c6c 100644 --- a/src/execution/values.ts +++ b/src/execution/values.ts @@ -1,5 +1,4 @@ import { inspect } from '../jsutils/inspect'; -import { keyMap } from '../jsutils/keyMap'; import type { Maybe } from '../jsutils/Maybe'; import type { ObjMap } from '../jsutils/ObjMap'; import { printPathArray } from '../jsutils/printPathArray'; @@ -15,15 +14,17 @@ import type { import { Kind } from '../language/kinds'; import { print } from '../language/printer'; -import type { GraphQLArgument } from '../type/definition'; -import { isInputType, isNonNullType } from '../type/definition'; +import type { GraphQLArgument, GraphQLField } from '../type/definition'; +import { isNonNullType } from '../type/definition'; import type { GraphQLDirective } from '../type/directives'; import type { GraphQLSchema } from '../type/schema'; import { coerceInputValue } from '../utilities/coerceInputValue'; -import { typeFromAST } from '../utilities/typeFromAST'; +import type { GraphQLVariableSignature } from '../utilities/getVariableSignature'; +import { getVariableSignature } from '../utilities/getVariableSignature'; import { valueFromAST } from '../utilities/valueFromAST'; -import { valueFromASTUntyped } from '../utilities/valueFromASTUntyped'; + +import type { FragmentVariables } from './collectFields'; type CoercedVariableValues = | { errors: ReadonlyArray; coerced?: never } @@ -79,24 +80,16 @@ function coerceVariableValues( ): { [variable: string]: unknown } { const coercedValues: { [variable: string]: unknown } = {}; for (const varDefNode of varDefNodes) { - const varName = varDefNode.variable.name.value; - const varType = typeFromAST(schema, varDefNode.type); - if (!isInputType(varType)) { - // Must use input types for variables. This should be caught during - // validation, however is checked again here for safety. - const varTypeStr = print(varDefNode.type); - onError( - new GraphQLError( - `Variable "$${varName}" expected value of type "${varTypeStr}" which cannot be used as an input type.`, - { nodes: varDefNode.type }, - ), - ); + const varSignature = getVariableSignature(schema, varDefNode); + if (varSignature instanceof GraphQLError) { + onError(varSignature); continue; } + const { name: varName, type: varType } = varSignature; if (!hasOwnProperty(inputs, varName)) { if (varDefNode.defaultValue) { - coercedValues[varName] = valueFromAST(varDefNode.defaultValue, varType); + coercedValues[varName] = varSignature.defaultValue; } else if (isNonNullType(varType)) { const varTypeStr = inspect(varType); onError( @@ -151,25 +144,25 @@ function coerceVariableValues( * exposed to user code. Care should be taken to not pull values from the * Object prototype. */ -export function getArgumentValues( - node: FieldNode | DirectiveNode, - argDefs: ReadonlyArray, - variableValues?: Maybe>, - fragmentArgValues?: Maybe>, +export function experimentalGetArgumentValues( + node: FieldNode | DirectiveNode | FragmentSpreadNode, + argDefs: ReadonlyArray, + variableValues: Maybe>, + fragmentVariables?: Maybe, ): { [argument: string]: unknown } { const coercedValues: { [argument: string]: unknown } = {}; // FIXME: https://github.com/graphql/graphql-js/issues/2203 /* c8 ignore next */ const argumentNodes = node.arguments ?? []; - const argNodeMap = keyMap(argumentNodes, (arg) => arg.name.value); + const argNodeMap = new Map(argumentNodes.map((arg) => [arg.name.value, arg])); for (const argDef of argDefs) { const name = argDef.name; const argType = argDef.type; - const argumentNode = argNodeMap[name]; + const argumentNode = argNodeMap.get(name); - if (!argumentNode) { + if (argumentNode == null) { if (argDef.defaultValue !== undefined) { coercedValues[name] = argDef.defaultValue; } else if (isNonNullType(argType)) { @@ -183,39 +176,32 @@ export function getArgumentValues( } const valueNode = argumentNode.value; - let hasValue = valueNode.kind !== Kind.NULL; + let isNull = valueNode.kind === Kind.NULL; if (valueNode.kind === Kind.VARIABLE) { const variableName = valueNode.name.value; + const scopedVariableValues = fragmentVariables?.signatures[variableName] + ? fragmentVariables.values + : variableValues; if ( - fragmentArgValues != null && - hasOwnProperty(fragmentArgValues, variableName) + scopedVariableValues == null || + !hasOwnProperty(scopedVariableValues, variableName) ) { - hasValue = fragmentArgValues[variableName] != null; - if (!hasValue && argDef.defaultValue !== undefined) { + if (argDef.defaultValue !== undefined) { coercedValues[name] = argDef.defaultValue; - continue; + } else if (isNonNullType(argType)) { + throw new GraphQLError( + `Argument "${name}" of required type "${inspect(argType)}" ` + + `was provided the variable "$${variableName}" which was not provided a runtime value.`, + { nodes: valueNode }, + ); } - } else if ( - variableValues != null && - hasOwnProperty(variableValues, variableName) - ) { - hasValue = variableValues[variableName] != null; - } else if (argDef.defaultValue !== undefined) { - coercedValues[name] = argDef.defaultValue; - continue; - } else if (isNonNullType(argType)) { - throw new GraphQLError( - `Argument "${name}" of required type "${inspect(argType)}" ` + - `was provided the variable "$${variableName}" which was not provided a runtime value.`, - { nodes: valueNode }, - ); - } else { continue; } + isNull = scopedVariableValues[variableName] == null; } - if (!hasValue && isNonNullType(argType)) { + if (isNull && isNonNullType(argType)) { throw new GraphQLError( `Argument "${name}" of non-null type "${inspect(argType)}" ` + 'must not be null.', @@ -223,11 +209,12 @@ export function getArgumentValues( ); } - const coercedValue = valueFromAST(valueNode, argType, { - ...variableValues, - ...fragmentArgValues, - }); - + const coercedValue = valueFromAST( + valueNode, + argType, + variableValues, + fragmentVariables?.values, + ); if (coercedValue === undefined) { // Note: ValuesOfCorrectTypeRule validation should catch this before // execution. This is a runtime check to ensure execution does not @@ -242,76 +229,6 @@ export function getArgumentValues( return coercedValues; } -export function getArgumentValuesFromSpread( - /** NOTE: For error annotations only */ - node: FragmentSpreadNode, - schema: GraphQLSchema, - fragmentVarDefs: ReadonlyArray, - variableValues: Maybe>, - fragmentArgValues?: Maybe>, -): { [argument: string]: unknown } { - const coercedValues: { [argument: string]: unknown } = {}; - const argNodeMap = keyMap(node?.arguments ?? [], (arg) => arg.name.value); - - for (const varDef of fragmentVarDefs) { - const name = varDef.variable.name.value; - const argType = typeFromAST(schema, varDef.type); - const argumentNode = argNodeMap[name]; - - if (argumentNode == null) { - if (varDef.defaultValue !== undefined) { - coercedValues[name] = valueFromASTUntyped(varDef.defaultValue); - } else if (isNonNullType(argType)) { - throw new GraphQLError( - `Argument "${name}" of required type "${inspect(argType)}" ` + - 'was not provided.', - { nodes: node }, - ); - } else { - coercedValues[name] = undefined; - } - continue; - } - - const valueNode = argumentNode.value; - - let hasValue = valueNode.kind !== Kind.NULL; - if (valueNode.kind === Kind.VARIABLE) { - const variableName = valueNode.name.value; - if ( - fragmentArgValues != null && - hasOwnProperty(fragmentArgValues, variableName) - ) { - hasValue = fragmentArgValues[variableName] != null; - } else if ( - variableValues != null && - hasOwnProperty(variableValues, variableName) - ) { - hasValue = variableValues[variableName] != null; - } - } - - if (!hasValue && isNonNullType(argType)) { - throw new GraphQLError( - `Argument "${name}" of non-null type "${inspect(argType)}" ` + - 'must not be null.', - { nodes: valueNode }, - ); - } - - let coercedValue; - if (argType && isInputType(argType)) { - coercedValue = valueFromAST(valueNode, argType, { - ...variableValues, - ...fragmentArgValues, - }); - } - - coercedValues[name] = coercedValue; - } - return coercedValues; -} - /** * Prepares an object map of argument values given a directive definition * and a AST node which may contain directives. Optionally also accepts a map @@ -323,17 +240,31 @@ export function getArgumentValuesFromSpread( * exposed to user code. Care should be taken to not pull values from the * Object prototype. */ +export function getArgumentValues( + def: GraphQLField | GraphQLDirective, + node: FieldNode | DirectiveNode, + variableValues?: Maybe>, +): { [argument: string]: unknown } { + return experimentalGetArgumentValues(node, def.args, variableValues); +} + export function getDirectiveValues( directiveDef: GraphQLDirective, node: { readonly directives?: ReadonlyArray }, variableValues?: Maybe>, + fragmentVariables?: Maybe, ): undefined | { [argument: string]: unknown } { const directiveNode = node.directives?.find( (directive) => directive.name.value === directiveDef.name, ); if (directiveNode) { - return getArgumentValues(directiveNode, directiveDef.args, variableValues); + return experimentalGetArgumentValues( + directiveNode, + directiveDef.args, + variableValues, + fragmentVariables, + ); } } diff --git a/src/index.ts b/src/index.ts index 877939d879..ae2f4215ac 100644 --- a/src/index.ts +++ b/src/index.ts @@ -263,6 +263,7 @@ export type { SelectionNode, FieldNode, ArgumentNode, + FragmentArgumentNode, ConstArgumentNode, FragmentSpreadNode, InlineFragmentNode, diff --git a/src/language/__tests__/visitor-test.ts b/src/language/__tests__/visitor-test.ts index 4b11497cea..8dab3026e7 100644 --- a/src/language/__tests__/visitor-test.ts +++ b/src/language/__tests__/visitor-test.ts @@ -1393,12 +1393,12 @@ describe('Visitor', () => { ['enter', 'FragmentSpread', undefined], ['enter', 'Name', 's'], ['leave', 'Name', 's'], - ['enter', 'Argument', { kind: 'BooleanValue', value: false }], + ['enter', 'FragmentArgument', { kind: 'BooleanValue', value: false }], ['enter', 'Name', 'v'], ['leave', 'Name', 'v'], ['enter', 'BooleanValue', false], ['leave', 'BooleanValue', false], - ['leave', 'Argument', { kind: 'BooleanValue', value: false }], + ['leave', 'FragmentArgument', { kind: 'BooleanValue', value: false }], ['leave', 'FragmentSpread', undefined], ['leave', 'SelectionSet', undefined], ['leave', 'FragmentDefinition', undefined], diff --git a/src/language/ast.ts b/src/language/ast.ts index b1e278162a..f6d1bfbcb3 100644 --- a/src/language/ast.ts +++ b/src/language/ast.ts @@ -145,6 +145,7 @@ export type ASTNode = | SelectionSetNode | FieldNode | ArgumentNode + | FragmentArgumentNode | FragmentSpreadNode | InlineFragmentNode | FragmentDefinitionNode @@ -208,6 +209,7 @@ export const QueryDocumentKeys: { SelectionSet: ['selections'], Field: ['alias', 'name', 'arguments', 'directives', 'selectionSet'], Argument: ['name', 'value'], + FragmentArgument: ['name', 'value'], FragmentSpread: ['name', 'arguments', 'directives'], InlineFragment: ['typeCondition', 'directives', 'selectionSet'], @@ -382,7 +384,7 @@ export interface FragmentSpreadNode { readonly kind: Kind.FRAGMENT_SPREAD; readonly loc?: Location; readonly name: NameNode; - readonly arguments?: ReadonlyArray; + readonly arguments?: ReadonlyArray; readonly directives?: ReadonlyArray; } @@ -501,6 +503,13 @@ export interface ConstObjectFieldNode { readonly value: ConstValueNode; } +export interface FragmentArgumentNode { + readonly kind: Kind.FRAGMENT_ARGUMENT; + readonly loc?: Location | undefined; + readonly name: NameNode; + readonly value: ValueNode; +} + /** Directives */ export interface DirectiveNode { diff --git a/src/language/index.ts b/src/language/index.ts index ec4d195e1a..79b0dc2f20 100644 --- a/src/language/index.ts +++ b/src/language/index.ts @@ -43,6 +43,7 @@ export type { SelectionNode, FieldNode, ArgumentNode, + FragmentArgumentNode /* for experimental fragment arguments */, ConstArgumentNode, FragmentSpreadNode, InlineFragmentNode, diff --git a/src/language/kinds.ts b/src/language/kinds.ts index cd05f66a3b..8e6e258801 100644 --- a/src/language/kinds.ts +++ b/src/language/kinds.ts @@ -12,6 +12,7 @@ enum Kind { SELECTION_SET = 'SelectionSet', FIELD = 'Field', ARGUMENT = 'Argument', + FRAGMENT_ARGUMENT = 'FragmentArgument', /** Fragments */ FRAGMENT_SPREAD = 'FragmentSpread', diff --git a/src/language/parser.ts b/src/language/parser.ts index d4504d2271..10dbbb04d3 100644 --- a/src/language/parser.ts +++ b/src/language/parser.ts @@ -23,6 +23,7 @@ import type { FieldDefinitionNode, FieldNode, FloatValueNode, + FragmentArgumentNode, FragmentDefinitionNode, FragmentSpreadNode, InlineFragmentNode, @@ -524,7 +525,7 @@ export class Parser { return this.node(start, { kind: Kind.FRAGMENT_SPREAD, name, - arguments: this.parseArguments(false), + arguments: this.parseFragmentArguments(), directives: this.parseDirectives(false), }); } @@ -542,6 +543,25 @@ export class Parser { }); } + /* experimental */ + parseFragmentArguments(): Array { + const item = this.parseFragmentArgument; + return this.optionalMany(TokenKind.PAREN_L, item, TokenKind.PAREN_R); + } + + /* experimental */ + parseFragmentArgument(): FragmentArgumentNode { + const start = this._lexer.token; + const name = this.parseName(); + + this.expectToken(TokenKind.COLON); + return this.node(start, { + kind: Kind.FRAGMENT_ARGUMENT, + name, + value: this.parseValueLiteral(false), + }); + } + /** * FragmentDefinition : * - fragment FragmentName VariableDefinitions? on TypeCondition Directives? SelectionSet diff --git a/src/language/printer.ts b/src/language/printer.ts index 673d3e69fc..3c89f004e0 100644 --- a/src/language/printer.ts +++ b/src/language/printer.ts @@ -66,6 +66,7 @@ const printDocASTReducer: ASTReducer = { }, Argument: { leave: ({ name, value }) => name + ': ' + value }, + FragmentArgument: { leave: ({ name, value }) => name + ': ' + value }, // Fragments diff --git a/src/type/definition.ts b/src/type/definition.ts index 7eaac560dc..a4f36e0ce7 100644 --- a/src/type/definition.ts +++ b/src/type/definition.ts @@ -40,6 +40,7 @@ import type { import { Kind } from '../language/kinds'; import { print } from '../language/printer'; +import type { GraphQLVariableSignature } from '../utilities/getVariableSignature'; import { valueFromASTUntyped } from '../utilities/valueFromASTUntyped'; import { assertEnumValueName, assertName } from './assertName'; @@ -1069,7 +1070,9 @@ export interface GraphQLArgument { astNode: Maybe; } -export function isRequiredArgument(arg: GraphQLArgument): boolean { +export function isRequiredArgument( + arg: GraphQLArgument | GraphQLVariableSignature, +): boolean { return isNonNullType(arg.type) && arg.defaultValue === undefined; } diff --git a/src/utilities/TypeInfo.ts b/src/utilities/TypeInfo.ts index 160a93fffb..55887a0ddf 100644 --- a/src/utilities/TypeInfo.ts +++ b/src/utilities/TypeInfo.ts @@ -3,9 +3,10 @@ import type { ObjMap } from '../jsutils/ObjMap'; import type { ASTNode, + DocumentNode, FieldNode, FragmentDefinitionNode, - FragmentSpreadNode, + VariableDefinitionNode, } from '../language/ast'; import { isNode } from '../language/ast'; import { Kind } from '../language/kinds'; @@ -43,8 +44,11 @@ import { import type { GraphQLSchema } from '../type/schema'; import { typeFromAST } from './typeFromAST'; -import { valueFromAST } from './valueFromAST'; +export interface FragmentSignature { + readonly definition: FragmentDefinitionNode; + readonly variableDefinitions: ObjMap; +} /** * TypeInfo is a utility class which, given a GraphQL schema, can keep track * of the current field and type definitions at any point in a GraphQL document @@ -60,8 +64,12 @@ export class TypeInfo { private _directive: Maybe; private _argument: Maybe; private _enumValue: Maybe; - private _fragmentSpread: Maybe; - private _fragmentDefinitions: ObjMap; + private _fragmentSignaturesByName: ( + fragmentName: string, + ) => Maybe; + + private _fragmentSignature: Maybe; + private _fragmentArgument: Maybe; private _getFieldDef: GetFieldDefFn; constructor( @@ -73,7 +81,10 @@ export class TypeInfo { initialType?: Maybe, /** @deprecated will be removed in 17.0.0 */ - getFieldDefFn?: GetFieldDefFn, + getFieldDefFn?: Maybe, + fragmentSignatures?: Maybe< + (fragmentName: string) => Maybe + >, ) { this._schema = schema; this._typeStack = []; @@ -84,8 +95,9 @@ export class TypeInfo { this._directive = null; this._argument = null; this._enumValue = null; - this._fragmentSpread = null; - this._fragmentDefinitions = {}; + this._fragmentSignaturesByName = fragmentSignatures ?? (() => null); + this._fragmentSignature = null; + this._fragmentArgument = null; this._getFieldDef = getFieldDefFn ?? getFieldDef; if (initialType) { if (isInputType(initialType)) { @@ -148,6 +160,20 @@ export class TypeInfo { return this._argument; } + getFragmentSignature(): Maybe { + return this._fragmentSignature; + } + + getFragmentSignatureByName(): ( + fragmentName: string, + ) => Maybe { + return this._fragmentSignaturesByName; + } + + getFragmentArgument(): Maybe { + return this._fragmentArgument; + } + getEnumValue(): Maybe { return this._enumValue; } @@ -160,14 +186,9 @@ export class TypeInfo { // 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; - } - } + const fragmentSignatures = getFragmentSignatures(node); + this._fragmentSignaturesByName = (fragmentName: string) => + fragmentSignatures[fragmentName]; break; } case Kind.SELECTION_SET: { @@ -200,7 +221,9 @@ export class TypeInfo { break; } case Kind.FRAGMENT_SPREAD: { - this._fragmentSpread = node; + this._fragmentSignature = this.getFragmentSignatureByName()( + node.name.value, + ); break; } case Kind.INLINE_FRAGMENT: @@ -219,52 +242,29 @@ export class TypeInfo { ); break; } + case Kind.FRAGMENT_ARGUMENT: { + const fragmentSignature = this.getFragmentSignature(); + const argDef = fragmentSignature?.variableDefinitions[node.name.value]; + this._fragmentArgument = argDef; + let argType: unknown; + if (argDef) { + argType = typeFromAST(this._schema, argDef.type); + } + this._inputTypeStack.push(isInputType(argType) ? argType : undefined); + break; + } case Kind.ARGUMENT: { let argDef; let argType: unknown; - 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, + const fieldOrDirective = this.getDirective() ?? this.getFieldDef(); + if (fieldOrDirective) { + argDef = fieldOrDirective.args.find( + (arg) => arg.name === node.name.value, ); - - if (fragVarDef) { - const fragVarType = typeFromAST(schema, fragVarDef.type); - if (isInputType(fragVarType)) { - const fragVarDefault = fragVarDef.defaultValue - ? valueFromAST(fragVarDef.defaultValue, fragVarType) - : undefined; - - 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; - } + if (argDef) { + argType = argDef.type; } - } 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); @@ -315,7 +315,8 @@ export class TypeInfo { leave(node: ASTNode) { switch (node.kind) { case Kind.DOCUMENT: - this._fragmentDefinitions = {}; + this._fragmentSignaturesByName = /* c8 ignore start */ () => + null /* c8 ignore end */; break; case Kind.SELECTION_SET: this._parentTypeStack.pop(); @@ -328,7 +329,7 @@ export class TypeInfo { this._directive = null; break; case Kind.FRAGMENT_SPREAD: - this._fragmentSpread = null; + this._fragmentSignature = null; break; case Kind.OPERATION_DEFINITION: case Kind.INLINE_FRAGMENT: @@ -338,6 +339,12 @@ export class TypeInfo { case Kind.VARIABLE_DEFINITION: this._inputTypeStack.pop(); break; + case Kind.FRAGMENT_ARGUMENT: { + this._fragmentArgument = null; + this._defaultValueStack.pop(); + this._inputTypeStack.pop(); + break; + } case Kind.ARGUMENT: this._argument = null; this._defaultValueStack.pop(); @@ -391,6 +398,25 @@ function getFieldDef( } } +function getFragmentSignatures( + document: DocumentNode, +): ObjMap { + const fragmentSignatures = Object.create(null); + for (const definition of document.definitions) { + if (definition.kind === Kind.FRAGMENT_DEFINITION) { + const variableDefinitions = Object.create(null); + if (definition.variableDefinitions) { + for (const varDef of definition.variableDefinitions) { + variableDefinitions[varDef.variable.name.value] = varDef; + } + } + const signature = { definition, variableDefinitions }; + fragmentSignatures[definition.name.value] = signature; + } + } + return fragmentSignatures; +} + /** * Creates a new visitor instance which maintains a provided TypeInfo instance * along with visiting visitor. diff --git a/src/utilities/__tests__/TypeInfo-test.ts b/src/utilities/__tests__/TypeInfo-test.ts index 0f7ed49f20..e2a7b6110f 100644 --- a/src/utilities/__tests__/TypeInfo-test.ts +++ b/src/utilities/__tests__/TypeInfo-test.ts @@ -68,6 +68,9 @@ describe('TypeInfo', () => { expect(typeInfo.getDirective()).to.equal(null); expect(typeInfo.getArgument()).to.equal(null); expect(typeInfo.getEnumValue()).to.equal(null); + expect(typeInfo.getFragmentSignature()).to.equal(null); + expect(typeInfo.getFragmentSignatureByName()('')).to.equal(null); + expect(typeInfo.getFragmentArgument()).to.equal(null); }); }); @@ -511,12 +514,12 @@ describe('visitWithTypeInfo', () => { ['enter', 'FragmentSpread', null, 'QueryRoot', 'undefined'], ['enter', 'Name', 'Foo', 'QueryRoot', 'undefined'], ['leave', 'Name', 'Foo', 'QueryRoot', 'undefined'], - ['enter', 'Argument', null, 'QueryRoot', 'ID!'], + ['enter', 'FragmentArgument', 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', 'FragmentArgument', null, 'QueryRoot', 'ID!'], ['leave', 'FragmentSpread', null, 'QueryRoot', 'undefined'], ['leave', 'SelectionSet', null, 'QueryRoot', 'undefined'], ['leave', 'OperationDefinition', null, 'QueryRoot', 'undefined'], @@ -617,12 +620,12 @@ describe('visitWithTypeInfo', () => { ['enter', 'FragmentSpread', null, 'QueryRoot', 'undefined'], ['enter', 'Name', 'Foo', 'QueryRoot', 'undefined'], ['leave', 'Name', 'Foo', 'QueryRoot', 'undefined'], - ['enter', 'Argument', null, 'QueryRoot', 'ID'], + ['enter', 'FragmentArgument', null, 'QueryRoot', 'ID'], ['enter', 'Name', 'x', 'QueryRoot', 'ID'], ['leave', 'Name', 'x', 'QueryRoot', 'ID'], ['enter', 'NullValue', null, 'QueryRoot', 'ID'], ['leave', 'NullValue', null, 'QueryRoot', 'ID'], - ['leave', 'Argument', null, 'QueryRoot', 'ID'], + ['leave', 'FragmentArgument', null, 'QueryRoot', 'ID'], ['leave', 'FragmentSpread', null, 'QueryRoot', 'undefined'], ['leave', 'SelectionSet', null, 'QueryRoot', 'undefined'], ['leave', 'OperationDefinition', null, 'QueryRoot', 'undefined'], diff --git a/src/utilities/getVariableSignature.ts b/src/utilities/getVariableSignature.ts new file mode 100644 index 0000000000..362e39314f --- /dev/null +++ b/src/utilities/getVariableSignature.ts @@ -0,0 +1,46 @@ +import { GraphQLError } from '../error/GraphQLError'; + +import type { VariableDefinitionNode } from '../language/ast'; +import { print } from '../language/printer'; + +import { isInputType } from '../type/definition'; +import type { GraphQLInputType, GraphQLSchema } from '../type/index'; + +import { typeFromAST } from './typeFromAST'; +import { valueFromAST } from './valueFromAST'; + +/** + * A GraphQLVariableSignature is required to coerce a variable value. + * + * Designed to have comparable interface to GraphQLArgument so that + * getArgumentValues() can be reused for fragment arguments. + * */ +export interface GraphQLVariableSignature { + name: string; + type: GraphQLInputType; + defaultValue: unknown; +} + +export function getVariableSignature( + schema: GraphQLSchema, + varDefNode: VariableDefinitionNode, +): GraphQLVariableSignature | GraphQLError { + const varName = varDefNode.variable.name.value; + const varType = typeFromAST(schema, varDefNode.type); + + if (!isInputType(varType)) { + // Must use input types for variables. This should be caught during + // validation, however is checked again here for safety. + const varTypeStr = print(varDefNode.type); + return new GraphQLError( + `Variable "$${varName}" expected value of type "${varTypeStr}" which cannot be used as an input type.`, + { nodes: varDefNode.type }, + ); + } + + return { + name: varName, + type: varType, + defaultValue: valueFromAST(varDefNode.defaultValue, varType), + }; +} diff --git a/src/utilities/valueFromAST.ts b/src/utilities/valueFromAST.ts index 2e6cc1c613..bdc964c175 100644 --- a/src/utilities/valueFromAST.ts +++ b/src/utilities/valueFromAST.ts @@ -39,6 +39,7 @@ export function valueFromAST( valueNode: Maybe, type: GraphQLInputType, variables?: Maybe>, + fragmentVariables?: Maybe>, ): unknown { if (!valueNode) { // When there is no node, then there is also no value. @@ -48,11 +49,12 @@ export function valueFromAST( if (valueNode.kind === Kind.VARIABLE) { const variableName = valueNode.name.value; - if (variables == null || variables[variableName] === undefined) { + const variableValue = + fragmentVariables?.[variableName] ?? variables?.[variableName]; + if (variableValue === undefined) { // No valid return value. return; } - const variableValue = variables[variableName]; if (variableValue === null && isNonNullType(type)) { return; // Invalid: intentionally return no value. } @@ -66,7 +68,7 @@ export function valueFromAST( if (valueNode.kind === Kind.NULL) { return; // Invalid: intentionally return no value. } - return valueFromAST(valueNode, type.ofType, variables); + return valueFromAST(valueNode, type.ofType, variables, fragmentVariables); } if (valueNode.kind === Kind.NULL) { @@ -79,7 +81,7 @@ export function valueFromAST( if (valueNode.kind === Kind.LIST) { const coercedValues = []; for (const itemNode of valueNode.values) { - if (isMissingVariable(itemNode, variables)) { + if (isMissingVariable(itemNode, variables, fragmentVariables)) { // If an array contains a missing variable, it is either coerced to // null or if the item type is non-null, it considered invalid. if (isNonNullType(itemType)) { @@ -87,7 +89,12 @@ export function valueFromAST( } coercedValues.push(null); } else { - const itemValue = valueFromAST(itemNode, itemType, variables); + const itemValue = valueFromAST( + itemNode, + itemType, + variables, + fragmentVariables, + ); if (itemValue === undefined) { return; // Invalid: intentionally return no value. } @@ -96,7 +103,12 @@ export function valueFromAST( } return coercedValues; } - const coercedValue = valueFromAST(valueNode, itemType, variables); + const coercedValue = valueFromAST( + valueNode, + itemType, + variables, + fragmentVariables, + ); if (coercedValue === undefined) { return; // Invalid: intentionally return no value. } @@ -111,7 +123,10 @@ export function valueFromAST( const fieldNodes = keyMap(valueNode.fields, (field) => field.name.value); for (const field of Object.values(type.getFields())) { const fieldNode = fieldNodes[field.name]; - if (!fieldNode || isMissingVariable(fieldNode.value, variables)) { + if ( + !fieldNode || + isMissingVariable(fieldNode.value, variables, fragmentVariables) + ) { if (field.defaultValue !== undefined) { coercedObj[field.name] = field.defaultValue; } else if (isNonNullType(field.type)) { @@ -119,7 +134,12 @@ export function valueFromAST( } continue; } - const fieldValue = valueFromAST(fieldNode.value, field.type, variables); + const fieldValue = valueFromAST( + fieldNode.value, + field.type, + variables, + fragmentVariables, + ); if (fieldValue === undefined) { return; // Invalid: intentionally return no value. } @@ -165,9 +185,12 @@ export function valueFromAST( function isMissingVariable( valueNode: ValueNode, variables: Maybe>, + fragmentVariables: Maybe>, ): boolean { return ( valueNode.kind === Kind.VARIABLE && + (fragmentVariables == null || + fragmentVariables[valueNode.name.value] === undefined) && (variables == null || variables[valueNode.name.value] === undefined) ); } diff --git a/src/validation/ValidationContext.ts b/src/validation/ValidationContext.ts index 27a3d456c0..a96fe5fb97 100644 --- a/src/validation/ValidationContext.ts +++ b/src/validation/ValidationContext.ts @@ -27,15 +27,15 @@ import type { import type { GraphQLDirective } from '../type/directives'; import type { GraphQLSchema } from '../type/schema'; -import type { TypeInfo } from '../utilities/TypeInfo'; -import { visitWithTypeInfo } from '../utilities/TypeInfo'; +import type { FragmentSignature } from '../utilities/TypeInfo'; +import { TypeInfo, visitWithTypeInfo } from '../utilities/TypeInfo'; type NodeWithSelectionSet = OperationDefinitionNode | FragmentDefinitionNode; interface VariableUsage { readonly node: VariableNode; readonly type: Maybe; readonly defaultValue: Maybe; - readonly fragmentVarDef: Maybe; + readonly fragmentVariableDefinition: Maybe; } /** @@ -202,26 +202,42 @@ export class ValidationContext extends ASTValidationContext { let usages = this._variableUsages.get(node); if (!usages) { const newUsages: Array = []; - const typeInfo = this._typeInfo; - const fragmentVariableDefinitions = - node.kind === Kind.FRAGMENT_DEFINITION - ? node.variableDefinitions - : undefined; + const typeInfo = new TypeInfo( + this._schema, + undefined, + undefined, + this._typeInfo.getFragmentSignatureByName(), + ); + const fragmentDefinition = + node.kind === Kind.FRAGMENT_DEFINITION ? node : undefined; visit( node, visitWithTypeInfo(typeInfo, { VariableDefinition: () => false, Variable(variable) { - const fragmentVarDef = fragmentVariableDefinitions?.find( - (varDef) => varDef.variable.name.value === variable.name.value, - ); - newUsages.push({ - node: variable, - type: typeInfo.getInputType(), - defaultValue: typeInfo.getDefaultValue(), - fragmentVarDef, - }); + let fragmentVariableDefinition; + if (fragmentDefinition) { + const fragmentSignature = typeInfo.getFragmentSignatureByName()( + fragmentDefinition.name.value, + ); + + fragmentVariableDefinition = + fragmentSignature?.variableDefinitions[variable.name.value]; + newUsages.push({ + node: variable, + type: typeInfo.getInputType(), + defaultValue: undefined, // fragment variables have a variable default but no location default, which is what this default value represents + fragmentVariableDefinition, + }); + } else { + newUsages.push({ + node: variable, + type: typeInfo.getInputType(), + defaultValue: typeInfo.getDefaultValue(), + fragmentVariableDefinition: undefined, + }); + } }, }), ); @@ -245,20 +261,6 @@ export class ValidationContext extends ASTValidationContext { return usages; } - getOperationVariableUsages( - operation: OperationDefinitionNode, - ): ReadonlyArray { - let usages = this._recursiveVariableUsages.get(operation); - if (!usages) { - usages = this.getVariableUsages(operation); - for (const frag of this.getRecursivelyReferencedFragments(operation)) { - usages = usages.concat(this.getVariableUsages(frag)); - } - this._recursiveVariableUsages.set(operation, usages); - } - return usages.filter(({ fragmentVarDef }) => !fragmentVarDef); - } - getType(): Maybe { return this._typeInfo.getType(); } @@ -287,6 +289,16 @@ export class ValidationContext extends ASTValidationContext { return this._typeInfo.getArgument(); } + getFragmentSignature(): Maybe { + return this._typeInfo.getFragmentSignature(); + } + + getFragmentSignatureByName(): ( + fragmentName: string, + ) => Maybe { + return this._typeInfo.getFragmentSignatureByName(); + } + getEnumValue(): Maybe { return this._typeInfo.getEnumValue(); } diff --git a/src/validation/__tests__/KnownArgumentNamesRule-test.ts b/src/validation/__tests__/KnownArgumentNamesRule-test.ts index 4ce5fd190f..854a75ad66 100644 --- a/src/validation/__tests__/KnownArgumentNamesRule-test.ts +++ b/src/validation/__tests__/KnownArgumentNamesRule-test.ts @@ -97,6 +97,19 @@ describe('Validate: Known argument names', () => { `); }); + it('fragment args are known', () => { + expectValid(` + { + dog { + ...withArg(dogCommand: SIT) + } + } + fragment withArg($dogCommand: DogCommand) on Dog { + doesKnowCommand(dogCommand: $dogCommand) + } + `); + }); + it('field args are invalid', () => { expectErrors(` { @@ -145,6 +158,43 @@ describe('Validate: Known argument names', () => { ]); }); + it('arg passed to fragment without arg is reported', () => { + expectErrors(` + { + dog { + ...withoutArg(unknown: true) + } + } + fragment withoutArg on Dog { + doesKnowCommand + } + `).toDeepEqual([ + { + message: 'Unknown argument "unknown" on fragment "withoutArg".', + locations: [{ line: 4, column: 25 }], + }, + ]); + }); + + it('misspelled fragment args are reported', () => { + expectErrors(` + { + dog { + ...withArg(command: SIT) + } + } + fragment withArg($dogCommand: DogCommand) on Dog { + doesKnowCommand(dogCommand: $dogCommand) + } + `).toDeepEqual([ + { + message: + 'Unknown argument "command" on fragment "withArg". Did you mean "dogCommand"?', + locations: [{ line: 4, column: 22 }], + }, + ]); + }); + it('invalid arg name', () => { expectErrors(` fragment invalidArgName on Dog { diff --git a/src/validation/__tests__/NoUnusedFragmentVariablesRule-test.ts b/src/validation/__tests__/NoUnusedFragmentVariablesRule-test.ts deleted file mode 100644 index d7b0060e21..0000000000 --- a/src/validation/__tests__/NoUnusedFragmentVariablesRule-test.ts +++ /dev/null @@ -1,37 +0,0 @@ -import { describe, it } from 'mocha'; - -import { NoUnusedFragmentVariablesRule } from '../rules/NoUnusedFragmentVariablesRule'; - -import { expectValidationErrors } from './harness'; - -function expectErrors(queryStr: string) { - return expectValidationErrors(NoUnusedFragmentVariablesRule, queryStr); -} - -function expectValid(queryStr: string) { - expectErrors(queryStr).toDeepEqual([]); -} - -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', () => { - expectErrors(` - query Foo($b: String) { - ...FragA(a: $b) - } - fragment FragA($a: String) on Type { - field1 - } - `); - }); -}); diff --git a/src/validation/__tests__/NoUnusedVariablesRule-test.ts b/src/validation/__tests__/NoUnusedVariablesRule-test.ts index 6b95847e93..dcb94add26 100644 --- a/src/validation/__tests__/NoUnusedVariablesRule-test.ts +++ b/src/validation/__tests__/NoUnusedVariablesRule-test.ts @@ -248,8 +248,24 @@ describe('Validate: No unused variables', () => { ...FragA(a: $b) } fragment FragA($a: String) on Type { - field1 + field1(a: $a) } `); }); + + it('unused fragment variables are reported', () => { + expectErrors(` + query Foo { + ...FragA(a: "value") + } + fragment FragA($a: String) on Type { + field1 + } + `).toDeepEqual([ + { + message: 'Variable "$a" is never used in fragment "FragA".', + locations: [{ line: 5, column: 22 }], + }, + ]); + }); }); diff --git a/src/validation/__tests__/OverlappingFieldsCanBeMergedRule-test.ts b/src/validation/__tests__/OverlappingFieldsCanBeMergedRule-test.ts index 8d28bb565c..99580e417e 100644 --- a/src/validation/__tests__/OverlappingFieldsCanBeMergedRule-test.ts +++ b/src/validation/__tests__/OverlappingFieldsCanBeMergedRule-test.ts @@ -1177,20 +1177,20 @@ describe('Validate: Overlapping fields can be merged', () => { ]); }); - it('encounters conflict in fragment - field no args', () => { - expectErrors(` + it('allows operations with overlapping fields with arguments using identical operation variables', () => { + expectValid(` query ($y: Int = 1) { a(x: $y) - ...WithArgs + ...WithArgs(x: 1) } - fragment WithArgs on Type { + fragment WithArgs($x: Int = 1) on Type { a(x: $y) } - `).toDeepEqual([]); + `); }); - it('encounters conflict in fragment/field', () => { - expectErrors(` + it('allows operations with overlapping fields with identical variable arguments passed via fragment arguments', () => { + expectValid(` query ($y: Int = 1) { a(x: $y) ...WithArgs(x: $y) @@ -1198,10 +1198,113 @@ describe('Validate: Overlapping fields can be merged', () => { fragment WithArgs($x: Int) on Type { a(x: $x) } - `).toDeepEqual([]); + `); + }); + + it('allows operations with overlapping fields with identical variable arguments passed via nested fragment arguments', () => { + expectValid(` + query ($z: Int = 1) { + a(x: $z) + ...WithArgs(y: $z) + } + fragment WithArgs($y: Int) on Type { + ...NestedWithArgs(x: $y) + } + fragment NestedWithArgs($x: Int) on Type { + a(x: $x) + } + `); + }); + + it('allows operations with overlapping fields with identical arguments via fragment variable defaults', () => { + expectValid(` + query { + a(x: 1) + ...WithArgs + } + fragment WithArgs($x: Int = 1) on Type { + a(x: $x) + } + `); + }); + + it('raises errors with overlapping fields with arguments that conflict via operation variables even with defaults and fragment variable defaults', () => { + expectErrors(` + query ($y: Int = 1) { + a(x: $y) + ...WithArgs + } + fragment WithArgs($x: Int = 1) on Type { + a(x: $x) + } + `).toDeepEqual([ + { + message: + 'Fields "a" conflict because they have differing arguments. Use different aliases on the fields to fetch both if this was intentional.', + locations: [ + { line: 3, column: 11 }, + { line: 7, column: 11 }, + ], + }, + ]); + }); + + it('allows operations with overlapping list fields with identical variable arguments passed via fragment arguments', () => { + expectValid(` + query Query($stringListVarY: [String]) { + complicatedArgs { + stringListArgField(stringListArg: $stringListVarY) + ...WithArgs(stringListVarX: $stringListVarY) + } + } + fragment WithArgs($stringListVarX: [String]) on Type { + stringListArgField(stringListArg: $stringListVarX) + } + `); + }); + + it('allows operations with overlapping list fields with identical variable arguments in item position passed via fragment arguments', () => { + expectValid(` + query Query($stringListVarY: [String]) { + complicatedArgs { + stringListArgField(stringListArg: [$stringListVarY]) + ...WithArgs(stringListVarX: $stringListVarY) + } + } + fragment WithArgs($stringListVarX: [String]) on Type { + stringListArgField(stringListArg: [$stringListVarX]) + } + `); + }); + + it('allows operations with overlapping input object fields with identical variable arguments passed via fragment arguments', () => { + expectValid(` + query Query($complexVarY: ComplexInput) { + complicatedArgs { + complexArgField(complexArg: $complexVarY) + ...WithArgs(complexVarX: $complexVarY) + } + } + fragment WithArgs($complexVarX: ComplexInput) on Type { + complexArgField(complexArg: $complexVarX) + } + `); + }); + + it('allows operations with overlapping input object fields with identical variable arguments in field position passed via fragment arguments', () => { + expectValid(` + query Query($boolVarY: Boolean) { + complicatedArgs { + complexArgField(complexArg: {requiredArg: $boolVarY}) + ...WithArgs(boolVarX: $boolVarY) + } + } + fragment WithArgs($boolVarX: Boolean) on Type { + complexArgField(complexArg: {requiredArg: $boolVarX}) + } + `); }); - // This is currently not validated, should we? it('encounters nested field conflict in fragments that could otherwise merge', () => { expectErrors(` query ValidDifferingFragmentArgs($command1: DogCommand, $command2: DogCommand) { diff --git a/src/validation/__tests__/ProvidedRequiredArgumentsRule-test.ts b/src/validation/__tests__/ProvidedRequiredArgumentsRule-test.ts index 1cfedeeada..a5f1d235f5 100644 --- a/src/validation/__tests__/ProvidedRequiredArgumentsRule-test.ts +++ b/src/validation/__tests__/ProvidedRequiredArgumentsRule-test.ts @@ -366,26 +366,6 @@ describe('Validate: Provided required arguments', () => { `); }); - // 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(` { @@ -396,7 +376,7 @@ describe('Validate: Provided required arguments', () => { } `); }); - // Above proposal: this should be an error + it('Missing nullable argument is allowed', () => { expectValid(` { @@ -407,6 +387,7 @@ describe('Validate: Provided required arguments', () => { } `); }); + it('Missing non-nullable argument with default is allowed', () => { expectValid(` { @@ -429,11 +410,8 @@ describe('Validate: Provided required arguments', () => { `).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: 5, column: 22 }, - ], + 'Fragment "F" argument "x" of type "Int!" is required, but it was not provided.', + locations: [{ line: 3, column: 13 }], }, ]); }); diff --git a/src/validation/index.ts b/src/validation/index.ts index 14646ac270..587479e351 100644 --- a/src/validation/index.ts +++ b/src/validation/index.ts @@ -51,9 +51,6 @@ export { PossibleFragmentSpreadsRule } from './rules/PossibleFragmentSpreadsRule // Spec Section: "Argument Optionality" export { ProvidedRequiredArgumentsRule } from './rules/ProvidedRequiredArgumentsRule'; -// Spec Section: "All Fragment Variables Used" -export { NoUnusedFragmentVariablesRule } from './rules/NoUnusedFragmentVariablesRule'; - // Spec Section: "Leaf Field Selections" export { ScalarLeafsRule } from './rules/ScalarLeafsRule'; diff --git a/src/validation/rules/KnownArgumentNamesRule.ts b/src/validation/rules/KnownArgumentNamesRule.ts index 332b21c1ca..373bb6981b 100644 --- a/src/validation/rules/KnownArgumentNamesRule.ts +++ b/src/validation/rules/KnownArgumentNamesRule.ts @@ -26,6 +26,29 @@ export function KnownArgumentNamesRule(context: ValidationContext): ASTVisitor { return { // eslint-disable-next-line new-cap ...KnownArgumentNamesOnDirectivesRule(context), + FragmentArgument(argNode) { + const fragmentSignature = context.getFragmentSignature(); + if (fragmentSignature) { + const varDef = + fragmentSignature.variableDefinitions[argNode.name.value]; + if (!varDef) { + const argName = argNode.name.value; + const suggestions = suggestionList( + argName, + Array.from( + Object.values(fragmentSignature.variableDefinitions), + ).map((varSignature) => varSignature.variable.name.value), + ); + context.reportError( + new GraphQLError( + `Unknown argument "${argName}" on fragment "${fragmentSignature.definition.name.value}".` + + didYouMean(suggestions), + { nodes: argNode }, + ), + ); + } + } + }, Argument(argNode) { const argDef = context.getArgument(); const fieldDef = context.getFieldDef(); @@ -33,8 +56,10 @@ export function KnownArgumentNamesRule(context: ValidationContext): ASTVisitor { if (!argDef && fieldDef && parentType) { const argName = argNode.name.value; - const knownArgsNames = fieldDef.args.map((arg) => arg.name); - const suggestions = suggestionList(argName, knownArgsNames); + const suggestions = suggestionList( + argName, + fieldDef.args.map((arg) => arg.name), + ); context.reportError( new GraphQLError( `Unknown argument "${argName}" on field "${parentType.name}.${fieldDef.name}".` + diff --git a/src/validation/rules/NoUndefinedVariablesRule.ts b/src/validation/rules/NoUndefinedVariablesRule.ts index 82692a3a91..c91ede1e84 100644 --- a/src/validation/rules/NoUndefinedVariablesRule.ts +++ b/src/validation/rules/NoUndefinedVariablesRule.ts @@ -25,8 +25,8 @@ export function NoUndefinedVariablesRule( leave(operation) { const usages = context.getRecursiveVariableUsages(operation); - for (const { node, fragmentVarDef } of usages) { - if (fragmentVarDef) { + for (const { node, fragmentVariableDefinition } of usages) { + if (fragmentVariableDefinition) { continue; } const varName = node.name.value; diff --git a/src/validation/rules/NoUnusedFragmentVariablesRule.ts b/src/validation/rules/NoUnusedFragmentVariablesRule.ts deleted file mode 100644 index 9cfde886bd..0000000000 --- a/src/validation/rules/NoUnusedFragmentVariablesRule.ts +++ /dev/null @@ -1,40 +0,0 @@ -import { GraphQLError } from '../../error/GraphQLError'; - -import type { ASTVisitor } from '../../language/visitor'; - -import type { ValidationContext } from '../ValidationContext'; - -/** - * 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-Fragment-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 c2f58d05b0..fb67b35ebf 100644 --- a/src/validation/rules/NoUnusedVariablesRule.ts +++ b/src/validation/rules/NoUnusedVariablesRule.ts @@ -1,6 +1,5 @@ import { GraphQLError } from '../../error/GraphQLError'; -import type { VariableDefinitionNode } from '../../language/ast'; import type { ASTVisitor } from '../../language/visitor'; import type { ValidationContext } from '../ValidationContext'; @@ -11,41 +10,56 @@ import type { ValidationContext } from '../ValidationContext'; * A GraphQL operation is only valid if all variables defined by an operation * are used, either directly or within a spread fragment. * - * See https://spec.graphql.org/draft/#sec-All-Operation-Variables-Used + * See https://spec.graphql.org/draft/#sec-All-Variables-Used */ export function NoUnusedVariablesRule(context: ValidationContext): ASTVisitor { - let variableDefs: Array = []; - return { - OperationDefinition: { - enter() { - variableDefs = []; - }, - leave(operation) { - const variableNameUsed = Object.create(null); - const usages = context.getOperationVariableUsages(operation); - - for (const { node } of usages) { - variableNameUsed[node.name.value] = true; + 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 }, + ), + ); } + } + }, + OperationDefinition(operation) { + const usages = context.getRecursiveVariableUsages(operation); + const operationVariableNameUsed = new Set(); + for (const { node, fragmentVariableDefinition } of usages) { + const varName = node.name.value; + if (!fragmentVariableDefinition) { + operationVariableNameUsed.add(varName); + } + } - for (const variableDef of variableDefs) { - const variableName = variableDef.variable.name.value; - if (variableNameUsed[variableName] !== true) { - context.reportError( - new GraphQLError( - operation.name - ? `Variable "$${variableName}" is never used in operation "${operation.name.value}".` - : `Variable "$${variableName}" is never used.`, - { nodes: variableDef }, - ), - ); - } + // FIXME: https://github.com/graphql/graphql-js/issues/2203 + /* c8 ignore next */ + const variableDefinitions = operation.variableDefinitions ?? []; + for (const variableDef of variableDefinitions) { + const variableName = variableDef.variable.name.value; + if (!operationVariableNameUsed.has(variableName)) { + context.reportError( + new GraphQLError( + operation.name + ? `Variable "$${variableName}" is never used in operation "${operation.name.value}".` + : `Variable "$${variableName}" is never used.`, + { nodes: variableDef }, + ), + ); } - }, - }, - VariableDefinition(def) { - variableDefs.push(def); + } }, }; } diff --git a/src/validation/rules/OverlappingFieldsCanBeMergedRule.ts b/src/validation/rules/OverlappingFieldsCanBeMergedRule.ts index 0ac766b95c..51b5408e58 100644 --- a/src/validation/rules/OverlappingFieldsCanBeMergedRule.ts +++ b/src/validation/rules/OverlappingFieldsCanBeMergedRule.ts @@ -5,8 +5,9 @@ import type { ObjMap } from '../../jsutils/ObjMap'; import { GraphQLError } from '../../error/GraphQLError'; import type { - DirectiveNode, + ArgumentNode, FieldNode, + FragmentArgumentNode, FragmentDefinitionNode, FragmentSpreadNode, SelectionSetNode, @@ -15,7 +16,6 @@ import type { import { Kind } from '../../language/kinds'; import { print } from '../../language/printer'; import type { ASTVisitor } from '../../language/visitor'; -import { visit } from '../../language/visitor'; import type { GraphQLField, @@ -66,7 +66,7 @@ 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 cachedFieldsAndFragmentSpreads = new Map(); @@ -106,23 +106,17 @@ type NodeAndDef = [ ]; // Map of array of those. type NodeAndDefCollection = ObjMap>; -type FragmentSpreadsByName = ObjMap>; +interface FragmentSpread { + key: string; + node: FragmentSpreadNode; + varMap: Map | undefined; +} +type FragmentSpreads = ReadonlyArray; type FieldsAndFragmentSpreads = readonly [ NodeAndDefCollection, - FragmentSpreadsByName, + FragmentSpreads, ]; -const printFragmentSpreadArguments = (fragmentSpread: FragmentSpreadNode) => { - if (!fragmentSpread.arguments || fragmentSpread.arguments.length === 0) { - return fragmentSpread.name.value; - } - - const printedArguments: Array = fragmentSpread.arguments - .map(print) - .sort((a, b) => a.localeCompare(b)); - return fragmentSpread.name.value + '(' + printedArguments.join(',') + ')'; -}; - /** * Algorithm: * @@ -193,11 +187,12 @@ function findConflictsWithinSelectionSet( ): Array { const conflicts: Array = []; - const [fieldMap, fragmentSpreadMap] = getFieldsAndFragmentSpreads( + const [fieldMap, fragmentSpreads] = getFieldsAndFragmentSpreads( context, cachedFieldsAndFragmentSpreads, parentType, selectionSet, + undefined, ); // (A) First find all conflicts "within" the fields and fragment-spreads of this selection set. @@ -210,15 +205,10 @@ function findConflictsWithinSelectionSet( fieldMap, ); - const allFragmentSpreads = []; - for (const [, fragmentSpreads] of Object.entries(fragmentSpreadMap)) { - allFragmentSpreads.push(...fragmentSpreads); - } - - if (allFragmentSpreads.length !== 0) { + if (fragmentSpreads.length !== 0) { // (B) Then collect conflicts between these fields and those represented by // each spread fragment name found. - for (let i = 0; i < allFragmentSpreads.length; i++) { + for (let i = 0; i < fragmentSpreads.length; i++) { collectConflictsBetweenFieldsAndFragment( context, conflicts, @@ -226,21 +216,21 @@ function findConflictsWithinSelectionSet( comparedFragmentPairs, false, fieldMap, - allFragmentSpreads[i], + 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 < allFragmentSpreads.length; j++) { + for (let j = i + 1; j < fragmentSpreads.length; j++) { collectConflictsBetweenFragments( context, conflicts, cachedFieldsAndFragmentSpreads, comparedFragmentPairs, false, - allFragmentSpreads[i], - allFragmentSpreads[j], + fragmentSpreads[i], + fragmentSpreads[j], ); } } @@ -260,9 +250,9 @@ function collectConflictsBetweenFieldsAndFragment( comparedFragmentPairs: PairSet, areMutuallyExclusive: boolean, fieldMap: NodeAndDefCollection, - fragmentSpread: FragmentSpreadNode, + fragmentSpread: FragmentSpread, ): void { - const fragmentName = fragmentSpread.name.value; + const fragmentName = fragmentSpread.node.name.value; const fragment = context.getFragment(fragmentName); if (!fragment) { return; @@ -273,7 +263,7 @@ function collectConflictsBetweenFieldsAndFragment( context, cachedFieldsAndFragmentSpreads, fragment, - fragmentSpread, + fragmentSpread.varMap, ); // Do not compare a fragment's fieldMap to itself. @@ -290,27 +280,29 @@ function collectConflictsBetweenFieldsAndFragment( comparedFragmentPairs, areMutuallyExclusive, fieldMap, + undefined, fieldMap2, + fragmentSpread.varMap, ); // (E) Then collect any conflicts between the provided collection of fields // and any fragment names found in the given fragment. - for (const [referencedFragmentName, [spread]] of Object.entries( + for (const referencedFragmentSpread of Object.values( referencedFragmentNames, )) { // Memoize so two fragments are not compared for conflicts more than once. if ( comparedFragmentPairs.has( - referencedFragmentName, - fragmentName, + referencedFragmentSpread.key, + fragmentSpread.key, areMutuallyExclusive, ) ) { continue; } comparedFragmentPairs.add( - referencedFragmentName, - fragmentName, + referencedFragmentSpread.key, + fragmentSpread.key, areMutuallyExclusive, ); @@ -321,7 +313,7 @@ function collectConflictsBetweenFieldsAndFragment( comparedFragmentPairs, areMutuallyExclusive, fieldMap, - spread, + referencedFragmentSpread, ); } } @@ -337,46 +329,50 @@ function collectConflictsBetweenFragments( >, comparedFragmentPairs: PairSet, areMutuallyExclusive: boolean, - fragmentSpread1: FragmentSpreadNode, - fragmentSpread2: FragmentSpreadNode, + fragmentSpread1: FragmentSpread, + fragmentSpread2: FragmentSpread, ): void { - const fragmentName1 = fragmentSpread1.name.value; - const fragmentName2 = fragmentSpread2.name.value; - if ( - fragmentName1 === fragmentName2 && - !sameArguments(fragmentSpread1, fragmentSpread2) - ) { - context.reportError( - new GraphQLError( - `Spreads "${fragmentName1}" conflict because ${printFragmentSpreadArguments( - fragmentSpread1, - )} and ${printFragmentSpreadArguments( - fragmentSpread2, - )} have different fragment arguments.`, - { nodes: [fragmentSpread1, fragmentSpread2] }, - ), - ); + // No need to compare a fragment to itself. + if (fragmentSpread1.key === fragmentSpread2.key) { return; } - // No need to compare a fragment to itself. - if ( - fragmentName1 === fragmentName2 && - sameArguments(fragmentSpread1, fragmentSpread2) - ) { - return; + if (fragmentSpread1.node.name.value === fragmentSpread2.node.name.value) { + if ( + !sameArguments( + fragmentSpread1.node.arguments, + fragmentSpread1.varMap, + fragmentSpread2.node.arguments, + fragmentSpread2.varMap, + ) + ) { + context.reportError( + new GraphQLError( + `Spreads "${fragmentSpread1.node.name.value}" conflict because ${fragmentSpread1.key} and ${fragmentSpread2.key} have different fragment arguments.`, + { nodes: [fragmentSpread1.node, fragmentSpread2.node] }, + ), + ); + return; + } } - const fragKey1 = printFragmentSpreadArguments(fragmentSpread1); - const fragKey2 = printFragmentSpreadArguments(fragmentSpread2); - // Memoize so two fragments are not compared for conflicts more than once. - if (comparedFragmentPairs.has(fragKey1, fragKey2, areMutuallyExclusive)) { + if ( + comparedFragmentPairs.has( + fragmentSpread1.key, + fragmentSpread2.key, + areMutuallyExclusive, + ) + ) { return; } - comparedFragmentPairs.add(fragKey1, fragKey2, areMutuallyExclusive); + comparedFragmentPairs.add( + fragmentSpread1.key, + fragmentSpread2.key, + areMutuallyExclusive, + ); - const fragment1 = context.getFragment(fragmentName1); - const fragment2 = context.getFragment(fragmentName2); + const fragment1 = context.getFragment(fragmentSpread1.node.name.value); + const fragment2 = context.getFragment(fragmentSpread2.node.name.value); if (!fragment1 || !fragment2) { return; } @@ -386,14 +382,14 @@ function collectConflictsBetweenFragments( context, cachedFieldsAndFragmentSpreads, fragment1, - fragmentSpread1, + fragmentSpread1.varMap, ); const [fieldMap2, referencedFragmentSpreads2] = getReferencedFieldsAndFragmentSpreads( context, cachedFieldsAndFragmentSpreads, fragment2, - fragmentSpread2, + fragmentSpread2.varMap, ); // (F) First, collect all conflicts between these two collections of fields @@ -405,14 +401,14 @@ function collectConflictsBetweenFragments( comparedFragmentPairs, areMutuallyExclusive, fieldMap1, + fragmentSpread1.varMap, fieldMap2, + fragmentSpread2.varMap, ); // (G) Then collect conflicts between the first fragment and any nested // fragments spread in the second fragment. - for (const [referencedFragmentSpread2] of Object.values( - referencedFragmentSpreads2, - )) { + for (const referencedFragmentSpread2 of referencedFragmentSpreads2) { collectConflictsBetweenFragments( context, conflicts, @@ -426,9 +422,7 @@ function collectConflictsBetweenFragments( // (G) Then collect conflicts between the second fragment and any nested // fragments spread in the first fragment. - for (const [referencedFragmentSpread1] of Object.values( - referencedFragmentSpreads1, - )) { + for (const referencedFragmentSpread1 of referencedFragmentSpreads1) { collectConflictsBetweenFragments( context, conflicts, @@ -454,22 +448,26 @@ function findConflictsBetweenSubSelectionSets( areMutuallyExclusive: boolean, parentType1: Maybe, selectionSet1: SelectionSetNode, + varMap1: Map | undefined, parentType2: Maybe, selectionSet2: SelectionSetNode, + varMap2: Map | undefined, ): Array { const conflicts: Array = []; - const [fieldMap1, fragmentSpreadsByName1] = getFieldsAndFragmentSpreads( + const [fieldMap1, fragmentSpreads1] = getFieldsAndFragmentSpreads( context, cachedFieldsAndFragmentSpreads, parentType1, selectionSet1, + varMap1, ); - const [fieldMap2, fragmentSpreadsByName2] = getFieldsAndFragmentSpreads( + const [fieldMap2, fragmentSpreads2] = getFieldsAndFragmentSpreads( context, cachedFieldsAndFragmentSpreads, parentType2, selectionSet2, + varMap2, ); // (H) First, collect all conflicts between these two collections of field. @@ -480,12 +478,14 @@ function findConflictsBetweenSubSelectionSets( comparedFragmentPairs, areMutuallyExclusive, fieldMap1, + varMap1, fieldMap2, + varMap2, ); // (I) Then collect conflicts between the first collection of fields and // those referenced by each fragment name associated with the second. - for (const [fragmentSpread2] of Object.values(fragmentSpreadsByName2)) { + for (const fragmentSpread2 of fragmentSpreads2) { collectConflictsBetweenFieldsAndFragment( context, conflicts, @@ -499,7 +499,7 @@ function findConflictsBetweenSubSelectionSets( // (I) Then collect conflicts between the second collection of fields and // those referenced by each fragment name associated with the first. - for (const [fragmentSpread1] of Object.values(fragmentSpreadsByName1)) { + for (const fragmentSpread1 of fragmentSpreads1) { collectConflictsBetweenFieldsAndFragment( context, conflicts, @@ -511,11 +511,11 @@ function findConflictsBetweenSubSelectionSets( ); } - // (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 [fragmentSpread1] of Object.values(fragmentSpreadsByName1)) { - for (const [fragmentSpread2] of Object.values(fragmentSpreadsByName2)) { + // (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 fragmentSpreads1) { + for (const fragmentSpread2 of fragmentSpreads2) { collectConflictsBetweenFragments( context, conflicts, @@ -559,7 +559,9 @@ function collectConflictsWithin( false, // within one collection is never mutually exclusive responseName, fields[i], + undefined, fields[j], + undefined, ); if (conflict) { conflicts.push(conflict); @@ -585,7 +587,9 @@ function collectConflictsBetween( comparedFragmentPairs: PairSet, parentFieldsAreMutuallyExclusive: boolean, fieldMap1: NodeAndDefCollection, + varMap1: Map | undefined, fieldMap2: NodeAndDefCollection, + varMap2: Map | undefined, ): void { // A field map is a keyed collection, where each key represents a response // name and the value at that key is a list of all fields which provide that @@ -604,7 +608,9 @@ function collectConflictsBetween( parentFieldsAreMutuallyExclusive, responseName, field1, + varMap1, field2, + varMap2, ); if (conflict) { conflicts.push(conflict); @@ -627,7 +633,9 @@ function findConflict( parentFieldsAreMutuallyExclusive: boolean, responseName: string, field1: NodeAndDef, + varMap1: Map | undefined, field2: NodeAndDef, + varMap2: Map | undefined, ): Maybe { const [parentType1, node1, def1] = field1; const [parentType2, node2, def2] = field2; @@ -659,7 +667,7 @@ function findConflict( } // Two field calls must have the same arguments. - if (!sameArguments(node1, node2)) { + if (!sameArguments(node1.arguments, varMap1, node2.arguments, varMap2)) { return [ [responseName, 'they have differing arguments'], [node1], @@ -685,7 +693,7 @@ function findConflict( ]; } - // Collect and compare sub-fields. Use the same "visited fragment names" list + // Collect and compare sub-fields. Use the same "visited fragment spreads" list // for both collections so fields in a fragment reference are never // compared to themselves. const selectionSet1 = node1.selectionSet; @@ -698,20 +706,21 @@ function findConflict( areMutuallyExclusive, getNamedType(type1), selectionSet1, + varMap1, getNamedType(type2), selectionSet2, + varMap2, ); return subfieldConflicts(conflicts, responseName, node1, node2); } } -function sameArguments( - node1: FieldNode | DirectiveNode | FragmentSpreadNode, - node2: FieldNode | DirectiveNode | FragmentSpreadNode, +function sameArguments( + args1: ReadonlyArray | undefined, + varMap1: Map | undefined, + args2: ReadonlyArray | undefined, + varMap2: Map | undefined, ): boolean { - const args1 = node1.arguments; - const args2 = node2.arguments; - if (args1 === undefined || args1.length === 0) { return args2 === undefined || args2.length === 0; } @@ -726,9 +735,17 @@ function sameArguments( /* c8 ignore next */ } - const values2 = new Map(args2.map(({ name, value }) => [name.value, value])); + const values2 = new Map( + args2.map(({ name, value }) => [ + name.value, + varMap2 === undefined ? value : replaceFragmentVariables(value, varMap2), + ]), + ); return args1.every((arg1) => { - const value1 = arg1.value; + let value1 = arg1.value; + if (varMap1) { + value1 = replaceFragmentVariables(value1, varMap1); + } const value2 = values2.get(arg1.name.value); if (value2 === undefined) { return false; @@ -738,6 +755,34 @@ function sameArguments( }); } +function replaceFragmentVariables( + valueNode: ValueNode, + varMap: ReadonlyMap, +): ValueNode { + switch (valueNode.kind) { + case Kind.VARIABLE: + return varMap.get(valueNode.name.value) ?? valueNode; + case Kind.LIST: + return { + ...valueNode, + values: valueNode.values.map((node) => + replaceFragmentVariables(node, varMap), + ), + }; + case Kind.OBJECT: + return { + ...valueNode, + fields: valueNode.fields.map((field) => ({ + ...field, + value: replaceFragmentVariables(field.value, varMap), + })), + }; + default: { + return valueNode; + } + } +} + function stringifyValue(value: ValueNode): string | null { return print(sortValueNode(value)); } @@ -782,22 +827,26 @@ function getFieldsAndFragmentSpreads( >, parentType: Maybe, selectionSet: SelectionSetNode, + varMap: Map | undefined, ): FieldsAndFragmentSpreads { const cached = cachedFieldsAndFragmentSpreads.get(selectionSet); if (cached) { return cached; } const nodeAndDefs: NodeAndDefCollection = Object.create(null); - const fragmentSpreadsByName: ObjMap> = - Object.create(null); + const fragmentSpreads: ObjMap = Object.create(null); _collectFieldsAndFragmentNames( context, parentType, selectionSet, nodeAndDefs, - fragmentSpreadsByName, + fragmentSpreads, + varMap, ); - const result = [nodeAndDefs, fragmentSpreadsByName] as const; + const result: FieldsAndFragmentSpreads = [ + nodeAndDefs, + Array.from(Object.values(fragmentSpreads)), + ]; cachedFieldsAndFragmentSpreads.set(selectionSet, result); return result; } @@ -811,23 +860,9 @@ function getReferencedFieldsAndFragmentSpreads( FieldsAndFragmentSpreads >, fragment: FragmentDefinitionNode, - fragmentSpread: FragmentSpreadNode, + varMap: Map | undefined, ) { - const args = fragmentSpread.arguments; - const fragmentSelectionSet = visit(fragment.selectionSet, { - Variable: (node) => { - const name = node.name.value; - const argNode = args?.find((arg) => arg.name.value === name); - if (argNode) { - return argNode.value; - } - - return node; - }, - }); - - // Short-circuit building a type from the node if possible. - const cached = cachedFieldsAndFragmentSpreads.get(fragmentSelectionSet); + const cached = cachedFieldsAndFragmentSpreads.get(fragment.selectionSet); if (cached) { return cached; } @@ -837,7 +872,8 @@ function getReferencedFieldsAndFragmentSpreads( context, cachedFieldsAndFragmentSpreads, fragmentType, - fragmentSelectionSet, + fragment.selectionSet, + varMap, ); } @@ -846,7 +882,8 @@ function _collectFieldsAndFragmentNames( parentType: Maybe, selectionSet: SelectionSetNode, nodeAndDefs: NodeAndDefCollection, - fragmentSpreadsByName: ObjMap>, + fragmentSpreads: ObjMap, + varMap: Map | undefined, ): void { for (const selection of selectionSet.selections) { switch (selection.kind) { @@ -866,12 +903,8 @@ function _collectFieldsAndFragmentNames( break; } case Kind.FRAGMENT_SPREAD: { - const existing = fragmentSpreadsByName[selection.name.value]; - if (existing) { - existing.push(selection); - } else { - fragmentSpreadsByName[selection.name.value] = [selection]; - } + const fragmentSpread = getFragmentSpread(context, selection, varMap); + fragmentSpreads[fragmentSpread.key] = fragmentSpread; break; } case Kind.INLINE_FRAGMENT: { @@ -884,7 +917,8 @@ function _collectFieldsAndFragmentNames( inlineFragmentType, selection.selectionSet, nodeAndDefs, - fragmentSpreadsByName, + fragmentSpreads, + varMap, ); break; } @@ -892,6 +926,50 @@ function _collectFieldsAndFragmentNames( } } +function getFragmentSpread( + context: ValidationContext, + fragmentSpreadNode: FragmentSpreadNode, + varMap: Map | undefined, +): FragmentSpread { + let key = ''; + const newVarMap = new Map(); + const fragmentSignature = context.getFragmentSignatureByName()( + fragmentSpreadNode.name.value, + ); + const argMap = new Map(); + if (fragmentSpreadNode.arguments) { + for (const arg of fragmentSpreadNode.arguments) { + argMap.set(arg.name.value, arg.value); + } + } + if (fragmentSignature?.variableDefinitions) { + key += fragmentSpreadNode.name.value + '('; + for (const [varName, variable] of Object.entries( + fragmentSignature.variableDefinitions, + )) { + const value = argMap.get(varName); + if (value) { + key += varName + ': ' + print(sortValueNode(value)); + } + const arg = argMap.get(varName); + if (arg !== undefined) { + newVarMap.set( + varName, + varMap !== undefined ? replaceFragmentVariables(arg, varMap) : arg, + ); + } else if (variable.defaultValue) { + newVarMap.set(varName, variable.defaultValue); + } + } + key += ')'; + } + return { + key, + node: fragmentSpreadNode, + varMap: newVarMap.size > 0 ? newVarMap : undefined, + }; +} + // Given a series of Conflicts which occurred between two sub-fields, generate // a single Conflict. function subfieldConflicts( diff --git a/src/validation/rules/ProvidedRequiredArgumentsRule.ts b/src/validation/rules/ProvidedRequiredArgumentsRule.ts index 7a7e8ff153..e6a425e970 100644 --- a/src/validation/rules/ProvidedRequiredArgumentsRule.ts +++ b/src/validation/rules/ProvidedRequiredArgumentsRule.ts @@ -16,6 +16,8 @@ import type { GraphQLArgument } from '../../type/definition'; import { isRequiredArgument, isType } from '../../type/definition'; import { specifiedDirectives } from '../../type/directives'; +import { typeFromAST } from '../../utilities/typeFromAST'; + import type { SDLValidationContext, ValidationContext, @@ -40,7 +42,6 @@ export function ProvidedRequiredArgumentsRule( if (!fieldDef) { return false; } - const providedArgs = new Set( // FIXME: https://github.com/graphql/graphql-js/issues/2203 /* c8 ignore next */ @@ -60,10 +61,10 @@ export function ProvidedRequiredArgumentsRule( }, }, FragmentSpread: { - // Validate on leave to allow for directive errors to appear first. + // Validate on leave to allow for deeper errors to appear first. leave(spreadNode) { - const fragmentDef = context.getFragment(spreadNode.name.value); - if (!fragmentDef) { + const fragmentSignature = context.getFragmentSignature(); + if (!fragmentSignature) { return false; } @@ -72,18 +73,22 @@ export function ProvidedRequiredArgumentsRule( /* 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 ?? []) { + for (const [varName, variableDefinition] of Object.entries( + fragmentSignature.variableDefinitions, + )) { if ( - !providedArgs.has(varDef.variable.name.value) && - isRequiredArgumentNode(varDef) + !providedArgs.has(varName) && + isRequiredArgumentNode(variableDefinition) ) { - const argTypeStr = inspect(varDef.type); + const type = typeFromAST( + context.getSchema(), + variableDefinition.type, + ); + const argTypeStr = inspect(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] }, + `Fragment "${spreadNode.name.value}" argument "${varName}" of type "${argTypeStr}" is required, but it was not provided.`, + { nodes: spreadNode }, ), ); } diff --git a/src/validation/rules/SingleFieldSubscriptionsRule.ts b/src/validation/rules/SingleFieldSubscriptionsRule.ts index a93aeeb224..d8e40e4499 100644 --- a/src/validation/rules/SingleFieldSubscriptionsRule.ts +++ b/src/validation/rules/SingleFieldSubscriptionsRule.ts @@ -2,13 +2,11 @@ import type { ObjMap } from '../../jsutils/ObjMap'; import { GraphQLError } from '../../error/GraphQLError'; -import type { - FragmentDefinitionNode, - OperationDefinitionNode, -} from '../../language/ast'; +import type { OperationDefinitionNode } from '../../language/ast'; import { Kind } from '../../language/kinds'; import type { ASTVisitor } from '../../language/visitor'; +import type { FragmentDetails } from '../../execution/collectFields'; import { collectFields } from '../../execution/collectFields'; import type { ValidationContext } from '../ValidationContext'; @@ -35,10 +33,10 @@ export function SingleFieldSubscriptionsRule( [variable: string]: any; } = Object.create(null); const document = context.getDocument(); - const fragments: ObjMap = Object.create(null); + const fragments: ObjMap = Object.create(null); for (const definition of document.definitions) { if (definition.kind === Kind.FRAGMENT_DEFINITION) { - fragments[definition.name.value] = definition; + fragments[definition.name.value] = { definition }; } } const fields = collectFields( diff --git a/src/validation/rules/VariablesInAllowedPositionRule.ts b/src/validation/rules/VariablesInAllowedPositionRule.ts index 1a3c0cff02..0b616d2aa9 100644 --- a/src/validation/rules/VariablesInAllowedPositionRule.ts +++ b/src/validation/rules/VariablesInAllowedPositionRule.ts @@ -36,9 +36,18 @@ export function VariablesInAllowedPositionRule( leave(operation) { const usages = context.getRecursiveVariableUsages(operation); - for (const { node, type, defaultValue, fragmentVarDef } of usages) { + for (const { + node, + type, + defaultValue, + fragmentVariableDefinition, + } of usages) { const varName = node.name.value; - const varDef = fragmentVarDef ?? varDefMap[varName]; + let varDef = fragmentVariableDefinition; + if (!varDef) { + varDef = 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 d1e50c5acf..c312c9839c 100644 --- a/src/validation/specifiedRules.ts +++ b/src/validation/specifiedRules.ts @@ -27,8 +27,6 @@ import { NoFragmentCyclesRule } from './rules/NoFragmentCyclesRule'; import { NoUndefinedVariablesRule } from './rules/NoUndefinedVariablesRule'; // Spec Section: "Fragments must be used" import { NoUnusedFragmentsRule } from './rules/NoUnusedFragmentsRule'; -// Spec Section: "All Fragment Variables Used" -import { NoUnusedFragmentVariablesRule } from './rules/NoUnusedFragmentVariablesRule'; // Spec Section: "All Variables Used" import { NoUnusedVariablesRule } from './rules/NoUnusedVariablesRule'; // Spec Section: "Field Selection Merging" @@ -102,7 +100,6 @@ export const specifiedRules: ReadonlyArray = Object.freeze([ NoUndefinedVariablesRule, NoUnusedVariablesRule, KnownDirectivesRule, - NoUnusedFragmentVariablesRule, UniqueDirectivesPerLocationRule, KnownArgumentNamesRule, UniqueArgumentNamesRule,