fix(mock): avoid double-wrapping null branch for required nullable scalars - #3501
Merged
melloware merged 1 commit intoMay 31, 2026
Merged
Conversation
Contributor
|
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 (27)
✅ Files skipped from review due to trivial changes (11)
🚧 Files skipped from review as they are similar to previous changes (7)
📝 WalkthroughWalkthroughFixes double-wrapped null branches in OpenAPI 3.1 nullable scalar mocks. Adds ChangesFix double-wrapped null branches in nullable scalar mocks
Sequence DiagramsequenceDiagram
participant Spec as OpenAPI Spec (type: [string, null])
participant ScalarGetter as getMockScalar
participant ObjectGetter as getMockObject
participant Result as Mock result
rect rgba(255, 0, 0, 0.5)
Note over Spec,Result: OLD BEHAVIOR (Double-wrapped)
Spec->>ScalarGetter: Schema shows null-union
ScalarGetter->>ScalarGetter: Wraps value -> arrayElement([value, null])
ScalarGetter->>ObjectGetter: Returns resolvedValue (wrapped)
ObjectGetter->>ObjectGetter: Detects null-union again
ObjectGetter->>ObjectGetter: Wraps again -> arrayElement([wrappedValue, null])
ObjectGetter->>Result: Produces nested arrayElement([arrayElement([value, null]), null])
end
rect rgba(0, 255, 0, 0.5)
Note over Spec,Result: NEW BEHAVIOR (Single wrap with flag)
Spec->>ScalarGetter: Schema shows null-union
ScalarGetter->>ScalarGetter: Wraps value and sets nullWrapped=true
ScalarGetter->>ObjectGetter: Returns { value, nullWrapped: true }
ObjectGetter->>ObjectGetter: Sees nullWrapped and skips wrapping
ObjectGetter->>Result: Produces arrayElement([value, null])
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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 |
wadakatu
marked this pull request as draft
May 31, 2026 17:20
wadakatu
force-pushed
the
fix/mock-3484-double-null-wrap
branch
from
May 31, 2026 17:25
885c559 to
05ffb3e
Compare
wadakatu
marked this pull request as ready for review
May 31, 2026 17:26
…alars
A required property typed as an OpenAPI 3.1 nullable union
(`type: [<scalar>, 'null']`) — the same shape OAS 3.0 `nullable: true` is
upgraded to by @scalar/openapi-parser — was wrapped with a `null` branch
twice in faker/MSW mocks:
faker.helpers.arrayElement([
faker.helpers.arrayElement([faker.string.alpha(), null]),
null,
])
The scalar getter (`getNullable`) and the object property layer each
detected the null union independently and each added a branch, pushing
`null` to ~75% instead of ~50%.
Let the scalar getter own the null branch: it now flags the returned
`MockDefinition` with `nullWrapped` when it has already wrapped the value,
and the object property layer skips its own wrap in that case. Boolean
(and number enum/const) stay bare in the scalar getter, so the object
layer still contributes their single null branch.
Closes orval-labs#3484
wadakatu
force-pushed
the
fix/mock-3484-double-null-wrap
branch
from
May 31, 2026 19:28
05ffb3e to
f6b14a6
Compare
melloware
approved these changes
May 31, 2026
Contributor
|
Good catch! I ran into this issue at work :D |
3 tasks
This was referenced Jun 29, 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
3 participants
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.
What
Fixes the double-wrapped
nullbranch in faker/MSW mocks for required nullable scalar properties.A required property typed as an OpenAPI 3.1 nullable union (
type: [<scalar>, 'null']) — the same shape an OpenAPI 3.0nullable: truescalar is upgraded to by@scalar/openapi-parserbefore mock generation — was wrapped with anullbranch twice, pushingnullto ~75% instead of ~50%:Why
Two layers each detected the null union independently and each added a branch:
packages/mock/src/faker/getters/scalar.tswraps the leaf value viagetNullable→ innerarrayElement([value, null]).packages/mock/src/faker/getters/object.tsre-checks the property type and wraps the already-wrapped value again → outerarrayElement([..., null]).How
Let the scalar getter own the null branch. It now flags the returned
MockDefinitionwith a new optionalnullWrappedfield when it has already appliedgetNullable, and the object property layer skips its own wrap when that flag is set. Boolean (and numberenum/const, where the scalar getter does not keep the wrap) stay bare, so the object layer still contributes their single null branch — no behavior change for those.Only the required path is changed; the optional nullable branch (
arrayElement([value, undefined])/arrayElement([value, null])for omit) is intentionally untouched, as itsvalue | null | undefinednesting is a valid distribution.Tests
tests/specifications/issue-3484.yaml(required nullablestring/integer/string-enum /boolean) wired intomock.config.ts, with a focused assertion inapi-generation.spec.ts.default/all-of(category) andmock/recursive-discriminator-allof(BaseProp).format:check,build,typecheck,lint,test,test:snapshots, and the generated-output typecheck + mock verification (orval-tests build) all pass.Closes #3484
Summary by CodeRabbit
Bug Fixes
Tests
Chores
Samples