Fix nested formdata #1607
Fix nested formdata #1607
Conversation
WalkthroughA comprehensive schema transformation refactoring that extracts complex schema replacement and coercion logic into a dedicated module, enhances import resolution and file-type detection, updates ObjectString default value handling, and redistributes schema utilities to improve code organization and maintainability. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Areas requiring extra attention:
Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ 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 |
| /** | ||
| * Resolve a schema that might be a model reference (string) to the actual schema | ||
| */ | ||
| export const resolveSchema = ( |
There was a problem hiding this comment.
This is needed in order to resolve string imported model. Otherwise, t.model for example would not use coerceFormdata when needed.
| return models?.[schema] | ||
| } | ||
|
|
||
| export const hasType = (type: string, schema: TAnySchema): boolean => { |
There was a problem hiding this comment.
I somehow enhanced all edgecase that was previsouly not handled on this function
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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/schema.ts (1)
690-710:additionalPropertiesfallback withonlyFirst: 'object'likely stops recursion too early.In the fallback branch:
schema = replaceSchemaTypeFromManyOptions(schema, { onlyFirst: "object", from: t.Object({}), to(schema) { if (!schema.properties) return schema; if ("additionalProperties" in schema) return schema; return t.Object(schema.properties, { ...schema, additionalProperties: false, }); } });the walker:
- Treats the first object node it sees as
onlyFirst, and- Returns immediately from
walkfor that node without descending into its children.This has two effects:
- If that first object already has
additionalPropertiesset,to()returns the schema unchanged, but children are never visited, so nested objects elsewhere won’t get the default applied.- For non‑object roots (e.g., unions of multiple objects), only the first object encountered in the tree can ever be considered; other object branches are skipped.
If the intent is to ensure that all object schemas lacking
additionalProperties(outside the mainschema.type === 'object'branch) default tofalse,onlyFirstshould not be used here. Consider:schema = replaceSchemaTypeFromManyOptions(schema, { from: t.Object({}), to(schema) { if (!schema.properties) return schema; if ("additionalProperties" in schema) return schema; return t.Object(schema.properties, { ...schema, additionalProperties: false, }); } });This will visit every object node and only add
additionalProperties: falsewhere it’s currently missing.
🧹 Nitpick comments (5)
test/type-system/formdata.test.ts (1)
682-909: Skipped Zod tests are documented, but consider tracking as future work.The skipped Zod tests indicate that nested FormData coercion currently only works with the Elysia type system. This is consistent with the existing coercion behavior mentioned in the PR description.
Consider creating a follow-up issue to track Zod/standard schema support for nested FormData coercion if this is planned for future implementation.
src/index.ts (1)
45-52: Body coercion selection for File/FormData looks correct, but the lambda is duplicated.The new
additionalCoercelogic increateValidatorcorrectly:
- Resolves model strings via
resolveSchema.- Guards on
Kind in resolvedto avoid touching standard schemas.- Uses
hasType('File' | 'Files', resolved)to choosecoerceFormData()vscoercePrimitiveRoot().This should safely limit nested formdata coercion to Elysia/TypeBox schemas containing
File/Files, matching the PR intent.The same IIFE is duplicated in both the precompiled and lazy
createBodybranches, though. Consider extracting a small helper (e.g.getBodyAdditionalCoerce(cloned.body, models, modules)) to centralize this logic and reduce future drift.Also applies to: 167-167, 586-597, 659-665
src/replace-schema.ts (2)
5-19: Traversal semantics forreplaceSchemaTypeFromManyOptions/ReplaceSchemaTypeOptionsfit current call sites.
- The walker correctly:
- Stops at
elysiaMetawrappers to avoid double-wrapping.- Supports
excludeRoot,rootOnly,untilObjectFound, andonlyFirstin a way that matches how you use them (eg, stop at first nested object/array forcoerceFormData, stop before first object for primitive coercions withuntilObjectFound).- Recurses through
oneOf/anyOf/allOf,not,properties, anditemsand then applies the transform at the current node.Two small API nits:
original?: TAnySchemais currently unused anywhere.to(schema: TSchema): TSchema | nullis treated as always returningTSchema;nullis never handled.If you don’t plan to support “skip by returning null” or use
original, consider simplifying the interface toto(schema: TSchema): TSchemaand droppingoriginalto keep the contract tight.Also applies to: 52-152
175-184:revertObjAndArrStrrelies on the internalanyOflayout; document this assumption.The helper correctly de‑wraps
ObjectString/ArrayStringby returninganyOf[1]whenelysiaMetais"ObjectString"/"ArrayString". This is fine as long as:
- The wrapper always uses
[stringBranch, objectOrArrayBranch]ordering.- The inner branch has no
elysiaMeta.Consider explicitly stating this invariant in a comment or adding a very small runtime assert (e.g., check that
anyOf[1].typeis"object"/"array") to make future refactors safer.src/schema.ts (1)
147-183:hasTyperecursion works for File/Files detection, but doesn’t coversomeOf/not.The new implementation:
- Unwraps
Importschemas via$defsand$ref.- Traverses
anyOf/oneOf/allOf, arrays (including a specialFilesas “array of File” case), and object properties.This is enough for the new body coercion check (
hasType('File' | 'Files', ...)) to work across nested/unioned schemas.If you rely on
someOf/notcompositions elsewhere, consider extendinghasTypeto also traversesomeOfandnot(similar tohasAdditionalProperties/hasElysiaMeta) for completeness.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
src/compose.ts(2 hunks)src/index.ts(5 hunks)src/replace-schema.ts(1 hunks)src/schema.ts(7 hunks)src/type-system/index.ts(2 hunks)test/aot/has-type.test.ts(1 hunks)test/type-system/formdata.test.ts(1 hunks)test/type-system/object-string.test.ts(2 hunks)test/units/replace-schema-type.test.ts(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (5)
test/aot/has-type.test.ts (1)
src/schema.ts (1)
hasType(147-183)
test/type-system/formdata.test.ts (1)
src/universal/request.ts (1)
body(81-144)
test/units/replace-schema-type.test.ts (1)
src/replace-schema.ts (2)
revertObjAndArrStr(175-184)coerceFormData(248-266)
src/index.ts (2)
src/schema.ts (2)
resolveSchema(129-145)hasType(147-183)src/replace-schema.ts (2)
coerceFormData(248-266)coercePrimitiveRoot(228-244)
src/schema.ts (4)
src/index.ts (5)
models(453-473)modules(8131-8133)TSchema(8259-8259)replaceSchemaTypeFromManyOptions(8161-8161)t(8138-8138)src/types.ts (1)
StandardSchemaV1Like(58-70)src/replace-schema.ts (1)
replaceSchemaTypeFromManyOptions(37-50)src/utils.ts (1)
randomId(957-976)
🔇 Additional comments (21)
src/type-system/index.ts (2)
306-306: LGTM! Default value now configurable via options.The change from a hard-coded
'{}'tooptions?.defaultcorrectly implements configurable defaults for ObjectString, aligning with the PR objectives. When no default is provided,Value.Createwill now returnundefinedinstead of'{}', which fixes the reported issue #1606.
376-379: LGTM! Metadata addition for consistent schema identification.Adding
{ elysiaMeta: 'ArrayString' }to the Union aligns with the ObjectString pattern and enables consistent schema detection in the transformation pipeline. This supports the enhanced union-based file schema handling introduced in the compose layer.test/aot/has-type.test.ts (1)
71-153: LGTM! Comprehensive test coverage for File type detection.The new test cases thoroughly validate the enhanced
hasTypetraversal logic across multiple scenarios:
- Direct unions containing File
- Import-wrapped schemas at various nesting levels
- Array detection (both
t.Array(t.File())andt.Files())- Negative test cases for schemas without File
The tests align well with the enhanced unwrapping and traversal capabilities added to
hasTypeinsrc/schema.ts.test/type-system/object-string.test.ts (3)
8-15: LGTM! Tests validate the corrected default behavior.The updated test correctly validates that:
Value.Create(t.ObjectString({}))now returnsundefinedinstead of'{}'- Explicitly providing
{ default: '{}' }produces the expected defaultThis directly addresses the issue #1606 objective to restore correct default behavior.
93-110: LGTM! Tests validate optional ObjectString behavior.The test correctly validates that optional ObjectString fields:
- Don't produce validation errors when omitted
- Remain
undefinedinValue.Createoutput- Accept valid values when provided
- Reject invalid empty objects
This directly addresses issue #1606's objective to make
t.Optionalwork properly when wrappingt.ObjectString.
112-130: LGTM! Comprehensive validation of default value behavior.The test thoroughly validates ObjectString with explicit defaults:
Value.Createcorrectly returns the default objectValue.Checkaccepts both object and JSON string formats- Incomplete or undefined inputs fail validation as expected
This ensures the configurable default feature works correctly across various scenarios.
src/compose.ts (2)
65-65: LGTM! Import source updated to reflect module refactoring.The import of
coercePrimitiveRootfrom./replace-schemaaligns with the PR's architectural change to extract schema transformation utilities into a dedicated module. The function usage remains unchanged.
1476-1487: LGTM! Enhanced handling for union-wrapped file schemas.The new logic correctly handles schemas wrapped in unions (e.g., ObjectString, ArrayString) by:
- Attempting to derive properties from
candidate.schema.propertiesortype.properties- Falling back to searching for the object schema within
anyOfbranches- Skipping candidates without resolvable properties
This ensures file validation works correctly with the coerced FormData schemas introduced by this PR.
test/type-system/formdata.test.ts (1)
1-641: LGTM! Comprehensive test suite for nested FormData handling.The test file thoroughly validates the nested FormData feature:
- ✅ Mandatory and optional nested structures
- ✅ File uploads with nested Objects/Arrays
- ✅ JSON string coercion for nested data
- ✅ Validation errors for malformed data
- ✅ Model references with automatic coercion
- ✅ Both AOT and non-AOT modes
The test structure is well-organized with clear describe blocks for each scenario, making it easy to understand the expected behavior.
test/units/replace-schema-type.test.ts (4)
4-8: LGTM! Imports updated to use new public API from replace-schema module.The imports correctly reference the refactored schema transformation utilities:
replaceSchemaTypeFromManyOptionsaliased asreplaceSchemaType- New helpers
revertObjAndArrStrandcoerceFormDatafor transformation workflowsThis aligns with the PR's architectural changes to extract and expose transformation utilities.
103-103: LGTM! Updated to use enhanced transformation API.The existing tests now correctly use the enhanced
tofunction signature that receives the schema being replaced as a parameter. This enables:
- Preserving descriptive properties (line 103)
- Accessing schema properties for transformation (lines 153, 231)
The updates maintain test intent while leveraging the improved API.
Also applies to: 153-153, 231-231
222-562: LGTM! Comprehensive test coverage for transformation options.The new test suites thoroughly validate the schema transformation engine:
- ✅ Basic transformations (Object→ObjectString, Array→ArrayString)
- ✅ Root exclusion (
excludeRoot)- ✅ First-match stopping (
onlyFirst)- ✅ Root-only transformation (
rootOnly)- ✅ Double-wrapping prevention
- ✅ Bottom-up traversal order
- ✅ Multiple transformation passes
- ✅ Composition types (anyOf, oneOf)
Each test validates expected behavior with clear assertions.
564-943: LGTM! Comprehensive validation of reverse transformation and FormData coercion.The test suites thoroughly validate:
Reverse Transformation (lines 564-661):
- ✅ Extracting plain Object/Array from ObjectString/ArrayString
- ✅ Handling non-transformed schemas correctly
coerceFormData (lines 663-943):
- ✅ First-level conversion only (key behavior)
- ✅ Deeper nesting remains unchanged
- ✅ File and Files handling
- ✅ Multiple sibling transformations
- ✅ Complex mixed structures
The tests validate the critical behavior that only first-level nested Objects/Arrays are coerced, which is essential for FormData handling where JSON strings are only at the top level.
src/index.ts (1)
8160-8162: Public aliasreplaceSchemaTypefromreplace-schemais consistent with the refactor.Re‑exporting
replaceSchemaTypeFromManyOptionsasreplaceSchemaTypefrom./replace-schemakeeps the public API name stable while delegating to the new implementation module. No issues from this change alone.src/replace-schema.ts (4)
186-205:stringToStructureCoercionsmatches prior behavior for objects/arrays.The options:
- Convert non‑root objects to
ObjectString(excludeRoot: true).- Convert all arrays to
ArrayString.This aligns with how string‑to‑structure coercion was previously used for headers/params/query. No functional issues here.
206-224:queryCoercionsbehavior is consistent with string coercions, specialized for arrays.The query coercions:
- Wrap non‑root objects with
ObjectString(excludeRoot: true).- Wrap arrays with
ArrayQuery.This keeps query handling parallel to earlier implementations while enabling array‑specific semantics. Looks good.
226-244:coercePrimitiveRootcorrectly targets only the body’s primitive root schema.Defining:
Number → NumericandBoolean → BooleanStringwith
rootOnly: trueensures only the root schema is converted, and children are left untouched. This is appropriate for the “primitive body” case and matches how it’s consumed fromindex.ts.
246-265:coerceFormDataoptions align with “first nested object/array only” semantics.Using:
excludeRoot: trueandonlyFirst: 'object'for objects, andexcludeRoot: trueandonlyFirst: 'array'for arrays,ensures that:
- The route‑level body object is not wrapped.
- The first nested object/array (e.g., a nested payload in multipart/form-data) is converted to
ObjectString/ArrayString, and its children are not recursively re‑wrapped.That matches the nested formdata coercion goal.
src/schema.ts (3)
33-35:resolveSchemahelper behavior is sound and consistent with existing map logic.
resolveSchema:
- Short‑circuits non‑string schemas.
- Prefers
modules.$defs[name]viaImportovermodels[name].This matches how
mapSchemaresolves model names and is appropriate to reuse fromindex.tsfor additionalCoerce selection.Also applies to: 126-145
1236-1277: Cannot verify the review comment: code context not available in repository.The mentioned file
src/schema.tsat lines 1236-1277, thegetCookieValidatorfunction, and thestringToStructureCoercionsfunction from./replace-schemado not appear to exist in the current repository context. The search did not locate these functions or the referenced module, making it impossible to verify the claims about:
- Whether
stringToStructureCoercionsis actually imported from./replace-schema- Whether the cookie coercion behavior is preserved
- Whether the transformation definitions match Elysia's documented cookie coercion behavior
The original approval assessment cannot be confirmed without access to these code elements.
379-437: Consider defensive guards for Import schema resolution if handling untrusted input.When
coerceis true,replaceSchemacorrectly composes Number → Numeric and Boolean → BooleanString rules withadditionalCoerce. When false, onlyadditionalCoerceapplies, matching route body logic expectations.However, accessing
schema.$defs[schema.$ref]in the Import branch without guards could fail if the schema is malformed or constructed outside TypeBox's guarantees. If this function accepts user-provided schemas, add optional chaining:if (schema[Kind] === 'Import') { const refSchema = schema.$defs?.[schema.$ref]; if (refSchema && !hasRef(refSchema)) { let unwrapped = refSchema as TSchema; if (coerce || hasAdditionalCoerce) { unwrapped = replaceSchema(unwrapped); if ('$id' in unwrapped && !unwrapped.$defs) { unwrapped.$id = `${unwrapped.$id}_coerced_${randomId()}`; } } schema = unwrapped; } }to prevent undefined access from malformed imports.
commit: |
2 participants
This fix #1606 and #1138 by adding coerceFormdata capabilities.
At first I just wanted to fix #1606 when I saw that t.Optional was'nt working with t.ObjectString but when changing t.ObjectString default value from '{}' to options.default it was breaking t.ArrayQuery. I was in a dead end.
Then after looking at the code, I understood that Elysia coercion is amazing and that I just needed that out of the box for nested formdata. Thus, I wanted to fix replaceSchema function and somehow end up rewriting it to make it more readable and functional. The root cause was somewhere in the composeProperties function, but in any case with the rewrite it's not needed anymore.
Finally, since the rewrite was working and fixing 1606, I just needed to add a coerceFormdata function such as stringToStructureCoercions for ArrayQuery. And of course a bunch of new tests (most of the PR is new test).
The only drawback is that coerceFormdata is only working for elysia type system. But all already coercion is only working for elysia type system, thus I think it's ok like this.
I hope you will take some time to review it, and, in any case I learned so much reading the code base and for that big thanks!
Summary by CodeRabbit
New Features
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.