Implement SaveSnapshot and GetLatestSnapshot methods on InMemoryEventStore to satisfy the SnapshotStore interface, along with comprehensive unit tests.
Changes
Extended InMemoryEventStore with snapshots map storage
Implemented SaveSnapshot method for persisting snapshots
Implemented GetLatestSnapshot method that returns snapshot with highest version
Added 16 unit tests covering all acceptance criteria
Test Coverage
SaveSnapshot persists snapshots correctly
GetLatestSnapshot returns most recent snapshot by version
Behavior when no snapshot exists (returns nil, nil)
Snapshot versioning respected across actors
Version ordering (higher version wins, not insertion order)
Data integrity for complex, nested, and special character states
Edge cases: zero version, large version, empty/nil state
## Summary
Implement SaveSnapshot and GetLatestSnapshot methods on InMemoryEventStore to satisfy the SnapshotStore interface, along with comprehensive unit tests.
## Changes
- Extended InMemoryEventStore with snapshots map storage
- Implemented SaveSnapshot method for persisting snapshots
- Implemented GetLatestSnapshot method that returns snapshot with highest version
- Added 16 unit tests covering all acceptance criteria
## Test Coverage
- SaveSnapshot persists snapshots correctly
- GetLatestSnapshot returns most recent snapshot by version
- Behavior when no snapshot exists (returns nil, nil)
- Snapshot versioning respected across actors
- Version ordering (higher version wins, not insertion order)
- Data integrity for complex, nested, and special character states
- Edge cases: zero version, large version, empty/nil state
- Interface compliance verification
Closes #4
Implement SaveSnapshot and GetLatestSnapshot methods on InMemoryEventStore
to satisfy the SnapshotStore interface, and add comprehensive tests covering:
- SaveSnapshot persists snapshots correctly
- GetLatestSnapshot returns most recent snapshot by version
- Behavior when no snapshot exists (returns nil, nil)
- Snapshot versioning is respected across actors
- Version ordering (higher version wins, not insertion order)
- Data integrity for complex, nested, and special character states
- Edge cases: zero version, large version, empty/nil state
Closes#4
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
The snapshot methods lack mutex protection. Since PR #32 is adding mutex protection for event methods, please add the same protection to SaveSnapshot and GetLatestSnapshot for consistency.
2. No nil validation in SaveSnapshot - store/memory.go:63
Passing nil snapshot will cause a panic. Please add validation:
func(es*InMemoryEventStore)SaveSnapshot(snapshot*aether.ActorSnapshot)error{ifsnapshot==nil{returnfmt.Errorf("snapshot cannot be nil")}// ...}
3. Test key collision - store/memory_test.go:395
The key generation in TestSnapshotDataIntegrity_LargeState has collisions due to modulo arithmetic (i=0 and i=260 both produce "a0"). Consider using fmt.Sprintf("key-%d", i) instead.
Suggestions (optional)
Add a concurrent access test for snapshots (similar to PR #32's event concurrency test)
Once the thread safety and nil validation are addressed, this will be ready to merge.
## Code Review
### Summary
Good implementation of SnapshotStore interface with comprehensive tests. A few issues to address before merging.
### Issues
**1. Missing thread safety (Bug)** - `store/memory.go:63-88`
The snapshot methods lack mutex protection. Since PR #32 is adding mutex protection for event methods, please add the same protection to `SaveSnapshot` and `GetLatestSnapshot` for consistency.
**2. No nil validation in SaveSnapshot** - `store/memory.go:63`
Passing nil snapshot will cause a panic. Please add validation:
```go
func (es *InMemoryEventStore) SaveSnapshot(snapshot *aether.ActorSnapshot) error {
if snapshot == nil {
return fmt.Errorf("snapshot cannot be nil")
}
// ...
}
```
**3. Test key collision** - `store/memory_test.go:395`
The key generation in `TestSnapshotDataIntegrity_LargeState` has collisions due to modulo arithmetic (`i=0` and `i=260` both produce `"a0"`). Consider using `fmt.Sprintf("key-%d", i)` instead.
### Suggestions (optional)
- Add a concurrent access test for snapshots (similar to PR #32's event concurrency test)
Once the thread safety and nil validation are addressed, this will be ready to merge.
- Add mutex protection to SaveSnapshot and GetLatestSnapshot
- Add nil validation in SaveSnapshot
- Fix key collision in TestSnapshotDataIntegrity_LargeState (use fmt.Sprintf)
- Add concurrent access tests for snapshots
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
All feedback from the previous review has been addressed:
Thread safety: Added mutex protection to SaveSnapshot (Lock) and GetLatestSnapshot (RLock)
Nil validation: SaveSnapshot now returns error for nil snapshot
Test key collision: Fixed to use fmt.Sprintf("key-%d", i)
Concurrent tests: Added TestConcurrentSaveSnapshot and TestConcurrentSaveAndGetSnapshot
LGTM - merging now.
## Re-Review - Approved
All feedback from the previous review has been addressed:
- **Thread safety**: Added mutex protection to `SaveSnapshot` (Lock) and `GetLatestSnapshot` (RLock)
- **Nil validation**: `SaveSnapshot` now returns error for nil snapshot
- **Test key collision**: Fixed to use `fmt.Sprintf("key-%d", i)`
- **Concurrent tests**: Added `TestConcurrentSaveSnapshot` and `TestConcurrentSaveAndGetSnapshot`
LGTM - merging now.
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.
Summary
Implement SaveSnapshot and GetLatestSnapshot methods on InMemoryEventStore to satisfy the SnapshotStore interface, along with comprehensive unit tests.
Changes
Test Coverage
Closes #4
Code Review
Summary
Good implementation of SnapshotStore interface with comprehensive tests. A few issues to address before merging.
Issues
1. Missing thread safety (Bug) -
store/memory.go:63-88The snapshot methods lack mutex protection. Since PR #32 is adding mutex protection for event methods, please add the same protection to
SaveSnapshotandGetLatestSnapshotfor consistency.2. No nil validation in SaveSnapshot -
store/memory.go:63Passing nil snapshot will cause a panic. Please add validation:
3. Test key collision -
store/memory_test.go:395The key generation in
TestSnapshotDataIntegrity_LargeStatehas collisions due to modulo arithmetic (i=0andi=260both produce"a0"). Consider usingfmt.Sprintf("key-%d", i)instead.Suggestions (optional)
Once the thread safety and nil validation are addressed, this will be ready to merge.
Re-Review - Approved
All feedback from the previous review has been addressed:
SaveSnapshot(Lock) andGetLatestSnapshot(RLock)SaveSnapshotnow returns error for nil snapshotfmt.Sprintf("key-%d", i)TestConcurrentSaveSnapshotandTestConcurrentSaveAndGetSnapshotLGTM - merging now.