diff --git a/cache/cache.go b/cache/cache.go index d42ba127..af5ba8d4 100644 --- a/cache/cache.go +++ b/cache/cache.go @@ -168,35 +168,34 @@ func (c *cache[T]) set(key string, value T) { c.items = append(c.items, &item) } -// Get returns a pointer to an item in the cache for the given key. If no item -// is found, it's a nil pointer. +// Get returns an item in the cache for the given key. If no item is found, an +// error is returned. // The caller can record cache hit or miss based on the result with // Cache.RecordCacheEvent(). -func (c *Cache[T]) Get(key string) (*T, error) { +func (c *Cache[T]) Get(key string) (T, error) { + var res T c.mu.RLock() if c.closed { c.mu.RUnlock() recordRequest(c.metrics, StatusFailure) - return nil, ErrCacheClosed + return res, ErrCacheClosed } item, found := c.index[key] if !found { c.mu.RUnlock() recordRequest(c.metrics, StatusSuccess) - return nil, nil + return res, ErrNotFound } if !item.expiresAt.IsZero() { if item.expiresAt.Compare(time.Now()) < 0 { c.mu.RUnlock() recordRequest(c.metrics, StatusSuccess) - return nil, nil + return res, ErrNotFound } } c.mu.RUnlock() recordRequest(c.metrics, StatusSuccess) - // Copy the value to prevent writes to the cached item. - r := item.value - return &r, nil + return item.value, nil } // Delete an item from the cache. Does nothing if the key is not in the cache. diff --git a/cache/cache_test.go b/cache/cache_test.go index 5d5d3c6c..509801b1 100644 --- a/cache/cache_test.go +++ b/cache/cache_test.go @@ -40,8 +40,8 @@ func TestCache(t *testing.T) { key1 := "key1" value1 := "val1" got, err := cache.Get(key1) - g.Expect(err).ToNot(HaveOccurred()) - g.Expect(got).To(BeNil()) + g.Expect(err).To(Equal(ErrNotFound)) + g.Expect(got).To(BeEmpty()) // Add an item to the cache err = cache.Set(key1, value1) @@ -50,13 +50,13 @@ func TestCache(t *testing.T) { // Get the item from the cache got, err = cache.Get(key1) g.Expect(err).ToNot(HaveOccurred()) - g.Expect(*got).To(Equal(value1)) + g.Expect(got).To(Equal(value1)) // Writing to the obtained value doesn't update the cache. - *got = "val2" + got = "val2" got2, err := cache.Get(key1) g.Expect(err).ToNot(HaveOccurred()) - g.Expect(*got2).To(Equal(value1)) + g.Expect(got2).To(Equal(value1)) // Add another item to the cache key2 := "key2" @@ -68,7 +68,7 @@ func TestCache(t *testing.T) { // Get the item from the cache got, err = cache.Get(key2) g.Expect(err).ToNot(HaveOccurred()) - g.Expect(*got).To(Equal(value2)) + g.Expect(got).To(Equal(value2)) // Update an item in the cache key3 := "key3" @@ -84,7 +84,7 @@ func TestCache(t *testing.T) { // Get the item from the cache got, err = cache.Get(key3) g.Expect(err).ToNot(HaveOccurred()) - g.Expect(*got).To(Equal(value4)) + g.Expect(got).To(Equal(value4)) // cleanup the cache cache.Clear() @@ -118,15 +118,15 @@ func TestCache(t *testing.T) { // Get the item from the cache item, err := cache.Get(key) g.Expect(err).ToNot(HaveOccurred()) - g.Expect(*item).To(Equal(value)) + g.Expect(item).To(Equal(value)) // wait for the item to expire time.Sleep(3 * time.Second) // Get the item from the cache item, err = cache.Get(key) - g.Expect(err).ToNot(HaveOccurred()) - g.Expect(item).To(BeNil()) + g.Expect(err).To(Equal(ErrNotFound)) + g.Expect(item).To(BeEmpty()) }) t.Run("Cache of integer value", func(t *testing.T) { @@ -140,7 +140,7 @@ func TestCache(t *testing.T) { got, err := cache.Get(key) g.Expect(err).To(Succeed()) - g.Expect(*got).To(Equal(4)) + g.Expect(got).To(Equal(4)) }) } @@ -216,8 +216,8 @@ func Test_Cache_Get(t *testing.T) { value := "val1" got, err := cache.Get(key) - g.Expect(err).ToNot(HaveOccurred()) - g.Expect(got).To(BeNil()) + g.Expect(err).To(Equal(ErrNotFound)) + g.Expect(got).To(BeEmpty()) cache.RecordCacheEvent(CacheEventTypeMiss, recObjKind, recObjName, recObjNamespace) err = cache.Set(key, value) @@ -225,7 +225,7 @@ func Test_Cache_Get(t *testing.T) { got, err = cache.Get(key) g.Expect(err).ToNot(HaveOccurred()) - g.Expect(*got).To(Equal(value)) + g.Expect(got).To(Equal(value)) cache.RecordCacheEvent(CacheEventTypeHit, recObjKind, recObjName, recObjNamespace) validateMetrics(reg, ` @@ -472,7 +472,6 @@ func TestCache_Concurrent(t *testing.T) { for _, key := range keymap { val, err := cache.Get(key) g.Expect(err).ToNot(HaveOccurred()) - g.Expect(val).ToNot(BeNil(), "object %s not found", key) - g.Expect(*val).To(Equal("test-token")) + g.Expect(val).To(Equal("test-token")) } } diff --git a/cache/doc.go b/cache/doc.go index 699b5c69..f1456014 100644 --- a/cache/doc.go +++ b/cache/doc.go @@ -39,10 +39,10 @@ limitations under the License. // // Handle any error. // ... // -// if got != nil { -// cache.RecordCacheEvent(CacheEventTypeHit, "GitRepository", "repoA", "testNS") -// } else { +// if err == ErrNotFound { // cache.RecordCacheEvent(CacheEventTypeMiss, "GitRepository", "repoA", "testNS") +// } else { +// cache.RecordCacheEvent(CacheEventTypeHit, "GitRepository", "repoA", "testNS") // } // // When the Flux object associated with the cache metrics is deleted, the diff --git a/cache/lru.go b/cache/lru.go index 561eaf55..61fc5801 100644 --- a/cache/lru.go +++ b/cache/lru.go @@ -170,25 +170,24 @@ func (c *LRU[T]) delete(node *node[T]) { delete(c.cache, node.key) } -// Get returns a pointer to an item in the cache for the given key. If no item -// is found, it's a nil pointer. +// Get returns an item in the cache for the given key. If no item is found, an +// error is returned. // The caller can record cache hit or miss based on the result with // LRU.RecordCacheEvent(). -func (c *LRU[T]) Get(key string) (*T, error) { +func (c *LRU[T]) Get(key string) (T, error) { + var res T c.mu.Lock() node, ok := c.cache[key] if !ok { c.mu.Unlock() recordRequest(c.metrics, StatusSuccess) - return nil, nil + return res, ErrNotFound } c.delete(node) _ = c.add(node) c.mu.Unlock() recordRequest(c.metrics, StatusSuccess) - // Copy the value to prevent writes to the cached item. - r := node.value - return &r, nil + return node.value, nil } // ListKeys returns a list of keys in the cache. diff --git a/cache/lru_test.go b/cache/lru_test.go index 57b93c63..645f0fac 100644 --- a/cache/lru_test.go +++ b/cache/lru_test.go @@ -163,8 +163,8 @@ func Test_LRU_Get(t *testing.T) { key1 := "key1" value1 := "val1" got, err := cache.Get(key1) - g.Expect(err).ToNot(HaveOccurred()) - g.Expect(got).To(BeNil()) + g.Expect(err).To(Equal(ErrNotFound)) + g.Expect(got).To(BeEmpty()) cache.RecordCacheEvent(CacheEventTypeMiss, recObjKind, recObjName, recObjNamespace) err = cache.Set(key1, value1) @@ -172,7 +172,7 @@ func Test_LRU_Get(t *testing.T) { got, err = cache.Get(key1) g.Expect(err).ToNot(HaveOccurred()) - g.Expect(*got).To(Equal(value1)) + g.Expect(got).To(Equal(value1)) cache.RecordCacheEvent(CacheEventTypeHit, recObjKind, recObjName, recObjNamespace) validateMetrics(reg, ` @@ -309,8 +309,7 @@ func TestLRU_Concurrent(t *testing.T) { for _, key := range keymap { val, err := cache.Get(key) g.Expect(err).ToNot(HaveOccurred()) - g.Expect(val).ToNot(BeNil(), "object %s not found", key) - g.Expect(*val).To(Equal("test-token")) + g.Expect(val).To(Equal("test-token")) } } @@ -325,5 +324,5 @@ func TestLRU_int(t *testing.T) { got, err := cache.Get(key) g.Expect(err).To(Succeed()) - g.Expect(*got).To(Equal(4)) + g.Expect(got).To(Equal(4)) } diff --git a/cache/store.go b/cache/store.go index d0dce133..a5207bf9 100644 --- a/cache/store.go +++ b/cache/store.go @@ -27,7 +27,7 @@ type Store[T any] interface { // Set adds an item to the store for the given key. Set(key string, value T) error // Get returns an item stored in the store for the given key. - Get(key string) (*T, error) + Get(key string) (T, error) // Delete deletes an item in the store for the given key. Delete(key string) error }