Skip to content

fix(policy): support now() default value in access policy evaluation#2465

Merged
ymc9 merged 1 commit intodevfrom
fix/now-default-policy-support
Mar 9, 2026
Merged

fix(policy): support now() default value in access policy evaluation#2465
ymc9 merged 1 commit intodevfrom
fix/now-default-policy-support

Conversation

@ymc9
Copy link
Member

@ymc9 ymc9 commented Mar 9, 2026

Summary

  • Fill now() default in evalGenerator so @default(now()) 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.

Test plan

  • e2e test: create with @default(now()) field used in policy rule
  • e2e test: update policy comparing DateTime field against now()
  • e2e test: read policy filtering by publishedAt <= now()
  • e2e test: delete policy filtering by expiresAt < now()
  • e2e test: now() default combined with auth() check in create policy

🤖 Generated with Claude Code

Summary by CodeRabbit

  • New Features

    • Added now() as a default value generator for automatic timestamp population in models.
    • Expanded now() function with native dialect support for SQLite, MySQL, and PostgreSQL.
  • Tests

    • Added comprehensive test coverage for now() functionality across CRUD operations and access policies.

- 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 <[email protected]>
@claude
Copy link
Contributor

claude bot commented Mar 9, 2026

Claude finished @ymc9's task —— View job


Code Review Complete ✅

I've thoroughly reviewed this pull request that adds support for now() default values in access policy evaluation. Here's my detailed analysis:

Review Tasks

  • Analyze changes in packages/orm/src/client/crud/operations/base.ts
  • Review changes in packages/orm/src/client/functions.ts
  • Examine new test file tests/e2e/orm/policy/now-function.test.ts
  • Provide overall assessment and recommendations

Overall Assessment: Excellent Implementation

This PR effectively solves the core issue where @default(now()) fields were being treated as null during policy evaluation. The implementation is solid, well-tested, and follows good practices.

Code Quality & Best Practices

packages/orm/src/client/crud/operations/base.ts (+1 line)

  • ✅ Minimal, targeted change: Adding .with('now', () => new Date()) to the pattern match is exactly the right approach
  • ✅ Consistent with existing patterns: Follows the same structure as cuid, ulid generators
  • ✅ Type safety: Returns a JavaScript Date object that will be properly handled by the dialect transformation pipeline

packages/orm/src/client/functions.ts (+13 lines)

  • ✅ Excellent dialect-specific handling: Each database gets the appropriate SQL function
  • ✅ Well-documented rationale: Comments clearly explain why each dialect needs specific formatting
  • ✅ ISO 8601 compliance: Ensures consistent DateTime format across databases
  • ✅ Robust SQL generation: Uses Kysely's sql.raw() appropriately

Specific dialect implementations are well-researched:

  • SQLite: strftime('%Y-%m-%dT%H:%M:%fZ') produces correct ISO format
  • MySQL: Complex but correct concatenation to match 3-digit millisecond format
  • PostgreSQL: Simple CURRENT_TIMESTAMP leverages native support

Security Considerations

  • ✅ No SQL injection risks: Uses Kysely's sql.raw() with static strings only
  • ✅ No user input in SQL generation: All SQL is statically defined
  • ✅ Timezone handling: MySQL and SQLite explicitly use UTC, PostgreSQL uses server timezone (consistent with existing behavior)

Performance Considerations

  • ✅ Minimal overhead: new Date() evaluation only happens during default value generation
  • ✅ Database-native functions: Uses each DB's most efficient timestamp function
  • ✅ No unnecessary transformations: Direct SQL generation without extra processing

Test Coverage

Outstanding test coverage with comprehensive scenarios:

  1. ✅ Create policy with @default(now()) - Tests the core fix
  2. ✅ Update policy with now() comparison - Future vs past events
  3. ✅ Read policy filtering - Published vs scheduled articles
  4. ✅ Delete policy - Expired vs active tasks
  5. ✅ Combined auth + now() - Real-world authentication scenario

Test quality is excellent:

  • Uses realistic time differences (1 hour) for reliable testing
  • Properly tests both positive and negative cases
  • Includes edge case with authentication combination
  • Clear, descriptive test names and assertions

Potential Issues & Recommendations

⚠️ Minor: Timezone Documentation

