fix: post-update rules incorrectly reject update (#826)

ymc9 · ymc9 · commit d921a7ca6bef · 2023-11-13T16:34:12.000-08:00
diff --git a/package.json b/package.json
@@ -1,6 +1,6 @@
 {
     "name": "zenstack-monorepo",
-    "version": "1.2.1",
+    "version": "1.2.2",
     "description": "",
     "scripts": {
         "build": "pnpm -r build",
diff --git a/packages/language/package.json b/packages/language/package.json
@@ -1,6 +1,6 @@
 {
     "name": "@zenstackhq/language",
-    "version": "1.2.1",
+    "version": "1.2.2",
     "displayName": "ZenStack modeling language compiler",
     "description": "ZenStack modeling language compiler",
     "homepage": "https://zenstack.dev",
diff --git a/packages/plugins/openapi/package.json b/packages/plugins/openapi/package.json
@@ -1,7 +1,7 @@
 {
   "name": "@zenstackhq/openapi",
   "displayName": "ZenStack Plugin and Runtime for OpenAPI",
-  "version": "1.2.1",
+  "version": "1.2.2",
   "description": "ZenStack plugin and runtime supporting OpenAPI",
   "main": "index.js",
   "repository": {
diff --git a/packages/plugins/swr/package.json b/packages/plugins/swr/package.json
@@ -1,7 +1,7 @@
 {
     "name": "@zenstackhq/swr",
     "displayName": "ZenStack plugin for generating SWR hooks",
-    "version": "1.2.1",
+    "version": "1.2.2",
     "description": "ZenStack plugin for generating SWR hooks",
     "main": "index.js",
     "repository": {
diff --git a/packages/plugins/tanstack-query/package.json b/packages/plugins/tanstack-query/package.json
@@ -1,7 +1,7 @@
 {
     "name": "@zenstackhq/tanstack-query",
     "displayName": "ZenStack plugin for generating tanstack-query hooks",
-    "version": "1.2.1",
+    "version": "1.2.2",
     "description": "ZenStack plugin for generating tanstack-query hooks",
     "main": "index.js",
     "exports": {
diff --git a/packages/plugins/trpc/package.json b/packages/plugins/trpc/package.json
@@ -1,7 +1,7 @@
 {
     "name": "@zenstackhq/trpc",
     "displayName": "ZenStack plugin for tRPC",
-    "version": "1.2.1",
+    "version": "1.2.2",
     "description": "ZenStack plugin for tRPC",
     "main": "index.js",
     "repository": {
diff --git a/packages/runtime/package.json b/packages/runtime/package.json
@@ -1,7 +1,7 @@
 {
     "name": "@zenstackhq/runtime",
     "displayName": "ZenStack Runtime Library",
-    "version": "1.2.1",
+    "version": "1.2.2",
     "description": "Runtime of ZenStack for both client-side and server-side environments.",
     "repository": {
         "type": "git",
diff --git a/packages/schema/package.json b/packages/schema/package.json
@@ -3,7 +3,7 @@
     "publisher": "zenstack",
     "displayName": "ZenStack Language Tools",
     "description": "A toolkit for building secure CRUD apps with Next.js + Typescript",
-    "version": "1.2.1",
+    "version": "1.2.2",
     "author": {
         "name": "ZenStack Team"
     },
diff --git a/packages/schema/src/plugins/access-policy/expression-writer.ts b/packages/schema/src/plugins/access-policy/expression-writer.ts
@@ -223,15 +223,28 @@ export class ExpressionWriter {
     }
 
     private writeCollectionPredicate(expr: BinaryExpr, operator: string) {
-        this.block(() => {
-            this.writeFieldCondition(
-                expr.left,
-                () => {
-                    this.write(expr.right);
-                },
-                operator === '?' ? 'some' : operator === '!' ? 'every' : 'none'
-            );
-        });
+        // check if the operand should be compiled to a relation query
+        // or a plain expression
+        const compileToRelationQuery =
+            (this.isPostGuard && this.isFutureMemberAccess(expr.left)) ||
+            (!this.isPostGuard && !this.isFutureMemberAccess(expr.left));
+
+        if (compileToRelationQuery) {
+            this.block(() => {
+                this.writeFieldCondition(
+                    expr.left,
+                    () => {
+                        // inner scope of collection expression is always compiled as non-post-guard
+                        const innerWriter = new ExpressionWriter(this.writer, false);
+                        innerWriter.write(expr.right);
+                    },
+                    operator === '?' ? 'some' : operator === '!' ? 'every' : 'none'
+                );
+            });
+        } else {
+            const plain = this.plainExprBuilder.transform(expr);
+            this.writer.write(`${plain} ? ${TRUE} : ${FALSE}`);
+        }
     }
 
     private isFieldAccess(expr: Expression): boolean {
@@ -275,6 +288,19 @@ export class ExpressionWriter {
         }
     }
 
+    private writeIdFieldsCheck(model: DataModel, value: Expression) {
+        const idFields = this.requireIdFields(model);
+        idFields.forEach((idField, idx) => {
+            // eg: id: user.id
+            this.writer.write(`${idField.name}:`);
+            this.plain(value);
+            this.writer.write(`.${idField.name}`);
+            if (idx !== idFields.length - 1) {
+                this.writer.write(',');
+            }
+        });
+    }
+
     private writeComparison(expr: BinaryExpr, operator: ComparisonOperator) {
         const leftIsFieldAccess = this.isFieldAccess(expr.left);
         const rightIsFieldAccess = this.isFieldAccess(expr.right);
@@ -298,7 +324,7 @@ export class ExpressionWriter {
             operator = this.negateOperator(operator);
         }
 
-        if (isMemberAccessExpr(fieldAccess) && isFutureExpr(fieldAccess.operand)) {
+        if (this.isFutureMemberAccess(fieldAccess)) {
             // future().field should be treated as the "field" directly, so we
             // strip 'future().' and synthesize a reference expr
             fieldAccess = {
@@ -338,8 +364,6 @@ export class ExpressionWriter {
                             // right now this branch only serves comparison with `auth`, like
                             //     @@allow('all', owner == auth())
 
-                            const idFields = this.requireIdFields(dataModel);
-
                             if (operator !== '==' && operator !== '!=') {
                                 throw new PluginError(name, 'Only == and != operators are allowed');
                             }
@@ -354,25 +378,13 @@ export class ExpressionWriter {
                             }
 
                             this.block(() => {
-                                idFields.forEach((idField, idx) => {
-                                    const writeIdsCheck = () => {
-                                        // id: user.id
-                                        this.writer.write(`${idField.name}:`);
-                                        this.plain(operand);
-                                        this.writer.write(`.${idField.name}`);
-                                        if (idx !== idFields.length - 1) {
-                                            this.writer.write(',');
-                                        }
-                                    };
-
-                                    if (isThisExpr(fieldAccess) && operator === '!=') {
-                                        // wrap a not
-                                        this.writer.writeLine('NOT:');
-                                        this.block(() => writeIdsCheck());
-                                    } else {
-                                        writeIdsCheck();
-                                    }
-                                });
+                                if (isThisExpr(fieldAccess) && operator === '!=') {
+                                    // negate
+                                    this.writer.writeLine('isNot:');
+                                    this.block(() => this.writeIdFieldsCheck(dataModel, operand));
+                                } else {
+                                    this.writeIdFieldsCheck(dataModel, operand);
+                                }
                             });
                         } else {
                             if (this.equivalentRefs(fieldAccess, operand)) {
@@ -386,7 +398,13 @@ export class ExpressionWriter {
                                         // we should generate a field reference (comparing fields in the same model)
                                         this.writeFieldReference(operand);
                                     } else {
-                                        this.plain(operand);
+                                        if (dataModel && this.isModelTyped(operand)) {
+                                            // the comparison is between model types, generate id fields comparison block
+                                            this.block(() => this.writeIdFieldsCheck(dataModel, operand));
+                                        } else {
+                                            // scalar value, just generate the plain expression
+                                            this.plain(operand);
+                                        }
                                     }
                                 });
                             }
@@ -400,6 +418,10 @@ export class ExpressionWriter {
         );
     }
 
+    private isFutureMemberAccess(expr: Expression): expr is MemberAccessExpr {
+        return isMemberAccessExpr(expr) && isFutureExpr(expr.operand);
+    }
+
     private requireIdFields(dataModel: DataModel) {
         const idFields = getIdFields(dataModel);
         if (!idFields || idFields.length === 0) {
diff --git a/packages/schema/src/plugins/access-policy/policy-guard-generator.ts b/packages/schema/src/plugins/access-policy/policy-guard-generator.ts
@@ -207,9 +207,16 @@ export default class PolicyGenerator {
     }
 
     private processUpdatePolicies(expressions: Expression[], postUpdate: boolean) {
-        return expressions
-            .map((expr) => this.visitPolicyExpression(expr, postUpdate))
-            .filter((e): e is Expression => !!e);
+        const hasFutureReference = expressions.some((expr) => this.hasFutureReference(expr));
+        if (postUpdate) {
+            // when compiling post-update rules, if any rule contains `future()` reference,
+            // we include all as post-update rules
+            return hasFutureReference ? expressions : [];
+        } else {
+            // when compiling pre-update rules, if any rule contains `future()` reference,
+            // we completely skip pre-update check and defer them to post-update
+            return hasFutureReference ? [] : expressions;
+        }
     }
 
     private visitPolicyExpression(expr: Expression, postUpdate: boolean): Expression | undefined {
@@ -543,6 +550,9 @@ export default class PolicyGenerator {
                 } else {
                     return [];
                 }
+            } else if (isInvocationExpr(expr)) {
+                // recurse into function arguments
+                return expr.args.flatMap((arg) => collectReferencePaths(arg.value));
             } else {
                 // recurse
                 const children = streamContents(expr)
diff --git a/packages/schema/src/utils/typescript-expression-transformer.ts b/packages/schema/src/utils/typescript-expression-transformer.ts
@@ -5,6 +5,7 @@ import {
     DataModel,
     Expression,
     InvocationExpr,
+    isDataModel,
     isEnumField,
     isThisExpr,
     LiteralExpr,
@@ -29,6 +30,7 @@ export class TypeScriptExpressionTransformerError extends Error {
 type Options = {
     isPostGuard?: boolean;
     fieldReferenceContext?: string;
+    thisExprContext?: string;
     context: ExpressionContext;
 };
 
@@ -99,7 +101,7 @@ export class TypeScriptExpressionTransformer {
 
     private this(_expr: ThisExpr) {
         // "this" is mapped to the input argument
-        return 'input';
+        return this.options.thisExprContext ?? 'input';
     }
 
     private memberAccess(expr: MemberAccessExpr, normalizeUndefined: boolean) {
@@ -302,11 +304,19 @@ export class TypeScriptExpressionTransformer {
         return `(${expr.operator} ${this.transform(expr.operand, normalizeUndefined)})`;
     }
 
+    private isModelType(expr: Expression) {
+        return isDataModel(expr.$resolvedType?.decl);
+    }
+
     private binary(expr: BinaryExpr, normalizeUndefined: boolean): string {
-        const _default = `(${this.transform(expr.left, normalizeUndefined)} ${expr.operator} ${this.transform(
-            expr.right,
-            normalizeUndefined
-        )})`;
+        let left = this.transform(expr.left, normalizeUndefined);
+        let right = this.transform(expr.right, normalizeUndefined);
+        if (this.isModelType(expr.left) && this.isModelType(expr.right)) {
+            // comparison between model type values, map to id comparison
+            left = `(${left}?.id ?? null)`;
+            right = `(${right}?.id ?? null)`;
+        }
+        const _default = `(${left} ${expr.operator} ${right})`;
 
         return match(expr.operator)
             .with(
@@ -346,7 +356,9 @@ export class TypeScriptExpressionTransformer {
         const operand = this.transform(expr.left, normalizeUndefined);
         const innerTransformer = new TypeScriptExpressionTransformer({
             ...this.options,
+            isPostGuard: false,
             fieldReferenceContext: '_item',
+            thisExprContext: '_item',
         });
         const predicate = innerTransformer.transform(expr.right, normalizeUndefined);
 
diff --git a/packages/sdk/package.json b/packages/sdk/package.json
@@ -1,6 +1,6 @@
 {
     "name": "@zenstackhq/sdk",
-    "version": "1.2.1",
+    "version": "1.2.2",
     "description": "ZenStack plugin development SDK",
     "main": "index.js",
     "scripts": {
diff --git a/packages/server/package.json b/packages/server/package.json
@@ -1,6 +1,6 @@
 {
     "name": "@zenstackhq/server",
-    "version": "1.2.1",
+    "version": "1.2.2",
     "displayName": "ZenStack Server-side Adapters",
     "description": "ZenStack server-side adapters",
     "homepage": "https://zenstack.dev",
diff --git a/packages/testtools/package.json b/packages/testtools/package.json
@@ -1,6 +1,6 @@
 {
   "name": "@zenstackhq/testtools",
-  "version": "1.2.1",
+  "version": "1.2.2",
   "description": "ZenStack Test Tools",
   "main": "index.js",
   "private": true,
diff --git a/packages/testtools/src/schema.ts b/packages/testtools/src/schema.ts
@@ -84,8 +84,19 @@ generator js {
     previewFeatures = ['clientExtensions']
 }
 
+plugin meta {
+    provider = '@core/model-meta'
+    preserveTsFiles = true
+}
+
+plugin policy {
+    provider = '@core/access-policy'
+    preserveTsFiles = true
+}
+
 plugin zod {
     provider = '@core/zod'
+    preserveTsFiles = true
     modelOnly = ${!options.fullZod}
 }
 `;
diff --git a/tests/integration/tests/enhancements/with-policy/post-update.test.ts b/tests/integration/tests/enhancements/with-policy/post-update.test.ts
diff --git a/tests/integration/tests/enhancements/with-policy/refactor.test.ts b/tests/integration/tests/enhancements/with-policy/refactor.test.ts
diff --git a/tests/integration/tests/regression/issue-814.test.ts b/tests/integration/tests/regression/issue-814.test.ts
diff --git a/tests/integration/tests/regression/issue-825.test.ts b/tests/integration/tests/regression/issue-825.test.ts
diff --git a/tests/integration/tests/schema/todo.zmodel b/tests/integration/tests/schema/todo.zmodel

Original file line number	Diff line number	Diff line change
`@@ -1,6 +1,6 @@`
`1`	`1`	`{`
`2`	`2`	`"name": "zenstack-monorepo",`
`3`		`- "version": "1.2.1",`
	`3`	`+ "version": "1.2.2",`
`4`	`4`	`"description": "",`
`5`	`5`	`"scripts": {`
`6`	`6`	`"build": "pnpm -r build",`
Original file line number	Diff line number	Diff line change
`@@ -1,6 +1,6 @@`
`1`	`1`	`{`
`2`	`2`	`"name": "@zenstackhq/language",`
`3`		`- "version": "1.2.1",`
	`3`	`+ "version": "1.2.2",`
`4`	`4`	`"displayName": "ZenStack modeling language compiler",`
`5`	`5`	`"description": "ZenStack modeling language compiler",`
`6`	`6`	`"homepage": "https://zenstack.dev",`
Original file line number	Diff line number	Diff line change
`@@ -1,7 +1,7 @@`
`1`	`1`	`{`
`2`	`2`	`"name": "@zenstackhq/openapi",`
`3`	`3`	`"displayName": "ZenStack Plugin and Runtime for OpenAPI",`
`4`		`- "version": "1.2.1",`
	`4`	`+ "version": "1.2.2",`
`5`	`5`	`"description": "ZenStack plugin and runtime supporting OpenAPI",`
`6`	`6`	`"main": "index.js",`
`7`	`7`	`"repository": {`
Original file line number	Diff line number	Diff line change
`@@ -1,7 +1,7 @@`
`1`	`1`	`{`
`2`	`2`	`"name": "@zenstackhq/swr",`
`3`	`3`	`"displayName": "ZenStack plugin for generating SWR hooks",`
`4`		`- "version": "1.2.1",`
	`4`	`+ "version": "1.2.2",`
`5`	`5`	`"description": "ZenStack plugin for generating SWR hooks",`
`6`	`6`	`"main": "index.js",`
`7`	`7`	`"repository": {`
Original file line number	Diff line number	Diff line change
`@@ -1,7 +1,7 @@`
`1`	`1`	`{`
`2`	`2`	`"name": "@zenstackhq/tanstack-query",`
`3`	`3`	`"displayName": "ZenStack plugin for generating tanstack-query hooks",`
`4`		`- "version": "1.2.1",`
	`4`	`+ "version": "1.2.2",`
`5`	`5`	`"description": "ZenStack plugin for generating tanstack-query hooks",`
`6`	`6`	`"main": "index.js",`
`7`	`7`	`"exports": {`
Original file line number	Diff line number	Diff line change
`@@ -1,7 +1,7 @@`
`1`	`1`	`{`
`2`	`2`	`"name": "@zenstackhq/trpc",`
`3`	`3`	`"displayName": "ZenStack plugin for tRPC",`
`4`		`- "version": "1.2.1",`
	`4`	`+ "version": "1.2.2",`
`5`	`5`	`"description": "ZenStack plugin for tRPC",`
`6`	`6`	`"main": "index.js",`
`7`	`7`	`"repository": {`
Original file line number	Diff line number	Diff line change
`@@ -1,7 +1,7 @@`
`1`	`1`	`{`
`2`	`2`	`"name": "@zenstackhq/runtime",`
`3`	`3`	`"displayName": "ZenStack Runtime Library",`
`4`		`- "version": "1.2.1",`
	`4`	`+ "version": "1.2.2",`
`5`	`5`	`"description": "Runtime of ZenStack for both client-side and server-side environments.",`
`6`	`6`	`"repository": {`
`7`	`7`	`"type": "git",`