From 686fe565c00eb444179607f629a011d5c1a3c19c Mon Sep 17 00:00:00 2001 From: mjmahone Date: Mon, 2 Jan 2023 16:31:24 -0500 Subject: [PATCH] Update all validation rules: tests pass --- src/language/directiveLocation.ts | 1 + src/utilities/__tests__/TypeInfo-test.ts | 107 ++++ .../__tests__/KnownDirectivesRule-test.ts | 14 +- .../NoUnusedFragmentArgumentsRule-test.ts | 120 +++++ src/validation/index.ts | 3 + src/validation/rules/KnownDirectivesRule.ts | 2 + .../rules/NoUndefinedVariablesRule.ts | 5 +- .../rules/NoUnusedFragmentArgumentsRule.ts | 40 ++ src/validation/rules/NoUnusedVariablesRule.ts | 5 +- .../rules/OverlappingFieldsCanBeMergedRule.ts | 487 +++++++++++------- .../rules/ProvidedRequiredArgumentsRule.ts | 40 +- .../rules/VariablesAreInputTypesRule.ts | 37 +- .../rules/VariablesInAllowedPositionRule.ts | 9 +- src/validation/specifiedRules.ts | 3 + 14 files changed, 666 insertions(+), 207 deletions(-) create mode 100644 src/validation/__tests__/NoUnusedFragmentArgumentsRule-test.ts create mode 100644 src/validation/rules/NoUnusedFragmentArgumentsRule.ts diff --git a/src/language/directiveLocation.ts b/src/language/directiveLocation.ts index b10214d47f..3ff959e0c1 100644 --- a/src/language/directiveLocation.ts +++ b/src/language/directiveLocation.ts @@ -11,6 +11,7 @@ export enum DirectiveLocation { FRAGMENT_SPREAD = 'FRAGMENT_SPREAD', INLINE_FRAGMENT = 'INLINE_FRAGMENT', VARIABLE_DEFINITION = 'VARIABLE_DEFINITION', + FRAGMENT_ARGUMENT_DEFINITION = 'FRAGMENT_ARGUMENT_DEFINITION', /** Type System Definitions */ SCHEMA = 'SCHEMA', SCALAR = 'SCALAR', diff --git a/src/utilities/__tests__/TypeInfo-test.ts b/src/utilities/__tests__/TypeInfo-test.ts index 971316f8b4..8c3f9e2986 100644 --- a/src/utilities/__tests__/TypeInfo-test.ts +++ b/src/utilities/__tests__/TypeInfo-test.ts @@ -515,4 +515,111 @@ describe('visitWithTypeInfo', () => { ['leave', 'SelectionSet', null, 'Human', 'Human'], ]); }); + + it('supports traversals of fragment arguments', () => { + const typeInfo = new TypeInfo(testSchema); + + const ast = parse(` + query { + ...Foo(x: 4) + } + + fragment Foo( + "Human to get" + $x: ID! + ) on QueryRoot { + human(id: $x) { name } + } + `); + + const visited: Array = []; + visit( + ast, + visitWithTypeInfo(typeInfo, { + enter(node) { + const type = typeInfo.getType(); + const inputType = typeInfo.getInputType(); + visited.push([ + 'enter', + node.kind, + node.kind === 'Name' ? node.value : null, + String(type), + String(inputType), + ]); + }, + leave(node) { + const type = typeInfo.getType(); + const inputType = typeInfo.getInputType(); + visited.push([ + 'leave', + node.kind, + node.kind === 'Name' ? node.value : null, + String(type), + String(inputType), + ]); + }, + }), + ); + + expect(visited).to.deep.equal([ + ['enter', 'Document', null, 'undefined', 'undefined'], + ['enter', 'OperationDefinition', null, 'QueryRoot', 'undefined'], + ['enter', 'SelectionSet', null, 'QueryRoot', 'undefined'], + ['enter', 'FragmentSpread', null, 'QueryRoot', 'undefined'], + ['enter', 'Name', 'Foo', 'QueryRoot', 'undefined'], + ['leave', 'Name', 'Foo', 'QueryRoot', 'undefined'], + ['enter', 'Argument', null, 'QueryRoot', 'ID!'], + ['enter', 'Name', 'x', 'QueryRoot', 'ID!'], + ['leave', 'Name', 'x', 'QueryRoot', 'ID!'], + ['enter', 'IntValue', null, 'QueryRoot', 'ID!'], + ['leave', 'IntValue', null, 'QueryRoot', 'ID!'], + ['leave', 'Argument', null, 'QueryRoot', 'ID!'], + ['leave', 'FragmentSpread', null, 'QueryRoot', 'undefined'], + ['leave', 'SelectionSet', null, 'QueryRoot', 'undefined'], + ['leave', 'OperationDefinition', null, 'QueryRoot', 'undefined'], + ['enter', 'FragmentDefinition', null, 'QueryRoot', 'undefined'], + ['enter', 'Name', 'Foo', 'QueryRoot', 'undefined'], + ['leave', 'Name', 'Foo', 'QueryRoot', 'undefined'], + ['enter', 'FragmentArgumentDefinition', null, 'QueryRoot', 'ID!'], + ['enter', 'StringValue', null, 'QueryRoot', 'ID!'], + ['leave', 'StringValue', 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', 'FragmentArgumentDefinition', 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/validation/__tests__/KnownDirectivesRule-test.ts b/src/validation/__tests__/KnownDirectivesRule-test.ts index a3bbc198da..f172ce5ca1 100644 --- a/src/validation/__tests__/KnownDirectivesRule-test.ts +++ b/src/validation/__tests__/KnownDirectivesRule-test.ts @@ -41,6 +41,7 @@ const schemaWithDirectives = buildSchema(` directive @onSubscription on SUBSCRIPTION directive @onField on FIELD directive @onFragmentDefinition on FRAGMENT_DEFINITION + directive @onFragmentArgumentDefinition on FRAGMENT_ARGUMENT_DEFINITION directive @onFragmentSpread on FRAGMENT_SPREAD directive @onInlineFragment on INLINE_FRAGMENT directive @onVariableDefinition on VARIABLE_DEFINITION @@ -150,7 +151,9 @@ describe('Validate: Known directives', () => { someField @onField } - fragment Frag on Human @onFragmentDefinition { + fragment Frag( + $arg: Int @onFragmentArgumentDefinition + ) on Human @onFragmentDefinition { name @onField } `); @@ -175,7 +178,7 @@ describe('Validate: Known directives', () => { someField @onQuery } - fragment Frag on Human @onQuery { + fragment Frag($arg: Int @onField) 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 "@onField" may not be used on FRAGMENT_ARGUMENT_DEFINITION.', + locations: [{ column: 31, line: 19 }], + }, { message: 'Directive "@onQuery" may not be used on FRAGMENT_DEFINITION.', - locations: [{ column: 30, line: 19 }], + locations: [{ column: 50, line: 19 }], }, { message: 'Directive "@onQuery" may not be used on FIELD.', diff --git a/src/validation/__tests__/NoUnusedFragmentArgumentsRule-test.ts b/src/validation/__tests__/NoUnusedFragmentArgumentsRule-test.ts new file mode 100644 index 0000000000..9ca54af80c --- /dev/null +++ b/src/validation/__tests__/NoUnusedFragmentArgumentsRule-test.ts @@ -0,0 +1,120 @@ +import { describe, it } from 'mocha'; + +import { NoUnusedFragmentArgumentsRule } from '../rules/NoUnusedFragmentArgumentsRule.js'; + +import { expectValidationErrors } from './harness.js'; + +function expectErrors(queryStr: string) { + return expectValidationErrors(NoUnusedFragmentArgumentsRule, queryStr); +} + +function expectValid(queryStr: string) { + expectErrors(queryStr).toDeepEqual([]); +} + +describe('Validate: No unused fragment arguments', () => { + it('uses all arguments', () => { + expectValid(` + fragment Foo($a: String, $b: String, $c: String) on Type { + field(a: $a, b: $b, c: $c) + } + `); + }); + + it('uses all arguments deeply', () => { + expectValid(` + fragment Foo($a: String, $b: String, $c: String) on Type { + field(a: $a) { + field(b: $b) { + field(c: $c) + } + } + } + `); + }); + + it('uses all arguments deeply in inline fragments', () => { + expectValid(` + fragment Foo($a: String, $b: String, $c: String) on Type { + ... on Type { + field(a: $a) { + field(b: $b) { + ... on Type { + field(c: $c) + } + } + } + } + } + `); + }); + + it('argument not used', () => { + expectErrors(` + fragment Foo($a: String, $b: String, $c: String) on Type { + field(a: $a, b: $b) + } + `).toDeepEqual([ + { + message: 'Argument "$c" is never used in fragment "Foo".', + locations: [{ line: 2, column: 44 }], + }, + ]); + }); + + it('query passes in unused argument', () => { + expectErrors(` + query Q($c: String) { + type { + ...Foo(a: "", b: "", c: $c) + } + } + fragment Foo($a: String, $b: String, $c: String) on Type { + field(a: $a, b: $b) + } + `).toDeepEqual([ + { + message: 'Argument "$c" is never used in fragment "Foo".', + locations: [{ line: 7, column: 44 }], + }, + ]); + }); + + it('child fragment uses a variable of the same name', () => { + expectErrors(` + query Q($a: String) { + type { + ...Foo + } + } + fragment Foo($a: String) on Type { + ...Bar + } + fragment Bar on Type { + field(a: $a) + } + `).toDeepEqual([ + { + message: 'Argument "$a" is never used in fragment "Foo".', + locations: [{ line: 7, column: 20 }], + }, + ]); + }); + + it('multiple arguments not used', () => { + expectErrors(` + fragment Foo($a: String, $b: String, $c: String) on Type { + field(b: $b) + } + `).toDeepEqual([ + { + message: 'Argument "$a" is never used in fragment "Foo".', + locations: [{ line: 2, column: 20 }], + }, + { + message: 'Argument "$c" is never used in fragment "Foo".', + locations: [{ line: 2, column: 44 }], + }, + ]); + }); +}); diff --git a/src/validation/index.ts b/src/validation/index.ts index b0cc754490..b5c4312607 100644 --- a/src/validation/index.ts +++ b/src/validation/index.ts @@ -48,6 +48,9 @@ export { NoUndefinedVariablesRule } from './rules/NoUndefinedVariablesRule.js'; // Spec Section: "Fragments must be used" export { NoUnusedFragmentsRule } from './rules/NoUnusedFragmentsRule.js'; +// Spec Section: "All Fragment Arguments Used" +export { NoUnusedFragmentArgumentsRule } from './rules/NoUnusedFragmentArgumentsRule.js'; + // Spec Section: "All Variables Used" export { NoUnusedVariablesRule } from './rules/NoUnusedVariablesRule.js'; diff --git a/src/validation/rules/KnownDirectivesRule.ts b/src/validation/rules/KnownDirectivesRule.ts index 57641b91e7..daee27a798 100644 --- a/src/validation/rules/KnownDirectivesRule.ts +++ b/src/validation/rules/KnownDirectivesRule.ts @@ -86,6 +86,8 @@ function getDirectiveLocationForASTPath( return DirectiveLocation.INLINE_FRAGMENT; case Kind.FRAGMENT_DEFINITION: return DirectiveLocation.FRAGMENT_DEFINITION; + case Kind.FRAGMENT_ARGUMENT_DEFINITION: + return DirectiveLocation.FRAGMENT_ARGUMENT_DEFINITION; case Kind.VARIABLE_DEFINITION: return DirectiveLocation.VARIABLE_DEFINITION; case Kind.SCHEMA_DEFINITION: diff --git a/src/validation/rules/NoUndefinedVariablesRule.ts b/src/validation/rules/NoUndefinedVariablesRule.ts index d1672ecd0b..8375d82b5e 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, fragmentArgDef } of usages) { + if (fragmentArgDef) { + continue; + } const varName = node.name.value; if (!variableNameDefined.has(varName)) { context.reportError( diff --git a/src/validation/rules/NoUnusedFragmentArgumentsRule.ts b/src/validation/rules/NoUnusedFragmentArgumentsRule.ts new file mode 100644 index 0000000000..1002d4c1d1 --- /dev/null +++ b/src/validation/rules/NoUnusedFragmentArgumentsRule.ts @@ -0,0 +1,40 @@ +import { GraphQLError } from '../../error/GraphQLError.js'; + +import type { ASTVisitor } from '../../language/visitor.js'; + +import type { ValidationContext } from '../ValidationContext.js'; + +/** + * No unused variables + * + * A GraphQL fragment is only valid if all arguments defined by it + * are used within the same fragment. + * + * See https://spec.graphql.org/draft/#sec-All-Variables-Used + */ +export function NoUnusedFragmentArgumentsRule( + context: ValidationContext, +): ASTVisitor { + return { + FragmentDefinition(fragment) { + const usages = context.getVariableUsages(fragment); + const argumentNameUsed = new Set( + usages.map(({ node }) => node.name.value), + ); + // FIXME: https://github.com/graphql/graphql-js/issues/2203 + /* c8 ignore next */ + const argumentDefinitions = fragment.arguments ?? []; + for (const argDef of argumentDefinitions) { + const argName = argDef.variable.name.value; + if (!argumentNameUsed.has(argName)) { + context.reportError( + new GraphQLError( + `Argument "$${argName}" is never used in fragment "${fragment.name.value}".`, + { nodes: argDef }, + ), + ); + } + } + }, + }; +} diff --git a/src/validation/rules/NoUnusedVariablesRule.ts b/src/validation/rules/NoUnusedVariablesRule.ts index 7a0660cce0..7fbeb82764 100644 --- a/src/validation/rules/NoUnusedVariablesRule.ts +++ b/src/validation/rules/NoUnusedVariablesRule.ts @@ -17,7 +17,10 @@ export function NoUnusedVariablesRule(context: ValidationContext): ASTVisitor { OperationDefinition(operation) { const usages = context.getRecursiveVariableUsages(operation); const variableNameUsed = new Set( - usages.map(({ node }) => node.name.value), + usages + // Skip variables used as fragment arguments + .filter(({ fragmentArgDef }) => !fragmentArgDef) + .map(({ node }) => node.name.value), ); // FIXME: https://github.com/graphql/graphql-js/issues/2203 diff --git a/src/validation/rules/OverlappingFieldsCanBeMergedRule.ts b/src/validation/rules/OverlappingFieldsCanBeMergedRule.ts index e2444047c6..d46aae5a6f 100644 --- a/src/validation/rules/OverlappingFieldsCanBeMergedRule.ts +++ b/src/validation/rules/OverlappingFieldsCanBeMergedRule.ts @@ -8,6 +8,7 @@ import type { DirectiveNode, FieldNode, FragmentDefinitionNode, + FragmentSpreadNode, ObjectValueNode, SelectionSetNode, } from '../../language/ast.js'; @@ -29,7 +30,9 @@ import { isObjectType, } from '../../type/definition.js'; +import { keyForFragmentSpread } from '../../utilities/keyForFragmentSpread.js'; import { sortValueNode } from '../../utilities/sortValueNode.js'; +import { substituteFragmentArguments } from '../../utilities/substituteFragmentArguments.js'; import { typeFromAST } from '../../utilities/typeFromAST.js'; import type { ValidationContext } from '../ValidationContext.js'; @@ -38,17 +41,25 @@ import type { ValidationContext } from '../ValidationContext.js'; // This file contains a lot of such errors but we plan to refactor it anyway // so just disable it for entire file. -function reasonMessage(reason: ConflictReasonMessage): string { - if (Array.isArray(reason)) { - return reason - .map( - ([responseName, subReason]) => - `subfields "${responseName}" conflict because ` + - reasonMessage(subReason), - ) +function reasonMessage(message: ConflictReasonMessage): string { + if (Array.isArray(message)) { + return message + .map((subC) => { + const subConflict = subC; + if (subConflict.kind === 'FIELD') { + return ( + `subfields "${subConflict.responseName}" conflict because ` + + reasonMessage(subConflict.reasonMessage) + ); + } + return ( + `child spreads "${subConflict.fragmentName}" conflict because ` + + reasonMessage(subConflict.reasonMessage) + ); + }) .join(' and '); } - return reason; + return message; } /** @@ -68,36 +79,62 @@ export function OverlappingFieldsCanBeMergedRule( // dramatically improve the performance of this validator. const comparedFragmentPairs = new PairSet(); - // A cache for the "field map" and list of fragment names found in any given + // A cache for the "field map" and list of fragment spreads found in any given // selection set. Selection sets may be asked for this information multiple // times, so this improves the performance of this validator. - const cachedFieldsAndFragmentNames = new Map(); + const cachedFieldsAndFragmentSpreads = new Map(); return { SelectionSet(selectionSet) { const conflicts = findConflictsWithinSelectionSet( context, - cachedFieldsAndFragmentNames, + cachedFieldsAndFragmentSpreads, comparedFragmentPairs, context.getParentType(), selectionSet, ); - for (const [[responseName, reason], fields1, fields2] of conflicts) { - const reasonMsg = reasonMessage(reason); - context.reportError( - new GraphQLError( - `Fields "${responseName}" conflict because ${reasonMsg}. Use different aliases on the fields to fetch both if this was intentional.`, - { nodes: fields1.concat(fields2) }, - ), - ); + for (const { reason, selectionPath1, selectionPath2 } of conflicts) { + const reasonMsg = reasonMessage(reason.reasonMessage); + const errorNodes = { nodes: selectionPath1.concat(selectionPath2) }; + if (reason.kind === 'FIELD') { + context.reportError( + new GraphQLError( + `Fields "${reason.responseName}" conflict because ${reasonMsg}. Use different aliases on the fields to fetch both if this was intentional.`, + errorNodes, + ), + ); + } else { + // FRAGMENT_SPREAD + context.reportError( + new GraphQLError( + // Fragments can't be aliased, so there's no easy way to resolve these conflicts today. + `Spreads "${reason.fragmentName}" conflict because ${reasonMsg}.`, + errorNodes, + ), + ); + } } }, }; } -type Conflict = [ConflictReason, Array, Array]; +interface Conflict { + reason: ConflictReason; + selectionPath1: Array; + selectionPath2: Array; +} // Field name and reason. -type ConflictReason = [string, ConflictReasonMessage]; +type ConflictReason = FieldConflictReason | FragmentSpreadConflictReason; +interface FieldConflictReason { + kind: 'FIELD'; + responseName: string; + reasonMessage: ConflictReasonMessage; +} +interface FragmentSpreadConflictReason { + kind: 'FRAGMENT_SPREAD'; + fragmentName: string; + reasonMessage: ConflictReasonMessage; +} // Reason is a string, or a nested list of conflicts. type ConflictReasonMessage = string | Array; // Tuple defining a field node in a context. @@ -108,8 +145,11 @@ type NodeAndDef = [ ]; // Map of array of those. type NodeAndDefCollection = ObjMap>; -type FragmentNames = ReadonlyArray; -type FieldsAndFragmentNames = readonly [NodeAndDefCollection, FragmentNames]; +type FragmentSpreadsByKey = ObjMap; +type FieldsAndFragmentSpreads = readonly [ + NodeAndDefCollection, + FragmentSpreadsByKey, +]; /** * Algorithm: @@ -171,58 +211,60 @@ type FieldsAndFragmentNames = readonly [NodeAndDefCollection, FragmentNames]; // GraphQL Document. function findConflictsWithinSelectionSet( context: ValidationContext, - cachedFieldsAndFragmentNames: Map, + cachedFieldsAndFragmentSpreads: Map< + SelectionSetNode, + FieldsAndFragmentSpreads + >, comparedFragmentPairs: PairSet, parentType: Maybe, selectionSet: SelectionSetNode, ): Array { const conflicts: Array = []; - const [fieldMap, fragmentNames] = getFieldsAndFragmentNames( + const [fieldMap, spreadCollection] = getFieldsAndFragmentSpreads( context, - cachedFieldsAndFragmentNames, + cachedFieldsAndFragmentSpreads, parentType, selectionSet, ); - // (A) Find find all conflicts "within" the fields of this selection set. + // (A) First find all conflicts "within" the fields and spreads of this selection set. // Note: this is the *only place* `collectConflictsWithin` is called. collectConflictsWithin( context, conflicts, - cachedFieldsAndFragmentNames, + cachedFieldsAndFragmentSpreads, comparedFragmentPairs, fieldMap, ); - if (fragmentNames.length !== 0) { - // (B) Then collect conflicts between these fields and those represented by - // each spread fragment name found. - for (let i = 0; i < fragmentNames.length; i++) { - collectConflictsBetweenFieldsAndFragment( + // (B) Then collect conflicts between these fields and those represented by + // each spread fragment name found. + const fragmentSpreads = Object.values(spreadCollection); + for (let i = 0; i < fragmentSpreads.length; i++) { + collectConflictsBetweenFieldsAndFragment( + context, + conflicts, + cachedFieldsAndFragmentSpreads, + comparedFragmentPairs, + false, + fieldMap, + fragmentSpreads[i], + ); + // (C) Then compare this fragment with all other fragments found in this + // selection set to collect conflicts between fragments spread together. + // This compares each item in the list of fragment names to every other + // item in that same list (except for itself). + for (let j = i + 1; j < fragmentSpreads.length; j++) { + collectConflictsBetweenFragments( context, conflicts, - cachedFieldsAndFragmentNames, + cachedFieldsAndFragmentSpreads, comparedFragmentPairs, false, - fieldMap, - fragmentNames[i], + fragmentSpreads[i], + fragmentSpreads[j], ); - // (C) Then compare this fragment with all other fragments found in this - // selection set to collect conflicts between fragments spread together. - // This compares each item in the list of fragment names to every other - // item in that same list (except for itself). - for (let j = i + 1; j < fragmentNames.length; j++) { - collectConflictsBetweenFragments( - context, - conflicts, - cachedFieldsAndFragmentNames, - comparedFragmentPairs, - false, - fragmentNames[i], - fragmentNames[j], - ); - } } } return conflicts; @@ -233,22 +275,28 @@ function findConflictsWithinSelectionSet( function collectConflictsBetweenFieldsAndFragment( context: ValidationContext, conflicts: Array, - cachedFieldsAndFragmentNames: Map, + cachedFieldsAndFragmentSpreads: Map< + SelectionSetNode, + FieldsAndFragmentSpreads + >, comparedFragmentPairs: PairSet, areMutuallyExclusive: boolean, fieldMap: NodeAndDefCollection, - fragmentName: string, + fragmentSpread: FragmentSpreadNode, ): void { - const fragment = context.getFragment(fragmentName); - if (!fragment) { + const fragmentName = fragmentSpread.name.value; + const fragmentDef = context.getFragment(fragmentName); + if (!fragmentDef) { return; } - const [fieldMap2, referencedFragmentNames] = - getReferencedFieldsAndFragmentNames( + const fragmentKey = keyForFragmentSpread(fragmentSpread); + const [fieldMap2, referencedFragmentSpreads] = + getReferencedFieldsAndFragmentSpreads( context, - cachedFieldsAndFragmentNames, - fragment, + cachedFieldsAndFragmentSpreads, + fragmentDef, + fragmentSpread, ); // Do not compare a fragment's fieldMap to itself. @@ -261,7 +309,7 @@ function collectConflictsBetweenFieldsAndFragment( collectConflictsBetween( context, conflicts, - cachedFieldsAndFragmentNames, + cachedFieldsAndFragmentSpreads, comparedFragmentPairs, areMutuallyExclusive, fieldMap, @@ -270,31 +318,34 @@ function collectConflictsBetweenFieldsAndFragment( // (E) Then collect any conflicts between the provided collection of fields // and any fragment names found in the given fragment. - for (const referencedFragmentName of referencedFragmentNames) { + for (const [ + referencedFragmentKey, + referencedFragmentSpread, + ] of Object.entries(referencedFragmentSpreads)) { // Memoize so two fragments are not compared for conflicts more than once. if ( comparedFragmentPairs.has( - referencedFragmentName, - fragmentName, + referencedFragmentKey, + fragmentKey, areMutuallyExclusive, ) ) { continue; } comparedFragmentPairs.add( - referencedFragmentName, - fragmentName, + referencedFragmentKey, + fragmentKey, areMutuallyExclusive, ); collectConflictsBetweenFieldsAndFragment( context, conflicts, - cachedFieldsAndFragmentNames, + cachedFieldsAndFragmentSpreads, comparedFragmentPairs, areMutuallyExclusive, fieldMap, - referencedFragmentName, + referencedFragmentSpread, ); } } @@ -304,46 +355,64 @@ function collectConflictsBetweenFieldsAndFragment( function collectConflictsBetweenFragments( context: ValidationContext, conflicts: Array, - cachedFieldsAndFragmentNames: Map, + cachedFieldsAndFragmentSpreads: Map< + SelectionSetNode, + FieldsAndFragmentSpreads + >, comparedFragmentPairs: PairSet, areMutuallyExclusive: boolean, - fragmentName1: string, - fragmentName2: string, + fragmentSpread1: FragmentSpreadNode, + fragmentSpread2: FragmentSpreadNode, ): void { + const fragmentKey1 = keyForFragmentSpread(fragmentSpread1); + const fragmentKey2 = keyForFragmentSpread(fragmentSpread2); // No need to compare a fragment to itself. - if (fragmentName1 === fragmentName2) { + if (fragmentKey1 === fragmentKey2) { return; } // Memoize so two fragments are not compared for conflicts more than once. if ( - comparedFragmentPairs.has( - fragmentName1, - fragmentName2, - areMutuallyExclusive, - ) + comparedFragmentPairs.has(fragmentKey1, fragmentKey2, areMutuallyExclusive) ) { return; } - comparedFragmentPairs.add(fragmentName1, fragmentName2, areMutuallyExclusive); + comparedFragmentPairs.add(fragmentKey1, fragmentKey2, areMutuallyExclusive); + + // Two unique fragment spreads reference the same fragment, + // which is a conflict + if (fragmentSpread1.name.value === fragmentSpread2.name.value) { + conflicts.push({ + reason: { + kind: 'FRAGMENT_SPREAD', + fragmentName: fragmentSpread1.name.value, + reasonMessage: `${fragmentKey1} and ${fragmentKey2} have different fragment arguments`, + }, + selectionPath1: [fragmentSpread1], + selectionPath2: [fragmentSpread2], + }); + return; + } - const fragment1 = context.getFragment(fragmentName1); - const fragment2 = context.getFragment(fragmentName2); - if (!fragment1 || !fragment2) { + const fragmentDef1 = context.getFragment(fragmentSpread1.name.value); + const fragmentDef2 = context.getFragment(fragmentSpread2.name.value); + if (!fragmentDef1 || !fragmentDef2) { return; } - const [fieldMap1, referencedFragmentNames1] = - getReferencedFieldsAndFragmentNames( + const [fieldMap1, referencedFragmentSpreads1] = + getReferencedFieldsAndFragmentSpreads( context, - cachedFieldsAndFragmentNames, - fragment1, + cachedFieldsAndFragmentSpreads, + fragmentDef1, + fragmentSpread1, ); - const [fieldMap2, referencedFragmentNames2] = - getReferencedFieldsAndFragmentNames( + const [fieldMap2, referencedFragmentSpreads2] = + getReferencedFieldsAndFragmentSpreads( context, - cachedFieldsAndFragmentNames, - fragment2, + cachedFieldsAndFragmentSpreads, + fragmentDef2, + fragmentSpread2, ); // (F) First, collect all conflicts between these two collections of fields @@ -351,7 +420,7 @@ function collectConflictsBetweenFragments( collectConflictsBetween( context, conflicts, - cachedFieldsAndFragmentNames, + cachedFieldsAndFragmentSpreads, comparedFragmentPairs, areMutuallyExclusive, fieldMap1, @@ -360,29 +429,33 @@ function collectConflictsBetweenFragments( // (G) Then collect conflicts between the first fragment and any nested // fragments spread in the second fragment. - for (const referencedFragmentName2 of referencedFragmentNames2) { + for (const referencedFragmentSpread2 of Object.values( + referencedFragmentSpreads2, + )) { collectConflictsBetweenFragments( context, conflicts, - cachedFieldsAndFragmentNames, + cachedFieldsAndFragmentSpreads, comparedFragmentPairs, areMutuallyExclusive, - fragmentName1, - referencedFragmentName2, + fragmentSpread1, + referencedFragmentSpread2, ); } // (G) Then collect conflicts between the second fragment and any nested // fragments spread in the first fragment. - for (const referencedFragmentName1 of referencedFragmentNames1) { + for (const referencedFragmentSpread1 of Object.values( + referencedFragmentSpreads1, + )) { collectConflictsBetweenFragments( context, conflicts, - cachedFieldsAndFragmentNames, + cachedFieldsAndFragmentSpreads, comparedFragmentPairs, areMutuallyExclusive, - referencedFragmentName1, - fragmentName2, + referencedFragmentSpread1, + fragmentSpread2, ); } } @@ -392,7 +465,10 @@ function collectConflictsBetweenFragments( // between the sub-fields of two overlapping fields. function findConflictsBetweenSubSelectionSets( context: ValidationContext, - cachedFieldsAndFragmentNames: Map, + cachedFieldsAndFragmentSpreads: Map< + SelectionSetNode, + FieldsAndFragmentSpreads + >, comparedFragmentPairs: PairSet, areMutuallyExclusive: boolean, parentType1: Maybe, @@ -402,15 +478,15 @@ function findConflictsBetweenSubSelectionSets( ): Array { const conflicts: Array = []; - const [fieldMap1, fragmentNames1] = getFieldsAndFragmentNames( + const [fieldMap1, fragmentSpreads1] = getFieldsAndFragmentSpreads( context, - cachedFieldsAndFragmentNames, + cachedFieldsAndFragmentSpreads, parentType1, selectionSet1, ); - const [fieldMap2, fragmentNames2] = getFieldsAndFragmentNames( + const [fieldMap2, fragmentSpreads2] = getFieldsAndFragmentSpreads( context, - cachedFieldsAndFragmentNames, + cachedFieldsAndFragmentSpreads, parentType2, selectionSet2, ); @@ -419,54 +495,56 @@ function findConflictsBetweenSubSelectionSets( collectConflictsBetween( context, conflicts, - cachedFieldsAndFragmentNames, + cachedFieldsAndFragmentSpreads, comparedFragmentPairs, areMutuallyExclusive, fieldMap1, fieldMap2, ); + // (H.1) Collect all conflicts between the two sets of fragment spreads + // (I) Then collect conflicts between the first collection of fields and - // those referenced by each fragment name associated with the second. - for (const fragmentName2 of fragmentNames2) { + // those referenced by each fragment spread associated with the second. + for (const fragmentSpread2 of Object.values(fragmentSpreads2)) { collectConflictsBetweenFieldsAndFragment( context, conflicts, - cachedFieldsAndFragmentNames, + cachedFieldsAndFragmentSpreads, comparedFragmentPairs, areMutuallyExclusive, fieldMap1, - fragmentName2, + fragmentSpread2, ); } // (I) Then collect conflicts between the second collection of fields and - // those referenced by each fragment name associated with the first. - for (const fragmentName1 of fragmentNames1) { + // those referenced by each fragment spread associated with the first. + for (const fragmentSpread1 of Object.values(fragmentSpreads1)) { collectConflictsBetweenFieldsAndFragment( context, conflicts, - cachedFieldsAndFragmentNames, + cachedFieldsAndFragmentSpreads, comparedFragmentPairs, areMutuallyExclusive, fieldMap2, - fragmentName1, + fragmentSpread1, ); } - // (J) Also collect conflicts between any fragment names by the first and - // fragment names by the second. This compares each item in the first set of - // names to each item in the second set of names. - for (const fragmentName1 of fragmentNames1) { - for (const fragmentName2 of fragmentNames2) { + // (J) Also collect conflicts between any fragment spreads by the first and + // fragment spreads by the second. This compares each item in the first set of + // spreads to each item in the second set of spreads. + for (const fragmentSpread1 of Object.values(fragmentSpreads1)) { + for (const fragmentSpread2 of Object.values(fragmentSpreads2)) { collectConflictsBetweenFragments( context, conflicts, - cachedFieldsAndFragmentNames, + cachedFieldsAndFragmentSpreads, comparedFragmentPairs, areMutuallyExclusive, - fragmentName1, - fragmentName2, + fragmentSpread1, + fragmentSpread2, ); } } @@ -477,7 +555,10 @@ function findConflictsBetweenSubSelectionSets( function collectConflictsWithin( context: ValidationContext, conflicts: Array, - cachedFieldsAndFragmentNames: Map, + cachedFieldsAndFragmentSpreads: Map< + SelectionSetNode, + FieldsAndFragmentSpreads + >, comparedFragmentPairs: PairSet, fieldMap: NodeAndDefCollection, ): void { @@ -492,9 +573,9 @@ function collectConflictsWithin( if (fields.length > 1) { for (let i = 0; i < fields.length; i++) { for (let j = i + 1; j < fields.length; j++) { - const conflict = findConflict( + const conflict = findFieldConflict( context, - cachedFieldsAndFragmentNames, + cachedFieldsAndFragmentSpreads, comparedFragmentPairs, false, // within one collection is never mutually exclusive responseName, @@ -518,7 +599,10 @@ function collectConflictsWithin( function collectConflictsBetween( context: ValidationContext, conflicts: Array, - cachedFieldsAndFragmentNames: Map, + cachedFieldsAndFragmentSpreads: Map< + SelectionSetNode, + FieldsAndFragmentSpreads + >, comparedFragmentPairs: PairSet, parentFieldsAreMutuallyExclusive: boolean, fieldMap1: NodeAndDefCollection, @@ -534,9 +618,9 @@ function collectConflictsBetween( if (fields2) { for (const field1 of fields1) { for (const field2 of fields2) { - const conflict = findConflict( + const conflict = findFieldConflict( context, - cachedFieldsAndFragmentNames, + cachedFieldsAndFragmentSpreads, comparedFragmentPairs, parentFieldsAreMutuallyExclusive, responseName, @@ -554,9 +638,12 @@ function collectConflictsBetween( // Determines if there is a conflict between two particular fields, including // comparing their sub-fields. -function findConflict( +function findFieldConflict( context: ValidationContext, - cachedFieldsAndFragmentNames: Map, + cachedFieldsAndFragmentSpreads: Map< + SelectionSetNode, + FieldsAndFragmentSpreads + >, comparedFragmentPairs: PairSet, parentFieldsAreMutuallyExclusive: boolean, responseName: string, @@ -585,20 +672,28 @@ function findConflict( const name1 = node1.name.value; const name2 = node2.name.value; if (name1 !== name2) { - return [ - [responseName, `"${name1}" and "${name2}" are different fields`], - [node1], - [node2], - ]; + return { + reason: { + kind: 'FIELD', + responseName, + reasonMessage: `"${name1}" and "${name2}" are different fields`, + }, + selectionPath1: [node1], + selectionPath2: [node2], + }; } // Two field calls must have the same arguments. if (stringifyArguments(node1) !== stringifyArguments(node2)) { - return [ - [responseName, 'they have differing arguments'], - [node1], - [node2], - ]; + return { + reason: { + kind: 'FIELD', + responseName, + reasonMessage: 'they have differing arguments', + }, + selectionPath1: [node1], + selectionPath2: [node2], + }; } } @@ -606,11 +701,15 @@ function findConflict( const directives1 = /* c8 ignore next */ node1.directives ?? []; const directives2 = /* c8 ignore next */ node2.directives ?? []; if (!sameStreams(directives1, directives2)) { - return [ - [responseName, 'they have differing stream directives'], - [node1], - [node2], - ]; + return { + reason: { + kind: 'FIELD', + responseName, + reasonMessage: 'they have differing stream directives', + }, + selectionPath1: [node1], + selectionPath2: [node2], + }; } // The return type for each field. @@ -618,16 +717,17 @@ function findConflict( const type2 = def2?.type; if (type1 && type2 && doTypesConflict(type1, type2)) { - return [ - [ + return { + reason: { + kind: 'FIELD', responseName, - `they return conflicting types "${inspect(type1)}" and "${inspect( - type2, - )}"`, - ], - [node1], - [node2], - ]; + reasonMessage: `they return conflicting types "${inspect( + type1, + )}" and "${inspect(type2)}"`, + }, + selectionPath1: [node1], + selectionPath2: [node2], + }; } // Collect and compare sub-fields. Use the same "visited fragment names" list @@ -638,7 +738,7 @@ function findConflict( if (selectionSet1 && selectionSet2) { const conflicts = findConflictsBetweenSubSelectionSets( context, - cachedFieldsAndFragmentNames, + cachedFieldsAndFragmentSpreads, comparedFragmentPairs, areMutuallyExclusive, getNamedType(type1), @@ -720,58 +820,73 @@ function doTypesConflict( // Given a selection set, return the collection of fields (a mapping of response // name to field nodes and definitions) as well as a list of fragment names // referenced via fragment spreads. -function getFieldsAndFragmentNames( +function getFieldsAndFragmentSpreads( context: ValidationContext, - cachedFieldsAndFragmentNames: Map, + cachedFieldsAndFragmentSpreads: Map< + SelectionSetNode, + FieldsAndFragmentSpreads + >, parentType: Maybe, selectionSet: SelectionSetNode, -): FieldsAndFragmentNames { - const cached = cachedFieldsAndFragmentNames.get(selectionSet); +): FieldsAndFragmentSpreads { + const cached = cachedFieldsAndFragmentSpreads.get(selectionSet); if (cached) { return cached; } const nodeAndDefs: NodeAndDefCollection = Object.create(null); - const fragmentNames = new Set(); - _collectFieldsAndFragmentNames( + const fragmentSpreads: ObjMap = {}; + _collectFieldsAndFragmentSpreads( context, parentType, selectionSet, nodeAndDefs, - fragmentNames, + fragmentSpreads, ); - const result = [nodeAndDefs, [...fragmentNames]] as const; - cachedFieldsAndFragmentNames.set(selectionSet, result); + const result = [nodeAndDefs, { ...fragmentSpreads }] as const; + cachedFieldsAndFragmentSpreads.set(selectionSet, result); return result; } // Given a reference to a fragment, return the represented collection of fields -// as well as a list of nested fragment names referenced via fragment spreads. -function getReferencedFieldsAndFragmentNames( +// as well as a list of nested referenced fragment spreads. +function getReferencedFieldsAndFragmentSpreads( context: ValidationContext, - cachedFieldsAndFragmentNames: Map, - fragment: FragmentDefinitionNode, + cachedFieldsAndFragmentSpreads: Map< + SelectionSetNode, + FieldsAndFragmentSpreads + >, + fragmentDef: FragmentDefinitionNode, + fragmentSpread: FragmentSpreadNode, ) { + const fragmentSelectionSet = substituteFragmentArguments( + fragmentDef, + fragmentSpread, + ); + // Short-circuit building a type from the node if possible. - const cached = cachedFieldsAndFragmentNames.get(fragment.selectionSet); + const cached = cachedFieldsAndFragmentSpreads.get(fragmentSelectionSet); if (cached) { return cached; } - const fragmentType = typeFromAST(context.getSchema(), fragment.typeCondition); - return getFieldsAndFragmentNames( + const fragmentType = typeFromAST( + context.getSchema(), + fragmentDef.typeCondition, + ); + return getFieldsAndFragmentSpreads( context, - cachedFieldsAndFragmentNames, + cachedFieldsAndFragmentSpreads, fragmentType, - fragment.selectionSet, + fragmentSelectionSet, ); } -function _collectFieldsAndFragmentNames( +function _collectFieldsAndFragmentSpreads( context: ValidationContext, parentType: Maybe, selectionSet: SelectionSetNode, nodeAndDefs: NodeAndDefCollection, - fragmentNames: Set, + fragmentSpreads: FragmentSpreadsByKey, ): void { for (const selection of selectionSet.selections) { switch (selection.kind) { @@ -791,19 +906,19 @@ function _collectFieldsAndFragmentNames( break; } case Kind.FRAGMENT_SPREAD: - fragmentNames.add(selection.name.value); + fragmentSpreads[keyForFragmentSpread(selection)] = selection; break; case Kind.INLINE_FRAGMENT: { const typeCondition = selection.typeCondition; const inlineFragmentType = typeCondition ? typeFromAST(context.getSchema(), typeCondition) : parentType; - _collectFieldsAndFragmentNames( + _collectFieldsAndFragmentSpreads( context, inlineFragmentType, selection.selectionSet, nodeAndDefs, - fragmentNames, + fragmentSpreads, ); break; } @@ -820,11 +935,21 @@ function subfieldConflicts( node2: FieldNode, ): Maybe { if (conflicts.length > 0) { - return [ - [responseName, conflicts.map(([reason]) => reason)], - [node1, ...conflicts.map(([, fields1]) => fields1).flat()], - [node2, ...conflicts.map(([, , fields2]) => fields2).flat()], - ]; + return { + reason: { + kind: 'FIELD', + responseName, + reasonMessage: conflicts.map((conflict) => conflict.reason), + }, + selectionPath1: [ + node1, + ...conflicts.map((subConflict) => subConflict.selectionPath1).flat(), + ], + selectionPath2: [ + node2, + ...conflicts.map((subConflict) => subConflict.selectionPath2).flat(), + ], + }; } } diff --git a/src/validation/rules/ProvidedRequiredArgumentsRule.ts b/src/validation/rules/ProvidedRequiredArgumentsRule.ts index 350264496f..c513a721c4 100644 --- a/src/validation/rules/ProvidedRequiredArgumentsRule.ts +++ b/src/validation/rules/ProvidedRequiredArgumentsRule.ts @@ -4,7 +4,10 @@ import type { ObjMap } from '../../jsutils/ObjMap.js'; import { GraphQLError } from '../../error/GraphQLError.js'; -import type { InputValueDefinitionNode } from '../../language/ast.js'; +import type { + FragmentArgumentDefinitionNode, + InputValueDefinitionNode, +} from '../../language/ast.js'; import { Kind } from '../../language/kinds.js'; import { print } from '../../language/printer.js'; import type { ASTVisitor } from '../../language/visitor.js'; @@ -56,6 +59,37 @@ export function ProvidedRequiredArgumentsRule( } }, }, + FragmentSpread: { + // Validate on leave to allow for directive errors to appear first. + leave(spreadNode) { + const fragmentDef = context.getFragment(spreadNode.name.value); + if (!fragmentDef) { + return false; + } + + const providedArgs = new Set( + // FIXME: https://github.com/graphql/graphql-js/issues/2203 + /* c8 ignore next */ + spreadNode.arguments?.map((arg) => arg.name.value), + ); + // FIXME: https://github.com/graphql/graphql-js/issues/2203 + /* c8 ignore next */ + for (const argDef of fragmentDef.arguments ?? []) { + if ( + !providedArgs.has(argDef.variable.name.value) && + isRequiredArgumentNode(argDef) + ) { + const argTypeStr = inspect(argDef.type); + context.reportError( + new GraphQLError( + `Fragment "${spreadNode.name.value}" argument "${argDef.variable.name.value}" of type "${argTypeStr}" is required, but it was not provided.`, + { nodes: [spreadNode, argDef] }, + ), + ); + } + } + }, + }, }; } @@ -122,6 +156,8 @@ export function ProvidedRequiredArgumentsOnDirectivesRule( }; } -function isRequiredArgumentNode(arg: InputValueDefinitionNode): boolean { +function isRequiredArgumentNode( + arg: InputValueDefinitionNode | FragmentArgumentDefinitionNode, +): boolean { return arg.type.kind === Kind.NON_NULL_TYPE && arg.defaultValue == null; } diff --git a/src/validation/rules/VariablesAreInputTypesRule.ts b/src/validation/rules/VariablesAreInputTypesRule.ts index 019f3e762c..b4a7ad1de8 100644 --- a/src/validation/rules/VariablesAreInputTypesRule.ts +++ b/src/validation/rules/VariablesAreInputTypesRule.ts @@ -1,6 +1,9 @@ import { GraphQLError } from '../../error/GraphQLError.js'; -import type { VariableDefinitionNode } from '../../language/ast.js'; +import type { + FragmentArgumentDefinitionNode, + VariableDefinitionNode, +} from '../../language/ast.js'; import { print } from '../../language/printer.js'; import type { ASTVisitor } from '../../language/visitor.js'; @@ -21,21 +24,25 @@ import type { ValidationContext } from '../ValidationContext.js'; export function VariablesAreInputTypesRule( context: ValidationContext, ): ASTVisitor { - return { - VariableDefinition(node: VariableDefinitionNode) { - const type = typeFromAST(context.getSchema(), node.type); + const validateNode = ( + node: VariableDefinitionNode | FragmentArgumentDefinitionNode, + ) => { + const type = typeFromAST(context.getSchema(), node.type); - if (type !== undefined && !isInputType(type)) { - const variableName = node.variable.name.value; - const typeName = print(node.type); + if (type !== undefined && !isInputType(type)) { + const variableName = node.variable.name.value; + const typeName = print(node.type); - context.reportError( - new GraphQLError( - `Variable "$${variableName}" cannot be non-input type "${typeName}".`, - { nodes: node.type }, - ), - ); - } - }, + context.reportError( + new GraphQLError( + `Variable "$${variableName}" cannot be non-input type "${typeName}".`, + { nodes: node.type }, + ), + ); + } + }; + return { + VariableDefinition: validateNode, + FragmentArgumentDefinition: validateNode, }; } diff --git a/src/validation/rules/VariablesInAllowedPositionRule.ts b/src/validation/rules/VariablesInAllowedPositionRule.ts index 4039540eba..6f4f013bff 100644 --- a/src/validation/rules/VariablesInAllowedPositionRule.ts +++ b/src/validation/rules/VariablesInAllowedPositionRule.ts @@ -1,9 +1,10 @@ import { inspect } from '../../jsutils/inspect.js'; import type { Maybe } from '../../jsutils/Maybe.js'; +import type { ObjMap } from '../../jsutils/ObjMap.js'; import { GraphQLError } from '../../error/GraphQLError.js'; -import type { ValueNode } from '../../language/ast.js'; +import type { ValueNode, VariableDefinitionNode } from '../../language/ast.js'; import { Kind } from '../../language/kinds.js'; import type { ASTVisitor } from '../../language/visitor.js'; @@ -26,7 +27,7 @@ import type { ValidationContext } from '../ValidationContext.js'; export function VariablesInAllowedPositionRule( context: ValidationContext, ): ASTVisitor { - let varDefMap = Object.create(null); + let varDefMap: ObjMap = Object.create(null); return { OperationDefinition: { @@ -36,9 +37,9 @@ export function VariablesInAllowedPositionRule( leave(operation) { const usages = context.getRecursiveVariableUsages(operation); - for (const { node, type, defaultValue } of usages) { + for (const { node, type, defaultValue, fragmentArgDef } of usages) { const varName = node.name.value; - const varDef = varDefMap[varName]; + const varDef = fragmentArgDef ?? varDefMap[varName]; if (varDef && type) { // A var type is allowed if it is the same or more strict (e.g. is // a subtype of) than the expected type. It can be more strict if diff --git a/src/validation/specifiedRules.ts b/src/validation/specifiedRules.ts index 60c967f8f0..0911474c50 100644 --- a/src/validation/specifiedRules.ts +++ b/src/validation/specifiedRules.ts @@ -29,6 +29,8 @@ import { LoneSchemaDefinitionRule } from './rules/LoneSchemaDefinitionRule.js'; import { NoFragmentCyclesRule } from './rules/NoFragmentCyclesRule.js'; // Spec Section: "All Variable Used Defined" import { NoUndefinedVariablesRule } from './rules/NoUndefinedVariablesRule.js'; +// Spec Section: "All Fragment Arguments Used" +import { NoUnusedFragmentArgumentsRule } from './rules/NoUnusedFragmentArgumentsRule.js'; // Spec Section: "Fragments must be used" import { NoUnusedFragmentsRule } from './rules/NoUnusedFragmentsRule.js'; // Spec Section: "All Variables Used" @@ -99,6 +101,7 @@ export const specifiedRules: ReadonlyArray = Object.freeze([ UniqueVariableNamesRule, NoUndefinedVariablesRule, NoUnusedVariablesRule, + NoUnusedFragmentArgumentsRule, KnownDirectivesRule, UniqueDirectivesPerLocationRule, DeferStreamDirectiveOnRootFieldRule,