fix(core): Prevent exponential re-merge of shared entity references - #5085
Open
TheHypnoo wants to merge 4 commits into
Open
fix(core): Prevent exponential re-merge of shared entity references#5085TheHypnoo wants to merge 4 commits into
TheHypnoo wants to merge 4 commits into
Conversation
Fixes vendurehq#5083. Since vendurehq#4945, mergeDeep() tracks only the current recursion path, so a source object reachable by more than one path is re-merged once per path. Hydrated entity graphs are loaded with the 'query' relation strategy, which gives every referencing parent the same instance of a related entity, so path counts multiply with each level of sharing. On an Order in a multi-channel setup the merge cost grows an order of magnitude faster than the graph itself, and deeper graphs stop completing at all. Memoise each (target, source) pair instead: merging a source into a target is idempotent, so a given pair only needs to be merged once. The path-scoped cycle detection and the shared-instance behaviour added in vendurehq#4945 are unchanged.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthrough
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/core/src/service/helpers/entity-hydrator/merge-deep.spec.ts`:
- Around line 118-122: Replace the Date.now()-based performance assertion around
mergeDeep with a deterministic instrumented shared leaf that throws if its
properties are enumerated more than once. Keep the shared leaf referenced by
both a and b, and assert mergeDeep completes without triggering the throw so
repeated-path processing fails immediately.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 9b21e0cf-61b1-4f89-a0ea-2dff296d8355
📒 Files selected for processing (2)
packages/core/src/service/helpers/entity-hydrator/merge-deep.spec.tspackages/core/src/service/helpers/entity-hydrator/merge-deep.ts
michaelbromley
added
the
T2: Elevated risk
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Fixes #5083.
mergeDeep()re-merges a source object once for every path that reaches it, so hydration cost grows with the number of paths through the entity graph rather than the number of objects in it.Cause
Before #4945,
visitedaccumulated every source object seen during ahydrate()call, so each object was merged once. #4945 changed it to track only the current recursion path (visited.delete(b)on the way out) so that an instance shared by several targets is merged into each of them, which is correct — but it also means a shared instance is re-merged once per path that reaches it, and path counts multiply with each level of sharing.The graphs this runs on are full of shared instances by construction:
EntityHydrator.hydrate()loads withrelationLoadStrategy: 'query', which gives every referencing parent the same instance of a related entity. That is the same property the e2e test added in #4945 relies on (order.lines[0].productVariant === order.lines[1].productVariant).Fix
Memoise each
(target, source)pair. Merging a source into a target is idempotent, so a given pair only needs merging once, and the cost goes back to being a function of the graph size. The path-scoped cycle detection and the shared-instance behaviour from #4945 are both unchanged.Two alternatives were considered and rejected:
WeakMap<source, mergedResult>) is also linear, but aliases the merged sub-object between targets, so a target loses data it already had. The second unit test covers this.Measurements
Real data, sqljs e2e: an Order with 3 lines whose ProductVariants reference the same N Channels, each Channel sharing one Zone.
mergeDeepcalls during the secondhydrate()of the Order — the state a plugin sees after an order mutation leaves one line without the relation.Master's cost relative to the pre-#4945 implementation keeps climbing with the amount of sharing (4.0x, 6.7x, 9.0x, 9.8x, 10.4x). This PR settles at ~1.6x and stops growing; that remainder is the work #4945 requires, since merging a shared source into each distinct target is strictly more merging than skipping it.
The e2e seed data only has two levels of sharing, so it shows a large constant factor rather than a hang. The shape that stops completing is covered by the first unit test: on a graph with
depth + 2objects and2 ^ (depth + 1)paths and no cycles, master takes 11.6s at depth 22 and this PR takes under a millisecond.Trade-off
If the first merge of a pair was cut short by the cycle guard, the memo prevents it being completed later via another path. This matches the pre-#4945 behaviour, where a global
visitedhad the same effect, so it is not a new limitation.Tests
should merge each object once rather than once per path— bounds the cost on a cycle-free graph whose path count is exponential in its depth. Fails by assertion (11.6s) without the fix, not by timeout.should keep the existing data of every target a shared source is merged into— guards that the memo is keyed on the pair rather than on the source alone.Verified green: 1139 core unit tests, 25
entity-hydratore2e tests, 197order/shop-ordere2e tests.No e2e test is added. With a channel count small enough for CI it would not fail against the regression, so it would add runtime without detecting anything; the exponential shape is what the unit test pins down.
Breaking changes
None.
mergeDeepis exported from@vendure/core, so the memo is threaded through a new optional fourth parameter rather than by changing the type of the existingvisitedparameter.Collapsing both into a single
WeakMap<target, WeakSet<source>>would be tidier — the pair memo alone already guarantees termination on cycles, sovisitedbecomes redundant — but that changes cycle behaviour as well as cost, so it seemed better kept out of a patch fix. Happy to do it that way instead if you prefer.Checklist
📌 Always:
👍 Most of the time:
Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.