From f130906d22b01f45ac0013c35f66dc57a77e4deb Mon Sep 17 00:00:00 2001 From: Nicolas Lagoutte Date: Fri, 24 Dec 2021 11:40:22 +0000 Subject: [PATCH 1/5] Prevent Infinite Loop in OverlappingFieldsCanBeMergedRule --- .../OverlappingFieldsCanBeMergedRule-test.ts | 11 +++++++++++ .../rules/OverlappingFieldsCanBeMergedRule.ts | 4 ++++ 2 files changed, 15 insertions(+) diff --git a/src/validation/__tests__/OverlappingFieldsCanBeMergedRule-test.ts b/src/validation/__tests__/OverlappingFieldsCanBeMergedRule-test.ts index f9f4456b5e..5dd6c2e19f 100644 --- a/src/validation/__tests__/OverlappingFieldsCanBeMergedRule-test.ts +++ b/src/validation/__tests__/OverlappingFieldsCanBeMergedRule-test.ts @@ -1021,6 +1021,17 @@ describe('Validate: Overlapping fields can be merged', () => { `); }); + it('does not infinite loop on immediately recursive fragment mentionned in queries', () => { + expectValid(` + query myQuery { + todoRemove + ...fragA + } + + fragment fragA on Query { ...fragA } + `); + }); + it('does not infinite loop on transitively recursive fragment', () => { expectValid(` fragment fragA on Human { name, ...fragB } diff --git a/src/validation/rules/OverlappingFieldsCanBeMergedRule.ts b/src/validation/rules/OverlappingFieldsCanBeMergedRule.ts index 740454dda7..5027b2d57a 100644 --- a/src/validation/rules/OverlappingFieldsCanBeMergedRule.ts +++ b/src/validation/rules/OverlappingFieldsCanBeMergedRule.ts @@ -267,6 +267,10 @@ 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) { + if (referencedFragmentName === fragmentName) { + continue; + } + collectConflictsBetweenFieldsAndFragment( context, conflicts, From 61352ec967f258b3e43c232f6f7b57f1b81a47f0 Mon Sep 17 00:00:00 2001 From: Nicolas Lagoutte Date: Fri, 24 Dec 2021 11:46:56 +0000 Subject: [PATCH 2/5] Adjust comments and todos --- .../__tests__/OverlappingFieldsCanBeMergedRule-test.ts | 1 - src/validation/rules/OverlappingFieldsCanBeMergedRule.ts | 1 + 2 files changed, 1 insertion(+), 1 deletion(-) diff --git a/src/validation/__tests__/OverlappingFieldsCanBeMergedRule-test.ts b/src/validation/__tests__/OverlappingFieldsCanBeMergedRule-test.ts index 5dd6c2e19f..435e82620a 100644 --- a/src/validation/__tests__/OverlappingFieldsCanBeMergedRule-test.ts +++ b/src/validation/__tests__/OverlappingFieldsCanBeMergedRule-test.ts @@ -1024,7 +1024,6 @@ describe('Validate: Overlapping fields can be merged', () => { it('does not infinite loop on immediately recursive fragment mentionned in queries', () => { expectValid(` query myQuery { - todoRemove ...fragA } diff --git a/src/validation/rules/OverlappingFieldsCanBeMergedRule.ts b/src/validation/rules/OverlappingFieldsCanBeMergedRule.ts index 5027b2d57a..7a8d65f99d 100644 --- a/src/validation/rules/OverlappingFieldsCanBeMergedRule.ts +++ b/src/validation/rules/OverlappingFieldsCanBeMergedRule.ts @@ -267,6 +267,7 @@ 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) { + // Don't compare this fragment with itself if (referencedFragmentName === fragmentName) { continue; } From a842353012c5056a564359c5402624c0dfbda30c Mon Sep 17 00:00:00 2001 From: Nicolas Lagoutte Date: Fri, 24 Dec 2021 12:59:43 +0000 Subject: [PATCH 3/5] spellchecking --- .../__tests__/OverlappingFieldsCanBeMergedRule-test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/validation/__tests__/OverlappingFieldsCanBeMergedRule-test.ts b/src/validation/__tests__/OverlappingFieldsCanBeMergedRule-test.ts index 435e82620a..7655ac172a 100644 --- a/src/validation/__tests__/OverlappingFieldsCanBeMergedRule-test.ts +++ b/src/validation/__tests__/OverlappingFieldsCanBeMergedRule-test.ts @@ -1021,7 +1021,7 @@ describe('Validate: Overlapping fields can be merged', () => { `); }); - it('does not infinite loop on immediately recursive fragment mentionned in queries', () => { + it('does not infinite loop on immediately recursive fragment mentioned in queries', () => { expectValid(` query myQuery { ...fragA From 11e481c522097891b928013bb1eac1ac62ad591c Mon Sep 17 00:00:00 2001 From: Nicolas Lagoutte Date: Fri, 24 Dec 2021 14:21:33 +0000 Subject: [PATCH 4/5] Patch another infinite loop and add more tests --- .../OverlappingFieldsCanBeMergedRule-test.ts | 48 ++++++++++++++++++- .../rules/OverlappingFieldsCanBeMergedRule.ts | 15 +++++- 2 files changed, 60 insertions(+), 3 deletions(-) diff --git a/src/validation/__tests__/OverlappingFieldsCanBeMergedRule-test.ts b/src/validation/__tests__/OverlappingFieldsCanBeMergedRule-test.ts index 7655ac172a..e3ce615ba9 100644 --- a/src/validation/__tests__/OverlappingFieldsCanBeMergedRule-test.ts +++ b/src/validation/__tests__/OverlappingFieldsCanBeMergedRule-test.ts @@ -1023,14 +1023,47 @@ describe('Validate: Overlapping fields can be merged', () => { it('does not infinite loop on immediately recursive fragment mentioned in queries', () => { expectValid(` - query myQuery { + { + ...fragA + } + + fragment fragA on Query { ...fragA } + `); + }); + + it('does not infinite loop on recursive fragment with a field named after fragment', () => { + expectValid(` + { ...fragA + fragA } fragment fragA on Query { ...fragA } `); }); + it('finds invalid cases even with field named after fragment', () => { + expectErrors(` + { + fragA + ...fragA + } + + fragment fragA on Type { + fragA: b + } + `).toDeepEqual([ + { + message: + 'Fields "fragA" conflict because "fragA" and "b" are different fields. Use different aliases on the fields to fetch both if this was intentional.', + locations: [ + { line: 3, column: 9 }, + { line: 8, column: 9 }, + ], + }, + ]); + }); + it('does not infinite loop on transitively recursive fragment', () => { expectValid(` fragment fragA on Human { name, ...fragB } @@ -1039,6 +1072,19 @@ describe('Validate: Overlapping fields can be merged', () => { `); }); + it('does not infinite loop on transitively recursive fragment mentioned in queries', () => { + expectValid(` + { + ...fragA + fragB + } + + fragment fragA on Human { name, ...fragB } + fragment fragB on Human { name, ...fragC } + fragment fragC on Human { name, ...fragA } + `); + }); + it('finds invalid case even with immediately recursive fragment', () => { expectErrors(` fragment sameAliasesWithDifferentFieldTargets on Dog { diff --git a/src/validation/rules/OverlappingFieldsCanBeMergedRule.ts b/src/validation/rules/OverlappingFieldsCanBeMergedRule.ts index 7a8d65f99d..0cfea1ef26 100644 --- a/src/validation/rules/OverlappingFieldsCanBeMergedRule.ts +++ b/src/validation/rules/OverlappingFieldsCanBeMergedRule.ts @@ -267,10 +267,21 @@ 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) { - // Don't compare this fragment with itself - if (referencedFragmentName === fragmentName) { + // 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, From 2c312eeba895d70cca380de83061bec3e5a6d685 Mon Sep 17 00:00:00 2001 From: Ivan Goncharov Date: Wed, 5 Jan 2022 17:04:15 +0200 Subject: [PATCH 5/5] Review changes --- .../OverlappingFieldsCanBeMergedRule-test.ts | 22 +++++-------------- 1 file changed, 6 insertions(+), 16 deletions(-) diff --git a/src/validation/__tests__/OverlappingFieldsCanBeMergedRule-test.ts b/src/validation/__tests__/OverlappingFieldsCanBeMergedRule-test.ts index e3ce615ba9..9d864902da 100644 --- a/src/validation/__tests__/OverlappingFieldsCanBeMergedRule-test.ts +++ b/src/validation/__tests__/OverlappingFieldsCanBeMergedRule-test.ts @@ -1011,23 +1011,21 @@ describe('Validate: Overlapping fields can be merged', () => { it('does not infinite loop on recursive fragment', () => { expectValid(` + { + ...fragA + } + fragment fragA on Human { name, relatives { name, ...fragA } } `); }); it('does not infinite loop on immediately recursive fragment', () => { - expectValid(` - fragment fragA on Human { name, ...fragA } - `); - }); - - it('does not infinite loop on immediately recursive fragment mentioned in queries', () => { expectValid(` { ...fragA } - fragment fragA on Query { ...fragA } + fragment fragA on Human { name, ...fragA } `); }); @@ -1049,7 +1047,7 @@ describe('Validate: Overlapping fields can be merged', () => { ...fragA } - fragment fragA on Type { + fragment fragA on Type { fragA: b } `).toDeepEqual([ @@ -1065,14 +1063,6 @@ describe('Validate: Overlapping fields can be merged', () => { }); it('does not infinite loop on transitively recursive fragment', () => { - expectValid(` - fragment fragA on Human { name, ...fragB } - fragment fragB on Human { name, ...fragC } - fragment fragC on Human { name, ...fragA } - `); - }); - - it('does not infinite loop on transitively recursive fragment mentioned in queries', () => { expectValid(` { ...fragA