Skip to content

Commit 4dcdcec

Browse files
Azzerty23claude
andcommitted
refactor(policy): simplify fetchPolicyCodes guard and align test expectations
Move the fetchPolicyCodes===false early-return before policy filtering so the diagnostic query is skipped entirely for update/delete, making those operations behave identically to a model with no error codes (NOT_FOUND). Update test expectations accordingly and add an explicit equivalence test. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
1 parent 0ffadd4 commit 4dcdcec

2 files changed

Lines changed: 72 additions & 35 deletions

File tree

packages/plugins/policy/src/policy-handler.ts

Lines changed: 20 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -272,9 +272,10 @@ export class PolicyHandler<Schema extends SchemaDef> extends OperationNodeTransf
272272
) {
273273
if (this.isManyToManyJoinTable(model)) return;
274274
if (this.tryGetConstantPolicy(model, operation) === true) return;
275-
const codedPolicies = this.getModelPolicies(model, operation).filter((p) => p.code);
275+
if (this.options.fetchPolicyCodes === false) return;
276+
const policiesWithCode = this.getModelPolicies(model, operation).filter((p) => p.code);
276277
// Skip if no policies carry an error code — nothing to surface.
277-
if (codedPolicies.length === 0) return;
278+
if (policiesWithCode.length === 0) return;
278279

279280
const whereCondition = originalWhere ?? trueNode(this.dialect);
280281

@@ -283,10 +284,7 @@ export class PolicyHandler<Schema extends SchemaDef> extends OperationNodeTransf
283284
.select(this.eb.lit(1).as('_'))
284285
.where(() => new ExpressionWrapper(whereCondition));
285286

286-
const fetchCodes = this.options.fetchPolicyCodes !== false;
287-
const selectedPolicies = fetchCodes ? codedPolicies : [];
288-
289-
const codeSelections = selectedPolicies.map((policy, i) => {
287+
const codeSelections = policiesWithCode.map((policy, i) => {
290288
const condition = this.compilePolicyCondition(model, undefined, operation, policy);
291289
const violationCondition = policy.kind === 'allow' ? logicalNot(this.dialect, condition) : condition;
292290
const inner = this.eb
@@ -302,7 +300,10 @@ export class PolicyHandler<Schema extends SchemaDef> extends OperationNodeTransf
302300
kind: 'SelectQueryNode',
303301
selections: [
304302
SelectionNode.create(
305-
AliasNode.create(this.eb.exists(rowExistsInner).toOperationNode(), IdentifierNode.create('$exists')),
303+
AliasNode.create(
304+
this.eb.exists(rowExistsInner).toOperationNode(),
305+
IdentifierNode.create('$exists'),
306+
),
306307
),
307308
...codeSelections,
308309
],
@@ -311,8 +312,8 @@ export class PolicyHandler<Schema extends SchemaDef> extends OperationNodeTransf
311312
const row = result.rows[0] ?? {};
312313
if (!row.$exists) return;
313314

314-
const policyCodes = fetchCodes
315-
? selectedPolicies.filter((_, i) => row[`$c${i}`]).map((p) => p.code!)
315+
const policyCodes = policiesWithCode
316+
? policiesWithCode.filter((_, i) => row[`$c${i}`]).map((p) => p.code!)
316317
: undefined;
317318

318319
throw createRejectedByPolicyError(model, RejectedByPolicyReason.NO_ACCESS, undefined, policyCodes);
@@ -1139,12 +1140,12 @@ export class PolicyHandler<Schema extends SchemaDef> extends OperationNodeTransf
11391140
valuesTable: ReturnType<BaseCrudDialect<Schema>['buildValuesTableSelect']>,
11401141
proceed: ProceedKyselyQueryFunction,
11411142
): Promise<string[]> {
1142-
const codedPolicies = this.getModelPolicies(model, 'create').filter((p) => p.code);
1143-
if (codedPolicies.length === 0) {
1143+
const policiesWithCode = this.getModelPolicies(model, 'create').filter((p) => p.code);
1144+
if (policiesWithCode.length === 0) {
11441145
return [];
11451146
}
11461147

1147-
const selections = codedPolicies.map((policy, i) => {
1148+
const selections = policiesWithCode.map((policy, i) => {
11481149
const condition = this.compilePolicyCondition(model, undefined, 'create', policy);
11491150
// For allow rules, negate: EXISTS(NOT condition) = true when any proposed row violates allow.
11501151
// For deny rules, keep as-is: EXISTS(condition) = true when deny fires.
@@ -1158,7 +1159,7 @@ export class PolicyHandler<Schema extends SchemaDef> extends OperationNodeTransf
11581159
);
11591160
});
11601161

1161-
return this.evaluatePolicyDiagnostics(codedPolicies, selections, proceed);
1162+
return this.evaluatePolicyDiagnostics(policiesWithCode, selections, proceed);
11621163
}
11631164

11641165
private async findViolatingPostUpdatePolicyCodes(
@@ -1167,8 +1168,8 @@ export class PolicyHandler<Schema extends SchemaDef> extends OperationNodeTransf
11671168
beforeUpdateInfo: Awaited<ReturnType<typeof this.loadBeforeUpdateEntities>>,
11681169
proceed: ProceedKyselyQueryFunction,
11691170
): Promise<string[]> {
1170-
const codedPolicies = this.getModelPolicies(model, 'post-update').filter((p) => p.code);
1171-
if (codedPolicies.length === 0) {
1171+
const policiesWithCode = this.getModelPolicies(model, 'post-update').filter((p) => p.code);
1172+
if (policiesWithCode.length === 0) {
11721173
return [];
11731174
}
11741175

@@ -1201,7 +1202,7 @@ export class PolicyHandler<Schema extends SchemaDef> extends OperationNodeTransf
12011202
return eb.exists(inner).toOperationNode();
12021203
};
12031204

1204-
const selections = codedPolicies.map((policy, i) => {
1205+
const selections = policiesWithCode.map((policy, i) => {
12051206
const condition = this.compilePolicyCondition(model, undefined, 'post-update', policy);
12061207
// For allow rules, negate: EXISTS(NOT condition) = true when any updated row violates allow.
12071208
// For deny rules, keep as-is: EXISTS(condition) = true when deny fires.
@@ -1211,19 +1212,19 @@ export class PolicyHandler<Schema extends SchemaDef> extends OperationNodeTransf
12111212
);
12121213
});
12131214

1214-
return this.evaluatePolicyDiagnostics(codedPolicies, selections, proceed);
1215+
return this.evaluatePolicyDiagnostics(policiesWithCode, selections, proceed);
12151216
}
12161217

12171218
// Single diagnostic query: one EXISTS column per coded policy.
12181219
// EXISTS=true means a violation: deny condition fired, or allow condition wasn't met (negated in caller).
12191220
private async evaluatePolicyDiagnostics(
1220-
codedPolicies: Policy[],
1221+
policiesWithCode: Policy[],
12211222
selections: SelectionNode[],
12221223
proceed: ProceedKyselyQueryFunction,
12231224
): Promise<string[]> {
12241225
const result = await proceed({ kind: 'SelectQueryNode', selections } satisfies SelectQueryNode);
12251226
const row = result.rows[0] ?? {};
1226-
return codedPolicies.filter((_, i) => row[`$c${i}`]).map((p) => p.code!);
1227+
return policiesWithCode.filter((_, i) => row[`$c${i}`]).map((p) => p.code!);
12271228
}
12281229

12291230
private async processReadBack(node: CrudQueryNode, result: QueryResult<any>, proceed: ProceedKyselyQueryFunction) {

tests/e2e/orm/policy/crud/error-codes.test.ts

Lines changed: 52 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -553,16 +553,15 @@ model Foo {
553553
`,
554554
{ plugins: [new PolicyPlugin({ fetchPolicyCodes: false })] },
555555
);
556-
// create: error is still thrown but policyCodes is empty (y=0 triggers deny)
556+
// create: pre-create check still fires → REJECTED_BY_POLICY, but no codes (y=0 triggers deny)
557557
await expect(db.foo.create({ data: { x: 1, y: 0 } })).toBeRejectedByPolicy(undefined, []);
558558
const row = await db.foo.create({ data: { x: 1, y: 1 } });
559559
// negRow: x=-1 (denied for update/delete), but y=1 so create succeeds
560560
const negRow = await db.foo.create({ data: { x: -1, y: 1 } });
561-
// update: error is still thrown but policyCodes is empty
562-
await expect(db.foo.update({ where: { id: negRow.id }, data: { x: 0 } })).toBeRejectedByPolicy(undefined, []);
563-
// delete: error is still thrown but policyCodes is empty
564-
await expect(db.foo.delete({ where: { id: negRow.id } })).toBeRejectedByPolicy(undefined, []);
565-
// post-update: error is still thrown but policyCodes is empty
561+
// update/delete: diagnostic query skipped entirely — behaves as if no codes → NOT_FOUND
562+
await expect(db.foo.update({ where: { id: negRow.id }, data: { x: 0 } })).toBeRejectedNotFound();
563+
await expect(db.foo.delete({ where: { id: negRow.id } })).toBeRejectedNotFound();
564+
// post-update: postUpdateCheck fires independently of fetchPolicyCodes → REJECTED_BY_POLICY, no codes
566565
await expect(db.foo.update({ where: { id: row.id }, data: { x: -1 } })).toBeRejectedByPolicy(undefined, []);
567566
await expect(db.foo.update({ where: { id: row.id }, data: { x: 2 } })).resolves.toMatchObject({ x: 2 });
568567
});
@@ -596,19 +595,16 @@ model Foo {
596595
await expect(db.foo.create({ data: { x: 1, y: 0 } })).toBeRejectedByPolicy(undefined, ['NEGATIVE_Y_CREATE']);
597596
const row = await db.foo.create({ data: { x: 1, y: 1 } });
598597
const negRow = await db.foo.create({ data: { x: -1, y: 1 } });
599-
// update: flag suppresses codes
598+
// update: flag skips diagnostic query entirely → NOT_FOUND (same as no codes defined)
600599
await expect(
601600
db.foo.update({ where: { id: negRow.id }, data: { x: 0 }, fetchPolicyCodes: false }),
602-
).toBeRejectedByPolicy(undefined, []);
601+
).toBeRejectedNotFound();
603602
// update: without flag, codes surface
604603
await expect(db.foo.update({ where: { id: negRow.id }, data: { x: 0 } })).toBeRejectedByPolicy(undefined, [
605604
'NEGATIVE_X_UPDATE',
606605
]);
607-
// delete: flag suppresses codes
608-
await expect(db.foo.delete({ where: { id: negRow.id }, fetchPolicyCodes: false })).toBeRejectedByPolicy(
609-
undefined,
610-
[],
611-
);
606+
// delete: flag skips diagnostic query entirely → NOT_FOUND
607+
await expect(db.foo.delete({ where: { id: negRow.id }, fetchPolicyCodes: false })).toBeRejectedNotFound();
612608
// delete: without flag, codes surface
613609
await expect(db.foo.delete({ where: { id: negRow.id } })).toBeRejectedByPolicy(undefined, [
614610
'NEGATIVE_X_DELETE',
@@ -623,6 +619,46 @@ model Foo {
623619
]);
624620
});
625621

622+
it('fetchPolicyCodes:false for update/delete behaves identically to a model without error codes', async () => {
623+
const schema = `
624+
model Foo {
625+
id Int @id @default(autoincrement())
626+
x Int
627+
@@deny('update', x <= 0, 'NEGATIVE_X_UPDATE')
628+
@@allow('update', x > 0)
629+
@@deny('delete', x <= 0, 'NEGATIVE_X_DELETE')
630+
@@allow('delete', x > 0)
631+
@@allow('create,read', true)
632+
}
633+
`;
634+
const dbWithCodes = await createPolicyTestClient(schema);
635+
const dbOptOut = await createPolicyTestClient(schema);
636+
637+
const [rowWithCodes, rowOptOut] = await Promise.all([
638+
dbWithCodes.foo.create({ data: { x: -1 } }),
639+
dbOptOut.foo.create({ data: { x: -1 } }),
640+
]);
641+
642+
// With codes: update throws REJECTED_BY_POLICY with the error code
643+
await expect(dbWithCodes.foo.update({ where: { id: rowWithCodes.id }, data: { x: 0 } })).toBeRejectedByPolicy(
644+
undefined,
645+
['NEGATIVE_X_UPDATE'],
646+
);
647+
// Opt-out: same update → NOT_FOUND, matching the no-codes baseline
648+
await expect(
649+
dbOptOut.foo.update({ where: { id: rowOptOut.id }, data: { x: 0 }, fetchPolicyCodes: false }),
650+
).toBeRejectedNotFound();
651+
652+
// With codes: delete throws REJECTED_BY_POLICY with the error code
653+
await expect(dbWithCodes.foo.delete({ where: { id: rowWithCodes.id } })).toBeRejectedByPolicy(undefined, [
654+
'NEGATIVE_X_DELETE',
655+
]);
656+
// Opt-out: same delete → NOT_FOUND, matching the no-codes baseline
657+
await expect(
658+
dbOptOut.foo.delete({ where: { id: rowOptOut.id }, fetchPolicyCodes: false }),
659+
).toBeRejectedNotFound();
660+
});
661+
626662
it('query-level fetchPolicyCodes:true overrides plugin-level false', async () => {
627663
const db = await createTestClient(
628664
`
@@ -655,19 +691,19 @@ model Foo {
655691
db.foo.update({ where: { id: negRow.id }, data: { x: 0 }, fetchPolicyCodes: true }),
656692
).toBeRejectedByPolicy(undefined, ['NEGATIVE_X_UPDATE']);
657693
// update: without override, codes are suppressed
658-
await expect(db.foo.update({ where: { id: negRow.id }, data: { x: 0 } })).toBeRejectedByPolicy(undefined, []);
694+
await expect(db.foo.update({ where: { id: negRow.id }, data: { x: 0 } })).toBeRejectedNotFound();
659695
// delete: query-level true re-enables codes despite plugin false
660696
await expect(db.foo.delete({ where: { id: negRow.id }, fetchPolicyCodes: true })).toBeRejectedByPolicy(
661697
undefined,
662698
['NEGATIVE_X_DELETE'],
663699
);
664700
// delete: without override, codes are suppressed
665-
await expect(db.foo.delete({ where: { id: negRow.id } })).toBeRejectedByPolicy(undefined, []);
701+
await expect(db.foo.delete({ where: { id: negRow.id } })).toBeRejectedNotFound();
666702
// post-update: query-level true re-enables codes despite plugin false
667703
await expect(
668704
db.foo.update({ where: { id: row.id }, data: { x: -1 }, fetchPolicyCodes: true }),
669705
).toBeRejectedByPolicy(undefined, ['NEGATIVE_AFTER_UPDATE']);
670-
// post-update: without override, codes are suppressed
706+
// post-update: without override, codes are suppressed and we get a policy rejection without codes (not NotFound)
671707
await expect(db.foo.update({ where: { id: row.id }, data: { x: -1 } })).toBeRejectedByPolicy(undefined, []);
672708
});
673709
});

0 commit comments

Comments
 (0)