diff --git a/internal/responsestorerer.go b/internal/responsestorerer.go index eb92b1d..d442a01 100644 --- a/internal/responsestorerer.go +++ b/internal/responsestorerer.go @@ -18,6 +18,7 @@ import ( "maps" "net/http" "slices" + "strings" "time" ) @@ -59,7 +60,8 @@ func (r *responseStorer) StoreResponse( // Remove hop-by-hop headers as per RFC 9111 §3.1 removeHopByHopHeaders(resp) - vary := resp.Header.Get("Vary") + // Join multiple Vary headers into a single string + vary := strings.Join(resp.Header.Values("Vary"), ",") varyResolved := maps.Collect( r.vhn.NormalizeVaryHeader(vary, req.Header), ) diff --git a/roundtripper_test.go b/roundtripper_test.go index 1756dff..7543431 100644 --- a/roundtripper_test.go +++ b/roundtripper_test.go @@ -16,6 +16,7 @@ package httpcache import ( "errors" + "fmt" "io" "log/slog" "net/http" @@ -957,3 +958,90 @@ func Test_transport_RevalidationUpdatesCache(t *testing.T) { }) } } + +// Test_transport_Vary_MultipleHeaders verifies that the transport correctly +// handles multiple Vary headers that are sent as separate header lines +// (instead of a single comma-separated line). This is a common scenario in +// practice, and there was a bug where only the first Vary header line was +// being processed, causing incorrect cache hits when subsequent Vary headers +// were not considered. +// Regression test for: https://github.com/bartventer/httpcache/issues/32 +func Test_transport_Vary_MultipleHeaders(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + // Simulate server returning multiple separate Vary header lines + // (not comma-separated, but as distinct headers) + w.Header().Add("Vary", "Accept-Language") + w.Header().Add("Vary", "X-Compatibility-Date") + w.Header().Set("Cache-Control", "max-age=60") + w.WriteHeader(http.StatusOK) + + lang := r.Header.Get("Accept-Language") + date := r.Header.Get("X-Compatibility-Date") + _, _ = fmt.Fprintf(w, "lang=%s date=%s", lang, date) + })) + defer server.Close() + + c := memcache.Open() + tr := newTransport(c, WithLogger(slog.New(slog.NewJSONHandler(os.Stderr, &slog.HandlerOptions{ + Level: slog.LevelDebug, + })))) + + makeReq := func(lang, date string) *http.Response { + req, _ := http.NewRequest(http.MethodGet, server.URL, nil) + req.Header.Set("Accept-Language", lang) + req.Header.Set("X-Compatibility-Date", date) + resp, err := tr.RoundTrip(req) + testutil.RequireNoError(t, err) + return resp + } + + // Request 1: MISS — first request for en-us + 2025-01-01 + resp := makeReq("en-us", "2025-01-01") + testutil.AssertEqual(t, http.StatusOK, resp.StatusCode) + testutil.AssertEqual( + t, + internal.CacheStatusMiss.Value, + resp.Header.Get(internal.CacheStatusHeader), + ) + body, _ := io.ReadAll(resp.Body) + _ = resp.Body.Close() + testutil.AssertEqual(t, "lang=en-us date=2025-01-01", string(body)) + + // Request 2: HIT — same headers + resp = makeReq("en-us", "2025-01-01") + testutil.AssertEqual(t, http.StatusOK, resp.StatusCode) + testutil.AssertEqual( + t, + internal.CacheStatusHit.Value, + resp.Header.Get(internal.CacheStatusHeader), + ) + body, _ = io.ReadAll(resp.Body) + _ = resp.Body.Close() + testutil.AssertEqual(t, "lang=en-us date=2025-01-01", string(body)) + + // Request 3: MISS — X-Compatibility-Date changed, must be a different cache entry + // This is the bug: without the fix, this incorrectly returns a HIT + resp = makeReq("en-us", "2026-01-01") + testutil.AssertEqual(t, http.StatusOK, resp.StatusCode) + testutil.AssertEqual( + t, + internal.CacheStatusMiss.Value, + resp.Header.Get(internal.CacheStatusHeader), + "X-Compatibility-Date is a Vary header; different value must produce a cache MISS", + ) + body, _ = io.ReadAll(resp.Body) + _ = resp.Body.Close() + testutil.AssertEqual(t, "lang=en-us date=2026-01-01", string(body)) + + // Request 4: HIT — same as request 3 + resp = makeReq("en-us", "2026-01-01") + testutil.AssertEqual(t, http.StatusOK, resp.StatusCode) + testutil.AssertEqual( + t, + internal.CacheStatusHit.Value, + resp.Header.Get(internal.CacheStatusHeader), + ) + body, _ = io.ReadAll(resp.Body) + _ = resp.Body.Close() + testutil.AssertEqual(t, "lang=en-us date=2026-01-01", string(body)) +} diff --git a/store/expapi/expapi.go b/store/expapi/expapi.go index 2a3d542..cc4cfb7 100644 --- a/store/expapi/expapi.go +++ b/store/expapi/expapi.go @@ -98,7 +98,11 @@ func retrieve(conn driver.Conn) http.Handler { if errors.Is(err, driver.ErrNotExist) { http.Error(w, fmt.Sprintf("key %q not found", key), http.StatusNotFound) } else { - http.Error(w, fmt.Sprintf("failed to get value for key %q: %v", key, err), http.StatusInternalServerError) + http.Error( + w, + fmt.Sprintf("failed to get value for key %q: %v", key, err), + http.StatusInternalServerError, + ) } return } @@ -121,7 +125,11 @@ func destroy(conn driver.Conn) http.Handler { if errors.Is(err, driver.ErrNotExist) { http.Error(w, fmt.Sprintf("key %q not found", key), http.StatusNotFound) } else { - http.Error(w, fmt.Sprintf("failed to delete value for key %q: %v", key, err), http.StatusInternalServerError) + http.Error( + w, + fmt.Sprintf("failed to delete value for key %q: %v", key, err), + http.StatusInternalServerError, + ) } return }