fix(policy): logical combination issue and more tests for update #245
Conversation
WalkthroughImplements dialect-aware logical negation and centralized parentheses handling in policy expression utils, updates transformer call sites, and expands/introduces CRUD policy tests for create and update paths. Also adjusts conjunction/disjunction short-circuits and modifies test expectations/policies accordingly. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Pre-merge checks (2 passed, 1 warning)❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
Poem
✨ Finishing Touches
🧪 Generate unit tests
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 Complete ✅Reviewed PR: fix(policy): logical combination issue and more tests for update Todo List:
Review SummaryThis PR implements important fixes to the policy engine's logical combination handling and adds comprehensive test coverage for update operations. The changes are well-structured and address key issues in the expression transformation logic. ✅ Strengths1. Logical Combination Fixes (
2. Expression Transformer Improvements (
3. Comprehensive Test Coverage
4. Test Quality
🔍 Detailed AnalysisLogical Operations Fix (Critical)
Expression Transformer Consistency Test Coverage Enhancement 🚀 Performance Considerations
🔒 Security Assessment
📝 Minor Suggestions
🎯 Overall AssessmentScore: 9/10 This is a high-quality PR that addresses important logical combination issues with proper testing. The fixes are necessary for correct policy evaluation, and the test coverage ensures reliability. The code follows established patterns and maintains consistency with the existing codebase. Recommended Action: ✅ Approve and Merge The changes are safe, well-tested, and fix genuine issues in the policy engine's logical expression handling. |
There was a problem hiding this comment.
Pull Request Overview
This PR fixes logical combination issues in the policy system and adds comprehensive test coverage for update operations. The changes improve the handling of logical operations (AND, OR, NOT) and ensure proper parenthesization in SQL generation.
- Fixes logical combination handling with proper parenthesization and edge case management
- Adds comprehensive test suite for update policy scenarios
- Updates existing test to correct logical condition
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| packages/runtime/test/policy/crud/update.test.ts | New comprehensive test suite covering various update policy scenarios |
| packages/runtime/test/policy/crud/create.test.ts | Adds query builder tests and fixes logical condition in existing test |
| packages/runtime/src/plugins/policy/utils.ts | Improves logical combination functions with edge case handling and parenthesization |
| packages/runtime/src/plugins/policy/expression-transformer.ts | Updates to use improved logicalNot function signature |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/runtime/src/plugins/policy/expression-transformer.ts (1)
233-243: Fix swapped semantics for collection predicates: '!' vs '^'.For quantifiers:
- some ('?'): count(predicate) > 0
- none ('!'): count(predicate) == 0
- every ('^'): count(not predicate) == 0
Current code negates the predicate for '!' instead of '^', effectively swapping 'none' and 'every'. Move the negation to the '^' branch.
Apply:
- if (expr.op === '!') { - predicateFilter = logicalNot(this.dialect, predicateFilter); - } + if (expr.op === '^') { + predicateFilter = logicalNot(this.dialect, predicateFilter); + }
🧹 Nitpick comments (3)
packages/runtime/test/policy/crud/create.test.ts (1)
76-76: Mixed allow/deny rule updated — precedence validated.Allow now includes
x <= 0while deny blocks it; tests correctly keep0and1rejected and2allowed. Consider adding a sibling test that toggles parentheses (e.g.,(x <= 0 || x > 1) && true) to guard against future precedence regressions.packages/runtime/test/policy/crud/update.test.ts (1)
101-122: Auth-gated update path exercised.Good use of
db.$setAuthto flip authorization boundaries.Add tests for collection predicates on update (some/none/every) to catch quantifier-negation regressions (e.g.,
posts ? { x > 0 },posts ! { x > 0 },posts ^ { x > 0 }).packages/runtime/src/plugins/policy/utils.ts (1)
108-111: Nit: add JSDoc to clarify intent of wrapParensIf.Tiny doc improves readability for future maintainers.
Apply:
-function wrapParensIf(node: OperationNode, predicate: (node: OperationNode) => boolean): OperationNode { +/** + * Conditionally wrap the node in ParensNode when predicate(node) is true. + * If the node is already parenthesized, no additional wrapping is added. + */ +function wrapParensIf(node: OperationNode, predicate: (node: OperationNode) => boolean): OperationNode {
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
packages/runtime/src/plugins/policy/expression-transformer.ts(2 hunks)packages/runtime/src/plugins/policy/utils.ts(1 hunks)packages/runtime/test/policy/crud/create.test.ts(2 hunks)packages/runtime/test/policy/crud/update.test.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
{packages,samples,tests}/**
📄 CodeRabbit inference engine (CLAUDE.md)
Place packages only under
packages/,samples/, ortests/
Files:
packages/runtime/test/policy/crud/update.test.tspackages/runtime/src/plugins/policy/expression-transformer.tspackages/runtime/test/policy/crud/create.test.tspackages/runtime/src/plugins/policy/utils.ts
🧬 Code graph analysis (3)
packages/runtime/test/policy/crud/update.test.ts (1)
packages/runtime/test/policy/utils.ts (1)
createPolicyTestClient(14-25)
packages/runtime/src/plugins/policy/expression-transformer.ts (1)
packages/runtime/src/plugins/policy/utils.ts (1)
logicalNot(92-106)
packages/runtime/src/plugins/policy/utils.ts (1)
packages/sdk/src/schema/schema.ts (1)
SchemaDef(10-18)
⏰ 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)
- GitHub Check: claude-review
- GitHub Check: build-test (20.x)
🔇 Additional comments (12)
packages/runtime/src/plugins/policy/expression-transformer.ts (1)
308-309: Good: dialect-aware NOT usage.Passing
this.dialectintologicalNotaligns with the updated utils API and ensures dialect-correct NOT generation.packages/runtime/test/policy/crud/create.test.ts (1)
19-24: Nice: QB insert path covered.Good to assert policy enforcement through the query builder, not just the model API.
packages/runtime/test/policy/crud/update.test.ts (5)
5-28: Solid coverage for scalar-check updates, including QB path.Covers both model API and QB
.updateTable()with row-count andreturningAll()assertions.If CI runs across multiple dialects, confirm
returningAll()is supported or polyfilled for all of them; otherwise, gate or adjust expectations for dialects lacking RETURNING.
30-46: 'this' member check path covered.Validates policy evaluation against instance values during update.
48-61: Default deny for update verified.Good negative test to prevent permissive regressions.
63-78: Deny rule precedence confirmed for update.Ensures
@@deny('update', x <= 0)wins even with broad allow.
80-99: Mixed allow/deny update semantics validated.Selective success/failure across rows is well asserted.
packages/runtime/src/plugins/policy/utils.ts (5)
53-58: Good short-circuiting for conjunction base cases.Avoids reduce on empty arrays and returns identity/singleton correctly.
66-66: Correct precedence-preserving AND reduction.Wrapping OR operands with Parens when combining via AND preserves intended grouping. Symmetric to OR-path below. LGTM.
73-78: Good short-circuiting for disjunction base cases.Identity and singleton handling look correct and avoid unnecessary work.
86-86: Correct precedence-preserving OR reduction.Wrapping AND operands with Parens under OR is a safe, dialect-agnostic way to retain grouping.
92-106: Manual verification oflogicalNotcall sites
Automated checks were inconclusive; ensure every invocation now passes bothdialectandnode.
Summary by CodeRabbit
Bug Fixes
Tests
Refactor