fix(typescript): reject unresolved query body collisions - #17641
fix(typescript): reject unresolved query body collisions#17641dimitropoulos wants to merge 1 commit into
Conversation
![nitpickybot[bot]](https://avatars.githubusercontent.com/in/2901896?s=60&v=4){kind=small}
There was a problem hiding this comment.
AI Review Summary
The change turns previously-silent query/body name collisions into hard generation errors and broadens the body-property-name computation. Main concerns: a behavioral regression in the extends handling (nullable type declaration now dereferenced unguarded, and a new throw for non-object extends), redundant recomputation of body property names, and a misleading error message when resolveQueryParameterNameConflicts is already enabled.
- 🔴 1 critical issue(s)
- 🟡 3 warning(s)
- 🔵 2 suggestion(s)
AI Review
🔵 suggestion — generators/typescript/sdk/request-wrapper-generator/src/GeneratedRequestWrapperImpl.ts (line 1191)
The comment says "Validate that ..." but the variable is still named collidingQueryParamWireValues and the method name (getResolvableQueryParamWireValues) reads as a getter with no hint that it throws. Consider assertQueryParamCollisionsAreResolvable returning the set, or at least a doc comment stating it throws.
To request another review, comment /ai-review on this pull request.
| for (let extension of inlinedRequestBody.extends) { | ||
| const typeDeclaration = context.type.getTypeDeclaration(extension); | ||
| if (typeDeclaration?.shape.type === "object") { | ||
| for (const property of typeDeclaration.shape.properties) { | ||
| const propName = this.getPropertyNameOfTypeDeclarationProperty(property); | ||
| bodyPropertyNames.add(propName.propertyName); | ||
| } | ||
| if (typeDeclaration.shape.type === "alias" && typeDeclaration.shape.aliasOf.type === "named") { | ||
| extension = typeDeclaration.shape.aliasOf; | ||
| } | ||
| const generatedType = context.type.getGeneratedType(extension); | ||
| if (generatedType.type !== "object") { | ||
| throw new Error("Inlined request extends a non-object"); |
There was a problem hiding this comment.
🔴 critical
Two regressions here:
context.type.getTypeDeclaration(extension)was previously null-checked (typeDeclaration?.shape.type === "object"); nowtypeDeclaration.shapeis dereferenced unguarded. If the declaration is missing this throws a TypeError instead of skipping.- The old code silently skipped non-object extends; now it throws
"Inlined request extends a non-object". This path is reached from collision detection for every endpoint with an inlined body that extends something, so any IR the old code tolerated (e.g. extends an undiscriminated union alias) will now hard-fail generation. If that invariant really holds elsewhere, at least include the extension name in the error for debuggability.
| private getResolvableQueryParamWireValues(context: FileContext): Set<string> { | ||
| const collidingWireValues = this.getCollidingQueryParamWireValues(context); | ||
| const bodyPropertyNames = this.getBodyPropertyNames(context, { includeAllRequestBodyShapes: true }); | ||
| for (const queryParameter of this.getAllQueryParameters()) { | ||
| if (!collidingWireValues.has(getWireValue(queryParameter.name))) { | ||
| continue; | ||
| } | ||
| const propertyName = this.getPropertyNameOfQueryParameter(queryParameter).propertyName; | ||
| const overriddenPropertyName = this.getOverriddenPropertyNameOfQueryParameter(queryParameter).propertyName; | ||
| if ( | ||
| !this.resolveQueryParameterNameConflicts || | ||
| overriddenPropertyName === propertyName || | ||
| bodyPropertyNames.has(overriddenPropertyName) | ||
| ) { | ||
| throw new Error( | ||
| `Cannot generate ${this.wrapperName}: query parameter "${propertyName}" conflicts with a request body property. Set a distinct x-fern-parameter-name and enable resolveQueryParameterNameConflicts.` | ||
| ); | ||
| } | ||
| } | ||
| return collidingWireValues; | ||
| } |
There was a problem hiding this comment.
🟡 warning
getResolvableQueryParamWireValues recomputes getBodyPropertyNames(..., includeAllRequestBodyShapes: true) even though getCollidingQueryParamWireValues just built the same set internally — and this whole function is called three times per wrapper (getRequestProperties, getNonBodyKeys, getNonBodyKeysWithData). That's 6 full traversals of the body (each doing getGeneratedType / getAllPropertiesIncludingExtensions) per wrapper. Consider having getCollidingQueryParamWireValues return both sets, and memoizing the result per context.
| bodyPropertyNames.has(overriddenPropertyName) | ||
| ) { | ||
| throw new Error( | ||
| `Cannot generate ${this.wrapperName}: query parameter "${propertyName}" conflicts with a request body property. Set a distinct x-fern-parameter-name and enable resolveQueryParameterNameConflicts.` |
There was a problem hiding this comment.
🟡 warning
The error message tells the user to "Set a distinct x-fern-parameter-name and enable resolveQueryParameterNameConflicts" even when resolveQueryParameterNameConflicts is already true and the override itself collides (see the test at line 374). Branch the message on which condition failed so users aren't told to enable a flag they already enabled, and mention the colliding override name in that case.
| return; | ||
| } | ||
| } | ||
| bodyPropertyNames.add(this.getReferencedBodyPropertyName()); |
There was a problem hiding this comment.
🟡 warning
bodyPropertyNames.add(this.getReferencedBodyPropertyName()) now runs for every non-flattened reference body, meaning any query param named body (or whatever the referenced-body key is) now hard-fails generation where it previously generated fine. That's a real breaking change for existing users, not just invalid-TS rejection. Worth confirming this is intended and calling it out in the changelog entry.
| bodyPropertyNames.add(propKey.propertyName); | ||
| } | ||
| for (const extension of inlinedRequestBody.extends) { | ||
| for (let extension of inlinedRequestBody.extends) { |
There was a problem hiding this comment.
🔵 suggestion
let extension reassigned inside the loop is a bit sneaky. A local (const resolved = ...) reads better and avoids accidental reuse of the mutated loop variable later.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
There was a problem hiding this comment.
🟡 Changes recommended
The new thrown error message gives misleading guidance when conflict resolution is already enabled, which can confuse users and hinder debugging.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR hardens the TypeScript request-wrapper generator by detecting query-parameter vs request-body property name collisions early and failing fast instead of emitting invalid TypeScript, with additional coverage for referenced and multipart/file-upload bodies.
Changes:
- Introduces unconditional collision validation via
getResolvableQueryParamWireValues(...), requiring a distinctx-fern-parameter-namewhen conflict resolution is enabled. - Expands body-property name collection to cover additional request body shapes (referenced bodies and file upload properties) to properly detect collisions.
- Adds unit tests covering unresolved collisions, valid resolutions, and edge cases (literal body properties, referenced bodies, inline file properties), plus an unreleased changelog entry.
File summaries
| File | Description |
|---|---|
| generators/typescript/sdk/request-wrapper-generator/src/GeneratedRequestWrapperImpl.ts | Adds collision validation and broadens body property name detection across request body shapes. |
| generators/typescript/sdk/request-wrapper-generator/src/test/GeneratedRequestWrapperImpl.test.ts | Adds tests for collision rejection/resolution scenarios (inline, referenced, multipart/file upload). |
| generators/typescript/sdk/changes/unreleased/reject-unresolved-query-body-collisions.yml | Records the generator fix in the changelog. |
Review details
Suppressed comments (2)
generators/typescript/sdk/request-wrapper-generator/src/GeneratedRequestWrapperImpl.ts:1086
- The thrown error message always says to "enable resolveQueryParameterNameConflicts", even in cases where it is already enabled and the real problem is that the x-fern-parameter-name override is not distinct (or it collides with another body property). This can mislead users debugging generation failures.
throw new Error(
`Cannot generate ${this.wrapperName}: query parameter "${propertyName}" conflicts with a request body property. Set a distinct x-fern-parameter-name and enable resolveQueryParameterNameConflicts.`
);
generators/typescript/sdk/request-wrapper-generator/src/GeneratedRequestWrapperImpl.ts:1095
- This comment still says "(inlined) request body", but getBodyPropertyNames can include referenced and file-upload request bodies when includeAllRequestBodyShapes is true. Updating the comment will make the collision logic easier to reason about.
/**
* Computes the set of property names produced by the (inlined) request body. Used to detect
* collisions between non-body parameters (path/query) and body property names.
*/
- Files reviewed: 3/3 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| /** | ||
| * When resolveQueryParameterNameConflicts is enabled, computes the set of query parameter | ||
| * wire values that collide with body property names. Only these colliding query params | ||
| * will use their SDK override names instead of wire values. | ||
| */ |
Description
Fern generates invalid TypeScript when the request has a body and query parameter that are identical.
Reject query/body property collisions that would generate invalid TypeScript. Reproduction: https://github.com/dimitropoulos/scratchpad/tree/fern-query-body-parameter-collision (failing CI).
Changes Made
x-fern-parameter-namewithresolveQueryParameterNameConflicts.Testing
pnpm turbo run compile --filter @fern-typescript/request-wrapper-generatorpnpm turbo run test --filter @fern-typescript/request-wrapper-generator