fix(mock): preserve discriminator value when oneOf parent declares the same property - #3429
Conversation
…e same property When an OpenAPI schema combines `oneOf` with a `discriminator` that has a `mapping`, `combineSchemasMock` was emitting the parent's free-choice enum for the discriminator property AFTER spreading the picked variant. Spread merge semantics then overwrote the variant's constrained discriminator value with a fresh random pick from the parent's enum, producing a mock whose discriminator contradicts the picked variant ~2/3 of the time. Each variant already encodes a constrained discriminator value via `resolveDiscriminators` in `packages/core/src/getters/discriminators.ts`, so the parent does not need to re-emit it. When the parent's `properties` includes the discriminator key AND the discriminator has a `mapping`, strip just that key before computing `itemResolvedValue`. Other parent properties stay intact, so common fields shared across variants still get emitted by the parent as before. The fix is guarded by three conditions (`separator === 'oneOf'`, presence of `discriminator.mapping`, and the discriminator key appearing in the parent's own `properties`), so existing fixtures without all three are untouched. Verified with the existing `polymorphic`, `recursive-discriminator-allof`, `boolean-discriminator`, and `lowercase-discriminator` specs — no snapshot diffs. Refs orval-labs#2155 (this PR addresses the top-level discriminator mismatch; the per-variant sibling leakage in `allOf`-children variants is tracked separately as a follow-up).
📝 WalkthroughWalkthroughFilters a discriminator-mapped property from the parent schema during oneOf mock combination so the parent’s unconstrained enum is not emitted alongside a variant’s constrained discriminator. Adds an OpenAPI fixture, orval config, snapshots, and a regression test asserting the corrected behavior. ChangesDiscriminator mapping fix for 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 |
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Adds a regression fixture and generator changes to ensure mocks for oneOf discriminated unions don’t accidentally override the selected variant’s discriminator value (fixing mismatch issues like #2155).
Changes:
- Added a new OpenAPI spec fixture for a discriminator +
oneOfunion and wired it into the mock test config. - Updated
combineSchemasMockto omit the parent discriminator property when aoneOfdiscriminator mapping is present. - Added a unit regression test and updated/added snapshots for the generated mock output.
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/specifications/discriminator-oneof-union.yaml | New spec fixture to reproduce discriminator + oneOf union behavior. |
| tests/configs/mock.config.ts | Registers the new fixture for mock generation in tests. |
| tests/snapshots/mock/discriminator-oneof-union/** | New snapshots validating generated models and MSW/axios endpoints for the fixture. |
| packages/mock/src/faker/getters/combine.ts | Prevents parent discriminator property from overriding the variant’s discriminator in oneOf unions with mapping. |
| packages/mock/src/faker/getters/combine.test.ts | Regression test ensuring the parent enum discriminator value is not emitted for mapped oneOf. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const discriminator = item.discriminator as | ||
| | { propertyName?: string; mapping?: Record<string, string> } | ||
| | undefined; | ||
| const itemProperties = item.properties as Record<string, unknown> | undefined; | ||
| const discriminatorPropertyName = | ||
| separator === 'oneOf' && | ||
| discriminator?.mapping && | ||
| discriminator.propertyName && | ||
| itemProperties && | ||
| discriminator.propertyName in itemProperties | ||
| ? discriminator.propertyName | ||
| : undefined; |
| item1: '#/components/schemas/item1', | ||
| item2: '#/components/schemas/item2', | ||
| item3: '#/components/schemas/item3', |
| Item1: | ||
| type: object | ||
| required: | ||
| - type | ||
| properties: | ||
| type: | ||
| type: string | ||
| property1: | ||
| type: string |
There was a problem hiding this comment.
Intentionally not constraining type in Item1/Item2/Item3 directly here.
The fixture is designed to exercise the resolveDiscriminators injection path: when only the parent declares the constrained enum via discriminator.mapping, core walks each mapping target and injects properties.<propertyName> = { enum: [<mappingKey>] } into the variant (see packages/core/src/getters/discriminators.ts). That injection is precisely the load-bearing piece that makes the mock fix safe — dropping the parent's discriminator property is only valid because every variant already carries the constrained value via this mechanism.
If I hand-write enum: [item1] on Item1, the test no longer depends on that injection path and would still pass even if resolveDiscriminators regressed and stopped touching mapping targets. The issue's original repro (#2155) also leaves variant type unconstrained, so this fixture mirrors real-world usage.
Repo regression coverage for "variants constrain discriminator themselves" already exists in tests/specifications/polymorphic.yaml, so the two fixtures together cover both injection styles.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/mock/src/faker/getters/combine.test.ts (1)
504-506: ⚡ Quick winHarden the regression matcher to be quote-style agnostic.
The assertion is currently tied to single quotes; switching to a quote-agnostic pattern makes this regression test more robust to formatter/output style changes.
Proposed test tweak
- expect(result.value).not.toMatch( - /faker\.helpers\.arrayElement\(\[\s*'item1'\s*,\s*'item2'\s*,\s*'item3'\s*\]/, - ); + expect(result.value).not.toMatch( + /faker\.helpers\.arrayElement\(\[\s*['"]item1['"]\s*,\s*['"]item2['"]\s*,\s*['"]item3['"]\s*\]/, + );🤖 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/mock/src/faker/getters/combine.test.ts` around lines 504 - 506, The current test assertion on result.value is too tied to single quotes; update the regex used in the expect(...).not.toMatch(...) to accept either single or double quotes (e.g., use a character class for quotes like ['"] around each item) so the matcher is quote-style agnostic and still detects the unwanted faker.helpers.arrayElement([...]) pattern in the combine.test.ts test (reference: the expect on result.value).
🤖 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.
Nitpick comments:
In `@packages/mock/src/faker/getters/combine.test.ts`:
- Around line 504-506: The current test assertion on result.value is too tied to
single quotes; update the regex used in the expect(...).not.toMatch(...) to
accept either single or double quotes (e.g., use a character class for quotes
like ['"] around each item) so the matcher is quote-style agnostic and still
detects the unwanted faker.helpers.arrayElement([...]) pattern in the
combine.test.ts test (reference: the expect on result.value).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ad832032-1c8d-4d65-a136-60d65b4a854c
📒 Files selected for processing (14)
packages/mock/src/faker/getters/combine.test.tspackages/mock/src/faker/getters/combine.tstests/__snapshots__/mock/discriminator-oneof-union/endpoints.tstests/__snapshots__/mock/discriminator-oneof-union/model/discriminatorTest.tstests/__snapshots__/mock/discriminator-oneof-union/model/discriminatorTestType.tstests/__snapshots__/mock/discriminator-oneof-union/model/index.tstests/__snapshots__/mock/discriminator-oneof-union/model/item1.tstests/__snapshots__/mock/discriminator-oneof-union/model/item1Type.tstests/__snapshots__/mock/discriminator-oneof-union/model/item2.tstests/__snapshots__/mock/discriminator-oneof-union/model/item2Type.tstests/__snapshots__/mock/discriminator-oneof-union/model/item3.tstests/__snapshots__/mock/discriminator-oneof-union/model/item3Type.tstests/configs/mock.config.tstests/specifications/discriminator-oneof-union.yaml
…roperties Address review feedback on orval-labs#3429: - Also filter the discriminator propertyName out of the schema's `required` array when removing it from `properties`. Leaving it in `required` would describe a schema whose required field is missing from `properties` — an inconsistency that could surprise future readers or `resolveMockValue` callers, even though no runtime regression was observed today. - Align the discriminator mapping refs in the unit test with the repo's PascalCase fixture casing (`Item1`/`Item2`/`Item3`) for consistency with `tests/specifications/discriminator-oneof-union.yaml` and other discriminator specs.
Refs #2155 (partial fix — see Scope note below for the remaining sibling-leakage defect tracked separately)
When an OpenAPI schema combines all three of
oneOf: [<variant1>, <variant2>, ...]discriminator.propertyName: <name>with a populatedmappingproperties.<name>with an enum of every mapping keycombineSchemasMockemits the parent's free-choice enum for the discriminator key after spreading the picked variant. Object spread merges win on the right, so the variant's constrained discriminator value gets re-randomized — producing a mock whosetype(or whatever the discriminator key is) contradicts the picked variant ~2/3 of the time.Why dropping the parent's discriminator key is safe
resolveDiscriminatorsinpackages/core/src/getters/discriminators.tsalready walks everydiscriminator.mappingtarget and injects a constrained enum on the variant'sproperties[propertyName](e.g.enum: ['item1']onItem1). The picked variant therefore always carries the correct discriminator value on its own — the parent re-emitting it is redundant at best and corrupting at worst.Fix
In
combineSchemasMock(separator'oneOf'), strip the discriminator key from the parent'spropertiesbefore computingitemResolvedValue. Gated by three conditions so non-discriminator unions and discriminators without mappings are untouched:separator === 'oneOf'discriminator.mappingexists (guaranteesresolveDiscriminatorsinjected constrained values into variants)propertiesOther parent properties are preserved — if a parent has both the discriminator key and a shared field (say
requestId), only the discriminator key is removed andrequestIdis still emitted from the parent.Alternative considered
Swapping the spread order (variant after the parent's
itemResolvedValue) was the other reasonable option and is mentioned in the issue. Trade-offs:oneOf + propertiesschemaI chose the targeted approach for reviewer ergonomics and to keep semantics scoped to the documented bug. Happy to switch to the order swap if you prefer the broader rule — let me know in review.
Tests
packages/mock/src/faker/getters/combine.test.ts— focused regression test (Discriminator propertyName is randomized twice in mocks causing a missmatch #2155) that fails on a trailingarrayElement(['item1','item2','item3'])override.tests/specifications/discriminator-oneof-union.yaml+tests/configs/mock.config.tsentry + new snapshot undertests/__snapshots__/mock/discriminator-oneof-union/covering the end-to-end generated mock.polymorphic,recursive-discriminator-allof,boolean-discriminator,lowercase-discriminator) generate identically — verified locally with no snapshot diffs.Scope note
The issue's reproduction spec uses
Item N = allOf:[<discriminator parent>, ...]. In that shape the per-variant mocks (getGetTestResponseItem1Mocketc.) still re-expand the parent'soneOfinside theallOfchain, leaking sibling variants into a derived's body — a separate defect in the same generator path. To keep this PR small and the snapshot diff readable, the fixture here uses standalone variants (whichresolveDiscriminatorsstill constrains viamapping). A follow-up PR will extend the spec with theallOf:[parent, ...]shape and address the sibling-leakage path.Refs #2155
Summary by CodeRabbit
Bug Fixes
Tests