internal/frontend: add an interface for creating request caches

This change adds a new Cacher interface that is used to create
middlewares for caching requests. This abstracts away the use of redis
so that the frontend doesn't depend on redis. The tests still depend
on redis for the 404 page testing logic, but the 404 page logic will
be moved out into a different package so those tests will go too.

The Expirer and Middleware interfaces are not present on the Cache
function so that the interface can be defined in package
internal/frontend without needing the dependency on the Middleware
package.

For golang/go#61399

Change-Id: I6518b2ed1d772cb4deda3308c4190f0f1b8a35a0
Reviewed-on: https://go-review.googlesource.com/c/pkgsite/+/514518
kokoro-CI: kokoro <noreply+kokoro@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Jamal Carvalho <jamal@golang.org>
Run-TryBot: Michael Matloob <matloob@golang.org>

7 files changed, 49 insertions, 22 deletions

diff --git a/cmd/frontend/main.go b/cmd/frontend/main.go
index 3812db7b..d7c3f19a 100644
--- a/cmd/frontend/main.go
+++ b/cmd/frontend/main.go
@@ -132,17 +132,19 @@ func main() {
 	}
 
 	router := dcensus.NewRouter(frontend.TagRoute)
-	var cacheClient *redis.Client
+	var redisClient *redis.Client
+	var cacher frontend.Cacher
 	if cfg.RedisCacheHost != "" {
 		addr := cfg.RedisCacheHost + ":" + cfg.RedisCachePort
-		cacheClient = redis.NewClient(&redis.Options{Addr: addr})
-		if err := cacheClient.Ping(ctx).Err(); err != nil {
+		redisClient := redis.NewClient(&redis.Options{Addr: addr})
+		if err := redisClient.Ping(ctx).Err(); err != nil {
 			log.Errorf(ctx, "redis at %s: %v", addr, err)
 		} else {
 			log.Infof(ctx, "connected to redis at %s", addr)
 		}
+		cacher = middleware.NewCacher(redisClient)
 	}
-	server.Install(router.Handle, cacheClient, cfg.AuthValues)
+	server.Install(router.Handle, cacher, cfg.AuthValues)
 	views := append(dcensus.ServerViews,
 		postgres.SearchLatencyDistribution,
 		postgres.SearchResponseCount,
@@ -184,7 +186,7 @@ func main() {
 		middleware.AcceptRequests(http.MethodGet, http.MethodPost, http.MethodHead), // accept only GETs, POSTs and HEADs
 		middleware.BetaPkgGoDevRedirect(),
 		middleware.GodocOrgRedirect(),
-		middleware.Quota(cfg.Quota, cacheClient),
+		middleware.Quota(cfg.Quota, redisClient),
 		middleware.SecureHeaders(!*disableCSP), // must come before any caching for nonces to work
 		middleware.Experiment(experimenter),
 		middleware.Panic(panicHandler),
diff --git a/internal/frontend/frontend_test.go b/internal/frontend/frontend_test.go
index 8b51c3e1..ff35894f 100644
--- a/internal/frontend/frontend_test.go
+++ b/internal/frontend/frontend_test.go
@@ -10,7 +10,6 @@ import (
 	"testing"
 	"time"
 
-	"github.com/go-redis/redis/v8"
 	"github.com/google/safehtml/template"
 	"golang.org/x/pkgsite/internal"
 	"golang.org/x/pkgsite/internal/middleware"
@@ -46,7 +45,7 @@ type testPackage struct {
 	docs           []*internal.Documentation
 }
 
-func newTestServer(t *testing.T, proxyModules []*proxytest.Module, redisClient *redis.Client, experimentNames ...string) (*Server, http.Handler, func()) {
+func newTestServer(t *testing.T, proxyModules []*proxytest.Module, cacher Cacher, experimentNames ...string) (*Server, http.Handler, func()) {
 	t.Helper()
 	proxyClient, teardown := proxytest.SetupTestClient(t, proxyModules)
 	sourceClient := source.NewClient(sourceTimeout)
@@ -72,7 +71,7 @@ func newTestServer(t *testing.T, proxyModules []*proxytest.Module, redisClient *
 		t.Fatal(err)
 	}
 	mux := http.NewServeMux()
-	s.Install(mux.Handle, redisClient, nil)
+	s.Install(mux.Handle, cacher, nil)
 
 	var exps []*internal.Experiment
 	for _, n := range experimentNames {
diff --git a/internal/frontend/server.go b/internal/frontend/server.go
index a22ac999..4ec4ca50 100644
--- a/internal/frontend/server.go
+++ b/internal/frontend/server.go
@@ -22,7 +22,6 @@ import (
 	"time"
 
 	"cloud.google.com/go/errorreporting"
-	"github.com/go-redis/redis/v8"
 	"github.com/google/safehtml"
 	"github.com/google/safehtml/template"
 	"golang.org/x/pkgsite/internal"
@@ -127,24 +126,36 @@ func NewServer(scfg ServerConfig) (_ *Server, err error) {
 	return s, nil
 }
 
+// A Cacher is used to create request caches for http handlers.
+type Cacher interface {
+	// Cache returns a new middleware that caches every request.
+	// The name of the cache is used only for metrics.
+	// The expirer is a func that is used to map a new request to its TTL.
+	// authHeader is the header key used by the cache to know that a
+	// request should bypass the cache.
+	// authValues is the set of values that could be set on the authHeader in
+	// order to bypass the cache.
+	Cache(name string, expirer func(r *http.Request) time.Duration, authValues []string) func(http.Handler) http.Handler
+}
+
 // Install registers server routes using the given handler registration func.
 // authValues is the set of values that can be set on authHeader to bypass the
 // cache.
-func (s *Server) Install(handle func(string, http.Handler), redisClient *redis.Client, authValues []string) {
+func (s *Server) Install(handle func(string, http.Handler), cacher Cacher, authValues []string) {
 	var (
 		detailHandler http.Handler = s.errorHandler(s.serveDetails)
 		fetchHandler  http.Handler = s.errorHandler(s.serveFetch)
 		searchHandler http.Handler = s.errorHandler(s.serveSearch)
 		vulnHandler   http.Handler = s.errorHandler(s.serveVuln)
 	)
-	if redisClient != nil {
+	if cacher != nil {
 		// The cache middleware uses the URL string as the key for content served
 		// by the handlers it wraps. Be careful not to wrap the handler it returns
 		// with a handler that rewrites the URL in a way that could cause key
 		// collisions, like http.StripPrefix.
-		detailHandler = middleware.Cache("details", redisClient, detailsTTL, authValues)(detailHandler)
-		searchHandler = middleware.Cache("search", redisClient, searchTTL, authValues)(searchHandler)
-		vulnHandler = middleware.Cache("vuln", redisClient, vulnTTL, authValues)(vulnHandler)
+		detailHandler = cacher.Cache("details", detailsTTL, authValues)(detailHandler)
+		searchHandler = cacher.Cache("search", searchTTL, authValues)(searchHandler)
+		vulnHandler = cacher.Cache("vuln", vulnTTL, authValues)(vulnHandler)
 	}
 	// Each AppEngine instance is created in response to a start request, which
 	// is an empty HTTP GET request to /_ah/start when scaling is set to manual
diff --git a/internal/frontend/server_test.go b/internal/frontend/server_test.go
index 1d9e7bec..0bd94a16 100644
--- a/internal/frontend/server_test.go
+++ b/internal/frontend/server_test.go
@@ -31,6 +31,7 @@ import (
 	"golang.org/x/pkgsite/internal/cookie"
 	"golang.org/x/pkgsite/internal/derrors"
 	"golang.org/x/pkgsite/internal/experiment"
+	"golang.org/x/pkgsite/internal/middleware"
 	"golang.org/x/pkgsite/internal/postgres"
 	"golang.org/x/pkgsite/internal/testing/htmlcheck"
 	"golang.org/x/pkgsite/internal/testing/pagecheck"
@@ -1234,7 +1235,7 @@ func TestServer404Redirect_NoLoop(t *testing.T) {
 	}
 	defer rs.Close()
 
-	_, handler, _ := newTestServer(t, nil, redis.NewClient(&redis.Options{Addr: rs.Addr()}))
+	_, handler, _ := newTestServer(t, nil, middleware.NewCacher(redis.NewClient(&redis.Options{Addr: rs.Addr()})))
 
 	for _, test := range []struct {
 		name, path string
@@ -1311,7 +1312,7 @@ func TestServer404Redirect(t *testing.T) {
 	}
 	defer rs.Close()
 
-	_, handler, _ := newTestServer(t, nil, redis.NewClient(&redis.Options{Addr: rs.Addr()}))
+	_, handler, _ := newTestServer(t, nil, middleware.NewCacher(redis.NewClient(&redis.Options{Addr: rs.Addr()})))
 
 	for _, test := range []struct {
 		name, path, flash string
diff --git a/internal/middleware/caching.go b/internal/middleware/caching.go
index 4ef5e875..32cb0a87 100644
--- a/internal/middleware/caching.go
+++ b/internal/middleware/caching.go
@@ -98,13 +98,23 @@ type cache struct {
 // An Expirer computes the TTL that should be used when caching a page.
 type Expirer func(r *http.Request) time.Duration
 
-// TTL returns an Expirer that expires all pages after the given TTL.
-func TTL(ttl time.Duration) Expirer {
+// ttl returns an Expirer that expires all pages after the given TTL.
+func ttl(ttl time.Duration) Expirer {
 	return func(r *http.Request) time.Duration {
 		return ttl
 	}
 }
 
+// NewCacher returns a new Cacher, used for creating a middleware
+// that caches each request.
+func NewCacher(client *redis.Client) *cacher {
+	return &cacher{client: client}
+}
+
+type cacher struct {
+	client *redis.Client
+}
+
 // Cache returns a new Middleware that caches every request.
 // The name of the cache is used only for metrics.
 // The expirer is a func that is used to map a new request to its TTL.
@@ -112,12 +122,12 @@ func TTL(ttl time.Duration) Expirer {
 // request should bypass the cache.
 // authValues is the set of values that could be set on the authHeader in
 // order to bypass the cache.
-func Cache(name string, client *redis.Client, expirer Expirer, authValues []string) Middleware {
+func (c *cacher) Cache(name string, expirer func(r *http.Request) time.Duration, authValues []string) func(http.Handler) http.Handler {
 	return func(h http.Handler) http.Handler {
 		return &cache{
 			name:       name,
 			authValues: authValues,
-			cache:      icache.New(client),
+			cache:      icache.New(c.client),
 			delegate:   h,
 			expirer:    expirer,
 		}
diff --git a/internal/middleware/caching_test.go b/internal/middleware/caching_test.go
index 2f46b93e..7b8dd2ee 100644
--- a/internal/middleware/caching_test.go
+++ b/internal/middleware/caching_test.go
@@ -49,7 +49,7 @@ func TestCache(t *testing.T) {
 
 	c := redis.NewClient(&redis.Options{Addr: s.Addr()})
 	mux := http.NewServeMux()
-	mux.Handle("/A", Cache("A", c, TTL(1*time.Minute), []string{"yes"})(handler))
+	mux.Handle("/A", NewCacher(c).Cache("A", ttl(1*time.Minute), []string{"yes"})(handler))
 	mux.Handle("/B", handler)
 	ts := httptest.NewServer(mux)
 	view.Register(CacheResultCount)
diff --git a/internal/testing/integration/frontend_test.go b/internal/testing/integration/frontend_test.go
index f3cc6a4c..ceaea33f 100644
--- a/internal/testing/integration/frontend_test.go
+++ b/internal/testing/integration/frontend_test.go
@@ -46,7 +46,11 @@ func setupFrontend(ctx context.Context, t *testing.T, q queue.Queue, rc *redis.C
 		t.Fatal(err)
 	}
 	mux := http.NewServeMux()
-	s.Install(mux.Handle, rc, nil)
+	var cacher frontend.Cacher
+	if rc != nil {
+		cacher = middleware.NewCacher(rc)
+	}
+	s.Install(mux.Handle, cacher, nil)
 
 	// Get experiments from the context. Fully roll them out.
 	expNames := experiment.FromContext(ctx).Active()