The MySQL implementation uses UTC_TIMESTAMP(3) which is correct, but consider adding a comment about timezone behavior differences between databases in the policy documentation.

💡 Enhancement Opportunity

Consider adding a test case that verifies the exact ISO format string matching between @default(now()) and now() function calls to ensure they're truly equivalent across all dialects.

No Breaking Changes

This change is fully backward compatible and only enhances existing functionality.


Summary

This is a high-quality fix that solves a legitimate bug in policy evaluation. The implementation is:

  • Correct: Properly handles the core issue
  • Robust: Works across all supported database dialects
  • Well-tested: Comprehensive test coverage
  • Maintainable: Clear, documented code
  • Secure: No security vulnerabilities introduced

Recommendation: ✅ APPROVE

The code quality is excellent and this fix will prevent policy evaluation issues when using @default(now()) fields in access rules.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Mar 9, 2026

📝 Walkthrough

Walkthrough

The pull request adds dialect-aware support for a now() default value generator. A "now" generator is wired into the base operation handler, and the now() function implementation is expanded to emit dialect-specific SQL syntax for SQLite, MySQL, and PostgreSQL. A comprehensive test suite validates the new functionality across create, update, read, and delete operations with policy constraints.

Changes

Cohort / File(s) Summary
Default Value Generator
packages/orm/src/client/crud/operations/base.ts
Added "now" generator to the default value generator dispatcher, enabling now() expressions in default value generation. Single-line addition.
Dialect-Aware Function Implementation
packages/orm/src/client/functions.ts
Expanded now() function from a simple constant to dialect-aware implementation. SQLite uses strftime() with ISO format and Z suffix, MySQL builds concatenated UTC_TIMESTAMP(3) substrings with +00:00 offset, PostgreSQL retains native CURRENT_TIMESTAMP.
Policy Function Tests
tests/e2e/orm/policy/now-function.test.ts
Introduced comprehensive test suite validating now() across CRUD operations with policy constraints. Covers default population on create, usage in update/read/delete policies, and combinations with auth constraints. Tests both policy-allowed and policy-rejected scenarios.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~22 minutes

Poem

🐰 Hop, hop, the time flows free,
Now() speaks in dialects three!
SQLite strftime dances bright,
MySQL concatenates with might,
While Postgres keeps its way so right! ✨

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately captures the main change: adding support for now() default values in access policy evaluation, which is the core objective of the PR.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
  • 📝 Generate docstrings (stacked PR)
  • 📝 Generate docstrings (commit on current branch)
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch fix/now-default-policy-support

Tip

Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs).
Share your feedback on Discord.


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
tests/e2e/orm/policy/now-function.test.ts (1)

18-19: Minor: Comment doesn't match the actual policy.

The comment references "satisfying the <= now() check" but the policy on line 12 is createdAt != null. Consider updating the comment to accurately describe what's being tested.

-        // createdAt should be auto-filled with now(), satisfying the <= now() check
+        // createdAt should be auto-filled with now(), satisfying the != null check
🤖 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 18 - 19, The comment
above the test is inaccurate: it mentions "satisfying the <= now() check" but
the policy being tested is `createdAt != null`; update the comment to accurately
describe the assertion (e.g., "createdAt should be auto-filled and not null")
and ensure it references the actual policy semantics being enforced by
db.post.create in this test.
🤖 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/policy/now-function.test.ts`:
- Around line 18-19: The comment above the test is inaccurate: it mentions
"satisfying the <= now() check" but the policy being tested is `createdAt !=
null`; update the comment to accurately describe the assertion (e.g., "createdAt
should be auto-filled and not null") and ensure it references the actual policy
semantics being enforced by db.post.create in this test.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 4dd8df9a-95aa-4603-9bf2-227d5913f7b5

📥 Commits

Reviewing files that changed from the base of the PR and between 5aaef62 and 930d996.

📒 Files selected for processing (3)
  • packages/orm/src/client/crud/operations/base.ts
  • packages/orm/src/client/functions.ts
  • tests/e2e/orm/policy/now-function.test.ts

@ymc9 ymc9 merged commit d3ab3a6 into dev Mar 9, 2026
9 checks passed
@ymc9 ymc9 deleted the fix/now-default-policy-support branch March 9, 2026 15:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant