diff --git a/src/validation/__tests__/OverlappingFieldsCanBeMergedRule-test.ts b/src/validation/__tests__/OverlappingFieldsCanBeMergedRule-test.ts index 7d7c54f37b..582e4602af 100644 --- a/src/validation/__tests__/OverlappingFieldsCanBeMergedRule-test.ts +++ b/src/validation/__tests__/OverlappingFieldsCanBeMergedRule-test.ts @@ -638,10 +638,10 @@ describe('Validate: Overlapping fields can be merged', () => { expectErrors(` { field { - ...F + ...I } field { - ...I + ...F } } fragment F on T { @@ -661,14 +661,41 @@ describe('Validate: Overlapping fields can be merged', () => { `).toDeepEqual([ { message: - 'Fields "field" conflict because subfields "x" conflict because "a" and "b" are different fields and subfields "y" conflict because "c" and "d" are different fields. Use different aliases on the fields to fetch both if this was intentional.', + 'Fields "field" conflict because subfields "y" conflict because "d" and "c" are different fields and subfields "x" conflict because "b" and "a" are different fields. Use different aliases on the fields to fetch both if this was intentional.', locations: [ { line: 3, column: 9 }, - { line: 11, column: 9 }, - { line: 15, column: 9 }, - { line: 6, column: 9 }, - { line: 22, column: 9 }, { line: 18, column: 9 }, + { line: 22, column: 9 }, + { line: 6, column: 9 }, + { line: 15, column: 9 }, + { line: 11, column: 9 }, + ], + }, + ]); + }); + + it('reports deep conflict after nested fragments', () => { + expectErrors(` + fragment F on T { + ...G + } + fragment G on T { + ...H + } + fragment H on T { + x: a + } + { + x: b + ...F + } + `).toDeepEqual([ + { + message: + 'Fields "x" conflict because "b" and "a" are different fields. Use different aliases on the fields to fetch both if this was intentional.', + locations: [ + { line: 12, column: 9 }, + { line: 9, column: 9 }, ], }, ]); @@ -1265,6 +1292,33 @@ describe('Validate: Overlapping fields can be merged', () => { ]); }); + it('does not infinite loop on recursive fragments separated by fields', () => { + expectValid(` + { + ...fragA + ...fragB + } + + fragment fragA on T { + x { + ...fragA + x { + ...fragA + } + } + } + + fragment fragB on T { + x { + ...fragB + x { + ...fragB + } + } + } + `); + }); + describe('fragment arguments must produce fields that can be merged', () => { it('allows conflicting spreads at different depths', () => { expectValid(` diff --git a/src/validation/rules/OverlappingFieldsCanBeMergedRule.ts b/src/validation/rules/OverlappingFieldsCanBeMergedRule.ts index 0d9870c715..dae909c65c 100644 --- a/src/validation/rules/OverlappingFieldsCanBeMergedRule.ts +++ b/src/validation/rules/OverlappingFieldsCanBeMergedRule.ts @@ -65,10 +65,14 @@ function reasonMessage(reason: ConflictReasonMessage): string { export function OverlappingFieldsCanBeMergedRule( context: ValidationContext, ): ASTVisitor { - // A memoization for when two fragments are compared "between" each other for - // conflicts. Two fragments may be compared many times, so memoizing this can - // dramatically improve the performance of this validator. - const comparedFragmentPairs = new PairSet(); + // A memoization for when fields and a fragment or two fragments are compared + // "between" each other for conflicts. Comparisons made be made many times, + // so memoizing this can dramatically improve the performance of this validator. + const comparedFieldsAndFragmentPairs = new OrderedPairSet< + NodeAndDefCollection, + string + >(); + const comparedFragmentPairs = new PairSet(); // 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 @@ -80,6 +84,7 @@ export function OverlappingFieldsCanBeMergedRule( const conflicts = findConflictsWithinSelectionSet( context, cachedFieldsAndFragmentSpreads, + comparedFieldsAndFragmentPairs, comparedFragmentPairs, context.getParentType(), selectionSet, @@ -185,7 +190,8 @@ function findConflictsWithinSelectionSet( SelectionSetNode, FieldsAndFragmentSpreads >, - comparedFragmentPairs: PairSet, + comparedFieldsAndFragmentPairs: OrderedPairSet, + comparedFragmentPairs: PairSet, parentType: Maybe, selectionSet: SelectionSetNode, ): Array { @@ -205,6 +211,7 @@ function findConflictsWithinSelectionSet( context, conflicts, cachedFieldsAndFragmentSpreads, + comparedFieldsAndFragmentPairs, comparedFragmentPairs, fieldMap, ); @@ -217,6 +224,7 @@ function findConflictsWithinSelectionSet( context, conflicts, cachedFieldsAndFragmentSpreads, + comparedFieldsAndFragmentPairs, comparedFragmentPairs, false, fieldMap, @@ -231,6 +239,7 @@ function findConflictsWithinSelectionSet( context, conflicts, cachedFieldsAndFragmentSpreads, + comparedFieldsAndFragmentPairs, comparedFragmentPairs, false, fragmentSpreads[i], @@ -251,11 +260,29 @@ function collectConflictsBetweenFieldsAndFragment( SelectionSetNode, FieldsAndFragmentSpreads >, - comparedFragmentPairs: PairSet, + comparedFieldsAndFragmentPairs: OrderedPairSet, + comparedFragmentPairs: PairSet, areMutuallyExclusive: boolean, fieldMap: NodeAndDefCollection, fragmentSpread: FragmentSpread, ): void { + // Memoize so the fields and fragments are not compared for conflicts more + // than once. + if ( + comparedFieldsAndFragmentPairs.has( + fieldMap, + fragmentSpread.key, + areMutuallyExclusive, + ) + ) { + return; + } + comparedFieldsAndFragmentPairs.add( + fieldMap, + fragmentSpread.key, + areMutuallyExclusive, + ); + const fragment = context.getFragment(fragmentSpread.node.name.value); if (!fragment) { return; @@ -280,6 +307,7 @@ function collectConflictsBetweenFieldsAndFragment( context, conflicts, cachedFieldsAndFragmentSpreads, + comparedFieldsAndFragmentPairs, comparedFragmentPairs, areMutuallyExclusive, fieldMap, @@ -289,28 +317,13 @@ function collectConflictsBetweenFieldsAndFragment( ); // (E) Then collect any conflicts between the provided collection of fields - // and any fragment spreads found in the given fragment. + // and any fragment names found in the given fragment. for (const referencedFragmentSpread of referencedFragmentSpreads) { - // Memoize so two fragments are not compared for conflicts more than once. - if ( - comparedFragmentPairs.has( - referencedFragmentSpread.key, - fragmentSpread.key, - areMutuallyExclusive, - ) - ) { - continue; - } - comparedFragmentPairs.add( - referencedFragmentSpread.key, - fragmentSpread.key, - areMutuallyExclusive, - ); - collectConflictsBetweenFieldsAndFragment( context, conflicts, cachedFieldsAndFragmentSpreads, + comparedFieldsAndFragmentPairs, comparedFragmentPairs, areMutuallyExclusive, fieldMap, @@ -328,7 +341,8 @@ function collectConflictsBetweenFragments( SelectionSetNode, FieldsAndFragmentSpreads >, - comparedFragmentPairs: PairSet, + comparedFieldsAndFragmentPairs: OrderedPairSet, + comparedFragmentPairs: PairSet, areMutuallyExclusive: boolean, fragmentSpread1: FragmentSpread, fragmentSpread2: FragmentSpread, @@ -400,6 +414,7 @@ function collectConflictsBetweenFragments( context, conflicts, cachedFieldsAndFragmentSpreads, + comparedFieldsAndFragmentPairs, comparedFragmentPairs, areMutuallyExclusive, fieldMap1, @@ -415,6 +430,7 @@ function collectConflictsBetweenFragments( context, conflicts, cachedFieldsAndFragmentSpreads, + comparedFieldsAndFragmentPairs, comparedFragmentPairs, areMutuallyExclusive, fragmentSpread1, @@ -429,6 +445,7 @@ function collectConflictsBetweenFragments( context, conflicts, cachedFieldsAndFragmentSpreads, + comparedFieldsAndFragmentPairs, comparedFragmentPairs, areMutuallyExclusive, referencedFragmentSpread1, @@ -446,7 +463,8 @@ function findConflictsBetweenSubSelectionSets( SelectionSetNode, FieldsAndFragmentSpreads >, - comparedFragmentPairs: PairSet, + comparedFieldsAndFragmentPairs: OrderedPairSet, + comparedFragmentPairs: PairSet, areMutuallyExclusive: boolean, parentType1: Maybe, selectionSet1: SelectionSetNode, @@ -477,6 +495,7 @@ function findConflictsBetweenSubSelectionSets( context, conflicts, cachedFieldsAndFragmentSpreads, + comparedFieldsAndFragmentPairs, comparedFragmentPairs, areMutuallyExclusive, fieldMap1, @@ -492,6 +511,7 @@ function findConflictsBetweenSubSelectionSets( context, conflicts, cachedFieldsAndFragmentSpreads, + comparedFieldsAndFragmentPairs, comparedFragmentPairs, areMutuallyExclusive, fieldMap1, @@ -506,6 +526,7 @@ function findConflictsBetweenSubSelectionSets( context, conflicts, cachedFieldsAndFragmentSpreads, + comparedFieldsAndFragmentPairs, comparedFragmentPairs, areMutuallyExclusive, fieldMap2, @@ -522,6 +543,7 @@ function findConflictsBetweenSubSelectionSets( context, conflicts, cachedFieldsAndFragmentSpreads, + comparedFieldsAndFragmentPairs, comparedFragmentPairs, areMutuallyExclusive, fragmentSpread1, @@ -540,7 +562,8 @@ function collectConflictsWithin( SelectionSetNode, FieldsAndFragmentSpreads >, - comparedFragmentPairs: PairSet, + comparedFieldsAndFragmentPairs: OrderedPairSet, + comparedFragmentPairs: PairSet, fieldMap: NodeAndDefCollection, ): void { // A field map is a keyed collection, where each key represents a response @@ -557,6 +580,7 @@ function collectConflictsWithin( const conflict = findConflict( context, cachedFieldsAndFragmentSpreads, + comparedFieldsAndFragmentPairs, comparedFragmentPairs, false, // within one collection is never mutually exclusive responseName, @@ -586,7 +610,8 @@ function collectConflictsBetween( SelectionSetNode, FieldsAndFragmentSpreads >, - comparedFragmentPairs: PairSet, + comparedFieldsAndFragmentPairs: OrderedPairSet, + comparedFragmentPairs: PairSet, parentFieldsAreMutuallyExclusive: boolean, fieldMap1: NodeAndDefCollection, varMap1: Map | undefined, @@ -606,6 +631,7 @@ function collectConflictsBetween( const conflict = findConflict( context, cachedFieldsAndFragmentSpreads, + comparedFieldsAndFragmentPairs, comparedFragmentPairs, parentFieldsAreMutuallyExclusive, responseName, @@ -631,7 +657,8 @@ function findConflict( SelectionSetNode, FieldsAndFragmentSpreads >, - comparedFragmentPairs: PairSet, + comparedFieldsAndFragmentPairs: OrderedPairSet, + comparedFragmentPairs: PairSet, parentFieldsAreMutuallyExclusive: boolean, responseName: string, field1: NodeAndDef, @@ -715,6 +742,7 @@ function findConflict( const conflicts = findConflictsBetweenSubSelectionSets( context, cachedFieldsAndFragmentSpreads, + comparedFieldsAndFragmentPairs, comparedFragmentPairs, areMutuallyExclusive, getNamedType(type1), @@ -1030,37 +1058,60 @@ function subfieldConflicts( } /** - * A way to keep track of pairs of things when the ordering of the pair does not matter. + * A way to keep track of pairs of things where the ordering of the pair + * matters. + * + * Provides a third argument for has/set to allow flagging the pair as + * weakly or strongly present within the collection. */ -class PairSet { - _data: Map>; +class OrderedPairSet { + _data: Map>; constructor() { this._data = new Map(); } - has(a: string, b: string, areMutuallyExclusive: boolean): boolean { - const [key1, key2] = a < b ? [a, b] : [b, a]; - - const result = this._data.get(key1)?.get(key2); + has(a: T, b: U, weaklyPresent: boolean): boolean { + const result = this._data.get(a)?.get(b); if (result === undefined) { return false; } - // areMutuallyExclusive being false is a superset of being true, hence if - // we want to know if this PairSet "has" these two with no exclusivity, - // we have to ensure it was added as such. - return areMutuallyExclusive ? true : areMutuallyExclusive === result; + return weaklyPresent ? true : weaklyPresent === result; } - add(a: string, b: string, areMutuallyExclusive: boolean): void { - const [key1, key2] = a < b ? [a, b] : [b, a]; - - const map = this._data.get(key1); + add(a: T, b: U, weaklyPresent: boolean): void { + const map = this._data.get(a); if (map === undefined) { - this._data.set(key1, new Map([[key2, areMutuallyExclusive]])); + this._data.set(a, new Map([[b, weaklyPresent]])); + } else { + map.set(b, weaklyPresent); + } + } +} + +/** + * A way to keep track of pairs of similar things when the ordering of the pair + * does not matter. + */ +class PairSet { + _orderedPairSet: OrderedPairSet; + + constructor() { + this._orderedPairSet = new OrderedPairSet(); + } + + has(a: T, b: T, weaklyPresent: boolean): boolean { + return a < b + ? this._orderedPairSet.has(a, b, weaklyPresent) + : this._orderedPairSet.has(b, a, weaklyPresent); + } + + add(a: T, b: T, weaklyPresent: boolean): void { + if (a < b) { + this._orderedPairSet.add(a, b, weaklyPresent); } else { - map.set(key2, areMutuallyExclusive); + this._orderedPairSet.add(b, a, weaklyPresent); } } }