fix(mock): stop leaking sibling variants into allOf-inherited mocks by wadakatu · Pull Request #3431 · orval-labs/orval

wadakatu · 2026-05-24T14:57:44Z

Closes the second half of #2155, the part PR #3429 deliberately left out. When a derived schema is shaped as

Item1:
  allOf:
    - $ref: '#/components/schemas/DiscriminatorParent'
    - type: object
      properties:
        property1: { type: string }

and the parent declares both discriminator: { mapping } and oneOf, resolveMockValue was re-expanding the parent's oneOf while building Item1's allOf-chain mock body. That inlined sibling factory calls into the derived variant's mock, so the produced object had shapes belonging to other variants:

// Before — Item1's mock contained Item2/Item3 calls and Item3 had a `{ undefined }` artifact:
export const getGetTestResponseItem3Mock = (overrideResponse = {}) => ({
  ...{...{...{ undefined },                                  // ← from oneOf collapsing
                                                              //   to undefined inside the allOf chain
    ...{ property3: faker.helpers.arrayElement([...]) },
  }, type: faker.helpers.arrayElement(['item3'] as const)},
  ...overrideResponse,
});
export const getGetTestResponseItem2Mock = (overrideResponse = {}) => ({
  ...{...{...faker.helpers.arrayElement([{ ...getGetTestResponseItem3Mock() }]),
  ...},
  ...overrideResponse,
});
export const getGetTestResponseItem1Mock = (overrideResponse = {}) => ({
  ...{...{...faker.helpers.arrayElement([
    { ...getGetTestResponseItem2Mock() },                    // ← Item1 carries Item2's shape
    { ...getGetTestResponseItem3Mock() },                    //   and Item3's shape
  ]), ...},
  ...overrideResponse,
});

// After — each variant only describes its own properties + constrained discriminator:
export const getGetTestResponseItem1Mock = (overrideResponse = {}) => ({
  ...{...{...{ property1: faker.helpers.arrayElement([...]) }},
    type: faker.helpers.arrayElement(['item1'] as const),
  },
  ...overrideResponse,
});
// (Item2/Item3 follow the same clean shape.)

Fix

In resolveMockValue, after loading a $ref whose schema has both discriminator and oneOf, check whether the resolution stack is currently inside one of that parent's mapping targets. If so, we are by construction expanding a specific variant of the union — the parent's oneOf is descriptive, not additive — so:

Drop oneOf from a local copy of the parent before passing to getMockScalar (prevents sibling-factory inlining).
Symmetrically with fix(mock): preserve discriminator value when oneOf parent declares the same property #3429's oneOf-side fix, also drop the discriminator key from the parent's properties (and required). Each variant already carries a constrained discriminator value via resolveDiscriminators, so the parent's free-choice enum would otherwise become dead code shadowed by the variant's constrained value through spread merge.

if (
  combine?.separator === 'allOf' &&
  newSchema.discriminator &&
  newSchema.oneOf
) {
  const mappingTargetNames = Object.values(parentDiscriminator.mapping ?? {})
    .map(ref => pascal(ref.split('/').pop() ?? ''));
  const expandingAsVariant = existingReferencedProperties
    .some(refName => mappingTargetNames.includes(refName));

  if (expandingAsVariant) {
    delete (newSchema as Record<string, unknown>).oneOf;
    // ...also strip propertyName from properties + required
  }
}

Why the mapping-target guard matters

The naive form ("schema has discriminator + oneOf and we're inside allOf → strip oneOf") breaks cases where a discriminator parent is referenced via allOf but the surrounding schema is not a variant. Concrete example from this repo's tests/specifications/one-of-nested.yaml:

Example2:
  type: object
  properties:
    kind: { type: string, enum: [example2] }
    expiry:
      allOf:
        - $ref: '#/components/schemas/PointInFuture'   # PointInFuture is the discriminator parent

Example2.expiry is a field holding a PointInFuture value. The mock for expiry must still randomize across PointInFutureAbsolute / PointInFutureRelative. Without the mapping-target check, the naive fix would emit an empty expiry. The check fixes this — Example2 is not in PointInFuture.discriminator.mapping, so the strip does not apply and the snapshot for that fixture is unchanged.

Tests

New fixture: tests/specifications/discriminator-oneof-allof.yaml (the issue's exact reproduction shape).
New mock config + snapshot under tests/__snapshots__/mock/discriminator-oneof-allof/.
Focused regression in tests/api-generation.spec.ts that asserts each per-variant factory body does not reference its siblings' factories — a more precise signal than the full-file snapshot diff alone.
Existing discriminator fixtures (discriminator-oneof-union from fix(mock): preserve discriminator value when oneOf parent declares the same property #3429, recursive-discriminator-allof, one-of-nested, polymorphic, boolean-discriminator, lowercase-discriminator) generate identically — verified locally with no snapshot diffs.

Type-generation circularity (orthogonal)

The model files generated from this fixture trip a separate, pre-existing core type-generation circularity:

generated/mock/discriminator-oneof-allof/model/item1.ts(10,13):
  error TS2456: Type alias 'Item1' circularly references itself.

Item1 is emitted as Omit<DiscriminatorTest, 'type'> & { type: Item1Type, property1?: string } while DiscriminatorTest = Item1 | Item2 | Item3, which is a TS type-alias cycle. This is unrelated to the mock generator — the same fixture exhibits the issue with or without this PR — and tackling it cleanly belongs in @orval/core's getCombineSchema / discriminator paths.

To keep the mock regression testable today, the new fixture is added to the tests/scripts/typecheck-generated.mjs exclusion list (alongside the existing MCP exclusion). Follow-up issue filed for the core type-circularity so it's not lost: #3432.

Summary by CodeRabbit

Bug Fixes
- Fixed mock generation for discriminated union types with allOf-inherited variants to avoid duplicated union/discriminator behavior in generated responses.
Tests
- Added regression test ensuring inherited-variant mocks do not reference sibling factories.
- Added generated mock snapshots and model types for the discriminator/oneOf/allOf scenario.
Chores
- Added OpenAPI test spec, Orval mock config entry, and updated typecheck script exclusions.

Closes the second half of orval-labs#2155. When a derived schema is shaped as Item1: { allOf: [{ $ref: '#/components/schemas/DiscriminatorParent' }, ...] } and the parent declares both `discriminator: { mapping }` and `oneOf`, `resolveMockValue` was re-expanding the parent's `oneOf` while building the allOf-chain mock body. That inlined sibling factory calls (e.g. `getResponseItem2Mock()` and `getResponseItem3Mock()` inside `Item1`'s mock body), so each derived variant's mock contained shapes belonging to its siblings. When a discriminator parent is being expanded under an allOf chain rooted at one of its mapping targets, the current schema is — by construction — a specific variant. The parent's `oneOf` is descriptive of the union, not additive to this variant. Detect that situation (parent has both `discriminator` and `oneOf`, current `combine?.separator === 'allOf'`, and at least one mapping-target name is on the resolution stack via `existingReferencedProperties`) and drop the `oneOf` side from a local copy of the loaded parent schema before passing it to `getMockScalar`. The mapping-target check is what keeps unrelated allOf wrappings safe: e.g. `Example2.expiry: { allOf: [{ $ref: PointInFuture }] }` in `tests/specifications/one-of-nested.yaml` is NOT a variant of `PointInFuture`, just a field whose value is one of its variants, so its mock must keep randomizing across `oneOf`. Symmetrically with orval-labs#3429's oneOf-side fix, also drop the discriminator key from the parent's `properties` (and the matching entry from `required`) in the variant case. Each variant already encodes a constrained discriminator value via `resolveDiscriminators`, so leaving the parent's free-choice enum would just emit dead code immediately shadowed by the variant's constrained value through spread merge. ### Tests - `tests/specifications/discriminator-oneof-allof.yaml` mirrors the issue's exact reproduction (parent oneOf + discriminator + 3 `allOf`-inheriting variants). - Focused regression test in `tests/api-generation.spec.ts` asserts each per-variant factory body does not reference its siblings' factories. - Generated model files for this fixture trigger a separate, pre-existing core type-generation circularity (`DiscriminatorTest` -> `ItemN` via `Omit<...>`) that is orthogonal to the mock bug being fixed. The fixture is added to the `tests/scripts/typecheck-generated.mjs` exclusion list so the mock regression remains testable today; the underlying core type circularity can be tackled separately. Closes orval-labs#2155

coderabbitai · 2026-05-24T14:57:55Z

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: 18c9951f-d392-4a65-97ff-3a0a4c784cf6

📥 Commits

Reviewing files that changed from the base of the PR and between b6fd6cf and db234a4.

📒 Files selected for processing (2)

tests/api-generation.spec.ts
tests/scripts/typecheck-generated.mjs

📝 Walkthrough

Walkthrough

This PR fixes mock generation for discriminator-based unions with oneOf and allOf composition. The core resolver logic now conditionally removes the discriminator parent's oneOf and discriminator property when expanding as a variant. Test specification, generated model types, endpoint mocks, and a regression test validate the fix.

Changes

Discriminator oneOf/allOf variant mock resolution

Layer / File(s)	Summary
Core discriminator/oneOf/allOf resolution logic `packages/mock/src/faker/resolvers/value.ts`	`resolveMockValue` detects variant expansion by comparing `existingReferencedProperties` against discriminator mapping targets. When expanding as a variant, it removes the discriminator parent's `oneOf` and deletes the discriminator property from `properties` and `required` (or deletes those fields entirely if empty) to prevent shadowed union behavior.
Test specification and generator configuration `tests/specifications/discriminator-oneof-allof.yaml`, `tests/configs/mock.config.ts`, `tests/scripts/typecheck-generated.mjs`	OpenAPI 3.0.2 spec defines `DiscriminatorTest` with discriminator on `type` and `oneOf`/`allOf`-inherited variants (`Item1`, `Item2`, `Item3`); Orval config entry is added to generate mocks; typecheck excludes the fixture to cover the regression.
Generated model types and discriminator schemas `tests/__snapshots__/mock/discriminator-oneof-allof/model/*`	Exports `DiscriminatorTest` union type, `DiscriminatorTestType` discriminator literal, and three variant types (`Item1`, `Item2`, `Item3`) each with their corresponding discriminator type (`Item1Type`, `Item2Type`, `Item3Type`).
Generated endpoint client and mock factories `tests/__snapshots__/mock/discriminator-oneof-allof/endpoints.ts`	Exports axios `getTest` client helper, three variant mock object factories (`getGetTestResponseItem1Mock`, `getGetTestResponseItem2Mock`, `getGetTestResponseItem3Mock`), a discriminator union mock factory (`getGetTestResponseMock`), and MSW handler integration (`getGetTestMockHandler`, `getDiscriminatorWithOneOfUnionAndAllOfInheritedVariantsMock`).
Regression test validation `tests/api-generation.spec.ts`	Vitest test verifies that allOf-inherited variant mock factories do not inline sibling variant factory references, confirming the resolver fix prevents duplicate union behavior in generated mocks.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related PRs

orval-labs/orval#3084: Also modifies packages/mock/src/faker/resolvers/value.ts's discriminator/oneOf handling at the resolver level.
orval-labs/orval#3426: Related changes in resolveMockValue for referenced-schema expansion and discriminator handling.
orval-labs/orval#3429: Similar fix that removes parent discriminator keys before deriving variants to avoid cross-variant inlining.

Suggested labels

mock

Suggested reviewers

melloware

Poem

🐰 A discriminator's tale in code and art,
oneOf and allOf once played their part.
The resolver hops in, tidy and kind,
removes the duplicate, leaves variants aligned.
Now mocks dance true — no sibling left to find. ✨

🚥 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 describes the main fix: preventing sibling variant factories from being inlined into allOf-inherited mock variants, which is the core issue addressed in the PR.
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.

_{✏️ 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.}

Copilot

Pull request overview

Note

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

Adds a new OpenAPI fixture to reproduce a mock-generation regression involving discriminator oneOf unions with allOf-inherited variants, and updates the mock resolver + tests to prevent sibling factory leakage in generated variant mocks.

Changes:

Introduces discriminator-oneof-allof specification + mock generation config and snapshots.
Adds a focused regression test ensuring per-variant mock bodies don’t reference sibling factories.
Updates mock value resolution to drop parent oneOf (and discriminator property) when expanding a discriminator parent inside an allOf variant chain.

Reviewed changes

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

Show a summary per file

File	Description
tests/specifications/discriminator-oneof-allof.yaml	New fixture spec capturing the discriminator/oneOf + allOf-inheritance pattern.
tests/scripts/typecheck-generated.mjs	Excludes the new fixture’s generated mock output from typechecking due to known circular types.
tests/configs/mock.config.ts	Adds mock generation target for the new fixture.
tests/api-generation.spec.ts	Adds regression test asserting variant mocks don’t contain sibling factory calls.
tests/snapshots/mock/discriminator-oneof-allof/model/item3Type.ts	Snapshot for generated discriminator subtype constant/type.
tests/snapshots/mock/discriminator-oneof-allof/model/item3.ts	Snapshot for generated Item3 model type.
tests/snapshots/mock/discriminator-oneof-allof/model/item2Type.ts	Snapshot for generated discriminator subtype constant/type.
tests/snapshots/mock/discriminator-oneof-allof/model/item2.ts	Snapshot for generated Item2 model type.
tests/snapshots/mock/discriminator-oneof-allof/model/item1Type.ts	Snapshot for generated discriminator subtype constant/type.
tests/snapshots/mock/discriminator-oneof-allof/model/item1.ts	Snapshot for generated Item1 model type.
tests/snapshots/mock/discriminator-oneof-allof/model/index.ts	Snapshot barrel exports for generated model package.
tests/snapshots/mock/discriminator-oneof-allof/model/discriminatorTestType.ts	Snapshot for generated discriminator enum-like constant/type.
tests/snapshots/mock/discriminator-oneof-allof/model/discriminatorTest.ts	Snapshot for generated union model type.
tests/snapshots/mock/discriminator-oneof-allof/endpoints.ts	Snapshot for generated endpoints + mock factories/handlers.
packages/mock/src/faker/resolvers/value.ts	Fix: avoid re-expanding parent `oneOf` under allOf-variant expansion (prevents sibling factory leakage).

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

+  if (folder === 'mock') {
+    config.exclude = ['generated/mock/discriminator-oneof-allof/**'];
+  }


+    const block = endpoints.slice(
+      endpoints.indexOf(`export const ${funcName}`),
+      endpoints.indexOf(
+        'export const ',
+        endpoints.indexOf(`export const ${funcName}`) + 1,
+      ),
+    );
+    expect(


coderabbitai

Actionable comments posted: 1

🧹 Nitpick comments (1)

tests/scripts/typecheck-generated.mjs (1)

52-62: ⚡ Quick win

Add an explicit tracking issue reference for this temporary exclusion.

The rationale is clear, but please include a concrete issue link/ID in this block so removal can be tracked and verified later.

🤖 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 `@tests/scripts/typecheck-generated.mjs` around lines 52 - 62, Add a comment
with a tracking issue reference inside the conditional that excludes the fixture
so removal can be tracked later: update the if (folder === 'mock') {
config.exclude = ['generated/mock/discriminator-oneof-allof/**']; } block by
appending a short comment that includes the issue link or ID (e.g., “tracking:
ISSUE-1234” or a GitHub issue URL) and a brief note that the exclusion is
temporary; ensure the comment is adjacent to the config.exclude entry so future
readers scanning the folder/config.exclude logic see the tracking 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.

Inline comments:
In `@tests/api-generation.spec.ts`:
- Around line 540-547: The block extraction using endpoints.indexOf(`export
const ${funcName}`) can return -1 and produce an empty/truncated slice; update
the loop over variantBlocks to compute startIndex and nextIndex via
endpoints.indexOf, verify startIndex !== -1 and nextIndex !== -1 and nextIndex >
startIndex (or set nextIndex = endpoints.length when no subsequent 'export const
' exists) before slicing, and if the indices are invalid either fail the test or
skip the case with an explicit assertion error; reference the variables
variantBlocks, funcName, endpoints and the string lookup 'export const
${funcName}' when making the checks.

---

Nitpick comments:
In `@tests/scripts/typecheck-generated.mjs`:
- Around line 52-62: Add a comment with a tracking issue reference inside the
conditional that excludes the fixture so removal can be tracked later: update
the if (folder === 'mock') { config.exclude =
['generated/mock/discriminator-oneof-allof/**']; } block by appending a short
comment that includes the issue link or ID (e.g., “tracking: ISSUE-1234” or a
GitHub issue URL) and a brief note that the exclusion is temporary; ensure the
comment is adjacent to the config.exclude entry so future readers scanning the
folder/config.exclude logic see the tracking reference.

🪄 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: e05f960c-baaa-4b09-93fc-b96100157795

📥 Commits

Reviewing files that changed from the base of the PR and between f2b15fe and b6fd6cf.

📒 Files selected for processing (15)

packages/mock/src/faker/resolvers/value.ts
tests/__snapshots__/mock/discriminator-oneof-allof/endpoints.ts
tests/__snapshots__/mock/discriminator-oneof-allof/model/discriminatorTest.ts
tests/__snapshots__/mock/discriminator-oneof-allof/model/discriminatorTestType.ts
tests/__snapshots__/mock/discriminator-oneof-allof/model/index.ts
tests/__snapshots__/mock/discriminator-oneof-allof/model/item1.ts
tests/__snapshots__/mock/discriminator-oneof-allof/model/item1Type.ts
tests/__snapshots__/mock/discriminator-oneof-allof/model/item2.ts
tests/__snapshots__/mock/discriminator-oneof-allof/model/item2Type.ts
tests/__snapshots__/mock/discriminator-oneof-allof/model/item3.ts
tests/__snapshots__/mock/discriminator-oneof-allof/model/item3Type.ts
tests/api-generation.spec.ts
tests/configs/mock.config.ts
tests/scripts/typecheck-generated.mjs
tests/specifications/discriminator-oneof-allof.yaml

- `tests/scripts/typecheck-generated.mjs`: build the `exclude` array incrementally so adding a future per-folder rule cannot silently overwrite another folder's exclusion. Functionally identical today since `mcp` and `mock` are mutually exclusive, but matches Copilot's defensive suggestion. - `tests/api-generation.spec.ts` regression for orval-labs#2155: assert the block-start index is found before slicing, and fall back to `endpoints.length` when no trailing `export const` is present, so a missing/renamed factory fails the test deterministically instead of passing on an empty slice (Copilot / CodeRabbit review).

Copilot AI review requested due to automatic review settings May 24, 2026 14:57

Copilot AI reviewed May 24, 2026

View reviewed changes

wadakatu mentioned this pull request May 24, 2026

Discriminator parent with allOf-inheriting variants emits circular type aliases (Omit<Parent, key> & {...} ↔ Parent = ItemN | ...) #3432

Closed

coderabbitai Bot reviewed May 24, 2026

View reviewed changes

Comment thread tests/api-generation.spec.ts Outdated

melloware approved these changes May 24, 2026

View reviewed changes

melloware merged commit c5ff7bc into orval-labs:master May 24, 2026
6 checks passed

wadakatu mentioned this pull request May 25, 2026

fix(core): break circular type alias for allOf-inheriting discriminator variants #3435

Merged

4 tasks

This was referenced Jun 29, 2026

Add support for zod/mini generation #3666

Merged

Adds pkg.pr.new to publish PR preview versions #3668

Merged

Navigation Menu

Uh oh!

Uh oh!

fix(mock): stop leaking sibling variants into allOf-inherited mocks - #3431

fix(mock): stop leaking sibling variants into allOf-inherited mocks#3431
melloware merged 2 commits into
orval-labs:masterfrom
wadakatu:fix/2155-discriminator-allof-sibling-leakage

wadakatu commented May 24, 2026 •

edited by coderabbitai Bot

Loading

Uh oh!

coderabbitai Bot commented May 24, 2026 •

edited

Loading

Walkthrough

Changes

Estimated code review effort

Possibly related PRs

Suggested labels

Suggested reviewers

Poem

❌ Failed checks (1 warning)

Uh oh!

Copilot AI left a comment

Uh oh!

coderabbitai Bot left a comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Uh oh!

Conversation

wadakatu commented May 24, 2026 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Fix

Why the mapping-target guard matters

Tests

Type-generation circularity (orthogonal)

Summary by CodeRabbit

Uh oh!

coderabbitai Bot commented May 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Estimated code review effort

Possibly related PRs

Suggested labels

Suggested reviewers

Poem

❌ Failed checks (1 warning)

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

wadakatu commented May 24, 2026 •

edited by coderabbitai Bot

Loading

coderabbitai Bot commented May 24, 2026 •

edited

Loading