docs: specify Postgres repository contract#29
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis PR expands ChangesPostgres Event Store Contract Specification
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
docs/postgres-event-store.md (2)
178-180: 💤 Low valueConsider delimiter collision risk in interim ID formatting.
The interim solution formats
idas"{aggregate_type}:{aggregate_id}"forConcurrentWriteerrors. If aggregate types or IDs can contain colons, this formatting could be ambiguous. Since line 180 acknowledges this is temporary and "a later API can split type and ID if needed," consider using a less collision-prone delimiter (e.g.,|, tab, or JSON encoding) if this interim solution lasts longer than expected.🤖 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/postgres-event-store.md` around lines 178 - 180, The current interim formatting of the ConcurrentWrite error uses string concat "{aggregate_type}:{aggregate_id}" which risks collision if either part contains ':', so update the code that constructs RepositoryError::ConcurrentWrite { id, expected, actual } to use a collision-resistant encoding (for example encode id as JSON object like {"type":"...","id":"..."} serialized to a string, or use a less common delimiter such as '|' or '\t'); locate the code that formats the id for RepositoryError::ConcurrentWrite and replace the concat with the chosen encoding/delimiter and add a brief comment referencing this is an interim format to be split by a future API.
90-120: 💤 Low valueConsider documenting snapshot retention strategy.
The primary key
(aggregate_type, aggregate_id, version)allows multiple historical snapshots per stream, which is useful for auditing. However, the contract doesn't specify whether implementations should retain all snapshots indefinitely, implement a retention policy (e.g., keep last N), or replace snapshots. If this flexibility is intentional, consider adding a brief note so implementers know retention is an implementation decision.🤖 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/postgres-event-store.md` around lines 90 - 120, Add a short note to the Snapshot Table section clarifying that the primary key (aggregate_type, aggregate_id, version) permits multiple historical snapshots and that the specification intentionally does not mandate retention semantics; suggest common strategies implementers may choose (retain all for audit, keep last N, time-based expiry, or replace on new snapshot) and recommend documenting chosen policy per implementation so consumers know snapshot lifecycle expectations for aggregate_snapshots, version, and hydration 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 `@docs/postgres-event-store.md`:
- Around line 45-61: The contract for the aggregate_events table adds
payload_codec and payload_codec_version but doesn't specify codec validation
behavior; update the docs to add a "Codec validation and error model" subsection
that explicitly states whether validation occurs on write, on read, or both,
names the responsible components (e.g., Repository.write/Repository.read and
Serializer.deserialize/Serializer.validate), defines the exact error types or
error codes returned for codec mismatches, and documents the policy for allowing
unknown codecs on write (forward-compatibility) versus rejecting them (safety),
referencing the payload_codec and payload_codec_version fields and the
aggregate_events table so tests that assert "codec rejection for unknown
payload_codec or payload_codec_version" have a clear contract to validate
against.
- Around line 62-70: Update the schema constraints to enforce non-null,
non-empty aggregate identifiers by marking aggregate_type and aggregate_id NOT
NULL and adding CHECK clauses such as CHECK (aggregate_type <> '') and CHECK
(aggregate_id <> ''); ensure these are included alongside the existing PRIMARY
KEY (aggregate_type, aggregate_id, sequence) and the other CHECKs (sequence > 0,
event_version > 0, payload_codec <> '', payload_codec_version > 0) so the
database itself prevents empty/NULL aggregate_type and aggregate_id values.
---
Nitpick comments:
In `@docs/postgres-event-store.md`:
- Around line 178-180: The current interim formatting of the ConcurrentWrite
error uses string concat "{aggregate_type}:{aggregate_id}" which risks collision
if either part contains ':', so update the code that constructs
RepositoryError::ConcurrentWrite { id, expected, actual } to use a
collision-resistant encoding (for example encode id as JSON object like
{"type":"...","id":"..."} serialized to a string, or use a less common delimiter
such as '|' or '\t'); locate the code that formats the id for
RepositoryError::ConcurrentWrite and replace the concat with the chosen
encoding/delimiter and add a brief comment referencing this is an interim format
to be split by a future API.
- Around line 90-120: Add a short note to the Snapshot Table section clarifying
that the primary key (aggregate_type, aggregate_id, version) permits multiple
historical snapshots and that the specification intentionally does not mandate
retention semantics; suggest common strategies implementers may choose (retain
all for audit, keep last N, time-based expiry, or replace on new snapshot) and
recommend documenting chosen policy per implementation so consumers know
snapshot lifecycle expectations for aggregate_snapshots, version, and hydration
behavior.
🪄 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: e433dab8-93f0-48e8-9f45-45856d4503ad
📒 Files selected for processing (1)
docs/postgres-event-store.md
Implements [[tasks/design-postgres-repository-contract]]
patrickleet
force-pushed
the
docs/postgres-repository-contract
branch
from
e547569 to
b145f6c
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
docs/postgres-event-store.md (2)
59-59: 💤 Low valueConsider showing explicit DEFAULT in the schema DDL documentation.
Line 59 mentions
metadatawith "default{}" and lines 250-252 state migrations should writemetadata jsonb NOT NULL DEFAULT '{}', but the constraint section (lines 67-75) doesn't show this DEFAULT clause. For completeness, consider adding it to the schema documentation or constraints section.📋 Optional schema refinement
Update line 59 or add to the constraints section:
-| `metadata` | `jsonb` | Event metadata, default `{}`. | +| `metadata` | `jsonb` | Event metadata; `NOT NULL DEFAULT '{}'`. |Also applies to: 250-252
🤖 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/postgres-event-store.md` at line 59, Update the schema docs to explicitly show the DEFAULT for the metadata column: where `metadata` is described as `jsonb` (the column symbol `metadata`) include the `NOT NULL DEFAULT '{}'` clause in the DDL examples and in the constraints section; likewise update the migration guidance that currently states `metadata jsonb NOT NULL DEFAULT '{}'` so the constraints block and any sample CREATE TABLE/ALTER statements consistently show `DEFAULT '{}'` for `metadata`.
161-163: 💤 Low valueConsider clarifying snapshot version validation during hydration.
The hydration algorithm specifies loading the newest snapshot then replaying events where
sequence > snapshot.version. Consider adding guidance for the edge case wheresnapshot.versionexceeds the current maximum event sequence (which could indicate data inconsistency or corruption).📋 Suggested clarification
After line 163, add:
If the snapshot version exceeds the current maximum event sequence for the stream, the implementation should either reject the load with `RepositoryError::Model` or log a warning and proceed with the snapshot state. The first implementation should fail fast to detect data inconsistencies.🤖 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/postgres-event-store.md` around lines 161 - 163, Update the hydration docs to clarify validation when snapshot.version is larger than the current max event sequence: in the hydrate algorithm (the description that loads the newest snapshot and replays events where sequence > snapshot.version) add guidance that implementations must detect this inconsistency and either reject the load with RepositoryError::Model (fail-fast) or log a warning and continue using the snapshot state; recommend preferring the fail-fast RepositoryError::Model approach to surface data corruption.
🤖 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/postgres-event-store.md`:
- Around line 57-58: Update the table schema and constraint documentation to
mark payload_codec, payload_codec_version, event_name, and payload as NOT NULL:
locate the event table definition where columns payload_codec and
payload_codec_version are listed and add NOT NULL to those column specs, and
likewise mark event_name and payload as NOT NULL in their column entries; also
update the constraint list to reflect these NOT NULL requirements (in addition
to the existing CHECKs that prevent empty strings) so the documentation and
constraints consistently enforce non-null codec metadata and required fields.
- Around line 143-144: Update the snapshot table column definitions in
docs/postgres-event-store.md so that payload, payload_codec, and
payload_codec_version are declared NOT NULL (to match the validation contract),
and apply the same NOT NULL change to the other snapshot occurrences in the
file; optionally add CHECK constraints for payload_codec (e.g., allowed codec
labels) and for payload_codec_version (e.g., > 0) to make the contract explicit.
Ensure you update the snapshot table column list where `payload`,
`payload_codec`, and `payload_codec_version` are documented and any adjacent
examples/DDL so they all reflect NOT NULL and the optional CHECKs.
---
Nitpick comments:
In `@docs/postgres-event-store.md`:
- Line 59: Update the schema docs to explicitly show the DEFAULT for the
metadata column: where `metadata` is described as `jsonb` (the column symbol
`metadata`) include the `NOT NULL DEFAULT '{}'` clause in the DDL examples and
in the constraints section; likewise update the migration guidance that
currently states `metadata jsonb NOT NULL DEFAULT '{}'` so the constraints block
and any sample CREATE TABLE/ALTER statements consistently show `DEFAULT '{}'`
for `metadata`.
- Around line 161-163: Update the hydration docs to clarify validation when
snapshot.version is larger than the current max event sequence: in the hydrate
algorithm (the description that loads the newest snapshot and replays events
where sequence > snapshot.version) add guidance that implementations must detect
this inconsistency and either reject the load with RepositoryError::Model
(fail-fast) or log a warning and continue using the snapshot state; recommend
preferring the fail-fast RepositoryError::Model approach to surface data
corruption.
🪄 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: 6d3d6218-affb-4a8b-a5ce-37698e2521e0
📒 Files selected for processing (1)
docs/postgres-event-store.md
Collaborator
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
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
Note: all-features still reports the existing Reservation dead-code warning in tests/sagas/order/inventory.rs.
Summary by CodeRabbit