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 1 commit intozenstackhq:devfrom
Conversation
📝 WalkthroughWalkthroughUpdated Zod DateTime value schema 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? |
|
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.
| it('plain date string is accepted on a regular DateTime field (no @db.Date)', async () => { | ||
| // Plain date strings are accepted for all DateTime fields, not just @db.Date ones, | ||
| // to decouple filter validation from migration-engine attributes. | ||
| const filterSchema = client.$zod.makeFindManySchema('Event'); | ||
| const result = filterSchema.safeParse({ where: { createdAt: '2007-05-23' } }); | ||
| expect(result.success).toBe(true); | ||
| }); |
There was a problem hiding this comment.
This test expectation conflicts with the stated non-@db.Date behavior.
The PR objective says regular DateTime fields should still reject plain YYYY-MM-DD strings, but this test asserts acceptance. That locks in the broader behavior and masks regressions against the intended contract.
Suggested test correction
- const result = filterSchema.safeParse({ where: { createdAt: '2007-05-23' } });
- expect(result.success).toBe(true);
+ const result = filterSchema.safeParse({ where: { createdAt: '2007-05-23' } });
+ expect(result.success).toBe(false);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| it('plain date string is accepted on a regular DateTime field (no @db.Date)', async () => { | |
| // Plain date strings are accepted for all DateTime fields, not just @db.Date ones, | |
| // to decouple filter validation from migration-engine attributes. | |
| const filterSchema = client.$zod.makeFindManySchema('Event'); | |
| const result = filterSchema.safeParse({ where: { createdAt: '2007-05-23' } }); | |
| expect(result.success).toBe(true); | |
| }); | |
| it('plain date string is accepted on a regular DateTime field (no `@db.Date`)', async () => { | |
| // Plain date strings are accepted for all DateTime fields, not just `@db.Date` ones, | |
| // to decouple filter validation from migration-engine attributes. | |
| const filterSchema = client.$zod.makeFindManySchema('Event'); | |
| const result = filterSchema.safeParse({ where: { createdAt: '2007-05-23' } }); | |
| expect(result.success).toBe(false); | |
| }); |
🤖 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 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.
|
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. |
2 participants
Summary by CodeRabbit
New Features
Tests