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 <github@darkowlzz.space>

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.

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,16 +216,16 @@ 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)
 	g.Expect(err).ToNot(HaveOccurred())
 
 	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"))
 	}
 }

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

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.

cache/lru_test.go

@@ -163,16 +163,16 @@ 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)
 	g.Expect(err).ToNot(HaveOccurred())
 
 	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))
 }

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
 }