Conversation
- Fill now() default in evalGenerator so createdAt fields are populated before policy checks, preventing DefaultInsertValueNode from being treated as null during pre-create policy evaluation. - Fix now() SQL function to produce ISO 8601 format matching each dialect's DateTime storage format (SQLite: strftime, MySQL: DATE_FORMAT with trimmed microseconds), ensuring correct comparisons in policy expressions. - Add e2e tests for now() in create, read, update, and delete policies. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
Co-authored-by: ymc9 <104139426+ymc9@users.noreply.github.com>
…optional dependencies (#2476)
Contributor
|
Claude finished @ymc9's task —— View job PR Review: merge dev to main (v3.4.6)
|
Contributor
📝 WalkthroughWalkthroughThis PR bumps the monorepo version to 3.4.6 across all packages, enhances ORM JSON filtering to handle undefined and null values explicitly, introduces dialect-specific Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/orm/src/client/crud/operations/base.ts (1)
1085-1109:⚠️ Potential issue | 🟠 MajorUse one clock source for
@default(now())and policynow().This new branch stamps defaults with
new Date(), but policynow()inpackages/orm/src/client/functions.ts:112-124is evaluated in SQL. On deployments where the app host and database clocks drift, rules like@@allow('create', createdAt <= now())can reject freshly created rows intermittently. Please route both paths through the same database-side timestamp source.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/orm/src/client/crud/operations/base.ts` around lines 1085 - 1109, The evalGenerator currently returns a JS Date for the 'now' default which mismatches the policy now() (evaluated in SQL); update the 'now' branch inside evalGenerator in base.ts to produce the same DB-side timestamp expression used by the policy (use the same SQL-now helper/function used by packages/orm/src/client/functions.ts: now()), and pass that SQL-now expression into formatGeneratedValue instead of new Date() so both `@default`(now()) and policy now() use the identical database clock source.
🧹 Nitpick comments (3)
tests/regression/test/issue-2472.test.ts (1)
16-16: Disconnect the regression test clients.Both cases allocate a fresh client and never close it. That can leave open handles on PostgreSQL/MySQL runs; adding
afterEachortry/finallycleanup keeps this consistent with the other test suites.Also applies to: 35-41
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/regression/test/issue-2472.test.ts` at line 16, The test allocates database clients via createTestClient but never closes them; modify the test to ensure each created client is disconnected after use by either wrapping uses of createTestClient in a try/finally that calls client.disconnect() (or the appropriate close method) or by storing the client in a variable and adding an afterEach hook that disconnects any non-null client; update both places where createTestClient is called (the occurrence around the earlier call and the second block covering lines ~35-41) and reference the created client variable name so the disconnect/cleanup targets the same client instance.tests/e2e/orm/policy/now-function.test.ts (1)
6-16: Disconnect each policy test client.Every case creates a new policy client but none is closed. Please add
afterEachortry/finallycleanup so this suite doesn't leak connections across provider runs.Also applies to: 25-35, 51-61, 77-87, 101-116
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/orm/policy/now-function.test.ts` around lines 6 - 16, The policy tests create DB clients via createPolicyTestClient but never close them; add cleanup to avoid leaked connections by either (A) storing the returned client in a variable and wrapping each test body in try/finally where you call client.disconnect() in finally, or (B) push each created client into an array and add an afterEach hook that iterates and calls disconnect() (or close() if different) on each client; update all usages of createPolicyTestClient in this file (the blocks that create Post models) to follow one of these patterns so clients are reliably disconnected after each test.packages/orm/src/client/functions.ts (1)
112-124: Avoidsql.rawin the sharednow()helper.These branches bake vendor SQL strings directly into the ORM layer. Prefer Kysely builders or push the provider-specific expression down into the dialects so the SQL stays behind the dialect abstraction.
As per coding guidelines,
**/*.{ts,tsx}: Use Kysely query builder as escape hatch instead of raw SQL in ORM operations.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/orm/src/client/functions.ts` around lines 112 - 124, The now ZModelFunction currently embeds vendor SQL via sql.raw in the 'sqlite', 'mysql', and 'postgresql' match branches; replace this by either using Kysely expression builders or delegating provider-specific SQL generation into the dialect layer (i.e., move the sqlite/mysql/postgresql expressions out of packages/orm/src/client/functions.ts and call a dialect method like dialect.nowExpression() or use Kysely's function/expression helpers instead of sql.raw) so the ORM layer no longer contains raw SQL strings and each branch (sqlite, mysql, postgresql) returns a Kysely-compatible expression or calls the dialect API.
🤖 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/crud/dialects/base-dialect.ts`:
- Around line 759-775: The MySQL override of buildJsonEqualityFilter still emits
JSON_CONTAINS for all cases and must honor the null/undefined fast-path; update
the mysql implementation of buildJsonEqualityFilter (the override in the MySQL
dialect) to reuse the base logic by either calling this.buildValueFilter(lhs,
'Json', rhs) or delegating to super.buildJsonEqualityFilter(lhs, rhs) so that
rhs === undefined returns this.true(), rhs === null returns this.eb(lhs, 'is',
null), and only otherwise emits the JSON_CONTAINS/equals expression; ensure you
reference the existing buildValueFilter/transformInput/eb helpers so behavior
matches the base-dialect fast-path.
---
Outside diff comments:
In `@packages/orm/src/client/crud/operations/base.ts`:
- Around line 1085-1109: The evalGenerator currently returns a JS Date for the
'now' default which mismatches the policy now() (evaluated in SQL); update the
'now' branch inside evalGenerator in base.ts to produce the same DB-side
timestamp expression used by the policy (use the same SQL-now helper/function
used by packages/orm/src/client/functions.ts: now()), and pass that SQL-now
expression into formatGeneratedValue instead of new Date() so both
`@default`(now()) and policy now() use the identical database clock source.
---
Nitpick comments:
In `@packages/orm/src/client/functions.ts`:
- Around line 112-124: The now ZModelFunction currently embeds vendor SQL via
sql.raw in the 'sqlite', 'mysql', and 'postgresql' match branches; replace this
by either using Kysely expression builders or delegating provider-specific SQL
generation into the dialect layer (i.e., move the sqlite/mysql/postgresql
expressions out of packages/orm/src/client/functions.ts and call a dialect
method like dialect.nowExpression() or use Kysely's function/expression helpers
instead of sql.raw) so the ORM layer no longer contains raw SQL strings and each
branch (sqlite, mysql, postgresql) returns a Kysely-compatible expression or
calls the dialect API.
In `@tests/e2e/orm/policy/now-function.test.ts`:
- Around line 6-16: The policy tests create DB clients via
createPolicyTestClient but never close them; add cleanup to avoid leaked
connections by either (A) storing the returned client in a variable and wrapping
each test body in try/finally where you call client.disconnect() in finally, or
(B) push each created client into an array and add an afterEach hook that
iterates and calls disconnect() (or close() if different) on each client; update
all usages of createPolicyTestClient in this file (the blocks that create Post
models) to follow one of these patterns so clients are reliably disconnected
after each test.
In `@tests/regression/test/issue-2472.test.ts`:
- Line 16: The test allocates database clients via createTestClient but never
closes them; modify the test to ensure each created client is disconnected after
use by either wrapping uses of createTestClient in a try/finally that calls
client.disconnect() (or the appropriate close method) or by storing the client
in a variable and adding an afterEach hook that disconnects any non-null client;
update both places where createTestClient is called (the occurrence around the
earlier call and the second block covering lines ~35-41) and reference the
created client variable name so the disconnect/cleanup targets the same client
instance.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d948b428-0836-4f6c-8fc0-1a86a04d85c8
📒 Files selected for processing (31)
package.jsonpackages/auth-adapters/better-auth/package.jsonpackages/cli/package.jsonpackages/clients/client-helpers/package.jsonpackages/clients/tanstack-query/package.jsonpackages/common-helpers/package.jsonpackages/config/eslint-config/package.jsonpackages/config/typescript-config/package.jsonpackages/config/vitest-config/package.jsonpackages/create-zenstack/package.jsonpackages/ide/vscode/package.jsonpackages/language/package.jsonpackages/orm/package.jsonpackages/orm/src/client/crud/dialects/base-dialect.tspackages/orm/src/client/crud/dialects/postgresql.tspackages/orm/src/client/crud/operations/base.tspackages/orm/src/client/functions.tspackages/plugins/policy/package.jsonpackages/schema/package.jsonpackages/sdk/package.jsonpackages/server/package.jsonpackages/testtools/package.jsonpackages/zod/package.jsonsamples/orm/package.jsontests/e2e/orm/client-api/filter.test.tstests/e2e/orm/policy/now-function.test.tstests/e2e/package.jsontests/regression/package.jsontests/regression/test/issue-2472.test.tstests/runtimes/bun/package.jsontests/runtimes/edge-runtime/package.json
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
Bug Fixes
Tests
Version: 3.4.6