You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@apisix.apache.org by "nic-6443 (via GitHub)" <gi...@apache.org> on 2026/08/16 14:43:21 UTC
[PR] fix(graphql-limit-count): expand each fragment once when measuring depth [apisix]
nic-6443 opened a new pull request, #13830:
URL: https://github.com/apache/apisix/pull/13830
### Description
`node_depth()` in `graphql-limit-count` clears `visited[name]` right after walking a fragment spread, so the same fragment is walked again from every spread that references it. A document where each fragment spreads the next one twice therefore costs `2^n` traversals for `n` fragments — a ~1KB body reaches a billion node visits, and that runs in the `access` phase on an unauthenticated request, before the rate limit the plugin enforces. #1389 capped the body at 1MB, which still leaves plenty of room for this.
A fragment's depth doesn't depend on where it is spread from, so it's now computed once per document and memoized. The depth the plugin reports is unchanged; only the traversal cost is, from exponential to linear in the number of fragments.
The new test in `t/plugin/graphql-limit-count.t` sends a 26-fragment document of that shape and asserts the resulting quota consumption. It passes in milliseconds with this change and times out on master.
#### Which issue(s) this PR fixes:
N/A
### Checklist
- [x] I have explained the need for this PR and the problem it solves
- [x] I have explained the changes or the new features added to this PR
- [x] I have added tests corresponding to this change
- [ ] I have updated the documentation to reflect this change — no user visible behaviour change
- [x] I have verified that this change is backward compatible (If not, please discuss on the [APISIX mailing list](https://github.com/apache/apisix/tree/master#community) first)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(graphql-limit-count): expand each fragment once when measuring depth [apisix]
Posted by "nic-6443 (via GitHub)" <gi...@apache.org>.
nic-6443 commented on PR #13830:
URL: https://github.com/apache/apisix/pull/13830#issuecomment-5314319557
Closing as a duplicate of #13809, which I missed when I opened this — it landed 5 days earlier with the same fix (memo table threaded through `node_depth`, same `if cached then` short-circuit, same per-document memo hoisted in `access`) and a stronger test: 34 levels of doubling fragments versus my 26, plus the depth-correctness assertion. Nothing here to carry over. Sorry for the noise.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(graphql-limit-count): expand each fragment once when measuring depth [apisix]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #13830:
URL: https://github.com/apache/apisix/pull/13830#discussion_r3792004402
##########
apisix/plugins/graphql-limit-count.lua:
##########
@@ -101,30 +107,38 @@ local function node_depth(node, fragments, visited)
if not name or visited[name] then
return 0
end
+ local cached = depths[name]
+ if cached then
+ return cached
+ end
Review Comment:
`depths[name]` can legitimately be `0` (e.g., a fragment whose selections contain only leaf fields). The current cache check `if cached then` treats `0` as a cache miss, causing repeated recomputation and undermining the intended memoization for zero-depth fragments. Use a nil-check instead so `0` is cached correctly.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(graphql-limit-count): expand each fragment once when measuring depth [apisix]
Posted by "nic-6443 (via GitHub)" <gi...@apache.org>.
nic-6443 closed pull request #13830: fix(graphql-limit-count): expand each fragment once when measuring depth
URL: https://github.com/apache/apisix/pull/13830
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(graphql-limit-count): expand each fragment once when measuring depth [apisix]
Posted by "nic-6443 (via GitHub)" <gi...@apache.org>.
nic-6443 commented on code in PR #13830:
URL: https://github.com/apache/apisix/pull/13830#discussion_r3792151302
##########
apisix/plugins/graphql-limit-count.lua:
##########
@@ -101,30 +107,38 @@ local function node_depth(node, fragments, visited)
if not name or visited[name] then
return 0
end
+ local cached = depths[name]
+ if cached then
+ return cached
+ end
Review Comment:
Not an issue here: in Lua only `nil` and `false` are falsy, `0` is truthy, so `if cached then` returns a memoized zero from the cache just like any other value. A `nil` check would read the same but suggest `0` needs special handling, which it doesn't.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org