Add caching on top of gitlab domains source client

9 files changed, 41 insertions, 49 deletions

diff --git a/internal/source/gitlab/api/client.go b/internal/source/gitlab/api/client.go
index 7206e25a..5d575cc6 100644
--- a/internal/source/gitlab/api/client.go
+++ b/internal/source/gitlab/api/client.go
@@ -7,5 +7,5 @@ import (
 // Client represents an interface we use to retrieve information from GitLab
 type Client interface {
 	// GetLookup retrives an VirtualDomain from GitLab API and wraps it into Lookup
-	GetLookup(ctx context.Context, domain string) Lookup
+	GetLookup(ctx context.Context, domain string) *Lookup
 }
diff --git a/internal/source/gitlab/cache/cache.go b/internal/source/gitlab/cache/cache.go
index 85f44e11..7bf755a3 100644
--- a/internal/source/gitlab/cache/cache.go
+++ b/internal/source/gitlab/cache/cache.go
@@ -20,7 +20,7 @@ func NewCache(client api.Client) *Cache {
 	}
 }
 
-// Resolve is going to return a Lookup based on a domain name. The caching
+// GetLookup is going to return a lookup based on a domain name. The caching
 // algorithm works as follows:
 // - We first check if the cache entry exists, and if it is up-to-date. If it
 //   is fresh we return the lookup entry from cache and it is a cache hit.
