fix(16.x.x.): avoid stack size limit in overlapping field rules as well as execute

[JoviDeCroock]

Fixes https://github.com/graphql/graphql-js/issues/3938

This moves us from our recursion when we are in a fragment to discovering more referenced fragments to a stack-iterator based approach. The memory consumption might be slightly higher during validation now but this will stop us from crashing which is less desired I presume.

EDIT: found some bugs, working on it (fixed in https://github.com/graphql/graphql-js/pull/4116/commits/18f81bd5af0c9a45b11a27380c156db4e16f7b60)

[github-actions[bot]]

Hi @JoviDeCroock, 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]

@github-actions run-benchmark

[github-actions[bot]]

@github-actions run-benchmark

@JoviDeCroock Please, see benchmark results here: https://github.com/graphql/graphql-js/actions/runs/9579845798/job/26413168621#step:6:1

[JoviDeCroock]

@yaacovCR applied your suggestion, thank you!

[yaacovCR]

Does this approach get us to validating nested named fragments of any depth? What was the old limit, what is the new? We presumably have a maximum depth secondary to a stack limit in execution as well with respect to field collection…. Is that now comparable?

[JoviDeCroock]

This validates nested named fragments as the second test does. I'm not aware of any stack depth limitations in the implementation nor in the spec, this makes it limitless basically so no more runtime crash.

The old limit was runtime dependent so i.e. 900 in Node vs 9100 in Chrome

[yaacovCR]

I'm not aware of any stack depth limitations in the implementation nor in the spec, this makes it limitless basically so no more runtime crash.

There are none in the spec.

But our execution implementation uses synchronous recursion and so we should hit limits both in collect fields and in field execution:

Eg https://github.com/graphql/graphql-js/issues/4031

I am personally not quite sure what they are tbh. When I am no longer on mobile and have enough bandwidth, I can try to take a closer look at this validation algorithm, but at first glance, it seems with your change to actually not necessarily use recursion at all for the nested named fragments, as advertised, dissimilar to collect fields and therefore allow a deeper limit/no limit. I don’t think that’s a blocker to approval => just curious, perhaps it might make sense to do something similar within collect fields.

[yaacovCR]

I think this can also be merged into main

[JoviDeCroock]

@yaacovCR you are right though, this still fails in execution https://github.com/graphql/graphql-js/pull/4116/commits/d9c4bd3eefb50dbe6ffa15b0c21b76e5d761bcd5 I can look at this separately or as part of this PR, as you prefer

[yaacovCR]

I would be fine with either. I suppose we might want @graphql/tsc to address whether it's OK to change the simple recursive algorithm from the spec into something iterative.

It is certainly valid according to the specification, but perhaps there is some benefit in preserving a closer correspondence for the reference implementation. That small benefit might have to be weighed against the practical costs for this library users.

If I had a vote, I would vote in favor!

[JoviDeCroock]

@yaacovCR I've replaced it in https://github.com/graphql/graphql-js/pull/4116/commits/4407b5539d96839ee83caf29fb115ab4946fd057

[yaacovCR]

Is it possible to unify visited Fragment names set with the new encountered fragments collection?

Thinking about this more, I don't think you can.

[yaacovCR]

I think this can also be merged into main

@JoviDeCroock this seems like a straightforward fix that if approved should go both in v16.x and v17. It doesn't matter probably whether we start with merging into main or v16, but we should do both, right, and I guess in general for most things the idea would be to start with main and then backport as necessary. [An exception was the globalThis issue where we are not quite sure I think which direction we want to take for the next version.]

[yaacovCR]

@yaacovCR I've replaced it in 4407b55

Just for the record, this will fix arbitrarily nested fragments at a single level => which is cool => but it's not like we can have arbitrarily deep queries, the completion algorithm within execution is still recursive, so we will hit a limit. This is I think fine, but just want to make explicit that we are not necessarily solving the issue in #4031 ....

[yaacovCR]

@JoviDeCroock this seems like a straightforward fix

I think we have an ordering problem so it no longer seems so straightforward for the execution part.

[JoviDeCroock]

@yaacovCR what impact would ordering have, I thought that the whole collectFields pass would mitigate any ordering issues 😅 that being said, in practice switching to shift over pop would make these go away I reckon.

[yaacovCR]

Preserving the serialized map ordering https://spec.graphql.org/draft/#sec-Serialized-Map-Ordering

I think this whole approach would cause all fragments to appear after fields, so I think that would also have to be fixed.

[JoviDeCroock]

@yaacovCR the stack based depth first iterator of https://github.com/graphql/graphql-js/pull/4116/commits/43e36cc6f5c8e99ef69ac368af615e990f5a7de8 should resolve that

[JoviDeCroock]

@github-actions run-benchmark

[github-actions[bot]]

@github-actions run-benchmark

@JoviDeCroock Please, see benchmark results here: https://github.com/graphql/graphql-js/actions/runs/9684136411/job/26721184177#step:6:1

[JoviDeCroock]

Closing this out due to the lack of real world scenarios where this will occur. This also changes the implementation of the spec where we rely on recursion in the spec text, the scenario of > 1000 deep fragment-spreading, ... seems unlikely. If it however becomes a more apparent issue we can re-open this PR.