fix: union when there are multiple request/response types defined #1462
Conversation
Contributes to acacode#314 Signed-off-by: Stefan Cooper <stefan.cooper27@gmail.com>
🦋 Changeset detectedLatest commit: 25b2de2 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Signed-off-by: Stefan Cooper <stefan.cooper27@gmail.com>
Signed-off-by: Stefan Cooper <stefan.cooper27@gmail.com>
Signed-off-by: Stefan Cooper <stefan.cooper27@gmail.com>
stefan-cooper
force-pushed
the
314/fix
branch
from
2366109 to
25b2de2
Compare
|
@smorimoto I've had a stab at trying to fix this myself. LMK what you think! |
|
@codex review |
|
Codex Review: Didn't find any major issues. 🚀 ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
bugbot run |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull Request Overview
This PR adds support for generating union types when multiple media types are defined in OpenAPI request or response bodies. Previously, only the first media type was used; now all distinct schemas are combined into a union type.
Key Changes:
- Enhanced
getSchemaFromRequestTypeto detect multiple media types and create oneOf schemas for union type generation - Added logic in
getSchemaFromRequestTypeto check if schemas are structurally identical before creating unions - Updated request body type generation to handle oneOf schemas by creating TypeScript union types
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/schema-routes/schema-routes.ts | Core logic to detect multiple media types and generate union types via oneOf schemas |
| tests/spec/multiple-media-types/schema.json | Test OpenAPI schema with Cat and Dog schemas under different media types |
| tests/spec/multiple-media-types/index.spec.ts | Test case verifying union type generation for multiple media types |
| tests/spec/multiple-media-types/snapshots/index.spec.ts.snap | Snapshot showing generated TypeScript with Cat | Dog union type |
| tests/snapshots/simple.test.ts.snap | Updated snapshots reflecting union type changes in existing tests |
| tests/snapshots/extended.test.ts.snap | Updated snapshots reflecting union type changes in existing tests |
| .changeset/shaggy-otters-walk.md | Changeset documenting the new feature as a minor version bump |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| const contentTypes = Object.keys(content); | ||
|
|
||
| // if there's only one content type, return it | ||
| if (contentTypes.length === 1 && content[contentTypes[0]]?.schema) { | ||
| return { | ||
| ...content[contentTypes[0]].schema, | ||
| dataType: contentTypes[0], | ||
| }; | ||
| } | ||
|
|
||
| // Check if there are multiple media types with schemas | ||
| const schemasWithDataTypes = []; | ||
| for (const dataType in content) { | ||
| if (content[dataType]?.schema) { | ||
| return { | ||
| schemasWithDataTypes.push({ | ||
| ...content[dataType].schema, | ||
| dataType, | ||
| }; | ||
| }); | ||
| } | ||
| } | ||
|
|
||
| // If there's only one schema, return it directly | ||
| if (schemasWithDataTypes.length === 1) { | ||
| return schemasWithDataTypes[0]; | ||
| } |
There was a problem hiding this comment.
The logic for handling single content types is duplicated: lines 337-342 handle the case where contentTypes.length === 1, and lines 356-358 handle the case where schemasWithDataTypes.length === 1. These blocks will always produce the same result when there's only one schema. The first check (lines 337-342) should be removed since the loop and subsequent check (lines 346-358) already handle this case correctly.
| const allSchemasAreSame = schemasWithDataTypes.every((schema) => | ||
| lodash.isEqual( | ||
| lodash.omit(schema, "dataType"), | ||
| lodash.omit(firstSchema, "dataType"), |
There was a problem hiding this comment.
The comparison lodash.omit(schema, 'dataType') is executed repeatedly for every schema in the array, including for firstSchema which is omitted once per iteration. Consider computing lodash.omit(firstSchema, 'dataType') once before the every() call to avoid redundant operations.
| const allSchemasAreSame = schemasWithDataTypes.every((schema) => | |
| lodash.isEqual( | |
| lodash.omit(schema, "dataType"), | |
| lodash.omit(firstSchema, "dataType"), | |
| const firstSchemaOmitted = lodash.omit(firstSchema, "dataType"); | |
| const allSchemasAreSame = schemasWithDataTypes.every((schema) => | |
| lodash.isEqual( | |
| lodash.omit(schema, "dataType"), | |
| firstSchemaOmitted, |
2 participants
Contributes to #314
Signed-off-by: Stefan Cooper stefan.cooper27@gmail.com