Fixed diff between key not found and key found with empty value (#24)

xload/providers/cached/cache.go

@@ -6,10 +6,14 @@ import (
 )
 
 // Cacher is the interface for custom cache implementations.
+// Implementations must distinguish between key not found and
+// key found with empty value.
+// `Get` must return nil for key not found.
+// `Set` must cache empty values.
 //
 //go:generate mockery --name Cacher --structname MockCache --filename mock_test.go --outpkg cached --output .
 type Cacher interface {
-	Get(key string) (string, error)
+	Get(key string) (*string, error)
 	Set(key, value string, ttl time.Duration) error
 }
 
@@ -34,21 +38,22 @@ func NewMapCache() *MapCache {
 }
 
 // Get returns the value for the given key, if cached.
-func (c *MapCache) Get(key string) (string, error) {
+// If the value is not cached, it returns nil.
+func (c *MapCache) Get(key string) (*string, error) {
 	v, ok := c.m.Load(key)
 	if !ok {
-		return "", nil
+		return nil, nil
 	}
 
 	mv, ok := v.(*mv)
 
 	if !ok || c.now().After(mv.ttl) {
 		c.delete(key)
 
-		return "", nil
+		return nil, nil
 	}
 
-	return mv.val, nil
+	return &mv.val, nil
 }
 
 // Set sets the value for the given key.

xload/providers/cached/cache_test.go

@@ -15,11 +15,11 @@ func TestMapCache(t *testing.T) {
 
 	v, err := cache.Get("foo")
 	assert.NoError(t, err)
-	assert.Equal(t, "bar", v)
+	assert.Equal(t, "bar", *v)
 
 	v, err = cache.Get("not-found")
 	assert.NoError(t, err)
-	assert.Equal(t, "", v)
+	assert.Nil(t, v)
 }
 
 func TestMapCacheExpired(t *testing.T) {
@@ -39,5 +39,5 @@ func TestMapCacheExpired(t *testing.T) {
 
 	v, err := cache.Get("foo")
 	assert.NoError(t, err)
-	assert.Equal(t, "", v)
+	assert.Nil(t, v)
 }

xload/providers/cached/example_test.go

@@ -43,8 +43,8 @@ func NewCustomCache() *CustomCache {
 	return &CustomCache{}
 }
 
-func (c *CustomCache) Get(key string) (string, error) {
-	return "", nil
+func (c *CustomCache) Get(key string) (*string, error) {
+	return nil, nil
 }
 
 func (c *CustomCache) Set(key, value string, ttl time.Duration) error {

xload/providers/cached/loader.go

@@ -23,8 +23,8 @@ func NewLoader(l xload.Loader, opts ...Option) xload.LoaderFunc {
 			return "", err
 		}
 
-		if v != "" {
-			return v, nil
+		if v != nil {
+			return *v, nil
 		}
 
 		loaded, err := l.Load(ctx, key)

xload/providers/cached/loader_test.go

@@ -53,7 +53,7 @@ func TestNewLoader_WithTTL(t *testing.T) {
 	ttl := 123 * time.Second
 
 	mc := NewMockCache(t)
-	mc.On("Get", mock.Anything).Return("", nil).Times(3)
+	mc.On("Get", mock.Anything).Return(nil, nil).Times(3)
 	mc.On("Set", mock.Anything, mock.Anything, ttl).Return(nil).Times(3)
 
 	cachedLoader := NewLoader(loader, TTL(ttl), Cache(mc))
@@ -66,6 +66,42 @@ func TestNewLoader_WithTTL(t *testing.T) {
 	mc.AssertExpectations(t)
 }
 
+func TestNewLoader_EmptyValueHit(t *testing.T) {
+	loader := xload.MapLoader(map[string]string{
+		"KEY_1": "value-1",
+		"KEY_2": "value-2",
+	})
+
+	ttl := 123 * time.Second
+
+	mc := NewMockCache(t)
+	mc.On("Get", "KEY_1").Return(nil, nil)
+	mc.On("Set", "KEY_1", "value-1", ttl).Return(nil)
+
+	mc.On("Get", "KEY_2").Return(nil, nil)
+	mc.On("Set", "KEY_2", "value-2", ttl).Return(nil)
+
+	mc.On("Get", "KEY_3").Return(nil, nil)
+	mc.On("Set", "KEY_3", "", ttl).Return(nil)
+
+	cachedLoader := NewLoader(loader, TTL(ttl), Cache(mc))
+
+	cfg := config{}
+
+	err := xload.Load(context.Background(), &cfg, cachedLoader)
+	assert.NoError(t, err)
+
+	// load again to ensure that the empty value is cached
+	mc.On("Get", "KEY_1").Return("value-1", nil)
+	mc.On("Get", "KEY_2").Return("value-2", nil)
+	mc.On("Get", "KEY_3").Return("", nil)
+
+	err = xload.Load(context.Background(), &cfg, cachedLoader)
+	assert.NoError(t, err)
+
+	mc.AssertExpectations(t)
+}
+
 func TestNewLoader_WithDisableEmptyValueHit(t *testing.T) {
 	loader := xload.MapLoader(map[string]string{
 		"KEY_1": "value-1",
@@ -75,8 +111,13 @@ func TestNewLoader_WithDisableEmptyValueHit(t *testing.T) {
 	ttl := 123 * time.Second
 
 	mc := NewMockCache(t)
-	mc.On("Get", mock.Anything).Return("", nil).Times(6)
-	mc.On("Set", mock.Anything, mock.Anything, ttl).Return(nil).Times(4)
+	mc.On("Get", "KEY_1").Return(nil, nil)
+	mc.On("Set", "KEY_1", "value-1", ttl).Return(nil)
+
+	mc.On("Get", "KEY_2").Return(nil, nil)
+	mc.On("Set", "KEY_2", "value-2", ttl).Return(nil)
+
+	mc.On("Get", "KEY_3").Return(nil, nil)
 
 	cachedLoader := NewLoader(loader, TTL(ttl), Cache(mc), DisableEmptyValueHit)
 
@@ -86,6 +127,10 @@ func TestNewLoader_WithDisableEmptyValueHit(t *testing.T) {
 	assert.NoError(t, err)
 
 	// load again to ensure that the empty value is not cached
+	mc.On("Get", "KEY_1").Return("value-1", nil)
+	mc.On("Get", "KEY_2").Return("value-2", nil)
+	mc.On("Get", "KEY_3").Return(nil, nil)
+
 	err = xload.Load(context.Background(), &cfg, cachedLoader)
 	assert.NoError(t, err)
 
@@ -117,7 +162,7 @@ func TestNewLoader_CacheError(t *testing.T) {
 	t.Run("Cache SET error", func(t *testing.T) {
 		mc := NewMockCache(t)
 
-		mc.On("Get", "KEY_1").Return("", nil)
+		mc.On("Get", "KEY_1").Return(nil, nil)
 		mc.On("Set", "KEY_1", "value-1", mock.Anything).Return(assert.AnError)
 
 		cachedLoader := NewLoader(loader, Cache(mc))
@@ -132,7 +177,7 @@ func TestNewLoader_CacheError(t *testing.T) {
 	t.Run("Cache GET error", func(t *testing.T) {
 		mc := NewMockCache(t)
 
-		mc.On("Get", "KEY_1").Return("", assert.AnError)
+		mc.On("Get", "KEY_1").Return(nil, assert.AnError)
 
 		cachedLoader := NewLoader(loader, Cache(mc))