fix: Auto-stringify ArrayString/ObjectString in FormData #229
Conversation
… single File/BunFile for t.Files() elysiajs#228
WalkthroughEnhances FormData construction logic to automatically handle File objects, BunFile, and nested arrays, introducing helper functions for stringification decisions and value preparation. Updates type signatures to relax File array constraints using generic helpers, enabling more flexible multipart form data composition. Adds comprehensive test coverage for file handling and nested form data scenarios. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant treaty
participant FormData Handler
participant prepareValue
participant Server
Client->>treaty: send multipart form data<br/>(files + nested objects + arrays)
treaty->>FormData Handler: buildFormData(body)
FormData Handler->>FormData Handler: iterate body entries
alt Array field
FormData Handler->>FormData Handler: shouldStringify(array)
alt contains non-file objects
FormData Handler->>FormData Handler: stringify entire array
FormData Handler->>FormData Handler: append as single value
else all file types or primitives
loop each array element
FormData Handler->>prepareValue: prepare(element)
alt File or BunFile
prepareValue->>prepareValue: createNewFile(file)
prepareValue-->>FormData Handler: converted File
else non-file object
prepareValue->>prepareValue: JSON.stringify(object)
prepareValue-->>FormData Handler: stringified value
else primitive
prepareValue-->>FormData Handler: unchanged value
end
FormData Handler->>FormData Handler: append(name, prepared)
end
end
else Non-array field
FormData Handler->>prepareValue: prepare(value)
prepareValue-->>FormData Handler: processed value
FormData Handler->>FormData Handler: append(name, prepared)
end
FormData Handler-->>treaty: FormData constructed
treaty->>Server: POST with FormData
Server-->>Client: response
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related issues
Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 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 |
Teyik0
changed the title
Teyik0
changed the title
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/treaty2/types.ts (1)
2-7: SwitchBunFileto a type-only import to avoid runtime dependency on thebunmoduleThe type helpers are correct:
MaybeArrayFile<T>widens(File | BunFile)[]fields to accept singleFile | BunFileRelaxFileArrays<T>applies this to object bodies and is correctly wired into all non-GET/HEADbodypositions inTreaty.SignThe import at line 5 needs correction:
import { BunFile } from 'bun'This is a value import, which compiles to a runtime
require('bun'). SinceBunFileis only used in type positions (line 64 inMaybeArrayFile<T>), the runtime dependency will break consumers in environments without Bun (Node, browser bundlers, etc.).Change to:
-import { BunFile } from 'bun' +import type { BunFile } from 'bun'This keeps type compatibility without forcing a runtime dependency.
🧹 Nitpick comments (2)
test/treaty2-nested-form.test.ts (1)
5-21: Type alias naming and deep equality on file field could be made more robust
- Reusing the identifier
productModelfor both the schema value and the type alias is valid but hard to read; consider renaming the type to something likeProductModelto follow TS conventions and avoid confusion between value/type spaces.- Both tests assert
toEqual(testProduct), which includes theimage: Bun.file(...)field. This assumes the server returns the exact sameBunFileinstance; if Elysia ever clones or wraps files, these assertions could become brittle. A safer pattern is to assert structural equality on the data parts and check the file separately (e.g. name, size, type).Also applies to: 30-48, 68-82
test/treaty2.test.ts (1)
95-187: Clarify why some/empty-testcases are skipped in the test matrixThe
/empty-testgroup defines more variants (with-maybe-empty,with-optional,with-union-undefined) than are actually exercised in thetest.eachmatrix, where those three are commented out. That’s fine, but it would help future readers if there were a short comment/TODO explaining why those specific cases are excluded (e.g. known limitation, pending fix, or intentionally different behavior).Also applies to: 440-447
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (5)
src/treaty2/index.ts(2 hunks)src/treaty2/types.ts(5 hunks)test/treaty2-files.test.ts(1 hunks)test/treaty2-nested-form.test.ts(1 hunks)test/treaty2.test.ts(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
src/treaty2/types.ts (1)
src/types.ts (1)
Prettify(88-90)
test/treaty2-nested-form.test.ts (1)
src/treaty2/index.ts (1)
treaty(568-594)
test/treaty2-files.test.ts (1)
src/treaty2/index.ts (1)
treaty(568-594)
🔇 Additional comments (1)
test/treaty2-files.test.ts (1)
1-6: BunFile import is actively used in type assertions—review claim is incorrectThe
import { BunFile } from 'bun'on line 5 is not unused. It appears in the type assertions on lines 60, 66, and 72:expectTypeOf<RouteFiles>().parameter(0).toEqualTypeOf<{ files: Array<File | BunFile> | File | BunFile }>()However, one valid issue remains: the third type test on line 69 duplicates line 63. Both assign
client['any']['file']['post']to their respective types. The suggestion to change line 69 toclient['png']['file']['post']is correct and would properly exercise the PNG-specific route.The async
describepattern, while unconventional compared tobeforeAll, is supported by Bun's test runner and works as intended here.Likely an incorrect or invalid review comment.
| const shouldStringify = (value: any): boolean => { | ||
| if (typeof value === 'string') return false | ||
| if (isFile(value)) return false | ||
|
|
||
| // Objects and Arrays should be stringified | ||
| if (typeof value === 'object' && value !== null) { | ||
| return true | ||
| } | ||
|
|
||
| return false | ||
| } | ||
|
|
||
| const prepareValue = async (value: any): Promise<any> => { | ||
| if (value instanceof File) { | ||
| return await createNewFile(value) | ||
| } | ||
|
|
||
| if (shouldStringify(value)) { | ||
| return JSON.stringify(value) | ||
| } | ||
|
|
||
| return value | ||
| } | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
Narrow shouldStringify to avoid surprising behavior for non-plain objects (e.g. Date) in multipart bodies
Right now shouldStringify treats any non-null object that isn’t detected as a file as JSON-serializable:
const shouldStringify = (value: any): boolean => {
if (typeof value === 'string') return false
if (isFile(value)) return false
if (typeof value === 'object' && value !== null) {
return true
}
return false
}This means that in multipart/form-data requests with files:
Dateinstances (and other non-plain objects) will be turned intoJSON.stringify(value)(e.g."\"2025-01-01T...Z\""), which is different from how dates are encoded in the JSON body path ({ date: new Date() }→"2025-01-01T...Z"without the extra quoting).- Any custom value object that shouldn’t be JSON-encoded as a whole will also be stringified.
A safer approach would be:
- Only stringify plain objects and arrays.
- Treat
Datespecially, mapping to.toISOString(). - Leave other object types as-is.
For example:
- const shouldStringify = (value: any): boolean => {
- if (typeof value === 'string') return false
- if (isFile(value)) return false
-
- // Objects and Arrays should be stringified
- if (typeof value === 'object' && value !== null) {
- return true
- }
-
- return false
- }
-
- const prepareValue = async (value: any): Promise<any> => {
- if (value instanceof File) {
- return await createNewFile(value)
- }
-
- if (shouldStringify(value)) {
- return JSON.stringify(value)
- }
-
- return value
- }
+ const isPlainObject = (value: any): boolean =>
+ value !== null &&
+ typeof value === 'object' &&
+ !Array.isArray(value) &&
+ !isFile(value) &&
+ !(value instanceof Date)
+
+ const shouldStringify = (value: any): boolean => {
+ if (typeof value === 'string') return false
+ if (isFile(value)) return false
+ if (Array.isArray(value)) return true
+ if (isPlainObject(value)) return true
+ return false
+ }
+
+ const prepareValue = async (value: any): Promise<any> => {
+ if (value instanceof File) {
+ return await createNewFile(value)
+ }
+
+ if (value instanceof Date) {
+ // Match JSON body behavior for t.Date()
+ return value.toISOString()
+ }
+
+ if (shouldStringify(value)) {
+ return JSON.stringify(value)
+ }
+
+ return value
+ }This keeps the intended behavior for t.ArrayString / t.ObjectString while reducing the chance of surprising encodings for other schema types.
Also applies to: 418-419
🏁 Script executed:
#!/bin/bash
# Examine the code context around lines 320-343 in src/treaty2/index.ts
# and look for related code handling prepareValue, form-data, and Date serialization
# First, check the exact code around the flagged lines
echo "=== Code at lines 320-343 ==="
sed -n '310,350p' src/treaty2/index.ts
echo -e "\n=== Search for Date handling in the file ==="
rg -n 'Date|date' src/treaty2/index.ts -C 3
echo -e "\n=== Search for ArrayString or ObjectString ==="
rg -n 'ArrayString|ObjectString' src/treaty2/index.ts -C 2
echo -e "\n=== Search for prepareValue usage ==="
rg -n 'prepareValue' src/treaty2/index.ts -C 3
echo -e "\n=== Search for test files related to multipart/form-data ==="
fd -e test.ts -e spec.ts -e test.js -e spec.js | head -20Length of output: 5033
🏁 Script executed:
#!/bin/bash
# Look for test coverage of form-data with objects/arrays
# and how Date is currently being serialized
echo "=== Search for multipart/form-data tests ==="
rg -l 'multipart|form-data|FormData' --type=ts --type=js
echo -e "\n=== Search for Date-related tests ==="
rg -n 'new Date\(\)|Date\(' src/treaty2/ --type=ts -A 2 -B 2 | head -50Length of output: 540
🏁 Script executed:
#!/bin/bash
# Check parseStringifiedDate implementation and JSON serialization in the JSON body path
echo "=== parseStringifiedDate implementation ==="
rg -n 'parseStringifiedDate' src/utils/ -A 10 -B 2
echo -e "\n=== Check how JSON body (non-form-data) is serialized ==="
rg -n 'JSON.stringify|JSON.parse' src/treaty2/index.ts -B 3 -A 2 | head -40
echo -e "\n=== Check treaty2-nested-form.test.ts for Date tests ==="
rg -n 'Date|date' test/treaty2-nested-form.test.ts -B 2 -A 2
echo -e "\n=== Check treaty2-files.test.ts for form-data tests ==="
rg -n 'Date|ArrayString|ObjectString' test/treaty2-files.test.ts -B 2 -A 2Length of output: 3727
🏁 Script executed:
#!/bin/bash
# Search for actual test cases using Date in form-data contexts
echo "=== Full content of treaty2-nested-form.test.ts ==="
cat test/treaty2-nested-form.test.ts
echo -e "\n\n=== Full content of treaty2-files.test.ts ==="
head -100 test/treaty2-files.test.ts
echo -e "\n=== Check what isFile() does ==="
rg -n 'const isFile|function isFile' src/treaty2/index.ts -A 5
echo -e "\n=== Search for ArrayString/ObjectString schema definitions ==="
rg -n 'ArrayString|ObjectString' src/ -B 2 -A 2 | head -50Length of output: 6841
Narrow shouldStringify to prevent unintended double-encoding of Date and non-plain objects in multipart form-data
The current shouldStringify implementation catches all non-null objects, including Date instances. This causes unintended double-encoding in form-data bodies:
- Form-data:
new Date()→JSON.stringify()→"\"2025-01-01T...Z\"" - JSON body:
new Date()→JSON.stringify()→"2025-01-01T...Z"
This is confirmed by parseStringifiedDate explicitly removing quotes (line 15 of parsingUtils.ts), which indicates the codebase already works around this issue. The proposal to limit stringification to plain objects and arrays, with special Date handling, is sound and avoids the unnecessary quoting while maintaining t.ArrayString and t.ObjectString behavior.
🤖 Prompt for AI Agents
src/treaty2/index.ts around lines 320-343, the current shouldStringify treats
every non-null object (including Date and other non-plain objects) as needing
JSON.stringify which causes double-encoded dates in multipart form-data; change
shouldStringify to only return true for plain objects and arrays (e.g.,
Array.isArray(value) || isPlainObject(value)), and update prepareValue to
explicitly handle Date (return value.toISOString()) before calling createNewFile
or JSON.stringify so Dates are serialized as ISO strings without extra quotes
and non-plain objects aren’t double-encoded.
| if (Array.isArray(field)) { | ||
| for (let i = 0; i < field.length; i++) { | ||
| const value = (field as any)[i] | ||
| // Check if array contains non-file objects | ||
| // If so, stringify the entire array (for t.ArrayString()) | ||
| // Otherwise, append each element separately (for t.Files()) | ||
| const hasNonFileObjects = field.some( | ||
| (item) => | ||
| typeof item === 'object' && | ||
| item !== null && | ||
| !isFile(item) | ||
| ) | ||
|
|
||
| if (hasNonFileObjects) { | ||
| // ArrayString case: stringify the whole array | ||
| formData.append( | ||
| key as any, | ||
| value instanceof File | ||
| ? await createNewFile(value) | ||
| : value | ||
| JSON.stringify(field) | ||
| ) | ||
| } else { | ||
| // Files case: append each element separately | ||
| for (let i = 0; i < field.length; i++) { | ||
| const value = (field as any)[i] | ||
| const preparedValue = | ||
| await prepareValue(value) | ||
|
|
||
| formData.append( | ||
| key as any, | ||
| preparedValue | ||
| ) | ||
| } | ||
| } | ||
|
|
||
| continue |
There was a problem hiding this comment.
🧩 Analysis chain
Array heuristic may incorrectly JSON-stringify non-file object arrays for non-ArrayString schemas
The new array handling block:
if (Array.isArray(field)) {
const hasNonFileObjects = field.some(
(item) =>
typeof item === 'object' &&
item !== null &&
!isFile(item)
)
if (hasNonFileObjects) {
// ArrayString case: stringify the whole array
formData.append(key as any, JSON.stringify(field))
} else {
// Files case: append each element separately
for (let i = 0; i < field.length; i++) {
const value = (field as any)[i]
const preparedValue = await prepareValue(value)
formData.append(key as any, preparedValue)
}
}
continue
}applies whenever hasFile(body) is true. This means:
- Any array of non-file objects (e.g.
t.Array(t.Object(...))) will be sent as a single JSON string field, not as multiple entries, as soon as the body also contains a file somewhere. - That behavior is perfect for
t.ArrayString(...), but may break endpoints that intentionally use plaint.Array(t.Object(...))+t.File()and expect the form-data representation they had before.
If the intention is to only auto-stringify for t.ArrayString, this heuristic is broader than that and may be a breaking change for existing consumers that mix files with object arrays.
Consider either:
- Narrowing the condition so that only arrays of plain objects that are explicitly meant to be stringified are treated this way (if you can thread some schema metadata or convention down to this layer), or
- At minimum, documenting this as a behavior change and adding explicit tests for
t.Array(t.Object(...))+t.File()so the new semantics are deliberate.
If you keep the current heuristic, it might be worth adding a quick comment here noting that all non-file-object arrays will be JSON-encoded when files are present, not just t.ArrayString cases.
I'll verify this review comment by examining the codebase context and understanding the schema handling. Let me gather information about how the array heuristic interacts with schema types.
🏁 Script executed:
#!/bin/bash
# Get broader context around the problematic code block
cat src/treaty2/index.ts | sed -n '300,400p'Length of output: 4562
🏁 Script executed:
#!/bin/bash
# Find where hasFile is called and defined
rg -n "hasFile" src/treaty2/index.ts -B5 -A5Length of output: 783
🏁 Script executed:
#!/bin/bash
# Look for ArrayString, Array, File schema definitions
fd -e ts -e js src/ | head -20 | xargs rg -l "ArrayString|t\.Array|t\.File" | head -10Length of output: 515
🏁 Script executed:
#!/bin/bash
# Search for test files related to treaty2 and form data handling
fd -e test\. -e spec\. src/ | grep -i treatyLength of output: 321
🏁 Script executed:
#!/bin/bash
# Look for schema type information that might be available at this layer
rg -n "field.*schema\|schema.*type" src/treaty2/index.ts | head -20Length of output: 0
I need to examine the test files and look for schema type handling. Let me fix the commands:
🏁 Script executed:
#!/bin/bash
# Search test files for ArrayString and file handling
rg -n "ArrayString\|t\.Array.*File\|File.*t\.Array" test/ -i -A3 -B3Length of output: 0
🏁 Script executed:
#!/bin/bash
# Look at test files that might cover form data with mixed types
cat test/treaty2-files.test.ts | head -100Length of output: 3093
🏁 Script executed:
#!/bin/bash
# Check for schema type definitions - look for t.ArrayString
rg -n "ArrayString" . --type ts --type js -C5 | head -50Length of output: 3332
🏁 Script executed:
#!/bin/bash
# Look for where body/schema is processed before reaching this layer
rg -n "t\.Array\|t\.Object\|schema" src/treaty2/index.ts | head -30Length of output: 0
🏁 Script executed:
#!/bin/bash
# Check CHANGELOG to see if this is a recent change and what prompted it
cat CHANGELOG.md | head -150Length of output: 4620
Let me examine the test file more comprehensively to see what scenarios are covered:
🏁 Script executed:
#!/bin/bash
# Get full treaty2-nested-form test file to see all test cases
cat test/treaty2-nested-form.test.tsLength of output: 2516
🏁 Script executed:
#!/bin/bash
# Search for plain t.Array (not ArrayString) with files in tests
rg -n "t\.Array\(" test/ -A5 -B5 | grep -A10 -B10 "File\|file"Length of output: 1081
🏁 Script executed:
#!/bin/bash
# Look for any test that combines t.Array with t.File or t.Files
rg -n "post.*body" test/treaty2-nested-form.test.ts -A10 | head -50Length of output: 377
🏁 Script executed:
#!/bin/bash
# Check if there's any scenario testing Array of objects WITHOUT files
rg -n "variants\|metadata" test/treaty2-nested-form.test.ts -B5 -A5Length of output: 0
🏁 Script executed:
#!/bin/bash
# Search all test files for plain t.Array + File combinations (not ArrayString)
rg -n "t\.Array\(" test/ --type ts -A15 | grep -B15 "File\|Files"Length of output: 855
🏁 Script executed:
#!/bin/bash
# Check if there are tests in treaty2.test.ts with object arrays
cat test/treaty2.test.ts | grep -A20 "t.Object"Length of output: 7437
🏁 Script executed:
#!/bin/bash
# Search for any recent PR or comments about this heuristic
rg -n "hasNonFileObjects\|heuristic\|stringify.*array" . --type ts --type jsLength of output: 0
Heuristic for non-file object arrays is too broad and lacks test coverage for plain t.Array(t.Object(...)) + t.File() scenarios
The array handling logic at lines 349-364 stringifies any array containing non-file objects when hasFile(body) is true. However, this heuristic applies to all non-file-object arrays (e.g., t.Array(t.Object(...))), not just t.ArrayString(...) cases, because:
- No schema metadata is available at this serialization layer to distinguish between them
- The condition
hasNonFileObjectsis purely value-based and cannot determine schema intent - This will JSON-stringify plain object arrays whenever files are present in the body
This is a breaking change for existing consumers mixing t.Array(t.Object(...)) with t.File(). Test coverage only includes t.ArrayString + t.File() and t.ObjectString + t.File() scenarios; no tests cover plain t.Array(t.Object(...)) + t.File() combinations.
Required actions:
- Add explicit tests for
t.Array(t.Object(...))+t.File()to confirm expected behavior is intentional - Either narrow the heuristic to only handle
t.ArrayStringcases (if schema metadata can be threaded down), or document this as a behavior change with clear migration guidance - Add a code comment clarifying that all non-file-object arrays are JSON-encoded when files are present, not just
t.ArrayStringcases
🤖 Prompt for AI Agents
In src/treaty2/index.ts around lines 348-379 the value-based heuristic that
JSON-stringifies any array containing non-file objects is too broad and lacks
tests for plain t.Array(t.Object(...)) + t.File() cases; add explicit unit tests
covering t.Array(t.Object(...)) + t.File() to confirm behavior, then either (A)
narrow the heuristic to only stringify arrays known to be ArrayString by
threading schema metadata down to this serialization layer before making the
decision, or (B) if schema metadata cannot be made available, add a clear code
comment at this location stating that all non-file-object arrays will be
JSON-encoded when files are present and update the library documentation and
migration notes accordingly; choose one of the two fixes and implement the
corresponding tests and documentation updates so behavior and intent are
unambiguous.
|
Really need this PR as my code base is now full of JSON.stringtify jank anywhere XDD |
Add uspsa scoring method field Add walkthrough time input
2 participants
This PR auto stringify formdata related to issue #228 when using t.ArrayString and t.ObjectString avoiding manual JSON.stringify.
It also enhance t.File and t.Files support related to this PR in Elysia adding BunFile support to t.File and t.Files.
All test are done and fully working.
Do not merge if the BunFile PR in Elysia related is not merged too.
With this PR Documentation effort should be done to guide users that want to use nested data with files.
Summary by CodeRabbit
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.