fix(core): support oneOf/anyOf in multipart/form-data code generation#3245
Conversation
Fixes orval-labs#3242 Two independent bugs in getResReqTypes and getSchemaFormDataAndUrlEncoded when a multipart/form-data request body uses oneOf or anyOf. Bug 1: effectivePropName was only resolved when the media type schema was a direct $ref. When wrapped in oneOf/anyOf (common with NestJS decorators like @nestjs/swagger or zod-to-openapi for versioned schemas), the FormData code referenced a variable derived from the controller name instead of the DTO name, which was never declared. Bug 2: generating a sequential formData.append() block per oneOf/anyOf variant duplicated fields shared across variants at runtime, because TypeScript casts are erased at compile-time and every branch executes. Replaced the per-variant blocks with a single Object.entries() loop.
Adds three tests alongside the existing FormData composition suite: - oneOf with a single $ref: the generated FormData references a variable derived from the DTO name, not the controller-derived name that would never be declared (Bug 1). - oneOf with two $refs sharing a field: the generated FormData uses a single Object.entries() loop and does not duplicate per-variant appends via TypeScript casts (Bug 2). Variant types stay importable. - allOf: still emits per-field appends (no regression in the composition path that kept the original behavior).
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughDerive FormData parameter names from inner Changes
Sequence Diagram(s)(omitted) Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested labels
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
packages/core/src/getters/res-req-types.test.ts (1)
551-724: Good coverage for the happy-path regressions; consider extending.The three new cases lock in exactly the behavior described in the PR (DTO-derived propName, single
Object.entriesloop,allOfunchanged). Two follow-ups worth adding once the companion source-file issues are resolved:
- A test where the
oneOf/anyOfbody is optional (required: false) to pin down the guard againstObject.entries(undefined).- A test where a shared field is a file array (
Blob[]) to codify that multi-file fields undershouldCastare not JSON-stringified.Not blocking for this PR — flagging so the regressions discussed on the source file are also caught here.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/getters/res-req-types.test.ts` around lines 551 - 724, Add two additional tests to packages/core/src/getters/res-req-types.test.ts using the existing getResReqTypes test patterns: (1) a oneOf/anyOf multipart/form-data case where the requestBody has required: false to assert the generated formData code guards against Object.entries(undefined) (look for result = getResReqTypes(...)[0], then inspect formData for absence of unguarded Object.entries calls); (2) a oneOf case where a shared field is an array of files (e.g., type: 'array' items: { type: 'string', format: 'binary' } / Blob[]) and assert the generated formData does not JSON.stringify that multi-file field (inspect formData string for no JSON.stringify(uploadRequestBody.<field>) and that file append logic is present). Use the same ctx/context, reqBody, result, and formData variables and assertions style as the neighboring tests.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/core/src/getters/res-req-types.ts`:
- Around line 596-601: The loop inside getSchemaFormDataAndUrlEncoded that
pushes imports from resolveSchemaRef into the local additionalImports array (for
combinedSchemas) produces dead write-only code because additionalImports is
never used or returned; either remove that loop and the unused additionalImports
variable, or extend the function's return contract to include and return the
collected additionalImports so callers (or getFormDataAdditionalImports) can
consume them; locate getSchemaFormDataAndUrlEncoded, the additionalImports
declaration, the combinedSchemas loop, and resolveSchemaRef usage and either
delete the unused accumulation or add the additionalImports to the returned
object and update callers (e.g., getFormDataAdditionalImports call sites) to
handle the new return shape.
- Around line 259-277: The oneOf/anyOf handling in getResReqTypes silently falls
back when variants are non-$ref which can leave effectivePropName pointing to a
non-existent identifier and also shadows the outer name parameter; fix by only
assigning effectivePropName when at least one resolved ref yields a valid
identifier, rename the inner const name (from
resolveSchemaRef(...).imports[0]?.name) to avoid shadowing (e.g., refName), and
otherwise leave effectivePropName as propName (or explicitly comment that
mixed/inline variants fall back to the old path); update the branch around
getSchemaOneOf/getSchemaAnyOf, isReference loop, and resolveSchemaRef usage and
add a unit test covering mixed $ref + inline oneOf/anyOf to ensure no invalid
prop names are produced.
- Around line 582-594: The generated Object.entries block improperly references
File, ignores optional bodies, converts Blob/File elements to "{}" when inside
arrays, and turns null into the string "null"; fix by guarding the top-level
iteration with a nullish/default check (use isRequestBodyOptional to use
optional chaining or fallback to {} so Object.entries won't receive undefined),
change the value check to skip both undefined and null (value != null), guard
the File instanceof check with typeof File !== 'undefined' (keep the existing
Blob and Buffer.isBuffer checks), and in the Array.isArray(value) branch append
Blob/File/Buffer elements directly instead of JSON.stringify them while
serializing only plain object elements and primitives (so Blob/File elements are
preserved).
---
Nitpick comments:
In `@packages/core/src/getters/res-req-types.test.ts`:
- Around line 551-724: Add two additional tests to
packages/core/src/getters/res-req-types.test.ts using the existing
getResReqTypes test patterns: (1) a oneOf/anyOf multipart/form-data case where
the requestBody has required: false to assert the generated formData code guards
against Object.entries(undefined) (look for result = getResReqTypes(...)[0],
then inspect formData for absence of unguarded Object.entries calls); (2) a
oneOf case where a shared field is an array of files (e.g., type: 'array' items:
{ type: 'string', format: 'binary' } / Blob[]) and assert the generated formData
does not JSON.stringify that multi-file field (inspect formData string for no
JSON.stringify(uploadRequestBody.<field>) and that file append logic is
present). Use the same ctx/context, reqBody, result, and formData variables and
assertions style as the neighboring tests.
🪄 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
Run ID: 70eae977-fd97-423a-8cc4-62780a2d7a7a
📒 Files selected for processing (2)
packages/core/src/getters/res-req-types.test.tspackages/core/src/getters/res-req-types.ts
| } else if (mediaType.schema) { | ||
| // When schema is a oneOf/anyOf of $refs, concat schema names for consistent param naming | ||
| const combinedRefs = | ||
| getSchemaOneOf(mediaType.schema) ?? | ||
| getSchemaAnyOf(mediaType.schema); | ||
| if (combinedRefs) { | ||
| const names: string[] = []; | ||
| for (const ref of combinedRefs) { | ||
| if (!isReference(ref)) continue; | ||
| const name = resolveSchemaRef(ref, context).imports[0]?.name; | ||
| if (name) { | ||
| names.push(name); | ||
| } | ||
| } | ||
| if (names.length > 0) { | ||
| effectivePropName = names.join(''); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Silent fallback when oneOf/anyOf contains non-$ref variants.
The branch only collects names for $ref entries inside oneOf/anyOf and silently falls back to propName if no refs are found (e.g., inline object variants, or a oneOf combining a $ref with an inline variant). That can still leave effectivePropName referring to a variable that doesn't exist in the generated signature, re-creating a softer version of the bug this PR fixes.
Also, const name = resolveSchemaRef(ref, context).imports[0]?.name shadows the outer name parameter of getResReqTypes, which is easy to misread.
Not a blocker given the explicit scope of the fix (OpenAPI specs where oneOf/anyOf wraps named DTOs, like NestJS), but worth adding a test or a comment noting that mixed/inline oneOf variants still take the old path.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/core/src/getters/res-req-types.ts` around lines 259 - 277, The
oneOf/anyOf handling in getResReqTypes silently falls back when variants are
non-$ref which can leave effectivePropName pointing to a non-existent identifier
and also shadows the outer name parameter; fix by only assigning
effectivePropName when at least one resolved ref yields a valid identifier,
rename the inner const name (from resolveSchemaRef(...).imports[0]?.name) to
avoid shadowing (e.g., refName), and otherwise leave effectivePropName as
propName (or explicitly comment that mixed/inline variants fall back to the old
path); update the branch around getSchemaOneOf/getSchemaAnyOf, isReference loop,
and resolveSchemaRef usage and add a unit test covering mixed $ref + inline
oneOf/anyOf to ensure no invalid prop names are produced.
| for (const subSchema of combinedSchemas) { | ||
| const { imports } = resolveSchemaRef(subSchema, context); | ||
| if (imports[0]) { | ||
| additionalImports.push(imports[0]); | ||
| newPropName = `${propName}${pascal(imports[0].name)}`; | ||
| newPropDefinition = `const ${newPropName} = (${propName} as ${imports[0].name}${isRequestBodyOptional ? ' | undefined' : ''});\n`; | ||
| } | ||
|
|
||
| return ( | ||
| newPropDefinition + | ||
| resolveSchemaPropertiesToFormData({ | ||
| } |
There was a problem hiding this comment.
additionalImports is write-only dead code in this function.
additionalImports is declared at line 564 but never returned or read — getSchemaFormDataAndUrlEncoded returns only form. The imports for oneOf/anyOf variants are actually collected by getFormDataAdditionalImports at the call sites (lines 217-220, 351-354), which is what the new tests assert against.
This mirrors pre-existing dead code for the combinedSchemas path, but since the shadowing-free loop here is newly added, consider either removing it or wiring additionalImports into the return contract. Current version just allocates and discards objects.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/core/src/getters/res-req-types.ts` around lines 596 - 601, The loop
inside getSchemaFormDataAndUrlEncoded that pushes imports from resolveSchemaRef
into the local additionalImports array (for combinedSchemas) produces dead
write-only code because additionalImports is never used or returned; either
remove that loop and the unused additionalImports variable, or extend the
function's return contract to include and return the collected additionalImports
so callers (or getFormDataAdditionalImports) can consume them; locate
getSchemaFormDataAndUrlEncoded, the additionalImports declaration, the
combinedSchemas loop, and resolveSchemaRef usage and either delete the unused
accumulation or add the additionalImports to the returned object and update
callers (e.g., getFormDataAdditionalImports call sites) to handle the new return
shape.
Address review feedback on orval-labs#3245. - Guard `File` reference with `typeof File !== 'undefined'` so the generated code does not throw a ReferenceError in non-DOM environments. - Wrap the iterated body with `?? {}` to defend against `Object.entries(undefined)` when the request body is optional. - In the `Array.isArray` branch, append `Blob`/`File`/`Buffer` elements directly instead of serializing them with `JSON.stringify`, which would otherwise turn binary array items into the string "{}". - Skip both `null` and `undefined` values so `null` is no longer coerced to the string "null". - Drop the dead push to the local `additionalImports` array; variant imports are already collected by `getFormDataAdditionalImports`. - Rename inner const to `refName` to avoid shadowing the outer `name` parameter. - Add two regression tests covering the optional-body and array-of-files cases.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/core/src/getters/res-req-types.test.ts (1)
660-662: Minor: assertion is weaker than intended.
(formData.match(/\file`/g) ?? []).lengthcounts occurrences of the literal backtick-quoted key ``file``. In the pre-fix output, per-variant appends would have emitted `` formData.append(file, ...) `` for each variant, so this check is meaningful. However, since the fix-path emitsObject.entriesusingkey(not ``file``), this count is trivially0regardless of whether shared-field duplication was actually deduplicated. TheObject.entries(+not.toMatch(/as UploadDtoV[12]/)assertions already cover the intent; consider dropping theappendFileCountcheck or replacing it with an assertion that counts.append(calls for the shared field under the old per-variant path (e.g., assert nouploadDtoV1.file/uploadDtoV2.file` references) to make the regression guard meaningful on any future refactor.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/getters/res-req-types.test.ts` around lines 660 - 662, The existing appendFileCount assertion is too specific to the old backtick-quoted key and is now meaningless; remove the appendFileCount calculation and its expect, or replace it with an assertion that actually checks for per-variant `.append(` usages (e.g., assert formData does not contain references like `uploadDtoV1.file` or `uploadDtoV2.file`) so the test for shared-field deduplication in the res-req-types.test (the formData string and the existing not.toMatch(/as UploadDtoV[12]/) checks) remains a meaningful regression guard.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/core/src/getters/res-req-types.test.ts`:
- Around line 660-662: The existing appendFileCount assertion is too specific to
the old backtick-quoted key and is now meaningless; remove the appendFileCount
calculation and its expect, or replace it with an assertion that actually checks
for per-variant `.append(` usages (e.g., assert formData does not contain
references like `uploadDtoV1.file` or `uploadDtoV2.file`) so the test for
shared-field deduplication in the res-req-types.test (the formData string and
the existing not.toMatch(/as UploadDtoV[12]/) checks) remains a meaningful
regression guard.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 38a0b757-b26a-4a61-a477-df5822ae6310
📒 Files selected for processing (2)
packages/core/src/getters/res-req-types.test.tspackages/core/src/getters/res-req-types.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/core/src/getters/res-req-types.ts
melloware
left a comment
melloware
left a comment
There was a problem hiding this comment.
Good feedback from the AI code review please implement if possible
- Update nine orval-tests file snapshots to reflect the new Object.entries codegen for multipart/form-data with oneOf/anyOf request bodies: fetch/form-data-optional-request, fetch/form-data-with-custom-fetch, fetch/importFromSubdirectory, react-query/form-data, react-query/form-data-with-hook, react-query/importFromSubdirectory (endpoints.ts and endpoints.msw.ts), swr/form-data-optional-request, vue-query/form-data. - Replace the weak `file` backtick count assertion with checks against per-variant references (uploadDtoV1.file, uploadDtoV2.file), which remain meaningful if a future refactor reintroduces the cast-based append pattern.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/__snapshots__/vue-query/form-data/endpoints.ts (1)
92-133:⚠️ Potential issue | 🔴 CriticalDuplicate FormData entries when outer schema has both
oneOf/anyOfand directproperties.The generated code in this snapshot—and identically in the other five updated snapshots (
fetch/form-data-optional-request,fetch/form-data-with-custom-fetch,react-query/form-data,react-query/form-data-with-hook,swr/form-data-optional-request)—results from two independent code paths ingetSchemaFormDataAndUrlEncoded(packages/core/src/getters/res-req-types.ts):
- When
oneOforanyOfis present, theshouldCastbranch (line 576) emitsObject.entries(pet ?? {}).forEach(...), which appends every own enumerable property.- When
schema.propertiesexists, a subsequent branch (line 621) emits explicit appends for@id,callingCode,country.Because the Pet schema declares both
oneOf: [PetBase, PetExtended]and these four fields as direct properties, both code paths execute. The result is that each property is appended twice to the multipart body—once generically in the loop and once explicitly afterward.The fix requires modifying the
shouldCastbranch to skip keys that will be handled explicitly downstream. Suggested approach:
- Extract the set of keys from
schema.propertiesand skip them in theObject.entriesloop, or- Use conditional logic to emit only one code path when direct properties exist alongside
oneOf/anyOf.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/__snapshots__/vue-query/form-data/endpoints.ts` around lines 92 - 133, The Object.entries loop emitted in getSchemaFormDataAndUrlEncoded (the shouldCast branch) appends every pet property and then the later schema.properties branch appends the same keys again, causing duplicates; modify the shouldCast/Object.entries(pet ?? {}) emission to skip any keys present in schema.properties (extract property keys from schema.properties into a Set and filter them out before appending) or alternatively gate the entire shouldCast loop to not run when schema.properties exists so only one code-path (the explicit schema.properties appenders) emits those fields.
🧹 Nitpick comments (1)
tests/__snapshots__/react-query/form-data-with-hook/endpoints.ts (1)
17-25: Import grouping could be consolidated.
Error/Pet(L17) andPetBase/PetExtended(L24) come from the same./modelmodule; splitting them across two separateimport typestatements separated by unrelated imports is a codegen artifact worth cleaning up later. Not blocking.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/__snapshots__/react-query/form-data-with-hook/endpoints.ts` around lines 17 - 25, Consolidate the split type imports from the same module by merging the two separate "import type" statements for Error, Pet, PetBase, and PetExtended into a single import type from './model' (so replace the separate `import type { Error, Pet } from './model';` and `import type { PetBase, PetExtended } from './model';` with one `import type { Error, Pet, PetBase, PetExtended } from './model';`), removing the unrelated imports that currently sit between them so all model types are imported together and the remaining unrelated imports (faker, msw) remain unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@tests/__snapshots__/vue-query/form-data/endpoints.ts`:
- Around line 92-133: The Object.entries loop emitted in
getSchemaFormDataAndUrlEncoded (the shouldCast branch) appends every pet
property and then the later schema.properties branch appends the same keys
again, causing duplicates; modify the shouldCast/Object.entries(pet ?? {})
emission to skip any keys present in schema.properties (extract property keys
from schema.properties into a Set and filter them out before appending) or
alternatively gate the entire shouldCast loop to not run when schema.properties
exists so only one code-path (the explicit schema.properties appenders) emits
those fields.
---
Nitpick comments:
In `@tests/__snapshots__/react-query/form-data-with-hook/endpoints.ts`:
- Around line 17-25: Consolidate the split type imports from the same module by
merging the two separate "import type" statements for Error, Pet, PetBase, and
PetExtended into a single import type from './model' (so replace the separate
`import type { Error, Pet } from './model';` and `import type { PetBase,
PetExtended } from './model';` with one `import type { Error, Pet, PetBase,
PetExtended } from './model';`), removing the unrelated imports that currently
sit between them so all model types are imported together and the remaining
unrelated imports (faker, msw) remain unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1a45e030-834d-4f40-98e6-bbdd31b8bed2
📒 Files selected for processing (10)
packages/core/src/getters/res-req-types.test.tstests/__snapshots__/fetch/form-data-optional-request/endpoints.tstests/__snapshots__/fetch/form-data-with-custom-fetch/endpoints.tstests/__snapshots__/fetch/importFromSubdirectory/endpoints.tstests/__snapshots__/react-query/form-data-with-hook/endpoints.tstests/__snapshots__/react-query/form-data/endpoints.tstests/__snapshots__/react-query/importFromSubdirectory/endpoints.msw.tstests/__snapshots__/react-query/importFromSubdirectory/endpoints.tstests/__snapshots__/swr/form-data-optional-request/endpoints.tstests/__snapshots__/vue-query/form-data/endpoints.ts
✅ Files skipped from review due to trivial changes (3)
- tests/snapshots/react-query/importFromSubdirectory/endpoints.ts
- tests/snapshots/fetch/importFromSubdirectory/endpoints.ts
- packages/core/src/getters/res-req-types.test.ts
|
looks like build failure now? |
Two issues in the multipart oneOf/anyOf Object.entries codegen surfaced by the CodeRabbit review on orval-labs#3245 and the upstream "Verify generated code can build" CI job. - When the outer schema declares both `oneOf`/`anyOf` AND direct `properties`, both code paths ran: the runtime loop appended every key, then the properties branch re-appended each declared key. Shared keys (e.g. `@id`, `email`) were duplicated in the FormData body. The loop now skips keys that the properties branch will handle. - The previous Buffer branch handed the Buffer directly to `FormData.append`, which `@types/node` rejects against the DOM `FormData.append(name, Blob | string)` overload. The Buffer path now wraps the value in a `Blob` before appending and casts through `BlobPart` to bridge the `ArrayBufferLike`/`ArrayBuffer` generic mismatch that `@types/node` introduces. Add a regression test covering oneOf combined with direct properties, and refresh the six orval-tests file snapshots that exercise the codegen (fetch/form-data-optional-request, fetch/form-data-with-custom-fetch, react-query/form-data, react-query/form-data-with-hook, swr/form-data-optional-request, vue-query/form-data).
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
packages/core/src/getters/res-req-types.ts (2)
259-277: Name derivation looks correct; silent fallback for inline/mixed variants is a known limitation.Renaming the inner identifier to
refNameavoids shadowing the outernameparameter, and concatenating resolved names preserves the imported-type contract exercised atpackages/core/src/getters/res-req-types.test.tslines 667-669.As previously noted, if a
oneOf/anyOfcontains only inline object variants (no$refs),namesstays empty andeffectivePropNamefalls back to thepascal(name) + pascal(key)form that triggered the original bug. This is acceptable given the PR's explicit scope (NestJS/zod-to-openapi-style named DTOs), but a short inline comment here would help future readers understand the intended boundary.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/getters/res-req-types.ts` around lines 259 - 277, The loop over combinedRefs is shadowing the outer parameter name and can be clearer: rename the inner identifier to refName when extracting resolveSchemaRef(ref, context).imports[0]?.name and push refName into the names array, then join into effectivePropName as implemented; also add a short inline comment near this block (around getSchemaOneOf/getSchemaAnyOf handling and effectivePropName assignment) stating that if only inline variants exist (no $ref) names remains empty and the code intentionally falls back to the pascal(name)+pascal(key) form.
591-614: Runtime loop intentionally drops per-field codegen features — worth an inline note.For
oneOf/anyOfthe emitted loop usesString(value)/JSON.stringify(value)uniformly, which — per the PR description — loses:
- encoding
contentTypewrapping (new Blob([value], { type: 'text/plain' })etc.)- nullable-vs-null distinction (
nullis treated the same as "skip")- per-field array handling modes (
FormDataArrayHandling.EXPLODE/SERIALIZE_WITH_BRACKETS)- text-file detection (
getFormDataFieldFileType)Given this is a deliberate tradeoff, a short comment above the
form += 'Object.entries(...)'block documenting these losses (and pointing at the staticresolveSchemaPropertiesToFormDatapath used byallOf/direct properties for comparison) would save the next reader a spelunk through the PR history.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/getters/res-req-types.ts` around lines 591 - 614, Add an inline comment above the generated Object.entries loop (the block that builds form using variables propName and variableName) explaining that this runtime codegen intentionally simplifies oneOf/anyOf handling and therefore loses per-field behaviors: no contentType wrapping (e.g., new Blob([...], {type})), nullable vs null distinction, per-field array modes (FormDataArrayHandling.EXPLODE / SERIALIZE_WITH_BRACKETS), and text-file detection via getFormDataFieldFileType; also point readers to the static path resolveSchemaPropertiesToFormData (used for allOf/direct properties) for the full-featured implementation and why this tradeoff was made.packages/core/src/getters/res-req-types.test.ts (1)
551-905: Regression tests comprehensively cover both bugs and the documented edge cases.The block covers single-$ref naming (Bug 1), two-$refs shared-field loop (Bug 2), allOf non-regression, oneOf + direct-properties deduping, optional-body guard (
?? {}), and array-of-binary element handling — a good complement to the existinganyOf at roottest at lines 436-549.One small nit: the two-$refs test at lines 663-664 guards against reintroducing per-variant
uploadDtoV*.fileappends, but it doesn't assert which identifier the runtime loop iterates over (e.g., the concatenateduploadDtoV1UploadDtoV2vs a fallback name). Consider addingexpect(formData).toMatch(/Object\.entries\(uploadDtoV1UploadDtoV2\s*\?\?/)if you want to lock in the concatenation contract documented in the PR.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/getters/res-req-types.test.ts` around lines 551 - 905, Add a tightened assertion to the two-$refs test so the runtime loop identifier is locked to the concatenated name: after obtaining result.formData (from getResReqTypes) assert that the Object.entries call iterates over the concatenated variable (e.g., match /Object\.entries\(uploadDtoV1UploadDtoV2\s*\?\?/) and keep the existing checks that the loop exists and that UploadDtoV1/UploadDtoV2 remain imported; this ensures the test verifies the specific concatenation contract rather than only asserting absence of per-variant appends.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/core/src/getters/res-req-types.ts`:
- Around line 582-614: The loop unconditionally skips keys from directKeys
(built from getSchemaProperties(schema)) even when a direct property is readOnly
but a variant (oneOf/anyOf) makes it writable; change how directKeys is built by
filtering out properties whose direct schema has readOnly === true so only truly
direct writable properties are skipped—locate getSchemaProperties, directKeys,
propName and variableName in the form-building code and replace the current
Object.keys(directProperties) population with a filtered version that excludes
directProperties[key].readOnly.
---
Nitpick comments:
In `@packages/core/src/getters/res-req-types.test.ts`:
- Around line 551-905: Add a tightened assertion to the two-$refs test so the
runtime loop identifier is locked to the concatenated name: after obtaining
result.formData (from getResReqTypes) assert that the Object.entries call
iterates over the concatenated variable (e.g., match
/Object\.entries\(uploadDtoV1UploadDtoV2\s*\?\?/) and keep the existing checks
that the loop exists and that UploadDtoV1/UploadDtoV2 remain imported; this
ensures the test verifies the specific concatenation contract rather than only
asserting absence of per-variant appends.
In `@packages/core/src/getters/res-req-types.ts`:
- Around line 259-277: The loop over combinedRefs is shadowing the outer
parameter name and can be clearer: rename the inner identifier to refName when
extracting resolveSchemaRef(ref, context).imports[0]?.name and push refName into
the names array, then join into effectivePropName as implemented; also add a
short inline comment near this block (around getSchemaOneOf/getSchemaAnyOf
handling and effectivePropName assignment) stating that if only inline variants
exist (no $ref) names remains empty and the code intentionally falls back to the
pascal(name)+pascal(key) form.
- Around line 591-614: Add an inline comment above the generated Object.entries
loop (the block that builds form using variables propName and variableName)
explaining that this runtime codegen intentionally simplifies oneOf/anyOf
handling and therefore loses per-field behaviors: no contentType wrapping (e.g.,
new Blob([...], {type})), nullable vs null distinction, per-field array modes
(FormDataArrayHandling.EXPLODE / SERIALIZE_WITH_BRACKETS), and text-file
detection via getFormDataFieldFileType; also point readers to the static path
resolveSchemaPropertiesToFormData (used for allOf/direct properties) for the
full-featured implementation and why this tradeoff was made.
🪄 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
Run ID: 2ec08a91-b6d1-4702-a9bd-9fdc477e9384
📒 Files selected for processing (8)
packages/core/src/getters/res-req-types.test.tspackages/core/src/getters/res-req-types.tstests/__snapshots__/fetch/form-data-optional-request/endpoints.tstests/__snapshots__/fetch/form-data-with-custom-fetch/endpoints.tstests/__snapshots__/react-query/form-data-with-hook/endpoints.tstests/__snapshots__/react-query/form-data/endpoints.tstests/__snapshots__/swr/form-data-optional-request/endpoints.tstests/__snapshots__/vue-query/form-data/endpoints.ts
✅ Files skipped from review due to trivial changes (1)
- tests/snapshots/swr/form-data-optional-request/endpoints.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/snapshots/react-query/form-data-with-hook/endpoints.ts
- tests/snapshots/react-query/form-data/endpoints.ts
The previous Buffer wrapping emitted `new Blob([value as unknown as BlobPart])`. `BlobPart` only lives in `lib.dom.d.ts`, so a consumer that generates Orval clients in a pure-Node project without the DOM lib would see "Cannot find name 'BlobPart'" on the generated file. Replace the cast with `new Blob([Uint8Array.from(value)])`. `Uint8Array` is in the default ES lib, and its `.from()` overload returns `Uint8Array<ArrayBuffer>`, which satisfies the DOM/Node `BlobPart` signature without needing a cast. Runtime copies the Buffer bytes into a fresh Uint8Array once per value.
|
Thanks for the thorough review @coderabbitai. Addressed all feedback in the latest commits:
Local verification on the equivalent Node 22 env:
The one nitpick I did not act on is the import consolidation in the generated snapshots ( |
|
Tip For best results, initiate chat on the files or code changes.
The import consolidation deferral is fine — it's a pre-existing codegen ordering behavior and clearly out of scope here. A follow-up PR would be the right place for it. The local verification numbers (1643 unit + 3586 snapshot tests passing, all 14 build clients green) are reassuring given the breadth of snapshot changes across ✏️ Learnings added
🧠 Learnings used |
- Document the intentional fallback when a oneOf/anyOf union contains only inline variants (no $ref). The existing `pascal(name) + pascal(key)` path is reused for that shape; it's the original bug's naming, but this PR's scope is DTO-based unions (NestJS / zod-to-openapi). - Exclude readOnly direct properties from the skip list on the Object.entries loop. The previous filter dropped every key present in `schema.properties`, so a readOnly direct key that a oneOf variant also declared as writable would be skipped by both paths and never appended. Using the resolved schema's `readOnly` flag keeps the runtime loop as a fallback for that niche combination.
…$ref types (#3279) Closes #3269 Two distinct root causes regressed in 8.8.1 for the OpenAPI 3.1 `anyOf: [{$ref: X}, {type: 'null'}]` shape (the default emitted by FastAPI and Pydantic for `Optional[Model]`) when used with tags-split + msw mode: 1. `packages/core/src/getters/res-req-types.ts`: the oneOf/anyOf branch added in #3245 was firing for every media type. Outside form-data and url-encoded paths, the schema-derived `effectivePropName` collided with the schema import in split mode and produced a `Foo as __Foo` alias that did not need to exist. Scoped the branch to `(isFormData || isFormUrlEncoded)`. Both share the downstream `getSchemaFormDataAndUrlEncoded` path and both need the schema name to align with the runtime variable iterated over by the generated code. Other content types only seed type aliases. 2. `packages/mock/src/msw/index.ts`: the import filter searched `\b${name}\b` against the generated mock code. When an import is aliased, the code references the alias (`__Foo`), but the filter looked for the bare name. Since `_` is a JS regex word-character, `\bFoo\b` does not match `__Foo`, so the aliased import was filtered out and the mock file referenced an undeclared identifier (TS2304). Switched the filter to match alias OR name with `escapeRegExp`, mirroring the `addDependency` pattern in `core/src/generators/imports.ts`. Tests: - 3 new unit tests in `mock/src/msw/index.test.ts` covering the alias filter (alias-only reference, dropped unused alias with positive control, dual entry with both bare and aliased forms). - New regression fixture `tests/specifications/issue-3269.yaml` plus a `issue3269` config entry exercising response and request body anyOf-nullable in tags-split + msw mode end-to-end. - Three `importFromSubdirectory` snapshots refreshed; the aliased imports they previously contained existed solely because of #3245's overreach in non-form-data paths and are no longer emitted. Existing #3245 multipart tests remain green; the `Object.entries(... ?? {})` runtime loop and DTO-derived FormData variable name are intact.
3 participants
Fixes #3242
Two independent bugs in
getResReqTypesandgetSchemaFormDataAndUrlEncodedwhen amultipart/form-datarequest body usesoneOforanyOf. Full repro, compiler errors, and expected output in the issue.Solution
getResReqTypeswith anelse ifthat walksoneOf/anyOf, resolves the$ref(s) inside, and concatenates the resolved names.oneOf/anyOf(shouldCast === true), replace the N per-variantformData.appendbranches with a singleObject.entries()loop.allOfis unchanged.Known limitations (Bug 2)
The runtime approach drops some static-codegen features from the per-variant branches:
readOnlyfiltering,encodingmap, array handling modes,getFormDataFieldFileTypebinary/text detection, nullable handling. Only complex specs are affected.Testing
3 new tests in
res-req-types.test.tscovering: oneOf with 1$ref(Bug 1), oneOf with 2$refssharing a field (Bug 2), allOf regression.bun run test1640 passing, lint + typecheck green. Tests were also run against the pre-fix source to confirm they fail as expected.Same fix is also deployed as a
pnpm patch-packageon 8 TypeScript projects (NestJS +zod-to-openapi, 1-2 schema versions) in production - all compile and run correctly.Summary by CodeRabbit
Tests
Bug Fixes