From 157aadac1fcc59671b41c52a74f8514a3c7e5ecc Mon Sep 17 00:00:00 2001 From: Jovi De Croock Date: Fri, 6 Sep 2024 11:33:21 +0200 Subject: [PATCH] feat: add experimental support for parsing fragment arguments (#4015) This is a rebase of https://github.com/graphql/graphql-js/pull/3847 This implements execution of Fragment Arguments, and more specifically visiting, parsing and printing of fragment-spreads with arguments and fragment definitions with variables, as described by the spec changes in https://github.com/graphql/graphql-spec/pull/1081. There are a few amendments in terms of execution and keying the fragment-spreads, these are reflected in https://github.com/mjmahone/graphql-spec/pull/3 The purpose is to be able to independently review all the moving parts, the stacked PR's will contain mentions of open feedback that was present at the time. - [execution changes](https://github.com/JoviDeCroock/graphql-js/pull/2) - [TypeInfo & validation changes](https://github.com/JoviDeCroock/graphql-js/pull/4) - [validation changes in isolation](https://github.com/JoviDeCroock/graphql-js/pull/5) CC @mjmahone the original author --------- Co-authored-by: mjmahone Co-authored-by: Yaacov Rydzinski --- src/execution/__tests__/variables-test.ts | 403 ++++++++++++++++- src/execution/collectFields.ts | 78 +++- src/execution/execute.ts | 34 +- src/execution/getVariableSignature.ts | 46 ++ src/execution/values.ts | 63 ++- src/index.ts | 1 + src/language/__tests__/parser-test.ts | 25 +- src/language/__tests__/printer-test.ts | 44 +- src/language/__tests__/visitor-test.ts | 48 +- src/language/ast.ts | 13 +- src/language/directiveLocation.ts | 1 + src/language/index.ts | 1 + src/language/kinds.ts | 1 + src/language/parser.ts | 61 ++- src/language/printer.ts | 28 +- src/type/__tests__/introspection-test.ts | 5 + src/type/introspection.ts | 6 +- src/utilities/TypeInfo.ts | 98 +++- src/utilities/__tests__/TypeInfo-test.ts | 220 +++++++++ src/utilities/__tests__/printSchema-test.ts | 5 +- src/utilities/buildASTSchema.ts | 2 +- src/utilities/valueFromAST.ts | 39 +- src/validation/ValidationContext.ts | 49 +- .../__tests__/KnownArgumentNamesRule-test.ts | 50 +++ .../__tests__/KnownDirectivesRule-test.ts | 14 +- .../NoUndefinedVariablesRule-test.ts | 63 +++ .../__tests__/NoUnusedVariablesRule-test.ts | 38 ++ .../OverlappingFieldsCanBeMergedRule-test.ts | 235 ++++++++++ .../ProvidedRequiredArgumentsRule-test.ts | 79 ++++ .../__tests__/ValuesOfCorrectTypeRule-test.ts | 31 ++ .../VariablesAreInputTypesRule-test.ts | 27 ++ .../VariablesInAllowedPositionRule-test.ts | 104 +++++ src/validation/__tests__/harness.ts | 2 +- .../rules/KnownArgumentNamesRule.ts | 30 +- src/validation/rules/KnownDirectivesRule.ts | 9 +- .../rules/NoUndefinedVariablesRule.ts | 5 +- src/validation/rules/NoUnusedVariablesRule.ts | 32 +- .../rules/OverlappingFieldsCanBeMergedRule.ts | 418 +++++++++++++----- .../rules/ProvidedRequiredArgumentsRule.ts | 47 +- .../rules/SingleFieldSubscriptionsRule.ts | 15 +- .../rules/VariablesInAllowedPositionRule.ts | 13 +- 41 files changed, 2236 insertions(+), 247 deletions(-) create mode 100644 src/execution/getVariableSignature.ts diff --git a/src/execution/__tests__/variables-test.ts b/src/execution/__tests__/variables-test.ts index 778ee6d594..de60ea674e 100644 --- a/src/execution/__tests__/variables-test.ts +++ b/src/execution/__tests__/variables-test.ts @@ -7,6 +7,7 @@ import { inspect } from '../../jsutils/inspect.js'; import { GraphQLError } from '../../error/GraphQLError.js'; +import { DirectiveLocation } from '../../language/directiveLocation.js'; import { Kind } from '../../language/kinds.js'; import { parse } from '../../language/parser.js'; @@ -22,10 +23,14 @@ import { GraphQLObjectType, GraphQLScalarType, } from '../../type/definition.js'; -import { GraphQLString } from '../../type/scalars.js'; +import { + GraphQLDirective, + GraphQLIncludeDirective, +} from '../../type/directives.js'; +import { GraphQLBoolean, GraphQLString } from '../../type/scalars.js'; import { GraphQLSchema } from '../../type/schema.js'; -import { executeSync } from '../execute.js'; +import { executeSync, experimentalExecuteIncrementally } from '../execute.js'; import { getVariableValues } from '../values.js'; const TestFaultyScalarGraphQLError = new GraphQLError( @@ -59,6 +64,13 @@ const TestComplexScalar = new GraphQLScalarType({ }, }); +const NestedType: GraphQLObjectType = new GraphQLObjectType({ + name: 'NestedType', + fields: { + echo: fieldWithInputArg({ type: GraphQLString }), + }, +}); + const TestInputObject = new GraphQLInputObjectType({ name: 'TestInputObject', fields: { @@ -129,6 +141,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)), }), @@ -143,7 +159,30 @@ const TestType = new GraphQLObjectType({ }, }); -const schema = new GraphQLSchema({ query: TestType }); +const schema = new GraphQLSchema({ + query: TestType, + directives: [ + new GraphQLDirective({ + name: 'skip', + description: + 'Directs the executor to skip this field or fragment when the `if` argument is true.', + locations: [ + DirectiveLocation.FIELD, + DirectiveLocation.FRAGMENT_SPREAD, + DirectiveLocation.INLINE_FRAGMENT, + ], + args: { + if: { + type: new GraphQLNonNull(GraphQLBoolean), + description: 'Skipped when true.', + // default values will override operation variables in the setting of defined fragment variables that are not provided + defaultValue: true, + }, + }, + }), + GraphQLIncludeDirective, + ], +}); function executeQuery( query: string, @@ -153,6 +192,14 @@ function executeQuery( return executeSync({ schema, document, variableValues }); } +function executeQueryWithFragmentArguments( + query: string, + variableValues?: { [variable: string]: unknown }, +) { + const document = parse(query, { experimentalFragmentArguments: true }); + return executeSync({ schema, document, variableValues }); +} + describe('Execute: Handles inputs', () => { describe('Handles objects and nullability', () => { describe('using inline structs', () => { @@ -1137,4 +1184,354 @@ describe('Execute: Handles inputs', () => { }); }); }); + + describe('using fragment arguments', () => { + it('when there are no fragment arguments', () => { + const result = executeQueryWithFragmentArguments(` + 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 = executeQueryWithFragmentArguments(` + 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 = executeQueryWithFragmentArguments(` + query { + ...a + } + fragment a($value: String!) on TestType { + fieldWithNullableStringInput(input: $value) + } + `); + + expect(result).to.have.property('errors'); + expect(result.errors).to.have.length(1); + expect(result.errors?.at(0)?.message).to.match( + /Argument "value" of required type "String!"/, + ); + }); + + it('when the definition has a default and is provided', () => { + const result = executeQueryWithFragmentArguments(` + 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 = executeQueryWithFragmentArguments(` + query { + ...a + } + fragment a($value: String! = "B") on TestType { + fieldWithNonNullableStringInput(input: $value) + } + `); + expect(result).to.deep.equal({ + data: { + fieldWithNonNullableStringInput: '"B"', + }, + }); + }); + + it('when a definition has a default, is not provided, and spreads another fragment', () => { + const result = executeQueryWithFragmentArguments(` + query { + ...a + } + fragment a($a: String! = "B") on TestType { + ...b(b: $a) + } + fragment b($b: String!) on TestType { + fieldWithNonNullableStringInput(input: $b) + } + `); + expect(result).to.deep.equal({ + data: { + fieldWithNonNullableStringInput: '"B"', + }, + }); + }); + + it('when the definition has a non-nullable default and is provided null', () => { + const result = executeQueryWithFragmentArguments(` + query { + ...a(value: null) + } + fragment a($value: String! = "B") on TestType { + fieldWithNullableStringInput(input: $value) + } + `); + + expect(result).to.have.property('errors'); + expect(result.errors).to.have.length(1); + expect(result.errors?.at(0)?.message).to.match( + /Argument "value" of non-null type "String!"/, + ); + }); + + it('when the definition has no default and is not provided', () => { + const result = executeQueryWithFragmentArguments(` + 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 = executeQueryWithFragmentArguments(` + 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 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") { + ...a + } + fragment a($x: String) on TestType { + fieldWithNonNullableStringInputAndDefaultArgumentValue(input: $x) + } + `); + expect(result).to.deep.equal({ + data: { + fieldWithNonNullableStringInputAndDefaultArgumentValue: + '"Hello World"', + }, + }); + }); + + it('when a fragment-variable is shadowed by an intermediate fragment-spread but defined in the operation-variables', () => { + const result = executeQueryWithFragmentArguments(` + query($x: String = "A") { + ...a + } + fragment a($x: String) on TestType { + ...b + } + + fragment b on TestType { + fieldWithNullableStringInput(input: $x) + } + `); + expect(result).to.deep.equal({ + data: { + fieldWithNullableStringInput: '"A"', + }, + }); + }); + + it('when a fragment is used with different args', () => { + const result = executeQueryWithFragmentArguments(` + 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 = executeQueryWithFragmentArguments(` + 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 = executeQueryWithFragmentArguments(` + 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 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") { + ...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]', + }, + }); + }); + + 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 a nullable argument to a directive with a field default is not provided and shadowed by an operation variable', () => { + // this test uses the @defer directive and incremental delivery because the `if` argument for skip/include have no field defaults + const document = parse( + ` + query($shouldDefer: Boolean = false) { + ...a + } + fragment a($shouldDefer: Boolean) on TestType { + ... @defer(if: $shouldDefer) { + fieldWithDefaultArgumentValue + } + } + `, + { experimentalFragmentArguments: true }, + ); + const result = experimentalExecuteIncrementally({ schema, document }); + expect(result).to.include.keys('initialResult', 'subsequentResults'); + }); + }); }); diff --git a/src/execution/collectFields.ts b/src/execution/collectFields.ts index d411ff3f77..94dd2b50bd 100644 --- a/src/execution/collectFields.ts +++ b/src/execution/collectFields.ts @@ -24,26 +24,39 @@ import type { GraphQLSchema } from '../type/schema.js'; import { typeFromAST } from '../utilities/typeFromAST.js'; -import { getDirectiveValues } from './values.js'; +import type { GraphQLVariableSignature } from './getVariableSignature.js'; +import { experimentalGetArgumentValues, getDirectiveValues } from './values.js'; export interface DeferUsage { label: string | undefined; parentDeferUsage: DeferUsage | undefined; } +export interface FragmentVariables { + signatures: ObjMap; + values: ObjMap; +} + export interface FieldDetails { node: FieldNode; - deferUsage: DeferUsage | undefined; + deferUsage?: DeferUsage | undefined; + fragmentVariables?: FragmentVariables | undefined; } export type FieldGroup = ReadonlyArray; export type GroupedFieldSet = ReadonlyMap; +export interface FragmentDetails { + definition: FragmentDefinitionNode; + variableSignatures?: ObjMap | undefined; +} + interface CollectFieldsContext { schema: GraphQLSchema; - fragments: ObjMap; + fragments: ObjMap; variableValues: { [variable: string]: unknown }; + fragmentVariableValues?: FragmentVariables; operation: OperationDefinitionNode; runtimeType: GraphQLObjectType; visitedFragmentNames: Set; @@ -60,7 +73,7 @@ interface CollectFieldsContext { */ export function collectFields( schema: GraphQLSchema, - fragments: ObjMap, + fragments: ObjMap, variableValues: { [variable: string]: unknown }, runtimeType: GraphQLObjectType, operation: OperationDefinitionNode, @@ -101,7 +114,7 @@ export function collectFields( // eslint-disable-next-line max-params export function collectSubfields( schema: GraphQLSchema, - fragments: ObjMap, + fragments: ObjMap, variableValues: { [variable: string]: unknown }, operation: OperationDefinitionNode, returnType: GraphQLObjectType, @@ -140,12 +153,14 @@ export function collectSubfields( }; } +// eslint-disable-next-line max-params function collectFieldsImpl( context: CollectFieldsContext, selectionSet: SelectionSetNode, groupedFieldSet: AccumulatorMap, newDeferUsages: Array, deferUsage?: DeferUsage, + fragmentVariables?: FragmentVariables, ): void { const { schema, @@ -159,18 +174,19 @@ function collectFieldsImpl( for (const selection of selectionSet.selections) { switch (selection.kind) { case Kind.FIELD: { - if (!shouldIncludeNode(variableValues, selection)) { + if (!shouldIncludeNode(selection, variableValues, fragmentVariables)) { continue; } groupedFieldSet.add(getFieldEntryKey(selection), { node: selection, deferUsage, + fragmentVariables, }); break; } case Kind.INLINE_FRAGMENT: { if ( - !shouldIncludeNode(variableValues, selection) || + !shouldIncludeNode(selection, variableValues, fragmentVariables) || !doesFragmentConditionMatch(schema, selection, runtimeType) ) { continue; @@ -179,6 +195,7 @@ function collectFieldsImpl( const newDeferUsage = getDeferUsage( operation, variableValues, + fragmentVariables, selection, deferUsage, ); @@ -190,6 +207,7 @@ function collectFieldsImpl( groupedFieldSet, newDeferUsages, deferUsage, + fragmentVariables, ); } else { newDeferUsages.push(newDeferUsage); @@ -199,6 +217,7 @@ function collectFieldsImpl( groupedFieldSet, newDeferUsages, newDeferUsage, + fragmentVariables, ); } @@ -210,6 +229,7 @@ function collectFieldsImpl( const newDeferUsage = getDeferUsage( operation, variableValues, + fragmentVariables, selection, deferUsage, ); @@ -217,7 +237,7 @@ function collectFieldsImpl( if ( !newDeferUsage && (visitedFragmentNames.has(fragName) || - !shouldIncludeNode(variableValues, selection)) + !shouldIncludeNode(selection, variableValues, fragmentVariables)) ) { continue; } @@ -225,27 +245,44 @@ function collectFieldsImpl( const fragment = fragments[fragName]; if ( fragment == null || - !doesFragmentConditionMatch(schema, fragment, runtimeType) + !doesFragmentConditionMatch(schema, fragment.definition, runtimeType) ) { continue; } + + const fragmentVariableSignatures = fragment.variableSignatures; + let newFragmentVariables: FragmentVariables | undefined; + if (fragmentVariableSignatures) { + newFragmentVariables = { + signatures: fragmentVariableSignatures, + values: experimentalGetArgumentValues( + selection, + Object.values(fragmentVariableSignatures), + variableValues, + fragmentVariables, + ), + }; + } + if (!newDeferUsage) { visitedFragmentNames.add(fragName); collectFieldsImpl( context, - fragment.selectionSet, + fragment.definition.selectionSet, groupedFieldSet, newDeferUsages, deferUsage, + newFragmentVariables, ); } else { newDeferUsages.push(newDeferUsage); collectFieldsImpl( context, - fragment.selectionSet, + fragment.definition.selectionSet, groupedFieldSet, newDeferUsages, newDeferUsage, + newFragmentVariables, ); } break; @@ -262,10 +299,16 @@ function collectFieldsImpl( function getDeferUsage( operation: OperationDefinitionNode, variableValues: { [variable: string]: unknown }, + fragmentVariables: FragmentVariables | undefined, node: FragmentSpreadNode | InlineFragmentNode, parentDeferUsage: DeferUsage | undefined, ): DeferUsage | undefined { - const defer = getDirectiveValues(GraphQLDeferDirective, node, variableValues); + const defer = getDirectiveValues( + GraphQLDeferDirective, + node, + variableValues, + fragmentVariables, + ); if (!defer) { return; @@ -291,10 +334,16 @@ function getDeferUsage( * 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; } @@ -303,6 +352,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 351f2452c2..97e88f07ad 100644 --- a/src/execution/execute.ts +++ b/src/execution/execute.ts @@ -5,6 +5,7 @@ import { isAsyncIterable } from '../jsutils/isAsyncIterable.js'; import { isIterableObject } from '../jsutils/isIterableObject.js'; import { isObjectLike } from '../jsutils/isObjectLike.js'; import { isPromise } from '../jsutils/isPromise.js'; +import { mapValue } from '../jsutils/mapValue.js'; import type { Maybe } from '../jsutils/Maybe.js'; import { memoize3 } from '../jsutils/memoize3.js'; import type { ObjMap } from '../jsutils/ObjMap.js'; @@ -20,7 +21,6 @@ import { locatedError } from '../error/locatedError.js'; import type { DocumentNode, FieldNode, - FragmentDefinitionNode, OperationDefinitionNode, } from '../language/ast.js'; import { OperationTypeNode } from '../language/ast.js'; @@ -53,12 +53,14 @@ import { buildExecutionPlan } from './buildExecutionPlan.js'; import type { DeferUsage, FieldGroup, + FragmentDetails, GroupedFieldSet, } from './collectFields.js'; import { collectFields, collectSubfields as _collectSubfields, } from './collectFields.js'; +import { getVariableSignature } from './getVariableSignature.js'; import { buildIncrementalResponse } from './IncrementalPublisher.js'; import { mapAsyncIterable } from './mapAsyncIterable.js'; import type { @@ -74,6 +76,7 @@ import type { } from './types.js'; import { DeferredFragmentRecord } from './types.js'; import { + experimentalGetArgumentValues, getArgumentValues, getDirectiveValues, getVariableValues, @@ -132,7 +135,7 @@ const collectSubfields = memoize3( */ export interface ExecutionContext { schema: GraphQLSchema; - fragments: ObjMap; + fragments: ObjMap; rootValue: unknown; contextValue: unknown; operation: OperationDefinitionNode; @@ -462,7 +465,7 @@ export function buildExecutionContext( assertValidSchema(schema); 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: @@ -479,9 +482,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 } @@ -737,10 +749,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( - fieldDef, + const args = experimentalGetArgumentValues( fieldGroup[0].node, + fieldDef.args, exeContext.variableValues, + fieldGroup[0].fragmentVariables, ); // The resolve function's optional third argument is a context value that @@ -823,7 +836,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, @@ -1046,6 +1062,7 @@ function getStreamUsage( GraphQLStreamDirective, fieldGroup[0].node, exeContext.variableValues, + fieldGroup[0].fragmentVariables, ); if (!stream) { @@ -1074,6 +1091,7 @@ function getStreamUsage( const streamedFieldGroup: FieldGroup = fieldGroup.map((fieldDetails) => ({ node: fieldDetails.node, deferUsage: undefined, + fragmentVariables: fieldDetails.fragmentVariables, })); const streamUsage = { diff --git a/src/execution/getVariableSignature.ts b/src/execution/getVariableSignature.ts new file mode 100644 index 0000000000..984b816c9b --- /dev/null +++ b/src/execution/getVariableSignature.ts @@ -0,0 +1,46 @@ +import { GraphQLError } from '../error/GraphQLError.js'; + +import type { VariableDefinitionNode } from '../language/ast.js'; +import { print } from '../language/printer.js'; + +import { isInputType } from '../type/definition.js'; +import type { GraphQLInputType, GraphQLSchema } from '../type/index.js'; + +import { typeFromAST } from '../utilities/typeFromAST.js'; +import { valueFromAST } from '../utilities/valueFromAST.js'; + +/** + * 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/execution/values.ts b/src/execution/values.ts index 8f7a3d62bd..23863fd107 100644 --- a/src/execution/values.ts +++ b/src/execution/values.ts @@ -8,20 +8,24 @@ import { GraphQLError } from '../error/GraphQLError.js'; import type { DirectiveNode, FieldNode, + FragmentSpreadNode, VariableDefinitionNode, } from '../language/ast.js'; import { Kind } from '../language/kinds.js'; import { print } from '../language/printer.js'; -import type { GraphQLField } from '../type/definition.js'; -import { isInputType, isNonNullType } from '../type/definition.js'; +import type { GraphQLArgument, GraphQLField } from '../type/definition.js'; +import { isNonNullType } from '../type/definition.js'; import type { GraphQLDirective } from '../type/directives.js'; import type { GraphQLSchema } from '../type/schema.js'; import { coerceInputValue } from '../utilities/coerceInputValue.js'; -import { typeFromAST } from '../utilities/typeFromAST.js'; import { valueFromAST } from '../utilities/valueFromAST.js'; +import type { FragmentVariables } from './collectFields.js'; +import type { GraphQLVariableSignature } from './getVariableSignature.js'; +import { getVariableSignature } from './getVariableSignature.js'; + type CoercedVariableValues = | { errors: ReadonlyArray; coerced?: never } | { coerced: { [variable: string]: unknown }; errors?: never }; @@ -76,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 (!Object.hasOwn(inputs, varName)) { if (varDefNode.defaultValue) { - coercedValues[varName] = valueFromAST(varDefNode.defaultValue, varType); + coercedValues[varName] = varSignature.defaultValue; } else if (isNonNullType(varType)) { const varTypeStr = inspect(varType); onError( @@ -152,6 +148,15 @@ export function getArgumentValues( def: GraphQLField | GraphQLDirective, node: FieldNode | DirectiveNode, variableValues?: Maybe>, +): { [argument: string]: unknown } { + return experimentalGetArgumentValues(node, def.args, variableValues); +} + +export function experimentalGetArgumentValues( + node: FieldNode | DirectiveNode | FragmentSpreadNode, + argDefs: ReadonlyArray, + variableValues: Maybe>, + fragmentVariables?: Maybe, ): { [argument: string]: unknown } { const coercedValues: { [argument: string]: unknown } = {}; @@ -160,7 +165,7 @@ export function getArgumentValues( const argumentNodes = node.arguments ?? []; const argNodeMap = new Map(argumentNodes.map((arg) => [arg.name.value, arg])); - for (const argDef of def.args) { + for (const argDef of argDefs) { const name = argDef.name; const argType = argDef.type; const argumentNode = argNodeMap.get(name); @@ -183,9 +188,12 @@ export function getArgumentValues( if (valueNode.kind === Kind.VARIABLE) { const variableName = valueNode.name.value; + const scopedVariableValues = fragmentVariables?.signatures[variableName] + ? fragmentVariables.values + : variableValues; if ( - variableValues == null || - !Object.hasOwn(variableValues, variableName) + scopedVariableValues == null || + !Object.hasOwn(scopedVariableValues, variableName) ) { if (argDef.defaultValue !== undefined) { coercedValues[name] = argDef.defaultValue; @@ -198,7 +206,7 @@ export function getArgumentValues( } continue; } - isNull = variableValues[variableName] == null; + isNull = scopedVariableValues[variableName] == null; } if (isNull && isNonNullType(argType)) { @@ -209,7 +217,12 @@ export function getArgumentValues( ); } - const coercedValue = valueFromAST(valueNode, argType, variableValues); + 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 @@ -241,12 +254,18 @@ export function getDirectiveValues( directiveDef: GraphQLDirective, node: { readonly directives?: ReadonlyArray | undefined }, variableValues?: Maybe>, + fragmentVariables?: Maybe, ): undefined | { [argument: string]: unknown } { const directiveNode = node.directives?.find( (directive) => directive.name.value === directiveDef.name, ); if (directiveNode) { - return getArgumentValues(directiveDef, directiveNode, variableValues); + return experimentalGetArgumentValues( + directiveNode, + directiveDef.args, + variableValues, + fragmentVariables, + ); } } diff --git a/src/index.ts b/src/index.ts index fb67b4c75e..3d82bffdda 100644 --- a/src/index.ts +++ b/src/index.ts @@ -264,6 +264,7 @@ export type { SelectionNode, FieldNode, ArgumentNode, + FragmentArgumentNode, NullabilityAssertionNode, NonNullAssertionNode, ErrorBoundaryNode, diff --git a/src/language/__tests__/parser-test.ts b/src/language/__tests__/parser-test.ts index 34e9dff4b9..215e743be7 100644 --- a/src/language/__tests__/parser-test.ts +++ b/src/language/__tests__/parser-test.ts @@ -607,13 +607,32 @@ describe('Parser', () => { expect('loc' in result).to.equal(false); }); - it('Legacy: allows parsing fragment defined variables', () => { + it('allows parsing fragment defined variables', () => { const document = 'fragment a($v: Boolean = false) on t { f(v: $v) }'; expect(() => - parse(document, { allowLegacyFragmentVariables: true }), + parse(document, { experimentalFragmentArguments: true }), ).to.not.throw(); - expect(() => parse(document)).to.throw('Syntax Error'); + }); + + it('disallows parsing fragment defined variables without experimental flag', () => { + const document = 'fragment a($v: Boolean = false) on t { f(v: $v) }'; + + expect(() => parse(document)).to.throw(); + }); + + it('allows parsing fragment spread arguments', () => { + const document = 'fragment a on t { ...b(v: $v) }'; + + expect(() => + parse(document, { experimentalFragmentArguments: true }), + ).to.not.throw(); + }); + + it('disallows parsing fragment spread arguments without experimental flag', () => { + const document = 'fragment a on t { ...b(v: $v) }'; + + expect(() => parse(document)).to.throw(); }); it('contains location that can be Object.toStringified, JSON.stringified, or jsutils.inspected', () => { diff --git a/src/language/__tests__/printer-test.ts b/src/language/__tests__/printer-test.ts index 7eea508458..8669b64ec7 100644 --- a/src/language/__tests__/printer-test.ts +++ b/src/language/__tests__/printer-test.ts @@ -168,34 +168,62 @@ describe('Printer: Query document', () => { `); }); - it('Legacy: prints fragment with variable directives', () => { - const queryASTWithVariableDirective = parse( + it('prints fragment with argument definition directives', () => { + const fragmentWithArgumentDefinitionDirective = parse( 'fragment Foo($foo: TestType @test) on TestType @testDirective { id }', - { allowLegacyFragmentVariables: true }, + { experimentalFragmentArguments: true }, ); - expect(print(queryASTWithVariableDirective)).to.equal(dedent` + expect(print(fragmentWithArgumentDefinitionDirective)).to.equal(dedent` fragment Foo($foo: TestType @test) on TestType @testDirective { id } `); }); - it('Legacy: correctly prints fragment defined variables', () => { - const fragmentWithVariable = parse( + it('correctly prints fragment defined arguments', () => { + const fragmentWithArgumentDefinition = parse( ` fragment Foo($a: ComplexType, $b: Boolean = false) on TestType { id } `, - { allowLegacyFragmentVariables: true }, + { experimentalFragmentArguments: true }, ); - expect(print(fragmentWithVariable)).to.equal(dedent` + expect(print(fragmentWithArgumentDefinition)).to.equal(dedent` fragment Foo($a: ComplexType, $b: Boolean = false) on TestType { id } `); }); + it('prints fragment spread with arguments', () => { + const fragmentSpreadWithArguments = parse( + 'fragment Foo on TestType { ...Bar(a: {x: $x}, b: true) }', + { experimentalFragmentArguments: true }, + ); + expect(print(fragmentSpreadWithArguments)).to.equal(dedent` + fragment Foo on TestType { + ...Bar(a: { x: $x }, b: true) + } + `); + }); + + it('prints fragment spread with multi-line arguments', () => { + const fragmentSpreadWithArguments = parse( + 'fragment Foo on TestType { ...Bar(a: {x: $x, y: $y, z: $z, xy: $xy}, b: true, c: "a long string extending arguments over max length") }', + { experimentalFragmentArguments: true }, + ); + expect(print(fragmentSpreadWithArguments)).to.equal(dedent` + fragment Foo on TestType { + ...Bar( + a: { x: $x, y: $y, z: $z, xy: $xy } + b: true + c: "a long string extending arguments over max length" + ) + } + `); + }); + it('prints kitchen sink without altering ast', () => { const ast = parse(kitchenSinkQuery, { noLocation: true, diff --git a/src/language/__tests__/visitor-test.ts b/src/language/__tests__/visitor-test.ts index c70cb783d5..73b46d6830 100644 --- a/src/language/__tests__/visitor-test.ts +++ b/src/language/__tests__/visitor-test.ts @@ -455,10 +455,10 @@ describe('Visitor', () => { ]); }); - it('Legacy: visits variables defined in fragments', () => { + it('visits arguments defined on fragments', () => { const ast = parse('fragment a($v: Boolean = false) on t { f }', { noLocation: true, - allowLegacyFragmentVariables: true, + experimentalFragmentArguments: true, }); const visited: Array = []; @@ -505,6 +505,50 @@ describe('Visitor', () => { ]); }); + it('visits arguments on fragment spreads', () => { + const ast = parse('fragment a on t { ...s(v: false) }', { + noLocation: true, + experimentalFragmentArguments: true, + }); + const visited: Array = []; + + visit(ast, { + enter(node) { + checkVisitorFnArgs(ast, arguments); + visited.push(['enter', node.kind, getValue(node)]); + }, + leave(node) { + checkVisitorFnArgs(ast, arguments); + visited.push(['leave', node.kind, getValue(node)]); + }, + }); + + expect(visited).to.deep.equal([ + ['enter', 'Document', undefined], + ['enter', 'FragmentDefinition', undefined], + ['enter', 'Name', 'a'], + ['leave', 'Name', 'a'], + ['enter', 'NamedType', undefined], + ['enter', 'Name', 't'], + ['leave', 'Name', 't'], + ['leave', 'NamedType', undefined], + ['enter', 'SelectionSet', undefined], + ['enter', 'FragmentSpread', undefined], + ['enter', 'Name', 's'], + ['leave', 'Name', 's'], + ['enter', 'FragmentArgument', { kind: 'BooleanValue', value: false }], + ['enter', 'Name', 'v'], + ['leave', 'Name', 'v'], + ['enter', 'BooleanValue', false], + ['leave', 'BooleanValue', false], + ['leave', 'FragmentArgument', { kind: 'BooleanValue', value: false }], + ['leave', 'FragmentSpread', undefined], + ['leave', 'SelectionSet', undefined], + ['leave', 'FragmentDefinition', undefined], + ['leave', 'Document', undefined], + ]); + }); + it('properly visits the kitchen sink query', () => { const ast = parse(kitchenSinkQuery, { experimentalClientControlledNullability: true, diff --git a/src/language/ast.ts b/src/language/ast.ts index 22a7cc253c..8eb8eb132b 100644 --- a/src/language/ast.ts +++ b/src/language/ast.ts @@ -146,6 +146,7 @@ export type ASTNode = | SelectionSetNode | FieldNode | ArgumentNode + | FragmentArgumentNode | FragmentSpreadNode | InlineFragmentNode | FragmentDefinitionNode @@ -221,13 +222,14 @@ export const QueryDocumentKeys: { 'nullabilityAssertion', ], Argument: ['name', 'value'], + FragmentArgument: ['name', 'value'], // Note: Client Controlled Nullability is experimental and may be changed // or removed in the future. ListNullabilityOperator: ['nullabilityAssertion'], NonNullAssertion: ['nullabilityAssertion'], ErrorBoundary: ['nullabilityAssertion'], - FragmentSpread: ['name', 'directives'], + FragmentSpread: ['name', 'arguments', 'directives'], InlineFragment: ['typeCondition', 'directives', 'selectionSet'], FragmentDefinition: [ 'name', @@ -422,12 +424,20 @@ export interface ConstArgumentNode { readonly value: ConstValueNode; } +export interface FragmentArgumentNode { + readonly kind: Kind.FRAGMENT_ARGUMENT; + readonly loc?: Location | undefined; + readonly name: NameNode; + readonly value: ValueNode; +} + /** Fragments */ export interface FragmentSpreadNode { readonly kind: Kind.FRAGMENT_SPREAD; readonly loc?: Location | undefined; readonly name: NameNode; + readonly arguments?: ReadonlyArray | undefined; readonly directives?: ReadonlyArray | undefined; } @@ -443,7 +453,6 @@ export interface FragmentDefinitionNode { readonly kind: Kind.FRAGMENT_DEFINITION; readonly loc?: Location | undefined; readonly name: NameNode; - /** @deprecated variableDefinitions will be removed in v17.0.0 */ readonly variableDefinitions?: | ReadonlyArray | undefined; diff --git a/src/language/directiveLocation.ts b/src/language/directiveLocation.ts index b10214d47f..568de6bba1 100644 --- a/src/language/directiveLocation.ts +++ b/src/language/directiveLocation.ts @@ -23,4 +23,5 @@ export enum DirectiveLocation { ENUM_VALUE = 'ENUM_VALUE', INPUT_OBJECT = 'INPUT_OBJECT', INPUT_FIELD_DEFINITION = 'INPUT_FIELD_DEFINITION', + FRAGMENT_VARIABLE_DEFINITION = 'FRAGMENT_VARIABLE_DEFINITION', } diff --git a/src/language/index.ts b/src/language/index.ts index fc87a566cc..2899f47c64 100644 --- a/src/language/index.ts +++ b/src/language/index.ts @@ -44,6 +44,7 @@ export type { ErrorBoundaryNode, ListNullabilityOperatorNode, ArgumentNode, + FragmentArgumentNode /* for experimental fragment arguments */, ConstArgumentNode, FragmentSpreadNode, InlineFragmentNode, diff --git a/src/language/kinds.ts b/src/language/kinds.ts index d606c12cbe..f3d8fdc3c5 100644 --- a/src/language/kinds.ts +++ b/src/language/kinds.ts @@ -12,6 +12,7 @@ export enum Kind { SELECTION_SET = 'SelectionSet', FIELD = 'Field', ARGUMENT = 'Argument', + FRAGMENT_ARGUMENT = 'FragmentArgument', /** Nullability Modifiers */ LIST_NULLABILITY_OPERATOR = 'ListNullabilityOperator', diff --git a/src/language/parser.ts b/src/language/parser.ts index cd9345f6dd..0bda2b0c8f 100644 --- a/src/language/parser.ts +++ b/src/language/parser.ts @@ -24,6 +24,7 @@ import type { FieldDefinitionNode, FieldNode, FloatValueNode, + FragmentArgumentNode, FragmentDefinitionNode, FragmentSpreadNode, InlineFragmentNode, @@ -92,21 +93,25 @@ export interface ParseOptions { maxTokens?: number | undefined; /** - * @deprecated will be removed in the v17.0.0 + * EXPERIMENTAL: * - * If enabled, the parser will understand and parse variable definitions - * contained in a fragment definition. They'll be represented in the - * `variableDefinitions` field of the FragmentDefinitionNode. + * If enabled, the parser will understand and parse fragment variable definitions + * and arguments on fragment spreads. Fragment variable definitions will be represented + * in the `variableDefinitions` field of the FragmentDefinitionNode. + * Fragment spread arguments will be represented in the `arguments` field of FragmentSpreadNode. * - * The syntax is identical to normal, query-defined variables. For example: + * For example: * * ```graphql + * { + * t { ...A(var: true) } + * } * fragment A($var: Boolean = false) on T { - * ... + * ...B(x: $var) * } * ``` */ - allowLegacyFragmentVariables?: boolean | undefined; + experimentalFragmentArguments?: boolean | undefined; /** * EXPERIMENTAL: @@ -524,6 +529,12 @@ export class Parser { return this.optionalMany(TokenKind.PAREN_L, item, TokenKind.PAREN_R); } + /* experimental */ + parseFragmentArguments(): Array { + const item = this.parseFragmentArgument; + return this.optionalMany(TokenKind.PAREN_L, item, TokenKind.PAREN_R); + } + /** * Argument[Const] : Name : Value[?Const] */ @@ -545,12 +556,25 @@ export class Parser { return this.parseArgument(true); } + /* 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), + }); + } + // Implements the parsing rules in the Fragments section. /** * Corresponds to both FragmentSpread and InlineFragment in the spec. * - * FragmentSpread : ... FragmentName Directives? + * FragmentSpread : ... FragmentName Arguments? Directives? * * InlineFragment : ... TypeCondition? Directives? SelectionSet */ @@ -560,9 +584,21 @@ export class Parser { const hasTypeCondition = this.expectOptionalKeyword('on'); if (!hasTypeCondition && this.peek(TokenKind.NAME)) { + const name = this.parseFragmentName(); + if ( + this.peek(TokenKind.PAREN_L) && + this._options.experimentalFragmentArguments + ) { + return this.node(start, { + kind: Kind.FRAGMENT_SPREAD, + name, + arguments: this.parseFragmentArguments(), + directives: this.parseDirectives(false), + }); + } return this.node(start, { kind: Kind.FRAGMENT_SPREAD, - name: this.parseFragmentName(), + name, directives: this.parseDirectives(false), }); } @@ -576,17 +612,14 @@ export class Parser { /** * FragmentDefinition : - * - fragment FragmentName on TypeCondition Directives? SelectionSet + * - fragment FragmentName VariableDefinitions? on TypeCondition Directives? SelectionSet * * TypeCondition : NamedType */ parseFragmentDefinition(): FragmentDefinitionNode { const start = this._lexer.token; this.expectKeyword('fragment'); - // Legacy support for defining variables within fragments changes - // the grammar of FragmentDefinition: - // - fragment FragmentName VariableDefinitions? on TypeCondition Directives? SelectionSet - if (this._options.allowLegacyFragmentVariables === true) { + if (this._options.experimentalFragmentArguments === true) { return this.node(start, { kind: Kind.FRAGMENT_DEFINITION, name: this.parseFragmentName(), diff --git a/src/language/printer.ts b/src/language/printer.ts index 28bb25da17..9889e41bd8 100644 --- a/src/language/printer.ts +++ b/src/language/printer.ts @@ -64,14 +64,9 @@ const printDocASTReducer: ASTReducer = { selectionSet, }) { const prefix = join([wrap('', alias, ': '), name], ''); - let argsLine = prefix + wrap('(', join(args, ', '), ')'); - - if (argsLine.length > MAX_LINE_LENGTH) { - argsLine = prefix + wrap('(\n', indent(join(args, '\n')), '\n)'); - } return join([ - argsLine, + wrappedLineAndArgs(prefix, args), // Note: Client Controlled Nullability is experimental and may be // changed or removed in the future. nullabilityAssertion, @@ -81,6 +76,7 @@ const printDocASTReducer: ASTReducer = { }, }, Argument: { leave: ({ name, value }) => name + ': ' + value }, + FragmentArgument: { leave: ({ name, value }) => name + ': ' + value }, // Nullability Modifiers @@ -105,8 +101,12 @@ const printDocASTReducer: ASTReducer = { // Fragments FragmentSpread: { - leave: ({ name, directives }) => - '...' + name + wrap(' ', join(directives, ' ')), + leave: ({ name, arguments: args, directives }) => { + const prefix = '...' + name; + return ( + wrappedLineAndArgs(prefix, args) + wrap(' ', join(directives, ' ')) + ); + }, }, InlineFragment: { @@ -392,3 +392,15 @@ function hasMultilineItems(maybeArray: Maybe>): boolean { /* c8 ignore next */ return maybeArray?.some((str) => str.includes('\n')) ?? false; } + +function wrappedLineAndArgs( + prefix: string, + args: ReadonlyArray | undefined, +): string { + let argsLine = prefix + wrap('(', join(args, ', '), ')'); + + if (argsLine.length > MAX_LINE_LENGTH) { + argsLine = prefix + wrap('(\n', indent(join(args, '\n')), '\n)'); + } + return argsLine; +} diff --git a/src/type/__tests__/introspection-test.ts b/src/type/__tests__/introspection-test.ts index 3ff904a4a8..c914570713 100644 --- a/src/type/__tests__/introspection-test.ts +++ b/src/type/__tests__/introspection-test.ts @@ -859,6 +859,11 @@ describe('Introspection', () => { isDeprecated: false, deprecationReason: null, }, + { + name: 'FRAGMENT_VARIABLE_DEFINITION', + isDeprecated: false, + deprecationReason: null, + }, { name: 'SCHEMA', isDeprecated: false, diff --git a/src/type/introspection.ts b/src/type/introspection.ts index 5f3c9c1fa5..6d5c29dd90 100644 --- a/src/type/introspection.ts +++ b/src/type/introspection.ts @@ -155,7 +155,11 @@ export const __DirectiveLocation: GraphQLEnumType = new GraphQLEnumType({ }, VARIABLE_DEFINITION: { value: DirectiveLocation.VARIABLE_DEFINITION, - description: 'Location adjacent to a variable definition.', + description: 'Location adjacent to an operation variable definition.', + }, + FRAGMENT_VARIABLE_DEFINITION: { + value: DirectiveLocation.FRAGMENT_VARIABLE_DEFINITION, + description: 'Location adjacent to a fragment variable definition.', }, SCHEMA: { value: DirectiveLocation.SCHEMA, diff --git a/src/utilities/TypeInfo.ts b/src/utilities/TypeInfo.ts index ffcc964b4a..2fd4765c30 100644 --- a/src/utilities/TypeInfo.ts +++ b/src/utilities/TypeInfo.ts @@ -1,6 +1,12 @@ import type { Maybe } from '../jsutils/Maybe.js'; -import type { ASTNode, FieldNode } from '../language/ast.js'; +import type { + ASTNode, + DocumentNode, + FieldNode, + FragmentDefinitionNode, + VariableDefinitionNode, +} from '../language/ast.js'; import { isNode } from '../language/ast.js'; import { Kind } from '../language/kinds.js'; import type { ASTVisitor } from '../language/visitor.js'; @@ -32,6 +38,11 @@ import type { GraphQLSchema } from '../type/schema.js'; import { typeFromAST } from './typeFromAST.js'; +export interface FragmentSignature { + readonly definition: FragmentDefinitionNode; + readonly variableDefinitions: Map; +} + /** * 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 @@ -47,6 +58,12 @@ export class TypeInfo { private _directive: Maybe; private _argument: Maybe; private _enumValue: Maybe; + private _fragmentSignaturesByName: ( + fragmentName: string, + ) => Maybe; + + private _fragmentSignature: Maybe; + private _fragmentArgument: Maybe; private _getFieldDef: GetFieldDefFn; constructor( @@ -58,7 +75,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 = []; @@ -69,6 +89,9 @@ export class TypeInfo { this._directive = null; this._argument = null; this._enumValue = null; + this._fragmentSignaturesByName = fragmentSignatures ?? (() => null); + this._fragmentSignature = null; + this._fragmentArgument = null; this._getFieldDef = getFieldDefFn ?? getFieldDef; if (initialType) { if (isInputType(initialType)) { @@ -119,6 +142,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; } @@ -130,6 +167,12 @@ 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: { + const fragmentSignatures = getFragmentSignatures(node); + this._fragmentSignaturesByName = (fragmentName: string) => + fragmentSignatures.get(fragmentName); + break; + } case Kind.SELECTION_SET: { const namedType: unknown = getNamedType(this.getType()); this._parentTypeStack.push( @@ -159,6 +202,12 @@ export class TypeInfo { this._typeStack.push(isObjectType(rootType) ? rootType : undefined); break; } + case Kind.FRAGMENT_SPREAD: { + this._fragmentSignature = this.getFragmentSignatureByName()( + node.name.value, + ); + break; + } case Kind.INLINE_FRAGMENT: case Kind.FRAGMENT_DEFINITION: { const typeConditionAST = node.typeCondition; @@ -192,6 +241,19 @@ export class TypeInfo { this._inputTypeStack.push(isInputType(argType) ? argType : undefined); break; } + case Kind.FRAGMENT_ARGUMENT: { + const fragmentSignature = this.getFragmentSignature(); + const argDef = fragmentSignature?.variableDefinitions.get( + 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.LIST: { const listType: unknown = getNullableType(this.getInputType()); const itemType: unknown = isListType(listType) @@ -236,6 +298,10 @@ export class TypeInfo { leave(node: ASTNode) { switch (node.kind) { + case Kind.DOCUMENT: + this._fragmentSignaturesByName = /* c8 ignore start */ () => + null /* c8 ignore end */; + break; case Kind.SELECTION_SET: this._parentTypeStack.pop(); break; @@ -246,6 +312,9 @@ export class TypeInfo { case Kind.DIRECTIVE: this._directive = null; break; + case Kind.FRAGMENT_SPREAD: + this._fragmentSignature = null; + break; case Kind.OPERATION_DEFINITION: case Kind.INLINE_FRAGMENT: case Kind.FRAGMENT_DEFINITION: @@ -259,6 +328,12 @@ export class TypeInfo { this._defaultValueStack.pop(); this._inputTypeStack.pop(); break; + case Kind.FRAGMENT_ARGUMENT: { + this._fragmentArgument = null; + this._defaultValueStack.pop(); + this._inputTypeStack.pop(); + break; + } case Kind.LIST: case Kind.OBJECT_FIELD: this._defaultValueStack.pop(); @@ -287,6 +362,25 @@ function getFieldDef( return schema.getField(parentType, fieldNode.name.value); } +function getFragmentSignatures( + document: DocumentNode, +): Map { + const fragmentSignatures = new Map(); + for (const definition of document.definitions) { + if (definition.kind === Kind.FRAGMENT_DEFINITION) { + const variableDefinitions = new Map(); + if (definition.variableDefinitions) { + for (const varDef of definition.variableDefinitions) { + variableDefinitions.set(varDef.variable.name.value, varDef); + } + } + const signature = { definition, variableDefinitions }; + fragmentSignatures.set(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 971316f8b4..cc129287c3 100644 --- a/src/utilities/__tests__/TypeInfo-test.ts +++ b/src/utilities/__tests__/TypeInfo-test.ts @@ -68,6 +68,9 @@ describe('TypeInfo', () => { expect(typeInfo.getFieldDef()).to.equal(undefined); expect(typeInfo.getDefaultValue()).to.equal(undefined); expect(typeInfo.getDirective()).to.equal(null); + expect(typeInfo.getFragmentSignature()).to.equal(null); + expect(typeInfo.getFragmentSignatureByName()('')).to.equal(null); + expect(typeInfo.getFragmentArgument()).to.equal(null); expect(typeInfo.getArgument()).to.equal(null); expect(typeInfo.getEnumValue()).to.equal(null); }); @@ -515,4 +518,221 @@ 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) + ...Bar + } + fragment Foo( + $x: ID! + ) on QueryRoot { + human(id: $x) { name } + } + `, + { experimentalFragmentArguments: true }, + ); + + 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', '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', 'FragmentArgument', null, 'QueryRoot', 'ID!'], + ['leave', 'FragmentSpread', null, 'QueryRoot', 'undefined'], + ['enter', 'FragmentSpread', null, 'QueryRoot', 'undefined'], + ['enter', 'Name', 'Bar', 'QueryRoot', 'undefined'], + ['leave', 'Name', 'Bar', 'QueryRoot', 'undefined'], + ['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'], + ]); + }); + + it('supports traversals of fragment arguments with default-value', () => { + const typeInfo = new TypeInfo(testSchema); + + const ast = parse( + ` + query { + ...Foo(x: null) + } + fragment Foo( + $x: ID = 4 + ) on QueryRoot { + human(id: $x) { name } + } + `, + { experimentalFragmentArguments: true }, + ); + + 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', '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', 'FragmentArgument', 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', 'NamedType', null, 'QueryRoot', 'ID'], + ['enter', 'Name', 'ID', 'QueryRoot', 'ID'], + ['leave', 'Name', 'ID', 'QueryRoot', 'ID'], + ['leave', 'NamedType', null, 'QueryRoot', 'ID'], + ['enter', 'IntValue', null, 'QueryRoot', 'ID'], + ['leave', 'IntValue', 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/__tests__/printSchema-test.ts b/src/utilities/__tests__/printSchema-test.ts index 78f793b183..ac9002e388 100644 --- a/src/utilities/__tests__/printSchema-test.ts +++ b/src/utilities/__tests__/printSchema-test.ts @@ -971,9 +971,12 @@ describe('Type System Printer', () => { """Location adjacent to an inline fragment.""" INLINE_FRAGMENT - """Location adjacent to a variable definition.""" + """Location adjacent to an operation variable definition.""" VARIABLE_DEFINITION + """Location adjacent to a fragment variable definition.""" + FRAGMENT_VARIABLE_DEFINITION + """Location adjacent to a schema definition.""" SCHEMA diff --git a/src/utilities/buildASTSchema.ts b/src/utilities/buildASTSchema.ts index 09fd7cd7dd..5be0b6e421 100644 --- a/src/utilities/buildASTSchema.ts +++ b/src/utilities/buildASTSchema.ts @@ -93,7 +93,7 @@ export function buildSchema( ): GraphQLSchema { const document = parse(source, { noLocation: options?.noLocation, - allowLegacyFragmentVariables: options?.allowLegacyFragmentVariables, + experimentalFragmentArguments: options?.experimentalFragmentArguments, }); return buildASTSchema(document, { diff --git a/src/utilities/valueFromAST.ts b/src/utilities/valueFromAST.ts index 5e0d3c517e..3aec3f272f 100644 --- a/src/utilities/valueFromAST.ts +++ b/src/utilities/valueFromAST.ts @@ -38,6 +38,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. @@ -47,11 +48,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. } @@ -65,7 +67,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) { @@ -78,7 +80,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)) { @@ -86,7 +88,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. } @@ -95,7 +102,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. } @@ -112,7 +124,10 @@ export function valueFromAST( ); for (const field of Object.values(type.getFields())) { const fieldNode = fieldNodes.get(field.name); - if (fieldNode == null || isMissingVariable(fieldNode.value, variables)) { + if ( + fieldNode == null || + isMissingVariable(fieldNode.value, variables, fragmentVariables) + ) { if (field.defaultValue !== undefined) { coercedObj[field.name] = field.defaultValue; } else if (isNonNullType(field.type)) { @@ -120,7 +135,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. } @@ -166,9 +186,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 b0c5524fa7..7f881fc369 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,6 +27,7 @@ import type { import type { GraphQLDirective } from '../type/directives.js'; import type { GraphQLSchema } from '../type/schema.js'; +import type { FragmentSignature } from '../utilities/TypeInfo.js'; import { TypeInfo, visitWithTypeInfo } from '../utilities/TypeInfo.js'; type NodeWithSelectionSet = OperationDefinitionNode | FragmentDefinitionNode; @@ -33,6 +35,7 @@ interface VariableUsage { readonly node: VariableNode; readonly type: Maybe; readonly defaultValue: Maybe; + readonly fragmentVariableDefinition: Maybe; } /** @@ -199,17 +202,41 @@ export class ValidationContext extends ASTValidationContext { let usages = this._variableUsages.get(node); if (!usages) { const newUsages: Array = []; - const typeInfo = new TypeInfo(this._schema); + 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) { - newUsages.push({ - node: variable, - type: typeInfo.getInputType(), - defaultValue: typeInfo.getDefaultValue(), - }); + let fragmentVariableDefinition; + if (fragmentDefinition) { + const fragmentSignature = typeInfo.getFragmentSignatureByName()( + fragmentDefinition.name.value, + ); + + fragmentVariableDefinition = + fragmentSignature?.variableDefinitions.get(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, + }); + } }, }), ); @@ -261,6 +288,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 0fcffeca2c..28e3b564cb 100644 --- a/src/validation/__tests__/KnownArgumentNamesRule-test.ts +++ b/src/validation/__tests__/KnownArgumentNamesRule-test.ts @@ -100,6 +100,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(` { @@ -148,6 +161,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__/KnownDirectivesRule-test.ts b/src/validation/__tests__/KnownDirectivesRule-test.ts index a3bbc198da..2cc8c44ede 100644 --- a/src/validation/__tests__/KnownDirectivesRule-test.ts +++ b/src/validation/__tests__/KnownDirectivesRule-test.ts @@ -44,6 +44,7 @@ const schemaWithDirectives = buildSchema(` directive @onFragmentSpread on FRAGMENT_SPREAD directive @onInlineFragment on INLINE_FRAGMENT directive @onVariableDefinition on VARIABLE_DEFINITION + directive @onFragmentVariableDefinition on FRAGMENT_VARIABLE_DEFINITION `); const schemaWithSDLDirectives = buildSchema(` @@ -150,7 +151,9 @@ describe('Validate: Known directives', () => { someField @onField } - fragment Frag on Human @onFragmentDefinition { + fragment Frag( + $arg: Int @onFragmentVariableDefinition + ) on Human @onFragmentDefinition { name @onField } `); @@ -175,7 +178,7 @@ describe('Validate: Known directives', () => { someField @onQuery } - fragment Frag on Human @onQuery { + fragment Frag($arg: Int @onVariableDefinition) on Human @onQuery { name @onQuery } `).toDeepEqual([ @@ -219,9 +222,14 @@ describe('Validate: Known directives', () => { message: 'Directive "@onQuery" may not be used on FIELD.', locations: [{ column: 19, line: 16 }], }, + { + message: + 'Directive "@onVariableDefinition" may not be used on FRAGMENT_VARIABLE_DEFINITION.', + locations: [{ column: 31, line: 19 }], + }, { message: 'Directive "@onQuery" may not be used on FRAGMENT_DEFINITION.', - locations: [{ column: 30, line: 19 }], + locations: [{ column: 63, line: 19 }], }, { message: 'Directive "@onQuery" may not be used on FIELD.', 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__/NoUnusedVariablesRule-test.ts b/src/validation/__tests__/NoUnusedVariablesRule-test.ts index 47dac39c99..49076361be 100644 --- a/src/validation/__tests__/NoUnusedVariablesRule-test.ts +++ b/src/validation/__tests__/NoUnusedVariablesRule-test.ts @@ -230,4 +230,42 @@ 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(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 ecb56a15cf..7d7c54f37b 100644 --- a/src/validation/__tests__/OverlappingFieldsCanBeMergedRule-test.ts +++ b/src/validation/__tests__/OverlappingFieldsCanBeMergedRule-test.ts @@ -1264,4 +1264,239 @@ 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('allows operations with overlapping fields with arguments using identical operation variables', () => { + expectValid(` + query ($y: Int = 1) { + a(x: $y) + ...WithArgs(x: 1) + } + fragment WithArgs($x: Int = 1) on Type { + a(x: $y) + } + `); + }); + + 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) + } + fragment WithArgs($x: Int) on Type { + a(x: $x) + } + `); + }); + + 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}) + } + `); + }); + + 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: 13, column: 13 }, + { line: 12, column: 11 }, + { line: 11, 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: + 'Spreads "WithArgs" conflict because WithArgs(x: 3) and WithArgs(x: 4) have different fragment arguments.', + locations: [ + { + column: 15, + line: 5, + }, + { + column: 15, + line: 13, + }, + ], + }, + ]); + }); + }); }); diff --git a/src/validation/__tests__/ProvidedRequiredArgumentsRule-test.ts b/src/validation/__tests__/ProvidedRequiredArgumentsRule-test.ts index 3b2c71acf1..95953fc37f 100644 --- a/src/validation/__tests__/ProvidedRequiredArgumentsRule-test.ts +++ b/src/validation/__tests__/ProvidedRequiredArgumentsRule-test.ts @@ -356,4 +356,83 @@ describe('Validate: Provided required arguments', () => { ]); }); }); + + describe('Fragment required arguments', () => { + it('ignores unknown arguments', () => { + expectValid(` + { + ...Foo(unknownArgument: true) + } + fragment Foo on Query { + dog + } + `); + }); + + it('Missing nullable argument with default is allowed', () => { + expectValid(` + { + ...F + } + fragment F($x: Int = 3) on Query { + foo + } + `); + }); + 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 "Int!" is required, but it was not provided.', + locations: [{ line: 3, column: 13 }], + }, + ]); + }); + + 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 cd71952f53..819d103e6a 100644 --- a/src/validation/__tests__/ValuesOfCorrectTypeRule-test.ts +++ b/src/validation/__tests__/ValuesOfCorrectTypeRule-test.ts @@ -1284,4 +1284,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/__tests__/harness.ts b/src/validation/__tests__/harness.ts index b7710ff9d9..0db861f45b 100644 --- a/src/validation/__tests__/harness.ts +++ b/src/validation/__tests__/harness.ts @@ -129,7 +129,7 @@ export function expectValidationErrorsWithSchema( rule: ValidationRule, queryStr: string, ): any { - const doc = parse(queryStr); + const doc = parse(queryStr, { experimentalFragmentArguments: true }); const errors = validate(schema, doc, [rule]); return expectJSON(errors); } diff --git a/src/validation/rules/KnownArgumentNamesRule.ts b/src/validation/rules/KnownArgumentNamesRule.ts index 01046436b4..9e88d5a99d 100644 --- a/src/validation/rules/KnownArgumentNamesRule.ts +++ b/src/validation/rules/KnownArgumentNamesRule.ts @@ -26,6 +26,30 @@ 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.get( + argNode.name.value, + ); + if (!varDef) { + const argName = argNode.name.value; + const suggestions = suggestionList( + argName, + Array.from(fragmentSignature.variableDefinitions.values()).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 +57,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}.${fieldDef.name}".` + diff --git a/src/validation/rules/KnownDirectivesRule.ts b/src/validation/rules/KnownDirectivesRule.ts index a2c7ec81eb..9a1e87d029 100644 --- a/src/validation/rules/KnownDirectivesRule.ts +++ b/src/validation/rules/KnownDirectivesRule.ts @@ -89,8 +89,13 @@ function getDirectiveLocationForASTPath( return DirectiveLocation.INLINE_FRAGMENT; case Kind.FRAGMENT_DEFINITION: return DirectiveLocation.FRAGMENT_DEFINITION; - case Kind.VARIABLE_DEFINITION: - return DirectiveLocation.VARIABLE_DEFINITION; + case Kind.VARIABLE_DEFINITION: { + const parentNode = ancestors[ancestors.length - 3]; + invariant('kind' in parentNode); + return parentNode.kind === Kind.OPERATION_DEFINITION + ? DirectiveLocation.VARIABLE_DEFINITION + : DirectiveLocation.FRAGMENT_VARIABLE_DEFINITION; + } case Kind.SCHEMA_DEFINITION: case Kind.SCHEMA_EXTENSION: return DirectiveLocation.SCHEMA; diff --git a/src/validation/rules/NoUndefinedVariablesRule.ts b/src/validation/rules/NoUndefinedVariablesRule.ts index d1672ecd0b..ae010cab8e 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, fragmentVariableDefinition } of usages) { + if (fragmentVariableDefinition) { + continue; + } const varName = node.name.value; if (!variableNameDefined.has(varName)) { context.reportError( diff --git a/src/validation/rules/NoUnusedVariablesRule.ts b/src/validation/rules/NoUnusedVariablesRule.ts index 7a0660cce0..e55e1c16ed 100644 --- a/src/validation/rules/NoUnusedVariablesRule.ts +++ b/src/validation/rules/NoUnusedVariablesRule.ts @@ -14,18 +14,42 @@ import type { ValidationContext } from '../ValidationContext.js'; */ export function NoUnusedVariablesRule(context: ValidationContext): ASTVisitor { return { - OperationDefinition(operation) { - const usages = context.getRecursiveVariableUsages(operation); - const variableNameUsed = new Set( + 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); + } + } // 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 (!variableNameUsed.has(variableName)) { + if (!operationVariableNameUsed.has(variableName)) { context.reportError( new GraphQLError( operation.name diff --git a/src/validation/rules/OverlappingFieldsCanBeMergedRule.ts b/src/validation/rules/OverlappingFieldsCanBeMergedRule.ts index 851f7ea625..dddb66c07d 100644 --- a/src/validation/rules/OverlappingFieldsCanBeMergedRule.ts +++ b/src/validation/rules/OverlappingFieldsCanBeMergedRule.ts @@ -4,9 +4,12 @@ import type { Maybe } from '../../jsutils/Maybe.js'; import { GraphQLError } from '../../error/GraphQLError.js'; import type { + ArgumentNode, DirectiveNode, FieldNode, + FragmentArgumentNode, FragmentDefinitionNode, + FragmentSpreadNode, SelectionSetNode, ValueNode, } from '../../language/ast.js'; @@ -67,16 +70,16 @@ 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, @@ -107,8 +110,16 @@ type NodeAndDef = [ ]; // Map of array of those. type NodeAndDefCollection = Map>; -type FragmentNames = ReadonlyArray; -type FieldsAndFragmentNames = readonly [NodeAndDefCollection, FragmentNames]; +interface FragmentSpread { + key: string; + node: FragmentSpreadNode; + varMap: Map | undefined; +} +type FragmentSpreads = ReadonlyArray; +type FieldsAndFragmentSpreads = readonly [ + NodeAndDefCollection, + FragmentSpreads, +]; /** * Algorithm: @@ -170,56 +181,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, fragmentSpreads] = getFieldsAndFragmentSpreads( context, - cachedFieldsAndFragmentNames, + cachedFieldsAndFragmentSpreads, parentType, selectionSet, + undefined, ); - // (A) Find find all conflicts "within" the fields of this selection set. + // (A) Find find all conflicts "within" the fields and f of this selection set. // Note: this is the *only place* `collectConflictsWithin` is called. collectConflictsWithin( context, conflicts, - cachedFieldsAndFragmentNames, + cachedFieldsAndFragmentSpreads, comparedFragmentPairs, fieldMap, ); - if (fragmentNames.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 < fragmentNames.length; i++) { + // each spread found. + for (let i = 0; i < fragmentSpreads.length; i++) { collectConflictsBetweenFieldsAndFragment( context, conflicts, - cachedFieldsAndFragmentNames, + cachedFieldsAndFragmentSpreads, comparedFragmentPairs, false, fieldMap, - fragmentNames[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 + // This compares each item in the list of fragment spreads to every other // item in that same list (except for itself). - for (let j = i + 1; j < fragmentNames.length; j++) { + for (let j = i + 1; j < fragmentSpreads.length; j++) { collectConflictsBetweenFragments( context, conflicts, - cachedFieldsAndFragmentNames, + cachedFieldsAndFragmentSpreads, comparedFragmentPairs, false, - fragmentNames[i], - fragmentNames[j], + fragmentSpreads[i], + fragmentSpreads[j], ); } } @@ -232,22 +247,26 @@ function findConflictsWithinSelectionSet( function collectConflictsBetweenFieldsAndFragment( context: ValidationContext, conflicts: Array, - cachedFieldsAndFragmentNames: Map, + cachedFieldsAndFragmentSpreads: Map< + SelectionSetNode, + FieldsAndFragmentSpreads + >, comparedFragmentPairs: PairSet, areMutuallyExclusive: boolean, fieldMap: NodeAndDefCollection, - fragmentName: string, + fragmentSpread: FragmentSpread, ): void { - const fragment = context.getFragment(fragmentName); + const fragment = context.getFragment(fragmentSpread.node.name.value); if (!fragment) { return; } - const [fieldMap2, referencedFragmentNames] = - getReferencedFieldsAndFragmentNames( + const [fieldMap2, referencedFragmentSpreads] = + getReferencedFieldsAndFragmentSpreads( context, - cachedFieldsAndFragmentNames, + cachedFieldsAndFragmentSpreads, fragment, + fragmentSpread.varMap, ); // Do not compare a fragment's fieldMap to itself. @@ -260,40 +279,42 @@ function collectConflictsBetweenFieldsAndFragment( collectConflictsBetween( context, conflicts, - cachedFieldsAndFragmentNames, + cachedFieldsAndFragmentSpreads, 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 of referencedFragmentNames) { + // and any fragment spreads found in the given fragment. + for (const referencedFragmentSpread of referencedFragmentSpreads) { // 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, ); collectConflictsBetweenFieldsAndFragment( context, conflicts, - cachedFieldsAndFragmentNames, + cachedFieldsAndFragmentSpreads, comparedFragmentPairs, areMutuallyExclusive, fieldMap, - referencedFragmentName, + referencedFragmentSpread, ); } } @@ -303,46 +324,74 @@ function collectConflictsBetweenFieldsAndFragment( function collectConflictsBetweenFragments( context: ValidationContext, conflicts: Array, - cachedFieldsAndFragmentNames: Map, + cachedFieldsAndFragmentSpreads: Map< + SelectionSetNode, + FieldsAndFragmentSpreads + >, comparedFragmentPairs: PairSet, areMutuallyExclusive: boolean, - fragmentName1: string, - fragmentName2: string, + fragmentSpread1: FragmentSpread, + fragmentSpread2: FragmentSpread, ): void { // No need to compare a fragment to itself. - if (fragmentName1 === fragmentName2) { + if (fragmentSpread1.key === fragmentSpread2.key) { 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; + } + } + // Memoize so two fragments are not compared for conflicts more than once. if ( comparedFragmentPairs.has( - fragmentName1, - fragmentName2, + fragmentSpread1.key, + fragmentSpread2.key, areMutuallyExclusive, ) ) { return; } - comparedFragmentPairs.add(fragmentName1, fragmentName2, 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; } - const [fieldMap1, referencedFragmentNames1] = - getReferencedFieldsAndFragmentNames( + const [fieldMap1, referencedFragmentSpreads1] = + getReferencedFieldsAndFragmentSpreads( context, - cachedFieldsAndFragmentNames, + cachedFieldsAndFragmentSpreads, fragment1, + fragmentSpread1.varMap, ); - const [fieldMap2, referencedFragmentNames2] = - getReferencedFieldsAndFragmentNames( + const [fieldMap2, referencedFragmentSpreads2] = + getReferencedFieldsAndFragmentSpreads( context, - cachedFieldsAndFragmentNames, + cachedFieldsAndFragmentSpreads, fragment2, + fragmentSpread2.varMap, ); // (F) First, collect all conflicts between these two collections of fields @@ -350,38 +399,40 @@ function collectConflictsBetweenFragments( collectConflictsBetween( context, conflicts, - cachedFieldsAndFragmentNames, + cachedFieldsAndFragmentSpreads, 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 referencedFragmentName2 of referencedFragmentNames2) { + for (const referencedFragmentSpread2 of 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 referencedFragmentSpreads1) { collectConflictsBetweenFragments( context, conflicts, - cachedFieldsAndFragmentNames, + cachedFieldsAndFragmentSpreads, comparedFragmentPairs, areMutuallyExclusive, - referencedFragmentName1, - fragmentName2, + referencedFragmentSpread1, + fragmentSpread2, ); } } @@ -391,81 +442,90 @@ 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, selectionSet1: SelectionSetNode, + varMap1: Map | undefined, parentType2: Maybe, selectionSet2: SelectionSetNode, + varMap2: Map | undefined, ): Array { const conflicts: Array = []; - const [fieldMap1, fragmentNames1] = getFieldsAndFragmentNames( + const [fieldMap1, fragmentSpreads1] = getFieldsAndFragmentSpreads( context, - cachedFieldsAndFragmentNames, + cachedFieldsAndFragmentSpreads, parentType1, selectionSet1, + varMap1, ); - const [fieldMap2, fragmentNames2] = getFieldsAndFragmentNames( + const [fieldMap2, fragmentSpreads2] = getFieldsAndFragmentSpreads( context, - cachedFieldsAndFragmentNames, + cachedFieldsAndFragmentSpreads, parentType2, selectionSet2, + varMap2, ); // (H) First, collect all conflicts between these two collections of field. collectConflictsBetween( context, conflicts, - cachedFieldsAndFragmentNames, + cachedFieldsAndFragmentSpreads, 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 fragmentName2 of fragmentNames2) { + for (const fragmentSpread2 of 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) { + for (const fragmentSpread1 of 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 fragmentSpreads1) { + for (const fragmentSpread2 of fragmentSpreads2) { collectConflictsBetweenFragments( context, conflicts, - cachedFieldsAndFragmentNames, + cachedFieldsAndFragmentSpreads, comparedFragmentPairs, areMutuallyExclusive, - fragmentName1, - fragmentName2, + fragmentSpread1, + fragmentSpread2, ); } } @@ -476,7 +536,10 @@ function findConflictsBetweenSubSelectionSets( function collectConflictsWithin( context: ValidationContext, conflicts: Array, - cachedFieldsAndFragmentNames: Map, + cachedFieldsAndFragmentSpreads: Map< + SelectionSetNode, + FieldsAndFragmentSpreads + >, comparedFragmentPairs: PairSet, fieldMap: NodeAndDefCollection, ): void { @@ -493,12 +556,14 @@ function collectConflictsWithin( for (let j = i + 1; j < fields.length; j++) { const conflict = findConflict( context, - cachedFieldsAndFragmentNames, + cachedFieldsAndFragmentSpreads, comparedFragmentPairs, false, // within one collection is never mutually exclusive responseName, fields[i], + undefined, fields[j], + undefined, ); if (conflict) { conflicts.push(conflict); @@ -517,11 +582,16 @@ function collectConflictsWithin( function collectConflictsBetween( context: ValidationContext, conflicts: Array, - cachedFieldsAndFragmentNames: Map, + cachedFieldsAndFragmentSpreads: Map< + SelectionSetNode, + FieldsAndFragmentSpreads + >, 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 @@ -535,12 +605,14 @@ function collectConflictsBetween( for (const field2 of fields2) { const conflict = findConflict( context, - cachedFieldsAndFragmentNames, + cachedFieldsAndFragmentSpreads, comparedFragmentPairs, parentFieldsAreMutuallyExclusive, responseName, field1, + varMap1, field2, + varMap2, ); if (conflict) { conflicts.push(conflict); @@ -555,12 +627,17 @@ function collectConflictsBetween( // comparing their sub-fields. function findConflict( context: ValidationContext, - cachedFieldsAndFragmentNames: Map, + cachedFieldsAndFragmentSpreads: Map< + SelectionSetNode, + FieldsAndFragmentSpreads + >, comparedFragmentPairs: PairSet, 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; @@ -592,7 +669,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], @@ -604,7 +681,7 @@ function findConflict( // FIXME https://github.com/graphql/graphql-js/issues/2203 const directives1 = /* c8 ignore next */ node1.directives ?? []; const directives2 = /* c8 ignore next */ node2.directives ?? []; - if (!sameStreams(directives1, directives2)) { + if (!sameStreams(directives1, varMap1, directives2, varMap2)) { return [ [responseName, 'they have differing stream directives'], [node1], @@ -629,7 +706,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; @@ -637,25 +714,26 @@ function findConflict( if (selectionSet1 && selectionSet2) { const conflicts = findConflictsBetweenSubSelectionSets( context, - cachedFieldsAndFragmentNames, + cachedFieldsAndFragmentSpreads, comparedFragmentPairs, areMutuallyExclusive, getNamedType(type1), selectionSet1, + varMap1, getNamedType(type2), selectionSet2, + varMap2, ); return subfieldConflicts(conflicts, responseName, node1, node2); } } -function sameArguments( - node1: FieldNode | DirectiveNode, - node2: FieldNode | DirectiveNode, +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; } @@ -667,9 +745,17 @@ function sameArguments( return false; } - 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; @@ -679,6 +765,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)); } @@ -691,7 +805,9 @@ function getStreamDirective( function sameStreams( directives1: ReadonlyArray, + varMap1: Map | undefined, directives2: ReadonlyArray, + varMap2: Map | undefined, ): boolean { const stream1 = getStreamDirective(directives1); const stream2 = getStreamDirective(directives2); @@ -700,7 +816,12 @@ function sameStreams( return true; } else if (stream1 && stream2) { // check if both fields have equivalent streams - return sameArguments(stream1, stream2); + return sameArguments( + stream1.arguments, + varMap1, + stream2.arguments, + varMap2, + ); } // fields have a mix of stream and no stream return false; @@ -736,60 +857,74 @@ 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 +// name to field nodes and definitions) as well as a list of fragment spreads // 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); + varMap: Map | undefined, +): FieldsAndFragmentSpreads { + const cached = cachedFieldsAndFragmentSpreads.get(selectionSet); if (cached) { return cached; } const nodeAndDefs: NodeAndDefCollection = new Map(); - const fragmentNames = new Set(); - _collectFieldsAndFragmentNames( + const fragmentSpreads = new Map(); + _collectFieldsAndFragmentSpreads( context, parentType, selectionSet, nodeAndDefs, - fragmentNames, + fragmentSpreads, + varMap, ); - const result = [nodeAndDefs, [...fragmentNames]] as const; - cachedFieldsAndFragmentNames.set(selectionSet, result); + const result: FieldsAndFragmentSpreads = [ + nodeAndDefs, + Array.from(fragmentSpreads.values()), + ]; + 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 fragment spreads referenced via fragment spreads. +function getReferencedFieldsAndFragmentSpreads( context: ValidationContext, - cachedFieldsAndFragmentNames: Map, + cachedFieldsAndFragmentSpreads: Map< + SelectionSetNode, + FieldsAndFragmentSpreads + >, fragment: FragmentDefinitionNode, + varMap: Map | undefined, ) { // Short-circuit building a type from the node if possible. - const cached = cachedFieldsAndFragmentNames.get(fragment.selectionSet); + const cached = cachedFieldsAndFragmentSpreads.get(fragment.selectionSet); if (cached) { return cached; } const fragmentType = typeFromAST(context.getSchema(), fragment.typeCondition); - return getFieldsAndFragmentNames( + return getFieldsAndFragmentSpreads( context, - cachedFieldsAndFragmentNames, + cachedFieldsAndFragmentSpreads, fragmentType, fragment.selectionSet, + varMap, ); } -function _collectFieldsAndFragmentNames( +function _collectFieldsAndFragmentSpreads( context: ValidationContext, parentType: Maybe, selectionSet: SelectionSetNode, nodeAndDefs: NodeAndDefCollection, - fragmentNames: Set, + fragmentSpreads: Map, + varMap: Map | undefined, ): void { for (const selection of selectionSet.selections) { switch (selection.kind) { @@ -811,20 +946,23 @@ function _collectFieldsAndFragmentNames( nodeAndDefsList.push([parentType, selection, fieldDef]); break; } - case Kind.FRAGMENT_SPREAD: - fragmentNames.add(selection.name.value); + case Kind.FRAGMENT_SPREAD: { + const fragmentSpread = getFragmentSpread(context, selection, varMap); + fragmentSpreads.set(fragmentSpread.key, fragmentSpread); 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, + varMap, ); break; } @@ -832,6 +970,48 @@ 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 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 c42823b2bd..72737746aa 100644 --- a/src/validation/rules/ProvidedRequiredArgumentsRule.ts +++ b/src/validation/rules/ProvidedRequiredArgumentsRule.ts @@ -2,7 +2,10 @@ import { inspect } from '../../jsutils/inspect.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'; @@ -16,6 +19,8 @@ import { import { specifiedDirectives } from '../../type/directives.js'; import { isIntrospectionType } from '../../type/introspection.js'; +import { typeFromAST } from '../../utilities/typeFromAST.js'; + import type { SDLValidationContext, ValidationContext, @@ -69,6 +74,42 @@ export function ProvidedRequiredArgumentsRule( } }, }, + FragmentSpread: { + // Validate on leave to allow for deeper errors to appear first. + leave(spreadNode) { + const fragmentSignature = context.getFragmentSignature(); + if (!fragmentSignature) { + 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), + ); + for (const [ + varName, + variableDefinition, + ] of fragmentSignature.variableDefinitions) { + if ( + !providedArgs.has(varName) && + isRequiredArgumentNode(variableDefinition) + ) { + const type = typeFromAST( + context.getSchema(), + variableDefinition.type, + ); + const argTypeStr = inspect(type); + context.reportError( + new GraphQLError( + `Fragment "${spreadNode.name.value}" argument "${varName}" of type "${argTypeStr}" is required, but it was not provided.`, + { nodes: spreadNode }, + ), + ); + } + } + }, + }, }; } @@ -142,6 +183,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/SingleFieldSubscriptionsRule.ts b/src/validation/rules/SingleFieldSubscriptionsRule.ts index 700bc0bda7..dfefc6cdd8 100644 --- a/src/validation/rules/SingleFieldSubscriptionsRule.ts +++ b/src/validation/rules/SingleFieldSubscriptionsRule.ts @@ -2,15 +2,14 @@ import type { ObjMap } from '../../jsutils/ObjMap.js'; import { GraphQLError } from '../../error/GraphQLError.js'; -import type { - FieldNode, - FragmentDefinitionNode, - OperationDefinitionNode, -} from '../../language/ast.js'; +import type { FieldNode, OperationDefinitionNode } from '../../language/ast.js'; import { Kind } from '../../language/kinds.js'; import type { ASTVisitor } from '../../language/visitor.js'; -import type { FieldGroup } from '../../execution/collectFields.js'; +import type { + FieldGroup, + FragmentDetails, +} from '../../execution/collectFields.js'; import { collectFields } from '../../execution/collectFields.js'; import type { ValidationContext } from '../ValidationContext.js'; @@ -41,10 +40,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 { groupedFieldSet } = collectFields( diff --git a/src/validation/rules/VariablesInAllowedPositionRule.ts b/src/validation/rules/VariablesInAllowedPositionRule.ts index e662ba443c..e68a595c4f 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 } of usages) { + for (const { + node, + type, + defaultValue, + fragmentVariableDefinition, + } of usages) { const varName = node.name.value; - const varDef = varDefMap.get(varName); + + let varDef = fragmentVariableDefinition; + if (!varDef) { + varDef = varDefMap.get(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