fix(orm): coerce ISO strings on DateTime input, with strictDateInput opt-in (#2631)#2632
Open
erwan-joly wants to merge 4 commits intozenstackhq:devfrom
Open
Conversation
…input (zenstackhq#2631) The strict zod union introduced in 3.5+ broke every caller passing ISO strings to `DateTime` fields, including bare time-only strings like "09:00:00" for `@db.Time` columns. Earlier versions coerced these via Prisma's input layer; the new validator rejected them outright with no migration path called out in the release notes. This restores Prisma-compatible coercion as the default while leaving strict validation available behind a new `ClientOptions.strictDateInput` flag (default `false`) for users who want to opt in. Changes: - `packages/orm/src/client/options.ts`: new `strictDateInput?: boolean` - `packages/orm/src/client/zod/factory.ts`: new exported helper `coercedDateTimeSchema()` that anchors time-only strings to the Unix epoch and falls through to `z.date()` for all other paths; `makeDateTimeValueSchema` switches on `strictDateInput` - `packages/zod/src/factory.ts`: same coercion applied in the standalone factory (no setting — these schemas are typically used for form validation where coercion is even more important) - `packages/zod/test/factory.test.ts`: regression tests for the four accepted forms (Date, ISO datetime, ISO date, time-only with and without timezone) plus a non-parseable rejection case Fixes zenstackhq#2631
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 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. Review rate limit: 7/8 reviews remaining, refill in 7 minutes and 30 seconds.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: 1
🧹 Nitpick comments (1)
packages/zod/test/factory.test.ts (1)
161-204: ⚡ Quick winAdd strict-mode regression coverage too.
These tests validate the default coercion path well, but they don’t cover
strictDateInput: truebehavior in ORM schema generation. Adding that case will lock the contract and prevent drift.🤖 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 161 - 204, Add parallel tests that construct the model schema with strictDateInput enabled (e.g., call factory.makeModelSchema('User', { strictDateInput: true })) and assert that ISO date strings, bare time strings, and timezone time strings are rejected (result.success === false), while actual Date instances still pass; add one test per case mirroring the existing ones (names like "strict mode rejects ISO date string", "strict mode rejects time-only string", etc.) to lock the strict-mode contract.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/orm/src/client/zod/factory.ts`:
- Around line 880-882: The current strictDateInput branch for building schema
allows ISO strings via z.iso.datetime() and z.iso.date(), which contradicts the
ClientOptions<Schema> docs saying strict mode rejects all string forms; change
the implementation in the schema construction so that when (this.options as
ClientOptions<Schema>)?.strictDateInput is true the schema is strictly z.date()
(no z.iso.* unions) and otherwise use coercedDateTimeSchema(); update or run
related tests that assumed ISO-string acceptance if you instead choose to keep
the current behavior and prefer updating docs/tests to state that
strictDateInput still accepts ISO strings.
---
Nitpick comments:
In `@packages/zod/test/factory.test.ts`:
- Around line 161-204: Add parallel tests that construct the model schema with
strictDateInput enabled (e.g., call factory.makeModelSchema('User', {
strictDateInput: true })) and assert that ISO date strings, bare time strings,
and timezone time strings are rejected (result.success === false), while actual
Date instances still pass; add one test per case mirroring the existing ones
(names like "strict mode rejects ISO date string", "strict mode rejects
time-only string", etc.) to lock the strict-mode contract.
🪄 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: 9373f55b-22da-4966-9cc6-7778b82071d2
📒 Files selected for processing (4)
packages/orm/src/client/options.tspackages/orm/src/client/zod/factory.tspackages/zod/src/factory.tspackages/zod/test/factory.test.ts
- Strict mode now actually rejects all string forms (was still accepting ISO datetime/date because the union still included z.iso.datetime() / z.iso.date()). Switched to plain z.date() so the implementation matches the JSDoc on ClientOptions.strictDateInput. - Reverted the standalone @zenstackhq/zod factory back to its previous union (z.date() | z.iso.datetime()). The OpenAPI generator emits its spec from this factory and the preprocess version produced a different schema shape that broke the rpc-openapi baseline test. The ORM-side coercion in @zenstackhq/orm/zod/factory.ts still covers the runtime input validation that zenstackhq#2631 was about; standalone consumers can layer their own coercion if they need it. - Replaced the standalone-factory regression tests with an end-to-end ORM regression suite at tests/regression/test/issue-2631.test.ts covering both default coercion (Date, ISO datetime, ISO date, time-only with and without TZ) and the strict-mode contract (Date only — ISO datetime, ISO date, and time-only strings all rejected).
Contributor
Author
|
Thanks for the catch — pushed e8cc785e addressing both points:
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/regression/test/issue-2631.test.ts (1)
18-77: ⚡ Quick winAdd explicit coverage for
strictDateInput: falseand TZ/fraction time-only variantsCoverage is good overall, but this suite doesn’t currently assert the explicit
strictDateInput: falsepath, nor the documented time-only variants (09:30:00.123,09:30:00Z,09:30:00+12:00).🧪 Suggested additions
it('accepts a bare time-only string anchored to the Unix epoch', async () => { const e = await db.event.create({ data: { label: 'time-only', when: '09:30:00' } }); expect(e.when).toBeInstanceOf(Date); expect((e.when as Date).getUTCHours()).toBe(9); expect((e.when as Date).getUTCMinutes()).toBe(30); }); + + it.each(['09:30:00.123', '09:30:00Z', '09:30:00+12:00'])( + 'accepts time-only variant %s', + async (when) => { + const e = await db.event.create({ data: { label: `time-${when}`, when } }); + expect(e.when).toBeInstanceOf(Date); + } + ); + + it('accepts ISO strings when strictDateInput is explicitly false', async () => { + const laxDb = await createTestClient(schema, { + usePrismaPush: true, + provider: 'sqlite', + strictDateInput: false, + }); + try { + await expect( + laxDb.event.create({ data: { label: 'explicit-false', when: '2024-01-15T10:30:00.000Z' } }) + ).resolves.toBeTruthy(); + } finally { + await laxDb.$disconnect(); + } + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/regression/test/issue-2631.test.ts` around lines 18 - 77, Tests are missing an explicit describe for strictDateInput: false and additional time-only string variants; add a new describe or expand the existing "default (strictDateInput unset / false)" block to explicitly create the client with strictDateInput: false via createTestClient(schema, { usePrismaPush: true, provider: 'sqlite', strictDateInput: false }) and add itests that assert time-only strings with fractions and timezones ('09:30:00.123', '09:30:00Z', '09:30:00+12:00') are accepted and coerced to Date (and preserve expected UTC hour/minute), while keeping the existing non-parseable-string rejection test; reference the db.event.create calls and the expect(e.when).toBeInstanceOf(Date) / getUTCHours()/getUTCMinutes() assertions to guide placement.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/orm/src/client/zod/factory.ts`:
- Around line 105-106: The fallback date parsing currently uses new Date(val)
(variables d and val in packages/orm/src/client/zod/factory.ts) and therefore
accepts non-ISO engine-dependent formats; change it to first validate that val
matches the allowed ISO patterns (e.g., ISO date, ISO datetime with optional
timezone, or time-only formats documented) using an explicit regex or parser
check, and only then construct new Date(val) and return the Date; if the ISO
validation fails, return the original val unchanged to preserve the documented
ISO-only contract.
---
Nitpick comments:
In `@tests/regression/test/issue-2631.test.ts`:
- Around line 18-77: Tests are missing an explicit describe for strictDateInput:
false and additional time-only string variants; add a new describe or expand the
existing "default (strictDateInput unset / false)" block to explicitly create
the client with strictDateInput: false via createTestClient(schema, {
usePrismaPush: true, provider: 'sqlite', strictDateInput: false }) and add
itests that assert time-only strings with fractions and timezones
('09:30:00.123', '09:30:00Z', '09:30:00+12:00') are accepted and coerced to Date
(and preserve expected UTC hour/minute), while keeping the existing
non-parseable-string rejection test; reference the db.event.create calls and the
expect(e.when).toBeInstanceOf(Date) / getUTCHours()/getUTCMinutes() assertions
to guide placement.
🪄 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: 50f30409-6a8e-4ba1-975d-65bff2e41745
📒 Files selected for processing (2)
packages/orm/src/client/zod/factory.tstests/regression/test/issue-2631.test.ts
Addresses CodeRabbit nitpick on zenstackhq#2632: the implementation falls through to `new Date(val)` for non-time-only strings, so engine-dependent formats like "2024/01/15" are accepted. That is intentional — the schema mirrors Prisma's pre-3.5 behaviour for compatibility — but the JSDoc previously said "ISO strings" only. Reword to describe the actual contract and point users who want stricter validation at strictDateInput.
`coercedDateTimeSchema` previously returned `z.preprocess(fn, z.date())`,
which serialised to an empty `{}` and broke the rpc-openapi baseline
(`packages/server/test/openapi/baseline/rpc.baseline.yaml`) that
documents the accepted ISO datetime / ISO date / Date forms. Wrap the
preprocess around the original `z.union([z.iso.datetime(), z.iso.date(),
z.date()])` so OpenAPI generation still emits the documented variants.
Runtime behaviour is unchanged: preprocess coerces strings into Dates
first, the union's `z.date()` arm catches everything that parses, and
non-parseable strings fall through and are rejected.
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.
Fixes #2631.
Background
The strict zod input validator introduced in 3.5+ broke every caller passing ISO strings to
DateTimefields, including bare time-only strings like"09:00:00"for@db.Timecolumns. Earlier versions silently coerced these via Prisma's input layer; the new validator rejects them outright. Existing user code that worked unchanged across years of Prisma → ZenStack v2 → ZenStack v3.4 suddenly fails withInvalid input: expected date, received string.Approach
Per option 2 from the issue, this restores Prisma-compatible coercion as the default while leaving strict validation available behind a new opt-in flag. That keeps existing callers working while preserving the stricter semantics for users who want them:
Default (
strictDateInput: falseor unset) accepts:Dateobjects (already worked)"2024-01-15T10:30:00.000Z"(already worked)"2024-01-15"(already worked in orm factory; new in standalone zod factory)"09:30:00","09:30:00.123","09:30:00Z","09:30:00+12:00"(new)All string forms are coerced to a
Datefor the engine. Time-only strings are anchored to the Unix epoch (1970-01-01T<time>), matching the existing OID-1083 read-side behavior introduced in #2590.Files changed
packages/orm/src/client/options.ts— newstrictDateInput?: booleanoption with JSDoc explaining the default and tradeoffpackages/orm/src/client/zod/factory.ts— new exported helpercoercedDateTimeSchema();makeDateTimeValueSchemaswitches onstrictDateInputpackages/zod/src/factory.ts— same coercion applied in the standalone factory (no setting — these schemas are typically used for form validation where coercion is more important)packages/zod/test/factory.test.ts— regression tests for the four accepted forms plus a non-parseable rejection case, all referencing ZenStack 3.6: strict Date input validator rejects ISO strings (regression vs 3.4 / Prisma) #2631Tests
The strict path (when
strictDateInput: true) keeps the existingz.union([z.iso.datetime(), z.iso.date(), z.date()])behavior unchanged, so users who've adopted the strict semantics on 3.5/3.6 are unaffected.Happy to adjust naming (
strictDateInputvsstrictDateInputsvs anything else) or re-shape per review.Summary by CodeRabbit
New Features
Tests