fix: incorrect validation errors for abstract models with validation rules

packages/schema/src/language-server/validator/datamodel-validator.ts

@@ -6,7 +6,7 @@ import {
     isStringLiteral,
     ReferenceExpr,
 } from '@zenstackhq/language/ast';
-import { analyzePolicies, getLiteral, getModelIdFields, getModelUniqueFields } from '@zenstackhq/sdk';
+import { getLiteral, getModelIdFields, getModelUniqueFields } from '@zenstackhq/sdk';
 import { AstNode, DiagnosticInfo, getDocument, ValidationAcceptor } from 'langium';
 import { IssueCodes, SCALAR_TYPES } from '../constants';
 import { AstValidator } from '../types';
@@ -34,23 +34,19 @@ export default class DataModelValidator implements AstValidator<DataModel> {
         const modelUniqueFields = getModelUniqueFields(dm);
 
         if (
+            !dm.isAbstract &&
             idFields.length === 0 &&
             modelLevelIds.length === 0 &&
             uniqueFields.length === 0 &&
             modelUniqueFields.length === 0
         ) {
-            const { allows, denies, hasFieldValidation } = analyzePolicies(dm);
-            if (allows.length > 0 || denies.length > 0 || hasFieldValidation) {
-                // TODO: relax this requirement to require only @unique fields
-                // when access policies or field valdaition is used, require an @id field
-                accept(
-                    'error',
-                    'Model must include a field with @id or @unique attribute, or a model-level @@id or @@unique attribute to use access policies',
-                    {
-                        node: dm,
-                    }
-                );
-            }
+            accept(
+                'error',
+                'Model must have at least one unique criteria. Either mark a single field with `@id`, `@unique` or add a multi field criterion with `@@id([])` or `@@unique([])` to the model.',
+                {
+                    node: dm,
+                }
+            );
         } else if (idFields.length > 0 && modelLevelIds.length > 0) {
             accept('error', 'Model cannot have both field-level @id and model-level @@id attributes', {
                 node: dm,

packages/schema/tests/schema/validation/attribute-validation.test.ts

@@ -251,6 +251,7 @@ describe('Attribute tests', () => {
             ${prelude}
 
             model _String {
+                id String @id
                 _string String @db.String
                 _string1 String @db.String(1)
                 _text String @db.Text
@@ -275,6 +276,7 @@ describe('Attribute tests', () => {
             }
 
             model _Boolean {
+                id String @id
                 _boolean Boolean @db.Boolean
                 _bit Boolean @db.Bit
                 _bit1 Boolean @db.Bit(1)
@@ -283,6 +285,7 @@ describe('Attribute tests', () => {
             }
 
             model _Int {
+                id String @id
                 _int Int @db.Int
                 _integer Int @db.Integer
                 _smallInt Int @db.SmallInt
@@ -298,12 +301,14 @@ describe('Attribute tests', () => {
             }
 
             model _BigInt {
+                id String @id
                 _bigInt BigInt @db.BigInt
                 _unsignedBigInt BigInt @db.UnsignedBigInt
                 _int8 BigInt @db.Int8
             }
 
             model _FloatDecimal {
+                id String @id
                 _float Float @db.Float
                 _decimal Decimal @db.Decimal
                 _decimal1 Decimal @db.Decimal(10, 2)
@@ -318,6 +323,7 @@ describe('Attribute tests', () => {
             }
 
             model _DateTime {
+                id String @id
                 _dateTime DateTime @db.DateTime
                 _dateTime2 DateTime @db.DateTime2
                 _smallDateTime DateTime @db.SmallDateTime
@@ -334,11 +340,13 @@ describe('Attribute tests', () => {
             }
 
             model _Json {
+                id String @id
                 _json Json @db.Json
                 _jsonb Json @db.JsonB
             }
 
             model _Bytes {
+                id String @id
                 _bytes Bytes @db.Bytes
                 _byteA Bytes @db.ByteA
                 _longBlob Bytes @db.LongBlob
@@ -1118,6 +1126,7 @@ describe('Attribute tests', () => {
             }
 
             model M {
+                id String @id
                 e E @default(E1)
             }
         `);

packages/schema/tests/schema/validation/datamodel-validation.test.ts

@@ -120,16 +120,8 @@ describe('Data Model Validation Tests', () => {
     });
 
     it('id field', async () => {
-        // no need for '@id' field when there's no access policy or field validation
-        await loadModel(`
-            ${prelude}
-            model M {
-                x Int
-            }
-        `);
-
         const err =
-            'Model must include a field with @id or @unique attribute, or a model-level @@id or @@unique attribute to use access policies';
+            'Model must have at least one unique criteria. Either mark a single field with `@id`, `@unique` or add a multi field criterion with `@@id([])` or `@@unique([])` to the model.';
 
         expect(
             await loadModelWithError(`
@@ -630,9 +622,8 @@ describe('Data Model Validation Tests', () => {
                         b String
                     }
                 `);
-        expect(errors.length).toBe(1);
 
-        expect(errors[0]).toEqual(`Model A cannot be extended because it's not abstract`);
+        expect(errors).toContain(`Model A cannot be extended because it's not abstract`);
 
         // relation incomplete from multiple level inheritance
         expect(

tests/integration/tests/regression/issue-965.test.ts

@@ -0,0 +1,53 @@
+import { loadModel, loadModelWithError } from '@zenstackhq/testtools';
+
+describe('Regression: issue 965', () => {
+    it('regression1', async () => {
+        await loadModel(`
+        abstract model Base {
+            id String @id @default(cuid())
+        }
+
+        abstract model A {
+            URL String? @url
+        }
+
+        abstract model B {
+            anotherURL String? @url
+        }
+
+        abstract model C {
+            oneMoreURL String? @url
+        }
+
+        model D extends Base, A, B {
+        }
+
+        model E extends Base, B, C {
+        }`);
+    });
+
+    it('regression2', async () => {
+        await expect(
+            loadModelWithError(`
+        abstract model A {
+            URL String? @url
+        }
+
+        abstract model B {
+            anotherURL String? @url
+        }
+
+        abstract model C {
+            oneMoreURL String? @url
+        }
+
+        model D extends A, B {
+        }
+
+        model E extends B, C {
+        }`)
+        ).resolves.toContain(
+            'Model must have at least one unique criteria. Either mark a single field with `@id`, `@unique` or add a multi field criterion with `@@id([])` or `@@unique([])` to the model.'
+        );
+    });
+});