diff --git a/src/validation/ValidationContext.ts b/src/validation/ValidationContext.ts index f6944a6ebd..27a3d456c0 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'; import { Kind } from '../language/kinds'; @@ -26,13 +27,15 @@ import type { import type { GraphQLDirective } from '../type/directives'; import type { GraphQLSchema } from '../type/schema'; -import { TypeInfo, visitWithTypeInfo } from '../utilities/TypeInfo'; +import type { TypeInfo } from '../utilities/TypeInfo'; +import { visitWithTypeInfo } from '../utilities/TypeInfo'; type NodeWithSelectionSet = OperationDefinitionNode | FragmentDefinitionNode; interface VariableUsage { readonly node: VariableNode; readonly type: Maybe; readonly defaultValue: Maybe; + readonly fragmentVarDef: Maybe; } /** @@ -199,16 +202,25 @@ export class ValidationContext extends ASTValidationContext { let usages = this._variableUsages.get(node); if (!usages) { const newUsages: Array = []; - const typeInfo = new TypeInfo(this._schema); + const typeInfo = this._typeInfo; + const fragmentVariableDefinitions = + node.kind === Kind.FRAGMENT_DEFINITION + ? node.variableDefinitions + : undefined; + visit( node, visitWithTypeInfo(typeInfo, { VariableDefinition: () => false, Variable(variable) { + const fragmentVarDef = fragmentVariableDefinitions?.find( + (varDef) => varDef.variable.name.value === variable.name.value, + ); newUsages.push({ node: variable, type: typeInfo.getInputType(), defaultValue: typeInfo.getDefaultValue(), + fragmentVarDef, }); }, }), @@ -233,6 +245,20 @@ export class ValidationContext extends ASTValidationContext { return usages; } + getOperationVariableUsages( + operation: OperationDefinitionNode, + ): ReadonlyArray { + let usages = this._recursiveVariableUsages.get(operation); + if (!usages) { + usages = this.getVariableUsages(operation); + for (const frag of this.getRecursivelyReferencedFragments(operation)) { + usages = usages.concat(this.getVariableUsages(frag)); + } + this._recursiveVariableUsages.set(operation, usages); + } + return usages.filter(({ fragmentVarDef }) => !fragmentVarDef); + } + getType(): Maybe { return this._typeInfo.getType(); } diff --git a/src/validation/__tests__/NoUndefinedVariablesRule-test.ts b/src/validation/__tests__/NoUndefinedVariablesRule-test.ts index e027d4a49b..ca69fe0789 100644 --- a/src/validation/__tests__/NoUndefinedVariablesRule-test.ts +++ b/src/validation/__tests__/NoUndefinedVariablesRule-test.ts @@ -404,4 +404,67 @@ describe('Validate: No undefined variables', () => { }, ]); }); + + it('fragment defined arguments are not undefined variables', () => { + expectValid(` + query Foo { + ...FragA + } + fragment FragA($a: String) on Type { + field1(a: $a) + } + `); + }); + + it('defined variables used as fragment arguments are not undefined variables', () => { + expectValid(` + query Foo($b: String) { + ...FragA(a: $b) + } + fragment FragA($a: String) on Type { + field1 + } + `); + }); + + it('variables used as fragment arguments may be undefined variables', () => { + expectErrors(` + query Foo { + ...FragA(a: $a) + } + fragment FragA($a: String) on Type { + field1 + } + `).toDeepEqual([ + { + message: 'Variable "$a" is not defined by operation "Foo".', + locations: [ + { line: 3, column: 21 }, + { line: 2, column: 7 }, + ], + }, + ]); + }); + + it('variables shadowed by parent fragment arguments are still undefined variables', () => { + expectErrors(` + query Foo { + ...FragA + } + fragment FragA($a: String) on Type { + ...FragB + } + fragment FragB on Type { + field1(a: $a) + } + `).toDeepEqual([ + { + message: 'Variable "$a" is not defined by operation "Foo".', + locations: [ + { line: 9, column: 19 }, + { line: 2, column: 7 }, + ], + }, + ]); + }); }); diff --git a/src/validation/__tests__/NoUnusedFragmentVariablesRule-test.ts b/src/validation/__tests__/NoUnusedFragmentVariablesRule-test.ts new file mode 100644 index 0000000000..d7b0060e21 --- /dev/null +++ b/src/validation/__tests__/NoUnusedFragmentVariablesRule-test.ts @@ -0,0 +1,37 @@ +import { describe, it } from 'mocha'; + +import { NoUnusedFragmentVariablesRule } from '../rules/NoUnusedFragmentVariablesRule'; + +import { expectValidationErrors } from './harness'; + +function expectErrors(queryStr: string) { + return expectValidationErrors(NoUnusedFragmentVariablesRule, queryStr); +} + +function expectValid(queryStr: string) { + expectErrors(queryStr).toDeepEqual([]); +} + +describe('Validate: No unused variables', () => { + it('fragment defined arguments are not unused variables', () => { + expectValid(` + query Foo { + ...FragA + } + fragment FragA($a: String) on Type { + field1(a: $a) + } + `); + }); + + it('defined variables used as fragment arguments are not unused variables', () => { + expectErrors(` + query Foo($b: String) { + ...FragA(a: $b) + } + fragment FragA($a: String) on Type { + field1 + } + `); + }); +}); diff --git a/src/validation/__tests__/NoUnusedVariablesRule-test.ts b/src/validation/__tests__/NoUnusedVariablesRule-test.ts index 6be63cd23d..6b95847e93 100644 --- a/src/validation/__tests__/NoUnusedVariablesRule-test.ts +++ b/src/validation/__tests__/NoUnusedVariablesRule-test.ts @@ -230,4 +230,26 @@ describe('Validate: No unused variables', () => { }, ]); }); + + it('fragment defined arguments are not unused variables', () => { + expectValid(` + query Foo { + ...FragA + } + fragment FragA($a: String) on Type { + field1(a: $a) + } + `); + }); + + it('defined variables used as fragment arguments are not unused variables', () => { + expectValid(` + query Foo($b: String) { + ...FragA(a: $b) + } + fragment FragA($a: String) on Type { + field1 + } + `); + }); }); diff --git a/src/validation/__tests__/OverlappingFieldsCanBeMergedRule-test.ts b/src/validation/__tests__/OverlappingFieldsCanBeMergedRule-test.ts index 46cf014e46..8d28bb565c 100644 --- a/src/validation/__tests__/OverlappingFieldsCanBeMergedRule-test.ts +++ b/src/validation/__tests__/OverlappingFieldsCanBeMergedRule-test.ts @@ -1138,4 +1138,136 @@ describe('Validate: Overlapping fields can be merged', () => { }, ]); }); + + describe('fragment arguments must produce fields that can be merged', () => { + it('allows conflicting spreads at different depths', () => { + expectValid(` + query ValidDifferingFragmentArgs($command1: DogCommand, $command2: DogCommand) { + dog { + ...DoesKnowCommand(command: $command1) + mother { + ...DoesKnowCommand(command: $command2) + } + } + } + fragment DoesKnowCommand($command: DogCommand) on Dog { + doesKnowCommand(dogCommand: $command) + } + `); + }); + + it('encounters conflict in fragments', () => { + expectErrors(` + { + ...WithArgs(x: 3) + ...WithArgs(x: 4) + } + fragment WithArgs($x: Int) on Type { + a(x: $x) + } + `).toDeepEqual([ + { + message: + 'Spreads "WithArgs" conflict because WithArgs(x: 3) and WithArgs(x: 4) have different fragment arguments.', + locations: [ + { line: 3, column: 11 }, + { line: 4, column: 11 }, + ], + }, + ]); + }); + + it('encounters conflict in fragment - field no args', () => { + expectErrors(` + query ($y: Int = 1) { + a(x: $y) + ...WithArgs + } + fragment WithArgs on Type { + a(x: $y) + } + `).toDeepEqual([]); + }); + + it('encounters conflict in fragment/field', () => { + expectErrors(` + query ($y: Int = 1) { + a(x: $y) + ...WithArgs(x: $y) + } + fragment WithArgs($x: Int) on Type { + a(x: $x) + } + `).toDeepEqual([]); + }); + + // This is currently not validated, should we? + it('encounters nested field conflict in fragments that could otherwise merge', () => { + expectErrors(` + query ValidDifferingFragmentArgs($command1: DogCommand, $command2: DogCommand) { + 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 23a272572c..1cfedeeada 100644 --- a/src/validation/__tests__/ProvidedRequiredArgumentsRule-test.ts +++ b/src/validation/__tests__/ProvidedRequiredArgumentsRule-test.ts @@ -353,4 +353,108 @@ describe('Validate: Provided required arguments', () => { ]); }); }); + + describe('Fragment required arguments', () => { + it('ignores unknown arguments', () => { + expectValid(` + { + ...Foo(unknownArgument: true) + } + fragment Foo on Query { + dog + } + `); + }); + + // Query: should this be allowed? + // We could differentiate between required/optional (i.e. no default value) + // vs. nullable/non-nullable (i.e. no !), whereas now they are conflated. + // So today: + // $x: Int! `x:` is required and must not be null (NOT a nullable variable) + // $x: Int! = 3 `x:` is not required and must not be null (MAY BE a nullable variable) + // $x: Int `x:` is not required and may be null + // $x: Int = 3 `x:` is not required and may be null + // + // It feels weird to collapse the nullable cases but not the non-nullable ones. + // Whereas all four feel like they ought to mean something explicitly different. + // + // Potential proposal: + // $x: Int! `x:` is required and must not be null (NOT a nullable variable) + // $x: Int! = 3 `x:` is not required and must not be null (NOT a nullable variable) + // $x: Int `x:` is required and may be null + // $x: Int = 3 `x:` is not required and may be null + // + // Required then is whether there's a default value, + // and nullable is whether there's a ! + it('Missing nullable argument with default is allowed', () => { + expectValid(` + { + ...F + } + fragment F($x: Int = 3) on Query { + foo + } + `); + }); + // Above proposal: this should be an error + it('Missing nullable argument is allowed', () => { + expectValid(` + { + ...F + } + fragment F($x: Int) on Query { + foo + } + `); + }); + it('Missing non-nullable argument with default is allowed', () => { + expectValid(` + { + ...F + } + fragment F($x: Int! = 3) on Query { + foo + } + `); + }); + + it('Missing non-nullable argument is not allowed', () => { + expectErrors(` + { + ...F + } + fragment F($x: Int!) on Query { + foo + } + `).toDeepEqual([ + { + message: + 'Fragment "F" argument "x" of type "{ kind: "NonNullType", type: { kind: "NamedType", name: [Object], loc: [Object] }, loc: [Object] }" is required, but it was not provided.', + locations: [ + { line: 3, column: 13 }, + { line: 5, column: 22 }, + ], + }, + ]); + }); + + it('Supplies required variables', () => { + expectValid(` + { + ...F(x: 3) + } + fragment F($x: Int!) on Query { + foo + } + `); + }); + + it('Skips missing fragments', () => { + expectValid(` + { + ...Missing(x: 3) + } + `); + }); + }); }); diff --git a/src/validation/__tests__/ValuesOfCorrectTypeRule-test.ts b/src/validation/__tests__/ValuesOfCorrectTypeRule-test.ts index 3610fa648b..ab9da66ca5 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 7b754fd76f..226613720f 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 090f1680c4..3af5ef4e76 100644 --- a/src/validation/__tests__/VariablesInAllowedPositionRule-test.ts +++ b/src/validation/__tests__/VariablesInAllowedPositionRule-test.ts @@ -356,5 +356,109 @@ describe('Validate: Variables are in allowed positions', () => { dog @include(if: $boolVar) }`); }); + + it('undefined in directive with default value with option', () => { + expectValid(` + { + dog @include(if: $x) + }`); + }); + }); + + describe('Fragment arguments are validated', () => { + it('Boolean => Boolean', () => { + expectValid(` + query Query($booleanArg: Boolean) + { + complicatedArgs { + ...A(b: $booleanArg) + } + } + fragment A($b: Boolean) on ComplicatedArgs { + booleanArgField(booleanArg: $b) + } + `); + }); + + it('Boolean => Boolean with default value', () => { + expectValid(` + query Query($booleanArg: Boolean) + { + complicatedArgs { + ...A(b: $booleanArg) + } + } + fragment A($b: Boolean = true) on ComplicatedArgs { + booleanArgField(booleanArg: $b) + } + `); + }); + + it('Boolean => Boolean!', () => { + expectErrors(` + query Query($ab: Boolean) + { + complicatedArgs { + ...A(b: $ab) + } + } + fragment A($b: Boolean!) on ComplicatedArgs { + booleanArgField(booleanArg: $b) + } + `).toDeepEqual([ + { + message: + 'Variable "$ab" of type "Boolean" used in position expecting type "Boolean!".', + locations: [ + { line: 2, column: 21 }, + { line: 5, column: 21 }, + ], + }, + ]); + }); + + it('Int => Int! fails when variable provides null default value', () => { + expectErrors(` + query Query($intVar: Int = null) { + complicatedArgs { + ...A(i: $intVar) + } + } + fragment A($i: Int!) on ComplicatedArgs { + nonNullIntArgField(nonNullIntArg: $i) + } + `).toDeepEqual([ + { + message: + 'Variable "$intVar" of type "Int" used in position expecting type "Int!".', + locations: [ + { line: 2, column: 21 }, + { line: 4, column: 21 }, + ], + }, + ]); + }); + + it('Int fragment arg => Int! field arg fails even when shadowed by Int! variable', () => { + expectErrors(` + query Query($intVar: Int!) { + complicatedArgs { + ...A(i: $intVar) + } + } + fragment A($intVar: Int) on ComplicatedArgs { + nonNullIntArgField(nonNullIntArg: $intVar) + } + `).toDeepEqual([ + { + message: + 'Variable "$intVar" of type "Int" used in position expecting type "Int!".', + locations: [ + { line: 7, column: 20 }, + { line: 8, column: 45 }, + ], + }, + ]); + }); }); }); diff --git a/src/validation/index.ts b/src/validation/index.ts index 587479e351..14646ac270 100644 --- a/src/validation/index.ts +++ b/src/validation/index.ts @@ -51,6 +51,9 @@ export { PossibleFragmentSpreadsRule } from './rules/PossibleFragmentSpreadsRule // Spec Section: "Argument Optionality" export { ProvidedRequiredArgumentsRule } from './rules/ProvidedRequiredArgumentsRule'; +// Spec Section: "All Fragment Variables Used" +export { NoUnusedFragmentVariablesRule } from './rules/NoUnusedFragmentVariablesRule'; + // Spec Section: "Leaf Field Selections" export { ScalarLeafsRule } from './rules/ScalarLeafsRule'; diff --git a/src/validation/rules/NoUndefinedVariablesRule.ts b/src/validation/rules/NoUndefinedVariablesRule.ts index 3d499b5dcc..82692a3a91 100644 --- a/src/validation/rules/NoUndefinedVariablesRule.ts +++ b/src/validation/rules/NoUndefinedVariablesRule.ts @@ -25,7 +25,10 @@ export function NoUndefinedVariablesRule( leave(operation) { const usages = context.getRecursiveVariableUsages(operation); - for (const { node } of usages) { + for (const { node, fragmentVarDef } of usages) { + if (fragmentVarDef) { + continue; + } const varName = node.name.value; if (variableNameDefined[varName] !== true) { context.reportError( diff --git a/src/validation/rules/NoUnusedFragmentVariablesRule.ts b/src/validation/rules/NoUnusedFragmentVariablesRule.ts new file mode 100644 index 0000000000..9cfde886bd --- /dev/null +++ b/src/validation/rules/NoUnusedFragmentVariablesRule.ts @@ -0,0 +1,40 @@ +import { GraphQLError } from '../../error/GraphQLError'; + +import type { ASTVisitor } from '../../language/visitor'; + +import type { ValidationContext } from '../ValidationContext'; + +/** + * No unused variables + * + * A GraphQL fragment is only valid if all arguments defined by it + * are used within the same fragment. + * + * See https://spec.graphql.org/draft/#sec-All-Fragment-Variables-Used + */ +export function NoUnusedFragmentVariablesRule( + context: ValidationContext, +): ASTVisitor { + return { + FragmentDefinition(fragment) { + const usages = context.getVariableUsages(fragment); + const argumentNameUsed = new Set( + usages.map(({ node }) => node.name.value), + ); + // FIXME: https://github.com/graphql/graphql-js/issues/2203 + /* c8 ignore next */ + const variableDefinitions = fragment.variableDefinitions ?? []; + for (const varDef of variableDefinitions) { + const argName = varDef.variable.name.value; + if (!argumentNameUsed.has(argName)) { + context.reportError( + new GraphQLError( + `Variable "$${argName}" is never used in fragment "${fragment.name.value}".`, + { nodes: varDef }, + ), + ); + } + } + }, + }; +} diff --git a/src/validation/rules/NoUnusedVariablesRule.ts b/src/validation/rules/NoUnusedVariablesRule.ts index 5083af4f28..c2f58d05b0 100644 --- a/src/validation/rules/NoUnusedVariablesRule.ts +++ b/src/validation/rules/NoUnusedVariablesRule.ts @@ -11,7 +11,7 @@ import type { ValidationContext } from '../ValidationContext'; * A GraphQL operation is only valid if all variables defined by an operation * are used, either directly or within a spread fragment. * - * See https://spec.graphql.org/draft/#sec-All-Variables-Used + * See https://spec.graphql.org/draft/#sec-All-Operation-Variables-Used */ export function NoUnusedVariablesRule(context: ValidationContext): ASTVisitor { let variableDefs: Array = []; @@ -23,7 +23,7 @@ export function NoUnusedVariablesRule(context: ValidationContext): ASTVisitor { }, leave(operation) { const variableNameUsed = Object.create(null); - const usages = context.getRecursiveVariableUsages(operation); + const usages = context.getOperationVariableUsages(operation); for (const { node } of usages) { variableNameUsed[node.name.value] = true; diff --git a/src/validation/rules/OverlappingFieldsCanBeMergedRule.ts b/src/validation/rules/OverlappingFieldsCanBeMergedRule.ts index 4305064a6f..0ac766b95c 100644 --- a/src/validation/rules/OverlappingFieldsCanBeMergedRule.ts +++ b/src/validation/rules/OverlappingFieldsCanBeMergedRule.ts @@ -8,12 +8,14 @@ import type { DirectiveNode, FieldNode, FragmentDefinitionNode, + FragmentSpreadNode, SelectionSetNode, ValueNode, } from '../../language/ast'; import { Kind } from '../../language/kinds'; import { print } from '../../language/printer'; import type { ASTVisitor } from '../../language/visitor'; +import { visit } from '../../language/visitor'; import type { GraphQLField, @@ -67,13 +69,13 @@ export function OverlappingFieldsCanBeMergedRule( // A cache for the "field map" and list of fragment names 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, @@ -104,8 +106,22 @@ type NodeAndDef = [ ]; // Map of array of those. type NodeAndDefCollection = ObjMap>; -type FragmentNames = Array; -type FieldsAndFragmentNames = readonly [NodeAndDefCollection, FragmentNames]; +type FragmentSpreadsByName = ObjMap>; +type FieldsAndFragmentSpreads = readonly [ + NodeAndDefCollection, + FragmentSpreadsByName, +]; + +const printFragmentSpreadArguments = (fragmentSpread: FragmentSpreadNode) => { + if (!fragmentSpread.arguments || fragmentSpread.arguments.length === 0) { + return fragmentSpread.name.value; + } + + const printedArguments: Array = fragmentSpread.arguments + .map(print) + .sort((a, b) => a.localeCompare(b)); + return fragmentSpread.name.value + '(' + printedArguments.join(',') + ')'; +}; /** * Algorithm: @@ -167,56 +183,64 @@ 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, fragmentSpreadMap] = 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 fragment-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) { + const allFragmentSpreads = []; + for (const [, fragmentSpreads] of Object.entries(fragmentSpreadMap)) { + allFragmentSpreads.push(...fragmentSpreads); + } + + if (allFragmentSpreads.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++) { + for (let i = 0; i < allFragmentSpreads.length; i++) { collectConflictsBetweenFieldsAndFragment( context, conflicts, - cachedFieldsAndFragmentNames, + cachedFieldsAndFragmentSpreads, comparedFragmentPairs, false, fieldMap, - fragmentNames[i], + allFragmentSpreads[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 < fragmentNames.length; j++) { + for (let j = i + 1; j < allFragmentSpreads.length; j++) { collectConflictsBetweenFragments( context, conflicts, - cachedFieldsAndFragmentNames, + cachedFieldsAndFragmentSpreads, comparedFragmentPairs, false, - fragmentNames[i], - fragmentNames[j], + allFragmentSpreads[i], + allFragmentSpreads[j], ); } } @@ -229,22 +253,27 @@ 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 fragmentName = fragmentSpread.name.value; const fragment = context.getFragment(fragmentName); if (!fragment) { return; } const [fieldMap2, referencedFragmentNames] = - getReferencedFieldsAndFragmentNames( + getReferencedFieldsAndFragmentSpreads( context, - cachedFieldsAndFragmentNames, + cachedFieldsAndFragmentSpreads, fragment, + fragmentSpread, ); // Do not compare a fragment's fieldMap to itself. @@ -257,7 +286,7 @@ function collectConflictsBetweenFieldsAndFragment( collectConflictsBetween( context, conflicts, - cachedFieldsAndFragmentNames, + cachedFieldsAndFragmentSpreads, comparedFragmentPairs, areMutuallyExclusive, fieldMap, @@ -266,7 +295,9 @@ 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 [referencedFragmentName, [spread]] of Object.entries( + referencedFragmentNames, + )) { // Memoize so two fragments are not compared for conflicts more than once. if ( comparedFragmentPairs.has( @@ -286,11 +317,11 @@ function collectConflictsBetweenFieldsAndFragment( collectConflictsBetweenFieldsAndFragment( context, conflicts, - cachedFieldsAndFragmentNames, + cachedFieldsAndFragmentSpreads, comparedFragmentPairs, areMutuallyExclusive, fieldMap, - referencedFragmentName, + spread, ); } } @@ -300,28 +331,49 @@ 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 { - // No need to compare a fragment to itself. - if (fragmentName1 === fragmentName2) { + const fragmentName1 = fragmentSpread1.name.value; + const fragmentName2 = fragmentSpread2.name.value; + if ( + fragmentName1 === fragmentName2 && + !sameArguments(fragmentSpread1, fragmentSpread2) + ) { + context.reportError( + new GraphQLError( + `Spreads "${fragmentName1}" conflict because ${printFragmentSpreadArguments( + fragmentSpread1, + )} and ${printFragmentSpreadArguments( + fragmentSpread2, + )} have different fragment arguments.`, + { nodes: [fragmentSpread1, fragmentSpread2] }, + ), + ); return; } - // Memoize so two fragments are not compared for conflicts more than once. + // No need to compare a fragment to itself. if ( - comparedFragmentPairs.has( - fragmentName1, - fragmentName2, - areMutuallyExclusive, - ) + fragmentName1 === fragmentName2 && + sameArguments(fragmentSpread1, fragmentSpread2) ) { return; } - comparedFragmentPairs.add(fragmentName1, fragmentName2, areMutuallyExclusive); + + const fragKey1 = printFragmentSpreadArguments(fragmentSpread1); + const fragKey2 = printFragmentSpreadArguments(fragmentSpread2); + // Memoize so two fragments are not compared for conflicts more than once. + if (comparedFragmentPairs.has(fragKey1, fragKey2, areMutuallyExclusive)) { + return; + } + comparedFragmentPairs.add(fragKey1, fragKey2, areMutuallyExclusive); const fragment1 = context.getFragment(fragmentName1); const fragment2 = context.getFragment(fragmentName2); @@ -329,17 +381,19 @@ function collectConflictsBetweenFragments( return; } - const [fieldMap1, referencedFragmentNames1] = - getReferencedFieldsAndFragmentNames( + const [fieldMap1, referencedFragmentSpreads1] = + getReferencedFieldsAndFragmentSpreads( context, - cachedFieldsAndFragmentNames, + cachedFieldsAndFragmentSpreads, fragment1, + fragmentSpread1, ); - const [fieldMap2, referencedFragmentNames2] = - getReferencedFieldsAndFragmentNames( + const [fieldMap2, referencedFragmentSpreads2] = + getReferencedFieldsAndFragmentSpreads( context, - cachedFieldsAndFragmentNames, + cachedFieldsAndFragmentSpreads, fragment2, + fragmentSpread2, ); // (F) First, collect all conflicts between these two collections of fields @@ -347,7 +401,7 @@ function collectConflictsBetweenFragments( collectConflictsBetween( context, conflicts, - cachedFieldsAndFragmentNames, + cachedFieldsAndFragmentSpreads, comparedFragmentPairs, areMutuallyExclusive, fieldMap1, @@ -356,29 +410,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, ); } } @@ -388,7 +446,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, @@ -398,15 +459,15 @@ function findConflictsBetweenSubSelectionSets( ): Array { const conflicts: Array = []; - const [fieldMap1, fragmentNames1] = getFieldsAndFragmentNames( + const [fieldMap1, fragmentSpreadsByName1] = getFieldsAndFragmentSpreads( context, - cachedFieldsAndFragmentNames, + cachedFieldsAndFragmentSpreads, parentType1, selectionSet1, ); - const [fieldMap2, fragmentNames2] = getFieldsAndFragmentNames( + const [fieldMap2, fragmentSpreadsByName2] = getFieldsAndFragmentSpreads( context, - cachedFieldsAndFragmentNames, + cachedFieldsAndFragmentSpreads, parentType2, selectionSet2, ); @@ -415,7 +476,7 @@ function findConflictsBetweenSubSelectionSets( collectConflictsBetween( context, conflicts, - cachedFieldsAndFragmentNames, + cachedFieldsAndFragmentSpreads, comparedFragmentPairs, areMutuallyExclusive, fieldMap1, @@ -424,45 +485,45 @@ function findConflictsBetweenSubSelectionSets( // (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 Object.values(fragmentSpreadsByName2)) { 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 Object.values(fragmentSpreadsByName1)) { 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) { + for (const [fragmentSpread1] of Object.values(fragmentSpreadsByName1)) { + for (const [fragmentSpread2] of Object.values(fragmentSpreadsByName2)) { collectConflictsBetweenFragments( context, conflicts, - cachedFieldsAndFragmentNames, + cachedFieldsAndFragmentSpreads, comparedFragmentPairs, areMutuallyExclusive, - fragmentName1, - fragmentName2, + fragmentSpread1, + fragmentSpread2, ); } } @@ -473,7 +534,10 @@ function findConflictsBetweenSubSelectionSets( function collectConflictsWithin( context: ValidationContext, conflicts: Array, - cachedFieldsAndFragmentNames: Map, + cachedFieldsAndFragmentSpreads: Map< + SelectionSetNode, + FieldsAndFragmentSpreads + >, comparedFragmentPairs: PairSet, fieldMap: NodeAndDefCollection, ): void { @@ -490,7 +554,7 @@ 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, @@ -514,7 +578,10 @@ function collectConflictsWithin( function collectConflictsBetween( context: ValidationContext, conflicts: Array, - cachedFieldsAndFragmentNames: Map, + cachedFieldsAndFragmentSpreads: Map< + SelectionSetNode, + FieldsAndFragmentSpreads + >, comparedFragmentPairs: PairSet, parentFieldsAreMutuallyExclusive: boolean, fieldMap1: NodeAndDefCollection, @@ -532,7 +599,7 @@ function collectConflictsBetween( for (const field2 of fields2) { const conflict = findConflict( context, - cachedFieldsAndFragmentNames, + cachedFieldsAndFragmentSpreads, comparedFragmentPairs, parentFieldsAreMutuallyExclusive, responseName, @@ -552,7 +619,10 @@ function collectConflictsBetween( // comparing their sub-fields. function findConflict( context: ValidationContext, - cachedFieldsAndFragmentNames: Map, + cachedFieldsAndFragmentSpreads: Map< + SelectionSetNode, + FieldsAndFragmentSpreads + >, comparedFragmentPairs: PairSet, parentFieldsAreMutuallyExclusive: boolean, responseName: string, @@ -623,7 +693,7 @@ function findConflict( if (selectionSet1 && selectionSet2) { const conflicts = findConflictsBetweenSubSelectionSets( context, - cachedFieldsAndFragmentNames, + cachedFieldsAndFragmentSpreads, comparedFragmentPairs, areMutuallyExclusive, getNamedType(type1), @@ -636,8 +706,8 @@ function findConflict( } function sameArguments( - node1: FieldNode | DirectiveNode, - node2: FieldNode | DirectiveNode, + node1: FieldNode | DirectiveNode | FragmentSpreadNode, + node2: FieldNode | DirectiveNode | FragmentSpreadNode, ): boolean { const args1 = node1.arguments; const args2 = node2.arguments; @@ -704,49 +774,70 @@ 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: ObjMap = Object.create(null); + const fragmentSpreadsByName: ObjMap> = + Object.create(null); _collectFieldsAndFragmentNames( context, parentType, selectionSet, nodeAndDefs, - fragmentNames, + fragmentSpreadsByName, ); - const result = [nodeAndDefs, Object.keys(fragmentNames)] as const; - cachedFieldsAndFragmentNames.set(selectionSet, result); + const result = [nodeAndDefs, fragmentSpreadsByName] 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( +function getReferencedFieldsAndFragmentSpreads( context: ValidationContext, - cachedFieldsAndFragmentNames: Map, + cachedFieldsAndFragmentSpreads: Map< + SelectionSetNode, + FieldsAndFragmentSpreads + >, fragment: FragmentDefinitionNode, + fragmentSpread: FragmentSpreadNode, ) { + const args = fragmentSpread.arguments; + const fragmentSelectionSet = visit(fragment.selectionSet, { + Variable: (node) => { + const name = node.name.value; + const argNode = args?.find((arg) => arg.name.value === name); + if (argNode) { + return argNode.value; + } + + return node; + }, + }); + // Short-circuit building a type from the node if possible. - const cached = cachedFieldsAndFragmentNames.get(fragment.selectionSet); + const cached = cachedFieldsAndFragmentSpreads.get(fragmentSelectionSet); if (cached) { return cached; } const fragmentType = typeFromAST(context.getSchema(), fragment.typeCondition); - return getFieldsAndFragmentNames( + return getFieldsAndFragmentSpreads( context, - cachedFieldsAndFragmentNames, + cachedFieldsAndFragmentSpreads, fragmentType, - fragment.selectionSet, + fragmentSelectionSet, ); } @@ -755,7 +846,7 @@ function _collectFieldsAndFragmentNames( parentType: Maybe, selectionSet: SelectionSetNode, nodeAndDefs: NodeAndDefCollection, - fragmentNames: ObjMap, + fragmentSpreadsByName: ObjMap>, ): void { for (const selection of selectionSet.selections) { switch (selection.kind) { @@ -774,9 +865,15 @@ function _collectFieldsAndFragmentNames( nodeAndDefs[responseName].push([parentType, selection, fieldDef]); break; } - case Kind.FRAGMENT_SPREAD: - fragmentNames[selection.name.value] = true; + case Kind.FRAGMENT_SPREAD: { + const existing = fragmentSpreadsByName[selection.name.value]; + if (existing) { + existing.push(selection); + } else { + fragmentSpreadsByName[selection.name.value] = [selection]; + } break; + } case Kind.INLINE_FRAGMENT: { const typeCondition = selection.typeCondition; const inlineFragmentType = typeCondition @@ -787,7 +884,7 @@ function _collectFieldsAndFragmentNames( inlineFragmentType, selection.selectionSet, nodeAndDefs, - fragmentNames, + fragmentSpreadsByName, ); break; } diff --git a/src/validation/rules/ProvidedRequiredArgumentsRule.ts b/src/validation/rules/ProvidedRequiredArgumentsRule.ts index b111dcee1b..7a7e8ff153 100644 --- a/src/validation/rules/ProvidedRequiredArgumentsRule.ts +++ b/src/validation/rules/ProvidedRequiredArgumentsRule.ts @@ -4,7 +4,10 @@ import type { ObjMap } from '../../jsutils/ObjMap'; import { GraphQLError } from '../../error/GraphQLError'; -import type { InputValueDefinitionNode } from '../../language/ast'; +import type { + InputValueDefinitionNode, + VariableDefinitionNode, +} from '../../language/ast'; import { Kind } from '../../language/kinds'; import { print } from '../../language/printer'; import type { ASTVisitor } from '../../language/visitor'; @@ -56,6 +59,37 @@ export function ProvidedRequiredArgumentsRule( } }, }, + FragmentSpread: { + // Validate on leave to allow for directive errors to appear first. + leave(spreadNode) { + const fragmentDef = context.getFragment(spreadNode.name.value); + if (!fragmentDef) { + return false; + } + + const providedArgs = new Set( + // FIXME: https://github.com/graphql/graphql-js/issues/2203 + /* c8 ignore next */ + spreadNode.arguments?.map((arg) => arg.name.value), + ); + // FIXME: https://github.com/graphql/graphql-js/issues/2203 + /* c8 ignore next */ + for (const varDef of fragmentDef.variableDefinitions ?? []) { + if ( + !providedArgs.has(varDef.variable.name.value) && + isRequiredArgumentNode(varDef) + ) { + const argTypeStr = inspect(varDef.type); + context.reportError( + new GraphQLError( + `Fragment "${spreadNode.name.value}" argument "${varDef.variable.name.value}" of type "${argTypeStr}" is required, but it was not provided.`, + { nodes: [spreadNode, varDef] }, + ), + ); + } + } + }, + }, }; } @@ -122,6 +156,8 @@ export function ProvidedRequiredArgumentsOnDirectivesRule( }; } -function isRequiredArgumentNode(arg: InputValueDefinitionNode): boolean { +function isRequiredArgumentNode( + arg: InputValueDefinitionNode | VariableDefinitionNode, +): boolean { return arg.type.kind === Kind.NON_NULL_TYPE && arg.defaultValue == null; } diff --git a/src/validation/rules/VariablesInAllowedPositionRule.ts b/src/validation/rules/VariablesInAllowedPositionRule.ts index a0b7e991a6..1a3c0cff02 100644 --- a/src/validation/rules/VariablesInAllowedPositionRule.ts +++ b/src/validation/rules/VariablesInAllowedPositionRule.ts @@ -36,9 +36,9 @@ export function VariablesInAllowedPositionRule( leave(operation) { const usages = context.getRecursiveVariableUsages(operation); - for (const { node, type, defaultValue } of usages) { + for (const { node, type, defaultValue, fragmentVarDef } of usages) { const varName = node.name.value; - const varDef = varDefMap[varName]; + const varDef = fragmentVarDef ?? varDefMap[varName]; if (varDef && type) { // A var type is allowed if it is the same or more strict (e.g. is // a subtype of) than the expected type. It can be more strict if diff --git a/src/validation/specifiedRules.ts b/src/validation/specifiedRules.ts index c312c9839c..d1e50c5acf 100644 --- a/src/validation/specifiedRules.ts +++ b/src/validation/specifiedRules.ts @@ -27,6 +27,8 @@ import { NoFragmentCyclesRule } from './rules/NoFragmentCyclesRule'; import { NoUndefinedVariablesRule } from './rules/NoUndefinedVariablesRule'; // Spec Section: "Fragments must be used" import { NoUnusedFragmentsRule } from './rules/NoUnusedFragmentsRule'; +// Spec Section: "All Fragment Variables Used" +import { NoUnusedFragmentVariablesRule } from './rules/NoUnusedFragmentVariablesRule'; // Spec Section: "All Variables Used" import { NoUnusedVariablesRule } from './rules/NoUnusedVariablesRule'; // Spec Section: "Field Selection Merging" @@ -100,6 +102,7 @@ export const specifiedRules: ReadonlyArray = Object.freeze([ NoUndefinedVariablesRule, NoUnusedVariablesRule, KnownDirectivesRule, + NoUnusedFragmentVariablesRule, UniqueDirectivesPerLocationRule, KnownArgumentNamesRule, UniqueArgumentNamesRule,