fix(policy): relation access via this, more test cases
#252
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. WalkthroughUpdates nested-relation update logic to prefer Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ 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). (1)
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. 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 for Policy Relation Access via
|
There was a problem hiding this comment.
Pull Request Overview
This PR fixes policy relation access via this keyword and adds comprehensive test coverage for read policy scenarios. The main purpose is to enable proper policy evaluation when using this.relation syntax and ensure relation-based policies work correctly across different relationship types.
Key changes:
- Enhanced expression transformer to handle
this.relationaccess patterns properly - Added comprehensive test suite covering various relation access scenarios in read policies
- Improved binary operation handling for relation field comparisons
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/read.test.ts | New comprehensive test suite for read policy scenarios with relation access |
| packages/runtime/src/plugins/policy/expression-transformer.ts | Enhanced expression transformation to support this.relation access and relation field comparisons |
| packages/runtime/src/plugins/policy/policy-handler.ts | Added comment clarifying id field bypass behavior |
| packages/runtime/src/client/crud/operations/base.ts | Refactored entity lookup to use dedicated getEntityIds method |
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
🧹 Nitpick comments (2)
packages/runtime/src/client/crud/operations/base.ts (1)
1012-1012: Consider the implications of bypassingreadUniquewhen ID fields are complete.The change from
readUniquetogetEntityIdsintroduces a subtle behavior difference. When all ID fields are present in the filter,getEntityIdsreturns the filter directly without validating that the entity exists. This defers the existence check to later operations, which could potentially lead to different error timing or messaging.While this optimization avoids an unnecessary database query, it changes when
NotFoundErroris thrown - from this point to potentially later in the update flow.Consider adding a comment explaining this optimization and its implications:
} if (!thisEntity) { + // getEntityIds returns the filter directly if all ID fields are present, + // otherwise fetches the entity. Existence validation may be deferred. thisEntity = await this.getEntityIds(kysely, model, combinedWhere); if (!thisEntity) {packages/runtime/src/plugins/policy/expression-transformer.ts (1)
198-212: Add null-safety checks for ID field access.The normalization logic assumes that
idFields[0]exists. While most models have at least one ID field, it would be safer to handle edge cases.Consider adding validation:
private normalizeBinaryOperationOperands(expr: BinaryExpression, context: ExpressionTransformerContext<Schema>) { let normalizedLeft: Expression = expr.left; if (this.isRelationField(expr.left, context.model)) { invariant(ExpressionUtils.isNull(expr.right)); const idFields = getIdFields(this.schema, context.model); + invariant(idFields.length > 0, 'Model must have at least one ID field'); normalizedLeft = this.makeOrAppendMember(normalizedLeft, idFields[0]!); } let normalizedRight: Expression = expr.right; if (this.isRelationField(expr.right, context.model)) { invariant(ExpressionUtils.isNull(expr.left)); const idFields = getIdFields(this.schema, context.model); + invariant(idFields.length > 0, 'Model must have at least one ID field'); normalizedRight = this.makeOrAppendMember(normalizedRight, idFields[0]!); } return { normalizedLeft, normalizedRight }; }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
packages/runtime/src/client/crud/operations/base.ts(1 hunks)packages/runtime/src/plugins/policy/expression-transformer.ts(6 hunks)packages/runtime/src/plugins/policy/policy-handler.ts(1 hunks)packages/runtime/test/policy/crud/read.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/src/plugins/policy/policy-handler.tspackages/runtime/src/plugins/policy/expression-transformer.tspackages/runtime/src/client/crud/operations/base.tspackages/runtime/test/policy/crud/read.test.ts
🧬 Code graph analysis (2)
packages/runtime/src/plugins/policy/expression-transformer.ts (4)
packages/sdk/src/schema/expression.ts (2)
BinaryExpression(39-44)Expression(1-10)packages/runtime/src/schema/expression.ts (1)
ExpressionUtils(16-120)packages/runtime/src/client/query-utils.ts (2)
getIdFields(57-60)requireField(37-55)packages/sdk/src/schema/schema.ts (2)
GetModels(108-108)FieldDef(59-74)
packages/runtime/test/policy/crud/read.test.ts (1)
packages/runtime/test/policy/utils.ts (1)
createPolicyTestClient(14-25)
⏰ 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). (1)
- GitHub Check: build-test (20.x)
🔇 Additional comments (9)
packages/runtime/src/plugins/policy/policy-handler.ts (1)
105-105: LGTM!The added comment clearly documents the behavior that ID-only read backs bypass policy enforcement, which is an important security consideration for developers to understand.
packages/runtime/src/plugins/policy/expression-transformer.ts (5)
28-28: Verify that all imported utilities are used.The addition of
getIdFieldsimport is appropriate for the new relation field normalization logic.
164-166: Good abstraction for operand normalization.The introduction of
normalizeBinaryOperationOperandsproperly handles the special case where relation fields in binary operations need to be transformed to their ID fields for comparison.
230-245: Excellent refactoring of collection predicate handling.The use of
getFieldDefFromFieldRefcentralizes field resolution logic and properly handles both direct field references andthis.fieldpatterns. The subsequent traversal logic for member chains is clear and well-structured.
419-425: Clean handling ofthis.relationaccess pattern.The special case for single-member
this.expressions correctly delegates to the field handler, avoiding code duplication and ensuring consistent behavior.
581-598: Well-designed helper functions for relation field detection.The
isRelationFieldandgetFieldDefFromFieldRefhelpers provide a clean abstraction for determining whether an expression refers to a relation field. The implementation correctly handles bothFieldExpressionandthis.fieldmember access patterns.packages/runtime/test/policy/crud/read.test.ts (3)
1-249: Comprehensive test coverage for read policies.The test suite provides excellent coverage of various read policy scenarios including:
- Top-level filtering with field conditions
- To-one and to-many relation filtering
- Owner vs non-owner side relation access
- Null checks on relations
- Collection predicates with
this.accessThe tests effectively validate the expression transformation enhancements made in this PR.
71-73: Good test design noting potential behavior change.The TODO comment correctly identifies a potential consistency issue with v2 where non-optional child relations that are not readable might need different handling. This is valuable for tracking potential breaking changes.
194-219: Thorough validation of relation null checks.These tests effectively validate that both
bar != nullandthis.bar != nullpatterns work correctly, ensuring the expression transformation properly handles relation existence checks.
|
Claude finished @ymc9's task —— View job Code Review: Fix for Policy Relation Access via
|
Summary by CodeRabbit
Bug Fixes
Refactor
Tests
Documentation