-
-
Notifications
You must be signed in to change notification settings - Fork 131
perf(orm): avoid unnecessary pre-mutation read and transactions #2484
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.
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
base: dev
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -126,6 +126,10 @@ export class ZenStackQueryExecutor extends DefaultQueryExecutor { | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return (this.client.$options.plugins ?? []).some((plugin) => plugin.onEntityMutation?.afterEntityMutation); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| private get hasOnKyselyHooks() { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return (this.client.$options.plugins ?? []).some((plugin) => plugin.onKyselyQuery); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // #endregion | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // #region main entry point | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -135,11 +139,20 @@ export class ZenStackQueryExecutor extends DefaultQueryExecutor { | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // if the query is a raw query, we need to carry over the parameters | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const queryParams = (compiledQuery as any).$raw ? compiledQuery.parameters : undefined; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // needs to ensure transaction if we: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // - have plugins with Kysely hooks, as they may spawn more queries (check: should creating tx be plugin's responsibility?) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // - have entity mutation plugins that consume post-mutation entities | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const needEnsureTx = this.hasOnKyselyHooks || this.hasEntityMutationPluginsWithAfterMutationHooks; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const result = await this.provideConnection(async (connection) => { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| let startedTx = false; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| try { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // mutations are wrapped in tx if not already in one | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (this.isMutationNode(compiledQuery.query) && !this.driver.isTransactionConnection(connection)) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if ( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| this.isMutationNode(compiledQuery.query) && | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| !this.driver.isTransactionConnection(connection) && | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| needEnsureTx | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+142
to
+155
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Include Line 145 now skips the auto-transaction when a plugin only has 🛠️ Suggested fix+ private get hasEntityMutationHooks() {
+ return (this.client.$options.plugins ?? []).some(
+ (plugin) =>
+ plugin.onEntityMutation?.beforeEntityMutation || plugin.onEntityMutation?.afterEntityMutation,
+ );
+ }
+
override async executeQuery(compiledQuery: CompiledQuery) {
// proceed with the query with kysely interceptors
// if the query is a raw query, we need to carry over the parameters
const queryParams = (compiledQuery as any).$raw ? compiledQuery.parameters : undefined;
// needs to ensure transaction if we:
// - have plugins with Kysely hooks, as they may spawn more queries (check: should creating tx be plugin's responsibility?)
// - have entity mutation plugins that consume post-mutation entities
- const needEnsureTx = this.hasOnKyselyHooks || this.hasEntityMutationPluginsWithAfterMutationHooks;
+ const needEnsureTx = this.hasOnKyselyHooks || this.hasEntityMutationHooks;📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| await this.driver.beginTransaction(connection, { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| isolationLevel: TransactionIsolationLevel.ReadCommitted, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Treat delegate-cascade deletes as nested work too.
Line 67 only checks
baseModel, butBaseOperationHandler.delete()can also recurse throughprocessDelegateRelationDelete()before issuing the final delete. In that caserunDeleteandrunDeleteManynow skip the transaction and can commit the recursive delete even if the last statement fails.🛠️ Suggested fix
private needsNestedDelete() { const modelDef = this.requireModel(this.model); - return !!modelDef.baseModel; + if (modelDef.baseModel) { + return true; + } + + return Object.values(modelDef.fields).some((fieldDef) => { + if (!fieldDef.relation?.opposite) { + return false; + } + + const oppositeModelDef = this.getModel(fieldDef.type); + if (!oppositeModelDef?.baseModel) { + return false; + } + + const oppositeRelation = this.requireField(fieldDef.type, fieldDef.relation.opposite); + return oppositeRelation.relation?.onDelete === 'Cascade'; + }); }🤖 Prompt for AI Agents