(MOT-4235) feat(database): add row-change events and error hints - #614
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Warning Review limit reached
Next review available in: 21 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (37)
📝 WalkthroughWalkthroughReplaces the PostgreSQL logical-replication row-change trigger with a driver-agnostic ChangesRow-changed trigger delivery
Estimated code review effort: 5 (Critical) | ~90 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant DatabaseWorker
participant RowChangeBus
participant TriggerFunction
Client->>DatabaseWorker: execute mutation
DatabaseWorker->>RowChangeBus: emit or stage row change
DatabaseWorker->>RowChangeBus: commit staged changes
RowChangeBus->>TriggerFunction: dispatch event
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
skill-check — worker0 verified, 49 skipped (no docs/).
Four for four. Nicely done. |
![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
🧹 Nitpick comments (4)
database/src/driver/sqlite.rs (1)
316-331: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
returningarm ofstatement_returns_rowsis now dead at everyvalidate_returningcall site.Once
validate_returninghas passed,!returning.is_empty()impliescolumn_count() > 0, so the first disjunct instatement_returns_rowscan never decide routing inexecute/tx_execute. Consider dropping the parameter and routing purely oncolumn_count()to keep one source of truth.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@database/src/driver/sqlite.rs` around lines 316 - 331, Update statement_returns_rows and its execute/tx_execute call sites to route solely from the statement’s column_count(), removing the now-redundant returning-based condition and parameter if unused. Preserve validate_returning as the single validation for requested RETURNING clauses.database/tests/e2e/README.md (1)
105-118: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueLayout table omits the new case file. Sibling case files are listed; add
workers/harness/src/cases-row-changed.tsfor consistency.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@database/tests/e2e/README.md` around lines 105 - 118, Add workers/harness/src/cases-row-changed.ts to the Layout table in database/tests/e2e/README.md, listing it as the corresponding row-change case file alongside the other workers/harness/src case files.database/src/triggers/sql.rs (1)
216-224: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDoc claims quoted references match, but
barenever strips quotes. A binding configured withtable: "\"Orders\""(or`orders`) won't match the classifier's unquotedorders. Either trim"/`/[]inbare, or soften the doc comment to unquoted references only.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@database/src/triggers/sql.rs` around lines 216 - 224, Update the nested bare helper in same_table to normalize quoted table identifiers by stripping surrounding double quotes, backticks, or square brackets before lowercasing and comparing. Preserve schema-qualifier removal and the documented matching behavior for quoted and unquoted references.database/src/triggers/bus.rs (1)
265-320: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winDispatch is awaited, so it isn't really fire-and-forget.
commit/emitblock the calling handler for up todispatch_timeout_msper subscriber per event; a commit with several staged statements and a couple of slow subscribers adds seconds tocommitTransaction. Consider draining the buffer and spawning one task that fans out in order, keeping statement ordering intact while returning to the caller immediately.♻️ Sketch (requires `RowChangeBus` in an `Arc` at the call sites)
- pub async fn commit(&self, transaction_id: &str) { - let staged = self.lock().take_pending(transaction_id); - for p in staged { - self.fan_out(RowChangedEvent { .. }).await; - } - } + pub fn commit(self: &Arc<Self>, transaction_id: &str) { + let staged = self.lock().take_pending(transaction_id); + if staged.is_empty() { + return; + } + let bus = Arc::clone(self); + tokio::spawn(async move { + for p in staged { + bus.fan_out(RowChangedEvent { /* … */ }).await; + } + }); + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@database/src/triggers/bus.rs` around lines 265 - 320, Update RowChangeBus::commit and its emit/dispatch flow so draining pending events schedules fan_out work in a background task and returns without awaiting subscriber dispatch. Preserve staged statement ordering by having the spawned task process events sequentially, and adjust call sites or ownership as needed to provide RowChangeBus through Arc while retaining rollback behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@database/src/handlers/commit_transaction.rs`:
- Around line 70-73: Update the commit flow around commit_row_changes to release
the transaction’s pinned connection guard before awaiting subscriber/event
delivery. Ensure the guard is explicitly dropped after the durable commit and
before dispatching staged events, while preserving the existing statement-order
flush and successful CommitTxResp behavior.
In `@database/src/handlers/transaction.rs`:
- Around line 119-122: Update the transaction batch loop over sql_texts and
steps to preserve step.rows when emitting events: extend the batch step result
with the returned column names, convert each row into a JSON object, and pass
the resulting data instead of None to emit_row_change. Keep None only when no
RETURNING rows are available.
In `@database/src/triggers/sql.rs`:
- Around line 151-159: Update strip_conflict_clause to parse the optional “OR
<action>” prefix while treating consecutive whitespace, including newlines, as a
single separator. Ensure the returned SQL begins at the actual table name for
inputs such as repeated spaces, and preserve the existing behavior for
statements without an OR prefix.
---
Nitpick comments:
In `@database/src/driver/sqlite.rs`:
- Around line 316-331: Update statement_returns_rows and its execute/tx_execute
call sites to route solely from the statement’s column_count(), removing the
now-redundant returning-based condition and parameter if unused. Preserve
validate_returning as the single validation for requested RETURNING clauses.
In `@database/src/triggers/bus.rs`:
- Around line 265-320: Update RowChangeBus::commit and its emit/dispatch flow so
draining pending events schedules fan_out work in a background task and returns
without awaiting subscriber dispatch. Preserve staged statement ordering by
having the spawned task process events sequentially, and adjust call sites or
ownership as needed to provide RowChangeBus through Arc while retaining rollback
behavior.
In `@database/src/triggers/sql.rs`:
- Around line 216-224: Update the nested bare helper in same_table to normalize
quoted table identifiers by stripping surrounding double quotes, backticks, or
square brackets before lowercasing and comparing. Preserve schema-qualifier
removal and the documented matching behavior for quoted and unquoted references.
In `@database/tests/e2e/README.md`:
- Around line 105-118: Add workers/harness/src/cases-row-changed.ts to the
Layout table in database/tests/e2e/README.md, listing it as the corresponding
row-change case file alongside the other workers/harness/src case files.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: b6df332f-18dc-4a53-858b-ae8acb8781a0
⛔ Files ignored due to path filters (1)
database/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (35)
.github/workflows/database-e2e.ymldatabase/Cargo.tomldatabase/README.mddatabase/skills/SKILL.mddatabase/src/config.rsdatabase/src/driver/postgres.rsdatabase/src/driver/sqlite.rsdatabase/src/error.rsdatabase/src/handlers/begin_transaction.rsdatabase/src/handlers/commit_transaction.rsdatabase/src/handlers/execute.rsdatabase/src/handlers/list_databases.rsdatabase/src/handlers/mod.rsdatabase/src/handlers/prepare.rsdatabase/src/handlers/query.rsdatabase/src/handlers/rollback_transaction.rsdatabase/src/handlers/run_statement.rsdatabase/src/handlers/transaction.rsdatabase/src/handlers/transaction_execute.rsdatabase/src/main.rsdatabase/src/transaction.rsdatabase/src/triggers/bus.rsdatabase/src/triggers/handler.rsdatabase/src/triggers/mod.rsdatabase/src/triggers/row_change.rsdatabase/src/triggers/sql.rsdatabase/tests/e2e/README.mddatabase/tests/e2e/docker-compose.ymldatabase/tests/e2e/run-tests.shdatabase/tests/e2e/workers/harness/src/cases-interactive-tx.tsdatabase/tests/e2e/workers/harness/src/cases-row-change.tsdatabase/tests/e2e/workers/harness/src/cases-row-changed.tsdatabase/tests/e2e/workers/harness/src/database-config.tsdatabase/tests/e2e/workers/harness/src/runner.tsdatabase/tests/integration.rs
💤 Files with no reviewable changes (5)
- database/Cargo.toml
- database/tests/e2e/workers/harness/src/cases-row-change.ts
- database/tests/e2e/docker-compose.yml
- database/src/triggers/row_change.rs
- database/src/error.rs
45ffa82 to
f7905a6
Compare
f7905a6 to
66527f2
Compare
2 participants
Summary
database::row-changetrigger withdatabase::row-changedfor mutations issued through the database worker on SQLite, PostgreSQL, and MySQL.RETURNINGrows, and suppress rolled-back or zero-row writes.returningrequests when SQLite statements cannot produce rows.Why
The previous trigger depended on PostgreSQL logical replication support that was unavailable, leaving SQLite and MySQL without row-change notifications. Since worker-issued writes already pass through the database handlers, emitting after successful commit provides consistent cross-driver behavior without claiming to be external change-data capture.
Impact
Consumers can subscribe to committed worker-issued mutations by database, optional table, and optional operation list. Transaction rollbacks and failed commits remain silent, subscriber failures cannot undo committed writes, and error responses provide more accurate schema guidance.
Validation
cargo fmt --all -- --checkcargo clippy --all-targets --all-features -- -D warningscargo test --all-featuresFixes MOT-4235