chore: disallow this in collection predicate (#1176)

zenstackhq · Mar 25, 2024 · 6e9d242 · 6e9d242
1 parent 35b4be9
commit 6e9d242
Show file tree

Hide file tree

Showing 4 changed files with 51 additions and 6 deletions.
diff --git a/packages/schema/src/language-server/validator/expression-validator.ts b/packages/schema/src/language-server/validator/expression-validator.ts
@@ -13,8 +13,8 @@ import {
     isThisExpr,
 } from '@zenstackhq/language/ast';
 import { isAuthInvocation, isDataModelFieldReference, isEnumFieldReference } from '@zenstackhq/sdk';
-import { ValidationAcceptor } from 'langium';
-import { findUpAst, getContainingDataModel, isCollectionPredicate } from '../../utils/ast-utils';
+import { ValidationAcceptor, streamAst } from 'langium';
+import { findUpAst, getContainingDataModel } from '../../utils/ast-utils';
 import { AstValidator } from '../types';
 import { typeAssignable } from './utils';
 
@@ -32,8 +32,6 @@ export default class ExpressionValidator implements AstValidator<Expression> {
                     'auth() cannot be resolved because no model marked wth "@@auth()" or named "User" is found',
                     { node: expr }
                 );
-            } else if (isCollectionPredicate(expr)) {
-                accept('error', 'collection predicate can only be used on an array of model type', { node: expr });
             } else {
                 accept('error', 'expression cannot be resolved', {
                     node: expr,
@@ -221,6 +219,29 @@ export default class ExpressionValidator implements AstValidator<Expression> {
                 }
                 break;
             }
+
+            case '?':
+            case '!':
+            case '^':
+                this.validateCollectionPredicate(expr, accept);
+                break;
+        }
+    }
+
+    private validateCollectionPredicate(expr: BinaryExpr, accept: ValidationAcceptor) {
+        if (!expr.$resolvedType) {
+            accept('error', 'collection predicate can only be used on an array of model type', { node: expr });
+            return;
+        }
+
+        // TODO: revisit this when we implement lambda inside collection predicate
+        const thisExpr = streamAst(expr).find(isThisExpr);
+        if (thisExpr) {
+            accept(
+                'error',
+                'using `this` in collection predicate is not supported. To compare entity identity, use id field comparison instead.',
+                { node: thisExpr }
+            );
         }
     }
 

diff --git a/packages/schema/tests/schema/validation/datamodel-validation.test.ts b/packages/schema/tests/schema/validation/datamodel-validation.test.ts
@@ -78,6 +78,29 @@ describe('Data Model Validation Tests', () => {
         ).toMatchObject(errorLike('Field of "Unsupported" type cannot be used in expressions'));
     });
 
+    it('Using `this` in collection predicate', async () => {
+        expect(
+            await safelyLoadModel(`
+            ${prelude}
+            model User {
+                id String @id
+                members User[]
+                @@allow('all', members?[this == auth()])
+            }
+        `)
+        ).toMatchObject(errorLike('using `this` in collection predicate is not supported'));
+
+        expect(
+            await loadModel(`
+        model User {
+            id String @id
+            members User[]
+            @@allow('all', members?[id == auth().id])
+        }
+        `)
+        ).toBeTruthy();
+    });
+
     it('mix array and optional', async () => {
         expect(
             await safelyLoadModel(`

diff --git a/tests/integration/tests/regression/issue-925.test.ts b/tests/integration/tests/regression/issue-925.test.ts
@@ -35,7 +35,8 @@ describe('Regression: issue 925', () => {
         ).resolves.toContain("Could not resolve reference to ReferenceTarget named 'test'.");
     });
 
-    it('reference with this', async () => {
+    // eslint-disable-next-line jest/no-disabled-tests
+    it.skip('reference with this', async () => {
         await loadModel(
             `
             model User {

diff --git a/tests/integration/tests/regression/issues.test.ts b/tests/integration/tests/regression/issues.test.ts
@@ -600,7 +600,7 @@ model User {
     // can be created by anyone, even not logged in
     @@allow('create', true)
     // can be read by users in the same organization
-    @@allow('read', orgs?[members?[auth() == this]])
+    @@allow('read', orgs?[members?[auth().id == id]])
     // full access by oneself
     @@allow('all', auth() == this)
 }