feat(zmodel): collection predicate binding #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. 📝 WalkthroughWalkthroughThis PR introduces collection predicate iterator bindings across the language, schema, and policy layers. Changes include: grammar support for optional bindings in collection predicates, linker/scope resolution for binding contexts, expression type additions, policy evaluator and transformer extensions to thread binding scopes, validation updates, and comprehensive test coverage for binding semantics in various scenarios. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~55 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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. |
|
Hi @mwillbanks , I've finally got time to finish refactoring this change. Could you take a look at the PR sent to your branch? Thanks! mwillbanks#2 |
…nstruct (#2) - adjusted language processing chain accordingly - fixed several issues in policy transformer/evaluator - more test cases
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/language/src/zmodel-scope.ts (2)
146-163: Propagate auth/typedef context when resolving member access on bindings.Line 155–161 uses
allowTypeDefScopederived from the current member access operand, which ismform.tenantId. Ifmis bound from anauth()-derived collection, typedef fields will be incorrectly excluded. Consider derivingallowTypeDefScopefrom the binding’s collection expression.🔧 Proposed fix
.when(isCollectionPredicateBinding, (r) => - // build a scope from the collection predicate's collection - this.createScopeForCollectionPredicateCollection( - r.$container.left, - globalScope, - allowTypeDefScope, - ), + // build a scope from the collection predicate's collection + (() => { + const bindingCollection = r.$container.left; + const allowTypeDefScopeForBinding = + allowTypeDefScope || isAuthOrAuthMemberAccess(bindingCollection); + return this.createScopeForCollectionPredicateCollection( + bindingCollection, + globalScope, + allowTypeDefScopeForBinding, + ); + })(), )
227-261:isAuthInvocationbranch is unreachable due to earlierisInvocationExprmatch.Line 249 matches all invocation expressions, so Line 257 never executes. If
auth()needs special handling here, move it beforeisInvocationExpror fold it into that branch.🔧 Proposed fix
- .when(isInvocationExpr, (expr) => { - const returnTypeDecl = expr.function.ref?.returnType.reference?.ref; - if (isDataModel(returnTypeDecl)) { - return this.createScopeForContainer(returnTypeDecl, globalScope, allowTypeDefScope); - } else { - return EMPTY_SCOPE; - } - }) - .when(isAuthInvocation, (expr) => { - return this.createScopeForAuth(expr, globalScope); - }) + .when(isAuthInvocation, (expr) => { + return this.createScopeForAuth(expr, globalScope); + }) + .when(isInvocationExpr, (expr) => { + const returnTypeDecl = expr.function.ref?.returnType.reference?.ref; + if (isDataModel(returnTypeDecl)) { + return this.createScopeForContainer(returnTypeDecl, globalScope, allowTypeDefScope); + } else { + return EMPTY_SCOPE; + } + })
🤖 Fix all issues with AI agents
In `@packages/plugins/policy/src/expression-transformer.ts`:
- Around line 369-374: The bindingScope creation uses context.alias for the
binding alias which can point to the outer model; change it so the binding alias
always references the relation model used in the subquery. Specifically, update
the object built when expr.binding is truthy (the bindingScope construction) to
set the alias to the relation model identifier (newContextModel or the
relation-model-specific alias) instead of context.alias (i.e., use alias:
newContextModel), ensuring expr.binding maps to the relation element table for
correct column emission.
🧹 Nitpick comments (1)
tests/e2e/orm/policy/collection-predicate.test.ts (1)
4-4: Consider adding at least one established-project schema case.These E2E tests use synthetic schemas; a real‑world schema fixture would better validate compatibility. As per coding guidelines, consider adding a real‑world schema fixture.
📜 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 (15)
packages/language/src/validators/expression-validator.tspackages/language/src/zmodel-linker.tspackages/language/src/zmodel-scope.tspackages/language/src/zmodel.langiumpackages/language/test/expression-validation.test.tspackages/orm/src/client/crud/validator/utils.tspackages/orm/src/utils/schema-utils.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.tspackages/sdk/src/ts-schema-generator.tstests/e2e/orm/policy/auth-access.test.tstests/e2e/orm/policy/collection-predicate.test.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- tests/e2e/orm/policy/auth-access.test.ts
- packages/schema/src/expression.ts
- packages/sdk/src/prisma/prisma-schema-generator.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/collection-predicate.test.ts
🧠 Learnings (5)
📚 Learning: 2025-12-30T15:07:06.254Z
Learnt from: mwillbanks
Repo: zenstackhq/zenstack-v3 PR: 550
File: packages/orm/src/client/crud/operations/base.ts:158-159
Timestamp: 2025-12-30T15:07:06.254Z
Learning: Do not use ts-expect-error in production code within the zenstackhq/zenstack-v3 repo (e.g., packages/*). Use explicit type annotations, targeted type assertions, or refactor to resolve the type error. ts-expect-error may be acceptable only in test files for stubbing or temporary silencing. Ensure production code is type-safe and maintainable.
Applied to files:
packages/plugins/policy/src/expression-evaluator.tspackages/orm/src/client/crud/validator/utils.tspackages/orm/src/utils/schema-utils.tspackages/schema/src/expression-utils.tspackages/language/src/zmodel-scope.tspackages/plugins/policy/src/expression-transformer.tspackages/language/src/validators/expression-validator.tspackages/sdk/src/ts-schema-generator.tspackages/language/src/zmodel-linker.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/utils/schema-utils.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/e2e/orm/policy/collection-predicate.test.tspackages/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/e2e/**/*.{ts,tsx} : E2E tests should validate real-world schema compatibility with established projects
Applied to files:
tests/e2e/orm/policy/collection-predicate.test.tspackages/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:
tests/e2e/orm/policy/collection-predicate.test.tspackages/language/test/expression-validation.test.ts
🧬 Code graph analysis (7)
packages/plugins/policy/src/expression-evaluator.ts (2)
packages/schema/src/expression-utils.ts (1)
ExpressionUtils(20-134)packages/schema/src/expression.ts (1)
BindingExpression(34-37)
packages/orm/src/utils/schema-utils.ts (1)
packages/schema/src/expression.ts (1)
BindingExpression(34-37)
packages/language/src/zmodel-scope.ts (2)
packages/language/src/generated/ast.ts (6)
isDataField(357-359)isCollectionPredicateBinding(291-293)BinaryExpr(256-263)BinaryExpr(265-265)Expression(115-115)Expression(117-117)packages/language/src/utils.ts (2)
isCollectionPredicate(466-468)isAuthOrAuthMemberAccess(133-135)
packages/plugins/policy/src/expression-transformer.ts (3)
packages/schema/src/expression-utils.ts (1)
ExpressionUtils(20-134)packages/common-helpers/src/tiny-invariant.ts (1)
invariant(4-14)packages/schema/src/expression.ts (1)
BindingExpression(34-37)
packages/sdk/src/ts-schema-generator.ts (1)
packages/language/src/generated/ast.ts (3)
isDataField(357-359)isEnumField(459-461)isCollectionPredicateBinding(291-293)
packages/language/src/zmodel-linker.ts (1)
packages/language/src/generated/ast.ts (4)
isCollectionPredicateBinding(291-293)isEnumField(459-461)isDataField(357-359)isFunctionParam(502-504)
tests/e2e/orm/policy/collection-predicate.test.ts (1)
packages/testtools/src/client.ts (1)
createPolicyTestClient(258-269)
🔇 Additional comments (40)
packages/orm/src/utils/schema-utils.ts (3)
2-14: LGTM!The
BindingExpressionimport is correctly added and properly placed within the existing type imports.
19-32: LGTM!The new match case for
kind: 'binding'is correctly integrated into the visitor's exhaustive pattern matching. The.exhaustive()call ensures compile-time safety for handling all expression kinds.
73-74: LGTM!The
visitBindingmethod correctly follows the established pattern for leaf expression nodes (likevisitLiteral,visitThis,visitNull). SinceBindingExpressiononly contains aname: stringproperty with no child expressions, the empty base implementation is appropriate—subclasses can override to add custom behavior when needed.packages/orm/src/client/crud/validator/utils.ts (1)
313-315: Clear guard for unsupported binding expressions.Explicitly failing fast here prevents silent mis-evaluation in validation expressions.
packages/schema/src/expression-utils.ts (2)
1-6: Binary factory correctly threads optional binding metadata.The signature and returned shape align with the new BindingExpression plumbing.
Also applies to: 43-50
76-81: Binding factory + guard fit the existing utility surface.Consistent with other ExpressionUtils helpers.
Also applies to: 129-129
packages/sdk/src/ts-schema-generator.ts (2)
1275-1285: Binding argument is preserved in generated binary expressions.This keeps collection predicate bindings round-trippable in emitted schema.
16-17: Reference resolution now supports collection predicate bindings.Good mapping to
ExpressionUtils.binding(...)for binding targets.Also applies to: 1302-1313
packages/plugins/policy/src/expression-evaluator.ts (1)
6-7: Binding scope propagation and resolution look solid.The added dispatch + scope wiring makes binding evaluation explicit and predictable.
Also applies to: 16-37, 71-75, 123-155, 161-165
packages/plugins/policy/src/expression-transformer.ts (2)
62-102: Binding scope plumbing across evaluator/value paths is consistent.Nice job threading scope into evaluation and value-based predicate expansion.
Also applies to: 321-325, 376-381, 423-424, 435-455, 923-936
13-14: Binding-aware member access integrates cleanly with existing resolution.The binding receiver handling and scope resolution fit the existing access patterns well.
Also applies to: 349-361, 645-735, 788-792
packages/language/src/validators/expression-validator.ts (2)
36-79: Reference-level validation integration looks solid.The resolution-error guard and explicit
ReferenceExprhandling make the validation flow clearer without affecting existing member-access checks.
82-90: Binding access guard is clear and targeted.This enforces the intended alias usage pattern for collection predicates.
tests/e2e/orm/policy/collection-predicate.test.ts (11)
5-32: Good coverage for unbound collection predicates.
34-61: Binding reference path covered well.
63-90: Mixed bound/unbound predicate coverage looks good.
92-120:thisdisambiguation scenario covered.
122-164: Deep-context binding case 1 covered.
166-210: Deep-context binding case 2 covered.
212-259: To-one relation access from binding is exercised.
261-307: Nested multi-binding scenario covered.
309-353: Inner-binding masking scenario covered.
355-398: Auth-based binding predicate scenario looks solid.
400-446: Auth predicate with pure-value binding is well tested.packages/language/test/expression-validation.test.ts (10)
6-25: Model comparison rejection case covered.
27-55: Second model comparison rejection case covered.
56-80: Auth comparison positive case covered.
82-101: Auth comparison negative case covered.
105-128: Standalone binding rejection case covered.
130-150: Binding reference schema acceptance covered.
152-172: Unbound predicate syntax still exercised.
174-195: Mixed bound/unbound predicate schema covered.
196-268: Disambiguation withthisand type-mismatch cases covered.
270-358: Deep-context binding coverage looks solid.packages/language/src/zmodel-linker.ts (2)
251-268: Reference resolution now covers bindings, enum fields, and params cleanly.
517-520: Defensive guard against missing declared types is good.packages/language/src/zmodel.langium (1)
69-122: Grammar updates cleanly add optional binding support in collection predicates.packages/language/src/zmodel-scope.ts (3)
20-24: Import updates are aligned with new scope logic.
125-130: Collection predicate routing in getScope looks good.
194-225: Binding stack scope composition is solid.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
ymc9
changed the title
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
Release Notes
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.