From 1e2a4198e7f4e31efd32e7192fa328de8d0f00d8 Mon Sep 17 00:00:00 2001 From: "Sachin D. Shinde" Date: Thu, 15 Aug 2024 23:01:20 -0700 Subject: [PATCH 1/9] Add test that elicits bug with comparedFragmentPairs usage --- .../OverlappingFieldsCanBeMergedRule-test.ts | 27 +++++++++++++++++++ 1 file changed, 27 insertions(+) diff --git a/src/validation/__tests__/OverlappingFieldsCanBeMergedRule-test.ts b/src/validation/__tests__/OverlappingFieldsCanBeMergedRule-test.ts index 46cf014e46..b30f643c18 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(` { From 997923d32cfb0069eeafd356f4503d000d122802 Mon Sep 17 00:00:00 2001 From: "Sachin D. Shinde" Date: Fri, 16 Aug 2024 08:08:53 -0700 Subject: [PATCH 2/9] Revert non-test changes in https://github.com/graphql/graphql-js/pull/3442 , which introduced the bugged compareFragmentPairs usage --- .../rules/OverlappingFieldsCanBeMergedRule.ts | 16 ---------------- 1 file changed, 16 deletions(-) diff --git a/src/validation/rules/OverlappingFieldsCanBeMergedRule.ts b/src/validation/rules/OverlappingFieldsCanBeMergedRule.ts index 4305064a6f..0f2a9d4730 100644 --- a/src/validation/rules/OverlappingFieldsCanBeMergedRule.ts +++ b/src/validation/rules/OverlappingFieldsCanBeMergedRule.ts @@ -267,22 +267,6 @@ 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, From e1fb65aac9dad01bc01a56ee9592d8960613f17e Mon Sep 17 00:00:00 2001 From: "Sachin D. Shinde" Date: Fri, 16 Aug 2024 14:44:38 -0700 Subject: [PATCH 3/9] Modify collectConflictsBetweenFieldsAndFragment() to track seen fragments when recursing into fragments (but not when recursing into fields, to save memory) --- .../rules/OverlappingFieldsCanBeMergedRule.ts | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/src/validation/rules/OverlappingFieldsCanBeMergedRule.ts b/src/validation/rules/OverlappingFieldsCanBeMergedRule.ts index 0f2a9d4730..2aa38a3351 100644 --- a/src/validation/rules/OverlappingFieldsCanBeMergedRule.ts +++ b/src/validation/rules/OverlappingFieldsCanBeMergedRule.ts @@ -200,6 +200,7 @@ function findConflictsWithinSelectionSet( conflicts, cachedFieldsAndFragmentNames, comparedFragmentPairs, + null, false, fieldMap, fragmentNames[i], @@ -231,10 +232,15 @@ function collectConflictsBetweenFieldsAndFragment( conflicts: Array, cachedFieldsAndFragmentNames: Map, comparedFragmentPairs: PairSet, + comparedFragmentsForFields: null | Set, areMutuallyExclusive: boolean, fieldMap: NodeAndDefCollection, fragmentName: string, ): void { + if (comparedFragmentsForFields?.has(fragmentName)) { + return; + } + const fragment = context.getFragment(fragmentName); if (!fragment) { return; @@ -266,12 +272,15 @@ function collectConflictsBetweenFieldsAndFragment( // (E) Then collect any conflicts between the provided collection of fields // and any fragment names found in the given fragment. + const newComparedFragmentsForFields = + comparedFragmentsForFields ?? new Set([fragmentName]); for (const referencedFragmentName of referencedFragmentNames) { collectConflictsBetweenFieldsAndFragment( context, conflicts, cachedFieldsAndFragmentNames, comparedFragmentPairs, + newComparedFragmentsForFields, areMutuallyExclusive, fieldMap, referencedFragmentName, @@ -414,6 +423,7 @@ function findConflictsBetweenSubSelectionSets( conflicts, cachedFieldsAndFragmentNames, comparedFragmentPairs, + null, areMutuallyExclusive, fieldMap1, fragmentName2, @@ -428,6 +438,7 @@ function findConflictsBetweenSubSelectionSets( conflicts, cachedFieldsAndFragmentNames, comparedFragmentPairs, + null, areMutuallyExclusive, fieldMap2, fragmentName1, From 99f868ac85b1946d147e10c7c169c14d723cdc28 Mon Sep 17 00:00:00 2001 From: "Sachin D. Shinde" Date: Fri, 16 Aug 2024 14:52:04 -0700 Subject: [PATCH 4/9] Add test that elicits bug caused by not recursing into fields --- .../OverlappingFieldsCanBeMergedRule-test.ts | 27 +++++++++++++++++++ 1 file changed, 27 insertions(+) diff --git a/src/validation/__tests__/OverlappingFieldsCanBeMergedRule-test.ts b/src/validation/__tests__/OverlappingFieldsCanBeMergedRule-test.ts index b30f643c18..7418c3e4e8 100644 --- a/src/validation/__tests__/OverlappingFieldsCanBeMergedRule-test.ts +++ b/src/validation/__tests__/OverlappingFieldsCanBeMergedRule-test.ts @@ -1165,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 + } + } + } + `); + }); }); From a359a93bbf25c4360958ab3a4d7e97865a48dc6b Mon Sep 17 00:00:00 2001 From: "Sachin D. Shinde" Date: Fri, 16 Aug 2024 14:53:55 -0700 Subject: [PATCH 5/9] Revert previous changes for collectConflictsBetweenFieldsAndFragment() in light of bug --- .../rules/OverlappingFieldsCanBeMergedRule.ts | 11 ----------- 1 file changed, 11 deletions(-) diff --git a/src/validation/rules/OverlappingFieldsCanBeMergedRule.ts b/src/validation/rules/OverlappingFieldsCanBeMergedRule.ts index 2aa38a3351..0f2a9d4730 100644 --- a/src/validation/rules/OverlappingFieldsCanBeMergedRule.ts +++ b/src/validation/rules/OverlappingFieldsCanBeMergedRule.ts @@ -200,7 +200,6 @@ function findConflictsWithinSelectionSet( conflicts, cachedFieldsAndFragmentNames, comparedFragmentPairs, - null, false, fieldMap, fragmentNames[i], @@ -232,15 +231,10 @@ function collectConflictsBetweenFieldsAndFragment( conflicts: Array, cachedFieldsAndFragmentNames: Map, comparedFragmentPairs: PairSet, - comparedFragmentsForFields: null | Set, areMutuallyExclusive: boolean, fieldMap: NodeAndDefCollection, fragmentName: string, ): void { - if (comparedFragmentsForFields?.has(fragmentName)) { - return; - } - const fragment = context.getFragment(fragmentName); if (!fragment) { return; @@ -272,15 +266,12 @@ function collectConflictsBetweenFieldsAndFragment( // (E) Then collect any conflicts between the provided collection of fields // and any fragment names found in the given fragment. - const newComparedFragmentsForFields = - comparedFragmentsForFields ?? new Set([fragmentName]); for (const referencedFragmentName of referencedFragmentNames) { collectConflictsBetweenFieldsAndFragment( context, conflicts, cachedFieldsAndFragmentNames, comparedFragmentPairs, - newComparedFragmentsForFields, areMutuallyExclusive, fieldMap, referencedFragmentName, @@ -423,7 +414,6 @@ function findConflictsBetweenSubSelectionSets( conflicts, cachedFieldsAndFragmentNames, comparedFragmentPairs, - null, areMutuallyExclusive, fieldMap1, fragmentName2, @@ -438,7 +428,6 @@ function findConflictsBetweenSubSelectionSets( conflicts, cachedFieldsAndFragmentNames, comparedFragmentPairs, - null, areMutuallyExclusive, fieldMap2, fragmentName1, From b2a71fc9e9a56415b6ebf2e9ef2758c6a75d474a Mon Sep 17 00:00:00 2001 From: "Sachin D. Shinde" Date: Fri, 16 Aug 2024 15:05:57 -0700 Subject: [PATCH 6/9] Modify comparedFragmentPairs to allow a selection set to replace a fragment, and modify collectConflictsBetweenFieldsAndFragment() to memoize the selection set and fragment --- .../rules/OverlappingFieldsCanBeMergedRule.ts | 27 +++++++++++++++---- 1 file changed, 22 insertions(+), 5 deletions(-) diff --git a/src/validation/rules/OverlappingFieldsCanBeMergedRule.ts b/src/validation/rules/OverlappingFieldsCanBeMergedRule.ts index 0f2a9d4730..4ae580f5c8 100644 --- a/src/validation/rules/OverlappingFieldsCanBeMergedRule.ts +++ b/src/validation/rules/OverlappingFieldsCanBeMergedRule.ts @@ -235,6 +235,13 @@ function collectConflictsBetweenFieldsAndFragment( fieldMap: NodeAndDefCollection, fragmentName: string, ): void { + // Memoize so the fields and fragments are not compared for conflicts more + // than once. + if (comparedFragmentPairs.has(fieldMap, fragmentName, areMutuallyExclusive)) { + return; + } + comparedFragmentPairs.add(fieldMap, fragmentName, areMutuallyExclusive); + const fragment = context.getFragment(fragmentName); if (!fragment) { return; @@ -800,14 +807,19 @@ function subfieldConflicts( * A way to keep track of pairs of things when the ordering of the pair does not matter. */ class PairSet { - _data: Map>; + _data: Map>; constructor() { this._data = new Map(); } - has(a: string, b: string, areMutuallyExclusive: boolean): boolean { - const [key1, key2] = a < b ? [a, b] : [b, a]; + has( + a: string | NodeAndDefCollection, + b: string, + areMutuallyExclusive: boolean, + ): boolean { + const [key1, key2] = + typeof a !== 'string' ? [a, b] : a < b ? [a, b] : [b, a]; const result = this._data.get(key1)?.get(key2); if (result === undefined) { @@ -820,8 +832,13 @@ class PairSet { return areMutuallyExclusive ? true : areMutuallyExclusive === result; } - add(a: string, b: string, areMutuallyExclusive: boolean): void { - const [key1, key2] = a < b ? [a, b] : [b, a]; + add( + a: string | NodeAndDefCollection, + b: string, + areMutuallyExclusive: boolean, + ): void { + const [key1, key2] = + typeof a !== 'string' ? [a, b] : a < b ? [a, b] : [b, a]; const map = this._data.get(key1); if (map === undefined) { From 2d830cad102d36989ab7b377695716a30cc97853 Mon Sep 17 00:00:00 2001 From: "Sachin D. Shinde" Date: Sun, 8 Sep 2024 08:24:19 -0700 Subject: [PATCH 7/9] Separate fields-fragment memoization from fragment-fragment memoization Co-authored-by: Yaacov Rydzinski --- .../rules/OverlappingFieldsCanBeMergedRule.ts | 94 +++++++++++++------ 1 file changed, 64 insertions(+), 30 deletions(-) diff --git a/src/validation/rules/OverlappingFieldsCanBeMergedRule.ts b/src/validation/rules/OverlappingFieldsCanBeMergedRule.ts index 4ae580f5c8..b0cfdbd500 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 PairSet< + NodeAndDefCollection, + string + >((a, _) => typeof a === 'string'); + const comparedFragmentPairs = new PairSet((a, b) => a < b); // 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: PairSet, + 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,17 +239,28 @@ function collectConflictsBetweenFieldsAndFragment( context: ValidationContext, conflicts: Array, cachedFieldsAndFragmentNames: Map, - comparedFragmentPairs: PairSet, + comparedFieldsAndFragmentPairs: PairSet, + comparedFragmentPairs: PairSet, areMutuallyExclusive: boolean, fieldMap: NodeAndDefCollection, fragmentName: string, ): void { // Memoize so the fields and fragments are not compared for conflicts more // than once. - if (comparedFragmentPairs.has(fieldMap, fragmentName, areMutuallyExclusive)) { + if ( + comparedFieldsAndFragmentPairs.has( + fieldMap, + fragmentName, + areMutuallyExclusive, + ) + ) { return; } - comparedFragmentPairs.add(fieldMap, fragmentName, areMutuallyExclusive); + comparedFieldsAndFragmentPairs.add( + fieldMap, + fragmentName, + areMutuallyExclusive, + ); const fragment = context.getFragment(fragmentName); if (!fragment) { @@ -265,6 +285,7 @@ function collectConflictsBetweenFieldsAndFragment( context, conflicts, cachedFieldsAndFragmentNames, + comparedFieldsAndFragmentPairs, comparedFragmentPairs, areMutuallyExclusive, fieldMap, @@ -278,6 +299,7 @@ function collectConflictsBetweenFieldsAndFragment( context, conflicts, cachedFieldsAndFragmentNames, + comparedFieldsAndFragmentPairs, comparedFragmentPairs, areMutuallyExclusive, fieldMap, @@ -292,7 +314,8 @@ function collectConflictsBetweenFragments( context: ValidationContext, conflicts: Array, cachedFieldsAndFragmentNames: Map, - comparedFragmentPairs: PairSet, + comparedFieldsAndFragmentPairs: PairSet, + comparedFragmentPairs: PairSet, areMutuallyExclusive: boolean, fragmentName1: string, fragmentName2: string, @@ -339,6 +362,7 @@ function collectConflictsBetweenFragments( context, conflicts, cachedFieldsAndFragmentNames, + comparedFieldsAndFragmentPairs, comparedFragmentPairs, areMutuallyExclusive, fieldMap1, @@ -352,6 +376,7 @@ function collectConflictsBetweenFragments( context, conflicts, cachedFieldsAndFragmentNames, + comparedFieldsAndFragmentPairs, comparedFragmentPairs, areMutuallyExclusive, fragmentName1, @@ -366,6 +391,7 @@ function collectConflictsBetweenFragments( context, conflicts, cachedFieldsAndFragmentNames, + comparedFieldsAndFragmentPairs, comparedFragmentPairs, areMutuallyExclusive, referencedFragmentName1, @@ -380,7 +406,8 @@ function collectConflictsBetweenFragments( function findConflictsBetweenSubSelectionSets( context: ValidationContext, cachedFieldsAndFragmentNames: Map, - comparedFragmentPairs: PairSet, + comparedFieldsAndFragmentPairs: PairSet, + comparedFragmentPairs: PairSet, areMutuallyExclusive: boolean, parentType1: Maybe, selectionSet1: SelectionSetNode, @@ -407,6 +434,7 @@ function findConflictsBetweenSubSelectionSets( context, conflicts, cachedFieldsAndFragmentNames, + comparedFieldsAndFragmentPairs, comparedFragmentPairs, areMutuallyExclusive, fieldMap1, @@ -420,6 +448,7 @@ function findConflictsBetweenSubSelectionSets( context, conflicts, cachedFieldsAndFragmentNames, + comparedFieldsAndFragmentPairs, comparedFragmentPairs, areMutuallyExclusive, fieldMap1, @@ -434,6 +463,7 @@ function findConflictsBetweenSubSelectionSets( context, conflicts, cachedFieldsAndFragmentNames, + comparedFieldsAndFragmentPairs, comparedFragmentPairs, areMutuallyExclusive, fieldMap2, @@ -450,6 +480,7 @@ function findConflictsBetweenSubSelectionSets( context, conflicts, cachedFieldsAndFragmentNames, + comparedFieldsAndFragmentPairs, comparedFragmentPairs, areMutuallyExclusive, fragmentName1, @@ -465,7 +496,8 @@ function collectConflictsWithin( context: ValidationContext, conflicts: Array, cachedFieldsAndFragmentNames: Map, - comparedFragmentPairs: PairSet, + comparedFieldsAndFragmentPairs: PairSet, + comparedFragmentPairs: PairSet, fieldMap: NodeAndDefCollection, ): void { // A field map is a keyed collection, where each key represents a response @@ -482,6 +514,7 @@ function collectConflictsWithin( const conflict = findConflict( context, cachedFieldsAndFragmentNames, + comparedFieldsAndFragmentPairs, comparedFragmentPairs, false, // within one collection is never mutually exclusive responseName, @@ -506,7 +539,8 @@ function collectConflictsBetween( context: ValidationContext, conflicts: Array, cachedFieldsAndFragmentNames: Map, - comparedFragmentPairs: PairSet, + comparedFieldsAndFragmentPairs: PairSet, + comparedFragmentPairs: PairSet, parentFieldsAreMutuallyExclusive: boolean, fieldMap1: NodeAndDefCollection, fieldMap2: NodeAndDefCollection, @@ -524,6 +558,7 @@ function collectConflictsBetween( const conflict = findConflict( context, cachedFieldsAndFragmentNames, + comparedFieldsAndFragmentPairs, comparedFragmentPairs, parentFieldsAreMutuallyExclusive, responseName, @@ -544,7 +579,8 @@ function collectConflictsBetween( function findConflict( context: ValidationContext, cachedFieldsAndFragmentNames: Map, - comparedFragmentPairs: PairSet, + comparedFieldsAndFragmentPairs: PairSet, + comparedFragmentPairs: PairSet, parentFieldsAreMutuallyExclusive: boolean, responseName: string, field1: NodeAndDef, @@ -615,6 +651,7 @@ function findConflict( const conflicts = findConflictsBetweenSubSelectionSets( context, cachedFieldsAndFragmentNames, + comparedFieldsAndFragmentPairs, comparedFragmentPairs, areMutuallyExclusive, getNamedType(type1), @@ -806,20 +843,20 @@ function subfieldConflicts( /** * A way to keep track of pairs of things when the ordering of the pair does not matter. */ -class PairSet { - _data: Map>; +class PairSet { + _data: Map>; + _orderer: (a: T | U, b: T | U) => [T | U, T | U]; - constructor() { + constructor(comparator: (a: T | U, b: T | U) => boolean) { this._data = new Map(); + this._orderer = (a: T | U, b: T | U) => + comparator(a, b) ? [a, b] : [b, a]; } - has( - a: string | NodeAndDefCollection, - b: string, - areMutuallyExclusive: boolean, - ): boolean { - const [key1, key2] = - typeof a !== 'string' ? [a, b] : a < b ? [a, b] : [b, a]; + has(a: T, b: U, areMutuallyExclusive: boolean): boolean; + has(a: U, b: T, areMutuallyExclusive: boolean): boolean; + has(a: T | U, b: T | U, areMutuallyExclusive: boolean): boolean { + const [key1, key2] = this._orderer(a, b); const result = this._data.get(key1)?.get(key2); if (result === undefined) { @@ -832,13 +869,10 @@ class PairSet { return areMutuallyExclusive ? true : areMutuallyExclusive === result; } - add( - a: string | NodeAndDefCollection, - b: string, - areMutuallyExclusive: boolean, - ): void { - const [key1, key2] = - typeof a !== 'string' ? [a, b] : a < b ? [a, b] : [b, a]; + add(a: T, b: U, areMutuallyExclusive: boolean): void; + add(a: U, b: T, areMutuallyExclusive: boolean): void; + add(a: T | U, b: T | U, areMutuallyExclusive: boolean): void { + const [key1, key2] = this._orderer(a, b); const map = this._data.get(key1); if (map === undefined) { From 4be3fb81f2f2b4978ac35bc40c9f82682d771fe8 Mon Sep 17 00:00:00 2001 From: Yaacov Rydzinski Date: Mon, 9 Sep 2024 16:38:03 +0300 Subject: [PATCH 8/9] introduce OrderedPairSet class --- .../rules/OverlappingFieldsCanBeMergedRule.ts | 102 ++++++++++-------- 1 file changed, 60 insertions(+), 42 deletions(-) diff --git a/src/validation/rules/OverlappingFieldsCanBeMergedRule.ts b/src/validation/rules/OverlappingFieldsCanBeMergedRule.ts index b0cfdbd500..7d63b4266b 100644 --- a/src/validation/rules/OverlappingFieldsCanBeMergedRule.ts +++ b/src/validation/rules/OverlappingFieldsCanBeMergedRule.ts @@ -62,11 +62,11 @@ export function OverlappingFieldsCanBeMergedRule( // 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 PairSet< + const comparedFieldsAndFragmentPairs = new OrderedPairSet< NodeAndDefCollection, string - >((a, _) => typeof a === 'string'); - const comparedFragmentPairs = new PairSet((a, b) => a < b); + >(); + const comparedFragmentPairs = new PairSet((a, b) => a < b); // 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 @@ -173,8 +173,8 @@ type FieldsAndFragmentNames = readonly [NodeAndDefCollection, FragmentNames]; function findConflictsWithinSelectionSet( context: ValidationContext, cachedFieldsAndFragmentNames: Map, - comparedFieldsAndFragmentPairs: PairSet, - comparedFragmentPairs: PairSet, + comparedFieldsAndFragmentPairs: OrderedPairSet, + comparedFragmentPairs: PairSet, parentType: Maybe, selectionSet: SelectionSetNode, ): Array { @@ -239,8 +239,8 @@ function collectConflictsBetweenFieldsAndFragment( context: ValidationContext, conflicts: Array, cachedFieldsAndFragmentNames: Map, - comparedFieldsAndFragmentPairs: PairSet, - comparedFragmentPairs: PairSet, + comparedFieldsAndFragmentPairs: OrderedPairSet, + comparedFragmentPairs: PairSet, areMutuallyExclusive: boolean, fieldMap: NodeAndDefCollection, fragmentName: string, @@ -314,8 +314,8 @@ function collectConflictsBetweenFragments( context: ValidationContext, conflicts: Array, cachedFieldsAndFragmentNames: Map, - comparedFieldsAndFragmentPairs: PairSet, - comparedFragmentPairs: PairSet, + comparedFieldsAndFragmentPairs: OrderedPairSet, + comparedFragmentPairs: PairSet, areMutuallyExclusive: boolean, fragmentName1: string, fragmentName2: string, @@ -406,8 +406,8 @@ function collectConflictsBetweenFragments( function findConflictsBetweenSubSelectionSets( context: ValidationContext, cachedFieldsAndFragmentNames: Map, - comparedFieldsAndFragmentPairs: PairSet, - comparedFragmentPairs: PairSet, + comparedFieldsAndFragmentPairs: OrderedPairSet, + comparedFragmentPairs: PairSet, areMutuallyExclusive: boolean, parentType1: Maybe, selectionSet1: SelectionSetNode, @@ -496,8 +496,8 @@ function collectConflictsWithin( context: ValidationContext, conflicts: Array, cachedFieldsAndFragmentNames: Map, - comparedFieldsAndFragmentPairs: PairSet, - comparedFragmentPairs: PairSet, + comparedFieldsAndFragmentPairs: OrderedPairSet, + comparedFragmentPairs: PairSet, fieldMap: NodeAndDefCollection, ): void { // A field map is a keyed collection, where each key represents a response @@ -539,8 +539,8 @@ function collectConflictsBetween( context: ValidationContext, conflicts: Array, cachedFieldsAndFragmentNames: Map, - comparedFieldsAndFragmentPairs: PairSet, - comparedFragmentPairs: PairSet, + comparedFieldsAndFragmentPairs: OrderedPairSet, + comparedFragmentPairs: PairSet, parentFieldsAreMutuallyExclusive: boolean, fieldMap1: NodeAndDefCollection, fieldMap2: NodeAndDefCollection, @@ -579,8 +579,8 @@ function collectConflictsBetween( function findConflict( context: ValidationContext, cachedFieldsAndFragmentNames: Map, - comparedFieldsAndFragmentPairs: PairSet, - comparedFragmentPairs: PairSet, + comparedFieldsAndFragmentPairs: OrderedPairSet, + comparedFragmentPairs: PairSet, parentFieldsAreMutuallyExclusive: boolean, responseName: string, field1: NodeAndDef, @@ -841,44 +841,62 @@ 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>; - _orderer: (a: T | U, b: T | U) => [T | U, T | U]; +class OrderedPairSet { + _data: Map>; - constructor(comparator: (a: T | U, b: T | U) => boolean) { + constructor() { this._data = new Map(); - this._orderer = (a: T | U, b: T | U) => - comparator(a, b) ? [a, b] : [b, a]; } - has(a: T, b: U, areMutuallyExclusive: boolean): boolean; - has(a: U, b: T, areMutuallyExclusive: boolean): boolean; - has(a: T | U, b: T | U, areMutuallyExclusive: boolean): boolean { - const [key1, key2] = this._orderer(a, b); - - 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: T, b: U, areMutuallyExclusive: boolean): void; - add(a: U, b: T, areMutuallyExclusive: boolean): void; - add(a: T | U, b: T | U, areMutuallyExclusive: boolean): void { - const [key1, key2] = this._orderer(a, b); - - 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; + _comparator: (a: T, b: T) => boolean; + + constructor(comparator: (a: T, b: T) => boolean) { + this._orderedPairSet = new OrderedPairSet(); + this._comparator = comparator; + } + + has(a: T, b: T, weaklyPresent: boolean): boolean { + return this._comparator(a, b) + ? this._orderedPairSet.has(a, b, weaklyPresent) + : this._orderedPairSet.has(b, a, weaklyPresent); + } + + add(a: T, b: T, weaklyPresent: boolean): void { + if (this._comparator(a, b)) { + this._orderedPairSet.add(a, b, weaklyPresent); } else { - map.set(key2, areMutuallyExclusive); + this._orderedPairSet.add(b, a, weaklyPresent); } } } From dd7d0e639cc1f894de0cd08c7b13d35a28307720 Mon Sep 17 00:00:00 2001 From: Yaacov Rydzinski Date: Mon, 23 Sep 2024 14:06:55 +0300 Subject: [PATCH 9/9] remove custom comparator option we do not presently need it --- .../rules/OverlappingFieldsCanBeMergedRule.ts | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/src/validation/rules/OverlappingFieldsCanBeMergedRule.ts b/src/validation/rules/OverlappingFieldsCanBeMergedRule.ts index 7d63b4266b..8397a35b80 100644 --- a/src/validation/rules/OverlappingFieldsCanBeMergedRule.ts +++ b/src/validation/rules/OverlappingFieldsCanBeMergedRule.ts @@ -66,7 +66,7 @@ export function OverlappingFieldsCanBeMergedRule( NodeAndDefCollection, string >(); - const comparedFragmentPairs = new PairSet((a, b) => a < b); + 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 @@ -879,21 +879,19 @@ class OrderedPairSet { */ class PairSet { _orderedPairSet: OrderedPairSet; - _comparator: (a: T, b: T) => boolean; - constructor(comparator: (a: T, b: T) => boolean) { + constructor() { this._orderedPairSet = new OrderedPairSet(); - this._comparator = comparator; } has(a: T, b: T, weaklyPresent: boolean): boolean { - return this._comparator(a, b) + return a < b ? this._orderedPairSet.has(a, b, weaklyPresent) : this._orderedPairSet.has(b, a, weaklyPresent); } add(a: T, b: T, weaklyPresent: boolean): void { - if (this._comparator(a, b)) { + if (a < b) { this._orderedPairSet.add(a, b, weaklyPresent); } else { this._orderedPairSet.add(b, a, weaklyPresent);