Add SyncMap cache implementation and tests #5
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds a new cache implementation based on Go's sync.Map and includes corresponding tests and a minor .gitignore update. The implementation provides a simple, thread-safe in-memory cache that conforms to the Cache[T] interface defined in the codebase.
- Implements
SyncMap[T]cache backed bysync.Mapwith generic type safety - Adds basic test coverage for the cache implementation
- Updates .gitignore to exclude .windsurf directory
Reviewed changes
Copilot reviewed 2 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| syncmap.go | Implements SyncMap cache with Set, Get, and Del methods using embedded sync.Map |
| syncmap_test.go | Adds tests covering basic CRUD operations and embedded sync.Map method access |
| .gitignore | Adds .windsurf IDE directory to ignore list |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| type SyncMap[T any] struct { | ||
| sync.Map |
There was a problem hiding this comment.
Embedding sync.Map exposes its untyped methods (Store, Load, Delete, Range, etc.) which bypass the generic type safety provided by the Cache interface. This creates a confusing API where users can accidentally store values of the wrong type using cache.Store() directly, leading to runtime panics in Get(). Consider using composition instead of embedding (e.g., having a private field 'data sync.Map') to maintain type safety and provide a cleaner API surface.
| func TestSyncMapEmbeddedMethods(t *testing.T) { | ||
| cache := NewSyncMap[int]() | ||
|
|
||
| cache.Store("a", 1) | ||
| cache.Store("b", 2) | ||
|
|
||
| v, ok := cache.Load("a") | ||
| require.True(t, ok) | ||
| assert.Equal(t, 1, v) | ||
|
|
||
| count := 0 | ||
| cache.Range(func(k, v any) bool { | ||
| count++ | ||
| return true | ||
| }) | ||
| assert.Equal(t, 2, count) | ||
| } |
There was a problem hiding this comment.
This test demonstrates accessing embedded sync.Map methods directly, which bypasses type safety and can lead to runtime panics. While this test verifies that the embedding works, it doesn't test the critical failure scenario: what happens when someone stores a value of the wrong type (e.g., cache.Store("a", "wrongtype")) and then calls the typed Get method. This scenario would cause a panic that should be either tested or prevented through better API design.
| if !ok { | ||
| return zero, errors.Wrapf(&ErrKeyNotFound{}, "key not found in syncmap for key: %s", key) | ||
| } | ||
| return v.(T), nil |
There was a problem hiding this comment.
The unchecked type assertion can panic if a value of the wrong type is stored. This is particularly dangerous because the embedded sync.Map allows direct access to Store/Load/Range methods, which bypass the generic type safety. For example, if someone calls cache.Store("key", wrongTypeValue) directly and then calls cache.Get(ctx, "key"), the type assertion will panic. Consider either: (1) using a safe type assertion with ok check and returning an error if types don't match, or (2) not embedding sync.Map to prevent direct access to its methods.
xRef: