Skip to content

Conversation

@ymc9
Copy link
Member

@ymc9 ymc9 commented Sep 29, 2025

Summary by CodeRabbit

  • New Features

    • Added a post-update policy operation enforced after mutations and a before() helper to reference pre-update values.
  • Bug Fixes / Validation

    • Prevent relation-field access from before() with explicit validation errors.
    • Post-update rules default to allow when no explicit allows exist.
  • Refactor

    • Standardized update/return read-back to ensure correct post-update validation.
  • Tests

    • Added comprehensive post-update policy test coverage.

Copilot AI review requested due to automatic review settings September 29, 2025 05:31
@coderabbitai
Copy link

coderabbitai bot commented Sep 29, 2025

Warning

Rate limit exceeded

@ymc9 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 17 minutes and 8 seconds before requesting another review.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

📥 Commits

Reviewing files that changed from the base of the PR and between 8fc0da5 and fd5f66d.

📒 Files selected for processing (2)
  • packages/runtime/test/schemas/petstore/schema.ts (1 hunks)
  • packages/runtime/test/schemas/petstore/schema.zmodel (1 hunks)

Walkthrough

Adds a "post-update" policy operation and replaces future() with before() across language and runtime layers; updates validators, linkers, transformers, expression traversal, and policy enforcement to support pre-update reads and post-update checks; adds tests and a TODO entry.

Changes

Cohort / File(s) Summary
Docs / TODO
TODO.md
Adds a TODO: "Accessing tables not in the schema"; adjusts indentation.
Language stdlib
packages/language/res/stdlib.zmodel
Adds 'post-update' completion hint to @@allow/@@deny; renames helper future()before() and updates docs.
Language utils & validators & tests
packages/language/src/utils.ts, packages/language/src/validators/attribute-application-validator.ts, packages/language/src/validators/expression-validator.ts, packages/language/test/attribute-application.test.ts
Removes isFuture* helpers, adds isBeforeInvocation, switches validations to before(), allows post-update policy kind, disallows relation access from before() and adds a test asserting the error.
Language linker & scope
packages/language/src/zmodel-linker.ts, packages/language/src/zmodel-scope.ts
Special-case resolution moved from future() to before() so before() resolves to the containing model.
Runtime client types & ops
packages/runtime/src/client/contract.ts, packages/runtime/src/client/options.ts, packages/runtime/src/client/crud/operations/base.ts
Introduces CRUD_EXT = CRUD ∪ 'post-update'; updates contexts to use CRUD_EXT; refactors update returning flow to use executeQuery and return rows.
Policy plugin API & types
packages/runtime/src/plugins/policy/plugin.ts, packages/runtime/src/plugins/policy/types.ts
Removes transaction param from onKyselyQuery; switches policy operation types to CRUD_EXT.
Policy transformer, utils & traversal
packages/runtime/src/plugins/policy/expression-transformer.ts, packages/runtime/src/plugins/policy/utils.ts, packages/runtime/src/utils/expression-utils.ts
Adds isBeforeInvocation; transformer supports before() member path ($before) and uses CRUD_EXT; adds ExpressionVisitor for expression traversal; updates imports and transform call-sites.
Policy handler
packages/runtime/src/plugins/policy/policy-handler.ts
Implements post-update enforcement: pre-update reads for before() fields, post-update validation of updated rows, builds policy filters with CRUD_EXT, propagates filters into joins/nested reads, and adjusts returning/read-back logic.
Tests & schemas
packages/runtime/test/policy/crud/post-update.test.ts, packages/runtime/test/schemas/petstore/schema.ts, packages/runtime/test/schemas/petstore/schema.zmodel
Adds comprehensive post-update policy tests; updates test schemas to use before() instead of future().

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60–90 minutes

Possibly related PRs

Poem

🐰 I nibble code and hop before,
swapped future fog for "before" once more.
Pre-read carrots, post-check clover,
policies snug from end to over.
Hooray — I hop, rules safe evermore.

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check ✅ Passed The title “feat(policy): post-update policies” succinctly and accurately reflects the primary change in this pull request, which is the introduction of post-update policy support across the codebase. It follows conventional commit conventions, specifies the scope (“policy”), and clearly states the feature being added without extraneous detail or jargon.
Docstring Coverage ✅ Passed No functions found in the changes. Docstring coverage check skipped.

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@claude
Copy link

claude bot commented Sep 29, 2025

Claude finished @ymc9's task —— View job


PR Review: Post-Update Policies ✅

This PR implements post-update policies, which allow validation rules to be applied after database updates occur. The implementation adds support for @@allow and @@deny rules with the postUpdate operation type.

Review Summary

Overall Assessment: GOOD - Well-implemented feature with comprehensive testing, but has some areas requiring attention.

Strengths

  1. Comprehensive Test Coverage (packages/runtime/test/policy/crud/post-update.test.ts:1)

    • Excellent test suite covering various scenarios including simple rules, relation conditions, and before() function usage
    • Tests include both positive and negative cases
    • Good edge case coverage (e.g., accessing relation fields from before())
  2. Clean Language Integration (packages/language/res/stdlib.zmodel:669,687)

    • Proper addition of 'post-update' to completion hints for @@allow and @@deny attributes
    • Consistent with existing operation types
  3. Robust Expression Validation (packages/language/src/validators/expression-validator.ts:74-76)

    • Added validation to prevent accessing relation fields from before() function
    • Clear error message: "relation fields cannot be accessed from before()"
  4. Well-Structured Policy Handler (packages/runtime/src/plugins/policy/policy-handler.ts:221-274)

    • Clean separation between pre-update loading and post-update validation
    • Proper handling of ID field requirements for joining with before-update rows

⚠️ Areas of Concern

  1. Performance Impact (packages/runtime/src/plugins/policy/policy-handler.ts:226-243)

    • The loadBeforeUpdateEntities method executes an additional SELECT query before every update
    • This could impact performance for frequent updates, especially with large datasets
    • Recommendation: Consider documenting performance implications or adding configuration options
  2. Complex Query Logic (packages/runtime/src/plugins/policy/policy-handler.ts:135-187)

    • The post-update validation involves complex SQL generation with VALUES tables and JOINs
    • The logic for creating before-update tables using ValuesNode.create is intricate
    • Risk: Complex SQL generation increases chances of edge case bugs
  3. Missing Error Context (packages/runtime/src/plugins/policy/policy-handler.ts:180-186)

    • Post-update policy failures provide generic error messages
    • Users may struggle to identify which specific entities or rules failed
    • Recommendation: Enhanced error messages with entity identifiers

