refactor(httputil): generalise range support via ServeCacheHit hooks (#359)

Add a WithResponseDecorator option and a bytes-served return to
ServeCacheHit so endpoints needing custom headers or negotiation can
reuse the shared range/conditional machinery instead of reimplementing
206/416/304/412 handling.

Refactor the git snapshot and bundle serve paths onto it: both now
forward ConditionalOptions to Open and serve via ServeCacheHit,
deleting the bespoke range/conditional code and giving bundles range
support. Snapshot freshen metadata is injected through the decorator on
both full and ranged responses.

Drop the now-unused RangeOptions helper.

Author: alecthomas

internal/httputil/conditional.go

@@ -32,23 +32,6 @@ func ConditionalOptions(r *http.Request) []client.RequestOption {
 	return opts
 }
 
-// RangeOptions extracts only the Range/If-Range options from r, for callers
-// that evaluate If-Match/If-None-Match separately (e.g. via CheckConditionals)
-// and so must not double-handle them by forwarding the full set to Open.
-// If-Range is only meaningful alongside Range, so it is dropped when Range is
-// absent.
-func RangeOptions(r *http.Request) []client.RequestOption {
-	v := r.Header.Get("Range")
-	if v == "" {
-		return nil
-	}
-	opts := []client.RequestOption{func(o *client.RequestOptions) { o.Range = v }}
-	if ir := r.Header.Get("If-Range"); ir != "" {
-		opts = append(opts, client.IfRange(ir))
-	}
-	return opts
-}
-
 // CheckConditionals evaluates RFC 7232 If-Match and If-None-Match precondition
 // headers on r against etag. It returns 0 when all preconditions pass,
 // otherwise the HTTP status the caller should send: 412 Precondition Failed for
@@ -66,42 +49,74 @@ func CheckConditionals(r *http.Request, etag string) int {
 	}
 }
 
+// ServeOption configures ServeCacheHit.
+type ServeOption func(*serveConfig)
+
+type serveConfig struct {
+	decorate func(http.ResponseWriter, http.Header)
+}
+
+// WithResponseDecorator registers fn to run after the stored headers have been
+// copied to the response but before the status line is written, on the 200, 206
+// and 416 paths. It lets endpoints that delegate range/conditional resolution to
+// the cache still override or augment the response (e.g. force a Content-Type or
+// advertise endpoint-specific metadata) uniformly across full and partial
+// responses. fn is given the stored headers (so it can branch on, say, a
+// Content-Range) and must not write the body or call WriteHeader.
+func WithResponseDecorator(fn func(w http.ResponseWriter, stored http.Header)) ServeOption {
+	return func(c *serveConfig) { c.decorate = fn }
+}
+
 // ServeCacheHit writes the outcome of a cache Open to w. headers and body are
 // the Open return values and openErr its error. It handles the success and
 // conditional cases: a nil error streams the body (always closing it), a
 // satisfied If-None-Match (ErrNotModified) writes 304 with the stored headers,
 // and a failed If-Match (ErrPreconditionFailed) writes 412. It returns
 // handled=false for any other error (e.g. os.ErrNotExist) so the caller can map
