fix(core): extract named enum type for anyOf nullable enum composition - #3424
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThis PR detects OpenAPI 3.1 nullable-enum compositions (inline enum + ChangesNullable Enum Composition Detection
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
Suggested reviewers
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 |
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Adds support for OpenAPI 3.1 “nullable enum” compositions (anyOf/oneOf with an enum branch plus { type: 'null' }) so enums are extracted as named types (including const enum generation), matching the existing type: ['...', 'null'] enum behavior.
Changes:
- Update
combineSchemasto treat{ type: 'null' }union members as transparent when determining “enum-ness” for nullable enum compositions. - Add integration tests for query params to ensure nullable enum extraction and correct
constenum output. - Add unit tests for
combineSchemasnullable-enum detection and non-matching negative cases.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| packages/core/src/getters/query-params.test.ts | Adds integration coverage for anyOf: [enum, null] query params (including const enum mode). |
| packages/core/src/getters/combine.ts | Flags nullable enum compositions as isEnum so callers can extract a named enum type. |
| packages/core/src/getters/combine.test.ts | Adds focused unit tests for nullable enum composition detection and negative cases. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| !isAllEnums && | ||
| resolvedData.isEnum.some(Boolean) && | ||
| resolvedData.isEnum.every( | ||
| (isEnum, index) => isEnum || resolvedData.types[index] === 'null', |
There was a problem hiding this comment.
Both concerns addressed in 399db48. Verified the anyOf: [{$ref: '.../Status'}, {type: 'null'}] repro: before the fix it emitted a nested ExampleStatus = { Status: Status } const; now it correctly emits status?: Status | null reusing the referenced type. Added (separator === 'anyOf' || separator === 'oneOf') and !resolvedData.isRef[index] guards to isNullableEnumComposition, plus negative unit tests for the $ref + null and allOf [enum, null] shapes.
| ? [...resolvedData.dependencies, ...resolvedValue.dependencies] | ||
| : resolvedData.dependencies, | ||
| isEnum: false, | ||
| isEnum: isNullableEnumComposition, |
There was a problem hiding this comment.
Same fix as the L359 thread — addressed in 399db48. The detection now requires isUnionLikeSeparator (anyOf/oneOf only) and inline enum branches (!resolvedData.isRef[index]).
| describe('nullable enum composition (#2710)', () => { | ||
| it('flags anyOf [enum, null] as a nullable enum', () => { | ||
| const schema: OpenApiSchemaObject = { | ||
| anyOf: [{ enum: ['new', 'in_progress'] }, { type: 'null' }], | ||
| }; |
There was a problem hiding this comment.
Added in 399db48. Two new tests in combine.test.ts: does not flag anyOf [$ref enum, null] as a nullable enum (asserts isEnum: false), and an integration test in query-params.test.ts (queryParam with anyOf [$ref enum, null] reuses the referenced type) asserting the param emits status?: Status | null and no parameter-scoped enum is added to deps.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
packages/core/src/getters/query-params.test.ts (2)
201-305: ⚡ Quick winConsider adding oneOf test coverage for completeness.
The PR description mentions support for both
anyOfandoneOfnullable enum compositions, but the new tests only coveranyOf. While the behavior should be identical, adding a parallel test case foroneOfwould provide explicit integration-level coverage and increase confidence that both composition keywords are handled consistently.Example oneOf test case
it('queryParam with oneOf containing enum and null extracts a named nullable enum type', () => { const result = getQueryParams({ queryParams: [ { parameter: { name: 'priority', in: 'query', required: false, schema: { oneOf: [{ enum: ['low', 'high'] }, { type: 'null' }], title: 'Priority', }, }, imports: [], }, ], operationName: '', context, }); expect(result?.schema.model.trim()).toBe( `export type Params = {\npriority?: Priority;\n};`, ); const priorityEnum = result?.deps.find((schema) => schema.name === 'Priority'); expect(priorityEnum).toBeDefined(); expect(priorityEnum?.model).toContain(`'low' | 'high' | null`); });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/src/getters/query-params.test.ts` around lines 201 - 305, Add parallel oneOf-based tests mirroring the anyOf cases to ensure oneOf nullable-enum handling is covered: create a test that calls getQueryParams with a queryParam whose schema uses oneOf: [{ enum: [...] }, { type: 'null' }] (similar to the anyOf 'Status' test) and assert the Params model references the named type and that the dependency (e.g., find by name 'Priority' or 'Status') contains the enum union with | null; also add the const-mode variant using createTestContextSpec with EnumGeneration.CONST to assert the typeof+const pattern appears and that 'null: null' does not leak, and finally add a negative test where oneOf contains enum plus a non-null scalar to confirm it stays inlined (no named type emitted).
238-276: 💤 Low valueWell-designed test for const enum mode with nullable composition.
The defensive assertion at line 275 correctly prevents invalid
null: nullmembers in the const body. The test validates the typeof+const pattern and null handling.Optional: Consider adding explicit const body verification
For completeness, you could add an assertion that the const body contains the expected enum values:
expect(statusEnum?.model).toContain(`export const Status = {`); +expect(statusEnum?.model).toContain(`new:`); +expect(statusEnum?.model).toContain(`in_progress:`); // The null variant must not leak into the const body as a `null: null` // member — that would emit invalid TypeScript. expect(statusEnum?.model).not.toContain('null: null');This would make the test more thorough, though the current assertions are sufficient for integration-level testing.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/src/getters/query-params.test.ts` around lines 238 - 276, Add an explicit assertion to this test that the generated const body for the Status enum contains the expected enum members so we verify the actual const entries (e.g., that statusEnum.model includes the literal const mappings for "new" and "in_progress"); locate the statusEnum variable (result?.deps.find((schema) => schema.name === 'Status')) and add checks on statusEnum.model toContain the expected const entries (for example the string fragments representing the const object members) in addition to the existing null and typeof assertions.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/core/src/getters/combine.ts`:
- Around line 347-360: The isNullableEnumComposition check should only apply to
union-like compositions, so add a guard that requires (separator === 'anyOf' ||
separator === 'oneOf') before the existing !isAllEnums && resolvedData.isEnum...
predicate; update the isNullableEnumComposition declaration (the variable named
isNullableEnumComposition) to include this separator check alongside isAllEnums
and the resolvedData.isEnum conditions so that allOf compositions are not
misclassified as nullable enums.
---
Nitpick comments:
In `@packages/core/src/getters/query-params.test.ts`:
- Around line 201-305: Add parallel oneOf-based tests mirroring the anyOf cases
to ensure oneOf nullable-enum handling is covered: create a test that calls
getQueryParams with a queryParam whose schema uses oneOf: [{ enum: [...] }, {
type: 'null' }] (similar to the anyOf 'Status' test) and assert the Params model
references the named type and that the dependency (e.g., find by name 'Priority'
or 'Status') contains the enum union with | null; also add the const-mode
variant using createTestContextSpec with EnumGeneration.CONST to assert the
typeof+const pattern appears and that 'null: null' does not leak, and finally
add a negative test where oneOf contains enum plus a non-null scalar to confirm
it stays inlined (no named type emitted).
- Around line 238-276: Add an explicit assertion to this test that the generated
const body for the Status enum contains the expected enum members so we verify
the actual const entries (e.g., that statusEnum.model includes the literal const
mappings for "new" and "in_progress"); locate the statusEnum variable
(result?.deps.find((schema) => schema.name === 'Status')) and add checks on
statusEnum.model toContain the expected const entries (for example the string
fragments representing the const object members) in addition to the existing
null and typeof assertions.
🪄 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: a1f8f80e-a65f-4496-8fa9-63e7b594e1c5
📒 Files selected for processing (3)
packages/core/src/getters/combine.test.tspackages/core/src/getters/combine.tspackages/core/src/getters/query-params.test.ts
Addresses review feedback on orval-labs#3424: The initial detection toggled `isEnum: true` whenever the variants were "enum-or-null", but missed two cases that change the caller's behavior: - `allOf` is an intersection, not a union. `allOf: [{enum}, {null}]` is semantically empty and must not be misclassified. - When the enum branch is a `$ref` (e.g. `anyOf: [{$ref: '.../Status'}, {type: 'null'}]`) the existing referenced enum should be reused. Routing through `getEnum` re-extracted the ref as a parallel inline enum, producing a nested `ExampleStatus = { Status: Status }` const instead of `status?: Status | null`. Add two guards: (1) `separator === 'anyOf' || separator === 'oneOf'` and (2) `!resolvedData.isRef[index]` for non-null branches. New negative tests pin both shapes. Also add an `oneOf` integration test and explicit const-body member assertions per CodeRabbit's nitpicks.
OpenAPI 3.1 allows `anyOf: [{enum: [...]}, {type: 'null'}]` as a
nullable enum, but `combineSchemas` flagged the result as `isEnum: false`
because the `{type: 'null'}` branch broke the strict `isEnum.every()`
check. Downstream call sites (query-params, schema-definition,
resolvers/object) therefore inlined the union instead of extracting a
named type — diverging from the equivalent
`{type: ['string','null'], enum: [...]}` spelling, which already works.
Recognize the "every variant is enum-or-null, at least one enum" pattern
and propagate `isEnum: true` so the existing extraction paths produce a
named type. `getEnum`'s `stripNullUnion` handling already preserves the
trailing ` | null` for UNION and CONST modes.
Fixes orval-labs#2710
Addresses review feedback on orval-labs#3424: The initial detection toggled `isEnum: true` whenever the variants were "enum-or-null", but missed two cases that change the caller's behavior: - `allOf` is an intersection, not a union. `allOf: [{enum}, {null}]` is semantically empty and must not be misclassified. - When the enum branch is a `$ref` (e.g. `anyOf: [{$ref: '.../Status'}, {type: 'null'}]`) the existing referenced enum should be reused. Routing through `getEnum` re-extracted the ref as a parallel inline enum, producing a nested `ExampleStatus = { Status: Status }` const instead of `status?: Status | null`. Add two guards: (1) `separator === 'anyOf' || separator === 'oneOf'` and (2) `!resolvedData.isRef[index]` for non-null branches. New negative tests pin both shapes. Also add an `oneOf` integration test and explicit const-body member assertions per CodeRabbit's nitpicks.
399db48 to
0c5e372
Compare
Fixes #2710.
OpenAPI 3.1 lets users spell a nullable enum either as
{type: ['string','null'], enum: [...]}(which orval already extracts as a named type) or asanyOf: [{enum: [...]}, {type: 'null'}](FastAPI emits this form). The second spelling silently fell back to an inline union for query parameters, request body properties, and component schemas:Root cause
combineSchemasdecided whether to treat the result as an enum via:The
{type: 'null'}branch resolves toisEnum: false, so the strictevery()collapsed tofalse. The fall-through return path hard-codedisEnum: false, which made downstream callers (query-params.ts,schema-definition.ts,resolvers/object.ts) skip theirisEnum && !isRefextraction branch and inline the union instead.Fix
Detect the nullable enum composition pattern — every variant is either an enum or a
{type: 'null'}branch, with at least one enum — and propagateisEnum: trueon that single return path. No new code path needed:getEnum's existingstripNullUnionhandling ingetTypeConstEnumalready moves the trailing| nulloff the const body and onto the type alias, so UNION and CONST modes both produce correct output.The detection is intentionally narrow:
anyOf: [{enum}, {string}](genuine union of enum and non-null scalar) → unchanged, stays inlinedanyOf: [{string}, {null}](nullable non-enum) → unchanged, stays inlinedanyOf: [{enum}, {enum}, {null}](multi-enum + null) → now extracted as a nullable enum (pinned by a test)Scope of behavior change
getEnumresolvers/objectexport type X = 'a'|'b'|nulltypeof+constpattern (now consistent with other nullable enums)All 4133 snapshot tests in
tests/produce byte-identical output → no existing fixture is affected.Tests
10 tests, all OAS 3.1, covering positive and negative cases:
combine.test.ts(unit,combineSchemasdirectly) — underdescribe('nullable enum composition (#2710)'):isEnum: trueisEnum: trueisEnum: trueisEnum: trueisEnum: trueisEnum: falseisEnum: falsequery-params.test.ts(integration):status?: Status;+ named type with trailing| nullenumGenerationType: 'const'→ emitstypeof+constpattern, with explicitnot.toContain('null: null')guardVerification
@orval/core: 1797 unit tests pass (1789 → 1797, +8)packages/orval: 91 vitest tests passtests/: 4133 snapshot tests pass (byte-identical to master)bun run lint,bun run typecheck: passExampleStatusas expected under both default (CONST) andenumGenerationType: 'union'modesOut of scope
enumGenerationType: 'enum'(native TS enum) handling of nullable enums — pre-existing bug that emits an invalidnull = nullmember, reproducible with{type: ['string','null'], enum: [..., null]}on master without this PR.nullable: truesiblings insideanyOf— the@scalar/openapi-parserupgrader does not convert those to{type: 'null'}, so they remain inlined. Separate concern.Summary by CodeRabbit
Bug Fixes
Tests