diff --git a/internal/httputil/conditional.go b/internal/httputil/conditional.go new file mode 100644 index 00000000..81947bc7 --- /dev/null +++ b/internal/httputil/conditional.go @@ -0,0 +1,74 @@ +package httputil + +import ( + "io" + "maps" + "net/http" + "strings" + + "github.com/alecthomas/errors" +) + +// ETagHeader is the HTTP header used to carry an object's entity tag. +const ETagHeader = "ETag" + +// CheckConditionals evaluates RFC 7232 If-Match and If-None-Match precondition +// headers against the stored ETag. It returns 0 when all preconditions pass, +// otherwise the HTTP status code the caller should send: 412 Precondition +// Failed for a failed If-Match, or 304 Not Modified for a satisfied +// If-None-Match. +func CheckConditionals(r *http.Request, etag string) int { + if ifMatch := r.Header.Get("If-Match"); ifMatch != "" { + if etag == "" || !etagListMatches(ifMatch, etag) { + return http.StatusPreconditionFailed + } + } + if ifNoneMatch := r.Header.Get("If-None-Match"); ifNoneMatch != "" { + if etag != "" && etagListMatches(ifNoneMatch, etag) { + return http.StatusNotModified + } + } + return 0 +} + +// etagListMatches reports whether etag matches an If-Match / If-None-Match +// header value, which may be a comma-separated list of ETags or the "*" +// wildcard. Stored ETags are always strong, so weak comparison is not required. +func etagListMatches(headerValue, etag string) bool { + for candidate := range strings.SplitSeq(headerValue, ",") { + candidate = strings.TrimSpace(candidate) + if candidate == "*" || candidate == etag { + return true + } + } + return false +} + +// ServeCacheHit serves a cache hit over HTTP. It copies the stored headers onto +// the response, evaluates conditional request preconditions against the stored +// ETag, and either short-circuits with a 304/412 status or streams the body. +// The body is always closed. +// +// This consolidates the validator-aware serving path shared by handlers that +// return a single cached object (e.g. the API and the generic caching handler). +func ServeCacheHit(w http.ResponseWriter, r *http.Request, headers http.Header, body io.ReadCloser) error { + maps.Copy(w.Header(), headers) + if status := CheckConditionals(r, headers.Get(ETagHeader)); status != 0 { + w.WriteHeader(status) + return errors.WithStack(body.Close()) + } + _, copyErr := io.Copy(w, body) + return errors.Wrap(errors.Join(copyErr, body.Close()), "serve cache hit") +} + +// ServeCacheStat answers a metadata-only (HEAD) request from stored headers. It +// copies the headers onto the response and writes the status determined by the +// conditional request preconditions, defaulting to 200 OK. +func ServeCacheStat(w http.ResponseWriter, r *http.Request, headers http.Header) { + maps.Copy(w.Header(), headers) + status := CheckConditionals(r, headers.Get(ETagHeader)) + if status == 0 { + status = http.StatusOK + } + w.WriteHeader(status) +} diff --git a/internal/httputil/conditional_test.go b/internal/httputil/conditional_test.go new file mode 100644 index 00000000..912c6253 --- /dev/null +++ b/internal/httputil/conditional_test.go @@ -0,0 +1,151 @@ +package httputil_test + +import ( + "io" + "net/http" + "net/http/httptest" + "strings" + "testing" + + "github.com/alecthomas/assert/v2" + + "github.com/block/cachew/internal/httputil" +) + +const testETag = `"abc123"` + +func cacheHeaders(extra ...[2]string) http.Header { + h := http.Header{} + h.Set(httputil.ETagHeader, testETag) + for _, kv := range extra { + h.Set(kv[0], kv[1]) + } + return h +} + +func newRequest(t *testing.T, ifMatch, ifNoneMatch string) *http.Request { + t.Helper() + r := httptest.NewRequest(http.MethodGet, "/", nil) + if ifMatch != "" { + r.Header.Set("If-Match", ifMatch) + } + if ifNoneMatch != "" { + r.Header.Set("If-None-Match", ifNoneMatch) + } + return r +} + +func TestCheckConditionals(t *testing.T) { + tests := []struct { + name string + ifMatch string + ifNoneMatch string + etag string + want int + }{ + {name: "NoConditions", etag: testETag, want: 0}, + {name: "IfMatchExact", ifMatch: testETag, etag: testETag, want: 0}, + {name: "IfMatchWildcard", ifMatch: "*", etag: testETag, want: 0}, + {name: "IfMatchList", ifMatch: `"other", ` + testETag, etag: testETag, want: 0}, + {name: "IfMatchMismatch", ifMatch: `"other"`, etag: testETag, want: http.StatusPreconditionFailed}, + {name: "IfMatchNoETag", ifMatch: "*", etag: "", want: http.StatusPreconditionFailed}, + {name: "IfNoneMatchExact", ifNoneMatch: testETag, etag: testETag, want: http.StatusNotModified}, + {name: "IfNoneMatchWildcard", ifNoneMatch: "*", etag: testETag, want: http.StatusNotModified}, + {name: "IfNoneMatchList", ifNoneMatch: `"other", ` + testETag, etag: testETag, want: http.StatusNotModified}, + {name: "IfNoneMatchMismatch", ifNoneMatch: `"other"`, etag: testETag, want: 0}, + {name: "IfNoneMatchNoETag", ifNoneMatch: "*", etag: "", want: 0}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := httputil.CheckConditionals(newRequest(t, tt.ifMatch, tt.ifNoneMatch), tt.etag) + assert.Equal(t, tt.want, got) + }) + } +} + +// trackingReader records whether Close was called, so tests can assert the body +// is always released. +type trackingReader struct { + io.Reader + closed bool +} + +func (t *trackingReader) Close() error { + t.closed = true + return nil +} + +func TestServeCacheHit(t *testing.T) { + t.Run("StreamsBodyWhenFresh", func(t *testing.T) { + body := &trackingReader{Reader: strings.NewReader("payload")} + headers := cacheHeaders([2]string{"Content-Type", "text/plain"}) + w := httptest.NewRecorder() + + err := httputil.ServeCacheHit(w, newRequest(t, "", ""), headers, body) + assert.NoError(t, err) + assert.True(t, body.closed) + + resp := w.Result() + defer resp.Body.Close() + assert.Equal(t, http.StatusOK, resp.StatusCode) + assert.Equal(t, testETag, resp.Header.Get("ETag")) + assert.Equal(t, "text/plain", resp.Header.Get("Content-Type")) + data, _ := io.ReadAll(resp.Body) + assert.Equal(t, "payload", string(data)) + }) + + t.Run("NotModifiedSkipsBody", func(t *testing.T) { + body := &trackingReader{Reader: strings.NewReader("payload")} + headers := cacheHeaders() + w := httptest.NewRecorder() + + err := httputil.ServeCacheHit(w, newRequest(t, "", testETag), headers, body) + assert.NoError(t, err) + assert.True(t, body.closed) + + resp := w.Result() + defer resp.Body.Close() + assert.Equal(t, http.StatusNotModified, resp.StatusCode) + data, _ := io.ReadAll(resp.Body) + assert.Equal(t, "", string(data)) + }) + + t.Run("PreconditionFailedSkipsBody", func(t *testing.T) { + body := &trackingReader{Reader: strings.NewReader("payload")} + headers := cacheHeaders() + w := httptest.NewRecorder() + + err := httputil.ServeCacheHit(w, newRequest(t, `"other"`, ""), headers, body) + assert.NoError(t, err) + assert.True(t, body.closed) + + resp := w.Result() + defer resp.Body.Close() + assert.Equal(t, http.StatusPreconditionFailed, resp.StatusCode) + }) +} + +func TestServeCacheStat(t *testing.T) { + t.Run("OKWhenFresh", func(t *testing.T) { + headers := cacheHeaders() + w := httptest.NewRecorder() + + httputil.ServeCacheStat(w, newRequest(t, "", ""), headers) + + resp := w.Result() + defer resp.Body.Close() + assert.Equal(t, http.StatusOK, resp.StatusCode) + assert.Equal(t, testETag, resp.Header.Get("ETag")) + }) + + t.Run("NotModified", func(t *testing.T) { + headers := cacheHeaders() + w := httptest.NewRecorder() + + httputil.ServeCacheStat(w, newRequest(t, "", testETag), headers) + + resp := w.Result() + defer resp.Body.Close() + assert.Equal(t, http.StatusNotModified, resp.StatusCode) + }) +} diff --git a/internal/strategy/apiv1.go b/internal/strategy/apiv1.go index a6f18f22..75510eea 100644 --- a/internal/strategy/apiv1.go +++ b/internal/strategy/apiv1.go @@ -5,10 +5,8 @@ import ( "encoding/json" "io" "log/slog" - "maps" "net/http" "os" - "strings" "time" "github.com/alecthomas/errors" @@ -69,12 +67,7 @@ func (d *APIV1) statObject(w http.ResponseWriter, r *http.Request) { return } - maps.Copy(w.Header(), headers) - if status := CheckConditionals(r, headers.Get("ETag")); status != 0 { - w.WriteHeader(status) - return - } - w.WriteHeader(http.StatusOK) + httputil.ServeCacheStat(w, r, headers) } func (d *APIV1) getObject(w http.ResponseWriter, r *http.Request) { @@ -100,19 +93,8 @@ func (d *APIV1) getObject(w http.ResponseWriter, r *http.Request) { return } - maps.Copy(w.Header(), headers) - if status := CheckConditionals(r, headers.Get("ETag")); status != 0 { - _ = cr.Close() - w.WriteHeader(status) - return - } - - _, err = io.Copy(w, cr) - if err != nil { - d.logger.Error("Failed to copy cache object to response", "error", err, "key", key) - } - if cerr := cr.Close(); cerr != nil { - d.logger.Error("Failed to close cache reader", "error", cerr, "key", key) + if err := httputil.ServeCacheHit(w, r, headers, cr); err != nil { + d.logger.Error("Failed to serve cache object", "error", err, "key", key) } } @@ -218,34 +200,3 @@ func (d *APIV1) httpError(w http.ResponseWriter, code int, err error, message st d.logger.Error(message, args...) http.Error(w, message, code) } - -// CheckConditionals evaluates If-Match and If-None-Match precondition headers -// against the stored ETag for GET/HEAD requests. Returns 0 if all -// preconditions pass, otherwise the HTTP status code to send (304 or 412). -func CheckConditionals(r *http.Request, etag string) int { - if ifMatch := r.Header.Get("If-Match"); ifMatch != "" { - if etag == "" || !etagListMatches(ifMatch, etag) { - return http.StatusPreconditionFailed - } - } - if ifNoneMatch := r.Header.Get("If-None-Match"); ifNoneMatch != "" { - if etag != "" && etagListMatches(ifNoneMatch, etag) { - return http.StatusNotModified - } - } - return 0 -} - -// etagListMatches reports whether etag matches an RFC 7232 If-Match / -// If-None-Match header value, which may be a comma-separated list of ETags or -// the "*" wildcard. Stored ETags are always strong, so weak comparison is not -// required. -func etagListMatches(headerValue, etag string) bool { - for candidate := range strings.SplitSeq(headerValue, ",") { - candidate = strings.TrimSpace(candidate) - if candidate == "*" || candidate == etag { - return true - } - } - return false -} diff --git a/internal/strategy/git/snapshot.go b/internal/strategy/git/snapshot.go index 0c13ca62..7cd5c689 100644 --- a/internal/strategy/git/snapshot.go +++ b/internal/strategy/git/snapshot.go @@ -20,9 +20,9 @@ import ( "github.com/block/cachew/internal/cache" "github.com/block/cachew/internal/gitclone" + "github.com/block/cachew/internal/httputil" "github.com/block/cachew/internal/logging" "github.com/block/cachew/internal/snapshot" - "github.com/block/cachew/internal/strategy" ) const lfsFetchTimeout = 25 * time.Minute @@ -367,7 +367,7 @@ func (s *Strategy) serveSnapshotHead(ctx context.Context, w http.ResponseWriter, } status := http.StatusOK - if conditional := strategy.CheckConditionals(r, headers.Get(cache.ETagKey)); conditional != 0 { + if conditional := httputil.CheckConditionals(r, headers.Get(cache.ETagKey)); conditional != 0 { status = conditional } w.WriteHeader(status) @@ -557,7 +557,7 @@ func (s *Strategy) serveSnapshotWithBundle(ctx context.Context, w http.ResponseW // avoid streaming the full snapshot when the client already has it. var n int64 var err error - if status := strategy.CheckConditionals(r, headers.Get(cache.ETagKey)); status != 0 { + if status := httputil.CheckConditionals(r, headers.Get(cache.ETagKey)); status != 0 { w.WriteHeader(status) } else { n, err = serveReaderFast(w, r, reader) diff --git a/internal/strategy/handler/handler.go b/internal/strategy/handler/handler.go index 7079876c..0353463e 100644 --- a/internal/strategy/handler/handler.go +++ b/internal/strategy/handler/handler.go @@ -171,12 +171,7 @@ func (h *Handler) serveCached(w http.ResponseWriter, r *http.Request, key cache. } logging.FromContext(r.Context()).DebugContext(r.Context(), "Cache hit") - defer cr.Close() - maps.Copy(w.Header(), headers) - if _, err := io.Copy(w, cr); err != nil { - return true, errors.Wrap(err, "stream from cache") - } - return true, nil + return true, errors.WithStack(httputil.ServeCacheHit(w, r, headers, cr)) } func (h *Handler) fetchAndCache(w http.ResponseWriter, r *http.Request, key cache.Key) error {