fix(validation): catch OverlappingFieldsCanBeMergedRule violations with nested fragments

[sachindshinde]

In trying to prevent infinite loops, #3442 introduced a bug that causes certain violations of the Field Selection Merging validation to not be caught (released in 16.3.0, and backported to 15.9.0). This PR fixes this bug, while continuing to prevent infinite loops.

Specifically, the code introduced in that PR misuses comparedFragmentPairs in the collectConflictsBetweenFieldsAndFragment() function. That function takes in fields/a selection set and checks whether it conflicts with the given fragment, while recursively checking those fields against nested fragments in the given fragment. The comparedFragmentPairs data structure is meant to store whether fragments have been checked for conflict against other fragments, but the collectConflictsBetweenFieldsAndFragment() does not compare fragments against nested fragments (only the selection set against fragments).

For an example of how this causes violations to be missed, see the test in the first commit. The first fragment causes an entry to be added to comparedFragmentPairs for the pair of the second and third fragments. When the query's selection set introduces a field that conflicts with a field in the third fragment, collectConflictsBetweenFieldsAndFragment() will then erroneously skip the third fragment due to the presence in comparedFragmentPairs. The second commit accordingly reverses the non-test changes of #3442.

The third commit shows a naive approach to preventing infinite loops, where we keep track of fragments seen while recursing into nested fragments in collectConflictsBetweenFieldsAndFragment(). This approach notably doesn't track the set of seen fragments across field boundaries, which is less memory overhead. However this unfortunately introduces a separate bug, shown by the test in the fourth commit. This test creates nested fragments across field boundaries in such a way as to cause an infinite loop (collectConflictsBetweenFieldsAndFragment() eventually calls itself with the same arguments when it recurses into fields). The fifth commit accordingly reverses these changes.

The sixth commit introduces an actual fix. The comparedFragmentPairs data structure has been updated to additionally store a pair of fragment and fields/selection set instead of just two fragments. We then memoize comparisons between fields/fragments at the start of the collectConflictsBetweenFieldsAndFragment() function (similar to the fragment pair memoization at the start of collectConflictsBetweenFragments()). Note that this will increase the size of comparedFragmentPairs to scale with the product of the number of selection sets and the number of fragments, whereas previously it scaled with the square of the number of fragments (though I'm unsure if this extra overhead is significant in the context of full GraphQL validation).

[linux-foundation-easycla]

The committers listed above are authorized under a signed CLA.

• ✅ login: yaacovCR / name: Yaacov Rydzinski (4be3fb8, dd7d0e6)
• ✅ login: sachindshinde / name: Sachin D. Shinde (997923d, 1e2a419, 99f868a, a359a93, e1fb65a, 2d830ca, b2a71fc)

[JoviDeCroock]

This is a great find, just letting you know it's on my radar, will review it more in depth in the following days

[github-actions]

Hi @sachindshinde, I'm @github-actions bot happy to help you with this PR 👋

Supported commands

Please post this commands in separate comments and only one per comment:

• @github-actions run-benchmark - Run benchmark comparing base and merge commits for this PR
• @github-actions publish-pr-on-npm - Build package from this PR and publish it on NPM

[JoviDeCroock]

This makes a lot of sense to me CC @graphql/graphql-js-reviewers for the second review, this seems like a legit bug though

[yaacovCR]

Agree that this is a great find and fix!

Only feedback is whether we can perhaps use a separate compared<*>Map for when comparing fields and fragments, as otherwise the naming gets confusing.

I took a stab at how that might look: sachindshinde/graphql-js@fix-16.3.0-OverlappingFieldsCanBeMergedRule...yaacovCR:suggestions-for-overlapping-fix

We could always do that in a separate PR.

If we do choose to add a separate map, it might make sense to also add a separate ruleContext that bundles all of these maps, which should probably be a separate PR which we can do later: yaacovCR/graphql-js@suggestions-for-overlapping-fix...rule-context

[yaacovCR]

As an aside, applying this PR (and my first suggestion) onto main, we get the following results for our validation benchmarks as compared to main:

(note that the base of this PR is 16.x.x but we will of course need this on main as well -- again, great pickup @sachindshinde !!!!)

[sachindshinde]

Thanks for the reviews!

@yaacovCR
Regarding the first branch you've linked sachindshinde/graphql-js@fix-16.3.0-OverlappingFieldsCanBeMergedRule...yaacovCR:suggestions-for-overlapping-fix , I think there's two things that would be nice to change:

1. The first is that the API for comparedFieldsAndFragmentPairs doesn't actually restrict it to compare just fields to fragments. Callers could accidentally call comparedFieldsAndFragmentPairs.add(fieldMap1, fieldMap2) or comparedFieldsAndFragmentPairs.add(fragmentName1, fragmentName2).
2. The second is that the branch's logic would mean comparedFieldsAndFragmentPairs.add(fieldMap, fragmentName) and comparedFieldsAndFragmentPairs.add(fragmentName, fieldMap) would create two separate entries in the underlying Map<NodeAndDefCollection | string, Map<NodeAndDefCollection | string, boolean>> instead of one.

I've pushed another commit to this PR (the seventh commit), that addresses the above two issues via Typescript function overloading and by providing a comparator for comparedFieldsAndFragmentPairs. (I've also requested re-review from you both.)

[yaacovCR]

Great find, great fix, amazing TS wizardry!

[yaacovCR]

that addresses the above two issues

Looking at this again => it seems that our pairs in comparedFieldsAndFragmentNames collection are known to be of different types, and we always compare in the same order, so the flexibility of PairSet is somewhat unnecessary. Or, in other words, the comparator function will always return the same value.

We could introduce an OrderedPairSet class for that collection.

sachindshinde/graphql-js@fix-16.3.0-OverlappingFieldsCanBeMergedRule...yaacovCR:graphql-executor:thought

Let me know what you think?

Thanks again for finding this!

[yaacovCR]

@sachindshinde

here is the benchmark, shows an essential equivalence, but I would still favor that approach, I think it incidentally makes clearer what's going on in PairSet => let us know what you think when you get a chance!

[yaacovCR]

@sachindshinde -- I took the liberty of pushing my suggestion and merging, if you would like to make additional changes, feel free to open a PR and fix-up as you would like. Thanks again so much for finding this!!