Skip to content

factory methods: discriminator aliases and missing parent properties - fix - #3524

Merged
melloware merged 1 commit into
orval-labs:masterfrom
mironbalcerzak:factory-methods-discriminator-fix
Jun 2, 2026
Merged

factory methods: discriminator aliases and missing parent properties - fix#3524
melloware merged 1 commit into
orval-labs:masterfrom
mironbalcerzak:factory-methods-discriminator-fix

Conversation

@mironbalcerzak

@mironbalcerzak mironbalcerzak commented Jun 2, 2026

Copy link
Copy Markdown
Contributor

Problem

OpenAPI specifications defining polymorphic interfaces often use a combiner (oneOf/anyOf/allOf) alongside the parent schema's own properties.
However, Orval's factory generator treated combiners and properties as mutually exclusive.
When a combiner was found, the generator returned early and completely ignored the parent's own properties.

Relates to: #3523

Result of the bad code

The generated parent factory simply returned the variant's factory output, which lacked the parent's required properties.
This caused TypeScript compiler errors (e.g., TS2322: Type 'Variant' is not assignable to type 'Variant & { parentProp: string }') when trying to assign the factory output to the generated intersection type.

Example OpenAPI Spec causing the issue

openapi: 3.1.0
components:
  schemas:
    General:
      type: object
      oneOf:
        - $ref: '#/components/schemas/Specific'
      properties:
        responseType:
          type: string
      required:
        - responseType
    Specific:
      type: object
      properties:
        question:
          type: string

How we fixed it

Refactored buildPayload in packages/core/src/generators/factory.ts to no longer treat combiners and properties as mutually exclusive.
The logic now collects payloads from both the combiner and the parent properties into an array, and combines them using Object.assign({}, ...payloads).
This ensures the generated factory correctly outputs an object satisfying the full intersection type, including all required parent and variant properties.

Summary by CodeRabbit

  • Tests

    • Added test coverage for handling combined schema payloads.
  • Refactor

    • Improved payload generation logic for composed schemas to better support merging multiple property sources.

@coderabbitai

coderabbitai Bot commented Jun 2, 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: 3a51cce2-4ed6-4ad5-b229-d84b6f71eace

📥 Commits

Reviewing files that changed from the base of the PR and between 013b906 and 169f112.

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

📝 Walkthrough

Walkthrough

This PR refactors buildPayload in the factory generator to handle composed schemas (allOf/oneOf/anyOf) combined with sibling object properties. Instead of early-returning on composition keywords, fragments are accumulated and merged via Object.assign. A test case validates that oneOf payloads correctly merge with parent properties.

Changes

oneOf and object payload composition

Layer / File(s) Summary
buildPayload refactoring for composed schemas
packages/core/src/generators/factory.ts
buildPayload now accumulates payload fragments from allOf/oneOf/anyOf in an array and conditionally adds an object payload when the schema has properties. Multiple fragments are merged via Object.assign; single fragments return directly. The prior early-return behavior and the condition schemaType === 'object' || schema.properties are replaced with fragment accumulation and a simpler '{}' fallback for schema type 'object'.
Test oneOf with sibling parent properties
packages/core/src/generators/factory.test.ts
New test case verifies that when a schema uses oneOf alongside top-level required properties, the generated factory output correctly merges variant and parent properties via Object.assign with expected default values.

Estimated Code Review Effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Suggested Labels

bug

Suggested Reviewers

  • melloware
  • snebjorn

Poem

🐰 Fragments dance in gentle arrays,
No early returns block the ways,
oneOf and siblings now unite,
Object.assign folds them tight!
Tests celebrate the merger's 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 'factory methods: discriminator aliases and missing parent properties - fix' directly addresses the two main issues resolved in this PR: missing parent properties in factory generation and discriminator alias handling.
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.

@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 merged commit a880e28 into orval-labs:master Jun 2, 2026
5 checks passed
@mironbalcerzak
mironbalcerzak deleted the factory-methods-discriminator-fix branch June 2, 2026 14:00
@mironbalcerzak

Copy link
Copy Markdown
Contributor Author

@melloware thanks once again 👍

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