-
-
Notifications
You must be signed in to change notification settings - Fork 60
fix: Auto-stringify ArrayString/ObjectString in FormData #229
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -165,7 +165,7 @@ | |
|
|
||
| "@sinclair/typebox": ["@sinclair/[email protected]", "", {}, "sha512-5HAV9exOMcXRUxo+9iYB5n09XxzCXnfy4VTNW4xnDv+FgjzAGY989C28BIdljKqmF+ZltUwujE3aossvcVtq6g=="], | ||
|
|
||
| "@tokenizer/inflate": ["@tokenizer/inflate@0.3.1", "", { "dependencies": { "debug": "^4.4.1", "fflate": "^0.8.2", "token-types": "^6.0.0" } }, "sha512-4oeoZEBQdLdt5WmP/hx1KZ6D3/Oid/0cUb2nk4F0pTDAWy+KCH3/EnAkZF/bvckWo8I33EqBm01lIPgmgc8rCA=="], | ||
| "@tokenizer/inflate": ["@tokenizer/inflate@0.4.1", "", { "dependencies": { "debug": "^4.4.3", "token-types": "^6.1.1" } }, "sha512-2mAv+8pkG6GIZiF1kNg1jAjh27IDxEPKwdGul3snfztFerfPGI1LjDezZp3i7BElXompqEtPmoPx6c2wgtWsOA=="], | ||
|
|
||
| "@tokenizer/token": ["@tokenizer/[email protected]", "", {}, "sha512-OvjF+z51L3ov0OyAU0duzsYuvO01PH7x4t6DJx+guahgTnBHkhJdG7soQeTSFLWN3efnHyibZ4Z8l2EuWwJN3A=="], | ||
|
|
||
|
|
@@ -271,11 +271,9 @@ | |
|
|
||
| "fdir": ["[email protected]", "", { "peerDependencies": { "picomatch": "^3 || ^4" }, "optionalPeers": ["picomatch"] }, "sha512-tIbYtZbucOs0BRGqPJkshJUYdL+SDH7dVM8gjy+ERp3WAUjLEFJE+02kanyHtwjWOnwrKYBiwAmM0p4kLJAnXg=="], | ||
|
|
||
| "fflate": ["[email protected]", "", {}, "sha512-cPJU47OaAoCbg0pBvzsgpTPhmhqI5eJjh/JIu8tPj5q+T7iLvW/JAYUqmE7KOB4R1ZyEhzBaIQpQpardBF5z8A=="], | ||
|
|
||
| "file-entry-cache": ["[email protected]", "", { "dependencies": { "flat-cache": "^4.0.0" } }, "sha512-XXTUwCvisa5oacNGRP9SfNtYBNAMi+RPwBFmblZEF7N7swHYQS6/Zfk7SRwx4D5j3CH211YNRco1DEMNVfZCnQ=="], | ||
|
|
||
| "file-type": ["[email protected].0", "", { "dependencies": { "@tokenizer/inflate": "^0.3.1", "strtok3": "^10.3.1", "token-types": "^6.0.0", "uint8array-extras": "^1.4.0" } }, "sha512-boU4EHmP3JXkwDo4uhyBhTt5pPstxB6eEXKJBu2yu2l7aAMMm7QQYQEzssJmKReZYrFdFOJS8koVo6bXIBGDqA=="], | ||
| "file-type": ["[email protected].1", "", { "dependencies": { "@tokenizer/inflate": "^0.4.1", "strtok3": "^10.3.4", "token-types": "^6.1.1", "uint8array-extras": "^1.4.0" } }, "sha512-ifJXo8zUqbQ/bLbl9sFoqHNTNWbnPY1COImFfM6CCy7z+E+jC1eY9YfOKkx0fckIg+VljAy2/87T61fp0+eEkg=="], | ||
|
|
||
| "find-up": ["[email protected]", "", { "dependencies": { "locate-path": "^6.0.0", "path-exists": "^4.0.0" } }, "sha512-78/PXT1wlLLDgTzDs7sjq9hzz0vXD+zn+7wypEe4fXQxCmdmqfGsEPQxmiCSQI3ajFV91bVSsvNtrJRiW6nGng=="], | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -317,30 +317,81 @@ const createProxy = ( | |
| if (hasFile(body)) { | ||
| const formData = new FormData() | ||
|
|
||
| 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 | ||
| } | ||
|
|
||
| // FormData is 1 level deep | ||
| for (const [key, field] of Object.entries( | ||
| fetchInit.body | ||
| )) { | ||
| 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 | ||
|
Comment on lines
348
to
379
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧩 Analysis chainArray 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
If the intention is to only auto-stringify for Consider either:
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 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 The array handling logic at lines 349-364 stringifies any array containing non-file objects when
This is a breaking change for existing consumers mixing Required actions:
🤖 Prompt for AI Agents |
||
| } | ||
|
|
||
| if (isServer) { | ||
| if (Array.isArray(field)) | ||
| for (const f of field) | ||
| formData.append(key, f) | ||
| else formData.append(key, field as any) | ||
| for (const f of field) { | ||
| formData.append( | ||
| key, | ||
| await prepareValue(f) | ||
| ) | ||
| } | ||
| else | ||
| formData.append( | ||
| key, | ||
| await prepareValue(field) | ||
| ) | ||
|
|
||
| continue | ||
| } | ||
|
|
@@ -364,7 +415,7 @@ const createProxy = ( | |
| continue | ||
| } | ||
|
|
||
| formData.append(key, field as string) | ||
| formData.append(key, await prepareValue(field)) | ||
| } | ||
|
|
||
| // We don't do this because we need to let the browser set the content type with the correct boundary | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,183 @@ | ||
| import Elysia, { t } from 'elysia' | ||
| import { treaty } from '../src' | ||
| import { describe, expect, it } from 'bun:test' | ||
| import { expectTypeOf } from 'expect-type' | ||
| import { BunFile } from 'bun' | ||
|
|
||
| const app = new Elysia() | ||
| .post('/files', ({ body: { files } }) => files.map((file) => file.name), { | ||
| body: t.Object({ | ||
| files: t.Files() | ||
| }) | ||
| }) | ||
| .post('/any/file', ({ body: { file } }) => file.name, { | ||
| body: t.Object({ | ||
| file: t.File({ type: 'image/*' }) | ||
| }) | ||
| }) | ||
| .post('/png/file', ({ body: { file } }) => file.name, { | ||
| body: t.Object({ | ||
| file: t.File({ type: 'image/png' }) | ||
| }) | ||
| }) | ||
|
|
||
| const client = treaty(app) | ||
| type client = typeof client | ||
|
|
||
| describe('Treaty2 - Using t.File() and t.Files() from server', async () => { | ||
| const filePath1 = `${import.meta.dir}/public/aris-yuzu.jpg` | ||
| const filePath2 = `${import.meta.dir}/public/midori.png` | ||
| const filePath3 = `${import.meta.dir}/public/kyuukurarin.mp4` | ||
|
|
||
| const bunFile1 = Bun.file(filePath1) | ||
| const bunFile2 = Bun.file(filePath2) | ||
| const bunFile3 = Bun.file(filePath3) | ||
|
|
||
| const file1 = new File([await bunFile2.arrayBuffer()], 'aris-yuzu.jpg', { | ||
| type: 'image/jpeg' | ||
| }) | ||
| const file2 = new File([await bunFile1.arrayBuffer()], 'midori.png', { | ||
| type: 'image/png' | ||
| }) | ||
| const file3 = new File([await bunFile3.arrayBuffer()], 'kyuukurarin.mp4', { | ||
| type: 'video/mp4' | ||
| }) | ||
|
|
||
| const filesForm = new FormData() | ||
| filesForm.append('files', file1) | ||
| filesForm.append('files', file2) | ||
| filesForm.append('files', file3) | ||
|
|
||
| const bunFilesForm = new FormData() | ||
| bunFilesForm.append('files', bunFile1) | ||
| bunFilesForm.append('files', bunFile2) | ||
| bunFilesForm.append('files', bunFile3) | ||
|
|
||
| it('check route types', async () => { | ||
| type RouteFiles = client['files']['post'] | ||
|
|
||
| expectTypeOf<RouteFiles>().parameter(0).toEqualTypeOf<{ | ||
| files: Array<File | BunFile> | File | BunFile | ||
| }>() | ||
|
|
||
| type RouteFile = client['any']['file']['post'] | ||
|
|
||
| expectTypeOf<RouteFile>().parameter(0).toEqualTypeOf<{ | ||
| file: File | BunFile | ||
| }>() | ||
|
|
||
| type RouteFileWithSpecific = client['any']['file']['post'] | ||
|
|
||
| expectTypeOf<RouteFileWithSpecific>().parameter(0).toEqualTypeOf<{ | ||
| file: File | BunFile | ||
| }>() | ||
| }) | ||
|
|
||
| it('accept a single Bun.file', async () => { | ||
| const { data: files } = await client.files.post({ | ||
| files: bunFile1 | ||
| }) | ||
|
|
||
| expect(files).not.toBeNull() | ||
| expect(files).not.toBeUndefined() | ||
| expect(files).toEqual([bunFile1.name]) | ||
|
|
||
| const { data: filesbis } = await client.files.post({ | ||
| files: [bunFile1] | ||
| }) | ||
|
|
||
| expect(filesbis).not.toBeNull() | ||
| expect(filesbis).not.toBeUndefined() | ||
| expect(filesbis).toEqual([bunFile1.name]) | ||
|
|
||
| const { data: file } = await client.any.file.post({ | ||
| file: bunFile1 | ||
| }) | ||
|
|
||
| expect(file).not.toBeNull() | ||
| expect(file).not.toBeUndefined() | ||
| expect(file).toEqual(bunFile1.name) | ||
|
|
||
| const { data: pngFile } = await client.png.file.post({ | ||
| file: bunFile2 | ||
| }) | ||
|
|
||
| expect(pngFile).not.toBeNull() | ||
| expect(pngFile).not.toBeUndefined() | ||
| expect(pngFile).toEqual(bunFile2.name) | ||
|
|
||
| const { | ||
| data: notPngFile, | ||
| error, | ||
| status | ||
| } = await client.png.file.post({ | ||
| file: bunFile1 | ||
| }) | ||
|
|
||
| expect(notPngFile).toBeNull() | ||
| expect(error?.status).toBe(422) | ||
| expect(status).toBe(422) | ||
| }) | ||
|
|
||
| it('accept a single regular file', async () => { | ||
| const { data: files } = await client.files.post({ | ||
| files: file1 | ||
| }) | ||
|
|
||
| expect(files).not.toBeNull() | ||
| expect(files).not.toBeUndefined() | ||
| expect(files).toEqual([file1.name]) | ||
|
|
||
| const { data: filesbis } = await client.files.post({ | ||
| files: [file1] | ||
| }) | ||
|
|
||
| expect(filesbis).not.toBeNull() | ||
| expect(filesbis).not.toBeUndefined() | ||
| expect(filesbis).toEqual([file1.name]) | ||
|
|
||
| const { data: file } = await client.any.file.post({ | ||
| file: file1 | ||
| }) | ||
|
|
||
| expect(file).not.toBeNull() | ||
| expect(file).not.toBeUndefined() | ||
| expect(file).toEqual(file1.name) | ||
| }) | ||
|
|
||
| it('accept an array of multiple Bun.file', async () => { | ||
| const { data: files } = await client.files.post({ | ||
| files: [bunFile1, bunFile2, bunFile3] | ||
| }) | ||
|
|
||
| expect(files).not.toBeNull() | ||
| expect(files).not.toBeUndefined() | ||
| expect(files).toEqual([bunFile1.name, bunFile2.name, bunFile3.name]) | ||
|
|
||
| const { data: filesbis } = await client.files.post({ | ||
| files: bunFilesForm.getAll('files') as File[] | ||
| }) | ||
|
|
||
| expect(filesbis).not.toBeNull() | ||
| expect(filesbis).not.toBeUndefined() | ||
| expect(filesbis).toEqual([bunFile1.name, bunFile2.name, bunFile3.name]) | ||
| }) | ||
|
|
||
| it('accept an array of multiple regular file', async () => { | ||
| const { data: files } = await client.files.post({ | ||
| files: [file1, file2, file3] | ||
| }) | ||
|
|
||
| expect(files).not.toBeNull() | ||
| expect(files).not.toBeUndefined() | ||
| expect(files).toEqual([file1.name, file2.name, file3.name]) | ||
|
|
||
| const { data: filesbis } = await client.files.post({ | ||
| files: filesForm.getAll('files') as File[] | ||
| }) | ||
|
|
||
| expect(filesbis).not.toBeNull() | ||
| expect(filesbis).not.toBeUndefined() | ||
| expect(filesbis).toEqual([file1.name, file2.name, file3.name]) | ||
| }) | ||
| }) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧩 Analysis chain
Narrow
shouldStringifyto avoid surprising behavior for non-plain objects (e.g. Date) in multipart bodiesRight now
shouldStringifytreats any non-null object that isn’t detected as a file as JSON-serializable: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).A safer approach would be:
Datespecially, mapping to.toISOString().For example:
This keeps the intended behavior for
t.ArrayString/t.ObjectStringwhile reducing the chance of surprising encodings for other schema types.Also applies to: 418-419
🏁 Script executed:
Length of output: 5033
🏁 Script executed:
Length of output: 540
🏁 Script executed:
Length of output: 3727
🏁 Script executed:
Length of output: 6841
Narrow
shouldStringifyto prevent unintended double-encoding of Date and non-plain objects in multipart form-dataThe current
shouldStringifyimplementation catches all non-null objects, includingDateinstances. This causes unintended double-encoding in form-data bodies:new Date()→JSON.stringify()→"\"2025-01-01T...Z\""new Date()→JSON.stringify()→"2025-01-01T...Z"This is confirmed by
parseStringifiedDateexplicitly removing quotes (line 15 ofparsingUtils.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 maintainingt.ArrayStringandt.ObjectStringbehavior.🤖 Prompt for AI Agents