Skip to content

Commit c0c5a16

Browse files
ymc9ymc9
authored
fix: fix the incorrect query args reduction when there're mixed boolean operators (#690)
1 parent a3064dc commit c0c5a16

File tree

5 files changed

+305
-46
lines changed

5 files changed

+305
-46
lines changed

‎packages/runtime/src/enhancements/model-meta.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -28,7 +28,7 @@ export function getDefaultModelMeta(): ModelMeta {
2828
* Resolves a model field to its metadata. Returns undefined if not found.
2929
*/
3030
export function resolveField(modelMeta: ModelMeta, model: string, field: string): FieldInfo | undefined {
31-
return modelMeta.fields[lowerCaseFirst(model)][field];
31+
return modelMeta.fields[lowerCaseFirst(model)]?.[field];
3232
}
3333

3434
/**

‎packages/runtime/src/enhancements/policy/policy-utils.ts

Lines changed: 84 additions & 43 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@ import { getFields, resolveField } from '../model-meta';
1919
import { NestedWriteVisitorContext } from '../nested-write-vistor';
2020
import type { InputCheckFunc, ModelMeta, PolicyDef, ReadFieldCheckFunc, ZodSchemas } from '../types';
2121
import {
22+
enumerate,
2223
formatObject,
2324
getIdFields,
2425
getModelFields,
@@ -54,14 +55,28 @@ export class PolicyUtil {
5455
* Creates a conjunction of a list of query conditions.
5556
*/
5657
and(...conditions: (boolean | object | undefined)[]): object {
57-
return this.reduce({ AND: conditions });
58+
const filtered = conditions.filter((c) => c !== undefined);
59+
if (filtered.length === 0) {
60+
return this.makeTrue();
61+
} else if (filtered.length === 1) {
62+
return this.reduce(filtered[0]);
63+
} else {
64+
return this.reduce({ AND: filtered });
65+
}
5866
}
5967

6068
/**
6169
* Creates a disjunction of a list of query conditions.
6270
*/
6371
or(...conditions: (boolean | object | undefined)[]): object {
64-
return this.reduce({ OR: conditions });
72+
const filtered = conditions.filter((c) => c !== undefined);
73+
if (filtered.length === 0) {
74+
return this.makeFalse();
75+
} else if (filtered.length === 1) {
76+
return this.reduce(filtered[0]);
77+
} else {
78+
return this.reduce({ OR: filtered });
79+
}
6580
}
6681

6782
/**
@@ -116,48 +131,75 @@ export class PolicyUtil {
116131
return this.makeFalse();
117132
}
118133

119-
if ('AND' in condition && Array.isArray(condition.AND)) {
120-
const children = condition.AND.map((c: any) => this.reduce(c)).filter(
121-
(c) => c !== undefined && !this.isTrue(c)
122-
);
123-
if (children.length === 0) {
124-
return this.makeTrue();
125-
} else if (children.some((c) => this.isFalse(c))) {
126-
return this.makeFalse();
127-
} else if (children.length === 1) {
128-
return children[0];
129-
} else {
130-
return { AND: children };
131-
}
134+
if (condition === null) {
135+
return condition;
132136
}
133137

134-
if ('OR' in condition && Array.isArray(condition.OR)) {
135-
const children = condition.OR.map((c: any) => this.reduce(c)).filter(
136-
(c) => c !== undefined && !this.isFalse(c)
137-
);
138-
if (children.length === 0) {
139-
return this.makeFalse();
140-
} else if (children.some((c) => this.isTrue(c))) {
141-
return this.makeTrue();
142-
} else if (children.length === 1) {
143-
return children[0];
144-
} else {
145-
return { OR: children };
138+
const result: any = {};
139+
for (const [key, value] of Object.entries<any>(condition)) {
140+
if (value === null || value === undefined) {
141+
result[key] = value;
142+
continue;
146143
}
147-
}
148144

149-
if ('NOT' in condition && condition.NOT !== null && typeof condition.NOT === 'object') {
150-
const child = this.reduce(condition.NOT);
151-
if (this.isTrue(child)) {
152-
return this.makeFalse();
153-
} else if (this.isFalse(child)) {
154-
return this.makeTrue();
155-
} else {
156-
return { NOT: child };
145+
switch (key) {
146+
case 'AND': {
147+
const children = enumerate(value)
148+
.map((c: any) => this.reduce(c))
149+
.filter((c) => c !== undefined && !this.isTrue(c));
150+
if (children.length === 0) {
151+
result[key] = []; // true
152+
} else if (children.some((c) => this.isFalse(c))) {
153+
result['OR'] = []; // false
154+
} else {
155+
if (!this.isTrue({ AND: result[key] })) {
156+
// use AND only if it's not already true
157+
result[key] = !Array.isArray(value) && children.length === 1 ? children[0] : children;
158+
}
159+
}
160+
break;
161+
}
162+
163+
case 'OR': {
164+
const children = enumerate(value)
165+
.map((c: any) => this.reduce(c))
166+
.filter((c) => c !== undefined && !this.isFalse(c));
167+
if (children.length === 0) {
168+
result[key] = []; // false
169+
} else if (children.some((c) => this.isTrue(c))) {
170+
result['AND'] = []; // true
171+
} else {
172+
if (!this.isFalse({ OR: result[key] })) {
173+
// use OR only if it's not already false
174+
result[key] = !Array.isArray(value) && children.length === 1 ? children[0] : children;
175+
}
176+
}
177+
break;
178+
}
179+
180+
case 'NOT': {
181+
result[key] = this.reduce(value);
182+
break;
183+
}
184+
185+
default: {
186+
const booleanKeys = ['AND', 'OR', 'NOT', 'is', 'isNot', 'none', 'every', 'some'];
187+
if (
188+
typeof value === 'object' &&
189+
value &&
190+
// recurse only if the value has at least one boolean key
191+
Object.keys(value).some((k) => booleanKeys.includes(k))
192+
) {
193+
result[key] = this.reduce(value);
194+
} else {
195+
result[key] = value;
196+
}
197+
break;
198+
}
157199
}
158200
}
159201

160-
return condition;
202+
return result;
161203
}
162204

163205
//#endregion
@@ -349,18 +391,18 @@ export class PolicyUtil {
349391
operation: PolicyOperationKind
350392
) {
351393
const guard = this.getAuthGuard(db, fieldInfo.type, operation);
394+
395+
// is|isNot and flat fields conditions are mutually exclusive
396+
352397
if (payload.is || payload.isNot) {
353398
if (payload.is) {
354399
this.injectGuardForRelationFields(db, fieldInfo.type, payload.is, operation);
355-
// turn "is" into: { is: { AND: [ originalIs, guard ] }
356-
payload.is = this.and(payload.is, guard);
357400
}
358401
if (payload.isNot) {
359402
this.injectGuardForRelationFields(db, fieldInfo.type, payload.isNot, operation);
360-
// turn "isNot" into: { isNot: { AND: [ originalIsNot, { NOT: guard } ] } }
361-
payload.isNot = this.and(payload.isNot, this.not(guard));
362-
delete payload.isNot;
363403
}
404+
// merge guard with existing "is": { is: [originalIs, guard] }
405+
payload.is = this.and(payload.is, guard);
364406
} else {
365407
this.injectGuardForRelationFields(db, fieldInfo.type, payload, operation);
366408
// turn direct conditions into: { is: { AND: [ originalConditions, guard ] } }
@@ -1062,7 +1104,6 @@ export class PolicyUtil {
10621104
throw new Error('invalid where clause');
10631105
}
10641106

1065-
extra = this.reduce(extra);
10661107
if (this.isTrue(extra)) {
10671108
return;
10681109
}

‎tests/integration/tests/enhancements/with-policy/query-reduction.test.ts

Lines changed: 147 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,147 @@
1+
import { loadSchema } from '@zenstackhq/testtools';
2+
import path from 'path';
3+
4+
describe('With Policy: query reduction', () => {
5+
let origDir: string;
6+
7+
beforeAll(async () => {
8+
origDir = path.resolve('.');
9+
});
10+
11+
afterEach(() => {
12+
process.chdir(origDir);
13+
});
14+
15+
it('test query reduction', async () => {
16+
const { prisma, withPolicy } = await loadSchema(
17+
`
18+
model User {
19+
id Int @id @default(autoincrement())
20+
role String @default("User")
21+
posts Post[]
22+
private Boolean @default(false)
23+
age Int
24+
25+
@@allow('all', auth() == this)
26+
@@allow('read', !private)
27+
}
28+
29+
model Post {
30+
id Int @id @default(autoincrement())
31+
user User @relation(fields: [userId], references: [id])
32+
userId Int
33+
title String
34+
published Boolean @default(false)
35+
viewCount Int @default(0)
36+
37+
@@allow('all', auth() == user)
38+
@@allow('read', published)
39+
}
40+
`
41+
);
42+
43+
await prisma.user.create({
44+
data: {
45+
id: 1,
46+
role: 'User',
47+
age: 18,
48+
posts: {
49+
create: [
50+
{ id: 1, title: 'Post 1' },
51+
{ id: 2, title: 'Post 2', published: true },
52+
],
53+
},
54+
},
55+
});
56+
await prisma.user.create({
57+
data: {
58+
id: 2,
59+
role: 'Admin',
60+
age: 28,
61+
private: true,
62+
posts: {
63+
create: [{ id: 3, title: 'Post 3', viewCount: 100 }],
64+
},
65+
},
66+
});
67+
68+
const dbUser1 = withPolicy({ id: 1 });
69+
const dbUser2 = withPolicy({ id: 2 });
70+
71+
await expect(
72+
dbUser1.user.findMany({
73+
where: { id: 2, AND: { age: { gt: 20 } } },
74+
})
75+
).resolves.toHaveLength(0);
76+
77+
await expect(
78+
dbUser2.user.findMany({
79+
where: { id: 2, AND: { age: { gt: 20 } } },
80+
})
81+
).resolves.toHaveLength(1);
82+
83+
await expect(
84+
dbUser1.user.findMany({
85+
where: {
86+
AND: { age: { gt: 10 } },
87+
OR: [{ age: { gt: 25 } }, { age: { lt: 20 } }],
88+
NOT: { private: true },
89+
},
90+
})
91+
).resolves.toHaveLength(1);
92+
93+
await expect(
94+
dbUser2.user.findMany({
95+
where: {
96+
AND: { age: { gt: 10 } },
97+
OR: [{ age: { gt: 25 } }, { age: { lt: 20 } }],
98+
NOT: { private: true },
99+
},
100+
})
101+
).resolves.toHaveLength(1);
102+
103+
// to-many relation query
104+
await expect(
105+
dbUser1.user.findMany({
106+
where: { posts: { some: { published: true } } },
107+
})
108+
).resolves.toHaveLength(1);
109+
await expect(
110+
dbUser1.user.findMany({
111+
where: { posts: { some: { AND: [{ published: true }, { viewCount: { gt: 0 } }] } } },
112+
})
113+
).resolves.toHaveLength(0);
114+
await expect(
115+
dbUser2.user.findMany({
116+
where: { posts: { some: { AND: [{ published: false }, { viewCount: { gt: 0 } }] } } },
117+
})
118+
).resolves.toHaveLength(1);
119+
await expect(
120+
dbUser1.user.findMany({
121+
where: { posts: { every: { published: true } } },
122+
})
123+
).resolves.toHaveLength(0);
124+
await expect(
125+
dbUser1.user.findMany({
126+
where: { posts: { none: { published: true } } },
127+
})
128+
).resolves.toHaveLength(0);
129+
130+
// to-one relation query
131+
await expect(
132+
dbUser1.post.findMany({
133+
where: { user: { role: 'Admin' } },
134+
})
135+
).resolves.toHaveLength(0);
136+
await expect(
137+
dbUser1.post.findMany({
138+
where: { user: { is: { role: 'Admin' } } },
139+
})
140+
).resolves.toHaveLength(0);
141+
await expect(
142+
dbUser1.post.findMany({
143+
where: { user: { isNot: { role: 'User' } } },
144+
})
145+
).resolves.toHaveLength(0);
146+
});
147+
});

‎tests/integration/tests/enhancements/with-policy/relation-one-to-one-filter.test.ts

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -206,7 +206,7 @@ describe('With Policy: relation one-to-one filter', () => {
206206
},
207207
},
208208
})
209-
).toResolveTruthy();
209+
).toResolveFalsy();
210210

211211
// m1 with m2 and m3
212212
await db.m1.create({
@@ -257,7 +257,7 @@ describe('With Policy: relation one-to-one filter', () => {
257257
},
258258
},
259259
})
260-
).toResolveTruthy();
260+
).toResolveFalsy();
261261
});
262262

263263
it('direct object filter', async () => {

0 commit comments

Comments
 (0)