diff --git a/src/validation/__tests__/OverlappingFieldsCanBeMergedRule-test.ts b/src/validation/__tests__/OverlappingFieldsCanBeMergedRule-test.ts index ec29d788f5..ac7cf7991e 100644 --- a/src/validation/__tests__/OverlappingFieldsCanBeMergedRule-test.ts +++ b/src/validation/__tests__/OverlappingFieldsCanBeMergedRule-test.ts @@ -1405,8 +1405,8 @@ describe('Validate: Overlapping fields can be merged', () => { `); }); - it('allows operations with overlapping fields with identical variable arguments passed via fragment arguments', () => { - expectValid(` + it('rejects overlapping fields whose variables have different definitions', () => { + expectErrors(` query ($y: Int = 1) { a(x: $y) ...WithArgs(x: $y) @@ -1414,11 +1414,20 @@ describe('Validate: Overlapping fields can be merged', () => { fragment WithArgs($x: Int) 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 fields with identical variable arguments passed via nested fragment arguments', () => { - expectValid(` + it('rejects overlapping fields with different nested variable definitions', () => { + expectErrors(` query ($z: Int = 1) { a(x: $z) ...WithArgs(y: $z) @@ -1429,11 +1438,20 @@ describe('Validate: Overlapping fields can be merged', () => { fragment NestedWithArgs($x: Int) 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: 10, column: 11 }, + ], + }, + ]); }); - it('allows operations with overlapping fields with identical arguments via fragment variable defaults', () => { - expectValid(` + it('rejects a fragment variable even when its default matches a literal', () => { + expectErrors(` query { a(x: 1) ...WithArgs @@ -1441,7 +1459,37 @@ describe('Validate: Overlapping fields can be merged', () => { 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('keeps an omitted fragment variable distinct from a shadowed operation variable', () => { + expectErrors(` + query ($x: Int) { + a(x: $x) + ...WithArgs + } + fragment WithArgs($x: Int) 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('raises errors with overlapping fields with arguments that conflict via operation variables even with defaults and fragment variable defaults', () => { @@ -1465,8 +1513,8 @@ describe('Validate: Overlapping fields can be merged', () => { ]); }); - it('allows operations with overlapping list fields with identical variable arguments passed via fragment arguments', () => { - expectValid(` + it('rejects overlapping fields using list variables with different definitions', () => { + expectErrors(` query Query($stringListVarY: [String]) { complicatedArgs { stringListArgField(stringListArg: $stringListVarY) @@ -1476,25 +1524,43 @@ describe('Validate: Overlapping fields can be merged', () => { fragment WithArgs($stringListVarX: [String]) on Type { stringListArgField(stringListArg: $stringListVarX) } - `); + `).toDeepEqual([ + { + message: + 'Fields "stringListArgField" conflict because they have differing arguments. Use different aliases on the fields to fetch both if this was intentional.', + locations: [ + { line: 4, column: 13 }, + { line: 9, column: 11 }, + ], + }, + ]); }); - it('allows operations with overlapping list fields with identical variable arguments in item position passed via fragment arguments', () => { - expectValid(` + it('rejects nested list values with different variable definitions', () => { + expectErrors(` query Query($stringListVarY: [String]) { complicatedArgs { - stringListArgField(stringListArg: [$stringListVarY]) + stringListArgField(stringListArg: [$stringListVarY, "fixed"]) ...WithArgs(stringListVarX: $stringListVarY) } } fragment WithArgs($stringListVarX: [String]) on Type { - stringListArgField(stringListArg: [$stringListVarX]) + stringListArgField(stringListArg: [$stringListVarX, "fixed"]) } - `); + `).toDeepEqual([ + { + message: + 'Fields "stringListArgField" conflict because they have differing arguments. Use different aliases on the fields to fetch both if this was intentional.', + locations: [ + { line: 4, column: 13 }, + { line: 9, column: 11 }, + ], + }, + ]); }); - it('allows operations with overlapping input object fields with identical variable arguments passed via fragment arguments', () => { - expectValid(` + it('rejects overlapping fields using input object variables with different definitions', () => { + expectErrors(` query Query($complexVarY: ComplexInput) { complicatedArgs { complexArgField(complexArg: $complexVarY) @@ -1504,11 +1570,20 @@ describe('Validate: Overlapping fields can be merged', () => { fragment WithArgs($complexVarX: ComplexInput) on Type { complexArgField(complexArg: $complexVarX) } - `); + `).toDeepEqual([ + { + message: + 'Fields "complexArgField" conflict because they have differing arguments. Use different aliases on the fields to fetch both if this was intentional.', + locations: [ + { line: 4, column: 13 }, + { line: 9, column: 11 }, + ], + }, + ]); }); - it('allows operations with overlapping input object fields with identical variable arguments in field position passed via fragment arguments', () => { - expectValid(` + it('rejects nested input values with different variable definitions', () => { + expectErrors(` query Query($boolVarY: Boolean) { complicatedArgs { complexArgField(complexArg: {requiredArg: $boolVarY}) @@ -1518,6 +1593,54 @@ describe('Validate: Overlapping fields can be merged', () => { fragment WithArgs($boolVarX: Boolean) on Type { complexArgField(complexArg: {requiredArg: $boolVarX}) } + `).toDeepEqual([ + { + message: + 'Fields "complexArgField" conflict because they have differing arguments. Use different aliases on the fields to fetch both if this was intentional.', + locations: [ + { line: 4, column: 13 }, + { line: 9, column: 11 }, + ], + }, + ]); + }); + + it('rejects fragment spreads whose variables have different definitions', () => { + expectErrors(` + query Query($value: Int) { + ...A(value: $value) + ...B(value: $value) + } + fragment A($value: Int) on Type { + ...Shared(size: $value) + } + fragment B($value: Int) on Type { + ...Shared(size: $value) + } + fragment Shared($size: Int) on Type { + a(x: $size) + } + `).toDeepEqual([ + { + message: + 'Spreads "Shared" conflict because Shared(size: $value) and Shared(size: $value) have different fragment arguments.', + locations: [ + { line: 7, column: 11 }, + { line: 10, column: 11 }, + ], + }, + ]); + }); + + it('allows repeated spreads with the same local variable definitions', () => { + expectValid(` + query Query($size: Int) { + ...Wrapper(size: $size) + ...Wrapper(size: $size) + } + fragment Wrapper($size: Int) on Type { + a(x: $size) + } `); }); diff --git a/src/validation/rules/OverlappingFieldsCanBeMergedRule.ts b/src/validation/rules/OverlappingFieldsCanBeMergedRule.ts index 908d82d946..8be14136fa 100644 --- a/src/validation/rules/OverlappingFieldsCanBeMergedRule.ts +++ b/src/validation/rules/OverlappingFieldsCanBeMergedRule.ts @@ -115,7 +115,8 @@ export function OverlappingFieldsCanBeMergedRule( // A cache for the "field map" and list of fragment spreads found in any given // selection set. Selection sets may be asked for this information multiple // times, so this improves the performance of this validator. - const cachedFieldsAndFragmentSpreads = new Map(); + const cachedFieldsAndFragmentSpreads: FieldsAndFragmentSpreadsCache = + new Map(); return { SelectionSet(selectionSet) { @@ -163,6 +164,10 @@ type FieldsAndFragmentSpreads = readonly [ NodeAndDefCollection, FragmentSpreads, ]; +type FieldsAndFragmentSpreadsCache = Map< + SelectionSetNode, + Map | undefined, FieldsAndFragmentSpreads> +>; /** * Find all conflicts found "within" a selection set, including those found @@ -225,23 +230,21 @@ type FieldsAndFragmentSpreads = readonly [ */ function findConflictsWithinSelectionSet( context: ValidationContext, - cachedFieldsAndFragmentSpreads: Map< - SelectionSetNode, - FieldsAndFragmentSpreads - >, + cachedFieldsAndFragmentSpreads: FieldsAndFragmentSpreadsCache, comparedFieldsAndFragmentPairs: OrderedPairSet, comparedFragmentPairs: PairSet, parentType: Maybe, selectionSet: SelectionSetNode, ): Array { const conflicts: Array = []; + const varMap = getVarMap(context.getFragmentSignature()); const [fieldMap, fragmentSpreads] = getFieldsAndFragmentSpreads( context, cachedFieldsAndFragmentSpreads, parentType, selectionSet, - undefined, + varMap, ); // (A) Find find all conflicts "within" the fields and f of this selection set. @@ -253,6 +256,7 @@ function findConflictsWithinSelectionSet( comparedFieldsAndFragmentPairs, comparedFragmentPairs, fieldMap, + varMap, ); if (fragmentSpreads.length !== 0) { @@ -267,6 +271,7 @@ function findConflictsWithinSelectionSet( comparedFragmentPairs, false, fieldMap, + varMap, fragmentSpreads[i], ); // (C) Then compare this fragment with all other fragments found in this @@ -295,14 +300,12 @@ function findConflictsWithinSelectionSet( function collectConflictsBetweenFieldsAndFragment( context: ValidationContext, conflicts: Array, - cachedFieldsAndFragmentSpreads: Map< - SelectionSetNode, - FieldsAndFragmentSpreads - >, + cachedFieldsAndFragmentSpreads: FieldsAndFragmentSpreadsCache, comparedFieldsAndFragmentPairs: OrderedPairSet, comparedFragmentPairs: PairSet, areMutuallyExclusive: boolean, fieldMap: NodeAndDefCollection, + varMap: Map | undefined, fragmentSpread: FragmentSpread, ): void { // Memoize so the fields and fragments are not compared for conflicts more @@ -350,7 +353,7 @@ function collectConflictsBetweenFieldsAndFragment( comparedFragmentPairs, areMutuallyExclusive, fieldMap, - undefined, + varMap, fieldMap2, fragmentSpread.varMap, ); @@ -366,6 +369,7 @@ function collectConflictsBetweenFieldsAndFragment( comparedFragmentPairs, areMutuallyExclusive, fieldMap, + varMap, referencedFragmentSpread, ); } @@ -376,10 +380,7 @@ function collectConflictsBetweenFieldsAndFragment( function collectConflictsBetweenFragments( context: ValidationContext, conflicts: Array, - cachedFieldsAndFragmentSpreads: Map< - SelectionSetNode, - FieldsAndFragmentSpreads - >, + cachedFieldsAndFragmentSpreads: FieldsAndFragmentSpreadsCache, comparedFieldsAndFragmentPairs: OrderedPairSet, comparedFragmentPairs: PairSet, areMutuallyExclusive: boolean, @@ -393,21 +394,18 @@ function collectConflictsBetweenFragments( if (fragmentSpread1.node.name.value === fragmentSpread2.node.name.value) { if ( - !sameArguments( - fragmentSpread1.node.arguments, - fragmentSpread1.varMap, - fragmentSpread2.node.arguments, - fragmentSpread2.varMap, - ) + comparedFragmentPairs.has(fragmentSpread1.key, fragmentSpread2.key, false) ) { - 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; } + comparedFragmentPairs.add(fragmentSpread1.key, fragmentSpread2.key, false); + context.reportError( + new GraphQLError( + `Spreads "${fragmentSpread1.node.name.value}" conflict because ${getFragmentSpreadDescription(fragmentSpread1.node)} and ${getFragmentSpreadDescription(fragmentSpread2.node)} have different fragment arguments.`, + { nodes: [fragmentSpread1.node, fragmentSpread2.node] }, + ), + ); + return; } // Memoize so two fragments are not compared for conflicts more than once. @@ -498,10 +496,7 @@ function collectConflictsBetweenFragments( // between the sub-fields of two overlapping fields. function findConflictsBetweenSubSelectionSets( context: ValidationContext, - cachedFieldsAndFragmentSpreads: Map< - SelectionSetNode, - FieldsAndFragmentSpreads - >, + cachedFieldsAndFragmentSpreads: FieldsAndFragmentSpreadsCache, comparedFieldsAndFragmentPairs: OrderedPairSet, comparedFragmentPairs: PairSet, areMutuallyExclusive: boolean, @@ -554,6 +549,7 @@ function findConflictsBetweenSubSelectionSets( comparedFragmentPairs, areMutuallyExclusive, fieldMap1, + varMap1, fragmentSpread2, ); } @@ -569,6 +565,7 @@ function findConflictsBetweenSubSelectionSets( comparedFragmentPairs, areMutuallyExclusive, fieldMap2, + varMap2, fragmentSpread1, ); } @@ -597,13 +594,11 @@ function findConflictsBetweenSubSelectionSets( function collectConflictsWithin( context: ValidationContext, conflicts: Array, - cachedFieldsAndFragmentSpreads: Map< - SelectionSetNode, - FieldsAndFragmentSpreads - >, + cachedFieldsAndFragmentSpreads: FieldsAndFragmentSpreadsCache, comparedFieldsAndFragmentPairs: OrderedPairSet, comparedFragmentPairs: PairSet, fieldMap: NodeAndDefCollection, + varMap: 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 @@ -624,9 +619,9 @@ function collectConflictsWithin( false, // within one collection is never mutually exclusive responseName, fields[i], - undefined, + varMap, fields[j], - undefined, + varMap, ); if (conflict) { conflicts.push(conflict); @@ -645,10 +640,7 @@ function collectConflictsWithin( function collectConflictsBetween( context: ValidationContext, conflicts: Array, - cachedFieldsAndFragmentSpreads: Map< - SelectionSetNode, - FieldsAndFragmentSpreads - >, + cachedFieldsAndFragmentSpreads: FieldsAndFragmentSpreadsCache, comparedFieldsAndFragmentPairs: OrderedPairSet, comparedFragmentPairs: PairSet, parentFieldsAreMutuallyExclusive: boolean, @@ -692,10 +684,7 @@ function collectConflictsBetween( // comparing their sub-fields. function findConflict( context: ValidationContext, - cachedFieldsAndFragmentSpreads: Map< - SelectionSetNode, - FieldsAndFragmentSpreads - >, + cachedFieldsAndFragmentSpreads: FieldsAndFragmentSpreadsCache, comparedFieldsAndFragmentPairs: OrderedPairSet, comparedFragmentPairs: PairSet, parentFieldsAreMutuallyExclusive: boolean, @@ -928,15 +917,17 @@ function doTypesConflict( // referenced via fragment spreads. function getFieldsAndFragmentSpreads( context: ValidationContext, - cachedFieldsAndFragmentSpreads: Map< - SelectionSetNode, - FieldsAndFragmentSpreads - >, + cachedFieldsAndFragmentSpreads: FieldsAndFragmentSpreadsCache, parentType: Maybe, selectionSet: SelectionSetNode, varMap: Map | undefined, ): FieldsAndFragmentSpreads { - const cached = cachedFieldsAndFragmentSpreads.get(selectionSet); + let cache = cachedFieldsAndFragmentSpreads.get(selectionSet); + if (!cache) { + cache = new Map(); + cachedFieldsAndFragmentSpreads.set(selectionSet, cache); + } + const cached = cache.get(varMap); if (cached) { return cached; } @@ -954,7 +945,7 @@ function getFieldsAndFragmentSpreads( nodeAndDefs, Array.from(fragmentSpreads.values()), ]; - cachedFieldsAndFragmentSpreads.set(selectionSet, result); + cache.set(varMap, result); return result; } @@ -962,15 +953,14 @@ function getFieldsAndFragmentSpreads( // as well as a list of nested fragment spreads referenced via fragment spreads. function getReferencedFieldsAndFragmentSpreads( context: ValidationContext, - cachedFieldsAndFragmentSpreads: Map< - SelectionSetNode, - FieldsAndFragmentSpreads - >, + cachedFieldsAndFragmentSpreads: FieldsAndFragmentSpreadsCache, fragment: FragmentDefinitionNode, varMap: Map | undefined, ) { // Short-circuit building a type from the node if possible. - const cached = cachedFieldsAndFragmentSpreads.get(fragment.selectionSet); + const cached = cachedFieldsAndFragmentSpreads + .get(fragment.selectionSet) + ?.get(varMap); if (cached) { return cached; } @@ -1043,7 +1033,6 @@ function getFragmentSpread( varMap: Map | undefined, ): FragmentSpread { let key = ''; - const newVarMap = new Map(); const fragmentSignature = context.getFragmentSignatureByName()( fragmentSpreadNode.name.value, ); @@ -1055,19 +1044,13 @@ function getFragmentSpread( } if (fragmentSignature?.variableDefinitions) { key += fragmentSpreadNode.name.value + '('; - for (const [varName, variable] of fragmentSignature.variableDefinitions) { + for (const varName of fragmentSignature.variableDefinitions.keys()) { 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); + const scopedValue = varMap + ? replaceFragmentVariables(value, varMap) + : value; + key += varName + ': ' + stringifyValue(scopedValue); } } key += ')'; @@ -1075,10 +1058,31 @@ function getFragmentSpread( return { key, node: fragmentSpreadNode, - varMap: newVarMap.size > 0 ? newVarMap : undefined, + varMap: getVarMap(fragmentSignature, key), }; } +function getVarMap( + fragmentSignature: ReturnType, + key = '', +): Map | undefined { + if (!fragmentSignature || fragmentSignature.variableDefinitions.size === 0) { + return; + } + const varMap = new Map(); + for (const variableName of fragmentSignature.variableDefinitions.keys()) { + varMap.set(variableName, { + kind: Kind.VARIABLE, + name: { kind: Kind.NAME, value: JSON.stringify([key, variableName]) }, + }); + } + return varMap; +} + +function getFragmentSpreadDescription(node: FragmentSpreadNode): string { + return print({ ...node, directives: [] }).slice(3); +} + // Given a series of Conflicts which occurred between two sub-fields, generate // a single Conflict. function subfieldConflicts(