feat(zod): enforce literal true for select/include/omit options + exclude relation fields from makeModelSchema by default + add "optionality" setting to control runtime optionality of fields#2525
Open
marcsigmund wants to merge 7 commits intozenstackhq:devfrom
Conversation
BREAKING CHANGE: `makeModelSchema()` no longer includes relation fields by default to prevent infinite nesting with circular relations and align with ORM behavior. Use `include` or `select` options to explicitly opt in to relation fields.
Contributor
📝 WalkthroughWalkthroughmakeModelSchema() now omits relation fields by default; Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/zod/src/types.ts (1)
313-344:⚠️ Potential issue | 🟠 MajorPre-declared options with explicit type annotations produce incorrect schema shapes.
When options are stored in a typed variable before passing to
makeModelSchema, the optional mapped type inBuildIncludeOmitShapecauseskeyof Oto include all declared scalar field keys. This makesField extends keyof Otrue for every field, mapping them all toneverand producing an empty schema that diverges from runtime behavior and the inline literal case.For example:
const omitOptions: ModelSchemaOptions<Schema, 'User'>['omit'] = { username: true }; const schema = factory.makeModelSchema('User', { omit: omitOptions }); type Result = z.infer<typeof schema>; // Result type is {} (empty), but runtime schema correctly includes all scalar fields except usernameThe runtime behavior is correct because
rawOptionsSchema.parse()validates the actual object, but the inferred TypeScript type diverges from the runtime schema.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/zod/src/types.ts` around lines 313 - 344, The mapped type in BuildIncludeOmitShape currently omits a field merely if Field extends keyof O, which collapses to all declared scalar keys when O is a pre-typed options object; change the conditional to only omit when the option for that field is actually true (e.g. Field extends keyof O ? (O[Field] extends true ? never : Field) : Field) so the type mirrors runtime behavior; update the mapped branch inside BuildIncludeOmitShape that currently reads "O extends object ? Field extends keyof O ? never : Field : Field" to perform the value-check against O[Field] instead.
🧹 Nitpick comments (1)
packages/zod/test/factory.test.ts (1)
121-131: Add regression cases for rejectedfalseflags.The new assertions cover the default relation behavior, but they no longer pin the other breaking change:
{ select/include/omit: { field: false } }should throw. A fewas anycases here would keeprawOptionsSchemafrom regressing silently.Suggested cases
+ it('rejects false flags in select/include/omit', () => { + expect(() => factory.makeModelSchema('User', { select: { id: false } } as any)).toThrow(); + expect(() => factory.makeModelSchema('User', { include: { posts: false } } as any)).toThrow(); + expect(() => factory.makeModelSchema('User', { omit: { username: false } } as any)).toThrow(); + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/zod/test/factory.test.ts` around lines 121 - 131, Add regression tests to ensure option flags with false values are rejected: update the test file to call factory.makeModelSchema('User') and assert that raw options like { select: { posts: false } }, { include: { posts: false } } and { omit: { posts: false } } (cast as any to bypass TypeScript) produce validation failures via userSchema.safeParse(...). This targets the rawOptionsSchema behavior and prevents silent regression of the `{ field: false }` case.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@BREAKINGCHANGES.md`:
- Line 6: Update BREAKINGCHANGES.md to add a second, separate breaking-change
bullet explaining the literal-true option change: state that because
makeModelSchema() now excludes relation fields by default callers that
previously passed non-literal booleans (false or widened boolean types) to
select/include/omit will break, and they must now pass literal true (or narrow
the type) to opt into relations; reference the options by name (select, include,
omit) and the function makeModelSchema() so readers can locate affected call
sites and update their types/arguments accordingly.
In `@packages/zod/src/factory.ts`:
- Around line 52-56: The current schema uses z.object() for nested
rawOptionsSchema which silently strips unknown keys; replace the nested usage
with z.strictObject() for rawOptionsSchema (and any nested schemas used by
select/include entries) so validation rejects unknown keys early — update the
rawOptionsSchema definition referenced by select/include (and ensure any places
constructing rawOptionsSchema for select/include/omit use z.strictObject()
instead of z.object()) so typos like "selsct" are rejected during parsing.
---
Outside diff comments:
In `@packages/zod/src/types.ts`:
- Around line 313-344: The mapped type in BuildIncludeOmitShape currently omits
a field merely if Field extends keyof O, which collapses to all declared scalar
keys when O is a pre-typed options object; change the conditional to only omit
when the option for that field is actually true (e.g. Field extends keyof O ?
(O[Field] extends true ? never : Field) : Field) so the type mirrors runtime
behavior; update the mapped branch inside BuildIncludeOmitShape that currently
reads "O extends object ? Field extends keyof O ? never : Field : Field" to
perform the value-check against O[Field] instead.
---
Nitpick comments:
In `@packages/zod/test/factory.test.ts`:
- Around line 121-131: Add regression tests to ensure option flags with false
values are rejected: update the test file to call
factory.makeModelSchema('User') and assert that raw options like { select: {
posts: false } }, { include: { posts: false } } and { omit: { posts: false } }
(cast as any to bypass TypeScript) produce validation failures via
userSchema.safeParse(...). This targets the rawOptionsSchema behavior and
prevents silent regression of the `{ field: false }` case.
🪄 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
Run ID: 54ce258a-c6eb-4da0-a455-be9509e682eb
📒 Files selected for processing (4)
BREAKINGCHANGES.mdpackages/zod/src/factory.tspackages/zod/src/types.tspackages/zod/test/factory.test.ts
Add new `optionality` option to schema factory that controls which fields are made optional in generated Zod schemas: - `"all"`: makes every field optional - `"defaults"`: makes only fields with @default, @updatedat, or already optional fields optional - `undefined`: no changes (default behavior) The implementation preserves field metadata (descriptions) when wrapping schemas with `.optional()` to maintain documentation in generated schemas. This provides more flexibility when generating schemas for partial updates or forms where certain fields should be optional based on their defaults.
…lSchema Add extensive test coverage for the `optionality` option in the schema factory's `makeModelSchema` method. Tests cover three modes: - `optionality: 'all'` - makes all fields optional - `optionality: 'defaults'` - makes only fields with @default or @updatedat optional - `optionality: 'required'` - keeps all fields required (default behavior) Tests verify: - Field optionality behavior and type inference - Validation of field constraints when values are provided - Interaction with other options (omit, select, strict) - Preservation of metadata (descriptions) - Handling of computed fields and default values - Proper validation of required vs optional fields This ensures the optionality feature works correctly across different scenarios and combinations with other schema options.
…ema methods
Deprecate `makeModelCreateSchema` and `makeModelUpdateSchema` methods in favor of the more flexible `makeModelSchema` method with optionality parameter. Users should now use `makeModelSchema(model, { optionality: 'defaults' })` and `makeModelSchema(model, { optionality: 'all' })` respectively.
This change consolidates the API surface and provides a more consistent interface for schema generation with different optionality requirements.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/zod/test/factory.test.ts (1)
1344-1525: Consider consolidating duplicateoptionalitytest blocks.Lines 1344-1525 (
describe('optionality option', ...)) largely duplicate the tests from lines 1193-1342 (describe('optionality', ...)). Both blocks test the same functionality with very similar assertions. Consider merging these into a single comprehensivedescribeblock to reduce maintenance burden and test execution time.♻️ Suggested consolidation
The two
describeblocks can be merged. For example, keep the more descriptive test names from the second block while removing redundant assertions that are already covered in the first block:- // ── optionality ───────────────────────────────────────────────────────── - describe('optionality option', () => { - describe('optionality: "all" — every field becomes optional', () => { - it('accepts an empty object when optionality is "all"', () => { - // ... duplicate of line 1197-1200 - }); - // ... more duplicates - }); - });Keep unique tests from the second block (e.g.,
'makes@default(autoincrement) and@default(now) fields optional on Asset') and merge them into the first block.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/zod/test/factory.test.ts` around lines 1344 - 1525, The tests under describe('optionality option', ...) duplicate the earlier describe('optionality', ...) block; consolidate by removing one block and merging any unique cases: keep a single describe('optionality' or 'optionality option') block and ensure it contains all unique it(...) tests (e.g., the Asset `@default`(autoincrement)/@default(now) case) and the makeModelCreateSchema/makeModelUpdateSchema cases; update references to factory.makeModelSchema, factory.makeModelCreateSchema and factory.makeModelUpdateSchema so each unique assertion appears only once and delete the redundant describe block to avoid duplicated assertions and test execution.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/zod/test/factory.test.ts`:
- Around line 1344-1525: The tests under describe('optionality option', ...)
duplicate the earlier describe('optionality', ...) block; consolidate by
removing one block and merging any unique cases: keep a single
describe('optionality' or 'optionality option') block and ensure it contains all
unique it(...) tests (e.g., the Asset `@default`(autoincrement)/@default(now)
case) and the makeModelCreateSchema/makeModelUpdateSchema cases; update
references to factory.makeModelSchema, factory.makeModelCreateSchema and
factory.makeModelUpdateSchema so each unique assertion appears only once and
delete the redundant describe block to avoid duplicated assertions and test
execution.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 67ef622e-6a1d-4322-bb38-ec863acfb784
📒 Files selected for processing (4)
packages/zod/src/factory.tspackages/zod/src/types.tspackages/zod/src/utils.tspackages/zod/test/factory.test.ts
Consolidate optionality: 'all' tests by removing redundant runtime validation tests and keeping only type-level inference assertions. The runtime behavior is already covered by other test suites, so this change reduces duplication and improves test organization by grouping type inference tests together.
marcsigmund
changed the title
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
1 participant
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.
Summary by CodeRabbit
Breaking Changes
New Features
Bug Fixes / Behavior