refactor: clean up transaction semantics for entity mutation hooks #206
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughRefactors query execution to introduce interceptor-based flow and enhanced mutation hooks with transactional controls. Adds a shared Kysely alias utility, a forceTransaction client helper, safer commit callbacks, and multi-provider test coverage. Updates plugin APIs to pass a client to hooks and support in-transaction after-mutation execution. Minor TODO doc edits. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60–90 minutes Possibly related PRs
Poem
✨ Finishing Touches🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
Status, Documentation and Community
|
|
Claude finished @ymc9's task —— View job PR Review: Transaction Semantics for Entity Mutation HooksI've completed a thorough analysis of this PR that refactors transaction semantics for entity mutation hooks. Here's my comprehensive review: Todo List:
✅ Strengths1. Excellent Transaction Architecture
2. Sophisticated Hook Execution Logic
3. Comprehensive Test Coverage
4. Clean Plugin Architecture
|
There was a problem hiding this comment.
Pull Request Overview
This PR refactors entity mutation hooks to improve transaction semantics by providing better control over when hooks execute and access to a database client within hooks.
Key changes include:
- Enhanced hook transaction control through a new
runAfterMutationWithinTransactionoption - Added database client access to hook callbacks for performing additional operations
- Expanded test coverage to validate transaction behavior across different scenarios
Reviewed Changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/runtime/test/plugin/entity-mutation-hooks.test.ts | Adds comprehensive tests for new transaction semantics with both SQLite and PostgreSQL |
| packages/runtime/test/client-api/name-mapping.test.ts | Updates to use multi-provider test setup and fixes concat function usage |
| packages/runtime/src/client/plugin.ts | Adds new transaction control option and client parameter to hook interfaces |
| packages/runtime/src/client/executor/zenstack-query-executor.ts | Major refactor implementing new transaction semantics and client provisioning for hooks |
| packages/runtime/src/client/executor/zenstack-driver.ts | Adds error handling for transaction commit callbacks |
| packages/runtime/src/client/executor/name-mapper.ts | Refactors to use shared alias stripping utility |
| packages/runtime/src/client/executor/kysely-utils.ts | Introduces utility function for stripping aliases from query nodes |
| packages/runtime/src/client/client-impl.ts | Adds transaction forcing capability |
| TODO.md | Minor documentation updates and reorganization |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
packages/runtime/src/client/executor/zenstack-query-executor.ts
Outdated
Show resolved
Hide resolved
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai why don't you automatically review PR sent to the dev branch? |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain@ymc9 Let me check your current CodeRabbit configuration to understand why automatic reviews aren't happening for PRs to the dev branch. 🛠️ Debug Information Gathered @ymc9 Based on your current CodeRabbit configuration, automatic reviews are only enabled for PRs targeting the default branch (typically Looking at your configuration, I can see that auto_review:
enabled: true
auto_incremental_review: true
# ... other settings
base_branches: [] # This is why dev branch PRs aren't automatically reviewedTo enable automatic reviews for PRs targeting the Option 1: Review only dev branch PRs (in addition to default branch) reviews:
auto_review:
base_branches: ["dev"]Option 2: Review all branches reviews:
auto_review:
base_branches: [".*"]Option 3: Review specific branches reviews:
auto_review:
base_branches: ["dev", "staging", "release.*"]You can update your Once you update the configuration, CodeRabbit will automatically review PRs targeting the specified branches in addition to your default branch. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/runtime/src/client/plugin.ts (1)
96-110: Docs use singular “entity” while the API exposes plural “entities”The JSDoc for both hooks refers to
args.entity,args.beforeMutationEntity, andargs.afterMutationEntity, but the actual types defineentities,beforeMutationEntities, andafterMutationEntities(arrays). This will confuse plugin authors.- /** - * Called before an entity is mutated. - * @param args.entity Only available if `loadBeforeMutationEntities` is set to true in the - * return value of {@link RuntimePlugin.mutationInterceptionFilter}. - */ + /** + * Called before entities are mutated. + * @param args.entities Only available if `loadBeforeMutationEntities` is set to true in the + * return value of {@link RuntimePlugin.mutationInterceptionFilter}. + */ beforeEntityMutation?: BeforeEntityMutationCallback<Schema>; - /** - * Called after an entity is mutated. - * @param args.beforeMutationEntity Only available if `loadBeforeMutationEntities` is set to true in the - * return value of {@link RuntimePlugin.mutationInterceptionFilter}. - * @param args.afterMutationEntity Only available if `loadAfterMutationEntities` is set to true in the - * return value of {@link RuntimePlugin.mutationInterceptionFilter}. - */ + /** + * Called after entities are mutated. + * @param args.beforeMutationEntities Only available if `loadBeforeMutationEntities` is set to true in the + * return value of {@link RuntimePlugin.mutationInterceptionFilter}. + * @param args.afterMutationEntities Only available if `loadAfterMutationEntities` is set to true in the + * return value of {@link RuntimePlugin.mutationInterceptionFilter}. + */ afterEntityMutation?: AfterEntityMutationCallback<Schema>;
♻️ Duplicate comments (2)
packages/runtime/src/client/executor/zenstack-query-executor.ts (2)
216-216: Hard-coded isolation levelThis repeats a previous concern: the
'repeatable read'isolation level is hard-coded. Consider making this configurable (e.g., via client options) or using the driver’s default.
256-264: Avoid mutating executor internals from outside the constructor
innerExecutor.suppressMutationHooks = trueis a direct field mutation. Prefer a constructor parameter or a dedicatedwithSuppressedMutationHooks()factory to keep the executor immutable and prevent accidental misuse.Would you like me to propose a minimal refactor that threads this through the constructor and
with*methods?
🧹 Nitpick comments (15)
packages/runtime/src/client/executor/zenstack-driver.ts (2)
82-87: Prefer structured logging over console.error and preserve error objectsSwallowing callback errors is fine, but logging with console.error interpolates the error and loses stack/metadata. Use the existing Log instance for consistency and pass the error object directly.
- } catch (err) { - // errors in commit callbacks are logged but do not fail the commit - console.error(`Error executing transaction commit callback: ${err}`); - } + } catch (err) { + // errors in commit callbacks are logged but do not fail the commit + await this.#log.error(() => ({ + level: 'error', + error: err, + message: 'Error executing transaction commit callback', + })); + }
78-81: Commit-callback execution ordering and failure policy: document intentCallbacks currently run sequentially and independently; a failure is logged and does not short-circuit. If ordering doesn’t matter, Promise.allSettled could reduce tail latency. If ordering matters, keep as-is but document the guarantee.
Possible concurrent alternative:
- if (callbacks) { - for (const callback of callbacks) { - try { - await callback(); - } catch (err) { - await this.#log.error(() => ({ level: 'error', error: err, message: 'Error executing transaction commit callback' })); - } - } - } + if (callbacks?.length) { + const results = await Promise.allSettled(callbacks.map(cb => cb())); + for (const r of results) { + if (r.status === 'rejected') { + await this.#log.error(() => ({ level: 'error', error: r.reason, message: 'Error executing transaction commit callback' })); + } + } + }Also applies to: 90-95
TODO.md (2)
12-12: Fix list indentation to satisfy markdownlint MD007The linter flags inconsistent indentation for nested bullets. Normalize indentation per repo rules (expected vs actual in hints). Consider running markdownlint --fix or updating the MD007 config if intentional.
Minimal adjustments (illustrative; apply consistently across the file):
- - [x] typescript + - [x] typescript @@ - - [ ] Filtering + - [ ] Filtering - - [x] Unique fields - - [x] Scalar fields - - [x] Relation fields - - [ ] JSON filtering - - [ ] Full-text search + - [x] Unique fields + - [x] Scalar fields + - [x] Relation fields + - [ ] JSON filtering + - [ ] Full-text search @@ - - [ ] Extensibility + - [ ] Extensibility - - [x] Plugin + - [x] PluginAlternatively:
#!/usr/bin/env bash npx markdownlint-cli2 --fix TODO.mdAlso applies to: 35-41, 68-71
90-91: Clarify post-mutation hook semantics now that in-transaction execution is supportedThe TODO states “Post-mutation hooks should be called after transaction is committed” as done, but this PR introduces an option to run after-mutation within the transaction. Add a note reflecting the new toggle (default vs. opt-in) to avoid confusion.
Example:
-- [x] Post-mutation hooks should be called after transaction is committed +- [x] Post-mutation hooks called after transaction commit (default). + Option: runAfterMutationWithinTransaction to execute inside the transaction when needed.packages/runtime/src/client/executor/kysely-utils.ts (1)
7-14: Add an explicit return type for stripAliasMake the utility’s contract clear and improve IDE/helpfulness.
-export function stripAlias(node: OperationNode) { +export function stripAlias( + node: OperationNode, +): { alias: string | undefined; node: OperationNode } { if (AliasNode.is(node)) { invariant(IdentifierNode.is(node.alias), 'Expected identifier as alias'); return { alias: node.alias.name, node: node.node }; } else { return { alias: undefined, node }; } }packages/runtime/src/client/executor/name-mapper.ts (2)
98-107: Avoid non-null assertion when checking innerNodeThe non-null assertion operator is unnecessary; tighten the guard to eliminate potential undefined access and improve readability.
- const { alias, node: innerNode } = stripAlias(node.table); - if (TableNode.is(innerNode!)) { + const { alias, node: innerNode } = stripAlias(node.table); + if (innerNode && TableNode.is(innerNode)) { const modelName = innerNode.table.identifier.name; if (this.hasMappedColumns(modelName)) { // create a nested query with all fields selected and names mapped const select = this.createSelectAll(modelName); return { ...super.transformJoin(node), table: this.wrapAlias(select, alias ?? modelName) }; } }
176-185: Same non-null assertion nit as above in DeleteQuery transformMinor readability fix; align with the Join change.
- const { alias, node: innerNode } = stripAlias(from); - if (TableNode.is(innerNode!)) { + const { alias, node: innerNode } = stripAlias(from); + if (innerNode && TableNode.is(innerNode)) { // map table name return this.wrapAlias(this.processTableRef(innerNode), alias); } else { return super.transformNode(from); }packages/runtime/src/client/plugin.ts (2)
176-191: Client passthrough on hook argsExposing a client that automatically shares the mutation’s transaction context (and suppresses hooks) is a solid design. Consider explicitly stating that read queries also observe uncommitted changes when inside a transaction, subject to the isolation level.
Also applies to: 206-213
227-231: TightenProceedKyselyQueryFunctionreturn type
ProceedKyselyQueryFunctionreturnsPromise<QueryResult<any>>whileOnKyselyQueryCallbackreturnsPromise<QueryResult<UnknownRow>>. Consider aligning these toUnknownRowfor consistency and to avoid unnecessaryany.-export type ProceedKyselyQueryFunction = (query: RootOperationNode) => Promise<QueryResult<any>>; +export type ProceedKyselyQueryFunction = (query: RootOperationNode) => Promise<QueryResult<UnknownRow>>;packages/runtime/test/client-api/name-mapping.test.ts (3)
7-7: Avoid cross-provider DB name collisions
TEST_DBis the same for sqlite and postgresql runs. While your harness likely isolates per provider, suffixing with the provider makes parallel runs and debugging safer.-const TEST_DB = 'client-api-name-mapper-test'; +const TEST_DB = (provider: 'sqlite' | 'postgresql') => `client-api-name-mapper-test-${provider}`;And pass
dbName: TEST_DB(provider)increateTestClient.Also applies to: 9-13
146-158: Expectation couples Post.id with User.idThis assertion relies on both sequences starting at 1; it’s true in a fresh DB but can be brittle. Prefer asserting on
authorId(or select it) to validate the join, or assert only ontitle/- ).resolves.toMatchObject({ - id: user.id, - title: 'Post1', - email: 'u1@test.com', - }); + ).resolves.toMatchObject({ + title: 'Post1', + email: 'u1@test.com', + });
221-226: Same brittleness on delete RETURNINGThe assertion compares
Post.idtouser.id. Since you setPost.id = 1earlier, it currently works, but coupling across tables is accidental. Consider asserting only on stable fields (e.g.,title) or addauthorIdtoRETURNINGand match that.packages/runtime/test/plugin/entity-mutation-hooks.test.ts (3)
7-8: Per-provider DB naming for parallelism and claritySame suggestion as other test: include provider in
TEST_DBto avoid any accidental cross-provider interference and to ease test debugging.Also applies to: 9-13
436-454: Comment contradicts the assertion (nitpick)The comment says “mutation is persisted” but the assertion expects zero rows. Fix the comment to avoid confusion for future readers.
- // mutation is persisted + // mutation is NOT persisted
317-367: Duplicate test case: “triggers multiple afterEntityMutation hooks for multiple mutations”This scenario appears twice with effectively the same assertions. Consider keeping one to reduce test time and maintenance overhead.
Also applies to: 792-843
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (9)
TODO.md(3 hunks)packages/runtime/src/client/client-impl.ts(2 hunks)packages/runtime/src/client/executor/kysely-utils.ts(1 hunks)packages/runtime/src/client/executor/name-mapper.ts(6 hunks)packages/runtime/src/client/executor/zenstack-driver.ts(1 hunks)packages/runtime/src/client/executor/zenstack-query-executor.ts(15 hunks)packages/runtime/src/client/plugin.ts(3 hunks)packages/runtime/test/client-api/name-mapping.test.ts(2 hunks)packages/runtime/test/plugin/entity-mutation-hooks.test.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: CR
PR: zenstackhq/zenstack-v3#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-04T08:43:33.161Z
Learning: Plugin Architecture: Runtime plugins for query interception and entity mutation hooks
📚 Learning: 2025-08-04T08:43:33.161Z
Learnt from: CR
PR: zenstackhq/zenstack-v3#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-04T08:43:33.161Z
Learning: Kysely-Based ORM: V3 uses Kysely as query builder instead of Prisma runtime dependency
Applied to files:
packages/runtime/src/client/client-impl.ts
📚 Learning: 2025-08-04T08:43:33.161Z
Learnt from: CR
PR: zenstackhq/zenstack-v3#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-04T08:43:33.161Z
Learning: Plugin Architecture: Runtime plugins for query interception and entity mutation hooks
Applied to files:
packages/runtime/src/client/plugin.tspackages/runtime/src/client/executor/zenstack-query-executor.ts
📚 Learning: 2025-08-04T08:43:33.161Z
Learnt from: CR
PR: zenstackhq/zenstack-v3#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-04T08:43:33.161Z
Learning: Language-First Design: ZModel DSL compiles to TypeScript, not runtime code generation
Applied to files:
TODO.md
🪛 markdownlint-cli2 (0.17.2)
TODO.md
12-12: Unordered list indentation
Expected: 4; Actual: 8
(MD007, ul-indent)
35-35: Unordered list indentation
Expected: 4; Actual: 8
(MD007, ul-indent)
36-36: Unordered list indentation
Expected: 6; Actual: 12
(MD007, ul-indent)
37-37: Unordered list indentation
Expected: 6; Actual: 12
(MD007, ul-indent)
38-38: Unordered list indentation
Expected: 6; Actual: 12
(MD007, ul-indent)
39-39: Unordered list indentation
Expected: 6; Actual: 12
(MD007, ul-indent)
40-40: Unordered list indentation
Expected: 6; Actual: 12
(MD007, ul-indent)
68-68: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
69-69: Unordered list indentation
Expected: 4; Actual: 8
(MD007, ul-indent)
70-70: Unordered list indentation
Expected: 4; Actual: 8
(MD007, ul-indent)
71-71: Unordered list indentation
Expected: 4; Actual: 8
(MD007, ul-indent)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build-test (20.x)
🔇 Additional comments (10)
packages/runtime/src/client/executor/zenstack-driver.ts (1)
78-80: Good: delete callbacks before invocation to avoid re-triggeringDeleting the WeakMap entry before iterating prevents re-entrancy from re-registering and firing the same callbacks. Nice touch.
packages/runtime/src/client/executor/kysely-utils.ts (1)
8-11: LGTM: Centralized, type-safe alias unwrappingUsing Kysely’s node guards with invariant keeps failures explicit and reduces duplication across transformers.
packages/runtime/src/client/executor/name-mapper.ts (2)
154-169: Good defensive guard for UpdateQueryNode without tableEarly-returning when node.table is absent avoids unnecessary work and potential crashes before alias stripping.
307-315: Consistent alias handling via stripAlias looks solidCentralizing alias unwrapping reduces duplication and keeps the transformer lean. The scopes built from FromNode now reflect aliases accurately.
Also applies to: 322-337
packages/runtime/src/client/executor/zenstack-query-executor.ts (4)
78-85: Raw-query parameter propagation relies on non-typed $raw; verify robustness across Kysely versions
(compiledQuery as any).$rawis a fragile check and may break when Kysely changes internals. If this is an established convention in your codebase, fine; otherwise, consider a more principled detection (e.g., node-kind check) or add a small utility wrapper for tagging raw queries at creation time so you don’t reach intoany. At minimum, please verify this is intentional and covered by tests.Would you like me to draft a small utility (e.g.,
markRaw(compiledQuery)) and refactor call sites?
523-539: Potential alias mismatch when loading “before” entities
loadEntitiesmerges the mutation’swhere(which can contain aliased refs likeu.id) with aselectFrom(model)that has no alias. This can produce invalid SQL when updates/deletes used table aliases (e.g.,update "User as u"). Consider normalizing/remapping theWhereNodeto unaliased identifiers before merging, or building theselectFromwith a matching alias.I can add a focused test that performs
updateTable('User as u')with a plugin requestingloadBeforeMutationEntitiesand verify the behavior across providers. Want me to draft it?
251-253: Better error message with compiled SQLIncluding
compiled?.sqlin the thrownQueryErroris great for diagnosability. LGTM.
343-360: Robust model extraction with alias strippingThe use of
stripAliasandinvariantchecks across Insert/Update/Delete improves correctness when tables are aliased. Nicely done.packages/runtime/src/client/plugin.ts (1)
153-166: Transaction semantics flag is clear and usefulThe
runAfterMutationWithinTransactionaddition is well documented and matches the executor behavior. LGTM.packages/runtime/test/plugin/entity-mutation-hooks.test.ts (1)
369-571: Great coverage of transactional semantics and hook timingThe matrix of with/without outer transaction and with/without in-tx after-hooks is thorough and aligns with the documented behavior. Nicely done.
Also applies to: 574-789
|
Claude finished @ymc9's task —— View job PR Review: Transaction Semantics for Entity Mutation HooksI've completed a thorough analysis of this PR that refactors transaction semantics for entity mutation hooks. Here's my comprehensive review: Todo List:
✅ Strengths1. Excellent Transaction Architecture
2. Sophisticated Hook Execution Logic
3. Comprehensive Test Coverage
4. Clean Plugin Architecture
|
|
Claude finished @ymc9's task —— View job PR Review: Transaction Semantics for Entity Mutation HooksI've completed a thorough analysis of this PR that refactors transaction semantics for entity mutation hooks. Here's my comprehensive review: Todo List:
✅ Strengths1. Excellent Transaction Architecture
2. Sophisticated Hook Execution Logic
3. Comprehensive Test Coverage
4. Clean Plugin Architecture
|
Summary by CodeRabbit