From edc8a5a4f2ad4b6067a5be02b31253abd786c9c7 Mon Sep 17 00:00:00 2001 From: Sunny Date: Wed, 27 Nov 2024 16:59:26 +0000 Subject: [PATCH] cache: Return ErrNotFound error from Get() Previously, when an item is not found in the cache, no error was returned and instead the returned value was expected to be checked for nil to determine if the object was found or not. Considering that other cache functions like GetExpiration() and SetExpiration() return ErrNotFound for items not in cache, it's more consistent to return ErrNotFound for Get() as well. Returning ErrNotFound from Get() makes it clear that the object was not found, instead of needing to check the obtained value. There is no more need to return a pointer to the item. Return the item value instead. This changes the Store interface method signature for Get(). Signed-off-by: Sunny --- cache/cache.go | 17 ++++++++--------- cache/cache_test.go | 31 +++++++++++++++---------------- cache/doc.go | 6 +++--- cache/lru.go | 13 ++++++------- cache/lru_test.go | 11 +++++------ cache/store.go | 2 +- 6 files changed, 38 insertions(+), 42 deletions(-) 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 }