fix(orm): operator "!" should only be applicable to boolean fields #511
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdded UnaryExpr validation to require Boolean operands, centralized DB query error handling via an internal wrapper in the query executor, and added/updated regression tests exercising the unary validation and minor test description fixes. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (5)
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (2)
🧰 Additional context used🧠 Learnings (2)📚 Learning: 2025-11-26T01:55:04.540ZApplied to files:
📚 Learning: 2025-11-26T01:55:04.540ZApplied to files:
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
🔇 Additional comments (1)
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 |
|
Claude finished @ymc9's task —— View job Code Review: fix(orm): operator "!" should only be applicable to boolean fieldsReviewing PR #511 that fixes issue #510 regarding the "!" operator restriction to boolean fields. Review Tasks:
AnalysisProblem: The issue was that the "!" operator could be used on non-boolean fields, which should not be allowed. The error occurred in the ZModel schema on line 96: Solution Overview: The fix adds proper validation for unary expressions in the expression validator to ensure the "!" operator is only applied to boolean fields. Code Changes Review1. Expression Validator (
2. Query Executor (
3. Test Coverage (
Feedback & RecommendationsStrengths:
Minor Considerations:
Security & Performance Assessment
Code Quality Assessment
Overall Assessment: This is a well-implemented fix that properly addresses the reported issue. The changes are minimal, focused, and follow good engineering practices. The validation logic is sound, the error handling improvements are beneficial, and the test coverage ensures the fix works correctly. Recommendation: ✅ Approve - This PR is ready to merge. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/orm/src/client/executor/zenstack-query-executor.ts (1)
201-201: Good refactoring, but appears unrelated to the PR's main objective.The changes centralize error handling through the new
internalExecuteQuerywrapper, providing consistent error formatting with SQL context. The implementation correctly avoids double-wrapping by checking forORMErrorinstances in the outer catch block (lines 114-116).However, this refactoring seems tangential to the PR's stated objective of fixing the "!" operator validation. While it's a reasonable improvement, consider whether it should be in a separate PR for clarity.
Also applies to: 249-249, 473-473
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/language/src/validators/expression-validator.ts(3 hunks)packages/orm/src/client/executor/zenstack-query-executor.ts(4 hunks)tests/regression/test/issue-510.test.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (6)
📚 Learning: 2025-11-26T01:55:04.540Z
Learnt from: CR
Repo: zenstackhq/zenstack-v3 PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T01:55:04.540Z
Learning: Applies to tests/e2e/**/*.{ts,tsx} : E2E tests should validate real-world schema compatibility with established projects
Applied to files:
tests/regression/test/issue-510.test.ts
📚 Learning: 2025-11-26T01:55:04.540Z
Learnt from: CR
Repo: zenstackhq/zenstack-v3 PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T01:55:04.540Z
Learning: Applies to packages/zenstackhq/orm/**/*.test.{ts,tsx} : ORM package tests should include comprehensive client API tests and policy tests
Applied to files:
tests/regression/test/issue-510.test.tspackages/orm/src/client/executor/zenstack-query-executor.ts
📚 Learning: 2025-11-26T01:55:04.540Z
Learnt from: CR
Repo: zenstackhq/zenstack-v3 PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T01:55:04.540Z
Learning: Applies to tests/**/type*.{ts,tsx} : Ensure TypeScript inference and type coverage are validated through type coverage tests
Applied to files:
tests/regression/test/issue-510.test.ts
📚 Learning: 2025-11-26T01:55:04.540Z
Learnt from: CR
Repo: zenstackhq/zenstack-v3 PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T01:55:04.540Z
Learning: Applies to **/*.zmodel : ZModel schema files should define database structure and policies that compile to TypeScript via `zenstack generate`
Applied to files:
tests/regression/test/issue-510.test.ts
📚 Learning: 2025-11-26T01:55:04.540Z
Learnt from: CR
Repo: zenstackhq/zenstack-v3 PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T01:55:04.540Z
Learning: Applies to packages/zenstackhq/orm/**/*.{ts,tsx} : Use Kysely as the query builder interface for low-level database queries, avoiding raw SQL when possible
Applied to files:
packages/orm/src/client/executor/zenstack-query-executor.ts
📚 Learning: 2025-11-26T01:55:04.540Z
Learnt from: CR
Repo: zenstackhq/zenstack-v3 PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T01:55:04.540Z
Learning: Applies to packages/zenstackhq/orm/**/*.{ts,tsx} : Implement plugin hooks at ORM, Kysely, and entity mutation levels for query interception and customization
Applied to files:
packages/orm/src/client/executor/zenstack-query-executor.ts
🧬 Code graph analysis (3)
tests/regression/test/issue-510.test.ts (1)
packages/testtools/src/client.ts (1)
createPolicyTestClient(258-269)
packages/orm/src/client/executor/zenstack-query-executor.ts (2)
packages/orm/src/client/executor/zenstack-driver.ts (2)
connection(129-170)compiledQuery(181-188)packages/orm/src/client/errors.ts (1)
createDBQueryError(122-129)
packages/language/src/validators/expression-validator.ts (1)
packages/language/src/generated/ast.ts (2)
UnaryExpr(741-746)UnaryExpr(748-748)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Agent
- GitHub Check: build-test (20.x, postgresql)
- GitHub Check: claude-review
- GitHub Check: build-test (20.x, sqlite)
🔇 Additional comments (6)
packages/language/src/validators/expression-validator.ts (3)
15-15: LGTM! Import added correctly.The
UnaryExprimport is necessary for the new validation logic and is properly placed with other AST type imports.
71-73: LGTM! Switch case properly integrated.The new case for
UnaryExprfollows the established pattern and correctly delegates to the validation method.
252-256: LGTM! Validation correctly restricts "!" to boolean operands.The implementation correctly enforces that the operand of the "!" operator must be of Boolean type, directly addressing the PR objective. The error message is clear and matches the test expectation.
tests/regression/test/issue-510.test.ts (2)
1-99: LGTM! Comprehensive regression test with realistic schema.The test correctly sets up a scenario that triggers the validation error. Line 96 contains the key test case where
!Imageincorrectly applies the "!" operator to a relation field (File?) rather than a boolean, which should be caught by the new validation.
101-101: LGTM! Test assertion validates the fix correctly.The assertion properly verifies that the schema with an invalid unary expression is rejected with the expected error message, matching the validation added in
expression-validator.ts.packages/orm/src/client/executor/zenstack-query-executor.ts (1)
487-493: LGTM! Centralized error handling with helpful context.The wrapper method correctly catches query execution errors and enriches them with SQL and parameter information, making debugging easier. The implementation properly delegates to
connection.executeQueryand wraps any errors usingcreateDBQueryError.
There was a problem hiding this comment.
Pull request overview
This PR fixes issue #510 by adding validation to ensure the unary "!" operator can only be applied to boolean fields. Previously, the "!" operator could be incorrectly used on non-boolean types (such as relation fields) without a validation error. The fix adds a validateUnaryExpr method to the expression validator that checks if the operand is of Boolean type.
Key changes:
- Added validation for UnaryExpr to ensure "!" operator only accepts Boolean operands
- Added a regression test that verifies the fix with a schema containing
!Imagewhere Image is a File? relation - Refactored query execution error handling by introducing
internalExecuteQuerywrapper method
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| tests/regression/test/issue-510.test.ts | Adds regression test verifying that using "!" on a non-boolean field (Image relation) throws appropriate error |
| packages/language/src/validators/expression-validator.ts | Implements validateUnaryExpr method to check that "!" operator operands are Boolean type |
| packages/orm/src/client/executor/zenstack-query-executor.ts | Refactors query execution to use internalExecuteQuery wrapper for consistent error handling |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Claude finished @ymc9's task —— View job Code Review: fix(orm): operator "!" should only be applicable to boolean fieldsReviewing PR #511 that fixes issue #510 regarding the "!" operator restriction to boolean fields. Review Tasks:
AnalysisProblem: The issue was that the "!" operator could be used on non-boolean fields, which should not be allowed. The error occurred in the ZModel schema on line 96: Solution Overview: The fix adds proper validation for unary expressions in the expression validator to ensure the "!" operator is only applied to boolean fields. Code Changes Review1. Expression Validator (
2. Query Executor (
3. Test Coverage (
Feedback & RecommendationsStrengths:
Minor Considerations:
Security & Performance Assessment
Code Quality Assessment
Overall Assessment: This is a well-implemented fix that properly addresses the reported issue. The changes are minimal, focused, and follow good engineering practices. The validation logic is sound, the error handling improvements are beneficial, and the test coverage ensures the fix works correctly. Recommendation: ✅ Approve - This PR is ready to merge. |
fixes #510
Summary by CodeRabbit
Bug Fixes
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.