fix(core): break circular type alias for allOf-inheriting discriminator variants - #3435
Merged
melloware merged 3 commits intoMay 25, 2026
Conversation
Contributor
📝 WalkthroughWalkthroughRewrites variant allOf entries that reference a discriminator parent with top-level oneOf by inlining the parent's non-discriminator properties into each variant; adds tests, updates generated snapshots and mocks, and includes the fixture in typechecking to validate the TS2456 circular-alias fix. ChangesCircular type alias resolution for discriminated oneOf unions
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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 |
Contributor
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
This PR addresses a TypeScript circular type-alias issue caused by discriminator parents that define oneOf variants which themselves inherit from the parent via allOf, and updates related mock snapshots.
Changes:
- Adds schema-level rewriting in
resolveDiscriminatorsto remove/inline a parent$refwithin variantallOfto break TS2456 cycles. - Updates mock generator snapshots for
discriminator-oneof-allofto reflect the new, non-circular variant shapes. - Removes the typecheck exclusion for the
discriminator-oneof-allofmock fixture.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/scripts/typecheck-generated.mjs | Removes mock fixture exclusion so it is now included in typechecking. |
| tests/snapshots/mock/discriminator-oneof-allof/model/item1.ts | Updates snapshot to no longer depend on Omit<DiscriminatorTest, 'type'>. |
| tests/snapshots/mock/discriminator-oneof-allof/model/item2.ts | Updates snapshot to no longer depend on Omit<DiscriminatorTest, 'type'>. |
| tests/snapshots/mock/discriminator-oneof-allof/model/item3.ts | Updates snapshot to no longer depend on Omit<DiscriminatorTest, 'type'>. |
| packages/core/src/getters/discriminators.ts | Implements the parent-$ref rewrite/inline logic for variants to break circular type aliases. |
| packages/core/src/getters/discriminators.test.ts | Adds regression tests covering rewrite/inline behavior and a guard case. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+185
to
+193
| if (hasInheritableProps) { | ||
| rewritten.push({ | ||
| type: 'object', | ||
| properties: inheritableProps, | ||
| ...(inheritableRequired && inheritableRequired.length > 0 | ||
| ? { required: inheritableRequired } | ||
| : {}), | ||
| } as OpenApiSchemaObject); | ||
| } |
Comment on lines
+129
to
+139
| const inheritableProps: Record< | ||
| string, | ||
| OpenApiSchemaObject | OpenApiReferenceObject | ||
| > = {}; | ||
| if (parentProperties) { | ||
| for (const [key, value] of Object.entries(parentProperties)) { | ||
| if (key !== propertyName) { | ||
| inheritableProps[key] = value; | ||
| } | ||
| } | ||
| } |
Comment on lines
+381
to
+384
| it('inlines parent non-discriminator properties into variant allOf (#3432)', () => { | ||
| // When the parent has additional properties beyond the discriminator key, | ||
| // those properties must survive on each variant. Replace the $ref with an | ||
| // inline object carrying parent's properties minus the discriminator key. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/core/src/getters/discriminators.test.ts (1)
381-437: ⚡ Quick winAdd a regression case for non-property parent constraints during rewrite.
Current
#3432tests validate ref removal and property inlining, but not whether parent-level constraints (likeadditionalPropertiesor required-only contributions) survive when the parent$refis replaced. A focused case here would lock that behavior.🤖 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/discriminators.test.ts` around lines 381 - 437, The test needs a regression case ensuring parent-level constraints survive when the parent $ref is inlined by resolveDiscriminators: update the Parent schema in discriminators.test.ts to include a parent-level constraint (e.g., additionalProperties: false and/or other non-property constraints) and assert after calling resolveDiscriminators that the inlined object (inspect variantA.allOf[0] as done currently) preserves those constraints (expect(inlined).toHaveProperty('additionalProperties', false) and that required remains exactly the parent's required list); ensure you reference the existing resolveDiscriminators call and the variantA/allOf inspection logic so the new assertions are added to the same test case.
🤖 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/discriminators.ts`:
- Around line 125-143: When replacing a parent $ref inside variant `allOf` the
code currently only copies filtered `properties`/`required` (variables
parentProperties, parentRequired, inheritableProps, inheritableRequired,
hasInheritableProps) which drops other parent constraints; update the merge so
that after removing the parent `$ref` you shallow-merge all remaining keys from
`parentSchema` (e.g., additionalProperties, all composition/object constraints,
patternProperties, min/max*, etc.) into the variant schema except for the
removed property-specific entries — preserve every parent constraint not
explicitly overridden by the variant rather than only copying `properties` and
`required`.
---
Nitpick comments:
In `@packages/core/src/getters/discriminators.test.ts`:
- Around line 381-437: The test needs a regression case ensuring parent-level
constraints survive when the parent $ref is inlined by resolveDiscriminators:
update the Parent schema in discriminators.test.ts to include a parent-level
constraint (e.g., additionalProperties: false and/or other non-property
constraints) and assert after calling resolveDiscriminators that the inlined
object (inspect variantA.allOf[0] as done currently) preserves those constraints
(expect(inlined).toHaveProperty('additionalProperties', false) and that required
remains exactly the parent's required list); ensure you reference the existing
resolveDiscriminators call and the variantA/allOf inspection logic so the new
assertions are added to the same test case.
🪄 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: d3d47ddc-6d0b-4930-a416-de340be491d2
📒 Files selected for processing (6)
packages/core/src/getters/discriminators.test.tspackages/core/src/getters/discriminators.tstests/__snapshots__/mock/discriminator-oneof-allof/model/item1.tstests/__snapshots__/mock/discriminator-oneof-allof/model/item2.tstests/__snapshots__/mock/discriminator-oneof-allof/model/item3.tstests/scripts/typecheck-generated.mjs
💤 Files with no reviewable changes (1)
- tests/scripts/typecheck-generated.mjs
wadakatu
marked this pull request as draft
May 25, 2026 01:24
wadakatu
marked this pull request as ready for review
May 25, 2026 02:55
…or variants
When a discriminator parent has top-level `oneOf` listing variants that inherit
via `allOf: [{ $ref: <parent> }, ...]`, the type generator emitted
`type ParentN = (Variant1 & {...}) | ...` together with
`type Variant1 = Omit<Parent, key> & {...}`, producing TS2456: Type alias
'Variant1' circularly references itself.
Rewrite each variant's `$ref` back to the parent inside `resolveDiscriminators`:
inline the parent's non-discriminator properties (or drop the entry entirely
when the parent contributes nothing beyond the discriminator key). The variant
no longer depends on the parent's alias, breaking the cycle.
Other discriminator+oneOf+allOf fixtures (`recursive-discriminator-allof`,
`lowercase-discriminator`, `one-of-nested`, `polymorphic`) are unaffected:
either the parent has no top-level `oneOf`, the variants don't reference the
parent in their `allOf`, or the variants aren't mapping targets.
Closes orval-labs#3432
…props branch The companion `discriminator-oneof-allof` fixture exercises the empty-parent drop branch of the fix (parent carries only the discriminator key, so the $ref-to-parent is dropped entirely from each variant). This fixture pins the inline branch instead: `Animal` has both the discriminator key (`species`) and a common property (`name`) that must be inherited by each variant. Without the fix, `Cat` and `Dog` would emit as `Omit<Animal, 'species'> & ...` and circularly depend on `Animal`'s alias union. With the fix, `name` is inlined into each variant, breaking the cycle while preserving the property. Refs orval-labs#3432
…rval-labs#3432) Address review feedback on the orval-labs#3432 inline-parent-props rewrite: - Shallow-copy the parent schema and only strip the keys that would re-create the cycle (oneOf, discriminator, allOf, anyOf), so object-level constraints like additionalProperties, minProperties, description, etc. carry through to the inlined entry. The previous implementation only kept type/properties/required, silently dropping every other constraint. - Per-variant shallow-clone of properties and required so downstream in-place mutations on one variant don't leak across siblings under the same parent. - Drop the inline entry entirely when nothing meaningful beyond type:'object' survives — the second allOf member (variant's own object) already asserts object-ness, and dropping keeps the existing empty-parent snapshot stable. Adds two focused unit tests: one asserts that additionalProperties:false, minProperties, and description propagate from parent to inlined variant (and that oneOf/discriminator/allOf do not), the other asserts that sibling variants get independent properties objects.
wadakatu
force-pushed
the
fix/3432-discriminator-circular-type-alias
branch
from
May 25, 2026 02:55
983db8b to
32bb1ea
Compare
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
packages/core/src/getters/discriminators.ts (1)
194-195:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftPreserve non-cyclic parent composition constraints when inlining.
At Line 194-195, deleting
allOf/anyOfunconditionally can silently drop valid parent constraints (e.g., parent inheriting shared fields viaallOf). That changes variant semantics beyond just breaking the cycle.Suggested fix direction
- delete (inlinedParent as Record<string, unknown>).allOf; - delete (inlinedParent as Record<string, unknown>).anyOf; + const isRefToParent = (ref: string): boolean => { + try { + const originalName = getRefInfo(ref, context).originalName; + return ( + originalName === parentName || + pascal(originalName) === pascal(parentName) + ); + } catch { + return false; + } + }; + + const sanitizeComposition = ( + entries?: (OpenApiSchemaObject | OpenApiReferenceObject)[], + ) => + entries?.filter( + (entry) => !isReference(entry) || !entry.$ref || !isRefToParent(entry.$ref), + ); + + const preservedAllOf = sanitizeComposition( + parentSchema.allOf as (OpenApiSchemaObject | OpenApiReferenceObject)[] | undefined, + ); + const preservedAnyOf = sanitizeComposition( + parentSchema.anyOf as (OpenApiSchemaObject | OpenApiReferenceObject)[] | undefined, + ); + + if (preservedAllOf?.length) inlinedParent.allOf = preservedAllOf.map((e) => ({ ...e })); + else delete (inlinedParent as Record<string, unknown>).allOf; + if (preservedAnyOf?.length) inlinedParent.anyOf = preservedAnyOf.map((e) => ({ ...e })); + else delete (inlinedParent as Record<string, unknown>).anyOf;🤖 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/discriminators.ts` around lines 194 - 195, The current unconditional deletes of allOf/anyOf on inlinedParent drop legitimate parent composition constraints; instead, detect and remove only the composition entries that cause the cyclic reference to the inlined variant. Locate the inlining logic around inlinedParent and replace the unconditional delete of (inlinedParent as Record<string, unknown>).allOf / anyOf with code that: inspects those arrays, filters out only the elements that reference the child/variant being inlined (by $ref or identifier used elsewhere in this module), keeps other entries, and only deletes the property if the resulting array is empty; this preserves non-cyclic parent constraints while breaking only the actual cyclic reference.
🤖 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.
Duplicate comments:
In `@packages/core/src/getters/discriminators.ts`:
- Around line 194-195: The current unconditional deletes of allOf/anyOf on
inlinedParent drop legitimate parent composition constraints; instead, detect
and remove only the composition entries that cause the cyclic reference to the
inlined variant. Locate the inlining logic around inlinedParent and replace the
unconditional delete of (inlinedParent as Record<string, unknown>).allOf / anyOf
with code that: inspects those arrays, filters out only the elements that
reference the child/variant being inlined (by $ref or identifier used elsewhere
in this module), keeps other entries, and only deletes the property if the
resulting array is empty; this preserves non-cyclic parent constraints while
breaking only the actual cyclic reference.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ac46f1fc-f2a6-4735-a855-9acde8a520c9
📒 Files selected for processing (16)
packages/core/src/getters/discriminators.test.tspackages/core/src/getters/discriminators.tstests/__snapshots__/mock/discriminator-oneof-allof-inherited/endpoints.tstests/__snapshots__/mock/discriminator-oneof-allof-inherited/model/animal.tstests/__snapshots__/mock/discriminator-oneof-allof-inherited/model/animalSpecies.tstests/__snapshots__/mock/discriminator-oneof-allof-inherited/model/cat.tstests/__snapshots__/mock/discriminator-oneof-allof-inherited/model/catSpecies.tstests/__snapshots__/mock/discriminator-oneof-allof-inherited/model/dog.tstests/__snapshots__/mock/discriminator-oneof-allof-inherited/model/dogSpecies.tstests/__snapshots__/mock/discriminator-oneof-allof-inherited/model/index.tstests/__snapshots__/mock/discriminator-oneof-allof/model/item1.tstests/__snapshots__/mock/discriminator-oneof-allof/model/item2.tstests/__snapshots__/mock/discriminator-oneof-allof/model/item3.tstests/configs/mock.config.tstests/scripts/typecheck-generated.mjstests/specifications/discriminator-oneof-allof-inherited.yaml
💤 Files with no reviewable changes (1)
- tests/scripts/typecheck-generated.mjs
✅ Files skipped from review due to trivial changes (10)
- tests/snapshots/mock/discriminator-oneof-allof-inherited/model/dog.ts
- tests/snapshots/mock/discriminator-oneof-allof-inherited/model/dogSpecies.ts
- tests/snapshots/mock/discriminator-oneof-allof-inherited/model/animal.ts
- tests/snapshots/mock/discriminator-oneof-allof-inherited/model/animalSpecies.ts
- tests/snapshots/mock/discriminator-oneof-allof-inherited/model/catSpecies.ts
- tests/snapshots/mock/discriminator-oneof-allof-inherited/model/cat.ts
- tests/snapshots/mock/discriminator-oneof-allof-inherited/endpoints.ts
- tests/snapshots/mock/discriminator-oneof-allof/model/item3.ts
- tests/snapshots/mock/discriminator-oneof-allof/model/item2.ts
- tests/snapshots/mock/discriminator-oneof-allof/model/item1.ts
melloware
approved these changes
May 25, 2026
This was referenced Jun 29, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Status
Description
Closes #3432.
When a discriminator parent has top-level
oneOflisting variants that inherit viaallOf: [{ $ref: <parent> }, ...], the type generator emits:which fails to compile with
TS2456: Type alias 'X' circularly references itself. Thediscriminator-oneof-alloffixture (added in #3431 as part of the #2155 mock fix) was excluded fromtests/scripts/typecheck-generated.mjsprecisely to defer this.Why the suggestions in the issue body need adjusting
I drafted both proposed directions in #3432 and verified them in a TypeScript sandbox:
interface Variant extends Omit<Parent, key>does not compile. TypeScript errorsTS2310: Type 'Variant' recursively references itself as a base type.interface extends Omit<UnionType, K>can't resolve whenParentis a union containingVariant.Approach
Rewrite each variant's
$ref-back-to-the-parent insideresolveDiscriminatorsso the variant never depends on the parent's alias. For every parent that has top-leveloneOf+ adiscriminator.mapping, iterate the mapping targets and walk each variant'sallOf. When an entry is a$refwhoseoriginalNamematches the parent:$refwith an inline{ type: 'object', properties, required? }carrying those properties (minus the discriminator key).Downstream
normalizeAllOfSchemathen merges what remains back into the variant's own object, andshouldCreateInterfacelets the variant emit as a non-circular shape.Scope check — other fixtures with discriminator +
oneOf+allOfI grepped the spec directory for the trigger pattern and confirmed each is untouched:
recursive-discriminator-allof.yamlBasehas no top-leveloneOf→ gate failslowercase-discriminator.yamlBase, not from the discriminator parentresp→ ref-name check failsone-of-nested.yamlallOf);Example2.expiry: allOf:[$ref: PointInFuture]is a field, not a mapping targetpolymorphic.yamlParentTypehas no top-leveloneOf→ gate failsEmpirically confirmed by regenerating all 15 clients and observing that only the
mock/discriminator-oneof-alloffixture's three variantitem*.tsfiles changed shape.discriminatorTest.ts,endpoints.ts, and every other fixture are byte-identical.Snapshot diff for the affected fixture
model/item{1,2,3}.ts— drop theOmit<DiscriminatorTest, 'type'>wrap and theDiscriminatorTestimport; emit each variant as its own shape.model/discriminatorTest.ts— unchanged.endpoints.ts(mocks) — unchanged (PR fix(mock): stop leaking sibling variants into allOf-inherited mocks #3431 already produced non-leaking mocks).Verification
packages/core/src/getters/discriminators.test.tslock in three behaviours: the rewrite drops the$refwhen the parent has no inheritable props (Discriminator parent with allOf-inheriting variants emits circular type aliases (Omit<Parent, key> & {...}↔Parent = ItemN | ...) #3432's exact shape), the rewrite inlines parent props when present, and the existing non-oneOf-parent case is left untouched.discriminator-oneof-alloffixture is removed from thetypecheck-generated.mjsexclusion list;mocknow passes typechecking alongside the other 14 clients.pnpm test,pnpm lint,pnpm typecheck,pnpm test:snapshots, and the per-clientnode ./scripts/typecheck-generated.mjsall pass locally.Summary by CodeRabbit
Bug Fixes
New Features
Tests