fix(zod): report a misplaced boolean required instead of crashing - #3822
fix(zod): report a misplaced boolean required instead of crashing#3822luantaraschi wants to merge 2 commits into
required instead of crashing#3822Conversation
A schema object's `required` is an array of property names, but some
generators put `required: true` on the schema a request body references,
borrowing the boolean that belongs on the request body object itself.
Spreading that boolean failed with
(schema.required ?? []) is not iterable
which names neither the offending schema nor the expected shape, and
stopped generation of otherwise usable clients.
Both spread sites now read `required` through a helper that names the
schema and states what the keyword should contain. The document is not
rewritten: a boolean carries no property names, so there is nothing to
recover from it, and guessing would change validation silently.
Refs orval-labs#3719
📝 WalkthroughWalkthroughThe Zod package now validates schema-level ChangesRequired schema validation
Estimated code review effort: 2 (Simple) | ~10 minutes 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 |
![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
🤖 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/zod/src/index.ts`:
- Line 786: Update the allOf resolution handling near getRequiredKeys so each
resolved member is validated with getRequiredKeys(resolved, name) when resolved
exists, rather than validating only the composing schema. Preserve the existing
behavior for unresolved members and add a regression test covering an allOf
constraint-only member with required: true, which must report the malformed
schema.
🪄 Autofix
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: f5ffe00e-c519-488d-ba66-e45048f54fdb
📒 Files selected for processing (2)
packages/zod/src/index.tspackages/zod/src/zod.test.ts
@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: |
A constraint-only member has no properties, so it never reaches the object path that validates `required`. Its `required` was read with `Array.isArray`, which meant the same malformed keyword was reported on the composing schema and ignored one level down. The member is now read through the same helper. The label names the member rather than the composing schema, because pointing at the parent sends the reader to the wrong place in the document: an inline member reports as `Parent.allOf[1]` and a `$ref` member reports as its ref.
|
The finding holds. I checked it before changing anything: with { allOf: [{ type: 'object', properties: { a: { type: 'string' } } }, { required: true }] }generation completes today and the member renders as One change from the proposed diff. It passes Added two tests: the bad inline member, and a valid |
Implements option 2 from #3719, the one @snebjorn voted for and @melloware agreed with: report the invalid document rather than normalize it.
Reproduced first. A schema carrying
required: truereaches the two spread sites inpackages/zod/src/index.tsand fails with:That message names neither the schema nor what the keyword should hold, and generation of the whole client stops.
Both sites now read
requiredthrough a small helper that returns the array when the document is valid and otherwise throws:I did not normalize the value. A boolean carries no property names, so there is nothing to recover from it, and picking a set of required keys would change validation without the user knowing. That is the ambiguity @snebjorn raised.
Five tests, in
packages/zod/src/zod.test.ts: the plain object path, theallOfpath, theallOfmember one level down, and two controls asserting a validrequiredarray still generates, one on a schema and one on a member. Run against the commit before the fix, the first three fail and both controls pass on either side, so they are not asserting something that was already true. The three fail in two different ways, which is the point of the third one:The member one level down never threw at all. It rendered as
zod.unknown()and said nothing.packages/zodgoes from 320 to 325 tests, all passing.tsc --noEmitandvp fmt --checkare clean.Scope note:
packages/mock/src/faker/resolvers/value.tsspreadsschemaReference.requiredthe same way, so the same document shape would fail there too. I left it out to keep this focused on the reported path, and can send it separately if you want it covered.Summary by CodeRabbit
Bug Fixes
requiredvalues.Tests
requiredconfigurations, including combined and referenced schemas.