From 30d14f2c12fbb72912d03067db31aeb73678262e Mon Sep 17 00:00:00 2001 From: ymc9 <104139426+ymc9@users.noreply.github.com> Date: Sun, 11 Feb 2024 21:19:48 +0800 Subject: [PATCH 1/3] fix: incorrect validation errors for abstract models with validation rules --- .../validator/datamodel-validator.ts | 22 ++++---- .../tests/regression/issue-965.test.ts | 53 +++++++++++++++++++ 2 files changed, 62 insertions(+), 13 deletions(-) create mode 100644 tests/integration/tests/regression/issue-965.test.ts diff --git a/packages/schema/src/language-server/validator/datamodel-validator.ts b/packages/schema/src/language-server/validator/datamodel-validator.ts index ce1886f5e..3096d5257 100644 --- a/packages/schema/src/language-server/validator/datamodel-validator.ts +++ b/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 { 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, diff --git a/tests/integration/tests/regression/issue-965.test.ts b/tests/integration/tests/regression/issue-965.test.ts new file mode 100644 index 000000000..79bd92075 --- /dev/null +++ b/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.' + ); + }); +}); From 5c08b6a2804dd315890cbe45614ae594bd76a411 Mon Sep 17 00:00:00 2001 From: ymc9 <104139426+ymc9@users.noreply.github.com> Date: Sun, 11 Feb 2024 21:30:21 +0800 Subject: [PATCH 2/3] fix tests --- .../tests/schema/validation/attribute-validation.test.ts | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/packages/schema/tests/schema/validation/attribute-validation.test.ts b/packages/schema/tests/schema/validation/attribute-validation.test.ts index 8b7886334..6d8d02b14 100644 --- a/packages/schema/tests/schema/validation/attribute-validation.test.ts +++ b/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) } `); From f099e0771d02ccf75752c22f086390a68af36218 Mon Sep 17 00:00:00 2001 From: ymc9 <104139426+ymc9@users.noreply.github.com> Date: Sun, 11 Feb 2024 21:37:08 +0800 Subject: [PATCH 3/3] fix tests --- .../schema/validation/datamodel-validation.test.ts | 13 ++----------- 1 file changed, 2 insertions(+), 11 deletions(-) diff --git a/packages/schema/tests/schema/validation/datamodel-validation.test.ts b/packages/schema/tests/schema/validation/datamodel-validation.test.ts index e1f06d268..4212441fe 100644 --- a/packages/schema/tests/schema/validation/datamodel-validation.test.ts +++ b/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(