fix(mock): propagate required from $ref allOf members to overriding branches - #3801
Merged
melloware merged 4 commits intoAug 4, 2026
Merged
Conversation
…ranches The faker/msw mock generator collected allOf required fields only from inline members. A property required by a $ref'd base schema, such as an enum narrowed to a single literal in an override branch, was mocked as optional and wrapped in faker.helpers.arrayElement([..., undefined]), producing mocks that fail to typecheck against the generated intersection type. combineSchemasMock now dereferences $ref members when collecting the required union, following $ref alias chains and nested allOf compositions with a cycle guard. Required names collected from a nested composition cross its boundary only when that composition also declares the property, matching the model generator (orval-labs#3663) and keeping the orval-labs#910 cyclic reference guard from flipping property omission to null. The union is also applied to sibling properties declared next to allOf, and constraint-only members ({ required: [...] }) now count (orval-labs#3750 semantics). With faker schemas: true, delegation to a shared factory is skipped when it would drop the composition-wide required union; the schema body is inlined instead.
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThe mock generator now recursively propagates required fields through ChangesRequired-field propagation
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant combineSchemasMock
participant resolveMockValue
participant SharedMockFactory
combineSchemasMock->>resolveMockValue: resolve composed property with merged required fields
resolveMockValue->>resolveMockValue: compare composition requirements with referenced schema
resolveMockValue->>SharedMockFactory: delegate only when requirements are preserved
Possibly related issues
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
Contributor
There was a problem hiding this comment.
🟡 Not ready to approve
The new schema-factory delegation guard in packages/mock/src/faker/resolvers/value.ts only checks top-level properties/required and can still incorrectly delegate (dropping requiredness) when the referenced schema declares the affected property only via its own allOf members.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Pull request overview
This PR fixes faker/MSW mock generation for allOf compositions so that required fields declared on $ref’d members (including $ref-to-$ref aliases and nested allOf) correctly propagate to sibling override branches, preventing mocks that include undefined for still-required properties.
Changes:
- Enhance
combineSchemasMockto unionrequiredacrossallOfmembers by dereferencing$refmembers (with cycle-guarding) and applying the union to siblingpropertiesdeclared next toallOf. - Adjust faker schema-factory delegation logic to avoid delegating when it would drop composition-wide requiredness.
- Add a new regression OpenAPI spec plus unit/e2e snapshot coverage for narrowed enums,
$refaliases, sibling properties, and constraint-onlyrequiredoverlays.
File summaries
| File | Description |
|---|---|
| tests/specifications/allof-narrowed-enum.yaml | New regression spec covering narrowed-enum + $ref/alias/allOf required propagation cases. |
| tests/configs/mock.config.ts | Adds a new test config entry to generate mocks for the regression spec. |
| tests/snapshots/mock/allof-narrowed-enum/model/validationErrorViolationsItem.ts | New generated type snapshot for regression corpus. |
| tests/snapshots/mock/allof-narrowed-enum/model/validationErrorErrorType.ts | New generated enum/type snapshot for regression corpus. |
| tests/snapshots/mock/allof-narrowed-enum/model/validationError.ts | New generated composed type snapshot validating requiredness via intersection. |
| tests/snapshots/mock/allof-narrowed-enum/model/siblingValidationErrorErrorType.ts | New generated enum/type snapshot for sibling-properties case. |
| tests/snapshots/mock/allof-narrowed-enum/model/siblingValidationError.ts | New generated composed type snapshot for sibling-properties case. |
| tests/snapshots/mock/allof-narrowed-enum/model/itemDetail.ts | New generated type snapshot for constraint-only required overlay case. |
| tests/snapshots/mock/allof-narrowed-enum/model/item.ts | New generated base type snapshot for overlay case. |
| tests/snapshots/mock/allof-narrowed-enum/model/index.ts | New barrel snapshot for generated model exports. |
| tests/snapshots/mock/allof-narrowed-enum/model/index.faker.ts | New faker factory snapshot validating required vs optional output branches. |
| tests/snapshots/mock/allof-narrowed-enum/model/baseErrorErrorType.ts | New generated enum/type snapshot for base schema. |
| tests/snapshots/mock/allof-narrowed-enum/model/baseErrorAlias.ts | New generated alias snapshot for $ref-to-$ref propagation case. |
| tests/snapshots/mock/allof-narrowed-enum/model/baseError.ts | New generated base type snapshot ensuring required keys exist. |
| tests/snapshots/mock/allof-narrowed-enum/model/aliasedValidationErrorErrorType.ts | New generated enum/type snapshot for aliased composition case. |
| tests/snapshots/mock/allof-narrowed-enum/model/aliasedValidationError.ts | New generated composed type snapshot for aliased base case. |
| tests/snapshots/mock/allof-narrowed-enum/endpoints.ts | New MSW endpoints snapshot exercising the inline generation path. |
| packages/mock/src/faker/resolvers/value.ts | Prevents schema-factory delegation when it would drop composition-wide requiredness. |
| packages/mock/src/faker/getters/combine.ts | Dereferences $ref allOf members to collect required unions (incl. nested allOf/aliases) and applies union to sibling properties. |
| packages/mock/src/faker/getters/combine.test.ts | Adds unit tests covering required union propagation across $ref, nested allOf, aliases, sibling properties, and constraint-only members. |
Review details
- Files reviewed: 20/20 changed files
- Comments generated: 1
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
Comment on lines
+357
to
+366
| const targetProperties = schemaRef?.properties as | ||
| | Record<string, unknown> | ||
| | undefined; | ||
| const targetOwnRequired = (schemaRef?.required as string[]) ?? []; | ||
| const delegationDropsRequired = (schemaReference.required ?? []).some( | ||
| (requiredName) => | ||
| targetProperties && | ||
| requiredName in targetProperties && | ||
| !targetOwnRequired.includes(requiredName), | ||
| ); |
@orval/angular
@orval/axios
@orval/core
@orval/effect
@orval/fetch
@orval/hono
@orval/mcp
@orval/mock
orval
@orval/query
@orval/solid-start
@orval/swr
@orval/zod
commit: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/mock/src/faker/getters/combine.ts (1)
23-119: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a direct test for the
allOf-only reference cycle guard.The recursive required-field collection is correct against all seven added tests.
derefAllOfMember'sseenguard is new infinite-loop protection, but no test exercises a pureallOfcycle where two component schemas reference each other directly inallOf(for example,A: allOf: [$ref: B],B: allOf: [$ref: A]). The existing "does not propagate nested required names" test exercises a different case:Node's self-reference happens through aproperties.parentvalue, whichcollectAllOfRequiredWithDeclarednever walks into, so it does not exercise this new cycle guard.Add a test with two schemas whose
allOfarrays reference each other directly to confirmcollectAllOfRequiredterminates instead of looping.🤖 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.ts` around lines 23 - 119, Add a focused test for collectAllOfRequired using two component schemas whose allOf entries reference each other directly, such as A referencing B and B referencing A. Assert that required-field collection terminates and returns the expected result, exercising derefAllOfMember’s seen cycle guard rather than a self-reference nested under properties.
🤖 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.ts`:
- Around line 23-119: Add a focused test for collectAllOfRequired using two
component schemas whose allOf entries reference each other directly, such as A
referencing B and B referencing A. Assert that required-field collection
terminates and returns the expected result, exercising derefAllOfMember’s seen
cycle guard rather than a self-reference nested under properties.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 819a2459-7d4b-41e6-9021-17721d1e26b7
⛔ Files ignored due to path filters (15)
tests/__snapshots__/mock/allof-narrowed-enum/endpoints.tsis excluded by!**/__snapshots__/**tests/__snapshots__/mock/allof-narrowed-enum/model/aliasedValidationError.tsis excluded by!**/__snapshots__/**tests/__snapshots__/mock/allof-narrowed-enum/model/aliasedValidationErrorErrorType.tsis excluded by!**/__snapshots__/**tests/__snapshots__/mock/allof-narrowed-enum/model/baseError.tsis excluded by!**/__snapshots__/**tests/__snapshots__/mock/allof-narrowed-enum/model/baseErrorAlias.tsis excluded by!**/__snapshots__/**tests/__snapshots__/mock/allof-narrowed-enum/model/baseErrorErrorType.tsis excluded by!**/__snapshots__/**tests/__snapshots__/mock/allof-narrowed-enum/model/index.faker.tsis excluded by!**/__snapshots__/**tests/__snapshots__/mock/allof-narrowed-enum/model/index.tsis excluded by!**/__snapshots__/**tests/__snapshots__/mock/allof-narrowed-enum/model/item.tsis excluded by!**/__snapshots__/**tests/__snapshots__/mock/allof-narrowed-enum/model/itemDetail.tsis excluded by!**/__snapshots__/**tests/__snapshots__/mock/allof-narrowed-enum/model/siblingValidationError.tsis excluded by!**/__snapshots__/**tests/__snapshots__/mock/allof-narrowed-enum/model/siblingValidationErrorErrorType.tsis excluded by!**/__snapshots__/**tests/__snapshots__/mock/allof-narrowed-enum/model/validationError.tsis excluded by!**/__snapshots__/**tests/__snapshots__/mock/allof-narrowed-enum/model/validationErrorErrorType.tsis excluded by!**/__snapshots__/**tests/__snapshots__/mock/allof-narrowed-enum/model/validationErrorViolationsItem.tsis excluded by!**/__snapshots__/**
📒 Files selected for processing (5)
packages/mock/src/faker/getters/combine.test.tspackages/mock/src/faker/getters/combine.tspackages/mock/src/faker/resolvers/value.tstests/configs/mock.config.tstests/specifications/allof-narrowed-enum.yaml
…tion guard The delegation guard for composition-wide required unions read only the referenced schema's direct properties and required. When the target declares its properties behind its own allOf (overlay pattern), the guard saw nothing, delegation went ahead, and the shared factory kept the arrayElement([..., undefined]) branch for a property the composition requires, so the generated mock failed to typecheck. The guard now computes the target's effective declared and required sets with the same recursive collector used by combineSchemasMock, moved to getters/all-of-required.ts and shared by both call sites. Addresses the review finding on orval-labs#3801.
…tual $ref cycles Focused tests for the collector itself: a pair of component schemas whose allOf entries reference each other directly must terminate through the seen guard while still collecting the required fields declared on each side, and a cyclic $ref alias chain must resolve to nothing. Previously the guard was only exercised transitively through combineSchemasMock cycle tests that carry no required fields. Addresses a review nitpick on orval-labs#3801.
Contributor
There was a problem hiding this comment.
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/mock/src/faker/getters/all-of-required.ts`:
- Around line 63-96: Update collectAllOfRequiredWithDeclared so each schema
member is traversed with a branch-local copy of seen, preventing nested
traversal of one sibling from suppressing a later direct reference to the same
target. Preserve cycle protection within each traversal path and keep the
existing declared/required boundary filtering unchanged.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: a7c849ae-feaa-4d07-b9fc-29f1243ef471
⛔ Files ignored due to path filters (4)
tests/__snapshots__/mock/allof-narrowed-enum/model/index.faker.tsis excluded by!**/__snapshots__/**tests/__snapshots__/mock/allof-narrowed-enum/model/index.tsis excluded by!**/__snapshots__/**tests/__snapshots__/mock/allof-narrowed-enum/model/overlayBase.tsis excluded by!**/__snapshots__/**tests/__snapshots__/mock/allof-narrowed-enum/model/overlayDetail.tsis excluded by!**/__snapshots__/**
📒 Files selected for processing (5)
packages/mock/src/faker/getters/all-of-required.tspackages/mock/src/faker/getters/combine.test.tspackages/mock/src/faker/getters/combine.tspackages/mock/src/faker/resolvers/value.tstests/specifications/allof-narrowed-enum.yaml
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/mock/src/faker/getters/combine.ts
- packages/mock/src/faker/resolvers/value.ts
melloware
approved these changes
Aug 3, 2026
melloware
requested changes
Aug 3, 2026
melloware
left a comment
melloware
left a comment
Collaborator
There was a problem hiding this comment.
Can you address all the AI feedback?
The seen set in collectAllOfRequiredWithDeclared was shared across
sibling members and nesting levels, so the first visit of a target
consumed the guard for the whole traversal. A schema referenced both
inside one member's nested chain and directly as a later sibling was
skipped on the direct visit, losing its required list and defeating the
direct-member exemption:
Detail:
allOf:
- $ref: Wrapper # Wrapper.allOf: [$ref Constraint]
- $ref: Constraint # { required: [id] }
Each member now traverses with a branch-local copy of the set, so the
guard only blocks refs repeating within a single path. Termination is
preserved, and the collected union no longer depends on visit order.
Same pattern as resolvesToObjectLike in resolvers/value.ts.
Addresses the CodeRabbit review on orval-labs#3801.
Contributor
Author
All AI feedback addressed. Copilot's point about the shallow delegation guard was fixed in 2e53962 (recursive target resolution), and CodeRabbit's shared-seen finding is fixed in 533d552 (path-scoped cycle guard, with regression tests for the reported shape). All checks pass locally including the generated-output typecheck. |
melloware
approved these changes
Aug 4, 2026
This was referenced Aug 6, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
When an allOf member narrows an inherited enum property to a single literal without re-listing it in its own required, the mock generator emitted faker.helpers.arrayElement([value, undefined]) for that property. The property stays required through the $ref'd base schema, so the generated mock fails to typecheck (Type 'undefined' is not assignable to the enum union).
Root cause: combineSchemasMock collected the allOf required union only from inline members and never dereferenced $ref members, so required fields declared on a referenced base were invisible to sibling override branches.
The fix dereferences $ref members when collecting the union, following $ref alias chains and nested allOf compositions with a cycle guard. Names collected from a nested composition cross its boundary only when that composition also declares the property, keeping mock requiredness aligned with the generated types (#3663) and preventing the #910 cyclic reference guard from flipping property omission to null. The union now also applies to sibling properties declared next to allOf, and constraint-only members ({ required: [...] }) count as well (#3750 semantics). With faker schemas: true, factory delegation is skipped when it would drop the composition-wide required union; the schema body is inlined instead.
Covered by 7 new unit tests in packages/mock and an e2e regression spec (tests/specifications/allof-narrowed-enum.yaml) exercising both the msw inline path and the faker schema-factory path, with the generated output part of the typechecked corpus. All pre-existing snapshots are unchanged.
Summary by CodeRabbit
Bug Fixes
Tests