perf(orm): avoid unnecessary pre-mutation read and transactions#2484
Open
perf(orm): avoid unnecessary pre-mutation read and transactions#2484
Conversation
Contributor
|
Claude finished @ymc9's task —— View job PR Review: Performance Optimization for ORM TransactionsTodo List:
SummaryThis PR implements an excellent performance optimization that conditionally avoids unnecessary transactions and pre-mutation reads in the ORM layer. The approach is well-designed and maintains data consistency while significantly reducing overhead for simple operations. ✅ StrengthsSmart Transaction Management
Intelligent Read-Back DetectionThe
Comprehensive Test CoverageThe new atomicity tests (tests/e2e/orm/client-api/atomicity.test.ts) provide excellent coverage:
Clean Architecture
🔍 Code Quality ObservationsMinor Improvement Opportunities
🛡️ Security & Data ConsistencyExcellent Safety Measures
No Security Concerns Identified
🚀 Performance ImpactExpected Improvements
No Performance RegressionsThe conditional logic adds minimal overhead while providing substantial gains for the common case of simple mutations. 🧪 Test Coverage AssessmentThe atomicity tests are comprehensive and cover critical scenarios:
Final Recommendation: ✅ APPROVEDThis is an excellent performance optimization that:
The implementation demonstrates deep understanding of database transaction semantics and ORM optimization patterns. The conditional transaction approach is exactly what you'd want in a production ORM. |
Contributor
📝 WalkthroughWalkthroughIntroduces lazy-loading for update operations with improved return semantics, adds nested operation detection for conditional transaction wrapping across create/update/delete handlers, refactors PostgreSQL timezone parsing, implements hook-aware transaction management in the query executor, removes a public API getter from PolicyHandler, and adds comprehensive atomicity test coverage. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/e2e/orm/policy/now-function.test.ts (1)
100-123:⚠️ Potential issue | 🟡 MinorTest intent and assertion path are now misaligned.
Line 122 passes an explicit
createdAt, so this case no longer verifies the “default now()” behavior described by the test name/comment (Line 100 and Line 120). Please either rename/update the description or add a separate assertion that omitscreatedAtto keep default-path coverage explicit.Suggested minimal wording fix
-it('combines now() default with auth in create policy', async () => { +it('combines now() with auth in create policy', async () => { @@ - // authenticated user with auto-filled createdAt - allowed + // authenticated user with explicit createdAt <= now() - allowed🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/orm/policy/now-function.test.ts` around lines 100 - 123, The test currently passes an explicit createdAt which prevents verifying Log.createdAt `@default`(now()) behavior; update the test that uses createPolicyTestClient and db.$setAuth({ id: 1 }).log.create to include an additional assertion that omits createdAt (e.g. call db.$setAuth({ id: 1 }).log.create({ data: { message: 'test' } }) and assert it resolves and that the returned createdAt is recent), or alternatively rename the test to reflect that it supplies createdAt — but prefer adding the separate no-createdAt assertion to keep default(now()) coverage for the Log model and @@allow('create', createdAt <= now() && auth() != null) policy.
🧹 Nitpick comments (1)
tests/e2e/orm/client-api/atomicity.test.ts (1)
28-37: Consider tightening scenario-specific failure assertions.For tests explicitly targeting unique-constraint rollback (e.g., Line 28 and similar cases in this file),
.rejects.toThrow()is broad and may pass on unrelated errors. Prefer asserting duplicate/constraint semantics (code or message pattern) to keep these tests high-signal.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/orm/client-api/atomicity.test.ts` around lines 28 - 37, The test currently uses a broad .rejects.toThrow() for the client.post.create call in atomicity.test.ts; narrow this to assert the unique-constraint failure specifically by checking the error code/message instead (e.g., assert Prisma's unique-constraint error like code "P2002" or a message regex such as /Unique constraint failed|duplicate/i). Update the expectation around client.post.create({ data: { title: 'Post1', author: { create: { email: 'u1@test.com' } } } }) to assert the specific duplicate/constraint semantics (for example using .rejects.toMatchObject({ code: 'P2002' }) or .rejects.toThrow(/unique|P2002/i/regexp/), depending on how errors are surfaced in this test harness) so the test only passes for the intended rollback/constraint scenario.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/orm/src/client/crud/operations/delete.ts`:
- Around line 65-68: needsNestedDelete currently returns true only when
modelDef.baseModel is set, but it must also detect when
BaseOperationHandler.delete() will recurse via processDelegateRelationDelete();
update needsNestedDelete() to return true if the model has any
delegate/cascading relations (e.g., check modelDef.relations or
modelDef.delegateRelations for entries with delegate=true or onDelete ===
'cascade') so that runDelete and runDeleteMany keep these cases inside the
nested/transactional path; reference needsNestedDelete(),
BaseOperationHandler.delete(), processDelegateRelationDelete(), runDelete and
runDeleteMany when making this change.
In `@packages/orm/src/client/executor/zenstack-query-executor.ts`:
- Around line 142-155: The auto-transaction gating variable needEnsureTx
currently considers only this.hasOnKyselyHooks and
this.hasEntityMutationPluginsWithAfterMutationHooks; include plugins that define
before-entity-mutation hooks so those hooks run inside the ensured transaction
as well. Change the definition of needEnsureTx to also OR in the before-mutation
flag (e.g. this.hasEntityMutationPluginsWithBeforeMutationHooks) so the
subsequent mutation branch (in provideConnection / isMutationNode /
driver.isTransactionConnection) will start a transaction when
beforeEntityMutation hooks are present.
---
Outside diff comments:
In `@tests/e2e/orm/policy/now-function.test.ts`:
- Around line 100-123: The test currently passes an explicit createdAt which
prevents verifying Log.createdAt `@default`(now()) behavior; update the test that
uses createPolicyTestClient and db.$setAuth({ id: 1 }).log.create to include an
additional assertion that omits createdAt (e.g. call db.$setAuth({ id: 1
}).log.create({ data: { message: 'test' } }) and assert it resolves and that the
returned createdAt is recent), or alternatively rename the test to reflect that
it supplies createdAt — but prefer adding the separate no-createdAt assertion to
keep default(now()) coverage for the Log model and @@allow('create', createdAt
<= now() && auth() != null) policy.
---
Nitpick comments:
In `@tests/e2e/orm/client-api/atomicity.test.ts`:
- Around line 28-37: The test currently uses a broad .rejects.toThrow() for the
client.post.create call in atomicity.test.ts; narrow this to assert the
unique-constraint failure specifically by checking the error code/message
instead (e.g., assert Prisma's unique-constraint error like code "P2002" or a
message regex such as /Unique constraint failed|duplicate/i). Update the
expectation around client.post.create({ data: { title: 'Post1', author: {
create: { email: 'u1@test.com' } } } }) to assert the specific
duplicate/constraint semantics (for example using .rejects.toMatchObject({ code:
'P2002' }) or .rejects.toThrow(/unique|P2002/i/regexp/), depending on how errors
are surfaced in this test harness) so the test only passes for the intended
rollback/constraint scenario.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: bc21a6ee-604f-482a-835e-1b39f51a7539
📒 Files selected for processing (15)
packages/orm/src/client/crud/dialects/postgresql.tspackages/orm/src/client/crud/operations/base.tspackages/orm/src/client/crud/operations/create.tspackages/orm/src/client/crud/operations/delete.tspackages/orm/src/client/crud/operations/update.tspackages/orm/src/client/executor/zenstack-query-executor.tspackages/plugins/policy/src/policy-handler.tstests/e2e/orm/client-api/atomicity.test.tstests/e2e/orm/client-api/delegate.test.tstests/e2e/orm/client-api/type-coverage.test.tstests/e2e/orm/client-api/update.test.tstests/e2e/orm/plugin-infra/entity-mutation-hooks.test.tstests/e2e/orm/policy/migrated/nested-to-many.test.tstests/e2e/orm/policy/now-function.test.tstests/e2e/tsconfig.json
💤 Files with no reviewable changes (3)
- tests/e2e/orm/plugin-infra/entity-mutation-hooks.test.ts
- packages/plugins/policy/src/policy-handler.ts
- tests/e2e/orm/client-api/type-coverage.test.ts
Comment on lines
+65
to
+68
| private needsNestedDelete() { | ||
| const modelDef = this.requireModel(this.model); | ||
| return !!modelDef.baseModel; | ||
| } |
Contributor
There was a problem hiding this comment.
Treat delegate-cascade deletes as nested work too.
Line 67 only checks baseModel, but BaseOperationHandler.delete() can also recurse through processDelegateRelationDelete() before issuing the final delete. In that case runDelete and runDeleteMany now 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
Verify each finding against the current code and only fix it if needed.
In `@packages/orm/src/client/crud/operations/delete.ts` around lines 65 - 68,
needsNestedDelete currently returns true only when modelDef.baseModel is set,
but it must also detect when BaseOperationHandler.delete() will recurse via
processDelegateRelationDelete(); update needsNestedDelete() to return true if
the model has any delegate/cascading relations (e.g., check modelDef.relations
or modelDef.delegateRelations for entries with delegate=true or onDelete ===
'cascade') so that runDelete and runDeleteMany keep these cases inside the
nested/transactional path; reference needsNestedDelete(),
BaseOperationHandler.delete(), processDelegateRelationDelete(), runDelete and
runDeleteMany when making this change.
Comment on lines
+142
to
+155
| // 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 | ||
| ) { |
Contributor
There was a problem hiding this comment.
Include beforeEntityMutation hooks in needEnsureTx.
Line 145 now skips the auto-transaction when a plugin only has beforeEntityMutation. Those hooks still run later with a connection-scoped client, so any writes they perform can commit independently if the main mutation then fails or rolls back. That opens a partial-commit path for mutation hooks.
🛠️ 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
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // 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 | |
| ) { | |
| // 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.hasEntityMutationHooks; | |
| 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) && | |
| needEnsureTx | |
| ) { |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/orm/src/client/executor/zenstack-query-executor.ts` around lines 142
- 155, The auto-transaction gating variable needEnsureTx currently considers
only this.hasOnKyselyHooks and
this.hasEntityMutationPluginsWithAfterMutationHooks; include plugins that define
before-entity-mutation hooks so those hooks run inside the ensured transaction
as well. Change the definition of needEnsureTx to also OR in the before-mutation
flag (e.g. this.hasEntityMutationPluginsWithBeforeMutationHooks) so the
subsequent mutation branch (in provideConnection / isMutationNode /
driver.isTransactionConnection) will start a transaction when
beforeEntityMutation hooks are present.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Tests