-// it to its own status.
-func ServeCacheHit(w http.ResponseWriter, headers http.Header, body io.ReadCloser, openErr error) (handled bool, err error) {
+// it to its own status, and n is the number of body bytes written (0 for
+// bodiless responses).
+func ServeCacheHit(w http.ResponseWriter, headers http.Header, body io.ReadCloser, openErr error, opts ...ServeOption) (handled bool, n int64, err error) {
+	var cfg serveConfig
+	for _, opt := range opts {
+		opt(&cfg)
+	}
+	decorate := func() {
+		if cfg.decorate != nil {
+			cfg.decorate(w, headers)
+		}
+	}
+
 	switch {
 	case openErr == nil:
 		maps.Copy(w.Header(), headers)
 		w.Header().Set("Accept-Ranges", "bytes")
+		decorate()
 		// A Content-Range set by the cache signals a satisfied byte range.
 		if headers.Get("Content-Range") != "" {
 			w.WriteHeader(http.StatusPartialContent)
 		}
-		_, copyErr := io.Copy(w, body)
-		return true, errors.Wrap(errors.Join(copyErr, body.Close()), "serve cache hit")
+		var copyErr error
+		n, copyErr = io.Copy(w, body)
+		return true, n, errors.Wrap(errors.Join(copyErr, body.Close()), "serve cache hit")
 
 	case errors.Is(openErr, client.ErrNotModified):
 		maps.Copy(w.Header(), headers)
 		w.WriteHeader(http.StatusNotModified)
-		return true, nil
+		return true, 0, nil
 
 	case errors.Is(openErr, client.ErrPreconditionFailed):
 		w.WriteHeader(http.StatusPreconditionFailed)
-		return true, nil
+		return true, 0, nil
 
 	case errors.Is(openErr, client.ErrRangeNotSatisfiable):
 		maps.Copy(w.Header(), headers)
 		w.Header().Set("Accept-Ranges", "bytes")
+		decorate()
 		w.WriteHeader(http.StatusRequestedRangeNotSatisfiable)
-		return true, nil
+		return true, 0, nil
 
 	default:
-		return false, nil
+		return false, 0, nil
 	}
 }
 

internal/httputil/conditional_test.go

@@ -83,14 +83,16 @@ func TestServeCacheHit(t *testing.T) {
 		headers := cacheHeaders([2]string{"Content-Type", "text/plain"})
 		w := httptest.NewRecorder()
 
-		handled, err := httputil.ServeCacheHit(w, headers, body, nil)
+		handled, n, err := httputil.ServeCacheHit(w, headers, body, nil)
 		assert.True(t, handled)
+		assert.Equal(t, int64(len("payload")), n)
 		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, "bytes", resp.Header.Get("Accept-Ranges"))
 		assert.Equal(t, testETag, resp.Header.Get("ETag"))
 		assert.Equal(t, "text/plain", resp.Header.Get("Content-Type"))
 		data, _ := io.ReadAll(resp.Body)
@@ -101,8 +103,9 @@ func TestServeCacheHit(t *testing.T) {
 		headers := cacheHeaders()
 		w := httptest.NewRecorder()
 
-		handled, err := httputil.ServeCacheHit(w, headers, nil, client.ErrNotModified)
+		handled, n, err := httputil.ServeCacheHit(w, headers, nil, client.ErrNotModified)
 		assert.True(t, handled)
+		assert.Equal(t, int64(0), n)
 		assert.NoError(t, err)
 
 		resp := w.Result()
@@ -116,20 +119,96 @@ func TestServeCacheHit(t *testing.T) {
 	t.Run("PreconditionFailed", func(t *testing.T) {
 		w := httptest.NewRecorder()
 
-		handled, err := httputil.ServeCacheHit(w, nil, nil, client.ErrPreconditionFailed)
+		handled, n, err := httputil.ServeCacheHit(w, nil, nil, client.ErrPreconditionFailed)
 		assert.True(t, handled)
+		assert.Equal(t, int64(0), n)
 		assert.NoError(t, err)
 
 		resp := w.Result()
 		defer resp.Body.Close()
 		assert.Equal(t, http.StatusPreconditionFailed, resp.StatusCode)
 	})
 
+	t.Run("PartialContentWhenRanged", func(t *testing.T) {
+		body := &trackingReader{Reader: strings.NewReader("2345")}
+		headers := cacheHeaders([2]string{"Content-Range", "bytes 2-5/10"})
+		w := httptest.NewRecorder()
+
+		handled, n, err := httputil.ServeCacheHit(w, headers, body, nil)
+		assert.True(t, handled)
+		assert.Equal(t, int64(4), n)
+		assert.NoError(t, err)
+		assert.True(t, body.closed)
+
+		resp := w.Result()
+		defer resp.Body.Close()
+		assert.Equal(t, http.StatusPartialContent, resp.StatusCode)
+		assert.Equal(t, "bytes 2-5/10", resp.Header.Get("Content-Range"))
+		assert.Equal(t, "bytes", resp.Header.Get("Accept-Ranges"))
+	})
+
+	t.Run("RangeNotSatisfiable", func(t *testing.T) {
+		headers := cacheHeaders([2]string{"Content-Range", "bytes */10"})
+		w := httptest.NewRecorder()
+
+		handled, n, err := httputil.ServeCacheHit(w, headers, nil, client.ErrRangeNotSatisfiable)
+		assert.True(t, handled)
+		assert.Equal(t, int64(0), n)
+		assert.NoError(t, err)
+
+		resp := w.Result()
+		defer resp.Body.Close()
+		assert.Equal(t, http.StatusRequestedRangeNotSatisfiable, resp.StatusCode)
+		assert.Equal(t, "bytes */10", resp.Header.Get("Content-Range"))
+		assert.Equal(t, "bytes", resp.Header.Get("Accept-Ranges"))
+	})
+
+	t.Run("DecoratorRunsOnFullPartialAndUnsatisfiable", func(t *testing.T) {
+		// The decorator must run before the status is written on the 200, 206 and
+		// 416 paths, but not on the bodiless 304/412 paths.
+		cases := []struct {
+			name       string
+			headers    http.Header
+			body       io.ReadCloser
+			openErr    error
+			wantRun    bool
+			wantStatus int
+		}{
+			{name: "Full", headers: cacheHeaders(), body: &trackingReader{Reader: strings.NewReader("x")}, wantRun: true, wantStatus: http.StatusOK},
+			{name: "Partial", headers: cacheHeaders([2]string{"Content-Range", "bytes 0-0/2"}), body: &trackingReader{Reader: strings.NewReader("x")}, wantRun: true, wantStatus: http.StatusPartialContent},
+			{name: "Unsatisfiable", headers: cacheHeaders([2]string{"Content-Range", "bytes */2"}), openErr: client.ErrRangeNotSatisfiable, wantRun: true, wantStatus: http.StatusRequestedRangeNotSatisfiable},
+			{name: "NotModified", headers: cacheHeaders(), openErr: client.ErrNotModified, wantRun: false, wantStatus: http.StatusNotModified},
+			{name: "PreconditionFailed", openErr: client.ErrPreconditionFailed, wantRun: false, wantStatus: http.StatusPreconditionFailed},
+		}
+		for _, tc := range cases {
+			t.Run(tc.name, func(t *testing.T) {
+				w := httptest.NewRecorder()
+				var ran bool
+				decorate := func(rw http.ResponseWriter, _ http.Header) {
+					ran = true
+					rw.Header().Set("X-Decorated", "yes")
+				}
+				handled, _, err := httputil.ServeCacheHit(w, tc.headers, tc.body, tc.openErr, httputil.WithResponseDecorator(decorate))
+				assert.True(t, handled)
+				assert.NoError(t, err)
+				assert.Equal(t, tc.wantRun, ran)
+
+				resp := w.Result()
+				defer resp.Body.Close()
+				assert.Equal(t, tc.wantStatus, resp.StatusCode)
+				if tc.wantRun {
+					assert.Equal(t, "yes", resp.Header.Get("X-Decorated"))
+				}
+			})
+		}
+	})
+
 	t.Run("NotHandledForOtherError", func(t *testing.T) {
 		w := httptest.NewRecorder()
 
-		handled, err := httputil.ServeCacheHit(w, nil, nil, os.ErrNotExist)
+		handled, n, err := httputil.ServeCacheHit(w, nil, nil, os.ErrNotExist)
 		assert.False(t, handled)
+		assert.Equal(t, int64(0), n)
 		assert.NoError(t, err)
 		assert.Equal(t, http.StatusOK, w.Result().StatusCode) // response untouched
 	})

internal/strategy/apiv1.go

@@ -82,7 +82,7 @@ func (d *APIV1) getObject(w http.ResponseWriter, r *http.Request) {
 
 	namespacedCache := d.cache.Namespace(namespace)
 	cr, headers, err := namespacedCache.Open(r.Context(), key, httputil.ConditionalOptions(r)...)
-	if handled, serveErr := httputil.ServeCacheHit(w, headers, cr, err); handled {
+	if handled, _, serveErr := httputil.ServeCacheHit(w, headers, cr, err); handled {
 		if serveErr != nil {
 			d.logger.Error("Failed to serve cache object", "error", serveErr, "key", key)
 		}