feat: persist read models as relational rows#44
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughMigrates read models from document-style APIs to relational ReadModelWritePlan-based row mutations; updates docs, migrations, commit-builder, in-memory and SQL repositories, session/mutation model, table SQL ordering, CI, tests, and Bomberman handlers to use upsert/patch/delete and processed-message idempotency. ChangesRelational Read-Model Write Plans
Sequence Diagram(s) sequenceDiagram
participant CommitBuilder
participant ReadModelWritePlan
participant Repository
participant SQL
CommitBuilder->>ReadModelWritePlan: read_models(upsert/patch/delete, mark_processed)
ReadModelWritePlan->>Repository: commit(plan)
Repository->>SQL: apply mutations + processed_message marks (atomic)
SQL-->>Repository: success / unique_violation -> dedupe
Repository-->>CommitBuilder: outcome
Estimated code review effort 🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
✨ Finishing Touches🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@tests/bomberman/handlers/join_game.rs`:
- Line 28: The code indexes map.spawn_points with spawn_index (`let (sx, sy) =
map.spawn_points[spawn_index];`) which can panic for out-of-range indices;
update the join game handler (e.g., the function handling the join request) to
validate spawn_index before indexing and return an appropriate GameError instead
of panicking: check that spawn_index is within map.spawn_points.len(), and if
not, return Err(GameError::InvalidSpawnIndex { index: spawn_index }) (or
construct the project’s equivalent GameError) otherwise safely index and
proceed.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 494b8e3f-0442-46c4-a257-8b4286fb6345
📒 Files selected for processing (53)
README.mddocs/async-repositories.mddocs/postgres-event-store.mddocs/read-models.mdmigrations/postgres/0001_initial.sqlmigrations/sqlite/0001_initial.sqlsrc/commit_builder/mod.rssrc/hashmap_repo/repository.rssrc/lib.rssrc/lock/lock_manager.rssrc/lock/mod.rssrc/postgres_repo/mod.rssrc/read_model/in_memory.rssrc/read_model/mod.rssrc/read_model/queued.rssrc/read_model/repository.rssrc/read_model/session.rssrc/read_model/store.rssrc/repository/async_repository.rssrc/repository/mod.rssrc/sqlite_repo/mod.rssrc/sqlx_repo/mod.rssrc/table/mod.rstests/async_repository/main.rstests/bomberman/commands.rstests/bomberman/domain/bomb.rstests/bomberman/domain/explosion.rstests/bomberman/domain/game_map.rstests/bomberman/domain/player.rstests/bomberman/domain/tick_saga.rstests/bomberman/handlers/create_game.rstests/bomberman/handlers/get_player.rstests/bomberman/handlers/join_game.rstests/bomberman/handlers/mod.rstests/bomberman/handlers/move_player.rstests/bomberman/handlers/place_bomb.rstests/bomberman/handlers/shared.rstests/bomberman/handlers/tick.rstests/bomberman/main.rstests/bomberman/sim.rstests/bomberman/views.rstests/hashmap_repository_conformance/main.rstests/persistent_repository_conformance/read_models.rstests/postgres_repository/main.rstests/read_model_commit_bridge/main.rstests/read_model_distributed_idempotency/main.rstests/read_model_document_conformance/main.rstests/read_model_metadata/main.rstests/read_model_relationship_includes/main.rstests/read_model_session/main.rstests/read_models/main.rstests/read_models/views.rstests/sqlite_repository/main.rs
💤 Files with no reviewable changes (8)
- src/read_model/store.rs
- tests/read_models/views.rs
- src/read_model/repository.rs
- tests/read_models/main.rs
- tests/read_model_document_conformance/main.rs
- migrations/sqlite/0001_initial.sql
- tests/bomberman/commands.rs
- src/read_model/queued.rs
Collaborator
Author
|
Follow-up pushed in
Additional verification:
|
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@src/commit_builder/mod.rs`:
- Around line 34-38: The async example moves the variable read_models into
.read_models(read_models) causing a use-after-move; update the docs to rebuild
or recreate read_models (or use a cloned/owned copy) before the async snippet so
the second block can call .read_models(read_models) and then .commit(&mut game).
Specifically, split the examples into two independent blocks or explicitly
recreate the plan/read_models between the synchronous and async examples so
.read_models(read_models) does not consume a moved value.
- Around line 536-562: The async extension traits use starter method names that
collide with the sync traits; rename the async entrypoints to avoid ambiguity
(e.g., change AsyncCommitBuilderExt::outbox to outbox_async and
AsyncReadModelWritePlanCommitExt::read_models to read_models_async), update any
chained calls that relied on AsyncCommitBuilder::new(...).outbox / .read_models
to use the new names, and adjust any callers (including places that call
.aggregate() through AsyncCommitBuilder) to import/use the renamed methods;
ensure the trait impls (impl<R: AsyncTransactionalCommit> AsyncCommitBuilderExt
for R {} and impl<R: AsyncTransactionalCommit> AsyncReadModelWritePlanCommitExt
for R {}) remain and that documentation/comments are updated accordingly so
callers can use ergonomic method chaining without UFCS.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: cf561606-eed3-4f38-a005-f9fa86afd2b7
📒 Files selected for processing (8)
README.mddocs/read-models.mdsrc/commit_builder/mod.rssrc/lib.rssrc/read_model/mod.rstests/persistent_repository_conformance/read_models.rstests/postgres_repository/main.rstests/sqlite_repository/main.rs
✅ Files skipped from review due to trivial changes (1)
- docs/read-models.md
🚧 Files skipped from review as they are similar to previous changes (4)
- tests/persistent_repository_conformance/read_models.rs
- tests/sqlite_repository/main.rs
- README.md
- src/lib.rs
Remove the generic document read-model store path and lower read-model write plans into declared relational tables for in-memory, SQLite, and Postgres repositories. Update docs, migrations, tests, and the Bomberman example to use relational read-model schemas and handlers. Verified with cargo fmt --all, cargo test --test bomberman --all-features, cargo test --all-features, and git diff --check.
Add async commit builder entrypoints for read-model, outbox, and aggregate staging so async SQL repositories can use repo.read_models(plan).commit(&mut aggregate).await. Update SQLite/Postgres tests and docs to exercise the direct async read-model plus aggregate transaction shape.
patrickleet
force-pushed
the
feat/readmodel-persistence
branch
from
b7bf7c6 to
2ae3ece
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
tests/bomberman/handlers/join_game.rs (1)
28-28:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winGuard spawn-point access to avoid panic on invalid input.
Line 28 directly indexes
spawn_points; an out-of-rangespawn_indexwill panic instead of returning a domain error.Proposed fix
- let (sx, sy) = map.spawn_points[spawn_index]; + let (sx, sy) = map + .spawn_points + .get(spawn_index) + .copied() + .ok_or_else(|| GameError::InvalidSpawnIndex(spawn_index))?;🤖 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 `@tests/bomberman/handlers/join_game.rs` at line 28, Guard the direct indexing of map.spawn_points to avoid panics: check that spawn_index is within bounds (spawn_index < map.spawn_points.len()) before doing let (sx, sy) = map.spawn_points[spawn_index]; and if out of range return the appropriate domain error/result used by the surrounding test/harness (the same error type returned by the join handling code) instead of panicking. Update the code paths around spawn_index usage in the join_game test to use this boundary check so invalid input yields a controllable error rather than an index panic.
🤖 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.
Duplicate comments:
In `@tests/bomberman/handlers/join_game.rs`:
- Line 28: Guard the direct indexing of map.spawn_points to avoid panics: check
that spawn_index is within bounds (spawn_index < map.spawn_points.len()) before
doing let (sx, sy) = map.spawn_points[spawn_index]; and if out of range return
the appropriate domain error/result used by the surrounding test/harness (the
same error type returned by the join handling code) instead of panicking. Update
the code paths around spawn_index usage in the join_game test to use this
boundary check so invalid input yields a controllable error rather than an index
panic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 81822d11-2f0d-4de9-96d9-9f862742ff4b
📒 Files selected for processing (53)
README.mddocs/async-repositories.mddocs/postgres-event-store.mddocs/read-models.mdmigrations/postgres/0001_initial.sqlmigrations/sqlite/0001_initial.sqlsrc/commit_builder/mod.rssrc/hashmap_repo/repository.rssrc/lib.rssrc/lock/lock_manager.rssrc/lock/mod.rssrc/postgres_repo/mod.rssrc/read_model/in_memory.rssrc/read_model/mod.rssrc/read_model/queued.rssrc/read_model/repository.rssrc/read_model/session.rssrc/read_model/store.rssrc/repository/async_repository.rssrc/repository/mod.rssrc/sqlite_repo/mod.rssrc/sqlx_repo/mod.rssrc/table/mod.rstests/async_repository/main.rstests/bomberman/commands.rstests/bomberman/domain/bomb.rstests/bomberman/domain/explosion.rstests/bomberman/domain/game_map.rstests/bomberman/domain/player.rstests/bomberman/domain/tick_saga.rstests/bomberman/handlers/create_game.rstests/bomberman/handlers/get_player.rstests/bomberman/handlers/join_game.rstests/bomberman/handlers/mod.rstests/bomberman/handlers/move_player.rstests/bomberman/handlers/place_bomb.rstests/bomberman/handlers/shared.rstests/bomberman/handlers/tick.rstests/bomberman/main.rstests/bomberman/sim.rstests/bomberman/views.rstests/hashmap_repository_conformance/main.rstests/persistent_repository_conformance/read_models.rstests/postgres_repository/main.rstests/read_model_commit_bridge/main.rstests/read_model_distributed_idempotency/main.rstests/read_model_document_conformance/main.rstests/read_model_metadata/main.rstests/read_model_relationship_includes/main.rstests/read_model_session/main.rstests/read_models/main.rstests/read_models/views.rstests/sqlite_repository/main.rs
💤 Files with no reviewable changes (8)
- src/read_model/store.rs
- src/read_model/repository.rs
- tests/read_model_document_conformance/main.rs
- migrations/sqlite/0001_initial.sql
- tests/read_models/main.rs
- tests/bomberman/commands.rs
- tests/read_models/views.rs
- src/read_model/queued.rs
✅ Files skipped from review due to trivial changes (8)
- tests/read_model_metadata/main.rs
- src/table/mod.rs
- src/lock/mod.rs
- docs/postgres-event-store.md
- src/lock/lock_manager.rs
- README.md
- docs/read-models.md
- docs/async-repositories.md
🚧 Files skipped from review as they are similar to previous changes (29)
- src/repository/mod.rs
- migrations/postgres/0001_initial.sql
- tests/bomberman/handlers/mod.rs
- tests/hashmap_repository_conformance/main.rs
- tests/read_model_commit_bridge/main.rs
- src/repository/async_repository.rs
- tests/read_model_relationship_includes/main.rs
- tests/bomberman/sim.rs
- tests/bomberman/handlers/move_player.rs
- tests/bomberman/handlers/tick.rs
- src/sqlx_repo/mod.rs
- tests/bomberman/domain/game_map.rs
- tests/bomberman/domain/tick_saga.rs
- tests/read_model_session/main.rs
- tests/bomberman/main.rs
- tests/bomberman/handlers/shared.rs
- tests/persistent_repository_conformance/read_models.rs
- tests/async_repository/main.rs
- tests/bomberman/domain/player.rs
- src/lib.rs
- tests/sqlite_repository/main.rs
- src/read_model/mod.rs
- src/hashmap_repo/repository.rs
- src/read_model/session.rs
- tests/postgres_repository/main.rs
- src/postgres_repo/mod.rs
- src/read_model/in_memory.rs
- src/sqlite_repo/mod.rs
- src/commit_builder/mod.rs
Implements [[tasks/async-distributed-read-model-tests]]
Collaborator
Author
|
Added the async distributed read-model coverage after the rebase. What changed:
Verification:
|
Fixes [[postgres-read-model-schema-bootstrap-order]]
Collaborator
Author
|
Updated this branch with Postgres schema-per-test isolation and CI Postgres service coverage.\n\nWhat changed:\n- Added a shared Postgres integration-test helper that creates a unique schema and sets search_path for each pooled connection.\n- Switched postgres_repository, postgres_repository_conformance, and the async distributed read-model Postgres test to use per-schema isolation.\n- Fixed read-model table bootstrap SQL to create parent tables before FK dependents.\n- Added dedicated Postgres integration jobs to PR and main GitHub Actions workflows.\n- Cleaned up commit-builder test helper clippy warnings so all-target clippy is clean.\n\nLocal verification against Postgres container:\n- DATABASE_URL=postgres://postgres:postgres@127.0.0.1:36662/sourced_rust_test cargo test --test postgres_repository --all-features\n- DATABASE_URL=postgres://postgres:postgres@127.0.0.1:36662/sourced_rust_test cargo test --test postgres_repository_conformance --all-features\n- DATABASE_URL=postgres://postgres:postgres@127.0.0.1:36662/sourced_rust_test cargo test --test distributed_read_model --all-features\n- cargo test --lib table::sql --all-features\n- cargo clippy --all-targets --all-features -- -D warnings\n- DATABASE_URL=postgres://postgres:postgres@127.0.0.1:36662/sourced_rust_test cargo test --all-features\n- git diff --check |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 @.github/workflows/on-pr-quality.yaml:
- Around line 35-36: The workflow uses floating refs for actions/checkout and
dtolnay/rust-toolchain and omits disabling credential persistence; update the
checkout step that uses actions/checkout@v4 to a pinned ref (specific tag or
commit SHA) and add persist-credentials: false to that step, and likewise pin
dtolnay/rust-toolchain to a fixed tag/commit instead of the unstable "stable"
ref so both actions are immutable and checkout won't leak credentials.
In @.github/workflows/on-push-main-version-and-tag.yaml:
- Around line 41-42: Replace floating action refs with pinned commit SHAs for
both actions/checkout@v4 and dtolnay/rust-toolchain@stable and disable checkout
credential persistence by adding persist-credentials: false to the
actions/checkout step; specifically, update the checkout step reference
(actions/checkout@v4) to its full commit SHA and set persist-credentials: false,
and update the dtolnay/rust-toolchain@stable reference to a specific commit SHA
to avoid floating tags.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 28d5cc29-5ef3-4761-90c8-dd1743f7fbe5
📒 Files selected for processing (8)
.github/workflows/on-pr-quality.yaml.github/workflows/on-push-main-version-and-tag.yamlsrc/commit_builder/mod.rssrc/table/sql.rstests/distributed_read_model/main.rstests/postgres_repository/main.rstests/postgres_repository_conformance/main.rstests/support/postgres.rs
🚧 Files skipped from review as they are similar to previous changes (3)
- tests/postgres_repository/main.rs
- tests/distributed_read_model/main.rs
- src/commit_builder/mod.rs
This was referenced May 29, 2026
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
BoardView,#[sourced]aggregates, and splithandlers/modulesBreaking Changes
ReadModelStore, document mutations, queued document read-model store, and the generic SQL document tableVerification
cargo fmt --allcargo test --test bomberman --all-featurescargo test --all-featuresgit diff --checkSummary by CodeRabbit