🐛 Potential Issues

  1. Transaction Handling (packages/runtime/test/policy/crud/post-update.test.ts:129)

    • Test is skipped for query builder API due to "transaction issue"
    • This suggests potential problems with transaction isolation for post-update policies
    • Critical: This needs investigation as it could lead to data consistency issues
  2. Type Safety Gap (packages/runtime/src/plugins/policy/policy-handler.ts:153-157)

    const castedColumnRef = sql`CAST(${eb.ref(`column${index + 1}`)} as ${sql.raw(this.dialect.getFieldSqlType(def))})`.as(name);
    • Raw SQL injection for type casting could be vulnerable to SQL injection if field types aren't properly sanitized
    • Recommendation: Add validation for field type names
  3. Memory Usage (packages/runtime/src/plugins/policy/policy-handler.ts:241-242)

    • Before-update entities are loaded entirely into memory
    • For bulk updates, this could cause memory issues
    • Recommendation: Consider pagination or streaming for large result sets

🔒 Security Considerations

  1. Access Control Bypass (packages/runtime/src/plugins/policy/policy-handler.ts:790-792)

    • Post-update operations are allowed by default if no allow rules are defined
    • This differs from other operations which are denied by default
    • Security Risk: Could lead to unintended access if policies are misconfigured
  2. Expression Evaluation (packages/runtime/src/utils/expression-utils.ts:1)

    • New expression utilities for handling before() and future() references
    • Need to ensure proper validation to prevent injection attacks

🎯 Recommendations

  1. Address Transaction Issues (Priority: HIGH)

    • Investigate and fix the transaction handling problems indicated by the skipped test
    • This is critical for data consistency
  2. Performance Optimization (Priority: MEDIUM)

    • Add benchmarks for post-update policy performance
    • Consider lazy loading or caching strategies for before-update data
  3. Enhanced Error Messages (Priority: MEDIUM)

    • Include entity identifiers and specific rule failures in error messages
    • Add debugging information for complex policy failures
  4. Security Hardening (Priority: HIGH)

    • Review the default allow behavior for post-update operations
    • Add validation for SQL type casting operations
  5. Documentation (Priority: LOW)

    • Add performance considerations to documentation
    • Document the security implications of default allow behavior for post-update

Final Verdict

This is a well-architected feature with good test coverage. The main concerns are around transaction handling (which needs immediate attention) and performance implications for production usage. The code quality is good overall with appropriate error handling patterns.


Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR introduces post-update policies feature, allowing validation of entity states after database updates. The implementation replaces the previous future() function with before() to access pre-update values and adds support for 'post-update' operations in policy rules.

  • Replaced future() function with before() for accessing pre-update entity values
  • Added 'post-update' operation support to policy system with default allow behavior
  • Implemented comprehensive test coverage for post-update policy scenarios

Reviewed Changes

Copilot reviewed 19 out of 22 changed files in this pull request and generated no comments.

Show a summary per file
File Description
packages/runtime/test/schemas/petstore/schema.zmodel Updated policy rule to use before() instead of future()
packages/runtime/test/schemas/petstore/schema.ts Generated schema update reflecting the before() function change
packages/runtime/test/policy/crud/post-update.test.ts Comprehensive test suite for post-update policy functionality
packages/runtime/src/utils/expression-utils.ts New expression visitor utility for traversing AST nodes
packages/runtime/src/plugins/policy/utils.ts Added utility to detect before() invocations
packages/runtime/src/plugins/policy/types.ts Updated policy operation types to include 'post-update'
packages/runtime/src/plugins/policy/policy-handler.ts Core implementation of post-update policy validation logic
packages/runtime/src/plugins/policy/plugin.ts Simplified plugin interface by removing unused transaction parameter
packages/runtime/src/plugins/policy/expression-transformer.ts Added transformation logic for before() function calls
packages/runtime/src/client/options.ts Updated type definitions to include extended CRUD operations
packages/runtime/src/client/crud/operations/base.ts Fixed update operation to properly handle query execution
packages/runtime/src/client/contract.ts Added CRUD_EXT type including 'post-update' operation
packages/language/src/zmodel-scope.ts Updated scope resolution from future() to before()
packages/language/src/zmodel-linker.ts Updated type linking for before() function
packages/language/src/validators/expression-validator.ts Added validation to prevent relation field access from before()
packages/language/src/validators/attribute-application-validator.ts Updated validators to support 'post-update' operation and before() function
packages/language/src/utils.ts Renamed function from isFutureInvocation to isBeforeInvocation
packages/language/res/stdlib.zmodel Updated standard library to replace future() with before() function
TODO.md Added new TODO item for accessing tables not in schema

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 6

🧹 Nitpick comments (14)
packages/runtime/src/client/contract.ts (1)

226-230: Add re-export of CRUD_EXT in public barrel
Add export { CRUD_EXT } from './contract'; to your public barrel file (e.g., packages/runtime/src/client/index.ts) so external consumers can import it directly.

packages/language/src/zmodel-scope.ts (1)

166-167: Update stale comment: future()before()

The code now checks isBeforeInvocation, but the comment still mentions future().

Apply:

-                // deal with member access from `auth()` and `future()
+                // deal with member access from `auth()` and `before()`
packages/runtime/src/client/options.ts (1)

33-33: Consider updating the docstring to reflect 'post-update'

The comment says “The CRUD operation being performed” but the type now includes 'post-update'. Minor doc tweak to avoid confusion.

-     * The CRUD operation being performed
+     * The operation being performed (CRUD plus 'post-update')
packages/runtime/src/client/crud/operations/base.ts (2)

771-775: Optional: also route createMany(ReturnData=true) through executeQuery for consistency

For symmetry with updateMany, consider using the wrapper to get queryIds and uniform error handling.

-            const idFields = requireIdFields(this.schema, model);
-            const result = await query.returning(idFields as any).execute();
-            return result as Result;
+            const idFields = requireIdFields(this.schema, model);
+            const finalQuery = query.returning(idFields as any);
+            const result = await this.executeQuery(kysely, finalQuery, 'createMany');
+            return result.rows as Result;

1194-1197: Optional: widen makeContextComment to CRUD_EXT for future-proofing

Even though this file currently passes 'update'/'delete' etc., widening the type avoids friction if 'post-update' context comments are added later.

-import { TransactionIsolationLevel, type CRUD } from '../../contract';
+import { TransactionIsolationLevel, type CRUD_EXT } from '../../contract';
...
-    private makeContextComment(_context: { model: GetModels<Schema>; operation: CRUD }) {
+    private makeContextComment(_context: { model: GetModels<Schema>; operation: CRUD_EXT }) {
packages/language/src/validators/attribute-application-validator.ts (1)

169-170: Document that ‘all’ excludes post-update
Add a comment or update the API docs to state that 'all' covers create/read/update/delete but does not include post-update, which must be specified separately.

packages/runtime/src/plugins/policy/expression-transformer.ts (1)

441-443: Tiny simplification: drop redundant null coalescing for auth member arg

valueMemberAccess always returns a ValueNode. You can simplify.

-            const valNode = this.valueMemberAccess(this.auth, arg as MemberExpression, this.authType);
-            return valNode ? eb.val(valNode.value) : eb.val(null);
+            const valNode = this.valueMemberAccess(this.auth, arg as MemberExpression, this.authType) as ValueNode;
+            return eb.val(valNode.value);
packages/runtime/test/policy/crud/post-update.test.ts (1)

96-127: before() semantics well-covered; consider one more negative

Add a test asserting that using before() outside a 'post-update' rule is rejected early by validation.

I can add the test scaffold if you want.

packages/language/res/stdlib.zmodel (5)

666-668: Doc string: include 'post-update' in allowed operations

Reflect new operation in the parameter docs.

- * @param operation: comma-separated list of "create", "read", "update", "delete". Use "all" to denote all operations.
+ * @param operation: comma-separated list of "create", "read", "update", "post-update", "delete". Use "all" to denote all operations.

684-686: Doc string: include 'post-update' for deny as well

Mirror allow() doc change.

- * @param operation: comma-separated list of "create", "read", "update", "delete". Use "all" to denote all operations.
+ * @param operation: comma-separated list of "create", "read", "update", "post-update", "delete". Use "all" to denote all operations.

679-679: Field-level @Allow: should it support 'post-update'?

Currently completion hints exclude 'post-update'. If field-level post-update policies are supported, add it for symmetry; otherwise, consider documenting that it’s intentionally unsupported.

-attribute @allow(_ operation: String @@@completionHint(["'create'", "'read'", "'update'", "'delete'", "'all'"]), _ condition: Boolean, _ override: Boolean?)
+attribute @allow(_ operation: String @@@completionHint(["'create'", "'read'", "'update'", "'post-update'", "'delete'", "'all'"]), _ condition: Boolean, _ override: Boolean?)

695-695: Field-level @deny: mirror the decision for 'post-update'

Keep hints consistent with @Allow.

-attribute @deny(_ operation: String @@@completionHint(["'create'", "'read'", "'update'", "'delete'", "'all'"]), _ condition: Boolean)
+attribute @deny(_ operation: String @@@completionHint(["'create'", "'read'", "'update'", "'post-update'", "'delete'", "'all'"]), _ condition: Boolean)

188-197: currentOperation doc: include 'post-update'

Update narrative to reflect expansion.

- * "all" operation is expanded to "create", "read", "update", and "delete" rules,
+ * "all" operation is expanded to "create", "read", "update", "post-update", and "delete" rules,
packages/runtime/src/plugins/policy/policy-handler.ts (1)

139-160: Complex but necessary SQL construction for before-update table.

The VALUES table construction with proper type casting is well-implemented. This approach allows efficient in-database comparison between before and after states.

Consider adding a comment explaining the structure of this generated SQL for future maintainers, as the nested node creation is quite complex.

📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 3b13b72 and fd06893.

📒 Files selected for processing (19)
  • TODO.md (1 hunks)
  • packages/language/res/stdlib.zmodel (3 hunks)
  • packages/language/src/utils.ts (1 hunks)
  • packages/language/src/validators/attribute-application-validator.ts (3 hunks)
  • packages/language/src/validators/expression-validator.ts (2 hunks)
  • packages/language/src/zmodel-linker.ts (2 hunks)
  • packages/language/src/zmodel-scope.ts (2 hunks)
  • packages/runtime/src/client/contract.ts (1 hunks)
  • packages/runtime/src/client/crud/operations/base.ts (1 hunks)
  • packages/runtime/src/client/options.ts (2 hunks)
  • packages/runtime/src/plugins/policy/expression-transformer.ts (4 hunks)
  • packages/runtime/src/plugins/policy/plugin.ts (1 hunks)
  • packages/runtime/src/plugins/policy/policy-handler.ts (14 hunks)
  • packages/runtime/src/plugins/policy/types.ts (2 hunks)
  • packages/runtime/src/plugins/policy/utils.ts (2 hunks)
  • packages/runtime/src/utils/expression-utils.ts (1 hunks)
  • packages/runtime/test/policy/crud/post-update.test.ts (1 hunks)
  • packages/runtime/test/schemas/petstore/schema.ts (1 hunks)
  • packages/runtime/test/schemas/petstore/schema.zmodel (1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
{packages,samples,tests}/**

📄 CodeRabbit inference engine (CLAUDE.md)

Place packages only under packages/, samples/, or tests/

Files:

  • packages/runtime/src/plugins/policy/plugin.ts
  • packages/language/src/zmodel-scope.ts
  • packages/language/src/utils.ts
  • packages/runtime/test/schemas/petstore/schema.ts
  • packages/runtime/src/client/contract.ts
  • packages/language/src/zmodel-linker.ts
  • packages/runtime/src/client/crud/operations/base.ts
  • packages/runtime/src/plugins/policy/policy-handler.ts
  • packages/runtime/src/plugins/policy/types.ts
  • packages/runtime/src/plugins/policy/expression-transformer.ts
  • packages/runtime/src/client/options.ts
  • packages/language/src/validators/expression-validator.ts
  • packages/runtime/test/policy/crud/post-update.test.ts
  • packages/language/src/validators/attribute-application-validator.ts
  • packages/runtime/src/utils/expression-utils.ts
  • packages/runtime/test/schemas/petstore/schema.zmodel
  • packages/runtime/src/plugins/policy/utils.ts
  • packages/language/res/stdlib.zmodel
**/schema.ts

📄 CodeRabbit inference engine (CLAUDE.md)

The generated TypeScript schema should be named schema.ts

Files:

  • packages/runtime/test/schemas/petstore/schema.ts
**/schema.zmodel

📄 CodeRabbit inference engine (CLAUDE.md)

Name ZModel schema files schema.zmodel

Files:

  • packages/runtime/test/schemas/petstore/schema.zmodel
🧬 Code graph analysis (15)
packages/runtime/src/plugins/policy/plugin.ts (2)
packages/runtime/src/client/plugin.ts (1)
  • OnKyselyQueryArgs (188-193)
packages/runtime/src/plugins/policy/policy-handler.ts (1)
  • PolicyHandler (62-990)
packages/language/src/zmodel-scope.ts (2)
packages/language/src/utils.ts (1)
  • isBeforeInvocation (449-451)
packages/runtime/src/plugins/policy/utils.ts (1)
  • isBeforeInvocation (158-160)
packages/language/src/utils.ts (4)
packages/runtime/src/plugins/policy/utils.ts (1)
  • isBeforeInvocation (158-160)
packages/language/src/ast.ts (2)
  • AstNode (4-4)
  • AstNode (66-71)
packages/language/src/generated/ast.ts (1)
  • isInvocationExpr (542-544)
packages/sdk/src/model-utils.ts (1)
  • isFromStdlib (85-88)
packages/runtime/test/schemas/petstore/schema.ts (1)
packages/runtime/src/schema/expression.ts (1)
  • ExpressionUtils (16-120)
packages/language/src/zmodel-linker.ts (2)
packages/language/src/utils.ts (1)
  • isBeforeInvocation (449-451)
packages/runtime/src/plugins/policy/utils.ts (1)
  • isBeforeInvocation (158-160)
packages/runtime/src/client/crud/operations/base.ts (2)
packages/runtime/src/plugins/policy/policy-handler.ts (1)
  • kysely (70-72)
packages/runtime/src/client/executor/zenstack-query-executor.ts (1)
  • kysely (60-62)
packages/runtime/src/plugins/policy/policy-handler.ts (7)
packages/runtime/src/client/plugin.ts (1)
  • ProceedKyselyQueryFunction (195-195)
packages/runtime/src/client/query-utils.ts (3)
  • requireField (38-56)
  • requireIdFields (63-70)
  • requireModel (25-31)
packages/runtime/src/plugins/policy/utils.ts (6)
  • conjunction (49-67)
  • isBeforeInvocation (158-160)
  • trueNode (21-23)
  • falseNode (28-30)
  • disjunction (69-87)
  • buildIsFalse (127-139)
packages/runtime/src/utils/expression-utils.ts (1)
  • ExpressionVisitor (15-58)
packages/sdk/src/schema/expression.ts (1)
  • MemberExpression (27-31)
packages/common-helpers/src/tiny-invariant.ts (1)
  • invariant (4-14)
packages/runtime/src/client/contract.ts (2)
  • CRUD_EXT (219-219)
  • CRUD_EXT (229-229)
packages/runtime/src/plugins/policy/types.ts (1)
packages/runtime/src/client/contract.ts (2)
  • CRUD_EXT (219-219)
  • CRUD_EXT (229-229)
packages/runtime/src/plugins/policy/expression-transformer.ts (6)
packages/sdk/src/schema/schema.ts (2)
  • SchemaDef (10-18)
  • GetModels (109-109)
packages/runtime/src/client/contract.ts (2)
  • CRUD_EXT (219-219)
  • CRUD_EXT (229-229)
packages/sdk/src/schema/expression.ts (1)
  • MemberExpression (27-31)
packages/language/src/utils.ts (1)
  • isBeforeInvocation (449-451)
packages/runtime/src/plugins/policy/utils.ts (1)
  • isBeforeInvocation (158-160)
packages/common-helpers/src/tiny-invariant.ts (1)
  • invariant (4-14)
packages/runtime/src/client/options.ts (1)
packages/runtime/src/client/contract.ts (2)
  • CRUD_EXT (219-219)
  • CRUD_EXT (229-229)
packages/language/src/validators/expression-validator.ts (2)
packages/language/src/generated/ast.ts (3)
  • MemberAccessExpr (546-551)
  • MemberAccessExpr (553-553)
  • isDataModel (391-393)
packages/language/src/utils.ts (1)
  • isBeforeInvocation (449-451)
packages/runtime/test/policy/crud/post-update.test.ts (1)
packages/runtime/test/policy/utils.ts (1)
  • createPolicyTestClient (14-25)
packages/language/src/validators/attribute-application-validator.ts (2)
packages/language/src/utils.ts (1)
  • isBeforeInvocation (449-451)
packages/runtime/src/plugins/policy/utils.ts (1)
  • isBeforeInvocation (158-160)
packages/runtime/src/utils/expression-utils.ts (1)
packages/sdk/src/schema/expression.ts (9)
  • LiteralExpression (12-15)
  • ArrayExpression (17-20)
  • FieldExpression (22-25)
  • MemberExpression (27-31)
  • BinaryExpression (39-44)
  • UnaryExpression (33-37)
  • CallExpression (46-50)
  • ThisExpression (52-54)
  • NullExpression (56-58)
packages/runtime/src/plugins/policy/utils.ts (1)
packages/runtime/src/schema/expression.ts (1)
  • ExpressionUtils (16-120)
🪛 markdownlint-cli2 (0.18.1)
TODO.md

104-104: Unordered list indentation
Expected: 2; Actual: 4

(MD007, ul-indent)

🪛 Biome (2.1.2)
packages/runtime/src/utils/expression-utils.ts

[error] 17-27: The function should not return a value because its return type is void.

The function is here:

'void' signals the absence of value. The returned value is likely to be ignored by the caller.

(lint/correctness/noVoidTypeReturn)

⏰ 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: build-test (20.x, sqlite)
  • GitHub Check: build-test (20.x, postgresql)
  • GitHub Check: claude-review
🔇 Additional comments (35)
packages/runtime/test/schemas/petstore/schema.ts (1)

95-95: Update policy now uses before(); matches intent

Condition enforces name/category unchanged and unsold state. Looks correct for the “only orderId may change” rule.

Confirm the runtime translator recognizes before() (e.g., isBeforeInvocation in policy utils) in this context and tests cover attempts to change name/category.

packages/runtime/src/client/contract.ts (1)

216-220: Approved: CRUD_EXT applied consistently
Verified that all downstream usages import and use CRUD_EXT, and no explicit 'create' | 'read' | 'update' | 'delete' unions remain outside the base type definition.

packages/runtime/test/schemas/petstore/schema.zmodel (1)

39-39: Approve migration to before()
No remaining future() usages detected; approving the switch to before().

packages/language/src/utils.ts (1)

449-451: Approve before() helper; verify no legacy future() or isFutureInvocation usage remains
Please manually confirm that all references to future() and isFutureInvocation have been removed (e.g. search .ts files under packages/**, excluding build artifacts).

packages/runtime/src/plugins/policy/plugin.ts (1)

25-28: Signature alignment with PolicyHandler.handle looks good

Dropping transaction and forwarding { query, client, proceed } matches handle(node, proceed). No other changes needed here.

If there are other RuntimePlugin implementations in the repo, please verify they’ve adopted the same OnKyselyQueryArgs shape to prevent runtime mismatches.

packages/language/src/zmodel-scope.ts (1)

173-175: Correct resolution for before() member access

Resolving before() to the containing model via createScopeForContainingModel is consistent with the new semantics.

packages/runtime/src/plugins/policy/types.ts (1)

1-13: Adopting CRUD_EXT for PolicyOperation is consistent with post-update

Type surface is updated as expected; PolicyOperation = CRUD_EXT | 'all' reads clean.

Please confirm that places expanding 'all' to concrete ops now include 'post-update' (e.g., any helper that normalizes operations uses CRUD_EXT constants).

packages/runtime/src/plugins/policy/utils.ts (1)

158-160: Utility isBeforeInvocation is simple and correct

Leverages ExpressionUtils.isCall and checks function name 'before'. Matches language-layer helper and keeps concerns local to runtime.

packages/runtime/src/client/options.ts (1)

4-4: Type surface updated to CRUD_EXT — looks right

Importing CRUD_EXT aligns this module with the new operation union including 'post-update'. No issues spotted.

packages/language/src/validators/expression-validator.ts (2)

14-16: Import additions are consistent with the new rule

Bringing in MemberAccessExpr and isBeforeInvocation is necessary for the added validation path. Looks good.

Also applies to: 22-26


64-67: No tests found for before() member-access edge cases
Found no existing test cases covering nested before() chains. Please add/confirm tests for:

  • before().user.id
  • before().users
  • (before()).user
packages/language/src/validators/attribute-application-validator.ts (1)

26-27: Import swap to isBeforeInvocation — aligned

Matches the repo-wide rename from future() to before(). No issues.

packages/runtime/src/client/crud/operations/base.ts (1)

1299-1302: Confirmed return shape matches consumer usage
Callers of updateMany(..., returnData=true) use the returned rows array (e.g., updateResult.map(...) for IDs), so returning result.rows is correct.

packages/runtime/src/plugins/policy/expression-transformer.ts (4)

23-23: Importing CRUD_EXT — LGTM

Contract type switch aligns with new operation. No concerns.


53-53: Import isBeforeInvocation — LGTM

Brings in the runtime-level predicate needed for before() handling.


58-58: Context.operation typed to CRUD_EXT — LGTM

Matches the widened operation domain.


455-459: auth() member access path — LGTM

Directly resolves against runtime auth object, consistent with existing semantics.

packages/runtime/test/policy/crud/post-update.test.ts (5)

4-19: Default allow semantics — LGTM

Confirms no explicit post-update rule ⇒ updates proceed.


20-46: Allow + Deny interplay — LGTM

Covers both rejection paths and state rollback verification.


65-95: Relational condition coverage — LGTM

Good check of post-update with nested relation delete altering the predicate domain.


129-168: Skipped QB test: track and unblock

Please open/link an issue for the “transaction issue,” and add a TODO with the issue number so it doesn’t get lost.


170-189: Negative: relation access from before() — LGTM

Asserts the precise error string; keep message consistent in validator.

If the validator message changes, update this test accordingly.

packages/language/res/stdlib.zmodel (2)

669-669: @@Allow completion hint updated — LGTM


687-687: @@deny completion hint updated — LGTM

packages/runtime/src/plugins/policy/policy-handler.ts (11)

114-119: LGTM! Good defensive programming for post-update policy check.

The conditional loading of before-update entities only when needed (i.e., when post-update policies exist) is an efficient approach that avoids unnecessary database queries.


226-243: Well-structured before-update data loading.

The method efficiently loads only the necessary fields for post-update policy evaluation. Good optimization to return early when no fields are needed.


252-260: Clean implementation of field collection using visitor pattern.

The anonymous class extending ExpressionVisitor is a nice use of the visitor pattern. The invariant check ensures before() is only used with scalar field access, which prevents misuse.


180-186: Correct validation of post-update policy compliance.

The count-based validation efficiently checks if all updated rows pass the post-update policy without needing to fetch all the data.


209-219: Important correction for mutation result shape.

This method correctly handles the case where the implementation adds a returning clause internally (for ID fields) but the original query didn't request any returned data. This maintains the expected API contract.


378-387: Well-documented returning clause logic for updates.

The comments clearly explain the two scenarios where ID fields are needed. The implementation correctly handles both cases.


426-435: Good handling of SELECT * edge cases.

The logic correctly handles the case where SELECT * is used on a model that only contains ID fields, avoiding unnecessary read-back operations.


721-721: Correct use of ReferenceNode for qualified column references.

Using ReferenceNode with both column and table ensures proper qualification in the generated SQL, preventing ambiguity in queries with joins.


785-810: Important semantic difference for post-update policies.

The implementation correctly treats post-update differently from other operations - it defaults to "allow" when no explicit allow rules exist, while other operations default to "deny". This aligns with the expected behavior where post-update policies are meant to validate after the fact rather than gate access.

The combination logic with deny rules is correctly implemented using buildIsFalse to negate conditions.


348-360: Unable to automatically verify returning behavior change for inserts
Manually review all insert query usages and associated tests to ensure that changing the returning clause to only include ID fields does not break existing client expectations.


892-896: Filter logic aligns with ZenStack docs
The ‘all’ operation covers only create/read/update/delete; post-update checks require explicit future() rules per the Post-Update Checks guide.

@claude
Copy link

claude bot commented Sep 29, 2025

Claude finished @ymc9's task —— View job


PR Review: Post-Update Policies ✅

This PR implements post-update policies, which allow validation rules to be applied after database updates occur. The implementation adds support for @@allow and @@deny rules with the postUpdate operation type.

Review Summary

Overall Assessment: GOOD - Well-implemented feature with comprehensive testing, but has some areas requiring attention.

Strengths

  1. Comprehensive Test Coverage (packages/runtime/test/policy/crud/post-update.test.ts:1)

    • Excellent test suite covering various scenarios including simple rules, relation conditions, and before() function usage
    • Tests include both positive and negative cases
    • Good edge case coverage (e.g., accessing relation fields from before())
  2. Clean Language Integration (packages/language/res/stdlib.zmodel:669,687)

    • Proper addition of 'post-update' to completion hints for @@allow and @@deny attributes
    • Consistent with existing operation types
  3. Robust Expression Validation (packages/language/src/validators/expression-validator.ts:74-76)

    • Added validation to prevent accessing relation fields from before() function
    • Clear error message: "relation fields cannot be accessed from before()"
  4. Well-Structured Policy Handler (packages/runtime/src/plugins/policy/policy-handler.ts:221-274)

    • Clean separation between pre-update loading and post-update validation
    • Proper handling of ID field requirements for joining with before-update rows

⚠️ Areas of Concern

  1. Performance Impact (packages/runtime/src/plugins/policy/policy-handler.ts:226-243)

    • The loadBeforeUpdateEntities method executes an additional SELECT query before every update
    • This could impact performance for frequent updates, especially with large datasets
    • Recommendation: Consider documenting performance implications or adding configuration options
  2. Complex Query Logic (packages/runtime/src/plugins/policy/policy-handler.ts:135-187)

    • The post-update validation involves complex SQL generation with VALUES tables and JOINs
    • The logic for creating before-update tables using ValuesNode.create is intricate
    • Risk: Complex SQL generation increases chances of edge case bugs
  3. Missing Error Context (packages/runtime/src/plugins/policy/policy-handler.ts:180-186)

    • Post-update policy failures provide generic error messages
    • Users may struggle to identify which specific entities or rules failed
    • Recommendation: Enhanced error messages with entity identifiers

🐛 Potential Issues

  1. Transaction Handling (packages/runtime/test/policy/crud/post-update.test.ts:129)

    • Test is skipped for query builder API due to "transaction issue"
    • This suggests potential problems with transaction isolation for post-update policies
    • Critical: This needs investigation as it could lead to data consistency issues
  2. Type Safety Gap (packages/runtime/src/plugins/policy/policy-handler.ts:153-157)

    const castedColumnRef = sql`CAST(${eb.ref(`column${index + 1}`)} as ${sql.raw(this.dialect.getFieldSqlType(def))})`.as(name);
    • Raw SQL injection for type casting could be vulnerable to SQL injection if field types aren't properly sanitized
    • Recommendation: Add validation for field type names
  3. Memory Usage (packages/runtime/src/plugins/policy/policy-handler.ts:241-242)

    • Before-update entities are loaded entirely into memory
    • For bulk updates, this could cause memory issues
    • Recommendation: Consider pagination or streaming for large result sets

🔒 Security Considerations

  1. Access Control Bypass (packages/runtime/src/plugins/policy/policy-handler.ts:790-792)

    • Post-update operations are allowed by default if no allow rules are defined
    • This differs from other operations which are denied by default
    • Security Risk: Could lead to unintended access if policies are misconfigured
  2. Expression Evaluation (packages/runtime/src/utils/expression-utils.ts:1)

    • New expression utilities for handling before() and future() references
    • Need to ensure proper validation to prevent injection attacks

🎯 Recommendations

  1. Address Transaction Issues (Priority: HIGH)

    • Investigate and fix the transaction handling problems indicated by the skipped test
    • This is critical for data consistency
  2. Performance Optimization (Priority: MEDIUM)

    • Add benchmarks for post-update policy performance
    • Consider lazy loading or caching strategies for before-update data
  3. Enhanced Error Messages (Priority: MEDIUM)

    • Include entity identifiers and specific rule failures in error messages
    • Add debugging information for complex policy failures
  4. Security Hardening (Priority: HIGH)

    • Review the default allow behavior for post-update operations
    • Add validation for SQL type casting operations
  5. Documentation (Priority: LOW)

    • Add performance considerations to documentation
    • Document the security implications of default allow behavior for post-update

Final Verdict

This is a well-architected feature with good test coverage. The main concerns are around transaction handling (which needs immediate attention) and performance implications for production usage. The code quality is good overall with appropriate error handling patterns.


Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between fd06893 and bc610f4.

📒 Files selected for processing (1)
  • packages/runtime/src/plugins/policy/policy-handler.ts (14 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
{packages,samples,tests}/**

📄 CodeRabbit inference engine (CLAUDE.md)

Place packages only under packages/, samples/, or tests/

Files:

  • packages/runtime/src/plugins/policy/policy-handler.ts
🧬 Code graph analysis (1)
packages/runtime/src/plugins/policy/policy-handler.ts (7)
packages/runtime/src/client/plugin.ts (1)
  • ProceedKyselyQueryFunction (195-195)
packages/runtime/src/client/query-utils.ts (3)
  • requireField (38-56)
  • requireIdFields (63-70)
  • requireModel (25-31)
packages/runtime/src/plugins/policy/utils.ts (6)
  • conjunction (49-67)
  • isBeforeInvocation (158-160)
  • trueNode (21-23)
  • falseNode (28-30)
  • disjunction (69-87)
  • buildIsFalse (127-139)
packages/runtime/src/utils/expression-utils.ts (1)
  • ExpressionVisitor (15-58)
packages/sdk/src/schema/expression.ts (1)
  • MemberExpression (27-31)
packages/common-helpers/src/tiny-invariant.ts (1)
  • invariant (4-14)
packages/runtime/src/client/contract.ts (2)
  • CRUD_EXT (219-219)
  • CRUD_EXT (229-229)
⏰ 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: build-test (20.x, postgresql)
  • GitHub Check: build-test (20.x, sqlite)
  • GitHub Check: claude-review

@claude
Copy link

claude bot commented Sep 29, 2025

Claude finished @ymc9's task —— View job


PR Review: Post-Update Policies ✅

This PR implements post-update policies, which allow validation rules to be applied after database updates occur. The implementation adds support for @@allow and @@deny rules with the post-update operation type.

Review Summary

Overall Assessment: GOOD - Well-implemented feature with comprehensive testing, but has some areas requiring attention.

Strengths

  1. Comprehensive Test Coverage (packages/runtime/test/policy/crud/post-update.test.ts:1)

    • Excellent test suite covering various scenarios including simple rules, relation conditions, and before() function usage
    • Tests include both positive and negative cases
    • Good edge case coverage (e.g., accessing relation fields from before())
  2. Clean Language Integration (packages/language/res/stdlib.zmodel:669,687)

    • Proper addition of 'post-update' to completion hints for @@allow and @@deny attributes
    • Consistent with existing operation types
  3. Robust Expression Validation (packages/language/src/validators/expression-validator.ts:74-76)

    • Added validation to prevent accessing relation fields from before() function
    • Clear error message: "relation fields cannot be accessed from before()"
  4. Well-Structured Policy Handler (packages/runtime/src/plugins/policy/policy-handler.ts:221-274)

    • Clean separation between pre-update loading and post-update validation
    • Proper handling of ID field requirements for joining with before-update rows

⚠️ Areas of Concern

  1. Performance Impact (packages/runtime/src/plugins/policy/policy-handler.ts:226-243)

    • The loadBeforeUpdateEntities method executes an additional SELECT query before every update
    • This could impact performance for frequent updates, especially with large datasets
    • Recommendation: Consider documenting performance implications or adding configuration options
  2. Complex Query Logic (packages/runtime/src/plugins/policy/policy-handler.ts:135-187)

    • The post-update validation involves complex SQL generation with VALUES tables and JOINs
    • The logic for creating before-update tables using ValuesNode.create is intricate
    • Risk: Complex SQL generation increases chances of edge case bugs
  3. Missing Error Context (packages/runtime/src/plugins/policy/policy-handler.ts:180-186)

    • Post-update policy failures provide generic error messages
    • Users may struggle to identify which specific entities or rules failed
    • Recommendation: Enhanced error messages with entity identifiers

🐛 Potential Issues

  1. Transaction Handling (packages/runtime/test/policy/crud/post-update.test.ts:129)

    • Test is skipped for query builder API due to "transaction issue"
    • This suggests potential problems with transaction isolation for post-update policies
    • Critical: This needs investigation as it could lead to data consistency issues
  2. Type Safety Gap (packages/runtime/src/plugins/policy/policy-handler.ts:153-157)

    const castedColumnRef = sql`CAST(${eb.ref(`column${index + 1}`)} as ${sql.raw(this.dialect.getFieldSqlType(def))})`.as(name);
    • Raw SQL injection for type casting could be vulnerable to SQL injection if field types aren't properly sanitized
    • Recommendation: Add validation for field type names
  3. Memory Usage (packages/runtime/src/plugins/policy/policy-handler.ts:241-242)

    • Before-update entities are loaded entirely into memory
    • For bulk updates, this could cause memory issues
    • Recommendation: Consider pagination or streaming for large result sets

🔒 Security Considerations

  1. Access Control Bypass (packages/runtime/src/plugins/policy/policy-handler.ts:790-792)

    • Post-update operations are allowed by default if no allow rules are defined
    • This differs from other operations which are denied by default
    • Security Risk: Could lead to unintended access if policies are misconfigured
  2. Expression Evaluation (packages/runtime/src/utils/expression-utils.ts:1)

    • New expression utilities for handling before() and future() references
    • Need to ensure proper validation to prevent injection attacks

🎯 Recommendations

  1. Address Transaction Issues (Priority: HIGH)

    • Investigate and fix the transaction handling problems indicated by the skipped test
    • This is critical for data consistency
  2. Performance Optimization (Priority: MEDIUM)

    • Add benchmarks for post-update policy performance
    • Consider lazy loading or caching strategies for before-update data
  3. Enhanced Error Messages (Priority: MEDIUM)

    • Include entity identifiers and specific rule failures in error messages
    • Add debugging information for complex policy failures
  4. Security Hardening (Priority: HIGH)

    • Review the default allow behavior for post-update operations
    • Add validation for SQL type casting operations
  5. Documentation (Priority: LOW)

    • Add performance considerations to documentation
    • Document the security implications of default allow behavior for post-update

Final Verdict

This is a well-architected feature with good test coverage. The main concerns are around transaction handling (which needs immediate attention) and performance implications for production usage. The code quality is good overall with appropriate error handling patterns.


@claude
Copy link

claude bot commented Sep 29, 2025

Claude finished @ymc9's task —— View job


PR Review: Post-Update Policies ✅

This PR implements post-update policies, which allow validation rules to be applied after database updates occur. The implementation adds support for @@allow and @@deny rules with the post-update operation type and replaces future() with before() for accessing pre-update values.

Review Progress

  • Read and understand the implementation changes
  • Analyze core policy handler logic
  • Review test coverage and quality
  • Check for security concerns
  • Assess performance implications
  • Provide comprehensive feedback

Overall Assessment: GOOD

Well-architected feature with comprehensive testing, though there are some critical areas requiring attention.

Strengths

1. Comprehensive Test Coverage (packages/runtime/test/policy/crud/post-update.test.ts:1)

  • Excellent test suite covering various scenarios including simple rules, relation conditions, and before() function usage
  • Tests include both positive and negative cases
  • Good edge case coverage (e.g., accessing relation fields from before())

2. Clean Language Integration (packages/language/res/stdlib.zmodel:669,687)

  • Proper addition of 'post-update' to completion hints for @@allow and @@deny attributes
  • Consistent with existing operation types

3. Robust Expression Validation (packages/language/src/validators/expression-validator.ts:74-76)

  • Added validation to prevent accessing relation fields from before() function
  • Clear error message: "relation fields cannot be accessed from before()"

4. Well-Structured Policy Handler (packages/runtime/src/plugins/policy/policy-handler.ts:221-274)

  • Clean separation between pre-update loading and post-update validation
  • Proper handling of ID field requirements for joining with before-update rows

⚠️ Areas of Concern

1. Performance Impact (packages/runtime/src/plugins/policy/policy-handler.ts:226-243)

  • The loadBeforeUpdateEntities method executes an additional SELECT query before every update
  • This could impact performance for frequent updates, especially with large datasets
  • Recommendation: Consider documenting performance implications or adding configuration options

2. Complex Query Logic (packages/runtime/src/plugins/policy/policy-handler.ts:135-187)

  • The post-update validation involves complex SQL generation with VALUES tables and JOINs
  • The logic for creating before-update tables using ValuesNode.create is intricate
  • Risk: Complex SQL generation increases chances of edge case bugs

3. Missing Error Context (packages/runtime/src/plugins/policy/policy-handler.ts:180-186)

  • Post-update policy failures provide generic error messages
  • Users may struggle to identify which specific entities or rules failed
  • Recommendation: Enhanced error messages with entity identifiers

🐛 Critical Issues

1. Transaction Handling (packages/runtime/test/policy/crud/post-update.test.ts:129)

  • Test is skipped for query builder API due to "transaction issue"
  • This suggests potential problems with transaction isolation for post-update policies
  • Critical: This needs investigation as it could lead to data consistency issues

2. Runtime Validation Gap (packages/runtime/src/plugins/policy/expression-transformer.ts:464-467)

  • No runtime check that before() is only used in post-update policies
  • If language validation is bypassed, this could cause SQL errors
  • Recommendation: Add runtime assertion in expression transformer

3. Memory Usage (packages/runtime/src/plugins/policy/policy-handler.ts:241-242)

  • Before-update entities are loaded entirely into memory
  • For bulk updates, this could cause memory issues
  • Recommendation: Consider pagination or streaming for large result sets

🔒 Security Considerations

1. Access Control Bypass (packages/runtime/src/plugins/policy/policy-handler.ts:790-792)

  • Post-update operations are allowed by default if no allow rules are defined
  • This differs from other operations which are denied by default
  • Security Risk: Could lead to unintended access if policies are misconfigured

2. SQL Type Casting (packages/runtime/src/plugins/policy/policy-handler.ts:153-157)

const castedColumnRef = sql`CAST(${eb.ref(`column${index + 1}`)} as ${sql.raw(this.dialect.getFieldSqlType(def))})`.as(name);
  • Raw SQL injection for type casting could be vulnerable if field types aren't properly sanitized
  • Recommendation: Add validation for field type names

📊 Code Quality

Good Practices

  • ✅ Proper use of visitor pattern for expression traversal
  • ✅ Clean separation of concerns between language and runtime layers
  • ✅ Comprehensive type safety with CRUD_EXT type
  • ✅ Good use of invariants for runtime checks

Areas for Improvement

  • ⚠️ Complex SQL generation could benefit from more documentation
  • ⚠️ Error messages could be more user-friendly
  • ⚠️ Performance characteristics should be documented

🎯 Recommendations

Priority: HIGH

  1. Address Transaction Issues: Investigate and fix the transaction handling problems indicated by the skipped test
  2. Add Runtime Guards: Include operation validation in expression transformer
  3. Security Review: Review the default allow behavior for post-update operations

Priority: MEDIUM

  1. Performance Optimization: Add benchmarks for post-update policy performance
  2. Enhanced Error Messages: Include entity identifiers and specific rule failures
  3. Memory Management: Consider streaming for large result sets

Priority: LOW

  1. Documentation: Add performance considerations and security implications to docs

Final Verdict

This is a well-implemented feature with good test coverage and clean architecture. The main concerns are around transaction handling (which needs immediate attention) and performance implications for production usage. The code quality is good overall with appropriate error handling patterns.

Recommendation: Approve with required fixes for transaction handling and runtime validation gaps.


@claude
Copy link

claude bot commented Sep 29, 2025

Claude finished @ymc9's task —— View job


PR Review: Post-Update Policies ✅

This PR implements post-update policies, which allow validation rules to be applied after database updates occur. The implementation adds support for @@allow and @@deny rules with the post-update operation type and replaces future() with before() for accessing pre-update values.

Review Progress

  • Read and understand the implementation changes
  • Analyze core policy handler logic
  • Review test coverage and quality
  • Check for security concerns
  • Assess performance implications
  • Provide comprehensive feedback

Overall Assessment: GOOD

Well-architected feature with comprehensive testing, though there are some critical areas requiring attention.

Strengths

1. Comprehensive Test Coverage (packages/runtime/test/policy/crud/post-update.test.ts:1)

  • Excellent test suite covering various scenarios including simple rules, relation conditions, and before() function usage
  • Tests include both positive and negative cases
  • Good edge case coverage (e.g., accessing relation fields from before())

2. Clean Language Integration (packages/language/res/stdlib.zmodel:669,687)

  • Proper addition of 'post-update' to completion hints for @@allow and @@deny attributes
  • Consistent with existing operation types

3. Robust Expression Validation (packages/language/src/validators/expression-validator.ts:74-76)

  • Added validation to prevent accessing relation fields from before() function
  • Clear error message: "relation fields cannot be accessed from before()"

4. Well-Structured Policy Handler (packages/runtime/src/plugins/policy/policy-handler.ts:221-274)

  • Clean separation between pre-update loading and post-update validation
  • Proper handling of ID field requirements for joining with before-update rows

⚠️ Areas of Concern

1. Performance Impact (packages/runtime/src/plugins/policy/policy-handler.ts:226-243)

  • The loadBeforeUpdateEntities method executes an additional SELECT query before every update
  • This could impact performance for frequent updates, especially with large datasets
  • Recommendation: Consider documenting performance implications or adding configuration options

2. Complex Query Logic (packages/runtime/src/plugins/policy/policy-handler.ts:135-187)

  • The post-update validation involves complex SQL generation with VALUES tables and JOINs
  • The logic for creating before-update tables using ValuesNode.create is intricate
  • Risk: Complex SQL generation increases chances of edge case bugs

3. Missing Error Context (packages/runtime/src/plugins/policy/policy-handler.ts:180-186)

  • Post-update policy failures provide generic error messages
  • Users may struggle to identify which specific entities or rules failed
  • Recommendation: Enhanced error messages with entity identifiers

🐛 Critical Issues

1. Transaction Handling (packages/runtime/test/policy/crud/post-update.test.ts:129)

  • Test is skipped for query builder API due to "transaction issue"
  • This suggests potential problems with transaction isolation for post-update policies
  • Critical: This needs investigation as it could lead to data consistency issues

2. Runtime Validation Gap (packages/runtime/src/plugins/policy/expression-transformer.ts:464-467)

  • No runtime check that before() is only used in post-update policies
  • If language validation is bypassed, this could cause SQL errors
  • Recommendation: Add runtime assertion in expression transformer

3. Memory Usage (packages/runtime/src/plugins/policy/policy-handler.ts:241-242)

  • Before-update entities are loaded entirely into memory
  • For bulk updates, this could cause memory issues
  • Recommendation: Consider pagination or streaming for large result sets

🔒 Security Considerations

1. Access Control Bypass (packages/runtime/src/plugins/policy/policy-handler.ts:790-792)

  • Post-update operations are allowed by default if no allow rules are defined
  • This differs from other operations which are denied by default
  • Security Risk: Could lead to unintended access if policies are misconfigured

2. SQL Type Casting (packages/runtime/src/plugins/policy/policy-handler.ts:153-157)

const castedColumnRef = sql`CAST(${eb.ref(`column${index + 1}`)} as ${sql.raw(this.dialect.getFieldSqlType(def))})`.as(name);
  • Raw SQL injection for type casting could be vulnerable if field types aren't properly sanitized
  • Recommendation: Add validation for field type names

📊 Code Quality

Good Practices

  • ✅ Proper use of visitor pattern for expression traversal
  • ✅ Clean separation of concerns between language and runtime layers
  • ✅ Comprehensive type safety with CRUD_EXT type
  • ✅ Good use of invariants for runtime checks

Areas for Improvement

  • ⚠️ Complex SQL generation could benefit from more documentation
  • ⚠️ Error messages could be more user-friendly
  • ⚠️ Performance characteristics should be documented

🎯 Recommendations

Priority: HIGH

  1. Address Transaction Issues: Investigate and fix the transaction handling problems indicated by the skipped test
  2. Add Runtime Guards: Include operation validation in expression transformer
  3. Security Review: Review the default allow behavior for post-update operations

Priority: MEDIUM

  1. Performance Optimization: Add benchmarks for post-update policy performance
  2. Enhanced Error Messages: Include entity identifiers and specific rule failures
  3. Memory Management: Consider streaming for large result sets

Priority: LOW

  1. Documentation: Add performance considerations and security implications to docs

Final Verdict

This is a well-implemented feature with good test coverage and clean architecture. The main concerns are around transaction handling (which needs immediate attention) and performance implications for production usage. The code quality is good overall with appropriate error handling patterns.

Recommendation: Approve with required fixes for transaction handling and runtime validation gaps.


@ymc9 ymc9 merged commit 198c352 into dev Sep 29, 2025
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants