fix(uhttp): opt-in cache-key headers via DoWithCacheKeyHeaders

pkg/uhttp/client.go

@@ -104,17 +104,48 @@ func NewClient(ctx context.Context, options ...Option) (*http.Client, error) {
 }
 
 type icache interface {
-	Get(req *http.Request) (*http.Response, error)
-	Set(req *http.Request, value *http.Response) error
+	Get(req *http.Request, opts ...CacheOption) (*http.Response, error)
+	Set(req *http.Request, value *http.Response, opts ...CacheOption) error
 	Clear(ctx context.Context) error
 	Stats(ctx context.Context) CacheStats
 }
 
+type cacheKeyConfig struct {
+	headers []string
+}
+
+// CacheOption configures how CreateCacheKey computes its key, beyond the
+// default set of headers (Accept, Content-Type, Cookie, Range). Kept as an
+// interface so future dimensions (TTL, query-param keying, etc.) can be
+// added without changing CreateCacheKey's or icache's signatures again.
+type CacheOption interface {
+	applyCache(*cacheKeyConfig)
+}
+
+type cacheKeyHeadersOption []string
+
+func (o cacheKeyHeadersOption) applyCache(c *cacheKeyConfig) {
+	c.headers = append(c.headers, o...)
+}
+
+// WithCacheKeyHeaders returns a CacheOption that folds the named headers
+// into the HTTP response cache key, on top of the default set (Accept,
+// Content-Type, Cookie, Range). The value folded in for each is read from
+// req.Header at CreateCacheKey time, same as the default set.
+func WithCacheKeyHeaders(headers ...string) CacheOption {
+	return cacheKeyHeadersOption(headers)
+}
+
 // CreateCacheKey generates a cache key based on the request URL, query parameters, and headers.
-func CreateCacheKey(req *http.Request) (string, error) {
+func CreateCacheKey(req *http.Request, opts ...CacheOption) (string, error) {
 	if req == nil {
 		return "", fmt.Errorf("request is nil")
 	}
+	var cfg cacheKeyConfig
+	for _, o := range opts {
+		o.applyCache(&cfg)
+	}
+
 	var sortedParams []string
 	// Normalize the URL path
 	path := strings.ToLower(req.URL.Path)
@@ -137,6 +168,12 @@ func CreateCacheKey(req *http.Request) (string, error) {
 			}
 		}
 	}
+	for _, h := range cfg.headers {
+		key := http.CanonicalHeaderKey(h)
+		for _, value := range req.Header[key] {
+			headerParts = append(headerParts, fmt.Sprintf("%s=%s", key, value))
+		}
+	}
 
 	sort.Strings(headerParts)
 	headersString := strings.Join(headerParts, "&")

pkg/uhttp/client_test.go

@@ -0,0 +1,120 @@
+package uhttp
+
+import (
+	"context"
+	"net/http"
+	"testing"
+
+	"github.com/stretchr/testify/require"
+)
+
+func newCacheKeyRequest(t *testing.T, headerKey, headerValue string) *http.Request {
+	t.Helper()
+	req, err := http.NewRequestWithContext(context.Background(), http.MethodGet, "https://example.com/widgets?id=1", nil)
+	require.NoError(t, err)
+	if headerKey != "" {
+		req.Header.Set(headerKey, headerValue)
+	}
+	return req
+}
+
+func TestCreateCacheKey_NilRequest(t *testing.T) {
+	_, err := CreateCacheKey(nil)
+	require.Error(t, err)
+}
+
+func TestCreateCacheKey_IdenticalRequestsMatch(t *testing.T) {
+	req1 := newCacheKeyRequest(t, "Accept", "application/json")
+	req2 := newCacheKeyRequest(t, "Accept", "application/json")
+
+	key1, err := CreateCacheKey(req1)
+	require.NoError(t, err)
+	key2, err := CreateCacheKey(req2)
+	require.NoError(t, err)
+	require.Equal(t, key1, key2)
+}
+
+// TestCreateCacheKey_HeadersOutsideDefaultSetAreIgnoredByDefault documents
+// current, intentional behavior: only the default set affects the key
+// unless a caller opts in via extraCacheKeyHeaders. Folding in every header
+// unconditionally would key the cache on values that have nothing to do
+// with the response (transport-injected headers, tracing IDs, etc.) and
+// silently tank the hit rate for every caller who never asked for that.
+func TestCreateCacheKey_HeadersOutsideDefaultSetAreIgnoredByDefault(t *testing.T) {
+	headers := []string{"Authorization", "X-Api-Version", "X-Tenant-Id", "User-Agent"}
+	for _, header := range headers {
+		t.Run(header, func(t *testing.T) {
+			reqA := newCacheKeyRequest(t, header, "value-a")
+			reqB := newCacheKeyRequest(t, header, "value-b")
+
+			keyA, err := CreateCacheKey(reqA)
+			require.NoError(t, err)
+			keyB, err := CreateCacheKey(reqB)
+			require.NoError(t, err)
+			require.Equal(t, keyA, keyB, "%s is not in the default set and must not affect the key", header)
+		})
+	}
+}
+
+func TestCreateCacheKey_DefaultHeadersStillChangeKey(t *testing.T) {
+	headers := []string{"Accept", "Content-Type", "Cookie", "Range"}
+	for _, header := range headers {
+		t.Run(header, func(t *testing.T) {
+			reqA := newCacheKeyRequest(t, header, "value-a")
+			reqB := newCacheKeyRequest(t, header, "value-b")
+
+			keyA, err := CreateCacheKey(reqA)
+			require.NoError(t, err)
+			keyB, err := CreateCacheKey(reqB)
+			require.NoError(t, err)
+			require.NotEqual(t, keyA, keyB)
+		})
+	}
+}
+
+// TestCreateCacheKey_WithCacheKeyHeadersOptsInAdditionalHeaders is the
+// regression test for CE-1056: a caller that knows a header varies the
+// response (e.g. Authorization scoping the result set) can now opt that
+// header into the key instead of two requests silently colliding. The value
+// folded into the key is read from req.Header, same as the default set.
+func TestCreateCacheKey_WithCacheKeyHeadersOptsInAdditionalHeaders(t *testing.T) {
+	reqA := newCacheKeyRequest(t, "Authorization", "value-a")
+	reqB := newCacheKeyRequest(t, "Authorization", "value-b")
+
+	keyA, err := CreateCacheKey(reqA, WithCacheKeyHeaders("Authorization"))
+	require.NoError(t, err)
+	keyB, err := CreateCacheKey(reqB, WithCacheKeyHeaders("Authorization"))
+	require.NoError(t, err)
+	require.NotEqual(t, keyA, keyB)
+}
+
+// TestCreateCacheKey_WithCacheKeyHeadersOnlyAffectsNamedHeaders confirms
+// opting a header in doesn't widen the key to every header on the request --
+// a header present on req.Header but absent from the CacheOption still falls
+// back to the default-set rule.
+func TestCreateCacheKey_WithCacheKeyHeadersOnlyAffectsNamedHeaders(t *testing.T) {
+	reqA := newCacheKeyRequest(t, "X-Tenant-Id", "tenant-a")
+	reqA.Header.Set("Authorization", "same-token")
+	reqB := newCacheKeyRequest(t, "X-Tenant-Id", "tenant-b")
+	reqB.Header.Set("Authorization", "same-token")
+
+	extra := WithCacheKeyHeaders("Authorization")
+	keyA, err := CreateCacheKey(reqA, extra)
+	require.NoError(t, err)
+	keyB, err := CreateCacheKey(reqB, extra)
+	require.NoError(t, err)
+	require.Equal(t, keyA, keyB, "X-Tenant-Id was never opted in, so it must not affect the key")
+}
+
+// TestCreateCacheKey_WithCacheKeyHeadersCanonicalizesNames confirms header
+// names passed to WithCacheKeyHeaders are treated the same regardless of
+// casing.
+func TestCreateCacheKey_WithCacheKeyHeadersCanonicalizesNames(t *testing.T) {
+	req := newCacheKeyRequest(t, "Authorization", "value-a")
+
+	keyA, err := CreateCacheKey(req, WithCacheKeyHeaders("authorization"))
+	require.NoError(t, err)
+	keyB, err := CreateCacheKey(req, WithCacheKeyHeaders("Authorization"))
+	require.NoError(t, err)
+	require.Equal(t, keyA, keyB)
+}

