🔧 fix: keep JSON-looking multipart fields as strings when sche… - #1953
🔧 fix: keep JSON-looking multipart fields as strings when sche…#1953pmenta wants to merge 1 commit into
Conversation
…cts text Eager JSON.parse on single-value form fields since 1.4.23 made t.String() and Standard Schema string fields fail with 422. Parse into a raw body plus an optional structured alternate, then let validation pick the matching interpretation so structured multipart support from elysiajs#1697 stays intact. Fixes elysiajs#1946
WalkthroughMultipart parsing now produces raw and structured interpretations, with shared nested-key and file handling. Dynamic and generated handlers use structured data when body validation requires it, while string-schema behavior and fallback cases receive expanded coverage. ChangesMultipart structured parsing
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Request
participant parseFormData
participant MultipartHandler
participant BodyValidator
Request->>parseFormData: Parse multipart FormData
parseFormData-->>MultipartHandler: Return body and structured form
MultipartHandler->>BodyValidator: Validate raw body
BodyValidator-->>MultipartHandler: Return validation result
MultipartHandler->>BodyValidator: Validate structured form when needed
BodyValidator-->>MultipartHandler: Return accepted body
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
src/adapter/web-standard/index.tsOops! Something went wrong! :( ESLint: 9.39.5 Error: ESLint configuration in --config » plugin:sonarjs/recommended is invalid:
Referenced from: /.eslintrc.json ... [truncated 459 characters] ... /eslintrc/dist/eslintrc.cjs:2952:16) src/compose.tsOops! Something went wrong! :( ESLint: 9.39.5 Error: ESLint configuration in --config » plugin:sonarjs/recommended is invalid:
Referenced from: /.eslintrc.json ... [truncated 459 characters] ... /eslintrc/dist/eslintrc.cjs:2952:16) src/dynamic-handle.tsOops! Something went wrong! :( ESLint: 9.39.5 Error: ESLint configuration in --config » plugin:sonarjs/recommended is invalid:
Referenced from: /.eslintrc.json ... [truncated 459 characters] ... /eslintrc/dist/eslintrc.cjs:2952:16)
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: 4
🧹 Nitpick comments (4)
test/validator/multipart.test.ts (1)
205-249: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDynamic-mode coverage stops right before the interesting cases~ ♡ ( ˘•ω•˘ )
Only TypeBox is exercised with
aot: false. The two divergences I flagged live exactly in the uncovered corners: standard-schema (zod) bodies in dynamic mode, andnoValidateroutes wheresrc/compose.tsadopts the structured form butsrc/dynamic-handle.tsdoes not. Add anaot: false+z.object({ metadata: z.string() })case and anoValidatecase asserting AOT and dynamic agree — otherwise these regress silently, dummy~🤖 Prompt for 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. In `@test/validator/multipart.test.ts` around lines 205 - 249, Add dynamic-mode tests in multipart.test.ts covering a standard-schema body with z.object({ metadata: z.string() }) and JSON text preservation, plus a noValidate route that compares AOT and dynamic responses for the same multipart input. Ensure the assertions verify both modes agree on the structured form behavior.src/adapter/web-standard/index.ts (1)
54-59: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueBare
const form/const fdin a switch-case is asking for trouble later~ (・∀・)ﾉ♡These declarations are emitted directly into
switch(contentType){case ...:}blocks insrc/compose.ts. It works today only because exactly one case emits them; add a second emission site and you get a redeclarationSyntaxErroratFunction()build time. Cheap insurance: wrap the literal in{ ... }.🛡️ Defensive wrap
return ( - `\nconst form=await c.request.formData()\n` + + `\n{const form=await c.request.formData()\n` + `const fd=parseFormData(form)\n` + `c.body=fd.body\n` + - `if(fd.structured!==undefined)c[ELYSIA_STRUCTURED_FORM]=fd.structured\n` + `if(fd.structured!==undefined)c[ELYSIA_STRUCTURED_FORM]=fd.structured}\n` )🤖 Prompt for 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. In `@src/adapter/web-standard/index.ts` around lines 54 - 59, Wrap the emitted form-data statements in the switch case with a block scope by enclosing the generated literal from the relevant adapter method in braces. Keep the existing form parsing and structured-body assignment unchanged while ensuring the declarations from form handling cannot collide with declarations emitted by other cases.src/parse-form-data.ts (1)
87-93: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winObject branch throws away an existing JSON string, unlike the array branch~ ( ˘ ω ˘ )♡
Line 84 politely re-parses a JSON-object string before nesting under it, but line 89 just nukes whatever was there and replaces it with
{}. Someta="{"keep":true}"+meta.extra=vsilently loseskeep, whileitems[0]+items[0].extrakeeps it. Ordering-dependent and a bit sloppy, dummy~ ♡♻️ Suggested consistency fix
} else { // Initialize object property if needed - if (!current[key] || typeof current[key] !== 'object') - current[key] = {} + const existing = current[key] + if ( + !existing || + typeof existing !== 'object' || + Array.isArray(existing) || + (typeof File !== 'undefined' && existing instanceof File) + ) + current[key] = parseObjectString(existing) ?? {} current = current[key] }🤖 Prompt for 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. In `@src/parse-form-data.ts` around lines 87 - 93, Update the object-property initialization branch in the form-data parsing logic to re-parse an existing JSON object string before replacing it, matching the behavior of the array branch around line 84. Preserve valid parsed object contents when nesting subsequent keys, while still initializing `{}` for missing or non-object values.src/compose.ts (1)
1401-1416: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
vsbmeans two different things two screens apart~ (￣▽￣)ﾉ♡Here
vsbis the candidate body value; in the standard branches (lines 1480, 1510)vsbis a check result. Different scopes so it compiles, but debugging generated code with--debugoutput will be a delight. ConsidervsbInput/vsbResult.🤖 Prompt for 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. In `@src/compose.ts` around lines 1401 - 1416, Rename the structured-form candidate variable in structuredFormFallback from vsb to a distinct name such as vsbInput, updating every reference in that generated snippet. Preserve the existing vsb check-result naming in the standard validation branches.
🤖 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 `@src/compose.ts`:
- Around line 1467-1486: Update the standard-provider fallback in the generated
validation flow around validator.body.Check so the structured-form re-check
applies structuredDefault before validating c[ELYSIA_STRUCTURED_FORM]. Reuse the
existing structuredDefault logic, ensuring schemas with default sibling fields
can pass structured validation while preserving the current validation and
assignment behavior.
In `@src/dynamic-handle.ts`:
- Around line 477-483: Update the body decoding logic in the validator body
Decode path to unwrap the { value: ... } wrapper only when the decoder is the
provider that returns that wrapper, rather than based solely on decoded.value.
Preserve decoded bodies unchanged for all other validators, including user
schemas that legitimately contain a value field.
- Around line 458-475: Update the standard-provider branch in the body
validation flow to reuse the initial result from bodyValidator.Check(body)
without invoking it again. When that result has issues, validate structuredForm
and replace context.body only if the structured result passes; otherwise throw
ValidationError for the body using validator.body and the original body.
- Around line 273-277: Update the structuredForm selection in the dynamic
handling path to adopt it when no body validator exists or when the validator’s
body schema has noValidate set to true, matching the behavior in compose.ts.
Preserve validation-based interpretation for routes with an active body
validator that does not disable validation.
---
Nitpick comments:
In `@src/adapter/web-standard/index.ts`:
- Around line 54-59: Wrap the emitted form-data statements in the switch case
with a block scope by enclosing the generated literal from the relevant adapter
method in braces. Keep the existing form parsing and structured-body assignment
unchanged while ensuring the declarations from form handling cannot collide with
declarations emitted by other cases.
In `@src/compose.ts`:
- Around line 1401-1416: Rename the structured-form candidate variable in
structuredFormFallback from vsb to a distinct name such as vsbInput, updating
every reference in that generated snippet. Preserve the existing vsb
check-result naming in the standard validation branches.
In `@src/parse-form-data.ts`:
- Around line 87-93: Update the object-property initialization branch in the
form-data parsing logic to re-parse an existing JSON object string before
replacing it, matching the behavior of the array branch around line 84. Preserve
valid parsed object contents when nesting subsequent keys, while still
initializing `{}` for missing or non-object values.
In `@test/validator/multipart.test.ts`:
- Around line 205-249: Add dynamic-mode tests in multipart.test.ts covering a
standard-schema body with z.object({ metadata: z.string() }) and JSON text
preservation, plus a noValidate route that compares AOT and dynamic responses
for the same multipart input. Ensure the assertions verify both modes agree on
the structured form behavior.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 8695136e-8ec8-46cf-a18a-00a962e27e3c
📒 Files selected for processing (6)
src/adapter/web-standard/index.tssrc/compose.tssrc/dynamic-handle.tssrc/parse-form-data.tstest/units/parse-form-data.test.tstest/validator/multipart.test.ts
| const structuredDefault = | ||
| value !== undefined && | ||
| value !== null && | ||
| typeof value === 'object' && | ||
| !Array.isArray(value) | ||
| ? `vsb=Object.assign(${parsed},vsb)\n` | ||
| : '' | ||
|
|
||
| if (validator.body.provider === 'standard') { | ||
| fnLiteral += | ||
| `let vab=validator.body.Check(c.body)\n` + | ||
| `if(vab instanceof Promise)vab=await vab\n` + | ||
| `if(vab.issues&&c[ELYSIA_STRUCTURED_FORM]!==undefined){` + | ||
| `let vsb=validator.body.Check(c[ELYSIA_STRUCTURED_FORM])\n` + | ||
| `if(vsb instanceof Promise)vsb=await vsb\n` + | ||
| `if(!vsb.issues)vab=vsb` + | ||
| `}\n` + | ||
| `if(vab.issues){` + | ||
| validation.validate('body', undefined, 'vab.issues') + | ||
| '}else{c.body=vab.value}\n' |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Standard-provider fallback checks the structured form without the defaults~ ざんねん (￣ε￣๑)♡
You carefully built structuredDefault on lines 1467-1473... and then only the non-standard path (line 1491) actually uses it. The standard branch validates c[ELYSIA_STRUCTURED_FORM] raw, so a schema with a default sibling field plus a JSON-serialized structured field gets issues on both interpretations and 422s, even though defaults would have made it pass. Line 1456 applies defaults to c.body only.
🐛 Apply defaults before the structured re-check
`if(vab.issues&&c[ELYSIA_STRUCTURED_FORM]!==undefined){` +
- `let vsb=validator.body.Check(c[ELYSIA_STRUCTURED_FORM])\n` +
+ `let vsbInput=c[ELYSIA_STRUCTURED_FORM]\n` +
+ structuredDefault.replace(/^vsb=/, 'vsbInput=') +
+ `let vsb=validator.body.Check(vsbInput)\n` +
`if(vsb instanceof Promise)vsb=await vsb\n` +
`if(!vsb.issues)vab=vsb` +
`}\n` +(or generate the snippet with an explicit variable name parameter instead of a regex swap — that reads better)
Worth adding a standard-schema variant of the "parse structured field when a sibling field has a default" test in test/validator/multipart.test.ts to lock this down.
🤖 Prompt for 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.
In `@src/compose.ts` around lines 1467 - 1486, Update the standard-provider
fallback in the generated validation flow around validator.body.Check so the
structured-form re-check applies structuredDefault before validating
c[ELYSIA_STRUCTURED_FORM]. Reuse the existing structuredDefault logic, ensuring
schemas with default sibling fields can pass structured validation while
preserving the current validation and assignment behavior.
| // Without body validation the structured multipart | ||
| // interpretation is adopted as-is, otherwise validation decides | ||
| // which interpretation matches the schema | ||
| if (structuredForm !== undefined && !validator?.createBody?.()) | ||
| body = structuredForm |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Dynamic mode forgot noValidate, unlike the AOT path~ ヽ(≧□≦)ノ
src/compose.ts line 1184 adopts the structured form when there is no body validator or validator.body.schema?.noValidate === true. Here only the "no validator" half is implemented, so a noValidate route gets the raw strings in dynamic mode and the structured object in AOT mode. Same app, two answers — not very cute, is it? ♡
🩹 Proposed alignment
- if (structuredForm !== undefined && !validator?.createBody?.())
+ if (
+ structuredForm !== undefined &&
+ (!validator?.createBody?.() ||
+ validator.body?.schema?.noValidate === true)
+ )
body = structuredForm📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Without body validation the structured multipart | |
| // interpretation is adopted as-is, otherwise validation decides | |
| // which interpretation matches the schema | |
| if (structuredForm !== undefined && !validator?.createBody?.()) | |
| body = structuredForm | |
| // Without body validation the structured multipart | |
| // interpretation is adopted as-is, otherwise validation decides | |
| // which interpretation matches the schema | |
| if ( | |
| structuredForm !== undefined && | |
| (!validator?.createBody?.() || | |
| validator.body?.schema?.noValidate === true) | |
| ) | |
| body = structuredForm |
🤖 Prompt for 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.
In `@src/dynamic-handle.ts` around lines 273 - 277, Update the structuredForm
selection in the dynamic handling path to adopt it when no body validator exists
or when the validator’s body schema has noValidate set to true, matching the
behavior in compose.ts. Preserve validation-based interpretation for routes with
an active body validator that does not disable validation.
| } else if ( | ||
| structuredForm !== undefined && | ||
| bodyValidator?.provider === 'standard' | ||
| ) { | ||
| let result = bodyValidator.Check(body) as any | ||
| if (result instanceof Promise) result = await result | ||
|
|
||
| if (result?.issues) { | ||
| let structuredResult = bodyValidator.Check( | ||
| structuredForm | ||
| ) as any | ||
| if (structuredResult instanceof Promise) | ||
| structuredResult = await structuredResult | ||
|
|
||
| if (!structuredResult?.issues) | ||
| context.body = body = structuredForm | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -nP -C4 "provider\s*===\s*'standard'" src/dynamic-handle.ts
rg -nP -C3 "aot:\s*false" test --glob '*.ts' | rg -n -B2 -A6 "z\.object|standard" | head -80Repository: elysiajs/elysia
Length of output: 447
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "## dynamic-handle relevant sections"
sed -n '360,490p' src/dynamic-handle.ts | cat -n
echo
echo "## validation error definitions/usages"
rg -n -C3 "class ValidationError|ValidationError|422|status code" src test --glob '*.ts' | head -200
echo
echo "## locate body validators/standard provider"
rg -n -C3 "createBody|provider:" src test --glob '*.ts' | head -240
echo
echo "## deterministic control-flow probe for current branch"
node - <<'JS'
const body = { invalid: true }
const structuredForm = { valid: false }
const bodyValidator = {
provider: 'standard',
issues: false,
Check(value) {
return value === body
? { issues: ['body bad'] }
: structuredForm && value === structuredForm
? { issues: ['structured bad'] }
: { issues: [] }
}
}
let result = bodyValidator.Check(body)
if (result instanceof Promise) result = await result
console.log('first Check === false:', result === false)
if (result?.issues) {
let structuredResult = bodyValidator.Check(structuredForm)
if (structuredResult instanceof Promise) structuredResult = await structuredResult
console.log('body issues:', JSON.stringify(result.issues))
console.log('structured issues:', JSON.stringify(structuredResult.issues))
if (!structuredResult?.issues) console.log('body changed to structuredForm')
else console.log('no structured success, fell through')
}
console.log('final context body changed?', body === structuredForm)
JSRepository: elysiajs/elysia
Length of output: 18736
Fix body validation so standard-provider errors don’t slip through the crack (baka~ ♡)
For provider === 'standard', Check(body) returns an issues-bearing result, so Check(body) === false is skipped and the branch falls back to the raw body when both body and structuredForm contain issues. Throw ValidationError('body', validator.body!, body) unless the structured form actually passes, oho~ (￣ω￣;) Also avoid calling Check(body) again after the first result and letting this result be ignored.
🤖 Prompt for 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.
In `@src/dynamic-handle.ts` around lines 458 - 475, Update the standard-provider
branch in the body validation flow to reuse the initial result from
bodyValidator.Check(body) without invoking it again. When that result has
issues, validate structuredForm and replace context.body only if the structured
result passes; otherwise throw ValidationError for the body using validator.body
and the original body.
| if (validator.body?.Decode) { | ||
| let decoded = validator.body.Decode(body) as any | ||
| if (decoded instanceof Promise) decoded = await decoded | ||
|
|
||
| // Zod returns { value: ... } wrapper | ||
| context.body = decoded?.value ?? decoded | ||
| // Zod returns { value: ... } wrapper | ||
| context.body = decoded?.value ?? decoded | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
decoded?.value ?? decoded will happily eat a legit value field, baka~ ♡
Any decoded body shaped like { value: ... } (a perfectly normal user schema) gets unwrapped to its inner value. Gate the unwrap on the provider instead of guessing from the shape.
- // Zod returns { value: ... } wrapper
- context.body = decoded?.value ?? decoded
+ // Standard Schema (e.g. Zod) returns a { value } wrapper
+ context.body =
+ validator.body.provider === 'standard'
+ ? decoded?.value
+ : decoded📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (validator.body?.Decode) { | |
| let decoded = validator.body.Decode(body) as any | |
| if (decoded instanceof Promise) decoded = await decoded | |
| // Zod returns { value: ... } wrapper | |
| context.body = decoded?.value ?? decoded | |
| // Zod returns { value: ... } wrapper | |
| context.body = decoded?.value ?? decoded | |
| } | |
| if (validator.body?.Decode) { | |
| let decoded = validator.body.Decode(body) as any | |
| if (decoded instanceof Promise) decoded = await decoded | |
| // Standard Schema (e.g. Zod) returns a { value } wrapper | |
| context.body = | |
| validator.body.provider === 'standard' | |
| ? decoded?.value | |
| : decoded | |
| } |
🤖 Prompt for 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.
In `@src/dynamic-handle.ts` around lines 477 - 483, Update the body decoding logic
in the validator body Decode path to unwrap the { value: ... } wrapper only when
the decoder is the provider that returns that wrapper, rather than based solely
on decoded.value. Preserve decoded bodies unchanged for all other validators,
including user schemas that legitimately contain a value field.
1 participant
…ma expects text
Prerequisted
bun run testand they passRelated issue
Fixes #1946
Regression introduced by the multipart normalization in #1697.
Description
Bug
Since 1.4.23, any single-value multipart field whose text starts with
{or[is run throughJSON.parsebefore validation. A route declaring that field as a string receives an object or anarray instead and fails with 422:
The eager parse lives in two places: the inlined form-data program generated by the web-standard
adapter and
normalizeFormValuein the dynamic handler.Why not simply remove the JSON.parse
The eager parse is what lets plain
t.Object/z.objectfields accept JSON-serialized text, soremoving it would regress that behavior and its tests. Making the parser schema-aware is not
possible either: Standard Schema validators expose no introspection API, so the parser cannot
know which fields expect strings.
Fix
Multipart parsing now produces the raw body (every single-value field kept exactly as submitted)
and, when at least one field looks like serialized JSON, a structured alternate where those
fields are deserialized. Validation decides which interpretation matches the schema:
when it passes; otherwise the request fails with the usual 422
noValidate) adopt the structured alternate, keepingtoday's behavior
This works for TypeBox and Standard Schema, in both AOT and dynamic mode. A 422 for a mismatched
structured field now reports the field that actually failed instead of blaming a sibling string
field that was wrongly deserialized.
'{"theme":"dark"}'t.String()/z.string()/ optional / nested / union with string'[1,2,3]'t.String()'{"theme":"dark"}'t.Object(...)/z.object(...)/t.ObjectString(...)t.Object(...)Changes
src/parse-form-data.ts(new): shared implementation of multipart body construction. Thenested and dot-notation helpers (
setNestedValue,parseArrayKey, dangerous-key checks,file+JSON merge) are moved verbatim from
dynamic-handle.ts. The new logic is thedual-interpretation assembly, around 50 lines. The structured alternate is only allocated
when a JSON-looking field exists.
src/adapter/web-standard/index.ts: the inlined codegen string (around 80 lines) is replacedby a call to the injected
parseFormData, following the existingparseQuerypattern. Buninherits it.
src/compose.ts: injectsparseFormDataand theELYSIA_STRUCTURED_FORMcontext symbol(same mechanism as
ELYSIA_REQUEST_ID); adds the validation fallback to the TypeBox andStandard Schema branches, including the
hasDefaultpath.src/dynamic-handle.ts: the three duplicated multipart loops collapse into the shared helper,with the same validation fallback. Standard Schema needs its own branch since
Checkreturnsa result object rather than
false.The diff reads larger than it is: roughly 140 lines are new logic, the rest is moved code and
the deleted inline codegen.
Tests
test/units/parse-form-data.test.ts: 14 cases covering the helper in isolation (raw vsstructured output, dot and array notation, prototype-pollution keys, file+JSON merges,
invalid JSON, primitives)
test/validator/multipart.test.ts: 15 end-to-end cases across TypeBox and zod, AOT anddynamic, including optional, nested and union fields, mixed string+structured bodies,
defaults, no-schema routes, and 422s that must identify the mismatched structured field
tsccleanSummary by CodeRabbit
New Features
Bug Fixes