fix(core): Price every ProductVariant in relation arrays, avoid spread RangeError - #5058
Conversation
getRelationEntityAtPath() collected a terminal relation array with result.push(...target), which expands the array into call arguments and exceeds V8's stack budget on very large arrays (e.g. collection.productVariants on a big catalog), throwing RangeError: Maximum call stack size exceeded. This is the same defect that vendurehq#4986 fixed in getMissingRelations() in this class; the sibling occurrence was missed there. Replaced with a for...of loop, which is overflow-safe and, unlike forEach, preserves undefined holes in the collected result.
When hydrate() is called with applyProductVariantPrices, whether an array-valued relation received prices was decided by sampling element [0] (entity[0] instanceof ProductVariant). A relation array can contain null/undefined elements - getRelationEntityAtPath() pushes them deliberately - so the sample was wrong in both directions: a hole at [0] silently skipped pricing for every real ProductVariant in the array, while a ProductVariant at [0] passed the holes behind it into applyChannelPriceAndTax(), which dereferences its argument and throws a TypeError out of hydrate(). The ProductVariants to price are now selected per element with a type-guard filter, which fixes both directions at once: every real ProductVariant is priced, and holes never reach the applicator.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthrough
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 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 |
michaelbromley
added
the
T2: Elevated risk
HouseinIsProgramming
left a comment
HouseinIsProgramming
left a comment
There was a problem hiding this comment.
Solid follow-up to #4986. One ask, one suggestion.
1. Missing tests. getProductVariantsToPrice() is only tested with arrays. The non-array branch, which handles pricing a single variant (e.g. hydrate(ctx, orderLine, { relations: ['productVariant'], applyProductVariantPrices: true })), has no coverage: it could regress and no test would fail. Please add two cases: bare ProductVariant returns [variant], undefined returns [].
2. There is a problem that your fix might expose, would be nice if we guard against it.
The pricing step now runs applyChannelPriceAndTax() for all variants concurrently via Promise.all, without any limit.
Before your fix, it was being stopped by the RangeError crash which capped it. Now that the limit is gone, we would need to bound the Promise.all somehow.
I see no upside to the concurrency here, correct me if I am wrong.
So consider adding a limit or sequentialising this. ProductVariantService.assignProductVariantsToChannel has a similar pattern you can follow.
Will approve once both points above are addressed, thanks for the thorough PR.
Relation arrays are unbounded in size since the RangeError fix, so the pre-existing unlimited Promise.all could fan out arbitrarily many concurrent applyChannelPriceAndTax() calls. Applied per variant in sequence instead, following ProductVariantService.assignProductVariantsToChannel().
|
@HouseinIsProgramming Both points are in.
What I ran after the changes: hydrator spec 24/24, full core unit suite 1146, Thanks for the quick pickup and for pointing at the existing pattern to follow. |
HouseinIsProgramming
left a comment
HouseinIsProgramming
left a comment
There was a problem hiding this comment.
Good fix. The tests fail for the right reasons in both directions, and the newly-priced variants already had their price relations fetched, so there is no new exposure. The isTranslatable half is covered in #5059.
3 participants
Description
This is the first half of the follow-up agreed in #4986. @michaelbromley wrote there:
and @biggamesmallworld added a note on reachability:
This PR covers the price-applicator half, plus a sibling spread overflow in the same class. The
isTranslatable()half needs a change intranslate-entity.tsto avoid regressingnullrelations intoundefined, so it goes in companion PR #5059 to keep each review focused.Fix 1:
[0]sampling in the price application (second commit)Whether an array-valued relation received prices was decided by
entity[0] instanceof ProductVariant, butentity.map(...)then calledapplyChannelPriceAndTax()on every element — andapplyChannelPriceAndTax()dereferences its argument on its first statement (variant.productVariantPrices). SincegetRelationEntityAtPath()deliberately pushesnull/undefinedelements into its result, the[0]sample was wrong in both directions:[null, variant]variant[variant, null]TypeErrorout ofhydrate()variant[variant, variant]So
[0]was not acting as a guard — it was a coin flip on where the hole sits. The variants to price are now selected with a per-element type-guard filter (newgetProductVariantsToPrice()helper), which fixes both directions at once. Note that widening the gate alone (e.g.entity.some(e => e instanceof ProductVariant)) would have made things worse: it converts both silent-skip rows above into crashes because the body still maps over every element. I verified that variant against the new tests and it fails both of them.On reachability, to be precise: every core
@ManyToOneintoProductVariantis non-nullable, so pure core code mostly cannot produce the mixed array; the live path is theundefinedhole on entities held in plugin/event-handler code (the #4955 lineage), which #4986's hole-preservation made more visible, as noted in the quote above.Fix 2: spread
RangeErroringetRelationEntityAtPath()(first commit)getRelationEntityAtPath()collects a terminal relation array withresult.push(...target)— the exact defect you had me fix ingetMissingRelations()in #4986 (spreading a very large array into call arguments exceeds V8's stack budget and throwsRangeError: Maximum call stack size exceeded). I missed this sibling occurrence in the same class when writing that fix; found it while scoping this follow-up. Replaced with the samefor...ofloop merged in #4986.forEachwould also avoid theRangeErrorbut silently skips holes (measured: 999,999 elements collected instead of 1,000,000), which matters precisely because downstream code now handles those holes per element.The two fixes are in one PR because they are causally linked: fix 2 un-blocks arrays above ~110k elements so they now reach the price application instead of aborting
hydrate()— landing them together means the path fix 2 opens is already safe on arrival.Breaking changes
None. Arrays whose element
[0]was a hole now get their real ProductVariants priced (the documented intent ofapplyProductVariantPrices), and very large terminal relation arrays no longer throw. A variant that master's[0]sample skipped and this PR now prices takes exactly the exposure master already takes today on every array whose[0]is a variant —getRequiredProductVariantRelations()appends.taxCategory/.productVariantPricesfor those paths under the same option flag. Cost-wise, the filter adds oneinstanceofcheck per element, only underapplyProductVariantPrices: true, on an array the code already walks to price.Testing
Unit (
entity-hydrator.service.spec.ts, 7 new cases, no DB):hydrate()(stubbed query builder, realProductPriceApplicatorwith stubbed strategies) and are the regression guards for fix 1 — one per failure direction. RED on master:[null, variant]leaveslistPriceunset;[variant, null]rejects withTypeError: Cannot read properties of null (reading 'productVariantPrices').[variant, variant]case; passes before and after.[],[null, undefined], non-ProductVariant entities). These are spec-locks for the helper, not regression proofs — stated here so the coverage isn't overread.RangeErrorat thepush(...target)line; the hole assertion is what rules out aforEach-based fix. Costs ~35 ms / ~58 MB transient on an M-series Mac (measured), cheaper than the neighbouring 200k-distinct-objects fixture.Robustness checks on the tests themselves: I mutation-tested the suite — reverting each fix individually, substituting the
.some()gate for fix 1 andforEachfor fix 2 — and confirmed every mutant turns at least one of the new tests red (after asserting the mutant actually applied).What I ran:
entity-hydrator.service.spec.tsbefore the fixesentity-hydrator.service.spec.tsafterentity-hydrator.e2e-spec.ts(rebuilt dist)No new e2e: no core relation can produce the mixed array (non-nullable FKs, see reachability note), so an e2e would need a permanent nullable-FK fixture change purely to demonstrate a shape core cannot produce, and would only exercise one failure direction per run depending on row order. The DB-free call-site tests cover both directions deterministically. The pre-existing type error in
order.service.ts:1460that stopsnpm run buildlocally is unrelated (file untouched, reproduces on clean master).🤖 AI assistance
I used Claude Code while working on this. I have reviewed every line of the change myself, and the testing table above reflects test runs I actually performed and observed — including verifying that each new regression test fails without its fix and passes with it.
Checklist
📌 Always:
👍 Most of the time:
@codesmith-botwith what you need. Autofix is disabled.