From d00b864c0d42a9ca7a75464d2847d953d9779fb1 Mon Sep 17 00:00:00 2001 From: ymc9 <104139426+ymc9@users.noreply.github.com> Date: Thu, 5 Nov 2026 16:16:21 -0800 Subject: [PATCH 1/2] fix(policy): handle computed fields in relation, delegate-inherited and before() policy paths Follow-up to #2889, which only inlined computed fields referenced directly on the policy's own model. Other paths still emitted bare column refs: - relation member access (`owner.isActive`) selected `"$$t1"."isActive"` - computed fields inherited from a `@@delegate` base were evaluated against the sub-model table (`"Video"."visibility"`) instead of the base table - `before().` in post-update policies loaded the field as a column All field references in the expression transformer now go through a single `buildFieldRef` helper that handles regular, computed and delegate-inherited fields, and the before-update entity load inlines computed fields. Using a parameterized computed field in a policy now raises an ORMError (config-error). Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_0118JMseP4tLbCCwFQRQS4od --- .../policy/src/expression-transformer.ts | 44 ++-- packages/plugins/policy/src/policy-handler.ts | 9 + tests/regression/test/issue-2888.test.ts | 228 ++++++++++++++++++ 3 files changed, 261 insertions(+), 20 deletions(-) diff --git a/packages/plugins/policy/src/expression-transformer.ts b/packages/plugins/policy/src/expression-transformer.ts index 620aa7053..95c81883c 100644 --- a/packages/plugins/policy/src/expression-transformer.ts +++ b/packages/plugins/policy/src/expression-transformer.ts @@ -1,6 +1,8 @@ import { invariant } from '@zenstackhq/common-helpers'; import { getCrudDialect, + ORMError, + ORMErrorReason, QueryUtils, SchemaUtils, type BaseCrudDialect, @@ -982,12 +984,7 @@ export class ExpressionTransformer { invariant(i === members.length - 1, 'plain field access must be the last segment'); invariant(!currNode, 'plain field access must be the last segment'); - if (fieldDef.originModel && fieldDef.originModel !== fromModel) { - // field inherited from a delegate base model, look it up from the base table - currNode = this.buildDelegateBaseFieldSelect(fromModel, fromAlias, member, fieldDef.originModel); - } else { - currNode = ReferenceNode.create(ColumnNode.create(member), TableNode.create(fromAlias)); - } + currNode = this.buildFieldRef(fromModel, member, fromAlias); } } @@ -1126,29 +1123,35 @@ export class ExpressionTransformer { // if field comes from a delegate base model, we need to use the join alias // of that base model - const tableName = context.alias ?? context.modelOrType; + return this.buildFieldRef(context.modelOrType, column, context.alias ?? context.modelOrType); + } - const fieldDef = QueryUtils.requireField(this.schema, context.modelOrType, column); + /** + * Builds a reference to a scalar field of `model` whose table is aliased as `modelAlias`. + * Handles regular columns, `@computed` fields (inlined via the dialect), and fields inherited + * from a delegate base model (looked up from the base table with a correlated subquery). + */ + private buildFieldRef(model: string, field: string, modelAlias: string): OperationNode { + const fieldDef = QueryUtils.requireField(this.schema, model, field); - if (fieldDef.computed) { - // `@computed` fields don't physically exist as columns — inline the - // computed expression the same way every other query path does via - // `dialect.fieldRef()`, instead of emitting a bare (nonexistent) column ref. - if (fieldDef.params) { - throw new Error(`parameterized computed field "${column}" cannot be used in a policy expression`); - } - return this.dialect.fieldRef(context.modelOrType, column, tableName).toOperationNode(); + if (fieldDef.computed && fieldDef.params) { + throw new ORMError( + ORMErrorReason.CONFIG_ERROR, + `Parameterized computed field "${field}" of model "${model}" cannot be used in a policy expression`, + ); } - if (!fieldDef.originModel || fieldDef.originModel === context.modelOrType) { - return ReferenceNode.create(ColumnNode.create(column), TableNode.create(tableName)); + if (!fieldDef.originModel || fieldDef.originModel === model) { + return fieldDef.computed + ? this.dialect.fieldRef(model, field, modelAlias).toOperationNode() + : ReferenceNode.create(ColumnNode.create(field), TableNode.create(modelAlias)); } // For inherited fields in delegate sub-types, use a correlated subquery against the // base table. For create operations the VALUES table contains the sub-type's own // fields and its id FK, so the subquery can look up the base record (already inserted // in the same transaction) to resolve inherited field values correctly. - return this.buildDelegateBaseFieldSelect(context.modelOrType, tableName, column, fieldDef.originModel); + return this.buildDelegateBaseFieldSelect(model, modelAlias, field, fieldDef.originModel); } // convert transformer's binding scope to equivalent expression evaluator binding scope @@ -1173,7 +1176,8 @@ export class ExpressionTransformer { kind: 'SelectQueryNode', from: FromNode.create([TableNode.create(baseModel)]), selections: [ - SelectionNode.create(ReferenceNode.create(ColumnNode.create(field), TableNode.create(baseModel))), + // the base model owns the field, so a computed field is evaluated against the base table + SelectionNode.create(this.buildFieldRef(baseModel, field, baseModel) as SelectionNode['selection']), ], where: WhereNode.create( conjunction( diff --git a/packages/plugins/policy/src/policy-handler.ts b/packages/plugins/policy/src/policy-handler.ts index ddc58ee7a..2d2de6cf8 100644 --- a/packages/plugins/policy/src/policy-handler.ts +++ b/packages/plugins/policy/src/policy-handler.ts @@ -736,6 +736,15 @@ export class PolicyHandler extends OperationNodeTransf if (fieldDef?.originModel) { baseModelsToJoin.add(fieldDef.originModel); } + if (fieldDef?.computed) { + // computed fields aren't physical columns, inline the computed expression + return SelectionNode.create( + AliasNode.create( + this.dialect.fieldRef(owningTable, f, owningTable).toOperationNode(), + IdentifierNode.create(f), + ), + ); + } return SelectionNode.create(ReferenceNode.create(ColumnNode.create(f), TableNode.create(owningTable))); }); diff --git a/tests/regression/test/issue-2888.test.ts b/tests/regression/test/issue-2888.test.ts index aaa31a088..777608406 100644 --- a/tests/regression/test/issue-2888.test.ts +++ b/tests/regression/test/issue-2888.test.ts @@ -31,4 +31,232 @@ model Model { x: 2, }); }); + + it('binds computed expression to the right row and denies when false', async () => { + const db = await createPolicyTestClient( + ` +model Model { + id Int @id @default(autoincrement()) + x Int + canEdit Boolean @computed + + @@allow('create,read', true) + @@allow('update', canEdit) +} + `, + { + computedFields: { + Model: { + canEdit: (eb: any, ctx: any) => eb(eb.ref(`${ctx.modelAlias}.x`), '>', 0), + }, + }, + } as any, + ); + + await db.model.create({ data: { id: 1, x: 1 } }); + await db.model.create({ data: { id: 2, x: 0 } }); + + await expect(db.model.update({ where: { id: 1 }, data: { x: 2 } })).resolves.toMatchObject({ x: 2 }); + await expect(db.model.update({ where: { id: 2 }, data: { x: 5 } })).toBeRejectedNotFound(); + }); + + it('computed field reached through a relation member access', async () => { + const db = await createPolicyTestClient( + ` +model User { + id Int @id @default(autoincrement()) + status String + isActive Boolean @computed + posts Post[] + @@allow('all', true) +} + +model Post { + id Int @id @default(autoincrement()) + owner User @relation(fields: [ownerId], references: [id]) + ownerId Int + @@allow('create', true) + @@allow('read', owner.isActive) +} + `, + { + computedFields: { + User: { + isActive: (eb: any, ctx: any) => eb(eb.ref(`${ctx.modelAlias}.status`), '=', 'active'), + }, + }, + } as any, + ); + + await db.$unuseAll().user.create({ data: { id: 1, status: 'active', posts: { create: { id: 1 } } } }); + await db.$unuseAll().user.create({ data: { id: 2, status: 'inactive', posts: { create: { id: 2 } } } }); + + await expect(db.post.findMany()).resolves.toEqual([expect.objectContaining({ id: 1 })]); + }); + + it('computed field inherited from a delegate base model', async () => { + const db = await createPolicyTestClient( + ` +model Asset { + id Int @id @default(autoincrement()) + visibility String + isPublic Boolean @computed + type String + @@delegate(type) + @@allow('create', true) +} + +model Video extends Asset { + duration Int + @@allow('create', true) + @@allow('read', isPublic) +} + `, + { + computedFields: { + Asset: { + isPublic: (eb: any, ctx: any) => eb(eb.ref(`${ctx.modelAlias}.visibility`), '=', 'public'), + }, + }, + } as any, + ); + + await db.$unuseAll().video.create({ data: { id: 1, visibility: 'public', duration: 1 } }); + await db.$unuseAll().video.create({ data: { id: 2, visibility: 'private', duration: 1 } }); + + await expect(db.video.findMany()).resolves.toEqual([expect.objectContaining({ id: 1 })]); + }); + + it('before().computedField in post-update policy', async () => { + const db = await createPolicyTestClient( + ` +model Model { + id Int @id @default(autoincrement()) + x Int + canEdit Boolean @computed + + @@allow('all', true) + @@deny('post-update', !before().canEdit) +} + `, + { + computedFields: { + Model: { + canEdit: (eb: any, ctx: any) => eb(eb.ref(`${ctx.modelAlias}.x`), '>', 0), + }, + }, + } as any, + ); + + await db.model.create({ data: { id: 1, x: 1 } }); + await db.model.create({ data: { id: 2, x: 0 } }); + + await expect(db.model.update({ where: { id: 1 }, data: { x: 2 } })).resolves.toMatchObject({ x: 2 }); + await expect(db.model.update({ where: { id: 2 }, data: { x: 5 } })).toBeRejectedByPolicy(); + }); + + it('computed field in create policy (value-object context)', async () => { + const db = await createPolicyTestClient( + ` +model Model { + id Int @id @default(autoincrement()) + x Int + canCreate Boolean @computed + + @@allow('read', true) + @@allow('create', canCreate) +} + `, + { + computedFields: { + Model: { + canCreate: (eb: any, ctx: any) => eb(eb.ref(`${ctx.modelAlias}.x`), '>', 0), + }, + }, + } as any, + ); + + await expect(db.model.create({ data: { id: 1, x: 1 } })).resolves.toMatchObject({ x: 1 }); + await expect(db.model.create({ data: { id: 2, x: 0 } })).toBeRejectedByPolicy(); + }); + + it('before() on a computed field inherited from a delegate base, and relation access to it', async () => { + const db = await createPolicyTestClient( + ` +model Owner { + id Int @id @default(autoincrement()) + videos Video[] + @@allow('all', true) +} + +model Asset { + id Int @id @default(autoincrement()) + visibility String + isPublic Boolean @computed + type String + @@delegate(type) + @@allow('all', true) +} + +model Video extends Asset { + duration Int + owner Owner @relation(fields: [ownerId], references: [id]) + ownerId Int + comments Comment[] + @@allow('all', true) + @@deny('post-update', !before().isPublic) +} + +model Comment { + id Int @id @default(autoincrement()) + videoId Int + video Video @relation(fields: [videoId], references: [id]) + @@allow('create', true) + @@allow('read', video.isPublic) +} + `, + { + computedFields: { + Asset: { + isPublic: (eb: any, ctx: any) => eb(eb.ref(`${ctx.modelAlias}.visibility`), '=', 'public'), + }, + }, + } as any, + ); + + await db.owner.create({ data: { id: 1 } }); + await db.video.create({ data: { id: 1, visibility: 'public', duration: 1, ownerId: 1 } }); + await db.video.create({ data: { id: 2, visibility: 'private', duration: 1, ownerId: 1 } }); + await db.$unuseAll().comment.create({ data: { id: 1, videoId: 1 } }); + await db.$unuseAll().comment.create({ data: { id: 2, videoId: 2 } }); + + await expect(db.comment.findMany()).resolves.toEqual([expect.objectContaining({ id: 1 })]); + await expect(db.video.update({ where: { id: 1 }, data: { duration: 2 } })).resolves.toMatchObject({ + duration: 2, + }); + await expect(db.video.update({ where: { id: 2 }, data: { duration: 2 } })).toBeRejectedByPolicy(); + }); + + it('parameterized computed field in policy is rejected with a clear error', async () => { + const db = await createPolicyTestClient( + ` +model Model { + id Int @id @default(autoincrement()) + x Int + score(min: Int) Int @computed + @@allow('create', true) + @@allow('read', score > 0) +} + `, + { + computedFields: { + Model: { + score: (eb: any, ctx: any, args: any) => eb(eb.ref(`${ctx.modelAlias}.x`), '-', args?.min ?? 0), + }, + }, + } as any, + ); + await db.$unuseAll().model.create({ data: { id: 1, x: 1 } }); + await expect(db.model.findMany()).rejects.toThrow(/Parameterized computed field "score"/); + }); }); From 040fb65ee7d6a83ef71221976e619f55012932f4 Mon Sep 17 00:00:00 2001 From: ymc9 <104139426+ymc9@users.noreply.github.com> Date: Thu, 5 Nov 2026 18:10:25 -0800 Subject: [PATCH 2/2] fix(policy): reject parameterized computed fields in before() and preserve ORM errors in reads - check for parameterized computed fields when loading before-update entities, so `before().field` can't evaluate the implementation without args - don't re-wrap ORMErrors (e.g. CONFIG_ERROR from plugins) as DB_QUERY_ERROR in read/exists operation paths Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_0118JMseP4tLbCCwFQRQS4od --- .../orm/src/client/crud/operations/base.ts | 8 +++++ .../policy/src/expression-transformer.ts | 10 ++---- packages/plugins/policy/src/policy-handler.ts | 3 ++ packages/plugins/policy/src/utils.ts | 15 ++++++++- tests/regression/test/issue-2888.test.ts | 33 ++++++++++++++++++- 5 files changed, 59 insertions(+), 10 deletions(-) diff --git a/packages/orm/src/client/crud/operations/base.ts b/packages/orm/src/client/crud/operations/base.ts index b44ca4265..5bdaf1919 100644 --- a/packages/orm/src/client/crud/operations/base.ts +++ b/packages/orm/src/client/crud/operations/base.ts @@ -171,6 +171,10 @@ export abstract class BaseOperationHandler { const r = await kysely.getExecutor().executeQuery(compiled); result = r.rows as { $exists: number | boolean }[]; } catch (err) { + if (err instanceof ORMError) { + // already a structured error (e.g., raised by a plugin or wrapped by the executor) + throw err; + } throw createDBQueryError(`Failed to execute query: ${err}`, err, compiled.sql, compiled.parameters); } @@ -212,6 +216,10 @@ export abstract class BaseOperationHandler { const r = await kysely.getExecutor().executeQuery(compiled); result = r.rows; } catch (err) { + if (err instanceof ORMError) { + // already a structured error (e.g., raised by a plugin or wrapped by the executor) + throw err; + } throw createDBQueryError(`Failed to execute query: ${err}`, err, compiled.sql, compiled.parameters); } diff --git a/packages/plugins/policy/src/expression-transformer.ts b/packages/plugins/policy/src/expression-transformer.ts index 95c81883c..b6cd6dba5 100644 --- a/packages/plugins/policy/src/expression-transformer.ts +++ b/packages/plugins/policy/src/expression-transformer.ts @@ -1,8 +1,6 @@ import { invariant } from '@zenstackhq/common-helpers'; import { getCrudDialect, - ORMError, - ORMErrorReason, QueryUtils, SchemaUtils, type BaseCrudDialect, @@ -60,6 +58,7 @@ import { falseNode, isBeforeInvocation, logicalNot, + rejectParameterizedComputedField, trueNode, } from './utils'; @@ -1134,12 +1133,7 @@ export class ExpressionTransformer { private buildFieldRef(model: string, field: string, modelAlias: string): OperationNode { const fieldDef = QueryUtils.requireField(this.schema, model, field); - if (fieldDef.computed && fieldDef.params) { - throw new ORMError( - ORMErrorReason.CONFIG_ERROR, - `Parameterized computed field "${field}" of model "${model}" cannot be used in a policy expression`, - ); - } + rejectParameterizedComputedField(fieldDef, model); if (!fieldDef.originModel || fieldDef.originModel === model) { return fieldDef.computed diff --git a/packages/plugins/policy/src/policy-handler.ts b/packages/plugins/policy/src/policy-handler.ts index 2d2de6cf8..6b96c07e6 100644 --- a/packages/plugins/policy/src/policy-handler.ts +++ b/packages/plugins/policy/src/policy-handler.ts @@ -59,6 +59,7 @@ import { isBeforeInvocation, isTrueNode, logicalNot, + rejectParameterizedComputedField, trueNode, } from './utils'; @@ -737,6 +738,8 @@ export class PolicyHandler extends OperationNodeTransf baseModelsToJoin.add(fieldDef.originModel); } if (fieldDef?.computed) { + // parameterized computed fields can't be evaluated without query-time args + rejectParameterizedComputedField(fieldDef, model); // computed fields aren't physical columns, inline the computed expression return SelectionNode.create( AliasNode.create( diff --git a/packages/plugins/policy/src/utils.ts b/packages/plugins/policy/src/utils.ts index 5deef04bd..632864d5f 100644 --- a/packages/plugins/policy/src/utils.ts +++ b/packages/plugins/policy/src/utils.ts @@ -1,5 +1,5 @@ import { ORMError, ORMErrorReason, RejectedByPolicyReason, type BaseCrudDialect } from '@zenstackhq/orm'; -import { ExpressionUtils, type Expression, type SchemaDef } from '@zenstackhq/orm/schema'; +import { ExpressionUtils, type Expression, type FieldDef, type SchemaDef } from '@zenstackhq/orm/schema'; import type { OperationNode } from 'kysely'; import { AliasNode, @@ -191,3 +191,16 @@ export function createRejectedByPolicyError( export function createUnsupportedError(message: string) { return new ORMError(ORMErrorReason.NOT_SUPPORTED, message); } + +/** + * Throws a config error if the given field is a parameterized computed field. Such fields need + * query-time args, which policy expressions have no way to provide. + */ +export function rejectParameterizedComputedField(fieldDef: FieldDef, model: string) { + if (fieldDef.computed && fieldDef.params) { + throw new ORMError( + ORMErrorReason.CONFIG_ERROR, + `Parameterized computed field "${fieldDef.name}" of model "${model}" cannot be used in a policy expression`, + ); + } +} diff --git a/tests/regression/test/issue-2888.test.ts b/tests/regression/test/issue-2888.test.ts index 777608406..b2bdd04bc 100644 --- a/tests/regression/test/issue-2888.test.ts +++ b/tests/regression/test/issue-2888.test.ts @@ -1,3 +1,4 @@ +import { ORMErrorReason } from '@zenstackhq/orm'; import { createPolicyTestClient } from '@zenstackhq/testtools'; import { describe, expect, it } from 'vitest'; @@ -257,6 +258,36 @@ model Model { } as any, ); await db.$unuseAll().model.create({ data: { id: 1, x: 1 } }); - await expect(db.model.findMany()).rejects.toThrow(/Parameterized computed field "score"/); + await expect(db.model.findMany()).rejects.toMatchObject({ + reason: ORMErrorReason.CONFIG_ERROR, + message: expect.stringContaining('Parameterized computed field "score"'), + }); + }); + + it('parameterized computed field referenced via before() is rejected with a clear error', async () => { + const db = await createPolicyTestClient( + ` +model Model { + id Int @id @default(autoincrement()) + x Int + score(min: Int) Int @computed + @@allow('create,read,update', true) + @@allow('post-update', before().score > 0) +} + `, + { + computedFields: { + Model: { + // the default would let the update through if the impl were called without args + score: (eb: any, ctx: any, args: any) => eb(eb.ref(`${ctx.modelAlias}.x`), '-', args?.min ?? 0), + }, + }, + } as any, + ); + await db.model.create({ data: { id: 1, x: 1 } }); + await expect(db.model.update({ where: { id: 1 }, data: { x: 2 } })).rejects.toMatchObject({ + reason: ORMErrorReason.CONFIG_ERROR, + message: expect.stringContaining('Parameterized computed field "score"'), + }); }); });