fix: resolve "Body already used" when accessing request in derive/resolve hooks #1647
Conversation
WalkthroughPreserve and expose the raw request body when route inference detects request access, add a Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Client
participant Server as Elysia
participant Inference as Sucrose.Inference
participant Composer as compose.parse
participant Parser as app['~parser'] / default parser
participant Hooks as derive/resolve/hooks
Note over Client,Server: Incoming HTTP request with body
Client->>Server: POST /... with body
Server->>Inference: analyze route & handler for `request` usage
alt inference.request = true
Inference->>Server: signal preserve-raw
Server->>Composer: clone & buffer -> set `c.rawBody` (skip multipart)
Composer->>Hooks: run derive/resolve hooks (may read c.rawBody)
Composer->>Parser: parse using `c.rawBody` + content-type
Parser-->>Composer: parsed body or throw ElysiaCustomStatusResponse
Composer->>Server: attach `c.body`, `c.rawBody`, `c.contentType`
else inference.request = false
Server->>Composer: parse directly from request stream
Composer->>Parser: parse from request
Parser-->>Composer: parsed body
Composer->>Server: attach `c.body`
end
Server->>Hooks: invoke route handler with context
Server->>Client: response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches
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}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
test/sucrose/sucrose.test.ts (1)
47-49: Missing test coverage forrequest: trueinference.The tests only verify that
request: falseis correctly inferred. Consider adding test cases that verifyrequest: truewhen hooks destructure or accessrequest, e.g.:
({ request }) => request.arrayBuffer()(c) => c.request.text()This would validate the core fix for issue #1628.
src/compose.ts (2)
920-943: Consider reusing TextDecoder instance for efficiency.The generated code creates
new TextDecoder()multiple times per request. While not critical, you could declare a shared decoder in the function scope:🔎 Suggested optimization
if (inference.request && !hooks.parse?.length) { + fnLiteral += `const _td=new TextDecoder()\n` fnLiteral += `const _ct=c.request.headers.get('content-type')\n` + // ... }Then use
_td.decode(c.rawBody)instead ofnew TextDecoder().decode(c.rawBody).
883-886: Memory consideration for large request bodies.When
inference.requestis true, the entire request body is loaded into memory asArrayBuffer(or cloned for FormData). For large file uploads, this effectively doubles memory usage since both the raw body and parsed body exist simultaneously.This is a necessary tradeoff for the feature, but consider documenting this behavior so users are aware when using
request.arrayBuffer()etc. in hooks with large payloads.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/compose.tssrc/sucrose.tstest/sucrose/sucrose.test.ts
🧰 Additional context used
🧬 Code graph analysis (1)
src/compose.ts (1)
src/index.ts (1)
parser(1395-1441)
🔇 Additional comments (6)
test/sucrose/sucrose.test.ts (1)
196-199: LGTM - Correct behavior for context passed to function.When context is passed to another function, all standard inferences are set to true except
request. This is intentional since passing context doesn't necessarily mean the request body stream will be consumed.src/sucrose.ts (3)
20-21: LGTM - Well-documented new inference field.The JSDoc comment clearly explains the purpose of the
requestfield.
686-687: Key fix: Adding derive and resolve hooks to Sucrose analysis.This ensures that
requestusage in derive/resolve hooks is properly detected, which is essential for the fix to work correctly.
759-770: Intentional omission ofrequestfrom early exit condition.The main loop early exit doesn't include
inference.requestin the check, unlikeinferBodyReferenceat lines 508-519. This appears intentional since detectingrequestaccess shouldn't prevent analysis of other hooks for additional inferences like query/headers/etc.src/compose.ts (2)
945-949: LGTM - FormData handling correctly uses cloned request.For
multipart/form-data, the request is cloned at line 884, and the standard FormData parser reads from the clone. The original request is preserved inc._originalRequestfor hooks.
1044-1099: LGTM - Consistent rawBody handling in hasBodyInference branch.The content-type switch logic correctly handles rawBody parsing when
inference.requestis true, mirroring the single-parser case above.
| // When hooks access request.body/arrayBuffer, preserve raw body before parsing | ||
| // Clone request first so hooks can still read from it | ||
| // Skip if custom parse hooks exist - they'll read the request themselves | ||
| if (inference.request && !hooks.parse?.length) { | ||
| fnLiteral += | ||
| `const _ct=c.request.headers.get('content-type')\n` + | ||
| `if(_ct&&_ct.includes('multipart/form-data')){` + | ||
| `c._originalRequest=c.request;c.request=c.request.clone()` + | ||
| `}else{c.rawBody=await c.request.clone().arrayBuffer()}\n` | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -n '_originalRequest' --type tsRepository: elysiajs/elysia
Length of output: 142
Remove unused c._originalRequest assignment or implement its usage.
The code sets c._originalRequest for multipart/form-data cases (line 884), but this property is never read anywhere in the codebase. Either remove this assignment or add the logic that accesses it (e.g., in derive/resolve hooks).
🤖 Prompt for AI Agents
In src/compose.ts around lines 877 to 886, the code assigns c._originalRequest
for multipart/form-data but that property is never read; either remove the
unused assignment or implement its usage. Option A (clean): delete the
c._originalRequest=c.request assignment and just set c.request=c.request.clone()
so no unused property is created. Option B (implement): keep the assignment but
update the hook logic (derive/resolve or any code that reads request body) to
check for and use c._originalRequest when appropriate (i.e., read form data or
streams from the preserved original request) and ensure any code that clones or
parses the request uses c.request.clone()/c._originalRequest consistently.
eL1fe
force-pushed
the
fix/body-already-used-in-derive
branch
from
9315a1b to
c9e7e39
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/sucrose.ts (1)
759-769: Inconsistent completion check: missinginference.requestin outer loop.The inner loop break condition (lines 517-518) includes
inference.request, but the outer loop break condition here omits it. This inconsistency means:
- Inner loop (inferBodyReference): Stops scanning aliases once all properties including
requestare detected.- Outer loop (sucrose): Continues processing events even after
requestis detected, stopping only when all other properties are found.This likely results in processing more events than necessary. For consistency and optimization, add
inference.requestto this completion check:🔎 Proposed fix
if ( inference.query && inference.headers && inference.body && inference.cookie && inference.set && inference.server && inference.url && inference.route && - inference.path + inference.path && + inference.request ) break
🧹 Nitpick comments (1)
src/compose.ts (1)
954-970: Consider refactoring duplicated custom parser logic.The custom parser handling code (lines 954-970 and 1003-1019) is nearly identical between the
inference.requestbranch and the standard branch. Both blocks:
- Extract and normalize
contentType- Invoke
parser['${parser}'](c, contentType)- Handle promises and
ElysiaCustomStatusResponseWhile this duplication may be intentional for inline code generation performance, consider extracting the shared logic to reduce maintenance overhead and ensure consistency.
Also applies to: 1003-1019
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/compose.tssrc/sucrose.tstest/sucrose/sucrose.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- test/sucrose/sucrose.test.ts
🧰 Additional context used
🧬 Code graph analysis (1)
src/compose.ts (1)
src/index.ts (1)
parser(1395-1441)
🔇 Additional comments (2)
src/compose.ts (1)
877-887: LGTM! The rawBody preservation logic is well-implemented.The code correctly clones the request and preserves
rawBodywheninference.requestis true, allowing derive/resolve hooks to access the original request body. The exclusion ofmultipart/form-datais appropriate since FormData streams cannot be re-read after consumption.Note: The past review comment regarding unused
c._originalRequestappears to be resolved in the current code—no such assignment exists.src/sucrose.ts (1)
686-687: LGTM! Derive and resolve hooks now included in inference.Adding
deriveandresolvehooks to the events list ensures that Sucrose analysis detects request body access patterns within these lifecycle stages. This is essential for the fix, as it allows the framework to preserverawBodywhen these hooks accessrequest.arrayBuffer()or similar methods.
eL1fe
force-pushed
the
fix/body-already-used-in-derive
branch
from
c9e7e39 to
9148279
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/sucrose.ts (1)
759-770: Potential issue: early exit check excludesinference.request.The early-exit optimization at lines 759-770 breaks the loop when 9 inference fields are all
true, butrequestis not included in this check. This could cause the loop to exit before analyzing all hooks forrequestaccess, potentially missing validinference.request = truedetections.🔎 Suggested fix
if ( inference.query && inference.headers && inference.body && inference.cookie && inference.set && inference.server && inference.url && inference.route && - inference.path + inference.path && + inference.request ) break
🧹 Nitpick comments (2)
src/compose.ts (1)
922-944: Consider caching TextDecoder for slight performance improvement.Multiple branches create
new TextDecoder()inline. While correct, a single shared instance at the top would be marginally more efficient for repeated decoding operations.🔎 Suggested optimization
Add a single decoder instance before the switch:
if (inference.request) { + const decoder = new TextDecoder() switch (parser) { case 'json': case 'application/json': if (isOptionalBody) - fnLiteral += 'if(c.rawBody){try{c.body=JSON.parse(new TextDecoder().decode(c.rawBody))}catch{}}else{try{c.body=await c.request.json()}catch{}}\n' + fnLiteral += 'if(c.rawBody){try{c.body=JSON.parse(decoder.decode(c.rawBody))}catch{}}else{try{c.body=await c.request.json()}catch{}}\n' // ... similar for other casesNote: This would require injecting the decoder into the generated function scope.
test/sucrose/sucrose.test.ts (1)
47-49: Add test coverage forrequest: trueinference.All existing tests verify
request: false. A test exercisingrequest: truewould strengthen confidence in the inference logic—for example, a handler that callscontext.request.arrayBuffer()or destructures{ request }.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/compose.tssrc/sucrose.tstest/sucrose/sucrose.test.ts
🧰 Additional context used
🧬 Code graph analysis (1)
src/compose.ts (1)
src/index.ts (1)
parser(1395-1441)
🔇 Additional comments (12)
test/sucrose/sucrose.test.ts (1)
197-199: LGTM!The test correctly expects
request: falseeven when context is passed to a function. This aligns with the implementation whereisContextPassToFunctionintentionally doesn't inferrequestaccess to avoid false positives.src/compose.ts (4)
877-887: LGTM on the rawBody preservation approach.The logic correctly:
- Only activates when
inference.requestis true and no custom parse hooks exist- Checks content-type to skip cloning for multipart/form-data (which can't be re-read)
- Clones the request and reads arrayBuffer to preserve the raw body
This addresses the past review comment about unused
_originalRequest—that assignment has been replaced with thisrawBodyapproach.
946-950: LGTM!The FormData handling correctly uses the adapter parser with an accurate comment explaining the protocol limitation. This aligns with the multipart exclusion in the rawBody preservation logic.
952-970: LGTM!The custom parser handling correctly:
- Uses full parser name lookup (
parser in app['~parser'])- Extracts and normalizes content-type
- Calls the parser and handles async results
- Throws
ElysiaCustomStatusResponseproperlyThe past bug with
parser[0]has been fixed.
1045-1073: LGTM!The rawBody-based parsing for the default case correctly mirrors the adapter-based parsing, using the same
charCodeAt(12)content-type detection. The fallback fortext/plainat index 0 (charCodeAt(0) === 116) is also preserved.src/sucrose.ts (7)
20-22: LGTM!Good addition of the
requestfield with a clear JSDoc comment explaining its purpose.
296-296: LGTM!Follows the established pattern for detecting destructured parameters.
505-519: LGTM!The request inference logic correctly:
- Detects
alias.requestaccess patterns (line 505-506)- Includes
requestin the early-exit optimization check (line 517-518)
576-608: Verify:isContextPassToFunctionintentionally excludesrequestinference.When context is passed to another function, the code sets all inference flags to
trueexceptrequest. This appears intentional to avoid unnecessary request cloning, but it could cause issues if:
- A function receives the context and calls
request.arrayBuffer()- The framework would not have cloned the request, leading to "Body already used"
Is this acceptable because such cases are expected to be rare, or should
inference.requestalso be set totruehere for safety?
686-687: Key fix: derive and resolve hooks now included in Sucrose analysis.This is essential for the PR objective—without analyzing derive/resolve hooks,
inference.requestwould never be detected for code like:.derive(async ({ request }) => { const body = await request.arrayBuffer() // ... })Good addition.
655-656: LGTM!The merge logic correctly propagates
request: truefrom either inference source.
671-672: LGTM!Consistent initialization of
request: falsein both the default inference object and per-function inference.
…olve hooks When derive() or resolve() hooks access request.arrayBuffer()/text()/json(), they fail with "Body already used" because Elysia parses the body before running these hooks. This fix: 1. Adds `request` tracking to Sucrose inference to detect when hooks access the request body 2. When inference.request is true, clones the request before parsing so the original stream remains available for hooks 3. Saves rawBody from the clone and parses body from it, while keeping the original request available for derive/resolve hooks 4. Adds derive and resolve hooks to sucrose analysis (they were missing) Fixes elysiajs#1628
eL1fe
force-pushed
the
fix/body-already-used-in-derive
branch
from
9148279 to
4e4ae18
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/compose.ts (1)
926-928: Consider reusing TextDecoder instance for performance.Creating a new
TextDecoder()instance for each request could be optimized by reusing a single instance, asTextDecoderis stateless for UTF-8 decoding.🔎 Suggested optimization
Add a module-level constant:
const textDecoder = new TextDecoder()Then replace
new TextDecoder().decode(c.rawBody)withtextDecoder.decode(c.rawBody)throughout the generated code.Note: This is a minor optimization and can be deferred.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/compose.tssrc/sucrose.tstest/sucrose/sucrose.test.ts
✅ Files skipped from review due to trivial changes (1)
- test/sucrose/sucrose.test.ts
🧰 Additional context used
🧬 Code graph analysis (1)
src/compose.ts (1)
src/index.ts (1)
parser(1395-1441)
🔇 Additional comments (12)
src/sucrose.ts (8)
20-21: LGTM! Newrequestinference flag properly declared.The new
requestboolean field is correctly added to theInferenceinterface with a clear JSDoc comment explaining its purpose.
296-296: LGTM! Parameter reference detection forrequestis consistent.Follows the same pattern as other inference flags (
query,headers,body, etc.) when detecting root parameter destructuring.
477-477: LGTM! Body reference inference correctly extended forrequest.The
requestflag is properly set both when detected in destructured parameters (line 477) and when accessed via alias (lines 505-506), matching the pattern used for other inference flags.Also applies to: 505-506
517-519: LGTM! Early-exit condition correctly includesrequest.The optimization to break early when all inference flags are true now correctly includes
inference.request.
655-656: LGTM! Merge inference correctly propagatesrequestflag.The
mergeInferencefunction properly combines therequestfield from both inference objects using logical OR.
671-672: LGTM! Default initialization includesrequest: false.Both the main
sucrose()function's default inference and the per-functionfnInferenceare correctly initialized withrequest: false.Also applies to: 724-725
686-687: Critical fix:deriveandresolvehooks now included in Sucrose analysis.This is the key change that enables detection of
requestaccess in derive/resolve hooks, which was the root cause of issue #1628. Without this, the framework couldn't detect when hooks needed access to the raw request body.
768-769: LGTM! Final early-exit condition includesrequest.Consistent with the earlier early-exit condition at lines 517-519.
src/compose.ts (4)
877-887: LGTM! Request cloning logic correctly preserves raw body for hooks.The implementation correctly:
- Only activates when
inference.requestis true and no custom parse hooks exist- Skips cloning for
multipart/form-data(documented protocol limitation)- Clones the request before reading to preserve the original for derive/resolve hooks
- Comments accurately describe the behavior and limitations
This directly addresses issue #1628 by ensuring the request body isn't consumed before hooks can access it.
919-971: LGTM! Parsing from rawBody wheninference.requestis true.The parsing logic correctly:
- Uses
TextDecoderto decoderawBodyfor text-based formats (JSON, text, urlencoded)- Falls back to reading from request when
rawBodyis not available (FormData case)- Handles optional body validation appropriately with try-catch for JSON
- Custom parser check now correctly uses
parser in app['~parser'](addressing previous review feedback)
972-1021: LGTM! Non-inference path preserved for backward compatibility.The original parsing logic is maintained when
inference.requestis false, ensuring no regression for existing use cases without request access in hooks.
1045-1100: LGTM! Default parser switch also handles rawBody for inference.request.The character-code-based switch statement correctly branches between rawBody parsing and standard adapter parsing based on
inference.request, maintaining the same content-type detection logic.
1 participant
Summary
request.arrayBuffer()in derive hooksrequesttracking to Sucrose inference to detect when hooks access the request bodyProblem
When a derive() hook tries to access
request.arrayBuffer(),request.text(), orrequest.json(), it fails with "Body already used" because Elysia parses the body BEFORE running derive hooks:Solution
requestvia Sucrose inferenceTest plan
Summary by CodeRabbit
New Features
Chores
Tests
✏️ Tip: You can customize this high-level summary in your review settings.