feat: Add Entity Fetcher Service for Event Sourcing#226
Conversation
|
Claude finished @NickSeagull's task —— View job Code Review for Entity Reduction PRI've completed my review of this entity reduction implementation. Here's my comprehensive feedback: Todo List:
Code Quality and Best Practices✅ Excellent Areas:
|
|
Claude finished @NickSeagull's task —— View job Todo List:
Code Review for Entity Reduction PRI've completed my review of this entity reduction implementation. Here's my comprehensive feedback: Code Quality and Best Practices✅ Excellent Areas:
|
|
Claude encountered an error —— View job Todo List:
I'll analyze this entity reduction implementation and provide a thorough code review. |
|
Claude Code is working… I'll analyze this and get back to you. |
|
Claude finished @NickSeagull's task —— View job Code Review for Entity Reduction PRTodo List:
Code Quality and Best Practices✅ Excellent Areas:
|
|
Claude finished @NickSeagull's task —— View job Code Review for Entity Reduction PRTodo List:
Code Quality and Best Practices✅ Excellent Areas:
|
|
Claude finished @NickSeagull's task —— View job Code Review for Entity Reduction PRTodo List:
Code Quality and Best Practices✅ Excellent Areas:
Issues Found
|
|
Claude finished @NickSeagull's task —— View job Code Review for Entity Reduction PRTodo List:
Code Quality and Best Practices✅ Excellent Areas:
Issues Found
|
WalkthroughThis PR introduces the EntityFetcher service for fetching entity state from event stores via event reduction. Test event type is standardized from MyEvent to BankAccountEvent across test suites. A batching fix corrects duplicate event handling in Postgres event stream reads. Changes
Sequence DiagramsequenceDiagram
participant Test
participant EF as EntityFetcher
participant ES as EventStore
participant Stream as Event Stream
Test->>EF: new(eventStore, initialState, reducer)
EF->>ES: initialize with store
Note over EF: EntityFetcher ready
Test->>EF: fetch(entityName, streamId)
EF->>ES: readAllStreamEvents(streamId)
ES->>Stream: query events
Stream-->>ES: Event[...]
ES-->>EF: StreamEvent[]
loop for each event
EF->>EF: reducer(event, state)
Note over EF: accumulate state
end
EF-->>Test: final state
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 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 |
|
Claude finished @NickSeagull's task —— View job Code Review for Entity Reduction PRTodo List:
Code Quality and Best Practices✅ Excellent Areas:
Issues Found
|
NickSeagull
changed the title
|
Claude finished @NickSeagull's task —— View job Code Review for Entity Reduction PRTodo List:
Code Quality and Best Practices✅ Excellent Areas:
Issues Found
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (31)
core/nhcore.cabal(2 hunks)core/service/Service/EntityFetcher.hs(1 hunks)core/service/Service/EntityFetcher/Core.hs(1 hunks)core/service/Service/EventStore/Postgres/Internal.hs(2 hunks)core/test/Service/EventStore/InMemorySpec.hs(1 hunks)core/test/Service/EventStore/Postgres/Internal/SubscriptionStoreSpec.hs(2 hunks)core/test/Service/EventStore/PostgresSpec.hs(2 hunks)core/testlib/Test/Service/EntityFetcher.hs(1 hunks)core/testlib/Test/Service/EntityFetcher/Core.hs(1 hunks)core/testlib/Test/Service/EntityFetcher/Fetch/Context.hs(1 hunks)core/testlib/Test/Service/EntityFetcher/Fetch/Spec.hs(1 hunks)core/testlib/Test/Service/EventStore.hs(2 hunks)core/testlib/Test/Service/EventStore/BatchValidation/Context.hs(1 hunks)core/testlib/Test/Service/EventStore/BatchValidation/Spec.hs(1 hunks)core/testlib/Test/Service/EventStore/Core.hs(2 hunks)core/testlib/Test/Service/EventStore/GlobalStreamOrdering/Context.hs(2 hunks)core/testlib/Test/Service/EventStore/GlobalStreamOrdering/Spec.hs(2 hunks)core/testlib/Test/Service/EventStore/IndividualStreamOrdering/Context.hs(1 hunks)core/testlib/Test/Service/EventStore/IndividualStreamOrdering/Spec.hs(2 hunks)core/testlib/Test/Service/EventStore/LocalPositionStamping/Spec.hs(7 hunks)core/testlib/Test/Service/EventStore/OptimisticConcurrency/Context.hs(1 hunks)core/testlib/Test/Service/EventStore/OptimisticConcurrency/Spec.hs(11 hunks)core/testlib/Test/Service/EventStore/ReadAllBackwardsFromEnd/Context.hs(2 hunks)core/testlib/Test/Service/EventStore/ReadAllBackwardsFromEnd/Spec.hs(3 hunks)core/testlib/Test/Service/EventStore/ReadAllForwardsFromStart/Context.hs(2 hunks)core/testlib/Test/Service/EventStore/ReadAllForwardsFromStart/Spec.hs(3 hunks)core/testlib/Test/Service/EventStore/StreamTruncation/Context.hs(1 hunks)core/testlib/Test/Service/EventStore/StreamTruncation/Spec.hs(1 hunks)core/testlib/Test/Service/EventStore/Subscriptions/Context.hs(2 hunks)core/testlib/Test/Service/EventStore/Subscriptions/SimpleSpec.hs(2 hunks)core/testlib/Test/Service/EventStore/Subscriptions/Spec.hs(12 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: tests-linux
- GitHub Check: tests-macos
🔇 Additional comments (44)
core/test/Service/EventStore/Postgres/Internal/SubscriptionStoreSpec.hs (2)
16-16: BankAccountEvent import aligns this spec with the new test domainSummoning your attention: this import correctly centralizes the subscription tests on
BankAccountEvent, matching the rest of the EventStore test helpers and avoiding any leftoverMyEventusage.
315-321: createTestEvent now consistently uses BankAccountEventThis helper’s signature and payload now agree (
Event BankAccountEventwithMoneyDeposited {amount = 100}), keeping dispatch tests aligned with the new domain type without introducing extra complexity.core/nhcore.cabal (2)
130-131: EntityFetcher modules are correctly surfaced in the public APIYou have rightly exposed
Service.EntityFetcherandService.EntityFetcher.Core, making the new fetcher functionality available to downstream users in line with the rest of the service modules.
155-158: EntityFetcher test scaffolding integrated consistently with existing testlib exposureThe new
Test.Service.EntityFetcher*modules follow the established pattern of exposing test helpers viatestlib, which will keep higher-level specs and examples coherent around the BankAccount/EntityFetcher domain.core/testlib/Test/Service/EventStore/BatchValidation/Context.hs (1)
11-24: Context now correctly targets EventStore BankAccountEventThis Context/initialize pair cleanly migrates to
EventStore BankAccountEvent, keeping batch-validation tests aligned with the shared BankAccount domain while preserving the simple, composable API.core/testlib/Test/Service/EventStore/Core.hs (2)
1-27: BankAccountEvent test domain is well-shaped and cohesiveThe new
BankAccountEventADT, along with its JSON andDefaultinstances, forms a clear, reusable test domain that matches how the rest of the suite now reasons about bank-account behavior. This is an appropriate consolidation over the previousMyEvent.
30-49: newInsertion correctly stamps local positions for BankAccountEvent insertions
newInsertionnow yieldsEvent.Insertion BankAccountEventwith a fixedMoneyDeposited {amount = 10}payload and deriveslocalPositionfrom the provided index. This is simple, deterministic, and perfectly suited for ordering and pagination tests.core/testlib/Test/Service/EventStore/LocalPositionStamping/Spec.hs (7)
18-22: Spec now cleanly targets EventStore BankAccountEventThe import and
spectype signature have been correctly migrated toEventStore BankAccountEvent, bringing this suite in line with the unified BankAccount test domain.
31-33: Primary stamping test uses meaningful BankAccountEvent scenariosSwitching to
AccountOpened,MoneyDeposited, andMoneyWithdrawngives this test a concrete, domain-relevant sequence while still exercising local-position stamping over three events.
88-93: Stronger assertion that all subscription events carry localPositionThe universal check over
capturedensures no subscriber ever sees an event without a stampedlocalPosition, turning a previously implicit guarantee into an explicit, enforced contract.
95-111: Subscription test now validates presence of both positions 0 and 1The
hasPositionhelper and subsequent checks guarantee that exactly one event with position 0 and exactly one with position 1 were delivered, catching both missing and duplicated local-position scenarios.
117-124: Batch insertion test correctly uses BankAccountEvent variants
firstBatchandsecondBatchnow exercise a realistic mixture of BankAccount events while still asserting local positions 0–4, preserving the essence of the local-position derivation test.
158-179: Explicit localPosition preservation test updated to BankAccountEventThe two manual insertions using
AccountOpenedandMoneyDepositedwith explicit positions 0 and 1 keep this test focused on “do not override caller-provided localPosition” while embracing the new domain type.
219-229: Auto-assignment test now uses BankAccountEvent while preserving intentThe two
MoneyDepositedevents without localPosition rely on the store to assign sequential positions 0 and 1; the test logic remains sound and now matches the unified BankAccount event family.core/testlib/Test/Service/EventStore/ReadAllBackwardsFromEnd/Spec.hs (1)
14-14: Behold, the type refactoring is sound.The systematic migration from MyEvent to BankAccountEvent across all type signatures and imports is executed with precision. The test logic remains unchanged while now operating upon the standardized event type.
Also applies to: 20-20, 35-35, 83-83
core/testlib/Test/Service/EventStore/StreamTruncation/Context.hs (1)
10-10: The type alignment is ordained.The Context module now bears the BankAccountEvent type throughout its definition, from import to data field to function signature. This maintains perfect consistency with the broader refactoring.
Also applies to: 14-14, 19-19
core/test/Service/EventStore/InMemorySpec.hs (2)
7-9: The type application is decreed.The EntityFetcher modules are properly imported, and the InMemory store is correctly specialized to BankAccountEvent through type application. This establishes the foundation for the combined test flow.
Also applies to: 15-16
18-23: The combined test flow is blessed.The integration of EntityFetcher tests is accomplished with elegance. The store is paired with a fetcher constructed via EntityFetcherCore.newFetcher, and the combined entity is submitted to EntityFetcher.spec for validation.
core/testlib/Test/Service/EventStore/StreamTruncation/Spec.hs (1)
14-14: The truncation tests shall operate upon the ordained type.The StreamTruncation specification is correctly updated to utilize BankAccountEvent, maintaining consistency with the broader refactoring decree.
Also applies to: 19-19
core/testlib/Test/Service/EventStore.hs (1)
7-7: The master test suite embraces the standardized type.The primary EventStore test module is correctly updated to BankAccountEvent, ensuring all subordinate test specifications operate upon the unified event type.
Also applies to: 19-19
core/testlib/Test/Service/EventStore/ReadAllForwardsFromStart/Spec.hs (1)
14-14: The forward-reading tests are harmonized.The systematic type migration to BankAccountEvent is executed flawlessly across all signatures and helper annotations. The test logic endures unchanged while operating upon the standardized event type.
Also applies to: 20-20, 35-35, 83-83
core/test/Service/EventStore/PostgresSpec.hs (2)
10-13: The Postgres implementation is properly specialized.The EntityFetcher modules are imported, and all Postgres operations are correctly specialized to BankAccountEvent through type applications. This establishes type-safe event handling throughout the Postgres test suite.
Also applies to: 31-31, 39-39, 47-47, 54-54
59-64: The Postgres EntityFetcher integration mirrors its InMemory counterpart.The combined test flow for Postgres follows the same blessed pattern as InMemory: store is paired with fetcher, and the unified entity undergoes EntityFetcher validation. This symmetry across implementations is most pleasing.
core/testlib/Test/Service/EventStore/BatchValidation/Spec.hs (1)
13-13: The validation tests complete the transformation.The BatchValidation specification embraces BankAccountEvent, bringing the final module into alignment with the standardized event type. The refactoring across all test modules is now complete and consistent.
Also applies to: 17-17
core/testlib/Test/Service/EventStore/OptimisticConcurrency/Context.hs (1)
10-10: Behold, the type migration is sound.The transition from MyEvent to BankAccountEvent is executed with precision. The import, field type, and function signature align harmoniously, maintaining type consistency across the test infrastructure.
Also applies to: 14-14, 19-23
core/testlib/Test/Service/EventStore/GlobalStreamOrdering/Spec.hs (1)
14-14: The ordering tests now speak the language of domains.The type migration to BankAccountEvent is complete and correct. The explicit type annotation for
matchPositionsat line 94 demonstrates commendable type clarity in polymorphic contexts.Also applies to: 19-20, 94-96
core/service/Service/EntityFetcher.hs (1)
1-5: A worthy facade for thy new service.This module follows the blessed pattern of separating implementation (Core) from public interface. The re-export strategy provides a clean entry point for consuming code whilst maintaining internal flexibility.
core/testlib/Test/Service/EntityFetcher.hs (1)
1-15: The test scaffolding is architecturally sound.This module serves as a proper test orchestrator, composing EventStore and EntityFetcher dependencies through Task-based initialization. The delegation to
Fetch.specmaintains clean separation of concerns, and the type signatures correctly capture the test requirements with BankAccountEvent and BankAccountState.core/testlib/Test/Service/EventStore/OptimisticConcurrency/Spec.hs (1)
17-17: The concurrency tests now wield domain-specific events with authority.The migration from MyEvent to BankAccountEvent is executed comprehensively across all test scenarios. The use of semantically meaningful event types—AccountOpened, MoneyDeposited, MoneyWithdrawn—with appropriate domain fields (initialBalance, amount) enhances test readability whilst preserving the critical concurrency invariants being verified.
Also applies to: 22-22, 41-41, 68-68, 89-89, 160-160, 195-195, 228-228, 284-284, 338-338, 366-366, 387-387
core/testlib/Test/Service/EventStore/IndividualStreamOrdering/Spec.hs (1)
14-14: The stream ordering tests align with the new type hierarchy.The type signatures are updated correctly throughout, maintaining consistency with the BankAccountEvent migration. The test logic remains pristine, unchanged by this mechanical refactor.
Also applies to: 18-19, 33-34
core/testlib/Test/Service/EventStore/Subscriptions/SimpleSpec.hs (1)
13-13: The subscription tests have embraced the domain event paradigm.The explicit type annotation for
receivedEventsat line 38 demonstrates prudent type specification in the presence of polymorphism, ensuring the ConcurrentVar is correctly typed from initialization. The migration to BankAccountEvent is complete and correct.Also applies to: 17-18, 38-38
core/testlib/Test/Service/EventStore/ReadAllForwardsFromStart/Context.hs (1)
13-13: The context structure now properly reflects the domain event hierarchy.The Context data type and its initialization function are updated with perfect consistency. Both the
storeandgeneratedEventsfields now correctly reference BankAccountEvent, ensuring type safety throughout the test infrastructure.Also applies to: 22-25, 28-29
core/testlib/Test/Service/EventStore/ReadAllBackwardsFromEnd/Context.hs (1)
14-29: BankAccountEvent migration is consistent and type-safeThe switch to
BankAccountEventforstore,generatedEvents, andinitialize’sEventStoreparameter is coherent with the new import and preserves the existing behavior of the context.core/testlib/Test/Service/EventStore/IndividualStreamOrdering/Context.hs (1)
13-27: Type migration to BankAccountEvent is coherent with insertion logicThe update of
store,allInsertions, andinitializeto useEventStore BankAccountEventandEvent.Insertion BankAccountEventaligns correctly withnewInsertionand the rest of the context; no hidden behavioral changes are introduced.core/testlib/Test/Service/EntityFetcher/Fetch/Spec.hs (1)
21-299: Extensive EntityFetcher coverage is well structuredThe spec exercises a wide range of behaviors (empty streams, sequential events, multiple streams, concurrency, large event counts, edge cases) and wires the store/fetcher/context cleanly. This is a solid foundation for validating the new
EntityFetcherservice.core/testlib/Test/Service/EventStore/Subscriptions/Context.hs (1)
13-38: Subscriptions context correctly aligned to BankAccountEventThe migration of
store,testEvents,initialize, andcreateTestEventstoBankAccountEventis internally consistent and matches the updated import and insertion helper. The subscription tests will now run against the same bank-account domain as the rest of the suite.core/testlib/Test/Service/EventStore/GlobalStreamOrdering/Context.hs (1)
12-12: The type parameter refactoring is executed with precision.The transformation from MyEvent to BankAccountEvent has been applied consistently across imports, field types, and function signatures. This standardization strengthens the test infrastructure's semantic clarity.
Also applies to: 19-20, 24-24, 40-40
core/service/Service/EntityFetcher/Core.hs (2)
27-32: The EntityFetcher type design embodies elegant simplicity.The single-field record with parametric polymorphism over state and event types provides the necessary flexibility while maintaining clarity. The type signature of fetch is precise and well-documented.
46-82: The error handling on line 75 stands verified as correct.Stream.consume's type signature confirms it returns
Task Text accumulator, and the mapError correctly converts Text errors to EventStoreError by wrapping them as EventStore.StorageFailure. The lambda properly lifts the pure reducer function into the Task monad context, satisfying Stream.consume's type requirements. The implementation demonstrates sound streaming architecture with proper error propagation.core/testlib/Test/Service/EventStore/Subscriptions/Spec.hs (1)
18-18: The type system migration has been executed with unwavering consistency.Every occurrence of MyEvent has been systematically transformed to BankAccountEvent across function signatures, type annotations, and helper functions. The test logic remains intact while the type semantics are elevated to the domain-specific BankAccountEvent.
Also applies to: 27-29, 36-36, 42-42, 103-103, 165-165, 261-263, 323-323, 371-371, 437-437, 502-502, 577-577, 623-623, 640-640
core/testlib/Test/Service/EntityFetcher/Core.hs (3)
18-39: The BankAccountState model exhibits sound domain design.The state captures essential account properties: balance, open status, and version for event ordering. The initialState correctly represents an unopened account with zero balance, establishing a valid starting point for event reduction.
43-67: The event application logic faithfully implements state transitions.Each BankAccountEvent is correctly mapped to its corresponding state mutation:
- AccountOpened establishes initial balance and open status
- MoneyDeposited/MoneyWithdrawn adjust balance arithmetically
- AccountClosed terminates account operations while preserving balance
- Version increment ensures event ordering integrity
The absence of validation (overdraft checks, state preconditions) is appropriate for a test fixture where events are assumed valid.
71-73: The newFetcher convenience function elegantly encapsulates domain wiring.This helper correctly binds the EventStore to the bank account domain by providing initialState and applyEvent to EntityFetcher.new, simplifying test setup while maintaining type safety.
core/service/Service/EventStore/Postgres/Internal.hs (1)
765-793: The review comment rests upon an incorrect assumption about how therelativeparameter affects query semantics.The core claim—that using
FromAndAfterfor backward reads might alter comparison semantics from<to>=—is unfounded. The SQL comparison operator is determined solely byreadDirectionviatoPostgresLocalPositionComparison:-- Core.hs lines 68-72 toPostgresLocalPositionComparison :: Maybe ReadDirection -> Text toPostgresLocalPositionComparison dir = case dir of Just Backwards -> "<" _ -> ">="The
relativeparameter type (whetherFromAndAfterorBefore) is only checked forStart/Endspecial cases in the SQL filter construction (Sessions.hs lines 346-349). Otherwise, both constructors fall through to use the samepositionComparisonderived fromreadDirection. SincerelativePositionToInt64treats both constructors identically, the actual position value is extracted the same way.Therefore, using
FromAndAfterunconditionally for subsequent batches does not risk breaking backward-read semantics. The backward read will continue to use<comparison as documented in the position-update logic (lines 825–828, 843–846), regardless of whetherrelativeisFromAndAfterorBefore.Likely an incorrect or invalid review comment.
| data Error | ||
| = EventStoreError EventStore.Error | ||
| | ReductionError Text | ||
| deriving (Eq, Show) |
There was a problem hiding this comment.
The Error type's semantic boundaries require clarification.
The ReductionError constructor wraps Text, yet at line 75 it captures storage failures during stream consumption. This creates semantic ambiguity—is it a reduction error or a storage error? Consider either:
- Eliminating ReductionError if the reducer function cannot fail (pure state transitions)
- Renaming to StreamConsumptionError if it encompasses both reduction and streaming failures
- Using EventStoreError consistently for all EventStore-originated errors
🤖 Prompt for AI Agents
In core/service/Service/EntityFetcher/Core.hs around lines 16–19 the
ReductionError Text constructor is semantically ambiguous because it is used to
capture stream consumption/storage failures; rename ReductionError to
StreamConsumptionError (or alternatively collapse it into EventStoreError) and
update all usages (including the capture at line 75 and any pattern matches,
exports, and tests) to the new constructor so the type clearly denotes
stream/consumption failures rather than reducer logic errors; ensure Eq/Show
still derive and adjust any error construction sites to pass the same Text
payload under the new name.
| data Context = Context | ||
| { store :: EventStore BankAccountEvent, | ||
| fetcher :: EntityFetcher BankAccountState BankAccountEvent, | ||
| streamId :: Event.StreamId, | ||
| entityName :: Event.EntityName | ||
| } | ||
|
|
||
|
|
||
| initialize :: | ||
| Task Text (EventStore BankAccountEvent, EntityFetcher BankAccountState BankAccountEvent) -> | ||
| Task Text Context | ||
| initialize newStoreAndFetcher = do | ||
| (store, fetcher) <- newStoreAndFetcher | ||
| streamId <- StreamId.new | ||
| let entityName = Event.EntityName "BankAccount" | ||
| pure Context {store, fetcher, streamId, entityName} |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Avoid hard‑coding the BankAccount entity name in multiple places
Hard‑coding "BankAccount" here is acceptable for now, but it risks drifting from other helpers if the name ever changes. Consider exposing a shared bankAccountEntityName constant from your test core module and using it here to keep entity naming consistent across tests.
🤖 Prompt for AI Agents
In core/testlib/Test/Service/EntityFetcher/Fetch/Context.hs around lines 14 to
29, the entity name "BankAccount" is hard-coded which can drift from other
helpers; import and use a shared constant (e.g. bankAccountEntityName) from the
test core module instead of the literal string, replace let entityName =
Event.EntityName "BankAccount" with using that shared constant, and add the
required import to the top of this module so all tests reference the same source
of truth for the BankAccount entity name.
| it "returns error when fetching from non-existent entity type" \context -> do | ||
| let wrongEntityName = Event.EntityName "NonExistentEntity" | ||
|
|
||
| result <- | ||
| context.fetcher.fetch wrongEntityName context.streamId | ||
| |> Task.asResult | ||
|
|
||
| -- This should succeed with initial state (empty stream) | ||
| -- OR return a specific error depending on implementation | ||
| -- For now, let's expect initial state for consistency | ||
| result | ||
| |> Result.isOk | ||
| |> shouldBe True | ||
|
|
There was a problem hiding this comment.
Test description contradicts its actual expectation for non‑existent entity type
The test is named "returns error when fetching from non-existent entity type", but it asserts Result.isOk == True and treats success as the expected outcome. This contradiction will mislead future readers.
Align the description with the behavior (or vice versa). For example:
- it "returns error when fetching from non-existent entity type" \context -> do
+ it "returns initial state for non-existent entity type" \context -> do
@@
- -- This should succeed with initial state (empty stream)
- -- OR return a specific error depending on implementation
- -- For now, let's expect initial state for consistency
+ -- This should succeed with initial state (empty stream),
+ -- matching the current EntityFetcher behavior for unknown entity types.Alternatively, if you intend an actual error, update the assertions to check for an Err with the appropriate error variant.
🤖 Prompt for AI Agents
In core/testlib/Test/Service/EntityFetcher/Fetch/Spec.hs around lines 299-312
the test name says it "returns error when fetching from non-existent entity
type" but the assertions expect a successful Result.isOk == True; make them
consistent by either renaming the test to reflect the current behavior (e.g.,
"returns initial state when fetching from non-existent entity type" and update
the inline comment accordingly) or change the assertions to expect an error
(pattern-match the Result.Err and assert the specific error variant), and ensure
the test comment matches the chosen behavior.
| describe "Edge Cases" do | ||
| before (Context.initialize newStoreAndFetcher) do | ||
| it "handles fetching with empty stream ID gracefully" \context -> do | ||
| -- This tests that the fetcher doesn't crash with unusual input | ||
| -- Note: StreamId is generated via UUID, so we use a valid one but unused stream | ||
| unusedStreamId <- StreamId.new | ||
| result <- | ||
| context.fetcher.fetch context.entityName unusedStreamId | ||
| |> Task.asResult | ||
|
|
||
| case result of | ||
| Ok state -> do | ||
| -- Should return initial state for non-existent stream | ||
| state.balance |> shouldBe 0 | ||
| state.version |> shouldBe 0 | ||
| Err _ -> fail "Expected Ok but got Err" | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Clarify test name: this uses an unused stream ID, not an “empty” one
The test "handles fetching with empty stream ID gracefully" generates a fresh StreamId via StreamId.new and never uses it for writes, so it is actually validating behavior for an unused/non-existent stream rather than an “empty stream ID”.
Consider renaming the test to better reflect what’s happening, e.g.:
- it "handles fetching with empty stream ID gracefully" \context -> do
+ it "handles fetching from an unused stream ID gracefully" \context -> doThis will keep the intent obvious to anyone reading the suite.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| describe "Edge Cases" do | |
| before (Context.initialize newStoreAndFetcher) do | |
| it "handles fetching with empty stream ID gracefully" \context -> do | |
| -- This tests that the fetcher doesn't crash with unusual input | |
| -- Note: StreamId is generated via UUID, so we use a valid one but unused stream | |
| unusedStreamId <- StreamId.new | |
| result <- | |
| context.fetcher.fetch context.entityName unusedStreamId | |
| |> Task.asResult | |
| case result of | |
| Ok state -> do | |
| -- Should return initial state for non-existent stream | |
| state.balance |> shouldBe 0 | |
| state.version |> shouldBe 0 | |
| Err _ -> fail "Expected Ok but got Err" | |
| describe "Edge Cases" do | |
| before (Context.initialize newStoreAndFetcher) do | |
| it "handles fetching from an unused stream ID gracefully" \context -> do | |
| -- This tests that the fetcher doesn't crash with unusual input | |
| -- Note: StreamId is generated via UUID, so we use a valid one but unused stream | |
| unusedStreamId <- StreamId.new | |
| result <- | |
| context.fetcher.fetch context.entityName unusedStreamId | |
| |> Task.asResult | |
| case result of | |
| Ok state -> do | |
| -- Should return initial state for non-existent stream | |
| state.balance |> shouldBe 0 | |
| state.version |> shouldBe 0 | |
| Err _ -> fail "Expected Ok but got Err" |
🤖 Prompt for AI Agents
In core/testlib/Test/Service/EntityFetcher/Fetch/Spec.hs around lines 572 to
588, the test title "handles fetching with empty stream ID gracefully" is
misleading because the test generates a new StreamId and checks behavior for an
unused/non-existent stream; rename the test to something like "handles fetching
for unused/non-existent stream ID gracefully" and update the inline comment to
describe that a fresh StreamId is generated and not written to, ensuring the
test name and comments accurately reflect the intent.
| let events = Array.fromLinkedList [AccountOpened {initialBalance = 2000}, MoneyDeposited {amount = 15}] | ||
| payload <- Event.payloadFromEvents entityName streamId events | ||
| _result <- store.insert payload |> Task.mapError toText | ||
|
|
||
| -- Wait a bit for async notification | ||
| AsyncTask.sleep 100 |> Task.mapError (\_ -> "timeout") | ||
| -- Wait longer for async notifications to complete (increased from 100ms to 200ms to reduce flakiness) | ||
| AsyncTask.sleep 200 | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Subscription test events updated; timeout increased to reduce flakiness
Using AccountOpened and MoneyDeposited keeps the subscription path aligned with the BankAccount domain. The 200ms sleep is a pragmatic improvement for async delivery, though a future refactor to wait on captured length rather than a fixed delay would yield even more deterministic tests.
🤖 Prompt for AI Agents
In core/testlib/Test/Service/EventStore/LocalPositionStamping/Spec.hs around
lines 77 to 83, replace the fixed AsyncTask.sleep 200 used to wait for async
notifications with a deterministic wait that polls the subscription capture
until it has the expected number of events (or until a short timeout) instead of
sleeping; implement a small retry loop or helper that checks the captured
notification list length equals 2 (or the expected length) with a max timeout
(e.g. 500ms) and fails the test if the condition isn’t met, then remove the
hardcoded sleep.
1 participant
This PR introduces a new
EntityFetcherservice that makes it easy to reconstruct the current state of an entity by reading and applying all its events from the event store. Think of it as a "time machine" that replays your entity's history to show you what it looks like right now.✨ Why This Matters
If you're building applications with event sourcing (where you store changes as a series of events instead of just the final state), you need a way to reconstruct what an entity looks like at any point in time. This PR gives you that capability out of the box.
Real-world example: Imagine you have a bank account that stores events like "deposit $100" and "withdraw $50". The EntityFetcher reads all these events and tells you the current balance is $50.
🚀 Quick Start
Here's how you use it:
📦 What's Included
New Modules
Service.EntityFetcher.Core- The main EntityFetcher implementationService.EntityFetcher- Re-exports everything you needKey Features
Test Coverage
Added complete test suites covering:
🔧 Technical Details
How It Works
Memory Efficiency
The implementation uses
Stream.consumeto process events one-at-a-time, so even if you have thousands of events, it won't load them all into memory at once.Bug Fix Included
This PR also fixes a bug in the Postgres EventStore where reading stream events with
FromAndAftercould return duplicate events. The fix ensures subsequent batches use position-based pagination correctly.🧪 Testing
All existing tests pass, plus we added:
Run tests with:
nix-shell --run "cabal test core"📚 Example Domain
The tests include a complete example domain (bank accounts) showing:
AccountOpened,MoneyDeposited, etc.)BankAccountState)This serves as documentation by example!
🤔 Design Decisions
Why streaming? We could have loaded all events into an array first, but that would use too much memory for entities with long histories. Streaming keeps memory usage constant.
Why a separate service? This keeps the EventStore focused on storage/retrieval, while EntityFetcher handles the business logic of state reconstruction. Follows single responsibility principle.
Why pass the reduce function? This keeps the fetcher generic - it doesn't need to know about your specific domain events. You bring your own logic.
🎓 Learning Resources
If you're new to event sourcing, here's what you need to know:
The EntityFetcher handles the reduction part for you!
✅ Checklist
💬 Questions?
If anything is unclear or you get stuck trying to use this:
Time to review: ~10-15 minutes to understand the concepts, ~5 minutes to review the code structure
Summary by CodeRabbit
Bug Fixes
Tests