@@ -69,7 +69,7 @@ func NewCache(client api.Client) *Cache {
 //  - we create a lookup that contains information about an error
 //  - we cache this response
 //  - we pass this lookup upstream to all the clients
-func (c *Cache) Resolve(ctx context.Context, domain string) *api.Lookup {
+func (c *Cache) GetLookup(ctx context.Context, domain string) *api.Lookup {
 	entry := c.store.LoadOrCreate(domain)
 
 	if entry.IsUpToDate() {
@@ -84,8 +84,3 @@ func (c *Cache) Resolve(ctx context.Context, domain string) *api.Lookup {
 
 	return entry.Retrieve(ctx, c.client)
 }
-
-// New creates a new instance of Cache and sets default expiration
-func New() *Cache {
-	return &Cache{}
-}
diff --git a/internal/source/gitlab/cache/cache_test.go b/internal/source/gitlab/cache/cache_test.go
index ac7c6726..27c11b6d 100644
--- a/internal/source/gitlab/cache/cache_test.go
+++ b/internal/source/gitlab/cache/cache_test.go
@@ -21,7 +21,7 @@ type client struct {
 	failure error
 }
 
-func (c *client) GetLookup(ctx context.Context, _ string) api.Lookup {
+func (c *client) GetLookup(ctx context.Context, _ string) *api.Lookup {
 	var lookup api.Lookup
 
 	// TODO This might not work on some architectures
@@ -42,7 +42,7 @@ func (c *client) GetLookup(ctx context.Context, _ string) api.Lookup {
 		lookup.Error = c.failure
 	}
 
-	return lookup
+	return &lookup
 }
 
 func withTestCache(config resolverConfig, block func(*Cache, *client)) {
@@ -75,7 +75,7 @@ func (cache *Cache) withTestEntry(config entryConfig, block func(*Entry)) {
 	entry := cache.store.LoadOrCreate(domain)
 
 	if config.retrieved {
-		entry.setResponse(api.Lookup{Name: domain})
+		entry.setResponse(&api.Lookup{Name: domain})
 	}
 
 	if config.expired {
@@ -96,12 +96,12 @@ type entryConfig struct {
 	retrieved bool
 }
 
-func TestResolve(t *testing.T) {
+func TestGetLookup(t *testing.T) {
 	t.Run("when item is not cached", func(t *testing.T) {
 		withTestCache(resolverConfig{buffered: true}, func(cache *Cache, resolver *client) {
 			resolver.domain <- "my.gitlab.com"
 
-			lookup := cache.Resolve(context.Background(), "my.gitlab.com")
+			lookup := cache.GetLookup(context.Background(), "my.gitlab.com")
 
 			require.NoError(t, lookup.Error)
 			require.Equal(t, "my.gitlab.com", lookup.Name)
@@ -116,7 +116,7 @@ func TestResolve(t *testing.T) {
 
 			receiver := func() {
 				defer wg.Done()
-				cache.Resolve(ctx, "my.gitlab.com")
+				cache.GetLookup(ctx, "my.gitlab.com")
 			}
 
 			wg.Add(3)
@@ -136,7 +136,7 @@ func TestResolve(t *testing.T) {
 	t.Run("when item is in short cache", func(t *testing.T) {
 		withTestCache(resolverConfig{}, func(cache *Cache, resolver *client) {
 			cache.withTestEntry(entryConfig{expired: false, retrieved: true}, func(*Entry) {
-				lookup := cache.Resolve(context.Background(), "my.gitlab.com")
+				lookup := cache.GetLookup(context.Background(), "my.gitlab.com")
 
 				require.Equal(t, "my.gitlab.com", lookup.Name)
 				require.Equal(t, uint64(0), resolver.lookups)
@@ -150,7 +150,7 @@ func TestResolve(t *testing.T) {
 				lookup := make(chan *api.Lookup, 1)
 
 				go func() {
-					lookup <- cache.Resolve(context.Background(), "my.gitlab.com")
+					lookup <- cache.GetLookup(context.Background(), "my.gitlab.com")
 				}()
 
 				<-resolver.bootup
@@ -170,7 +170,7 @@ func TestResolve(t *testing.T) {
 	t.Run("when item is in long cache only", func(t *testing.T) {
 		withTestCache(resolverConfig{}, func(cache *Cache, resolver *client) {
 			cache.withTestEntry(entryConfig{expired: true, retrieved: true}, func(*Entry) {
-				lookup := cache.Resolve(context.Background(), "my.gitlab.com")
+				lookup := cache.GetLookup(context.Background(), "my.gitlab.com")
 
 				require.Equal(t, "my.gitlab.com", lookup.Name)
 				require.Equal(t, uint64(0), resolver.lookups)
@@ -184,9 +184,9 @@ func TestResolve(t *testing.T) {
 	t.Run("when item in long cache is requested multiple times", func(t *testing.T) {
 		withTestCache(resolverConfig{}, func(cache *Cache, resolver *client) {
 			cache.withTestEntry(entryConfig{expired: true, retrieved: true}, func(*Entry) {
-				cache.Resolve(context.Background(), "my.gitlab.com")
-				cache.Resolve(context.Background(), "my.gitlab.com")
-				cache.Resolve(context.Background(), "my.gitlab.com")
+				cache.GetLookup(context.Background(), "my.gitlab.com")
+				cache.GetLookup(context.Background(), "my.gitlab.com")
+				cache.GetLookup(context.Background(), "my.gitlab.com")
 
 				require.Equal(t, uint64(0), resolver.lookups)
 
@@ -200,7 +200,7 @@ func TestResolve(t *testing.T) {
 		withTestCache(resolverConfig{failure: errors.New("500 err")}, func(cache *Cache, resolver *client) {
 			maxRetrievalInterval = 0
 
-			lookup := cache.Resolve(context.Background(), "my.gitlab.com")
+			lookup := cache.GetLookup(context.Background(), "my.gitlab.com")
 
 			require.Equal(t, uint64(3), resolver.lookups)
 			require.EqualError(t, lookup.Error, "500 err")
@@ -212,7 +212,7 @@ func TestResolve(t *testing.T) {
 		cancel()
 
 		withTestCache(resolverConfig{}, func(cache *Cache, resolver *client) {
-			lookup := cache.Resolve(ctx, "my.gitlab.com")
+			lookup := cache.GetLookup(ctx, "my.gitlab.com")
 
 			require.Equal(t, uint64(0), resolver.lookups)
 			require.EqualError(t, lookup.Error, "context done")
@@ -224,7 +224,7 @@ func TestResolve(t *testing.T) {
 		defer func() { retrievalTimeout = 5 * time.Second }()
 
 		withTestCache(resolverConfig{}, func(cache *Cache, resolver *client) {
-			lookup := cache.Resolve(context.Background(), "my.gitlab.com")
+			lookup := cache.GetLookup(context.Background(), "my.gitlab.com")
 
 			require.Equal(t, uint64(0), resolver.lookups)
 			require.EqualError(t, lookup.Error, "retrieval context done")
@@ -237,7 +237,7 @@ func TestResolve(t *testing.T) {
 				ctx, cancel := context.WithCancel(context.Background())
 
 				response := make(chan *api.Lookup, 1)
-				go func() { response <- cache.Resolve(ctx, "my.gitlab.com") }()
+				go func() { response <- cache.GetLookup(ctx, "my.gitlab.com") }()
 
 				cancel()
 
diff --git a/internal/source/gitlab/cache/entry.go b/internal/source/gitlab/cache/entry.go
index d91bb331..0bf5c3a8 100644
--- a/internal/source/gitlab/cache/entry.go
+++ b/internal/source/gitlab/cache/entry.go
@@ -92,11 +92,11 @@ func (e *Entry) retrieveWithClient(client api.Client) {
 	e.setResponse(retriever.Retrieve(e.domain))
 }
 
-func (e *Entry) setResponse(lookup api.Lookup) {
+func (e *Entry) setResponse(lookup *api.Lookup) {
 	e.mux.Lock()
 	defer e.mux.Unlock()
 
-	e.response = &lookup
+	e.response = lookup
 	close(e.retrieved)
 }
 
diff --git a/internal/source/gitlab/cache/retriever.go b/internal/source/gitlab/cache/retriever.go
index 1284b021..ca5421df 100644
--- a/internal/source/gitlab/cache/retriever.go
+++ b/internal/source/gitlab/cache/retriever.go
@@ -3,9 +3,10 @@ package cache
 import (
 	"context"
 	"errors"
-	"fmt"
 	"time"
 
+	log "github.com/sirupsen/logrus"
+
 	"gitlab.com/gitlab-org/gitlab-pages/internal/source/gitlab/api"
 )
 
@@ -17,28 +18,26 @@ type Retriever struct {
 
 // Retrieve retrieves a lookup response from external source with timeout and
 // backoff. It has its own context with timeout.
-func (r *Retriever) Retrieve(domain string) api.Lookup {
+func (r *Retriever) Retrieve(domain string) (lookup *api.Lookup) {
 	ctx, cancel := context.WithTimeout(context.Background(), retrievalTimeout)
 	defer cancel()
 
-	var lookup api.Lookup
-
 	select {
 	case <-ctx.Done():
-		fmt.Println("retrieval context done") // TODO logme
-		lookup = api.Lookup{Error: errors.New("retrieval context done")}
+		log.Debug("retrieval context done")
+		lookup = &api.Lookup{Error: errors.New("retrieval context done")}
 	case lookup = <-r.resolveWithBackoff(ctx, domain):
-		fmt.Println("retrieval response sent") // TODO logme
+		log.Debug("retrieval response sent")
 	}
 
 	return lookup
 }
 
-func (r *Retriever) resolveWithBackoff(ctx context.Context, domain string) <-chan api.Lookup {
-	response := make(chan api.Lookup)
+func (r *Retriever) resolveWithBackoff(ctx context.Context, domain string) <-chan *api.Lookup {
+	response := make(chan *api.Lookup)
 
 	go func() {
-		var lookup api.Lookup
+		var lookup *api.Lookup
 
 		for i := 1; i <= maxRetrievalRetries; i++ {
 			lookup = r.client.GetLookup(ctx, domain)
diff --git a/internal/source/gitlab/client/client.go b/internal/source/gitlab/client/client.go
index 9bf4bf32..e66ef51c 100644
--- a/internal/source/gitlab/client/client.go
+++ b/internal/source/gitlab/client/client.go
@@ -57,23 +57,23 @@ func NewFromConfig(config Config) (*Client, error) {
 
 // GetLookup returns a VirtualDomain configuration wrap into a Lookup for a
 // given host
-func (gc *Client) GetLookup(ctx context.Context, host string) api.Lookup {
+func (gc *Client) GetLookup(ctx context.Context, host string) *api.Lookup {
 	params := url.Values{}
 	params.Set("host", host)
 
 	resp, err := gc.get(ctx, "/api/v4/internal/pages", params)
 	if err != nil {
-		return api.Lookup{Name: host, Error: err}
+		return &api.Lookup{Name: host, Error: err}
 	}
 
 	if resp == nil {
-		return api.Lookup{Name: host}
+		return &api.Lookup{Name: host}
 	}
 
 	lookup := api.Lookup{Name: host}
 	lookup.Error = json.NewDecoder(resp.Body).Decode(&lookup.Domain)
 
-	return lookup
+	return &lookup
 }
 
 func (gc *Client) get(ctx context.Context, path string, params url.Values) (*http.Response, error) {
diff --git a/internal/source/gitlab/client/client_stub.go b/internal/source/gitlab/client/client_stub.go
index 00a689d8..9c8cf19a 100644
--- a/internal/source/gitlab/client/client_stub.go
+++ b/internal/source/gitlab/client/client_stub.go
@@ -14,17 +14,17 @@ type StubClient struct {
 }
 
 // GetLookup reads a test fixture and unmarshalls it
-func (c StubClient) GetLookup(ctx context.Context, host string) api.Lookup {
+func (c StubClient) GetLookup(ctx context.Context, host string) *api.Lookup {
 	lookup := api.Lookup{Name: host}
 
 	f, err := os.Open(c.File)
 	if err != nil {
 		lookup.Error = err
-		return lookup
+		return &lookup
 	}
 	defer f.Close()
 
 	lookup.Error = json.NewDecoder(f).Decode(&lookup.Domain)
 
-	return lookup
+	return &lookup
 }
diff --git a/internal/source/gitlab/gitlab.go b/internal/source/gitlab/gitlab.go
index 7977e35a..18798b4f 100644
--- a/internal/source/gitlab/gitlab.go
+++ b/internal/source/gitlab/gitlab.go
@@ -19,7 +19,6 @@ import (
 // information about domains from GitLab instance.
 type Gitlab struct {
 	client api.Client
-	cache  *cache.Cache // WIP
 }
 
 // New returns a new instance of gitlab domain source.
@@ -29,7 +28,7 @@ func New(config client.Config) (*Gitlab, error) {
 		return nil, err
 	}
 
-	return &Gitlab{client: client, cache: cache.New()}, nil
+	return &Gitlab{client: cache.NewCache(client)}, nil
 }
 
 // GetDomain return a representation of a domain that we have fetched from
diff --git a/internal/source/gitlab/gitlab_test.go b/internal/source/gitlab/gitlab_test.go
index ad5144f5..d6a81269 100644
--- a/internal/source/gitlab/gitlab_test.go
+++ b/internal/source/gitlab/gitlab_test.go
@@ -6,14 +6,13 @@ import (
 
 	"github.com/stretchr/testify/require"
 
-	"gitlab.com/gitlab-org/gitlab-pages/internal/source/gitlab/cache"
 	"gitlab.com/gitlab-org/gitlab-pages/internal/source/gitlab/client"
 )
 
 func TestGetDomain(t *testing.T) {
 	t.Run("when the response if correct", func(t *testing.T) {
 		client := client.StubClient{File: "client/testdata/test.gitlab.io.json"}
-		source := Gitlab{client: client, cache: cache.New()}
+		source := Gitlab{client: client}
 
 		domain, err := source.GetDomain("test.gitlab.io")
 		require.NoError(t, err)
@@ -23,7 +22,7 @@ func TestGetDomain(t *testing.T) {
 
 	t.Run("when the response is not valid", func(t *testing.T) {
 		client := client.StubClient{File: "/dev/null"}
-		source := Gitlab{client: client, cache: cache.New()}
+		source := Gitlab{client: client}
 
 		domain, err := source.GetDomain("test.gitlab.io")
 
@@ -34,7 +33,7 @@ func TestGetDomain(t *testing.T) {
 
 func TestResolve(t *testing.T) {
 	client := client.StubClient{File: "client/testdata/test.gitlab.io.json"}
-	source := Gitlab{client: client, cache: cache.New()}
+	source := Gitlab{client: client}
 
 	t.Run("when requesting a nested group project", func(t *testing.T) {
 		target := "https://test.gitlab.io:443/my/pages/project/path/index.html"