Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: properly handle missing fields when evaluating @@validate model-level rules #1097

Merged
merged 2 commits into from
Mar 7, 2024
Merged

fix: properly handle missing fields when evaluating @@validate model-level rules #1097

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5524bb6
fix: properly handle missing fields when evaluating `@@validate` mode…
ymc9 Mar 7, 2024
ae586bb
fix test
ymc9 Mar 7, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 9 additions & 1 deletion packages/schema/src/plugins/zod/utils/schema-gen.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import {
getAttributeArg,
getAttributeArgLiteral,
getLiteral,
isDataModelFieldReference,
isFromStdlib,
} from '@zenstackhq/sdk';
import {
Expand Down Expand Up @@ -205,10 +206,17 @@ export function makeValidationRefinements(model: DataModel) {
const message = messageArg ? `, { message: ${JSON.stringify(messageArg)} }` : '';

try {
const expr = new TypeScriptExpressionTransformer({
let expr = new TypeScriptExpressionTransformer({
context: ExpressionContext.ValidationRule,
fieldReferenceContext: 'value',
}).transform(valueArg);

if (isDataModelFieldReference(valueArg)) {
// if the expression is a simple field reference, treat undefined
// as true since the all fields are optional in validation context
expr = `${expr} ?? true`;
}
Comment on lines +209 to +218
Copy link
Contributor

Choose a reason for hiding this comment

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

The update to the makeValidationRefinements function introduces a check for simple field references using isDataModelFieldReference. This logic treats undefined fields as true in the validation context, which aligns with the PR objectives of correctly handling missing fields in model-level validations.

  • Ensure that this change does not inadvertently affect other validation scenarios where undefined should not be treated as true. It might be beneficial to add more tests covering various validation scenarios to ensure the change's correctness and prevent regressions.
  • Consider documenting this behavior in the code comments or the project's documentation to make it clear to other developers how undefined fields are treated in validation rules.


return `.refine((value: any) => ${expr}${message})`;
} catch (err) {
if (err instanceof TypeScriptExpressionTransformerError) {
Expand Down
52 changes: 38 additions & 14 deletions packages/schema/src/utils/typescript-expression-transformer.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ import {
ThisExpr,
UnaryExpr,
} from '@zenstackhq/language/ast';
import { ExpressionContext, getLiteral, isFromStdlib, isFutureExpr } from '@zenstackhq/sdk';
import { ExpressionContext, getLiteral, isDataModelFieldReference, isFromStdlib, isFutureExpr } from '@zenstackhq/sdk';
import { match, P } from 'ts-pattern';
import { getIdFields } from './ast-utils';

Expand Down Expand Up @@ -258,7 +258,13 @@ export class TypeScriptExpressionTransformer {
}

private ensureBoolean(expr: string) {
return `(${expr} ?? false)`;
if (this.options.context === ExpressionContext.ValidationRule) {
// all fields are optional in a validation context, so we treat undefined
// as boolean true
return `(${expr} ?? true)`;
} else {
return `(${expr} ?? false)`;
}
}

// #endregion
Expand Down Expand Up @@ -300,8 +306,18 @@ export class TypeScriptExpressionTransformer {
}
}

private unary(expr: UnaryExpr, normalizeUndefined: boolean): string {
return `(${expr.operator} ${this.transform(expr.operand, normalizeUndefined)})`;
private unary(expr: UnaryExpr, normalizeUndefined: boolean) {
const operand = this.transform(expr.operand, normalizeUndefined);
let result = `(${expr.operator} ${operand})`;
if (
expr.operator === '!' &&
this.options.context === ExpressionContext.ValidationRule &&
isDataModelFieldReference(expr.operand)
) {
// in a validation context, we treat unary involving undefined as boolean true
result = `(${operand} !== undefined ? (${result}): true)`;
}
return result;
}

private isModelType(expr: Expression) {
Expand All @@ -316,16 +332,24 @@ export class TypeScriptExpressionTransformer {
left = `(${left}?.id ?? null)`;
right = `(${right}?.id ?? null)`;
}
const _default = `(${left} ${expr.operator} ${right})`;

let _default = `(${left} ${expr.operator} ${right})`;

if (this.options.context === ExpressionContext.ValidationRule) {
// in a validation context, we treat binary involving undefined as boolean true
if (isDataModelFieldReference(expr.left)) {
_default = `(${left} !== undefined ? (${_default}): true)`;
}
if (isDataModelFieldReference(expr.right)) {
_default = `(${right} !== undefined ? (${_default}): true)`;
}
}

return match(expr.operator)
.with(
'in',
() =>
`(${this.transform(expr.right, false)}?.includes(${this.transform(
expr.left,
normalizeUndefined
)}) ?? false)`
.with('in', () =>
this.ensureBoolean(
`${this.transform(expr.right, false)}?.includes(${this.transform(expr.left, normalizeUndefined)})`
)
)
.with(P.union('==', '!='), () => {
if (isThisExpr(expr.left) || isThisExpr(expr.right)) {
Expand Down Expand Up @@ -363,8 +387,8 @@ export class TypeScriptExpressionTransformer {
const predicate = innerTransformer.transform(expr.right, normalizeUndefined);

return match(operator)
.with('?', () => `!!((${operand})?.some((_item: any) => ${predicate}))`)
.with('!', () => `!!((${operand})?.every((_item: any) => ${predicate}))`)
.with('?', () => this.ensureBoolean(`(${operand})?.some((_item: any) => ${predicate})`))
.with('!', () => this.ensureBoolean(`(${operand})?.every((_item: any) => ${predicate})`))
.with('^', () => `!((${operand})?.some((_item: any) => ${predicate}))`)
.exhaustive();
}
Expand Down
2 changes: 1 addition & 1 deletion packages/testtools/src/schema.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,7 +105,7 @@ plugin policy {

plugin zod {
provider = '@core/zod'
// preserveTsFiles = true
preserveTsFiles = true
modelOnly = ${!options.fullZod}
}
`;
Expand Down
1 change: 1 addition & 0 deletions tests/integration/tests/cli/plugins.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -116,6 +116,7 @@ describe('CLI Plugins Tests', () => {
strict: true,
lib: ['esnext', 'dom'],
esModuleInterop: true,
skipLibCheck: true,
},
})
);
Expand Down
154 changes: 154 additions & 0 deletions tests/integration/tests/enhancements/with-policy/field-validation.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -609,3 +609,157 @@ describe('With Policy: field validation', () => {
expect(u.tasks[0]).toMatchObject({ slug: 'slug2' });
});
});

describe('With Policy: model-level validation', () => {
it('create', async () => {
const { enhance } = await loadSchema(`
model Model {
id Int @id @default(autoincrement())
x Int
y Int

@@validate(x > 0)
@@validate(x >= y)
@@allow('all', true)
}
`);

const db = enhance();

await expect(db.model.create({ data: { x: 0, y: 0 } })).toBeRejectedByPolicy();
await expect(db.model.create({ data: { x: 1, y: 2 } })).toBeRejectedByPolicy();
await expect(db.model.create({ data: { x: 2, y: 1 } })).toResolveTruthy();
});

it('update', async () => {
const { enhance } = await loadSchema(`
model Model {
id Int @id @default(autoincrement())
x Int
y Int

@@validate(x >= y)
@@allow('all', true)
}
`);

const db = enhance();

await expect(db.model.create({ data: { x: 2, y: 1 } })).toResolveTruthy();
await expect(db.model.create({ data: { x: 1, y: 2 } })).toBeRejectedByPolicy();
});

it('int optionality', async () => {
const { enhance } = await loadSchema(`
model Model {
id Int @id @default(autoincrement())
x Int?

@@validate(x > 0)
@@allow('all', true)
}
`);

const db = enhance();

await expect(db.model.create({ data: { x: 0 } })).toBeRejectedByPolicy();
await expect(db.model.create({ data: { x: 1 } })).toResolveTruthy();
await expect(db.model.create({ data: {} })).toResolveTruthy();
});

it('boolean optionality', async () => {
const { enhance } = await loadSchema(`
model Model {
id Int @id @default(autoincrement())
x Boolean?

@@validate(x)
@@allow('all', true)
}
`);

const db = enhance();

await expect(db.model.create({ data: { x: false } })).toBeRejectedByPolicy();
await expect(db.model.create({ data: { x: true } })).toResolveTruthy();
await expect(db.model.create({ data: {} })).toResolveTruthy();
});

it('optionality with comparison', async () => {
const { enhance } = await loadSchema(`
model Model {
id Int @id @default(autoincrement())
x Int?
y Int?

@@validate(x > y)
@@allow('all', true)
}
`);

const db = enhance();

await expect(db.model.create({ data: { x: 1, y: 2 } })).toBeRejectedByPolicy();
await expect(db.model.create({ data: { x: 1 } })).toResolveTruthy();
await expect(db.model.create({ data: { y: 1 } })).toResolveTruthy();
await expect(db.model.create({ data: {} })).toResolveTruthy();
});

it('optionality with complex expression', async () => {
const { enhance } = await loadSchema(`
model Model {
id Int @id @default(autoincrement())
x Int?
y Int?

@@validate(y > 1 && x > y)
@@allow('all', true)
}
`);

const db = enhance();

await expect(db.model.create({ data: { y: 1 } })).toBeRejectedByPolicy();
await expect(db.model.create({ data: { y: 2 } })).toResolveTruthy();
await expect(db.model.create({ data: {} })).toResolveTruthy();
await expect(db.model.create({ data: { x: 1, y: 2 } })).toBeRejectedByPolicy();
await expect(db.model.create({ data: { x: 3, y: 2 } })).toResolveTruthy();
});

it('optionality with negation', async () => {
const { enhance } = await loadSchema(`
model Model {
id Int @id @default(autoincrement())
x Boolean?

@@validate(!x)
@@allow('all', true)
}
`);

const db = enhance();

await expect(db.model.create({ data: { x: true } })).toBeRejectedByPolicy();
await expect(db.model.create({ data: { x: false } })).toResolveTruthy();
await expect(db.model.create({ data: {} })).toResolveTruthy();
});

it('update implied optionality', async () => {
const { enhance } = await loadSchema(`
model Model {
id Int @id @default(autoincrement())
x Int
y Int

@@validate(x > y)
@@allow('all', true)
}
`);

const db = enhance();

await expect(db.model.create({ data: { id: 1, x: 2, y: 1 } })).toResolveTruthy();
await expect(db.model.update({ where: { id: 1 }, data: { y: 1 } })).toResolveTruthy();
await expect(db.model.update({ where: { id: 1 }, data: {} })).toResolveTruthy();
});
});
52 changes: 52 additions & 0 deletions tests/integration/tests/regression/issue-1078.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
import { loadSchema } from '@zenstackhq/testtools';

describe('issue 1078', () => {
it('regression', async () => {
const { prisma, enhance } = await loadSchema(
`
model Counter {
id String @id

name String
value Int

@@validate(value >= 0)
@@allow('all', true)
}
`
);

const db = enhance();

const counter = await db.counter.create({
data: { id: '1', name: 'It should create', value: 1 },
});

//! This query fails validation
const updated = await db.counter.update({
where: { id: '1' },
data: { name: 'It should update' },
});
});
Comment on lines +3 to +30
Copy link
Contributor

Choose a reason for hiding this comment

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

The 'regression' test case effectively demonstrates the handling of missing fields during model-level validation, specifically testing the scenario where the 'value' field is omitted in an update operation. This aligns well with the PR objectives of ensuring that missing fields are correctly handled in validation rules.

  • Ensure that the expected behavior (e.g., passing or failing validation) is explicitly asserted in the test. Currently, the test performs operations but does not assert the outcome. Adding assertions will make the test more robust and clear about the expected behavior.
  • Consider adding negative test cases where the validation should fail, to ensure that the validation logic is not overly permissive.


it('read', async () => {
const { prisma, enhance } = await loadSchema(
`
model Post {
id Int @id() @default(autoincrement())
title String @allow('read', true, true)
content String
}
`,
{ logPrismaQuery: true }
);

const db = enhance();

const post = await prisma.post.create({ data: { title: 'Post1', content: 'Content' } });
await expect(db.post.findUnique({ where: { id: post.id } })).toResolveNull();
await expect(db.post.findUnique({ where: { id: post.id }, select: { title: true } })).resolves.toEqual({
title: 'Post1',
});
});
});
Loading