fix(normalizr): Add entity depth limit to prevent stack overflow in denormalization#3823
fix(normalizr): Add entity depth limit to prevent stack overflow in denormalization#3823
Conversation
…enormalization Large bidirectional entity graphs (e.g., Department → Building → Department) with thousands of unique entities cause RangeError: Maximum call stack size exceeded during denormalization. The existing same-pk cycle detection doesn't help because every entity in the chain has a unique pk. Add a depth counter (limit 128) at the entity boundary in unvisit. Entities beyond the limit are returned via createIfValid without resolving nested schema fields. A console.error is emitted once per denormalize call in dev mode. Benchmarked with no measurable regression on denormalizeLong (~538 ops/sec before and after, well within 1-3% run-to-run variance). Closes #3822 Made-with: Cursor
🦋 Changeset detectedLatest commit: 36d2c51 The changes in this PR will be included in the next version bump. This PR includes changesets to release 11 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
|
Size Change: +81 B (+0.1%) Total Size: 80.5 kB
ℹ️ View Unchanged
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Benchmark React
Details
| Benchmark suite | Current: 36d2c51 | Previous: 85c9ce3 | Ratio |
|---|---|---|---|
data-client: getlist-100 |
168.08 ops/s (± 5.4%) |
186.93 ops/s (± 3.6%) |
1.11 |
data-client: getlist-500 |
46.41 ops/s (± 7.3%) |
44.25 ops/s (± 6.2%) |
0.95 |
data-client: update-entity |
476.19 ops/s (± 4.6%) |
444.66 ops/s (± 5.3%) |
0.93 |
data-client: update-user |
454.55 ops/s (± 7.6%) |
434.78 ops/s (± 6.3%) |
0.96 |
data-client: getlist-500-sorted |
51.69 ops/s (± 3.0%) |
50.51 ops/s (± 6.3%) |
0.98 |
data-client: update-entity-sorted |
400 ops/s (± 9.0%) |
400 ops/s (± 7.8%) |
1 |
data-client: update-entity-multi-view |
400 ops/s (± 6.6%) |
384.62 ops/s (± 11.4%) |
0.96 |
data-client: list-detail-switch-10 |
13.42 ops/s (± 5.4%) |
13.29 ops/s (± 8.9%) |
0.99 |
data-client: update-user-10000 |
104.17 ops/s (± 1.5%) |
101.01 ops/s (± 5.8%) |
0.97 |
data-client: invalidate-and-resolve |
50.51 ops/s (± 4.0%) |
45.45 ops/s (± 6.9%) |
0.90 |
data-client: unshift-item |
253.21 ops/s (± 5.8%) |
263.16 ops/s (± 6.2%) |
1.04 |
data-client: delete-item |
384.62 ops/s (± 4.4%) |
434.78 ops/s (± 0.0%) |
1.13 |
data-client: move-item |
229.92 ops/s (± 6.2%) |
232.56 ops/s (± 3.4%) |
1.01 |
This comment was automatically generated by workflow using github-action-benchmark.
There was a problem hiding this comment.
Benchmark
Details
| Benchmark suite | Current: 36d2c51 | Previous: b3b1b61 | Ratio |
|---|---|---|---|
normalizeLong |
450 ops/sec (±1.38%) |
450 ops/sec (±1.74%) |
1 |
normalizeLong Values |
419 ops/sec (±0.26%) |
411 ops/sec (±0.55%) |
0.98 |
denormalizeLong |
284 ops/sec (±2.99%) |
278 ops/sec (±3.87%) |
0.98 |
denormalizeLong Values |
265 ops/sec (±2.08%) |
263 ops/sec (±2.11%) |
0.99 |
denormalizeLong donotcache |
1000 ops/sec (±0.12%) |
1047 ops/sec (±0.14%) |
1.05 |
denormalizeLong Values donotcache |
772 ops/sec (±0.10%) |
781 ops/sec (±0.41%) |
1.01 |
denormalizeShort donotcache 500x |
1581 ops/sec (±0.13%) |
1573 ops/sec (±0.07%) |
0.99 |
denormalizeShort 500x |
861 ops/sec (±2.26%) |
862 ops/sec (±2.13%) |
1.00 |
denormalizeShort 500x withCache |
6287 ops/sec (±0.16%) |
6324 ops/sec (±0.14%) |
1.01 |
queryShort 500x withCache |
2752 ops/sec (±0.09%) |
2774 ops/sec (±0.21%) |
1.01 |
buildQueryKey All |
52253 ops/sec (±0.43%) |
54717 ops/sec (±0.42%) |
1.05 |
query All withCache |
6193 ops/sec (±0.14%) |
5767 ops/sec (±0.35%) |
0.93 |
denormalizeLong with mixin Entity |
281 ops/sec (±1.96%) |
276 ops/sec (±2.07%) |
0.98 |
denormalizeLong withCache |
7397 ops/sec (±0.18%) |
7090 ops/sec (±0.20%) |
0.96 |
denormalizeLong Values withCache |
5123 ops/sec (±0.11%) |
5085 ops/sec (±0.45%) |
0.99 |
denormalizeLong All withCache |
5972 ops/sec (±0.11%) |
5666 ops/sec (±0.21%) |
0.95 |
denormalizeLong Query-sorted withCache |
6193 ops/sec (±0.16%) |
5935 ops/sec (±0.16%) |
0.96 |
denormalizeLongAndShort withEntityCacheOnly |
1742 ops/sec (±0.17%) |
1749 ops/sec (±0.27%) |
1.00 |
denormalize bidirectional 50 |
2838 ops/sec (±1.97%) |
||
denormalize bidirectional 50 donotcache |
26640 ops/sec (±0.83%) |
||
getResponse |
4709 ops/sec (±0.54%) |
4684 ops/sec (±0.66%) |
0.99 |
getResponse (null) |
10288118 ops/sec (±0.78%) |
10272874 ops/sec (±0.98%) |
1.00 |
getResponse (clear cache) |
269 ops/sec (±1.94%) |
268 ops/sec (±2.25%) |
1.00 |
getSmallResponse |
3436 ops/sec (±0.09%) |
3418 ops/sec (±0.17%) |
0.99 |
getSmallInferredResponse |
2523 ops/sec (±0.06%) |
2483 ops/sec (±0.12%) |
0.98 |
getResponse Collection |
4635 ops/sec (±0.41%) |
4632 ops/sec (±0.43%) |
1.00 |
get Collection |
4624 ops/sec (±0.25%) |
4615 ops/sec (±0.30%) |
1.00 |
get Query-sorted |
5281 ops/sec (±0.23%) |
5278 ops/sec (±0.51%) |
1.00 |
setLong |
462 ops/sec (±0.19%) |
456 ops/sec (±0.54%) |
0.99 |
setLongWithMerge |
258 ops/sec (±0.20%) |
258 ops/sec (±0.21%) |
1 |
setLongWithSimpleMerge |
272 ops/sec (±0.21%) |
274 ops/sec (±0.16%) |
1.01 |
setSmallResponse 500x |
948 ops/sec (±0.07%) |
948 ops/sec (±0.10%) |
1 |
This comment was automatically generated by workflow using github-action-benchmark.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #3823 +/- ##
=======================================
Coverage 98.06% 98.07%
=======================================
Files 151 151
Lines 2843 2855 +12
Branches 556 561 +5
=======================================
+ Hits 2788 2800 +12
Misses 11 11
Partials 44 44 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Expand bidirectional entity graph tests to cover uncovered branches in depthLimitEntity: missing entity at depth boundary, validation failure at depth limit, console.error assertion, MemoCache truncation behavior, and full resolution within the limit. Made-with: Cursor
|
Sorry late to the conversation here, and forgive me if I don't have all the context, but isn't the issue here that graph traversal (BF or DF) is failing to detect that it's visited a particular record already and so doesn't no if it needs to continue visiting it's relationstions? Especially in the presence of sparse relationship sets as per https://jsonapi.org/format/#fetching-sparse-fieldsets Would it be possible to expose affordances for detecting 'visisted' payload records based on something like a hash of the payload for a particular record, and a method of merging if they are different? (ie: two sparse field records requested from different queries) |
|
@joegaudet-atreides Cycle detection (hitting an already visited record) is done with https://github.com/reactive/data-client/blob/master/packages/normalizr/src/denormalize/localCache.ts set. Specifically https://github.com/reactive/data-client/blob/master/packages/normalizr/src/denormalize/unvisit.ts#L84 Merging of multiple of the same records in a response is done with https://dataclient.io/rest/api/Entity#merge. this can be overridden to behave in any manner you choose. |
2 participants
Fixes #3822
Motivation
Large bidirectional entity graphs with thousands of unique entities cause
RangeError: Maximum call stack size exceededduring denormalization. For example,Department → Building → Department → Building → ...where each entity has a unique pk creates a traversal chain thousands of frames deep. The existing same-pk cycle detection doesn't help because no(type, pk)pair repeats — every hop visits a genuinely new entity.Solution
Add a depth counter at the entity boundary in
unvisit()(packages/normalizr/src/denormalize/unvisit.ts). When the entity nesting depth reaches 128, entities are returned viacreateIfValidwithout resolving nested schema fields (FK fields stay as normalized ids). Aconsole.erroris emitted once perdenormalize()call in dev mode to help users diagnose the issue.Key design decisions:
getUnvisitclosure — onelet depthper denormalize invocation, zero API surface changeBenchmarked with no measurable regression —
denormalizeLongmedian 538 → 531 ops/sec (within 1-3% run-to-run variance). Thedepth++/depth--and one branch check are completely lost in the noise of existing per-entity work.Also adds a bidirectional chain benchmark (
denormalize bidirectional 50) for ongoing regression tracking.Open questions
N/A
Made with Cursor
Note
Medium Risk
Changes core denormalization behavior for very deep entity graphs by truncating nested resolution after 128 entity hops, which could surface partially-denormalized data in pathological schemas. Guardrails are in place (dev-only warning, validation via
createIfValid, and targeted tests), but it still affects a central codepath.Overview
Prevents
RangeError: Maximum call stack size exceededduring denormalization of large bidirectional/cross-type entity graphs by enforcing a max entity depth of 128 inpackages/normalizr/src/denormalize/unvisit.ts.When the limit is reached, entities are returned as validated instances with nested fields left as unresolved ids (and a single
console.errorin dev to aid diagnosis). Adds comprehensive regression tests for deep bidirectional chains, a benchmark scenario (denormalize bidirectional 50), and release/docs + changeset entries for the patch bump.Written by Cursor Bugbot for commit 36d2c51. This will update automatically on new commits. Configure here.