fix(@db.Date): accept plain date strings in DateTime @db.Date field filters#2572
fix(@db.Date): accept plain date strings in DateTime @db.Date field filters#2572lsmith77 wants to merge 3 commits intozenstackhq:devfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
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 (2)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughDateTime Zod schemas were widened to accept ISO date strings ( Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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.
🧹 Nitpick comments (1)
tests/e2e/orm/client-api/db-date-filter.test.ts (1)
30-33: Remove unnecessary writes in schema-only validation test.This case only checks
safeParse, so the twocreatecalls are not needed and add E2E runtime overhead.♻️ Optional cleanup
it('accepts a plain date string ("YYYY-MM-DD") in an equality filter on a `@db.Date` field', async () => { - await client.event.create({ data: { name: 'Conference', eventDate: new Date('2007-05-23') } }); - await client.event.create({ data: { name: 'Workshop', eventDate: new Date('2024-01-15') } }); - const filterSchema = client.$zod.makeFindManySchema('Event'); const result = filterSchema.safeParse({ where: { eventDate: '2007-05-23' } }); expect(result.success, `Expected plain date string to be accepted, got: ${JSON.stringify(result.error)}`).toBe( true, ); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/orm/client-api/db-date-filter.test.ts` around lines 30 - 33, The test "accepts a plain date string (\"YYYY-MM-DD\") in an equality filter on a `@db.Date` field" currently performs two unnecessary writes via client.event.create; remove the two await client.event.create(...) lines so the test only performs schema-only validation (safeParse) to avoid E2E runtime overhead—keep the rest of the test intact and ensure any references to those created records are also removed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/e2e/orm/client-api/db-date-filter.test.ts`:
- Around line 30-33: The test "accepts a plain date string (\"YYYY-MM-DD\") in
an equality filter on a `@db.Date` field" currently performs two unnecessary
writes via client.event.create; remove the two await client.event.create(...)
lines so the test only performs schema-only validation (safeParse) to avoid E2E
runtime overhead—keep the rest of the test intact and ensure any references to
those created records are also removed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 9e4bbe47-96a5-49d9-9aa8-a631eaa38b88
📒 Files selected for processing (2)
packages/orm/src/client/zod/factory.tstests/e2e/orm/client-api/db-date-filter.test.ts
|
Hi @lsmith77 , thanks for working on this. Since the change is only related to filtering, I think we can unconditionally support it for What do you think? |
lsmith77
force-pushed
the
plain-date
branch
2 times, most recently
from
4234f76 to
bd695df
Compare
|
I made the necessary changes. |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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`:
- Line 855: The shared makeDateTimeValueSchema (used via
makeScalarSchema('DateTime')) currently includes z.iso.date(), which
unintentionally permits plain date strings everywhere; remove z.iso.date() from
the union in makeDateTimeValueSchema so the shared DateTime scalar only accepts
Date objects and ISO datetimes (keep z.date() and z.iso.datetime()), and instead
add z.iso.date() only at the field/filter-specific validation code that handles
`@db.Date` filters (i.e., where field-level or filter-level schemas are created),
so date-only strings are accepted only for `@db.Date` contexts.
In `@tests/e2e/orm/client-api/db-date-filter.test.ts`:
- Around line 76-82: The test "plain date string is accepted on a regular
DateTime field (no `@db.Date`)" contradicts the intended behavior; update the test
so the filter schema for Event (created via
client.$zod.makeFindManySchema('Event')) rejects a plain 'YYYY-MM-DD' string for
the createdAt DateTime field: call filterSchema.safeParse({ where: { createdAt:
'2007-05-23' } }) and assert result.success is false (or assert that parsing
fails/throws), ensuring the expectation aligns with the non-@db.Date rule.
🪄 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: 5e4dc56c-2518-44a2-b2ba-a904456d85c6
📒 Files selected for processing (2)
packages/orm/src/client/zod/factory.tstests/e2e/orm/client-api/db-date-filter.test.ts
| @cache() | ||
| private makeDateTimeValueSchema(): ZodType { | ||
| const schema = z.union([z.iso.datetime(), z.date()]); | ||
| const schema = z.union([z.iso.date(), z.iso.datetime(), z.date()]); |
There was a problem hiding this comment.
Scope regression: this change accepts plain date strings globally, not only @db.Date filters.
makeDateTimeValueSchema() is shared via makeScalarSchema('DateTime'), so adding z.iso.date() expands acceptance beyond the intended filter-only @db.Date case (including regular DateTime and mutation input validation paths). Please gate date-only acceptance at the field/filter level instead of the shared scalar DateTime value schema.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/orm/src/client/zod/factory.ts` at line 855, The shared
makeDateTimeValueSchema (used via makeScalarSchema('DateTime')) currently
includes z.iso.date(), which unintentionally permits plain date strings
everywhere; remove z.iso.date() from the union in makeDateTimeValueSchema so the
shared DateTime scalar only accepts Date objects and ISO datetimes (keep
z.date() and z.iso.datetime()), and instead add z.iso.date() only at the
field/filter-specific validation code that handles `@db.Date` filters (i.e., where
field-level or filter-level schemas are created), so date-only strings are
accepted only for `@db.Date` contexts.
|
wait .. I need to do some more cleanups in the tests. |
|
ok no. I think all is well. CodeRabbit appears to be confused because of the change of scope of this PR. not sure how to tell CodeRabbit to deal with that. |
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
♻️ Duplicate comments (1)
packages/orm/src/client/zod/factory.ts (1)
855-867:⚠️ Potential issue | 🟠 MajorKeep plain-date support filter-scoped instead of broadening the shared DateTime scalar.
Line 855 adds
z.iso.date()tomakeDateTimeValueSchema(), but this schema is shared viamakeScalarSchema('DateTime')and is reused outside filters (e.g., create/update/procedure params). That widens accepted inputs beyond filter parsing.Suggested change
private makeDateTimeValueSchema(): ZodType { - const schema = z.union([z.iso.date(), z.iso.datetime(), z.date()]); + const schema = z.union([z.iso.datetime(), z.date()]); this.registerSchema('DateTime', schema); return schema; } +private makeDateTimeFilterValueSchema(): ZodType { + return z.union([z.iso.date(), z.iso.datetime(), z.date()]); +} private makeDateTimeFilterSchema( @@ - const schema = this.makeCommonPrimitiveFilterSchema( - this.makeDateTimeValueSchema(), + const schema = this.makeCommonPrimitiveFilterSchema( + this.makeDateTimeFilterValueSchema(), optional, () => z.lazy(() => this.makeDateTimeFilterSchema(optional, withAggregations, allowedFilterKinds)), withAggregations ? ['_count', '_min', '_max'] : undefined, allowedFilterKinds, );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/orm/src/client/zod/factory.ts` around lines 855 - 867, The shared DateTime scalar (created via makeDateTimeValueSchema and registered in makeScalarSchema('DateTime')) was broadened by adding z.iso.date(), which incorrectly allows plain-date inputs outside filters; remove z.iso.date() from makeDateTimeValueSchema so the shared scalar only accepts the intended datetime/date types, and instead add plain-date support only inside the filter pipeline by adjusting makeDateTimeFilterSchema to accept an additional union that includes z.iso.date() (e.g., compose the filter value schema as z.union([z.iso.date(), this.makeDateTimeValueSchema()]) before passing it into makeCommonPrimitiveFilterSchema) so filters get plain-date support without changing the global DateTime scalar.
🧹 Nitpick comments (1)
tests/e2e/orm/client-api/db-date-filter.test.ts (1)
63-63: Test name saysgt/lt, but onlygtis exercised.Either rename the test to
gtonly or add anltassertion for parity.Possible rename
-it('supports gt/lt filters with plain date strings on a `@db.Date` field', async () => { +it('supports gt filters with plain date strings on a `@db.Date` field', async () => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/orm/client-api/db-date-filter.test.ts` at line 63, The test "supports gt/lt filters with plain date strings on a `@db.Date` field" only exercises the gt case; update the test to match its name by adding an lt assertion (mirror the existing gt assertion using a less-than filter on the same `@db.Date` field) or rename the it(...) description to "supports gt filters with plain date strings on a `@db.Date` field"; locate the it block in db-date-filter.test.ts and either add the corresponding lt filter assertion or adjust the string literal in the it(...) call to reflect only gt.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@packages/orm/src/client/zod/factory.ts`:
- Around line 855-867: The shared DateTime scalar (created via
makeDateTimeValueSchema and registered in makeScalarSchema('DateTime')) was
broadened by adding z.iso.date(), which incorrectly allows plain-date inputs
outside filters; remove z.iso.date() from makeDateTimeValueSchema so the shared
scalar only accepts the intended datetime/date types, and instead add plain-date
support only inside the filter pipeline by adjusting makeDateTimeFilterSchema to
accept an additional union that includes z.iso.date() (e.g., compose the filter
value schema as z.union([z.iso.date(), this.makeDateTimeValueSchema()]) before
passing it into makeCommonPrimitiveFilterSchema) so filters get plain-date
support without changing the global DateTime scalar.
---
Nitpick comments:
In `@tests/e2e/orm/client-api/db-date-filter.test.ts`:
- Line 63: The test "supports gt/lt filters with plain date strings on a
`@db.Date` field" only exercises the gt case; update the test to match its name by
adding an lt assertion (mirror the existing gt assertion using a less-than
filter on the same `@db.Date` field) or rename the it(...) description to
"supports gt filters with plain date strings on a `@db.Date` field"; locate the it
block in db-date-filter.test.ts and either add the corresponding lt filter
assertion or adjust the string literal in the it(...) call to reflect only gt.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 716fb41f-22c6-4d38-bfae-be3a20ef60db
📒 Files selected for processing (2)
packages/orm/src/client/zod/factory.tstests/e2e/orm/client-api/db-date-filter.test.ts
|
I added a follow-up in f168be8 that allows plain Before I leave that in this PR, do you want this extra behavior change here, or would you prefer to keep this PR limited to filter support only and handle |
I think it's fine to include such support for mutation too. Time portion will be zeroed which should be intuitive. |
2 participants
Summary by CodeRabbit
New Features
Tests