Add tracked read model relationship includes - #36
Merged
Merged
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 PR adds opt-in, primary-key–anchored relationship includes for relational read models: macro-generated include hooks, in-memory relational row storage and schema registry, load_graph-based include loading, session unit-of-work tracking with diff-based save_changes, docs, and tests. ChangesRelational Read Model Explicit Includes
Sequence DiagramsequenceDiagram
participant App
participant ReadModelSession
participant InMemoryStore
participant SchemaRegistry
App->>ReadModelSession: load(root_key).include("weapons")
ReadModelSession->>InMemoryStore: load_graph(request)
InMemoryStore->>SchemaRegistry: resolve_request_schemas()
SchemaRegistry-->>InMemoryStore: Player & Weapon schemas
InMemoryStore->>InMemoryStore: load root row by id
InMemoryStore->>InMemoryStore: load related weapons (HasMany)
InMemoryStore-->>ReadModelSession: ReadModelLoadGraph
ReadModelSession->>ReadModelSession: hydrate_include("weapons")
ReadModelSession-->>App: Player with weapons Vec
App->>App: mutate player.name & weapons
App->>ReadModelSession: save_changes()
ReadModelSession->>ReadModelSession: diff_rows vs tracked
ReadModelSession-->>App: write mutations
App->>InMemoryStore: commit(plan)
InMemoryStore->>InMemoryStore: validate versions
InMemoryStore->>InMemoryStore: apply UpsertRow/PatchRow
InMemoryStore-->>App: success
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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 |
Collaborator
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@docs/read-models.md`:
- Around line 116-140: The PlayerView schema shown earlier is missing the
relationship and field referenced by the example (player.weapons and
.include("weapons")); update the PlayerView struct (the ReadModel definition
named PlayerView) to add a weapons field typed Vec<PlayerWeaponView> and
annotate it with the read-model relationship metadata your macro uses (e.g.
has_many/foreign_key or the equivalent attribute your ReadModel macro expects)
so that include("weapons") and accessing player.weapons in the example compile
and resolve to PlayerWeaponView.
In `@src/read_model/in_memory.rs`:
- Around line 601-621: The helper belongs_to_target_column currently tries to
pick a single column when the target has a composite primary key which leads to
building an invalid partial RowKey; change belongs_to_target_column to
immediately reject any composite primary key (primary_key.columns.len() != 1) by
returning a ReadModelError::Metadata indicating the target model and
source_column, and only return the sole primary key column when
primary_key.columns.len() == 1; also update any callers (e.g.,
load_belongs_to_rows) to rely on that error instead of receiving a single column
from a composite key.
- Around line 174-181: The InsertMissing branch writes
mutation.patch.into_values() directly which can omit primary-key or required
non-null fields; update the None if matches!(mutation.mode,
PatchMode::InsertMissing) branch so you build a complete StoredRow before
inserting: derive a full values map by starting from the key (the same key used
for staged_rows.insert) and overlaying mutation.patch.into_values(), validate
that all primary-key columns and non-null required columns are present (or
return an error/skip the insert), then insert StoredRow { values: full_values,
version: INITIAL_MODEL_VERSION }; reference PatchMode::InsertMissing,
mutation.patch.into_values(), staged_rows.insert, StoredRow,
INITIAL_MODEL_VERSION and the consumer key_from_row to locate where to change.
🪄 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: 51755964-bc69-46a3-8f5c-f70e0b02f837
📒 Files selected for processing (9)
docs/read-models.mdsourced_rust_macros/src/read_model.rssrc/lib.rssrc/read_model/in_memory.rssrc/read_model/metadata.rssrc/read_model/mod.rssrc/read_model/session.rstests/read_model_distributed_idempotency/main.rstests/read_model_relationship_includes/main.rs
Collaborator
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/read_model_session/main.rs (1)
186-199: ⚡ Quick winAssert that failed commit does not persist a partial row.
This checks the error text, but not write atomicity. Add a post-failure load assertion (
None) so regressions that write before failing are caught.Proposed test hardening
#[test] fn insert_missing_patch_rejects_partial_new_row() { let store = InMemoryReadModelStore::new(); + store.register_schema::<AccountSummary>().unwrap(); let patch = RowPatch::new().set("owner", RowValue::String("Grace".into())); let mut session = ReadModelSession::new(); session .upsert_patch::<AccountSummary>(account_key("acct-1"), patch) .unwrap(); let err = session.commit(&store).unwrap_err(); assert!( matches!(err, ReadModelError::Metadata(message) if message.contains("missing required column `balance_cents`")) ); + + let mut read_models = store.session(); + let loaded = read_models + .load::<AccountSummary>(account_key("acct-1")) + .one() + .unwrap(); + assert!(loaded.is_none()); }🤖 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/read_model_session/main.rs` around lines 186 - 199, The test insert_missing_patch_rejects_partial_new_row currently verifies the commit error but not that a partial write wasn't persisted; after capturing err from session.commit(&store).unwrap_err(), load the row from the InMemoryReadModelStore (using the same key used in upsert_patch, e.g. account_key("acct-1")) via the store.load::<AccountSummary>(...) or the appropriate read method and assert it returns None to ensure atomicity; use the same types (ReadModelSession, InMemoryReadModelStore, AccountSummary, account_key, commit) to locate where to add this post-failure assertion.
🤖 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/read_model/in_memory.rs`:
- Around line 225-239: The patch loop in row_values_from_key_and_patch currently
lets patch_values overwrite primary-key columns copied from the RowKey; update
row_values_from_key_and_patch to reject any patch entry that targets a
primary-key column unless the patched value exactly equals the value from the
provided key (return an appropriate ReadModelError on mismatch), mirroring the
same PK-guard used in the existing-row patch path; keep copying non-PK columns
as before and then call validate_row_values(schema, &values, true) and return
Ok(values).
---
Nitpick comments:
In `@tests/read_model_session/main.rs`:
- Around line 186-199: The test insert_missing_patch_rejects_partial_new_row
currently verifies the commit error but not that a partial write wasn't
persisted; after capturing err from session.commit(&store).unwrap_err(), load
the row from the InMemoryReadModelStore (using the same key used in
upsert_patch, e.g. account_key("acct-1")) via the
store.load::<AccountSummary>(...) or the appropriate read method and assert it
returns None to ensure atomicity; use the same types (ReadModelSession,
InMemoryReadModelStore, AccountSummary, account_key, commit) to locate where to
add this post-failure assertion.
🪄 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: 504282a1-f45f-4ef1-b28c-53f6605aedc1
📒 Files selected for processing (5)
docs/read-models.mdsrc/read_model/in_memory.rssrc/read_model/session.rstests/read_model_relationship_includes/main.rstests/read_model_session/main.rs
✅ Files skipped from review due to trivial changes (1)
- docs/read-models.md
Collaborator
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Collaborator
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Make `save_changes` reconcile included collections to the struct: an owned has_many child dropped from the loaded Vec is deleted, lowering to an explicit DeleteRow with the loaded expected version. belongs_to clear-to-None stays a no-op on the target. Safe because has_many includes load the complete owned set. Replaces the prior "removal does not delete by default" behavior, which was asymmetric (auto-persisted adds/edits but silently dropped removals). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Rework tests/distributed_read_model into a Catalog + Order CQRS slice over normalized relational read models (ProductView, OrderView has_many OrderLineView belongs_to ProductView, JSONB columns), and add a kanban Board + Cards example. Add an order-fulfillment saga (inventory, payment, saga orchestrator) driving confirm/cancel with a compensation path, projected into an OrderFulfillmentStepView has_many child for a multi-include query. Conventions: each write service is a microsvc::Service with service.rs + handlers/ (one file per message) + models/ (aggregate); the projection service is one dispatcher organized into handler modules; published domain events are lowercase dot-namespaced. Services publish via the outbox and subscribe via microsvc::subscribe — no bespoke transport. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This was referenced May 24, 2026
Merged
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
Verification
Summary by CodeRabbit
New Features
Documentation
Tests