Skip to content

Commit 49395f7

Browse files
ymc9ymc9
authored
perf(orm): avoid unnecessary pre-mutation read and transactions (#2484)
1 parent 7363096 commit 49395f7

15 files changed

Lines changed: 775 additions & 175 deletions

File tree

‎packages/orm/src/client/crud/dialects/postgresql.ts‎

Lines changed: 24 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -34,23 +34,39 @@ export class PostgresCrudDialect<Schema extends SchemaDef> extends LateralJoinDi
3434
if (this.options.fixPostgresTimezone !== false && !PostgresCrudDialect.typeParserOverrideApplied) {
3535
PostgresCrudDialect.typeParserOverrideApplied = true;
3636

37+
const fixTimezone = (value: unknown) => {
38+
if (typeof value !== 'string') {
39+
return value;
40+
}
41+
if (!this.hasTimezoneOffset(value)) {
42+
// force UTC if no offset
43+
value += 'Z';
44+
}
45+
const result = new Date(value);
46+
return isNaN(result.getTime())
47+
? value // fallback to original value if parsing fails
48+
: result;
49+
};
50+
3751
// override node-pg's default type parser to resolve the timezone handling issue
3852
// with "TIMESTAMP WITHOUT TIME ZONE" fields
3953
// https://github.com/brianc/node-postgres/issues/429
4054
import(/* webpackIgnore: true */ 'pg') // suppress bundler analysis warnings
4155
.then((pg) => {
42-
pg.types.setTypeParser(pg.types.builtins.TIMESTAMP, (value) => {
56+
// timestamp
57+
pg.types.setTypeParser(pg.types.builtins.TIMESTAMP, fixTimezone);
58+
pg.types.setTypeParser(1115, (value) => {
59+
// timestamp array
4360
if (typeof value !== 'string') {
4461
return value;
4562
}
46-
if (!this.hasTimezoneOffset(value)) {
47-
// force UTC if no offset
48-
value += 'Z';
63+
try {
64+
const arr = parsePostgresArray(value);
65+
return arr.map(fixTimezone);
66+
} catch {
67+
// fallback to original value if parsing fails
68+
return value;
4969
}
50-
const result = new Date(value);
51-
return isNaN(result.getTime())
52-
? value // fallback to original value if parsing fails
53-
: result;
5470
});
5571
})
5672
.catch(() => {

‎packages/orm/src/client/crud/operations/base.ts‎

Lines changed: 123 additions & 84 deletions
Large diffs are not rendered by default.

‎packages/orm/src/client/crud/operations/create.ts‎

Lines changed: 31 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -26,8 +26,10 @@ export class CreateOperationHandler<Schema extends SchemaDef> extends BaseOperat
2626
// analyze if we need to read back the created record, or just return the create result
2727
const { needReadBack, selectedFields } = this.mutationNeedsReadBack(this.model, args);
2828

29-
// TODO: avoid using transaction for simple create
30-
const result = await this.safeTransaction(async (tx) => {
29+
// analyze if the create involves nested creates
30+
const needsNestedCreate = this.needsNestedCreate(args.data);
31+
32+
const result = await this.safeTransactionIf(needReadBack || needsNestedCreate, async (tx) => {
3133
const createResult = await this.create(tx, this.model, args.data, undefined, false, selectedFields);
3234

3335
if (needReadBack) {
@@ -58,7 +60,10 @@ export class CreateOperationHandler<Schema extends SchemaDef> extends BaseOperat
5860
return { count: 0 };
5961
}
6062

61-
return this.safeTransaction((tx) => this.createMany(tx, this.model, args, false));
63+
// analyze if the create involves nested creates
64+
const needsNestedCreate = this.needsNestedCreate(args.data);
65+
66+
return this.safeTransactionIf(needsNestedCreate, (tx) => this.createMany(tx, this.model, args, false));
6267
}
6368

6469
private async runCreateManyAndReturn(args?: any) {
@@ -69,8 +74,10 @@ export class CreateOperationHandler<Schema extends SchemaDef> extends BaseOperat
6974
// analyze if we need to read back the created record, or just return the create result
7075
const { needReadBack, selectedFields } = this.mutationNeedsReadBack(this.model, args);
7176

72-
// TODO: avoid using transaction for simple create
73-
return this.safeTransaction(async (tx) => {
77+
// analyze if the create involves nested creates
78+
const needsNestedCreate = this.needsNestedCreate(args.data);
79+
80+
return this.safeTransactionIf(needReadBack || needsNestedCreate, async (tx) => {
7481
const createResult = await this.createMany(tx, this.model, args, true, undefined, selectedFields);
7582

7683
if (needReadBack) {
@@ -90,4 +97,23 @@ export class CreateOperationHandler<Schema extends SchemaDef> extends BaseOperat
9097
}
9198
});
9299
}
100+
101+
private needsNestedCreate(data: any) {
102+
const modelDef = this.requireModel(this.model);
103+
if (modelDef.baseModel) {
104+
// involve delegate base models
105+
return true;
106+
}
107+
108+
// has relation manipulation in the payload
109+
const hasRelation = Object.entries(data).some(([field, value]) => {
110+
const fieldDef = this.getField(this.model, field);
111+
return fieldDef?.relation && value !== undefined;
112+
});
113+
if (hasRelation) {
114+
return true;
115+
}
116+
117+
return false;
118+
}
93119
}

‎packages/orm/src/client/crud/operations/delete.ts‎

Lines changed: 29 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -20,8 +20,10 @@ export class DeleteOperationHandler<Schema extends SchemaDef> extends BaseOperat
2020
// analyze if we need to read back the deleted record, or just return delete result
2121
const { needReadBack, selectedFields } = this.mutationNeedsReadBack(this.model, args);
2222

23-
// TODO: avoid using transaction for simple delete
24-
const result = await this.safeTransaction(async (tx) => {
23+
// analyze if the delete involves nested deletes
24+
const needsNestedDelete = this.needsNestedDelete();
25+
26+
const result = await this.safeTransactionIf(needReadBack || needsNestedDelete, async (tx) => {
2527
let preDeleteRead: any = undefined;
2628
if (needReadBack) {
2729
preDeleteRead = await this.readUnique(tx, this.model, {
@@ -51,9 +53,33 @@ export class DeleteOperationHandler<Schema extends SchemaDef> extends BaseOperat
5153
}
5254

5355
async runDeleteMany(args?: any) {
54-
return await this.safeTransaction(async (tx) => {
56+
// analyze if the delete involves nested deletes
57+
const needsNestedDelete = this.needsNestedDelete();
58+
59+
return await this.safeTransactionIf(needsNestedDelete, async (tx) => {
5560
const result = await this.delete(tx, this.model, args?.where, args?.limit);
5661
return { count: Number(result.numAffectedRows ?? 0) };
5762
});
5863
}
64+
65+
private needsNestedDelete() {
66+
const modelDef = this.requireModel(this.model);
67+
if (modelDef.baseModel) {
68+
return true;
69+
}
70+
71+
// Check if any relation points to a delegate sub-model with cascade delete,
72+
// which would trigger processDelegateRelationDelete in BaseOperationHandler.delete()
73+
for (const fieldDef of Object.values(modelDef.fields)) {
74+
if (fieldDef.relation?.opposite) {
75+
const oppositeModelDef = this.requireModel(fieldDef.type);
76+
const oppositeRelation = this.requireField(fieldDef.type, fieldDef.relation.opposite);
77+
if (oppositeModelDef.baseModel && oppositeRelation.relation?.onDelete === 'Cascade') {
78+
return true;
79+
}
80+
}
81+
}
82+
83+
return false;
84+
}
5985
}

‎packages/orm/src/client/crud/operations/update.ts‎

Lines changed: 66 additions & 34 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,7 @@ import { match } from 'ts-pattern';
22
import type { GetModels, SchemaDef } from '../../../schema';
33
import type { WhereInput } from '../../crud-types';
44
import { createRejectedByPolicyError, RejectedByPolicyReason } from '../../errors';
5-
import { getIdValues } from '../../query-utils';
5+
import { getIdValues, requireIdFields } from '../../query-utils';
66
import { BaseOperationHandler } from './base';
77

88
export class UpdateOperationHandler<Schema extends SchemaDef> extends BaseOperationHandler<Schema> {
@@ -28,7 +28,10 @@ export class UpdateOperationHandler<Schema extends SchemaDef> extends BaseOperat
2828
// analyze if we need to read back the update record, or just return the updated result
2929
const { needReadBack, selectedFields } = this.needReadBack(args);
3030

31-
const result = await this.safeTransaction(async (tx) => {
31+
// analyze if the update involves nested updates
32+
const needsNestedUpdate = this.needsNestedUpdate(args.data);
33+
34+
const result = await this.safeTransactionIf(needReadBack || needsNestedUpdate, async (tx) => {
3235
const updateResult = await this.update(
3336
tx,
3437
this.model,
@@ -76,10 +79,11 @@ export class UpdateOperationHandler<Schema extends SchemaDef> extends BaseOperat
7679
return result;
7780
}
7881
}
79-
8082
private async runUpdateMany(args: any) {
81-
// TODO: avoid using transaction for simple update
82-
return this.safeTransaction(async (tx) => {
83+
// analyze if the update involves nested updates
84+
const needsNestedUpdate = this.needsNestedUpdate(args.data);
85+
86+
return this.safeTransactionIf(needsNestedUpdate, async (tx) => {
8387
return this.updateMany(tx, this.model, args.where, args.data, args.limit, false);
8488
});
8589
}
@@ -92,37 +96,43 @@ export class UpdateOperationHandler<Schema extends SchemaDef> extends BaseOperat
9296
// analyze if we need to read back the updated record, or just return the update result
9397
const { needReadBack, selectedFields } = this.needReadBack(args);
9498

95-
const { readBackResult, updateResult } = await this.safeTransaction(async (tx) => {
96-
const updateResult = await this.updateMany(
97-
tx,
98-
this.model,
99-
args.where,
100-
args.data,
101-
args.limit,
102-
true,
103-
undefined,
104-
undefined,
105-
selectedFields,
106-
);
99+
// analyze if the update involves nested updates
100+
const needsNestedUpdate = this.needsNestedUpdate(args.data);
107101

108-
if (needReadBack) {
109-
const readBackResult = await this.read(
102+
const { readBackResult, updateResult } = await this.safeTransactionIf(
103+
needReadBack || needsNestedUpdate,
104+
async (tx) => {
105+
const updateResult = await this.updateMany(
110106
tx,
111107
this.model,
112-
{
113-
select: args.select,
114-
omit: args.omit,
115-
where: {
116-
OR: updateResult.map((item) => getIdValues(this.schema, this.model, item) as any),
117-
},
118-
} as any, // TODO: fix type
108+
args.where,
109+
args.data,
110+
args.limit,
111+
true,
112+
undefined,
113+
undefined,
114+
selectedFields,
119115
);
120116

121-
return { readBackResult, updateResult };
122-
} else {
123-
return { readBackResult: updateResult, updateResult };
124-
}
125-
});
117+
if (needReadBack) {
118+
const readBackResult = await this.read(
119+
tx,
120+
this.model,
121+
{
122+
select: args.select,
123+
omit: args.omit,
124+
where: {
125+
OR: updateResult.map((item) => getIdValues(this.schema, this.model, item) as any),
126+
},
127+
} as any, // TODO: fix type
128+
);
129+
130+
return { readBackResult, updateResult };
131+
} else {
132+
return { readBackResult: updateResult, updateResult };
133+
}
134+
},
135+
);
126136

127137
if (readBackResult.length < updateResult.length && this.hasPolicyEnabled) {
128138
// some of the updated entities cannot be read back
@@ -140,6 +150,7 @@ export class UpdateOperationHandler<Schema extends SchemaDef> extends BaseOperat
140150
// analyze if we need to read back the updated record, or just return the update result
141151
const { needReadBack, selectedFields } = this.needReadBack(args);
142152

153+
// upsert is intrinsically multi-step and is always run in a transaction
143154
const result = await this.safeTransaction(async (tx) => {
144155
let mutationResult: unknown = await this.update(
145156
tx,
@@ -191,9 +202,11 @@ export class UpdateOperationHandler<Schema extends SchemaDef> extends BaseOperat
191202
return baseResult;
192203
}
193204

205+
const idFields = requireIdFields(this.schema, this.model);
206+
194207
if (!this.dialect.supportsReturning) {
195208
// if dialect doesn't support "returning", we always need to read back
196-
return { needReadBack: true, selectedFields: undefined };
209+
return { needReadBack: true, selectedFields: idFields };
197210
}
198211

199212
// further check if we're not updating any non-relation fields, because if so,
@@ -206,14 +219,33 @@ export class UpdateOperationHandler<Schema extends SchemaDef> extends BaseOperat
206219

207220
// update/updateMany payload
208221
if (args.data && !Object.keys(args.data).some((field) => nonRelationFields.includes(field))) {
209-
return { needReadBack: true, selectedFields: undefined };
222+
return { needReadBack: true, selectedFields: idFields };
210223
}
211224

212225
// upsert payload
213226
if (args.update && !Object.keys(args.update).some((field: string) => nonRelationFields.includes(field))) {
214-
return { needReadBack: true, selectedFields: undefined };
227+
return { needReadBack: true, selectedFields: idFields };
215228
}
216229

217230
return baseResult;
218231
}
232+
233+
private needsNestedUpdate(data: any) {
234+
const modelDef = this.requireModel(this.model);
235+
if (modelDef.baseModel) {
236+
// involve delegate base models
237+
return true;
238+
}
239+
240+
// has relation manipulation in the payload
241+
const hasRelation = Object.entries(data).some(([field, value]) => {
242+
const fieldDef = this.getField(this.model, field);
243+
return fieldDef?.relation && value !== undefined;
244+
});
245+
if (hasRelation) {
246+
return true;
247+
}
248+
249+
return false;
250+
}
219251
}

‎packages/orm/src/client/executor/zenstack-query-executor.ts‎

Lines changed: 14 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -126,6 +126,10 @@ export class ZenStackQueryExecutor extends DefaultQueryExecutor {
126126
return (this.client.$options.plugins ?? []).some((plugin) => plugin.onEntityMutation?.afterEntityMutation);
127127
}
128128

129+
private get hasOnKyselyHooks() {
130+
return (this.client.$options.plugins ?? []).some((plugin) => plugin.onKyselyQuery);
131+
}
132+
129133
// #endregion
130134

131135
// #region main entry point
@@ -135,11 +139,20 @@ export class ZenStackQueryExecutor extends DefaultQueryExecutor {
135139
// if the query is a raw query, we need to carry over the parameters
136140
const queryParams = (compiledQuery as any).$raw ? compiledQuery.parameters : undefined;
137141

142+
// needs to ensure transaction if we:
143+
// - have plugins with Kysely hooks, as they may spawn more queries (check: should creating tx be plugin's responsibility?)
144+
// - have entity mutation plugins
145+
const needEnsureTx = this.hasOnKyselyHooks || this.hasEntityMutationPlugins;
146+
138147
const result = await this.provideConnection(async (connection) => {
139148
let startedTx = false;
140149
try {
141150
// mutations are wrapped in tx if not already in one
142-
if (this.isMutationNode(compiledQuery.query) && !this.driver.isTransactionConnection(connection)) {
151+
if (
152+
this.isMutationNode(compiledQuery.query) &&
153+
!this.driver.isTransactionConnection(connection) &&
154+
needEnsureTx
155+
) {
143156
await this.driver.beginTransaction(connection, {
144157
isolationLevel: TransactionIsolationLevel.ReadCommitted,
145158
});

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

Lines changed: 0 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -72,10 +72,6 @@ export class PolicyHandler<Schema extends SchemaDef> extends OperationNodeTransf
7272
this.dialect = getCrudDialect(this.client.$schema, this.client.$options);
7373
}
7474

75-
get kysely() {
76-
return this.client.$qb;
77-
}
78-
7975
// #region main entry point
8076

8177
async handle(node: RootOperationNode, proceed: ProceedKyselyQueryFunction) {

0 commit comments

Comments
 (0)