Skip to content

feat(core): resolveDiscriminators in cycle - fix - #3523

Merged
melloware merged 1 commit into
orval-labs:masterfrom
mironbalcerzak:resolveDiscriminators_cycle_fix
Jun 2, 2026
Merged

feat(core): resolveDiscriminators in cycle - fix#3523
melloware merged 1 commit into
orval-labs:masterfrom
mironbalcerzak:resolveDiscriminators_cycle_fix

Conversation

@mironbalcerzak

@mironbalcerzak mironbalcerzak commented Jun 2, 2026

Copy link
Copy Markdown
Contributor

Problem

OpenAPI specifications defining polymorphic interfaces permits a discriminator with a propertyName and a oneOf/anyOf array, but omit the explicit mapping dictionary.

Result of the bad code

Orval's resolveDiscriminators cycle-breaking logic was strictly guarded by a check for discriminator.mapping. When the mapping was missing, Orval skipped inlining the parent schema's properties into the variants.
This resulted in the parent type being generated as a union of variants (type Parent = VariantA | VariantB), while variants were generated as intersections extending the parent (type VariantA = Parent & { ... }).
This circular dependency caused TypeScript compiler error TS2456: Type alias circularly references itself.

Example OpenAPI Spec causing the issue

openapi: 3.1.0
components:
  schemas:
    Animal:
      type: object
      discriminator:
        propertyName: type
      oneOf / anyOf:
        - $ref: '#/components/schemas/Dog'
        - $ref: '#/components/schemas/Cat'
      properties:
        type:
          type: string
      required:
        - type
    Dog:
      allOf:
        - $ref: '#/components/schemas/Animal'
        - type: object
          properties:
            bark:
              type: string
    Cat:
      allOf:
        - $ref: '#/components/schemas/Animal'
        - type: object
          properties:
            meow:
              type: string

How we fixed it

Relaxed the guard condition in packages/core/src/getters/discriminators.ts to check for the presence of discriminator and oneOf/anyOf, rather than strictly requiring discriminator.mapping. When mapping is absent, the logic now dynamically extracts the variant $ref strings directly from the oneOf/anyOf array to identify which schemas need the parent's properties inlined.

Summary by CodeRabbit

  • Bug Fixes

    • Improved discriminator handling to also resolve variant references when explicit mappings are absent, deriving them from schema composition (oneOf/anyOf) to ensure correct variant rewriting.
  • Tests

    • Added test coverage validating discriminator resolution and correct transformation of variant schemas when explicit mappings are omitted.

@coderabbitai

coderabbitai Bot commented Jun 2, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

📝 Walkthrough

Walkthrough

The PR broadens resolveDiscriminators to handle discriminator parents that use oneOf or anyOf but have no discriminator.mapping; variant $ref lists are built from mapping values (when present) plus $ref entries found in the parent composition, and tests verify variant allOf entries drop the parent $ref.

Changes

Discriminator Resolution Without Mapping

Layer / File(s) Summary
Variant reference derivation and processing
packages/core/src/getters/discriminators.ts, packages/core/src/getters/discriminators.test.ts
resolveDiscriminators now derives variant $refs from discriminator.mapping when present or from $ref entries in the parent oneOf/anyOf when mapping is absent, deduplicates them, and iterates over that list to rewrite variant allOf entries. New tests validate behavior for unmapped oneOf and anyOf parents where variant allOf entries lose the parent $ref and reduce to a single inlined object.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

  • orval-labs/orval#3159: Also updates resolveDiscriminators to change discriminator-driven variant handling; related changes to mapping and emitted discriminator types.
  • orval-labs/orval#3140: Modifies discriminator resolution logic in resolveDiscriminators, related to merging/discarding discriminator properties.
  • orval-labs/orval#3431: Adjusts handling of parent oneOf discriminator behavior and mock value propagation for derived variants.

Suggested labels

openapi

Suggested reviewers

  • melloware
  • snebjorn

Poem

🐰 A discriminator danced in the schemas' glen,
When mapping was absent, we found refs again.
From oneOf or anyOf the variants we glean,
Now allOf drops parent links — tidy and clean. ✨

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title directly describes the main fix: resolveDiscriminators circular type alias issue. It accurately reflects the core change in the discriminators.ts file.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 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 124-131: The current logic that builds variantRefs only uses
mapping when present, which omits implicit parentSchema.oneOf refs not covered
by discriminator.mapping; update the construction of variantRefs in
discriminators.ts to combine both sources: take Object.values(mapping) (if
mapping exists) and also include any refs from parentSchema.oneOf that are
reference items (isReference(...) && typeof $ref === 'string') but not already
present in the mapping values, then deduplicate the result so all oneOf variants
(mapped and unmapped) get rewritten; refer to the existing symbols variantRefs,
mapping, parentSchema.oneOf and isReference when locating and changing the code.
🪄 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: d9f96f34-297c-484f-8a42-fda499fcedda

📥 Commits

Reviewing files that changed from the base of the PR and between 146124d and 412a704.

📒 Files selected for processing (2)
  • packages/core/src/getters/discriminators.test.ts
  • packages/core/src/getters/discriminators.ts

Comment thread packages/core/src/getters/discriminators.ts Outdated
@melloware melloware added the bug Something isn't working label Jun 2, 2026
@melloware melloware added this to the 8.15.0 milestone Jun 2, 2026

@melloware melloware left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Linter fixes needed

@mironbalcerzak
mironbalcerzak force-pushed the resolveDiscriminators_cycle_fix branch from 412a704 to fa57d15 Compare June 2, 2026 11:48
@melloware
melloware merged commit 013b906 into orval-labs:master Jun 2, 2026
5 checks passed
@mironbalcerzak

Copy link
Copy Markdown
Contributor Author

thanks @melloware 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants