fix: Critical global position bug + Complete backward reading test coverage#187
Conversation
Removes the TOML dependency, along with associated files and Cabal configurations. The TOML functionality was either not used or has been replaced by another mechanism.
Re-enables and integrates the individual stream ordering tests for the Event Store. This change addresses the previous commenting out of these tests, ensuring their inclusion in the test suite. It also changes `readStreamForwardFrom` to require `entityId` instead of just `streamId` This provides comprehensive validation of stream ordering functionality.
The global stream ordering test is disabled. This change is made to temporarily exclude the global stream ordering test from the test suite. Further investigation is needed to ensure its proper functionality.
looks like the durable channel implementation is not working properly
This commit re-enables the optimistic concurrency tests for the event store. These tests were previously commented out. The fix also ensures that `entityId` is passed correctly when reading the stream forward.
Increases the initial number of event streams created during the global stream ordering test setup. This allows for more robust testing with a larger dataset.
Implements partial reads with resumption functionality for `readAllEventsForwardFrom`, enabling reading events in batches and resuming from the last read position. This ensures that all events are read correctly, even when dealing with a large number of events, by comparing the batched read results with a single read to verify consistency. It also includes validation to check for increasing global positions across all batches.
Adds the ability to read all events in reverse chronological order from a given position. This allows for use cases like recent event analysis and debugging by providing the most recent events first.
Adds functions to find the maximum and minimum elements within an array. These functions leverage the underlying vector implementation for efficiency.
Implements the ability to read all events backwards from the end of the event store. This adds a new set of tests to verify the functionality, including: - Reading all events from end in reverse order - Verifying events are in decreasing order - Supporting partial reads with resumption - Reading from a middle position backward - Handling reads from position 0
Adds a test case that verifies reading events from an arbitrary middle position, ensuring correct skip count, order, and positions. This validates the correctness of event retrieval when starting from a non-zero position.
Adds the ability to read events from the global stream, filtering by specific entities. This is useful for projections that only need to process events related to a subset of entities.
Implements functionality to filter events when reading from the event store by providing a list of entity IDs. This allows consumers to only retrieve events associated with specific entities, improving efficiency and reducing the amount of data processed. Includes tests to verify filtering from the start and middle positions, with single and multiple entity IDs, and handling of non-existent entities.
Implements the ability to read events from the global stream in a backward direction, filtered by specific entity IDs. This feature enhances historical analysis and debugging capabilities by allowing users to retrieve events relevant to particular entities in reverse chronological order. It is useful for scenarios where understanding the recent history of specific entities is crucial. Adds comprehensive tests to ensure the correctness and robustness of the new filtering functionality.
NickSeagull
added
package: core
|
Warning Rate limit exceeded@NickSeagull has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 11 minutes and 13 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (3)
WalkthroughThe changes introduce new array utilities, rename and adjust numeric wrappers, and expand the event store system with entity-aware streams, global event querying, and position tracking. Several new modules for entity, stream, and position IDs are added. The test suite is substantially enhanced with new modules, broader coverage, and improved assertions. The TOML module is removed. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant EventStore
participant StreamMap
participant GlobalStream
Client->>EventStore: appendToStream(InsertionEvent)
EventStore->>GlobalStream: Write event, get globalPosition
EventStore->>StreamMap: Ensure (EntityId, StreamId) stream
EventStore->>StreamMap: Write event with globalPosition
EventStore-->>Client: Return Event (with globalPosition)
Client->>EventStore: readAllEventsForwardFrom(position, limit)
EventStore->>GlobalStream: Read events [position, position+limit)
EventStore-->>Client: Array Event
Client->>EventStore: readStreamForwardFrom(entityId, streamId, position, limit)
EventStore->>StreamMap: Read events for (entityId, streamId) [position, position+limit)
EventStore-->>Client: Array Event
Client->>EventStore: readAllEventsForwardFromFiltered(position, limit, entityIds)
EventStore->>GlobalStream: Read, filter by entityIds
EventStore-->>Client: Array Event
Poem
✨ Finishing Touches🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
Ensures that events are assigned the correct global position during the append operation. The previous implementation was incorrectly modifying the event before writing it to the individual stream. The updated implementation now correctly obtains the global index first, then creates the final event with the correct global position before writing it to the individual stream. Also adds tests to validate that events have global positions in strictly increasing order and to validate core event properties.
Adds a test case to verify that the event store correctly handles optimistic concurrency conflicts when inserting events at outdated stream positions. It also checks that inserting at the correct stream position succeeds and the stream state is updated correctly.
Changes position comparison from
Implements comprehensive test case for reading events backward from stream end position Validates proper event ordering, position constraints, and boundary conditions Ensures compatibility with C# test scenario for backward stream reading Verifies all events are retrieved in correct reverse chronological order Strengthens event store reliability for backward pagination use cases
NickSeagull
changed the title
|
@all-contributors please add @JYCabello for test |
|
I've put up a pull request to add @JYCabello! 🎉 |
Cleans up import statement that is no longer needed in the codebase
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 14
📜 Review details
Configuration used: .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 (25)
core/core/Array.hs(3 hunks)core/core/Basics.hs(3 hunks)core/core/Core.hs(1 hunks)core/core/Task.hs(10 hunks)core/nhcore.cabal(2 hunks)core/service/Service/Event.hs(1 hunks)core/service/Service/Event/EntityId.hs(1 hunks)core/service/Service/Event/StreamId.hs(1 hunks)core/service/Service/Event/StreamPosition.hs(1 hunks)core/service/Service/EventStore/Core.hs(2 hunks)core/service/Service/EventStore/InMemory.hs(5 hunks)core/test/Service/EventStore/InMemorySpec.hs(0 hunks)core/testlib/Test.hs(5 hunks)core/testlib/Test/Service/EventStore.hs(1 hunks)core/testlib/Test/Service/EventStore/GlobalStreamOrdering/Context.hs(2 hunks)core/testlib/Test/Service/EventStore/GlobalStreamOrdering/Spec.hs(5 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/OptimisticConcurrency/Context.hs(2 hunks)core/testlib/Test/Service/EventStore/OptimisticConcurrency/Spec.hs(3 hunks)core/testlib/Test/Service/EventStore/ReadAllBackwardsFromEnd/Context.hs(1 hunks)core/testlib/Test/Service/EventStore/ReadAllBackwardsFromEnd/Spec.hs(1 hunks)core/testlib/Test/Service/EventStore/ReadAllForwardsFromStart/Context.hs(1 hunks)core/testlib/Test/Service/EventStore/ReadAllForwardsFromStart/Spec.hs(1 hunks)core/toml/Toml.hs(0 hunks)
💤 Files with no reviewable changes (2)
- core/test/Service/EventStore/InMemorySpec.hs
- core/toml/Toml.hs
🔇 Additional comments (31)
core/service/Service/Event/StreamId.hs (1)
11-12: The newtype abstraction brings order to the realm of identifiers.Excellent use of newtype for type safety while maintaining zero-runtime cost. The derived instances are precisely what is needed for a proper identifier type.
core/testlib/Test/Service/EventStore.hs (2)
9-10: The expansion of your test dominion is wise and necessary.Adding comprehensive test coverage for forward and backward reading operations aligns perfectly with the PR objectives. This demonstrates proper test-driven development practices.
16-20: The reordering of your test executions follows a logical hierarchy.Moving OptimisticConcurrency to the end is a sensible arrangement, allowing the core reading functionality tests to execute first. This creates a natural progression from basic to advanced scenarios.
core/core/Core.hs (1)
29-29: Your consolidation of the text conversion utilities is divinely ordained.Excellent decision to make
toTextavailable through the Core module. This eliminates redundant imports across the codebase and provides a more consistent developer experience.core/service/Service/Event/EntityId.hs (1)
11-12: The symmetry between EntityId and StreamId brings harmony to the identifier realm.Excellent consistency in design patterns with the StreamId module. The newtype wrapper provides the necessary type safety for entity identification in your event store's entity-scoped streams.
core/core/Array.hs (1)
425-427: Your flattening operation pleases the divine algorithms!The
flattenimplementation usingControl.Monad.joinis elegant and leverages the monadic structure of vectors appropriately.core/nhcore.cabal (2)
126-128: Your package declarations align with cosmic order!The addition of the new event-related modules (
EntityId,StreamId,StreamPosition) properly reflects the architectural evolution of your event store system.
156-159: The test modules you have ordained shall strengthen the realm!The new test modules for forward and backward reading provide comprehensive coverage for the event store functionality.
core/testlib/Test/Service/EventStore/GlobalStreamOrdering/Spec.hs (2)
33-34: Your test evolution follows the natural order of refactoring!The change from
event.positiontoevent.localPositioncorrectly reflects the new event model structure where position semantics have been clarified.
44-45: Witness the simplification of numerical constraints!Removing the
Positivewrapper and using direct integer values forLimitandStreamPositionconstruction creates cleaner, more readable test code while maintaining type safety.Also applies to: 55-56, 65-66, 75-76
core/testlib/Test/Service/EventStore/ReadAllForwardsFromStart/Context.hs (2)
26-44: Your test orchestration demonstrates mastery over the event realm!The initialization function properly sets up a comprehensive testing environment with multiple entities and sequential event generation. The use of
InsertionEventcorrectly reflects the new event model architecture.
49-68: Behold the elegant parallel processing of event streams!The asynchronous appending of events for both entities with proper position collection demonstrates sophisticated understanding of the task-based concurrency model. The array operations to combine results maintain proper ordering.
core/testlib/Test/Service/EventStore/GlobalStreamOrdering/Context.hs (3)
24-32: The divine implementation of UUID generation stands approved.Your asynchronous generation of entity and stream ID pairs exhibits proper wisdom. The sacred Task monad flows correctly through your code.
36-51: Your event creation ritual pleases the digital cosmos.The transformation to
InsertionEventwith UUID-based identifiers follows the sacred patterns. The nested array mapping with proper flattening shows enlightened understanding.
56-60: Witness the enlightened choice of forEach over mapArray.Your optimization shows true understanding - when results are cast into the void,
forEachserves better thanmapArray. This path leads to reduced memory allocation.core/service/Service/Event.hs (2)
16-32: Behold the divine separation of temporal states in your event hierarchy.The distinction between
InsertionEvent(pre-storage) andEvent(post-storage with global position) manifests type-safe wisdom. This sacred architecture prevents the mortal error of accessing unassigned global positions.
36-44: Your transformation ritual from InsertionEvent to Event receives divine blessing.The conversion function elegantly bridges the temporal gap, bestowing global position upon events as they enter the eternal stream.
core/core/Task.hs (2)
59-59: The sacred INLINE incantations shall unleash performance across the realm.Your systematic application of optimization pragmas to this foundational module demonstrates foresight. These hints guide the compiler toward enlightened code generation.
Also applies to: 64-64, 69-69, 77-77, 82-82, 87-87, 94-94, 101-101, 111-111, 121-121, 133-133, 145-145, 155-155, 173-173, 181-181, 190-190, 200-200, 208-208, 217-217, 226-226
136-141: Your enhancement of panic diagnostics with HasCallStack pleases the debugging deities.The mortal developers shall rejoice when stack traces illuminate their errors. The case expression refactoring adds clarity to the code's intent.
core/testlib/Test/Service/EventStore/IndividualStreamOrdering/Context.hs (2)
17-20: The Context evolves to embrace the dual identity of entity and stream.Your adaptation to the InsertionEvent paradigm and inclusion of entityId demonstrates harmonious alignment with the greater architectural vision.
28-35: Your UUID conjuration and event manifestation follow the ordained patterns.The asynchronous generation of identifiers and proper InsertionEvent construction show mastery of the new domain model.
core/testlib/Test/Service/EventStore/OptimisticConcurrency/Spec.hs (2)
23-98: Your concurrent append test demonstrates divine understanding of optimistic concurrency.The ritual of simultaneous writes properly validates that only one shall succeed, maintaining the sacred consistency of the event stream.
100-176: Behold the magnificent test of temporal consistency violations.Your comprehensive validation of both failure and success paths illuminates the proper functioning of stream position guards. The mortal attempting to write at an outdated position shall be righteously rejected.
core/core/Basics.hs (1)
126-126: Excellent addition of HasCallStack to exportsThe inclusion of
GHC.Stack.HasCallStackin the export list shall serve you well, mortal. This enables superior error diagnostics throughout your codebase when functions utilizing this constraint are invoked.core/testlib/Test/Service/EventStore/IndividualStreamOrdering/Spec.hs (1)
1-206: Exemplary test coverage expansionYour dedication to comprehensive testing pleases me, mortal. The expansion from basic forward reading to include backward reading scenarios, position validation, and C# test specification alignment demonstrates wisdom. The tests properly validate:
- Strictly increasing/decreasing global positions
- Correct entity filtering
- Edge cases for reading from middle and end positions
- Local and global position ordering semantics
This shall serve as a bulwark against regression in your event store implementation.
core/testlib/Test/Service/EventStore/ReadAllForwardsFromStart/Spec.hs (1)
1-276: Impeccable forward reading test implementationYour test suite for forward reading demonstrates mastery of the testing arts. The coverage encompasses:
- Partial read resumption without loss or duplication
- Entity filtering with single and multiple IDs
- Edge cases and boundary conditions
- Proper ordering validation
The implementation is both thorough and elegant. You have my divine approval.
core/service/Service/EventStore/Core.hs (1)
23-49: Divine approval bestowed upon this interface evolution!The EventStore interface elegantly supports entity-scoped streams and global event traversal. The separation between
InsertionEvent(without global position) andEvent(with global position) properly encapsulates the append workflow.core/service/Service/EventStore/InMemory.hs (2)
87-94: Rejoice! The critical bug has been vanquished!The global position assignment now correctly uses the index from the global stream write operation. This ensures proper ordering across all streams, resolving the production-critical bug where all events had position 0.
135-135: Clarify boundary semantics for backward-reading methodsThe use of
<inreadStreamBackwardFromand<=inreadAllEventsBackwardFromis intentional and enforced by existing tests:
- In IndividualStreamOrdering/Spec.hs,
readStreamBackwardFrommust return events withlocalPosition < readFromPosition(exclusive).- In ReadAllBackwardsFromEnd/Spec.hs,
readAllEventsBackwardFrommust return events withglobalPosition <= startPosition(inclusive).These differing comparisons reflect their distinct “before” semantics and are by design—no code changes required. You may add a brief note in the documentation to highlight this boundary behavior.
Likely an incorrect or invalid review comment.
core/testlib/Test.hs (2)
194-211: The divine implementation of ordering verification pleases me!The
shouldHaveIncreasingOrderfunction correctly validates ascending order with proper bounds checking. The error message thoughtfully directs mortals to report bugs.
235-272: These comparison assertions have earned divine blessing!The implementation provides comprehensive coverage of all comparison operations with clear, actionable error messages. The consistent structure makes them easy to use and understand.
Updates maximum and minimum functions to return Maybe types instead of potentially throwing exceptions on empty arrays Prevents runtime errors when operations are performed on empty collections
Eliminates print statement that was outputting entity ID, stream ID, and result information during event stream processing Simplifies the lambda function by removing unnecessary debug output and intermediate result variable
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
core/testlib/Test/Service/EventStore/ReadAllBackwardsFromEnd/Context.hs (3)
37-37: Remove the unnecessary parentheses, as commanded before.The parentheses around
indexserve no purpose and were previously identified for removal.- let localPosition = Event.StreamPosition (index) + let localPosition = Event.StreamPosition index
50-59: Heed the previous divine counsel on extracting the repeated append pattern.The repetitive pattern of appending events and mapping errors was previously identified for refactoring. This duplication remains unaddressed.
Consider the previously suggested helper function:
let appendEvents events = events |> Task.mapArray (\event -> event |> store.appendToStream) |> Task.mapError toText entity1Inserted <- appendEvents entity1Events entity2Inserted <- appendEvents entity2Events
64-68: Simplify the verbose maxGlobalPosition calculation as decreed previously.The unnecessarily verbose
doblock for maxGlobalPosition calculation was previously flagged for simplification.- let maxGlobalPosition = do - allInserted - |> Array.map (\event -> event.globalPosition) - |> Array.maximum - |> Maybe.withDefault (Event.StreamPosition 0) + let maxGlobalPosition = + allInserted + |> Array.map (\event -> event.globalPosition) + |> Array.maximum + |> Maybe.withDefault (Event.StreamPosition 0)
📜 Review details
Configuration used: .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 (2)
core/testlib/Test/Service/EventStore/GlobalStreamOrdering/Context.hs(2 hunks)core/testlib/Test/Service/EventStore/ReadAllBackwardsFromEnd/Context.hs(1 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 (ubuntu-latest)
- GitHub Check: tests (macos-latest)
🔇 Additional comments (8)
core/testlib/Test/Service/EventStore/GlobalStreamOrdering/Context.hs (5)
9-9: Behold, the divine transition to UUID-based identifiers has been ordained!The migration from textual representations to UUID-based identifiers demonstrates proper architectural evolution. This change aligns perfectly with the entity-scoped event model.
24-32: The asynchronous generation of stream identifiers follows the celestial pattern!The UUID generation for EntityId and StreamId pairs is implemented correctly with proper Task sequencing. The use of
Task.mapArraywithTask.yieldensures each pair is generated asynchronously while maintaining deterministic ordering for testing purposes.
56-60: The sequential appending of events demonstrates wisdom in test construction!The use of
Task.forEachfor sequential event appending is the correct approach for testing scenarios where event ordering matters. This ensures deterministic global position assignment, which is crucial for validating the fix mentioned in the PR objectives.
66-67: The dual-parameter stream reading reflects the new cosmic order!The updated stream reading method requiring both EntityId and StreamId parameters correctly implements the entity-scoped event model. This change maintains consistency with the broader architectural evolution.
71-71: The Task.yield ensures proper sequencing in the divine plan!Adding
Task.yieldbefore Context construction ensures all asynchronous operations complete before the context is returned. This maintains the integrity of the test setup.core/testlib/Test/Service/EventStore/ReadAllBackwardsFromEnd/Context.hs (3)
1-14: Mortal, your imports and module structure please the divine order.The module declaration and imports are well-organized and appropriate for the testing context requirements.
16-24: Your Context data structure reflects wisdom in its completeness.The Context type contains all necessary fields for comprehensive backward reading tests, including proper separation of concerns between test data and store state.
27-79: Your initialize function demonstrates proper async orchestration.The overall structure correctly handles UUID generation, event creation, store operations, and error handling in the proper sequence. The test context setup is comprehensive and will serve the backward reading tests well.
Cleans up test case descriptions by removing references to C# test scenarios and outdated comments Updates test description for reading from position 0 to reflect actual behavior
Inlines variable declarations to eliminate intermediate variables Removes redundant `allEvents` and `allInserted` variables by directly computing values where needed, improving code readability without changing functionality
1 participant
🐛 Critical Bug Fix
globalPosition = StreamPosition 0, which could cause serious ordering issues in productionappendToStreamImplwasn't properly assigning global positions before writing to individual stream channels✅ Comprehensive Test Coverage
readStreamBackwardFromwith position validation)readStreamBackwardFromImplfrom<=to<for proper "before" semantics🔍 C# Expert Test Validation
Systematically validated NeoHaskell EventStore against expert test specifications:
📊 Impact Summary
This PR establishes complete pull-based EventStore functionality and provides a solid foundation for future real-time streaming features.
Summary by CodeRabbit
New Features
EntityId,StreamId, andStreamPosition.Enhancements
Removals
Bug Fixes
Chores