Skip to content

fix(graphql-limit-count): measure each fragment once when computing query depth - #13809

Open
shreemaan-abhishek wants to merge 2 commits into
apache:masterfrom
shreemaan-abhishek:graphql-limit-count-fragment-depth
Open

fix(graphql-limit-count): measure each fragment once when computing query depth#13809
shreemaan-abhishek wants to merge 2 commits into
apache:masterfrom
shreemaan-abhishek:graphql-limit-count-fragment-depth

Conversation

@shreemaan-abhishek

Copy link
Copy Markdown
Contributor

Description

graphql-limit-count computes a query's selection-nesting depth (used as the rate-limit cost) by expanding fragment spreads in place. The cycle guard (visited) was cleared after each descent, so a fragment referenced from more than one place was fully re-measured on every occurrence. When fragments reference other fragments repeatedly, the depth computation grows exponentially in the number of fragments, so a small document can become expensive to process in the access phase.

This memoizes each fragment's resolved depth so a fragment is measured once regardless of how many times it is spread. visited still guards fragment cycles; a new memo table caches the per-fragment result, keeping the traversal linear in the document size. The computed depth is unchanged for existing queries.

Added tests: a route whose query reuses fragments many times now returns normally instead of stalling the worker.

Which issue(s) this PR fixes:

Checklist

  • I have explained the need for this PR and the problem it solves
  • I have explained the changes or the new features added to this PR
  • I have added tests corresponding to this change
  • I have updated the documentation to reflect this change
  • I have verified that this change is backward compatible

@dosubot dosubot Bot added size:M This PR changes 30-99 lines, ignoring generated files. bug Something isn't working performance generate flamegraph for the current PR labels Aug 12, 2026
@dosubot dosubot Bot added size:L This PR changes 100-499 lines, ignoring generated files. and removed size:M This PR changes 30-99 lines, ignoring generated files. labels Aug 12, 2026

@membphis membphis left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P2] Fragment memoization caches a cycle-context-dependent depth

memo is keyed only by fragment name, but the cached depth also depends on the current visited recursion stack. With mutually recursive fragments, the first traversal can cache an incomplete depth and reuse it from another path, making the computed cost traversal-order-dependent and potentially undercounting quota usage.

Please avoid memoizing cycle-dependent results, for example by excluding cyclic fragment components from memoization or rejecting cyclic fragment graphs, and add regression tests for mutually recursive fragments reached at different nesting depths and in different traversal orders.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working performance generate flamegraph for the current PR size:L This PR changes 100-499 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants