Skip to content

fix(core): skip discriminator mapping entries whose target schema is absent - #3551

Merged
melloware merged 3 commits into
orval-labs:masterfrom
clementinelove:fix/discriminator-mapping-filtered-subtype
Jun 7, 2026
Merged

fix(core): skip discriminator mapping entries whose target schema is absent#3551
melloware merged 3 commits into
orval-labs:masterfrom
clementinelove:fix/discriminator-mapping-filtered-subtype

Conversation

@clementinelove

@clementinelove clementinelove commented Jun 6, 2026

Copy link
Copy Markdown
Contributor

Close #3550

Problem

When input.filters.tags is used and a surviving schema is a discriminator base (has discriminator.mapping), generation crashes:

🛑 <project> - Cannot read properties of undefined (reading 'properties')

It fails during schema generation, before any client code is emitted, so it is not client-specific — every client/httpClient combination fails identically.

Root cause

filters.tags prunes schemas not reachable via a real $ref from a matching operation. A discriminator.mapping value is a bare string ('#/components/schemas/Foo'), not a $ref object, so collectReferencedComponents correctly does not follow it (following it would re-pull most of the spec and defeat filtering). The kept base therefore retains its full discriminator.mapping, which now lists subtypes that were just pruned.

resolveDiscriminators iterates that mapping, looks each target up in the filtered schema set, and the existing guard only checked isBoolean(subTypeSchema) — not subTypeSchema === undefined — so it dereferenced .properties on the first missing target.

Fix

Make resolveDiscriminators skip a mapping entry whose target is absent (subTypeSchema === undefined) instead of dereferencing it. The kept base is emitted, surviving subtypes are generated with their discriminant, and pruned subtypes are simply ignored.

This also hardens against malformed specs that reference a non-existent subtype.

Why resolver-side only

The issue suggests two composable fixes: (1) trim pruned entries from kept bases' mappings on the filter side, and (2) guard the resolver. In the end I went with (2) only, deliberately:

  • discriminator.mapping has exactly one consumer that dereferences the target schema — resolveDiscriminators. This guard makes it safe. Other consumers (core combine, mock value/combine) read the mapping only as strings and never chase a target, so a leftover entry is inert. The mapping is never emitted
    into generated code.
  • A filter-side trim adds code that runs on every tag-filtered generation and needs casing normalization to match against the kept set — a new, small failure mode (it could wrongly drop a valid entry) for only a cosmetically cleaner intermediate spec. For a stability fix it's probably not the right call.

Testing

  • New regression test in discriminators.test.ts: a mapping with an absent target no longer throws, and the surviving subtype is still augmented with its discriminant.
  • Verified against the issue's minimal repro (filters.tags: ['cats'], prunes Dog): generates cleanly; Pet + Cat emitted with Cat.petType constrained, Dog absent.
  • Verified against a real-world spec (72 discriminator bases, tag-filtered): all projects generate — axios, react-query, and msw mock — and the output type-checks under tsc --strict.

Summary by CodeRabbit

  • Bug Fixes

    • Better handling of mappings that reference missing or undefined target schemas to prevent errors; only existing subtypes are augmented with their discriminator property.
  • Tests

    • Added a test ensuring mappings to absent schemas are skipped and remaining subtypes receive the expected discriminator restriction.

…absent

resolveDiscriminators looked up each discriminator.mapping target in the schema
set and dereferenced .properties on it. When the target was missing (e.g. a
subtype removed by filters.tags) it threw "Cannot read properties of undefined
(reading 'properties')". Skip undefined targets instead of dereferencing them.
@coderabbitai

coderabbitai Bot commented Jun 6, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 2e7561d0-df95-4099-83f0-c00dfcd219de

📥 Commits

Reviewing files that changed from the base of the PR and between 607996c and af62c21.

📒 Files selected for processing (2)
  • packages/core/src/getters/discriminators.test.ts
  • packages/core/src/getters/discriminators.ts
🚧 Files skipped from review as they are similar to previous changes (2)
  • packages/core/src/getters/discriminators.ts
  • packages/core/src/getters/discriminators.test.ts

📝 Walkthrough

Walkthrough

Skip dereferencing missing discriminator mapping targets during subtype augmentation; add a Vitest regression test confirming pruned mapping entries are ignored and only existing subtype(s) receive the discriminator property with the correct enum.

Changes

Discriminator handling for missing subtype schemas

Layer / File(s) Summary
Guard and skip logic in resolver
packages/core/src/getters/discriminators.ts, packages/core/src/getters/discriminators.test.ts
resolveDiscriminators now skips augmentation when a mapping's target schema is undefined (in addition to boolean schemas and missing propertyName); added comments explaining missing-subtype scenarios. A Vitest regression test asserts no throw and that only the existing subtype (Cat) is augmented with petType whose enum contains only the surviving mapping key ('cat').

Sequence Diagram(s)

sequenceDiagram
  participant resolveDiscriminators
  participant discriminatorMapping
  participant subTypeSchema
  resolveDiscriminators->>discriminatorMapping: iterate mapping entries (name -> ref)
  discriminatorMapping->>subTypeSchema: attempt to resolve target schema
  alt subTypeSchema is defined and valid
    resolveDiscriminators->>subTypeSchema: add discriminator property with enum [key]
  else subTypeSchema is undefined or boolean or propertyName missing
    resolveDiscriminators-->>subTypeSchema: skip augmentation
  end
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

  • orval-labs/orval#3523: Both PRs modify resolveDiscriminators to change handling of missing discriminator mapping targets and add related tests.
  • orval-labs/orval#3140: Also updates resolveDiscriminators and its tests; touches merged discriminator property handling.

Suggested labels

bug

Suggested reviewers

  • melloware
  • snebjorn

Poem

🐰 A gentle hop, a careful peek,
Missing pups no longer leak,
Cats keep their tag, the code stays bright,
Tests snug in burrow, all is right,
Rabbit cheers — small fix, big delight!

🚥 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
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately summarizes the main code change: adding a guard to skip discriminator mapping entries whose target schema is absent.
Linked Issues check ✅ Passed The PR successfully implements the resolver-side guard required by issue #3550 to prevent crashes when discriminator mapping targets are absent.
Out of Scope Changes check ✅ Passed All changes are directly scoped to fixing the discriminator mapping issue: a test case validating the fix and the guard logic in the resolver.

✏️ 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.

@clementinelove
clementinelove marked this pull request as ready for review June 6, 2026 01:59
Copilot AI review requested due to automatic review settings June 6, 2026 01:59

Copilot AI 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.

Pull request overview

Note

Copilot was unable to run its full agentic suite in this review.

Prevents resolveDiscriminators from throwing when a discriminator mapping points to a schema that isn’t present (e.g., removed by tag filtering), and adds a regression test for that scenario.

Changes:

  • Skip discriminator mapping entries when the target subtype schema is missing.
  • Add a test ensuring missing mapping targets don’t throw and remaining subtypes are still augmented.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File Description
packages/core/src/getters/discriminators.ts Adds a guard to skip missing mapped subtype schemas before dereferencing.
packages/core/src/getters/discriminators.test.ts Adds coverage for absent mapping targets and verifies discriminator augmentation still occurs.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread packages/core/src/getters/discriminators.ts
Comment thread packages/core/src/getters/discriminators.test.ts Outdated
@clementinelove
clementinelove marked this pull request as draft June 6, 2026 02:07
@clementinelove
clementinelove force-pushed the fix/discriminator-mapping-filtered-subtype branch from c2f406b to adc84aa Compare June 6, 2026 02:14
A throw would already fail the test, so the separate .not.toThrow() call is
redundant. Call resolveDiscriminators once and assert on the result.
@clementinelove
clementinelove marked this pull request as ready for review June 6, 2026 02:20

@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.

Looks like its failing Lint

@melloware melloware added the bug Something isn't working label Jun 6, 2026
Indexing the schema set with a mapping target that was filtered out (e.g.
by `filters.tags`) or absent in a malformed spec returns `undefined` at
runtime. Guard with `!subTypeSchema` before dereferencing, mirroring the
existing `!variantSchema` check in the same function, and drop a redundant
type assertion in the test.
@clementinelove
clementinelove requested a review from melloware June 6, 2026 23:06
@melloware
melloware merged commit cfca7da into orval-labs:master Jun 7, 2026
5 checks passed
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

3 participants