diff --git a/src/validation/__tests__/OverlappingFieldsCanBeMergedRule-test.ts b/src/validation/__tests__/OverlappingFieldsCanBeMergedRule-test.ts index 46cf014e46..7418c3e4e8 100644 --- a/src/validation/__tests__/OverlappingFieldsCanBeMergedRule-test.ts +++ b/src/validation/__tests__/OverlappingFieldsCanBeMergedRule-test.ts @@ -548,6 +548,33 @@ describe('Validate: Overlapping fields can be merged', () => { ]); }); + 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 }, + ], + }, + ]); + }); + it('ignores unknown fragments', () => { expectValid(` { @@ -1138,4 +1165,31 @@ 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 + } + } + } + `); + }); }); diff --git a/src/validation/rules/OverlappingFieldsCanBeMergedRule.ts b/src/validation/rules/OverlappingFieldsCanBeMergedRule.ts index 4305064a6f..8397a35b80 100644 --- a/src/validation/rules/OverlappingFieldsCanBeMergedRule.ts +++ b/src/validation/rules/OverlappingFieldsCanBeMergedRule.ts @@ -59,10 +59,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 names found in any given // selection set. Selection sets may be asked for this information multiple @@ -74,6 +78,7 @@ export function OverlappingFieldsCanBeMergedRule( const conflicts = findConflictsWithinSelectionSet( context, cachedFieldsAndFragmentNames, + comparedFieldsAndFragmentPairs, comparedFragmentPairs, context.getParentType(), selectionSet, @@ -168,7 +173,8 @@ type FieldsAndFragmentNames = readonly [NodeAndDefCollection, FragmentNames]; function findConflictsWithinSelectionSet( context: ValidationContext, cachedFieldsAndFragmentNames: Map, - comparedFragmentPairs: PairSet, + comparedFieldsAndFragmentPairs: OrderedPairSet, + comparedFragmentPairs: PairSet, parentType: Maybe, selectionSet: SelectionSetNode, ): Array { @@ -187,6 +193,7 @@ function findConflictsWithinSelectionSet( context, conflicts, cachedFieldsAndFragmentNames, + comparedFieldsAndFragmentPairs, comparedFragmentPairs, fieldMap, ); @@ -199,6 +206,7 @@ function findConflictsWithinSelectionSet( context, conflicts, cachedFieldsAndFragmentNames, + comparedFieldsAndFragmentPairs, comparedFragmentPairs, false, fieldMap, @@ -213,6 +221,7 @@ function findConflictsWithinSelectionSet( context, conflicts, cachedFieldsAndFragmentNames, + comparedFieldsAndFragmentPairs, comparedFragmentPairs, false, fragmentNames[i], @@ -230,11 +239,29 @@ function collectConflictsBetweenFieldsAndFragment( context: ValidationContext, conflicts: Array, cachedFieldsAndFragmentNames: Map, - comparedFragmentPairs: PairSet, + comparedFieldsAndFragmentPairs: OrderedPairSet, + comparedFragmentPairs: PairSet, areMutuallyExclusive: boolean, fieldMap: NodeAndDefCollection, fragmentName: string, ): void { + // Memoize so the fields and fragments are not compared for conflicts more + // than once. + if ( + comparedFieldsAndFragmentPairs.has( + fieldMap, + fragmentName, + areMutuallyExclusive, + ) + ) { + return; + } + comparedFieldsAndFragmentPairs.add( + fieldMap, + fragmentName, + areMutuallyExclusive, + ); + const fragment = context.getFragment(fragmentName); if (!fragment) { return; @@ -258,6 +285,7 @@ function collectConflictsBetweenFieldsAndFragment( context, conflicts, cachedFieldsAndFragmentNames, + comparedFieldsAndFragmentPairs, comparedFragmentPairs, areMutuallyExclusive, fieldMap, @@ -267,26 +295,11 @@ 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) { - // Memoize so two fragments are not compared for conflicts more than once. - if ( - comparedFragmentPairs.has( - referencedFragmentName, - fragmentName, - areMutuallyExclusive, - ) - ) { - continue; - } - comparedFragmentPairs.add( - referencedFragmentName, - fragmentName, - areMutuallyExclusive, - ); - collectConflictsBetweenFieldsAndFragment( context, conflicts, cachedFieldsAndFragmentNames, + comparedFieldsAndFragmentPairs, comparedFragmentPairs, areMutuallyExclusive, fieldMap, @@ -301,7 +314,8 @@ function collectConflictsBetweenFragments( context: ValidationContext, conflicts: Array, cachedFieldsAndFragmentNames: Map, - comparedFragmentPairs: PairSet, + comparedFieldsAndFragmentPairs: OrderedPairSet, + comparedFragmentPairs: PairSet, areMutuallyExclusive: boolean, fragmentName1: string, fragmentName2: string, @@ -348,6 +362,7 @@ function collectConflictsBetweenFragments( context, conflicts, cachedFieldsAndFragmentNames, + comparedFieldsAndFragmentPairs, comparedFragmentPairs, areMutuallyExclusive, fieldMap1, @@ -361,6 +376,7 @@ function collectConflictsBetweenFragments( context, conflicts, cachedFieldsAndFragmentNames, + comparedFieldsAndFragmentPairs, comparedFragmentPairs, areMutuallyExclusive, fragmentName1, @@ -375,6 +391,7 @@ function collectConflictsBetweenFragments( context, conflicts, cachedFieldsAndFragmentNames, + comparedFieldsAndFragmentPairs, comparedFragmentPairs, areMutuallyExclusive, referencedFragmentName1, @@ -389,7 +406,8 @@ function collectConflictsBetweenFragments( function findConflictsBetweenSubSelectionSets( context: ValidationContext, cachedFieldsAndFragmentNames: Map, - comparedFragmentPairs: PairSet, + comparedFieldsAndFragmentPairs: OrderedPairSet, + comparedFragmentPairs: PairSet, areMutuallyExclusive: boolean, parentType1: Maybe, selectionSet1: SelectionSetNode, @@ -416,6 +434,7 @@ function findConflictsBetweenSubSelectionSets( context, conflicts, cachedFieldsAndFragmentNames, + comparedFieldsAndFragmentPairs, comparedFragmentPairs, areMutuallyExclusive, fieldMap1, @@ -429,6 +448,7 @@ function findConflictsBetweenSubSelectionSets( context, conflicts, cachedFieldsAndFragmentNames, + comparedFieldsAndFragmentPairs, comparedFragmentPairs, areMutuallyExclusive, fieldMap1, @@ -443,6 +463,7 @@ function findConflictsBetweenSubSelectionSets( context, conflicts, cachedFieldsAndFragmentNames, + comparedFieldsAndFragmentPairs, comparedFragmentPairs, areMutuallyExclusive, fieldMap2, @@ -459,6 +480,7 @@ function findConflictsBetweenSubSelectionSets( context, conflicts, cachedFieldsAndFragmentNames, + comparedFieldsAndFragmentPairs, comparedFragmentPairs, areMutuallyExclusive, fragmentName1, @@ -474,7 +496,8 @@ function collectConflictsWithin( context: ValidationContext, conflicts: Array, cachedFieldsAndFragmentNames: Map, - comparedFragmentPairs: PairSet, + comparedFieldsAndFragmentPairs: OrderedPairSet, + comparedFragmentPairs: PairSet, fieldMap: NodeAndDefCollection, ): void { // A field map is a keyed collection, where each key represents a response @@ -491,6 +514,7 @@ function collectConflictsWithin( const conflict = findConflict( context, cachedFieldsAndFragmentNames, + comparedFieldsAndFragmentPairs, comparedFragmentPairs, false, // within one collection is never mutually exclusive responseName, @@ -515,7 +539,8 @@ function collectConflictsBetween( context: ValidationContext, conflicts: Array, cachedFieldsAndFragmentNames: Map, - comparedFragmentPairs: PairSet, + comparedFieldsAndFragmentPairs: OrderedPairSet, + comparedFragmentPairs: PairSet, parentFieldsAreMutuallyExclusive: boolean, fieldMap1: NodeAndDefCollection, fieldMap2: NodeAndDefCollection, @@ -533,6 +558,7 @@ function collectConflictsBetween( const conflict = findConflict( context, cachedFieldsAndFragmentNames, + comparedFieldsAndFragmentPairs, comparedFragmentPairs, parentFieldsAreMutuallyExclusive, responseName, @@ -553,7 +579,8 @@ function collectConflictsBetween( function findConflict( context: ValidationContext, cachedFieldsAndFragmentNames: Map, - comparedFragmentPairs: PairSet, + comparedFieldsAndFragmentPairs: OrderedPairSet, + comparedFragmentPairs: PairSet, parentFieldsAreMutuallyExclusive: boolean, responseName: string, field1: NodeAndDef, @@ -624,6 +651,7 @@ function findConflict( const conflicts = findConflictsBetweenSubSelectionSets( context, cachedFieldsAndFragmentNames, + comparedFieldsAndFragmentPairs, comparedFragmentPairs, areMutuallyExclusive, getNamedType(type1), @@ -813,37 +841,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); } } }