From 23e2ba9453e646d6a233b267c4559f00811c896a Mon Sep 17 00:00:00 2001 From: "Sachin D. Shinde" Date: Mon, 23 Sep 2024 04:12:17 -0700 Subject: [PATCH 1/2] fix(validation): catch OverlappingFieldsCanBeMergedRule violations with nested fragments (#4168) Port of #4168 to v17 --- .../OverlappingFieldsCanBeMergedRule-test.ts | 54 +++++++ .../rules/OverlappingFieldsCanBeMergedRule.ts | 141 ++++++++++++------ 2 files changed, 150 insertions(+), 45 deletions(-) diff --git a/src/validation/__tests__/OverlappingFieldsCanBeMergedRule-test.ts b/src/validation/__tests__/OverlappingFieldsCanBeMergedRule-test.ts index 7d7c54f37b..19fab1dd79 100644 --- a/src/validation/__tests__/OverlappingFieldsCanBeMergedRule-test.ts +++ b/src/validation/__tests__/OverlappingFieldsCanBeMergedRule-test.ts @@ -674,6 +674,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(` { @@ -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); } } } From 022396e70c542d9de00531db50591239aeb7bbc7 Mon Sep 17 00:00:00 2001 From: Yaacov Rydzinski Date: Mon, 23 Sep 2024 14:29:15 +0300 Subject: [PATCH 2/2] adjust a test to for code coverage --- .../OverlappingFieldsCanBeMergedRule-test.ts | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/src/validation/__tests__/OverlappingFieldsCanBeMergedRule-test.ts b/src/validation/__tests__/OverlappingFieldsCanBeMergedRule-test.ts index 19fab1dd79..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,14 @@ 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 }, ], }, ]);