pkg/uhttp/dbcache.go

@@ -190,12 +190,12 @@ func (d *DBCache) removeDB(ctx context.Context) error {
 }
 
 // Get returns cached response (if exists).
-func (d *DBCache) Get(req *http.Request) (*http.Response, error) {
+func (d *DBCache) Get(req *http.Request, opts ...CacheOption) (*http.Response, error) {
 	var (
 		isFound = false
 		resp    *http.Response
 	)
-	key, err := CreateCacheKey(req)
+	key, err := CreateCacheKey(req, opts...)
 	if err != nil {
 		return nil, err
 	}
@@ -250,8 +250,8 @@ func (d *DBCache) pick(ctx context.Context, key string) ([]byte, error) {
 }
 
 // Set stores and save response in the db.
-func (d *DBCache) Set(req *http.Request, value *http.Response) error {
-	key, err := CreateCacheKey(req)
+func (d *DBCache) Set(req *http.Request, value *http.Response, opts ...CacheOption) error {
+	key, err := CreateCacheKey(req, opts...)
 	if err != nil {
 		return err
 	}

pkg/uhttp/gocache.go

@@ -58,13 +58,13 @@ func NewNoopCache(ctx context.Context) *NoopCache {
 	return &NoopCache{}
 }
 
-func (g *NoopCache) Get(req *http.Request) (*http.Response, error) {
+func (g *NoopCache) Get(req *http.Request, opts ...CacheOption) (*http.Response, error) {
 	// This isn't threadsafe but who cares? It's the noop cache.
 	g.counter++
 	return nil, nil
 }
 
-func (n *NoopCache) Set(req *http.Request, value *http.Response) error {
+func (n *NoopCache) Set(req *http.Request, value *http.Response, opts ...CacheOption) error {
 	return nil
 }
 
@@ -219,12 +219,12 @@ func (g *GoCache) Stats(ctx context.Context) CacheStats {
 	}
 }
 
-func (g *GoCache) Get(req *http.Request) (*http.Response, error) {
+func (g *GoCache) Get(req *http.Request, opts ...CacheOption) (*http.Response, error) {
 	if g.rootLibrary == nil {
 		return nil, nil
 	}
 
-	key, err := CreateCacheKey(req)
+	key, err := CreateCacheKey(req, opts...)
 	if err != nil {
 		return nil, err
 	}
@@ -247,12 +247,12 @@ func (g *GoCache) Get(req *http.Request) (*http.Response, error) {
 	return resp, nil
 }
 
-func (g *GoCache) Set(req *http.Request, value *http.Response) error {
+func (g *GoCache) Set(req *http.Request, value *http.Response, opts ...CacheOption) error {
 	if g.rootLibrary == nil {
 		return nil
 	}
 
-	key, err := CreateCacheKey(req)
+	key, err := CreateCacheKey(req, opts...)
 	if err != nil {
 		return err
 	}

pkg/uhttp/wrapper.go

@@ -436,6 +436,22 @@ func (c *BaseHttpClient) recordCacheMiss(ctx context.Context) {
 }
 
 func (c *BaseHttpClient) Do(req *http.Request, options ...DoOption) (*http.Response, error) {
+	return c.do(req, nil, options...)
+}
+
+// DoWithCacheKeyHeaders is a sibling to Do that additionally folds the named
+// headers into the HTTP response cache key for this call, on top of the
+// default set (Accept, Content-Type, Cookie, Range). Use this when a request
+// varies by a header the cache wouldn't otherwise key on -- e.g. a per-call
+// Authorization token -- so requests that only differ in that header don't
+// collide. Each header's value is read from req.Header, same as the default
+// set. Do's own signature is untouched -- zero compatibility risk for
+// existing callers.
+func (c *BaseHttpClient) DoWithCacheKeyHeaders(req *http.Request, cacheKeyHeaders []string, options ...DoOption) (*http.Response, error) {
+	return c.do(req, []CacheOption{WithCacheKeyHeaders(cacheKeyHeaders...)}, options...)
+}
+
+func (c *BaseHttpClient) do(req *http.Request, cacheOpts []CacheOption, options ...DoOption) (*http.Response, error) {
 	var (
 		err  error
 		resp *http.Response
@@ -448,7 +464,7 @@ func (c *BaseHttpClient) Do(req *http.Request, options ...DoOption) (*http.Respo
 	}
 
 	if req.Method == http.MethodGet && req.Header.Get("Cache-Control") != "no-cache" {
-		resp, err = c.baseHttpCache.Get(req)
+		resp, err = c.baseHttpCache.Get(req, cacheOpts...)
 		if err != nil {
 			return nil, err
 		}
@@ -520,7 +536,7 @@ func (c *BaseHttpClient) Do(req *http.Request, options ...DoOption) (*http.Respo
 	}
 
 	if req.Method == http.MethodGet && resp.StatusCode == http.StatusOK {
-		cacheErr := c.baseHttpCache.Set(req, resp)
+		cacheErr := c.baseHttpCache.Set(req, resp, cacheOpts...)
 		if cacheErr != nil {
 			l.Warn("error setting cache", zap.String("url", req.URL.String()), zap.Error(cacheErr))
 		}

pkg/uhttp/wrapper_test.go

@@ -8,7 +8,9 @@ import (
 	"fmt"
 	"io"
 	"net/http"
+	"net/http/httptest"
 	"net/url"
+	"sync/atomic"
 	"testing"
 	"time"
 
@@ -727,3 +729,44 @@ func TestWrapper_RedactSensitiveHeaders(t *testing.T) {
 		"Custom-Api-Key":      {"REDACTED"},
 	}, redactedHeaders)
 }
+
+func newCountingServer(hits *int32) *httptest.Server {
+	return httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		atomic.AddInt32(hits, 1)
+		w.WriteHeader(http.StatusOK)
+		_, _ = w.Write([]byte(`{}`))
+	}))
+}
+
+// TestWrapper_DoWithCacheKeyHeaders_DistinguishesRequests is the regression
+// test for CE-1056: without opting Authorization into the cache key via
+// DoWithCacheKeyHeaders, two GET requests that only differ in that header
+// would collide. With it named, they don't -- both reach the server -- while
+// two calls sharing the same Authorization value still hit the cache on the
+// second call.
+func TestWrapper_DoWithCacheKeyHeaders_DistinguishesRequests(t *testing.T) {
+	var hits int32
+	ts := newCountingServer(&hits)
+	defer ts.Close()
+
+	client, err := NewBaseHttpClientWithContext(ctx, http.DefaultClient)
+	require.NoError(t, err)
+
+	u, err := url.Parse(ts.URL)
+	require.NoError(t, err)
+
+	doWithAuth := func(token string) {
+		req, err := client.NewRequest(ctx, http.MethodGet, u, WithBearerToken(token))
+		require.NoError(t, err)
+		resp, err := client.DoWithCacheKeyHeaders(req, []string{"Authorization"})
+		require.NoError(t, err)
+		resp.Body.Close()
+	}
+
+	doWithAuth("token-a")
+	doWithAuth("token-b")
+	require.EqualValues(t, 2, atomic.LoadInt32(&hits), "different Authorization values must not collide in the cache")
+
+	doWithAuth("token-a")
+	require.EqualValues(t, 2, atomic.LoadInt32(&hits), "repeating the same Authorization value should be served from cache")
+}