fix(mock): detect $ref in single-element allOf/oneOf/anyOf array items - #3421
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughExtracts ChangesArray Item $ref Extraction
Sequence DiagramsequenceDiagram
participant getMockScalar
participant extractItemsRef
participant resolveMockValue
getMockScalar->>extractItemsRef: extract $ref from items (direct or single-element composed)
extractItemsRef-->>getMockScalar: return itemsRef or undefined
alt itemsRef is visited
getMockScalar->>getMockScalar: return [] (short-circuit)
else itemsRef not visited
getMockScalar->>resolveMockValue: pass normalized items (direct $ref when applicable)
resolveMockValue-->>getMockScalar: return mock value
end
🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/mock/src/faker/getters/scalar.test.ts (1)
665-744: ⚡ Quick winAdd one regression test for single-element wrapper + sibling keywords.
Please add a case like
items: { allOf: [{ $ref: ... }], nullable: true }so wrapper normalization doesn’t silently strip sibling semantics in future edits.🤖 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/scalar.test.ts` around lines 665 - 744, Add a regression test in the existing describe block for getMockScalar that ensures single-element wrapper plus sibling keywords are preserved: copy the pattern used for the "returns [] when items is allOf with a single circular $ref" test but make items = { allOf: [{ $ref: '`#/components/schemas/Foo`' }], nullable: true } (using the same baseArg and item.name) and assert expect(result.value).toBe('[]'); to ensure wrapper normalization doesn't strip sibling semantics; place this next to the other single-element wrapper tests referencing getMockScalar and baseArg.
🤖 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/scalar.ts`:
- Around line 272-273: The code collapses a wrapped single-element items schema
to { $ref: itemsRef } and thus drops any sibling keywords (e.g., nullable,
default, constraints); change the assignment that builds resolvedItems to merge
the $ref into the original item.items instead of replacing it (e.g., produce a
new object that spreads item.items and adds $ref when itemsRef exists and
item.items lacks $ref). Apply the same merge fix to the analogous occurrence at
the other spot (the similar resolvedAdditionalItems/resolvedItems creation near
the second reference) so sibling keys are preserved.
---
Nitpick comments:
In `@packages/mock/src/faker/getters/scalar.test.ts`:
- Around line 665-744: Add a regression test in the existing describe block for
getMockScalar that ensures single-element wrapper plus sibling keywords are
preserved: copy the pattern used for the "returns [] when items is allOf with a
single circular $ref" test but make items = { allOf: [{ $ref:
'`#/components/schemas/Foo`' }], nullable: true } (using the same baseArg and
item.name) and assert expect(result.value).toBe('[]'); to ensure wrapper
normalization doesn't strip sibling semantics; place this next to the other
single-element wrapper tests referencing getMockScalar and baseArg.
🪄 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: a2a1fcf3-6520-4dd8-8858-8c7a4a49c7b1
📒 Files selected for processing (2)
packages/mock/src/faker/getters/scalar.test.tspackages/mock/src/faker/getters/scalar.ts
0b29c9e to
b2862a4
Compare
bd22b10 to
011df32
Compare
The recursion guard in the array case only checked for a direct `$ref`
on `item.items`, missing the common pattern where specs wrap the
reference in a single-element composition (e.g. `items: { allOf:
[{ $ref }] }`). This caused self-referential schemas to produce
`undefined[]` and enum arrays to double-wrap as `SomeEnum[][]`.
Add `extractItemsRef` helper that returns the underlying `$ref` whether
direct or wrapped in a single-element allOf/oneOf/anyOf, then normalize
the items before passing to `resolveMockValue`. Only single-element
compositions are unwrapped; multi-element compositions still flow
through the combine path.
Includes unit tests for the recursion guard across all composition
wrappers and a negative test for multi-element compositions.
011df32 to
b62be12
Compare
|
@melloware I think this is one of the last bugs that is preventing us at Turo from using the faker mocks. We might be interested in exploring support for generating mocks for models too, not just operations, that can come later. |
|
Awesome I will check it tomorrow! |
|
@jakiestfu did you want to filter the issues and look for label=mock to see if this fixes any open issues or if you want to fix any of those open issues before the next release?? |
|
@melloware I will look to see if this solves any open issues and follow up with them. Unfortunately, I likely will not be able to invest much time in tackling general orval issues unless they directly impact our ability to use the library, apologies. |
|
8.12.3 is published |
So fast, @melloware, thank you so much! |
2 participants
Summary
extractItemsRefhelper to detect$refwhether direct onitem.itemsor wrapped in a single-elementallOf/oneOf/anyOfcomposition[]instead ofundefined[]{ $ref }beforeresolveMockValue, preventing enum arrays from double-wrapping asSomeEnum[][]Reproduction
Self-referential schema producing
undefined[]Before fix: The recursion guard didn't fire because it only checked
item.items.$ref(which isundefinedwhen wrapped inallOf). The code recursed intocombineSchemasMock, which eventually skipped the already-visited ref but left the parent array case generatingArray.from(...).map(() => undefined)— producingundefined[].After fix:
extractItemsRefunwraps the single-elementallOfand the recursion guard correctly returns[].Enum array double-wrapping as
PetStatus[][]Before fix: The
allOf-wrapped items went through the composition path, so the outer array case never received theenumsflag back fromresolveMockValue. It wrapped the already-correctfaker.helpers.arrayElements(Object.values(PetStatus))(which itself returnsPetStatus[]) in an extraArray.from(...).map(...), yieldingPetStatus[][].After fix: The wrapped items are normalized to
{ $ref: '#/components/schemas/PetStatus' }before callingresolveMockValue, which resolves the enum through the normal$refpath, properly propagates theenumsflag, and the array case returns the value directly without double-wrapping.Test plan
$ref,allOf,oneOf,anyOfwrappersallOfis not unwrappednpx vitest run packages/mock/src/faker/getters/scalar.test.ts— 40 tests)Summary by CodeRabbit
Bug Fixes
Tests