refactor: decouple memstore from retriever in cache package

4 files changed, 70 insertions, 75 deletions

diff --git a/internal/source/gitlab/cache/cache.go b/internal/source/gitlab/cache/cache.go
index cb12c088..baae3b29 100644
--- a/internal/source/gitlab/cache/cache.go
+++ b/internal/source/gitlab/cache/cache.go
@@ -2,6 +2,7 @@ package cache
 
 import (
 	"context"
+	"errors"
 
 	"gitlab.com/gitlab-org/gitlab-pages/internal/config"
 	"gitlab.com/gitlab-org/gitlab-pages/internal/source/gitlab/api"
@@ -10,15 +11,18 @@ import (
 
 // Cache is a short and long caching mechanism for GitLab source
 type Cache struct {
-	client api.Client
-	store  Store
+	client    api.Client
+	store     Store
+	retriever *Retriever
 }
 
 // NewCache creates a new instance of Cache.
 func NewCache(client api.Client, cc *config.Cache) *Cache {
+	r := NewRetriever(client, cc.RetrievalTimeout, cc.MaxRetrievalInterval, cc.MaxRetrievalRetries)
 	return &Cache{
-		client: client,
-		store:  newMemStore(client, cc),
+		client:    client,
+		store:     newMemStore(client, cc),
+		retriever: r,
 	}
 }
 
@@ -79,13 +83,53 @@ func (c *Cache) Resolve(ctx context.Context, domain string) *api.Lookup {
 		return entry.Lookup()
 	}
 
+	metrics.DomainsSourceCacheMiss.Inc()
 	if entry.NeedsRefresh() {
-		entry.Refresh(c.store)
-
-		metrics.DomainsSourceCacheHit.Inc()
+		c.Refresh(entry)
 		return entry.Lookup()
 	}
 
-	metrics.DomainsSourceCacheMiss.Inc()
-	return entry.Retrieve(ctx)
+	return c.retrieve(ctx, entry)
+}
+
+func (c *Cache) retrieve(ctx context.Context, entry *Entry) *api.Lookup {
+	// We run the code within an additional func() to run both `e.setResponse`
+	// and `e.retrieve.Retrieve` asynchronously.
+	entry.retrieve.Do(func() { go func() { entry.setResponse(c.retriever.Retrieve(ctx, entry.domain)) }() })
+
+	var lookup *api.Lookup
+	select {
+	case <-ctx.Done():
+		lookup = &api.Lookup{Name: entry.domain, Error: errors.New("context done")}
+	case <-entry.retrieved:
+		lookup = entry.Lookup()
+	}
+
+	return lookup
+}
+
+// Refresh will update the entry in the store only when it gets resolved successfully.
+// If an existing successful entry exists, it will only be replaced if the new resolved
+// entry is successful too.
+// Errored refreshed Entry responses will not replace the previously successful entry.response
+// for a maximum time of e.expirationTimeout.
+func (c *Cache) Refresh(entry *Entry) {
+	entry.refresh.Do(func() {
+		go c.refreshFunc(entry)
+	})
+}
+
+func (c *Cache) refreshFunc(e *Entry) {
+	entry := newCacheEntry(e.domain, e.refreshTimeout, e.expirationTimeout)
+
+	c.retrieve(context.Background(), entry)
+
+	// do not replace existing Entry `e.response` when `entry.response` has an error
+	// and `e` has not expired. See https://gitlab.com/gitlab-org/gitlab-pages/-/issues/281.
+	if !e.isExpired() && entry.hasTemporaryError() {
+		entry.response = e.response
+		entry.refreshedOriginalTimestamp = e.created
+	}
+
+	c.store.ReplaceOrCreate(e.domain, entry)
 }
diff --git a/internal/source/gitlab/cache/entry.go b/internal/source/gitlab/cache/entry.go
index 769f1713..742df9c8 100644
--- a/internal/source/gitlab/cache/entry.go
+++ b/internal/source/gitlab/cache/entry.go
@@ -1,7 +1,6 @@
 package cache
 
 import (
-	"context"
 	"errors"
 	"os"
 	"sync"
@@ -25,10 +24,9 @@ type Entry struct {
 	response                   *api.Lookup
 	refreshTimeout             time.Duration
 	expirationTimeout          time.Duration
-	retriever                  *Retriever
 }
 
-func newCacheEntry(domain string, refreshTimeout, entryExpirationTimeout time.Duration, retriever *Retriever) *Entry {
+func newCacheEntry(domain string, refreshTimeout, entryExpirationTimeout time.Duration) *Entry {
 	return &Entry{
 		domain:            domain,
 		created:           time.Now(),
@@ -38,7 +36,6 @@ func newCacheEntry(domain string, refreshTimeout, entryExpirationTimeout time.Du
 		retrieved:         make(chan struct{}),
 		refreshTimeout:    refreshTimeout,
 		expirationTimeout: entryExpirationTimeout,
-		retriever:         retriever,
 	}
 }
 
@@ -68,48 +65,6 @@ func (e *Entry) Lookup() *api.Lookup {
 	return e.response
 }
 
-// Retrieve perform a blocking retrieval of the cache entry response.
-func (e *Entry) Retrieve(ctx context.Context) (lookup *api.Lookup) {
-	// We run the code within an additional func() to run both `e.setResponse`
-	// and `e.retrieve.Retrieve` asynchronously.
-	e.retrieve.Do(func() { go func() { e.setResponse(e.retriever.Retrieve(ctx, e.domain)) }() })
-
-	select {
-	case <-ctx.Done():
-		lookup = &api.Lookup{Name: e.domain, Error: errors.New("context done")}
-	case <-e.retrieved:
-		lookup = e.Lookup()
-	}
-
-	return lookup
-}
-
-// Refresh will update the entry in the store only when it gets resolved successfully.
-// If an existing successful entry exists, it will only be replaced if the new resolved
-// entry is successful too.
-// Errored refreshed Entry responses will not replace the previously successful entry.response
-// for a maximum time of e.expirationTimeout.
-func (e *Entry) Refresh(store Store) {
-	e.refresh.Do(func() {
-		go e.refreshFunc(store)
-	})
-}
-
-func (e *Entry) refreshFunc(store Store) {
-	entry := newCacheEntry(e.domain, e.refreshTimeout, e.expirationTimeout, e.retriever)
-
-	entry.Retrieve(context.Background())
-
-	// do not replace existing Entry `e.response` when `entry.response` has an error
-	// and `e` has not expired. See https://gitlab.com/gitlab-org/gitlab-pages/-/issues/281.
-	if !e.isExpired() && entry.hasTemporaryError() {
-		entry.response = e.response
-		entry.refreshedOriginalTimestamp = e.created
-	}
-
-	store.ReplaceOrCreate(e.domain, entry)
-}
-
 func (e *Entry) setResponse(lookup api.Lookup) {
 	e.mux.Lock()
 	defer e.mux.Unlock()
diff --git a/internal/source/gitlab/cache/entry_test.go b/internal/source/gitlab/cache/entry_test.go
index 07282cb5..d50822ce 100644
--- a/internal/source/gitlab/cache/entry_test.go
+++ b/internal/source/gitlab/cache/entry_test.go
@@ -55,7 +55,7 @@ func TestIsUpToDateAndNeedsRefresh(t *testing.T) {
 
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
-			entry := newCacheEntry("my.gitlab.com", testCacheConfig.EntryRefreshTimeout, testCacheConfig.CacheExpiry, nil)
+			entry := newCacheEntry("my.gitlab.com", testCacheConfig.EntryRefreshTimeout, testCacheConfig.CacheExpiry)
 			if tt.resolved {
 				entry.response = &api.Lookup{}
 			}
@@ -85,6 +85,7 @@ func TestEntryRefresh(t *testing.T) {
 			},
 		},
 	}
+
 	cc := &config.Cache{
 		CacheExpiry:          100 * time.Millisecond,
 		EntryRefreshTimeout:  time.Millisecond,
@@ -92,26 +93,25 @@ func TestEntryRefresh(t *testing.T) {
 		MaxRetrievalInterval: time.Millisecond,
 		MaxRetrievalRetries:  1,
 	}
-
-	store := newMemStore(client, cc)
+	cache := NewCache(client, cc)
 
 	t.Run("entry is the same after refreshed lookup has error", func(t *testing.T) {
-		entry := newCacheEntry("test.gitlab.io", cc.EntryRefreshTimeout, cc.CacheExpiry, store.(*memstore).retriever)
+		entry := newCacheEntry("test.gitlab.io", cc.EntryRefreshTimeout, cc.CacheExpiry)
 		originalEntryCreated := entry.created
 
 		ctx, cancel := context.WithTimeout(context.Background(), cc.RetrievalTimeout)
 		defer cancel()
 
-		lookup := entry.Retrieve(ctx)
+		lookup := cache.retrieve(ctx, entry)
 		require.NoError(t, lookup.Error)
 
 		require.Eventually(t, entry.NeedsRefresh, 100*time.Millisecond, time.Millisecond, "entry should need refresh")
 
-		entry.refreshFunc(store)
+		cache.refreshFunc(entry)
 
 		require.True(t, client.failed, "refresh should have failed")
 
-		storedEntry := loadEntry(t, "test.gitlab.io", store)
+		storedEntry := loadEntry(t, "test.gitlab.io", cache.store)
 
 		require.NoError(t, storedEntry.Lookup().Error, "resolving failed but lookup should still be valid")
 		require.Equal(t, storedEntry.refreshedOriginalTimestamp.UnixNano(), originalEntryCreated.UnixNano(),
@@ -123,23 +123,23 @@ func TestEntryRefresh(t *testing.T) {
 		client.failed = false
 		err := os.Setenv("FF_DISABLE_REFRESH_TEMPORARY_ERROR", "true")
 
-		entry := newCacheEntry("test.gitlab.io", cc.EntryRefreshTimeout, cc.CacheExpiry, store.(*memstore).retriever)
+		entry := newCacheEntry("test.gitlab.io", cc.EntryRefreshTimeout, cc.CacheExpiry)
 
 		ctx, cancel := context.WithTimeout(context.Background(), cc.RetrievalTimeout)
 		defer cancel()
 
-		lookup := entry.Retrieve(ctx)
+		lookup := cache.retrieve(ctx, entry)
 		require.NoError(t, lookup.Error)
 
 		require.Eventually(t, entry.NeedsRefresh, 100*time.Millisecond, time.Millisecond, "entry should need refresh")
 
 		require.NoError(t, err)
 
-		entry.refreshFunc(store)
+		cache.refreshFunc(entry)
 
 		require.True(t, client.failed, "refresh should have failed")
 
-		storedEntry := loadEntry(t, "test.gitlab.io", store)
+		storedEntry := loadEntry(t, "test.gitlab.io", cache.store)
 
 		require.Error(t, storedEntry.Lookup().Error, "resolving failed")
 		require.True(t, storedEntry.refreshedOriginalTimestamp.IsZero())
@@ -149,23 +149,23 @@ func TestEntryRefresh(t *testing.T) {
 	t.Run("entry is different after it expired and calling refresh on it", func(t *testing.T) {
 		client.failed = false
 
-		entry := newCacheEntry("error.gitlab.io", cc.EntryRefreshTimeout, cc.CacheExpiry, store.(*memstore).retriever)
+		entry := newCacheEntry("error.gitlab.io", cc.EntryRefreshTimeout, cc.CacheExpiry)
 
 		ctx, cancel := context.WithTimeout(context.Background(), cc.RetrievalTimeout)
 		defer cancel()
 
-		lookup := entry.Retrieve(ctx)
+		lookup := cache.retrieve(ctx, entry)
 		require.Error(t, lookup.Error)
 		require.Eventually(t, entry.NeedsRefresh, 100*time.Millisecond, time.Millisecond, "entry should need refresh")
 
 		// wait for entry to expire
 		time.Sleep(cc.CacheExpiry)
 		// refreshing the entry after it has expired should create a completely new one
-		entry.refreshFunc(store)
+		cache.refreshFunc(entry)
 
 		require.True(t, client.failed, "refresh should have failed")
 
-		storedEntry := loadEntry(t, "error.gitlab.io", store)
+		storedEntry := loadEntry(t, "error.gitlab.io", cache.store)
 		require.NotEqual(t, storedEntry, entry, "stored entry should be different")
 		require.Greater(t, storedEntry.created.UnixNano(), entry.created.UnixNano(), "")
 	})
diff --git a/internal/source/gitlab/cache/memstore.go b/internal/source/gitlab/cache/memstore.go
index 8ba069e4..40661ce9 100644
--- a/internal/source/gitlab/cache/memstore.go
+++ b/internal/source/gitlab/cache/memstore.go
@@ -13,18 +13,14 @@ import (
 type memstore struct {
 	store                  *cache.Cache
 	mux                    *sync.RWMutex
-	retriever              *Retriever
 	entryRefreshTimeout    time.Duration
 	entryExpirationTimeout time.Duration
 }
 
 func newMemStore(client api.Client, cc *config.Cache) Store {
-	retriever := NewRetriever(client, cc.RetrievalTimeout, cc.MaxRetrievalInterval, cc.MaxRetrievalRetries)
-
 	return &memstore{
 		store:                  cache.New(cc.CacheExpiry, cc.CacheCleanupInterval),
 		mux:                    &sync.RWMutex{},
-		retriever:              retriever,
 		entryRefreshTimeout:    cc.EntryRefreshTimeout,
 		entryExpirationTimeout: cc.CacheExpiry,
 	}
@@ -48,7 +44,7 @@ func (m *memstore) LoadOrCreate(domain string) *Entry {
 		return entry.(*Entry)
 	}
 
-	newEntry := newCacheEntry(domain, m.entryRefreshTimeout, m.entryExpirationTimeout, m.retriever)
+	newEntry := newCacheEntry(domain, m.entryRefreshTimeout, m.entryExpirationTimeout)
 	m.store.SetDefault(domain, newEntry)
 
 	return newEntry