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 <[email protected]>
TryBot-Result: Gopher Robot <[email protected]>
Reviewed-by: Jamal Carvalho <[email protected]>
Run-TryBot: Michael Matloob <[email protected]>

Author: matloob

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),

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 {

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

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

internal/middleware/caching.go

@@ -98,26 +98,36 @@ 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.
 // 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.
-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,
 		}

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)

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()