feat: audit policy collection aliases #548
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. 📝 WalkthroughWalkthroughAdds optional iterator bindings for collection-predicate binary expressions and threads that binding through grammar, AST types, scope/linking, code generation, evaluation, transformation, SDK generators, and tests. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
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}
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/language/src/zmodel-scope.ts (1)
211-241: Reorder match cases to checkisAuthInvocationbeforeisInvocationExpr.The
isAuthInvocationcheck (line 237) is unreachable becauseisAuthInvocationis defined asisInvocationExpr(node) && node.function.ref?.name === 'auth' && isFromStdlib(node.function.ref)(utils.ts line 69). Since auth() invocations matchisInvocationExprfirst, they are handled by the generic invocation branch (lines 229-236), which attempts to accessexpr.function.ref?.returnTypethat may not exist for auth(). Move theisAuthInvocationcheck beforeisInvocationExprto ensure auth() is routed to the correct handler.
🧹 Nitpick comments (2)
packages/language/test/expression-validation.test.ts (1)
102-144: Good test coverage for the new binding syntax.Both tests appropriately validate the new iterator binding syntax and backward compatibility with the existing unbound syntax. The schema structures are valid with proper model relationships.
Consider adding negative test cases in a follow-up to validate error handling when:
- The binding variable shadows an existing field name
- The binding is used outside its scope (e.g., outside the collection predicate where it's defined)
packages/language/src/validators/attribute-application-validator.ts (1)
494-503: Improved defensive extraction of field types.The refactoring adds proper guards for
isReferenceExprand safely handles missing or non-string names, which aligns with the broader AST changes that now includeBinaryExpras a possibleReferenceTarget.Consider using a more explicit type check instead of
(ref as any).namefor better type safety:🔎 Suggested improvement
const ref = item.target.ref; - return ref && 'name' in ref ? (ref as any).name : undefined; + return ref && 'name' in ref && typeof (ref as { name?: unknown }).name === 'string' + ? (ref as { name: string }).name + : undefined;
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
packages/language/src/generated/ast.tsis excluded by!**/generated/**packages/language/src/generated/grammar.tsis excluded by!**/generated/**
📒 Files selected for processing (12)
packages/language/src/ast.tspackages/language/src/validators/attribute-application-validator.tspackages/language/src/zmodel-code-generator.tspackages/language/src/zmodel-linker.tspackages/language/src/zmodel-scope.tspackages/language/src/zmodel.langiumpackages/language/test/expression-validation.test.tspackages/plugins/policy/src/expression-evaluator.tspackages/plugins/policy/src/expression-transformer.tspackages/schema/src/expression-utils.tspackages/schema/src/expression.tspackages/sdk/src/prisma/prisma-schema-generator.ts
🧰 Additional context used
🧠 Learnings (2)
📚 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:
packages/language/test/expression-validation.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 tests/**/type*.{ts,tsx} : Ensure TypeScript inference and type coverage are validated through type coverage tests
Applied to files:
packages/language/test/expression-validation.test.ts
🧬 Code graph analysis (8)
packages/language/src/zmodel-code-generator.ts (1)
packages/language/src/utils.ts (1)
isCollectionPredicate(466-468)
packages/plugins/policy/src/expression-transformer.ts (1)
packages/schema/src/expression-utils.ts (1)
ExpressionUtils(19-124)
packages/language/test/expression-validation.test.ts (1)
packages/language/test/utils.ts (1)
loadSchema(10-21)
packages/language/src/validators/attribute-application-validator.ts (1)
packages/language/src/generated/ast.ts (3)
ArrayExpr(185-189)ArrayExpr(191-191)isReferenceExpr(698-700)
packages/language/src/zmodel-scope.ts (3)
packages/language/src/generated/ast.ts (3)
isBinaryExpr(267-269)Expression(115-115)Expression(117-117)packages/language/src/utils.ts (1)
isCollectionPredicate(466-468)packages/schema/src/expression.ts (1)
Expression(1-10)
packages/sdk/src/prisma/prisma-schema-generator.ts (1)
packages/language/src/generated/ast.ts (1)
isBinaryExpr(267-269)
packages/language/src/zmodel-linker.ts (1)
packages/language/src/generated/ast.ts (7)
isBinaryExpr(267-269)EnumField(437-443)EnumField(445-445)DataField(334-341)DataField(343-343)FunctionParam(480-486)FunctionParam(488-488)
packages/language/src/ast.ts (1)
packages/language/src/generated/ast.ts (2)
BinaryExpr(256-263)BinaryExpr(265-265)
⏰ 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). (3)
- GitHub Check: Agent
- GitHub Check: build-test (22.x, sqlite)
- GitHub Check: build-test (22.x, postgresql)
🔇 Additional comments (21)
packages/language/src/ast.ts (1)
49-54: LGTM!The module augmentation for
BinaryExpr.bindingis consistent with existing patterns in this file (e.g.,AttributeArg.$resolvedParam) and correctly extends the generated AST type to support the optional iterator binding for collection predicates.packages/schema/src/expression.ts (1)
39-45: LGTM!The optional
bindingproperty is correctly added toBinaryExpression, maintaining consistency with the AST-level changes and preserving backward compatibility for non-collection-predicate binary expressions.packages/language/src/zmodel-code-generator.ts (1)
255-263: LGTM!The code generator correctly handles the optional binding in collection predicates, producing
[binding, expr]when a binding exists and[expr]otherwise. This aligns with the grammar definition inzmodel.langium.packages/language/src/zmodel.langium (1)
69-69: LGTM!The grammar changes correctly implement the collection alias feature:
- Adding
BinaryExprtoReferenceTargetenables the linker to resolve references to binding variables- The optional binding syntax
(binding=RegularID ',')?cleanly supports bothcollection?[binding, expr]and the existingcollection?[expr]formsAlso applies to: 112-117
packages/sdk/src/prisma/prisma-schema-generator.ts (1)
355-367: Correct handling of BinaryExpr targets in Prisma schema generation.The logic properly resolves reference names from either the standard
nameproperty or thebindingproperty forBinaryExprtargets. The error handling for unsupported targets is appropriate.One minor observation: the non-null assertion
node.target.ref!on line 356 could theoretically fail if the reference is unresolved. Consider whether a guard is warranted here, though it may be acceptable if the caller guarantees resolution.packages/language/src/zmodel-linker.ts (3)
125-126: LGTM!The fallback chain
target.name ?? target.binding ?? reference.$refTextcorrectly handles the three cases: named targets (DataField, FunctionParam, EnumField), binding targets (BinaryExpr for collection predicates), and a final fallback to the reference text.
254-274: Correct type resolution for collection predicate bindings.The new resolution logic properly handles
BinaryExprtargets representing collection predicates:
- It extracts the element type from the collection's left operand (
collectionType.decl)- Sets
array: falsesince the binding refers to a single element during iteration- Preserves
nullablefrom the collection typeThe operator check
['?', '!', '^']correctly matches the collection predicate operators defined in the grammar andisCollectionPredicateutility.
522-525: Good defensive guard.The early return for missing
typeprevents potential runtime errors whenresolveToDeclaredTypeis called with an undefined type parameter.packages/plugins/policy/src/expression-evaluator.ts (3)
18-18: LGTM: Scope addition to context type.The optional
scopeproperty enables binding propagation during expression evaluation, which aligns with the PR's goal of supporting collection aliases.
67-72: LGTM: Scope-first field resolution.Prioritizing scope lookup over
thisValueensures that bound aliases (e.g.,minmemberships?[m, ...]) correctly resolve to the bound item rather than the current context value.
119-155: LGTM: Consistent scope propagation across all collection operators.All three operators (
?,!,^) correctly propagate the binding into the scope when evaluating the right-hand side. The scope spreading pattern with nullish coalescing handles the case where no prior scope exists.One minor consideration: if
expr.bindingshadows an existing key incontext.scope, the inner binding takes precedence, which is the expected shadowing behavior for nested scopes.packages/schema/src/expression-utils.ts (1)
42-50: LGTM: Backward-compatible extension of binary factory.The optional
bindingparameter is correctly threaded through to the resultingBinaryExpression. This is backward compatible since existing callers can continue omitting the parameter.Note: When
bindingisundefined, the property is still present on the object (asbinding: undefined). This should be fine since downstream consumers check for truthiness (if (expr.binding)), but verify this doesn't cause issues with serialization or strict property checks.packages/plugins/policy/src/expression-transformer.ts (6)
61-61: LGTM: Well-structured binding scope type.The
BindingScopetype elegantly captures the dual nature of bindings:
typeandaliasfor SQL-based transformations (relation joins)valuefor value-based evaluationsThis separation enables the transformer to handle both code paths correctly.
319-324: LGTM: Scope propagation to evaluator.The
getEvaluationScopehelper correctly extracts only the value-bearing entries from the binding scope, matching the simplerRecord<string, any>expected byExpressionEvaluator.
359-371: LGTM: Binding scope construction for relation-based predicates.When transforming collection predicates that operate on relations (not values), the binding scope correctly captures
typeandaliasfor the bound element. The alias defaults to the model name, enabling proper SQL column references.
425-438: LGTM: Binding scope construction for value-based predicates.For value-based collection predicates (e.g.,
auth().memberships), the binding scope correctly captures thevalueof each item, enabling nested predicates to access the bound element's data.
880-893: LGTM: Evaluation scope extraction.The helper correctly filters the binding scope to only include entries with defined
valueproperties, returningundefinedwhen empty. This ensures the evaluator receives a clean scope without type-only entries.
628-645: Verify rewritten context consistency — Already correctly implemented.The code properly distinguishes between value-based and relation-based bindings. When
bindingReceiver.valueis undefined (relation-based), the expression is rewritten to usethisand contextValue is passed as undefined through the recursive call. The subsequentif (ExpressionUtils.isThis(expr.receiver))block at line 670 explicitly setscontextValue: undefinedwhen delegating to relation access (line 678). Downstream code consistently checksif (context.contextValue)before treating expressions as value objects (lines 177, 283, 312), ensuring relation-based bindings follow the SQL join path rather than value evaluation.packages/language/src/zmodel-scope.ts (3)
10-10: LGTM: Required import for StreamScope construction.The
streamfunction import is needed to construct theStreamScopewith the binding description.
151-153: LGTM: Collection predicate handling in member access scope.When a reference target is a
BinaryExprrepresenting a collection predicate (e.g., accessingm.tenantIdwheremis bound to a collection predicate), this correctly delegates tocreateScopeForCollectionElementto resolve the element type's fields.
197-209: LGTM: Binding description injection into scope.When a collection predicate has a binding, a description is created and added to a
StreamScopethat wraps the collection scope. This enables the binding identifier (e.g.,m) to be resolved as a reference target, pointing to theBinaryExpritself.
There was a problem hiding this comment.
Pull request overview
This PR introduces support for aliasing collection elements in @@allow policy expressions, enabling more expressive access control rules. The new syntax allows binding an iterator variable to collection elements (e.g., memberships?[m, m.tenantId == id]), making it easier to reference collection items in nested predicates.
Key changes:
- Extended the grammar to support optional iterator bindings in collection predicate syntax
- Updated the AST to include a
bindingproperty onBinaryExprnodes for collection predicates - Enhanced the expression transformer and evaluator to properly handle scoped bindings throughout policy evaluation
Reviewed changes
Copilot reviewed 12 out of 14 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/language/src/zmodel.langium | Added optional binding parameter (binding=RegularID ',')? to collection predicate syntax |
| packages/language/src/generated/ast.ts | Updated AST type definitions to include BinaryExpr as a ReferenceTarget and added binding property |
| packages/language/src/ast.ts | Added TypeScript interface extension for BinaryExpr with optional binding property and documentation |
| packages/language/src/generated/grammar.ts | Generated grammar implementation reflecting the new binding syntax |
| packages/schema/src/expression.ts | Added optional binding field to BinaryExpression type |
| packages/schema/src/expression-utils.ts | Extended binary utility function to accept optional binding parameter |
| packages/language/src/zmodel-scope.ts | Enhanced scope provider to create scope entries for collection predicate bindings and support BinaryExpr in collection element resolution |
| packages/language/src/zmodel-linker.ts | Updated linker to handle BinaryExpr as a ReferenceTarget and resolve types for bound collection predicates |
| packages/language/src/zmodel-code-generator.ts | Modified code generator to output binding syntax in collection predicates |
| packages/language/src/validators/attribute-application-validator.ts | Updated validator to handle references to BinaryExpr targets |
| packages/plugins/policy/src/expression-transformer.ts | Added BindingScope type and integrated binding support throughout expression transformation with proper scope propagation |
| packages/plugins/policy/src/expression-evaluator.ts | Enhanced evaluator to support scoped bindings during expression evaluation |
| packages/sdk/src/prisma/prisma-schema-generator.ts | Updated schema generator to extract binding names from BinaryExpr references |
| packages/language/test/expression-validation.test.ts | Added tests validating both new binding syntax and backward compatibility with unbound syntax |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
packages/language/src/validators/attribute-application-validator.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
tests/e2e/orm/policy/auth-access.test.ts (1)
133-175: Consider adding edge cases for more comprehensive coverage.The test correctly validates the iterator binding feature for the basic scenario. To align with the comprehensiveness of similar tests in this file (e.g., lines 177-208), consider adding test cases for:
- Empty memberships array:
memberships: []- Missing memberships field: just
{ tenantId: 1 }- Multiple memberships with at least one match:
memberships: [{ tenantId: 3 }, { tenantId: 1 }]- Multiple memberships with none matching:
memberships: [{ tenantId: 3 }, { tenantId: 4 }]These edge cases would provide stronger validation of the iterator binding semantics across different collection states.
Example test additions
// Empty memberships - should deny await expect( db.$setAuth({ tenantId: 1, memberships: [] }).foo.findMany(), ).resolves.toEqual([]); // Missing memberships field - should deny await expect( db.$setAuth({ tenantId: 1 }).foo.findMany(), ).resolves.toEqual([]); // Multiple memberships with one match - should allow await expect( db.$setAuth({ tenantId: 1, memberships: [{ id: 20, tenantId: 3 }, { id: 10, tenantId: 1 }] }).foo.findMany(), ).resolves.toEqual([ { id: 1, tenantId: 1 }, ]);packages/language/src/zmodel-linker.ts (1)
259-278: Collection predicate resolution logic is sound.The BinaryExpr handling correctly derives the element type from the collection type by setting
array: false, which aligns with the iterator alias semantics introduced in this PR.For improved type safety, consider using type guard functions instead of
$typestring comparisons where available:🔎 Verify if type guards exist for EnumField and FunctionParam
#!/bin/bash # Description: Check if type guard functions exist for EnumField and FunctionParam # Search for type guard function definitions rg -n "function isEnumField|function isFunctionParam" --type=ts packages/language/src/ # Also check in generated AST rg -n "export function isEnumField|export function isFunctionParam" --type=ts packages/language/src/generated/ast.tsIf these type guards exist, import and use them instead of
$typecomparisons for better type narrowing.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
packages/language/src/validators/attribute-application-validator.tspackages/language/src/zmodel-linker.tspackages/sdk/src/prisma/prisma-schema-generator.tspackages/sdk/src/ts-schema-generator.tstests/e2e/orm/policy/auth-access.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/sdk/src/prisma/prisma-schema-generator.ts
- packages/language/src/validators/attribute-application-validator.ts
🧰 Additional context used
📓 Path-based instructions (1)
tests/e2e/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
E2E tests should validate real-world schema compatibility with established projects
Files:
tests/e2e/orm/policy/auth-access.test.ts
🧠 Learnings (2)
📚 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/e2e/orm/policy/auth-access.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 tests/e2e/**/*.{ts,tsx} : E2E tests should validate real-world schema compatibility with established projects
Applied to files:
tests/e2e/orm/policy/auth-access.test.ts
🧬 Code graph analysis (3)
tests/e2e/orm/policy/auth-access.test.ts (1)
packages/testtools/src/client.ts (1)
createPolicyTestClient(258-269)
packages/language/src/zmodel-linker.ts (1)
packages/language/src/generated/ast.ts (6)
isBinaryExpr(267-269)EnumField(437-443)EnumField(445-445)isDataField(345-347)FunctionParam(480-486)FunctionParam(488-488)
packages/sdk/src/ts-schema-generator.ts (1)
packages/language/src/generated/ast.ts (3)
isDataField(345-347)isEnumField(447-449)isBinaryExpr(267-269)
⏰ 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: build-test (22.x, sqlite)
- GitHub Check: build-test (22.x, postgresql)
🔇 Additional comments (4)
packages/language/src/zmodel-linker.ts (2)
527-529: Good defensive guard.The early return prevents potential null/undefined errors when accessing properties on the
typeparameter. This is a solid defensive programming practice.
125-131: Type safety for binding property access is properly implemented.The
bindingproperty is explicitly defined as optional (binding?: string) in the BinaryExpr interface atpackages/language/src/ast.ts:53, and the code correctly checks for property existence and type before accessing it. The implementation is type-safe.packages/sdk/src/ts-schema-generator.ts (2)
1274-1285: LGTM! Clean conditional binding support.The implementation correctly passes the optional binding parameter to
ExpressionUtils.binarywhen present, aligning with the PR's goal of supporting iterator bindings in collection predicates.
1300-1323: Binding resolution through the language linker is properly implemented.The code correctly handles binding references. The language linker (zmodel-linker.ts:257-267) resolves references to collection predicates as
BinaryExprtargets, and the scoping system (zmodel-scope.ts:199-205) creates scope entries for binding names. The code properly extracts binding names fromBinaryExprtargets, and the fallback behavior correctly treats unresolved references as field names for iterator bindings.No further changes needed.
|
@ymc9 have you been able to take a look at this one? i was curious if you have any feedback or if this is looking correct to you. |
I'm half way into reviewing it and will probably make a bit of refactor to how the binding is represented in the ZModel language definition. Will make a separate PR based on your branch soon. |
2 participants
provides a means to alias collections in
@@allowcollections by extending the ast this allows for utilizing collections inside of@@allowlike:Summary by CodeRabbit
New Features
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.