Add SQLite persistent repository - #38
Conversation
|
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 ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis pull request adds a complete SQLite repository implementation as an optional feature for an event-sourced Rust framework. The PR includes schema migrations, the core ChangesSQLite Repository Feature
Sequence DiagramsequenceDiagram
participant Client
participant SqliteRepository
participant SQLTransaction
participant Pool
Client->>SqliteRepository: commit_batch(events, read_models)
SqliteRepository->>Pool: begin transaction
SQLTransaction->>SQLTransaction: check MAX(sequence) for version
SQLTransaction->>SQLTransaction: validate codecs and sequence numbers
SQLTransaction->>SQLTransaction: insert events<br/>(handle unique constraint→ConcurrentWrite)
SQLTransaction->>SQLTransaction: apply read-model writes<br/>(upsert with dedup)
SQLTransaction->>SQLTransaction: upsert snapshots
Pool->>SQLTransaction: commit
SqliteRepository-->>Client: ✓ streams marked committed
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
@coderabbitai review this |
|
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
🧹 Nitpick comments (2)
docs/async-repositories.md (1)
45-46: ⚡ Quick winConsider adding a brief note about enabling the feature flag.
While the documentation mentions the feature is optional, users might benefit from a quick reminder that they need to enable it in
Cargo.toml:sourced_rust = { version = "...", features = ["sqlite"] }This would make the documentation more self-contained for developers unfamiliar with the codebase.
🤖 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 `@docs/async-repositories.md` around lines 45 - 46, Add a short note telling users to enable the optional sqlite feature in Cargo.toml so the exported SqliteRepository is available; mention the exact feature name ("sqlite") and show that they should add a dependency entry like sourced_rust with features = ["sqlite"] in their Cargo.toml, and place this note near the sentence that introduces SqliteRepository in docs/async-repositories.md so readers see how to enable the feature flag.src/sqlite_repo/mod.rs (1)
1004-1015: 💤 Low valueConsider logging or flagging malformed timestamps.
system_time_from_storagesilently returnsUNIX_EPOCHwhen parsing fails. While this provides graceful degradation, it could mask data corruption issues. Since timestamps are metadata and not critical for correctness, this tradeoff is acceptable, but you may want to add tracing/logging for observability in production.🤖 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 `@src/sqlite_repo/mod.rs` around lines 1004 - 1015, The function system_time_from_storage currently swallows malformed timestamp strings and returns UNIX_EPOCH silently; update it to emit an observability signal (e.g., tracing::warn! or log::warn!) whenever parsing fails so malformed values are visible in logs. Specifically, inside system_time_from_storage, when split_once('.') returns None, when secs.parse::<u64>() fails, or when nanos.parse::<u32>() fails, log a warning that includes the original value and the parsing error/context (referencing the value, secs, nanos parsing attempts) and then continue returning UNIX_EPOCH to preserve current 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.
Nitpick comments:
In `@docs/async-repositories.md`:
- Around line 45-46: Add a short note telling users to enable the optional
sqlite feature in Cargo.toml so the exported SqliteRepository is available;
mention the exact feature name ("sqlite") and show that they should add a
dependency entry like sourced_rust with features = ["sqlite"] in their
Cargo.toml, and place this note near the sentence that introduces
SqliteRepository in docs/async-repositories.md so readers see how to enable the
feature flag.
In `@src/sqlite_repo/mod.rs`:
- Around line 1004-1015: The function system_time_from_storage currently
swallows malformed timestamp strings and returns UNIX_EPOCH silently; update it
to emit an observability signal (e.g., tracing::warn! or log::warn!) whenever
parsing fails so malformed values are visible in logs. Specifically, inside
system_time_from_storage, when split_once('.') returns None, when
secs.parse::<u64>() fails, or when nanos.parse::<u32>() fails, log a warning
that includes the original value and the parsing error/context (referencing the
value, secs, nanos parsing attempts) and then continue returning UNIX_EPOCH to
preserve current behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: c396a1ab-ea6a-44b7-be85-d8a325e0118b
📒 Files selected for processing (6)
Cargo.tomldocs/async-repositories.mdmigrations/sqlite/0001_initial.sqlsrc/lib.rssrc/sqlite_repo/mod.rstests/sqlite_repository/main.rs
Implements [[tasks/extract-shared-sqlx-repository-helpers]]
Summary
SqliteRepositorywith explicit SQLite migrationsVerification
cargo checkcargo fmt --checkgit diff --checkcargo test --test sqlite_repository --features sqlitecargo test --all-featurescargo clippy --lib --all-features -- -D warningscargo clippy --test sqlite_repository --features sqlite -- -D warningsNote:
cargo clippy --all-targets --all-features -- -D warningsstill fails on pre-existing unrelated test-target lints intests/todos,tests/bomberman, andtests/sagas.Summary by CodeRabbit
Release Notes
New Features
Documentation
Tests