chore: apply feedback suggestions

Co-authored-by: Marcin Rataj <lidel@lidel.org>

gateway/gateway_test.go

@@ -150,7 +150,7 @@ func TestHeaders(t *testing.T) {
 		dagCborRoots = dirRoots + "," + dagCborCID
 	)
 
-	t.Run("Control-Cache-Immutable is not immutable for directories", func(t *testing.T) {
+	t.Run("Cache-Control is not immutable on generated /ipfs/  HTML dir listings", func(t *testing.T) {
 		req := mustNewRequest(t, http.MethodGet, ts.URL+"/ipfs/"+root.String()+"/", nil)
 		res := mustDoWithoutRedirect(t, req)
 
@@ -163,7 +163,7 @@ func TestHeaders(t *testing.T) {
 		}
 	})
 
-	t.Run("ETag contains expected values", func(t *testing.T) {
+	t.Run("ETag is based on CID and response format", func(t *testing.T) {
 		test := func(responseFormat string, path string, format string, args ...any) {
 			t.Run(responseFormat, func(t *testing.T) {
 				url := ts.URL + path

gateway/handler.go

@@ -185,12 +185,20 @@ type requestData struct {
 
 	// Defined if resolution has already happened.
 	pathMetadata *ContentPathMetadata
-	resolvedPath *ImmutablePath
 }
 
-func (rq *requestData) maybeResolvedPath() ImmutablePath {
-	if rq.resolvedPath != nil {
-		return *rq.resolvedPath
+// mostlyResolvedPath is an opportunistic optimization that returns the mostly
+// resolved version of ImmutablePath available. It does not guarantee it is fully
+// resolved, nor that it is the original.
+func (rq *requestData) mostlyResolvedPath() ImmutablePath {
+	if rq.pathMetadata != nil {
+		imPath, err := NewImmutablePath(rq.pathMetadata.LastSegment)
+		if err != nil {
+			// This will never happen. This error has previously been checked in
+			// [handleIfNoneMatch] and the request will have returned 500.
+			panic(err)
+		}
+		return imPath
 	}
 	return rq.immutablePath
 }
@@ -469,7 +477,12 @@ func setContentDispositionHeader(w http.ResponseWriter, filename string, disposi
 
 // setIpfsRootsHeader sets the X-Ipfs-Roots header with logical CID array for
 // efficient HTTP cache invalidation.
-func setIpfsRootsHeader(w http.ResponseWriter, pathMetadata ContentPathMetadata) {
+func setIpfsRootsHeader(w http.ResponseWriter, rq *requestData, md *ContentPathMetadata) {
+	// Update requestData with the latest ContentPathMetadata if it wasn't set yet.
+	if rq.pathMetadata == nil {
+		rq.pathMetadata = md
+	}
+
 	// These are logical roots where each CID represent one path segment
 	// and resolves to either a directory or the root block of a file.
 	// The main purpose of this header is allow HTTP caches to do smarter decisions
@@ -492,10 +505,10 @@ func setIpfsRootsHeader(w http.ResponseWriter, pathMetadata ContentPathMetadata)
 	// the last root (responsible for specific article) may not change at all.
 
 	var pathRoots []string
-	for _, c := range pathMetadata.PathSegmentRoots {
+	for _, c := range rq.pathMetadata.PathSegmentRoots {
 		pathRoots = append(pathRoots, c.String())
 	}
-	pathRoots = append(pathRoots, pathMetadata.LastSegment.Cid().String())
+	pathRoots = append(pathRoots, rq.pathMetadata.LastSegment.Cid().String())
 	rootCidList := strings.Join(pathRoots, ",") // convention from rfc2616#sec4.2
 
 	w.Header().Set("X-Ipfs-Roots", rootCidList)
@@ -686,8 +699,7 @@ func (i *handler) handleIfNoneMatch(w http.ResponseWriter, r *http.Request, rq *
 			return true
 		}
 
-		resolvedPath := pathMetadata.LastSegment
-		pathCid := resolvedPath.Cid()
+		pathCid := pathMetadata.LastSegment.Cid()
 
 		// Checks against both file, dir listing, and dag index Etags.
 		// This is an inexpensive check, and it happens before we do any I/O.
@@ -701,14 +713,14 @@ func (i *handler) handleIfNoneMatch(w http.ResponseWriter, r *http.Request, rq *
 			return true
 		}
 
-		resolvedImPath, err := NewImmutablePath(resolvedPath)
+		// Check if the resolvedPath is an immutable path.
+		_, err = NewImmutablePath(pathMetadata.LastSegment)
 		if err != nil {
 			i.webError(w, r, err, http.StatusInternalServerError)
 			return true
 		}
 
 		rq.pathMetadata = &pathMetadata
-		rq.resolvedPath = &resolvedImPath
 		return false
 	}
 

gateway/handler_block.go

@@ -14,16 +14,13 @@ func (i *handler) serveRawBlock(ctx context.Context, w http.ResponseWriter, r *h
 	ctx, span := spanTrace(ctx, "Handler.ServeRawBlock", trace.WithAttributes(attribute.String("path", rq.immutablePath.String())))
 	defer span.End()
 
-	pathMetadata, data, err := i.backend.GetBlock(ctx, rq.maybeResolvedPath())
+	pathMetadata, data, err := i.backend.GetBlock(ctx, rq.mostlyResolvedPath())
 	if !i.handleRequestErrors(w, r, rq.contentPath, err) {
 		return false
 	}
-	if rq.pathMetadata == nil {
-		rq.pathMetadata = &pathMetadata
-	}
 	defer data.Close()
 
-	setIpfsRootsHeader(w, *rq.pathMetadata)
+	setIpfsRootsHeader(w, rq, &pathMetadata)
 
 	blockCid := pathMetadata.LastSegment.Cid()
 

gateway/handler_car.go

@@ -83,7 +83,7 @@ func (i *handler) serveCAR(ctx context.Context, w http.ResponseWriter, r *http.R
 		return false
 	}
 	defer carFile.Close()
-	setIpfsRootsHeader(w, md)
+	setIpfsRootsHeader(w, rq, &md)
 
 	// Make it clear we don't support range-requests over a car stream
 	// Partial downloads and resumes should be handled using requests for

gateway/handler_codec.go

@@ -62,16 +62,13 @@ func (i *handler) serveCodec(ctx context.Context, w http.ResponseWriter, r *http
 	ctx, span := spanTrace(ctx, "Handler.ServeCodec", trace.WithAttributes(attribute.String("path", rq.immutablePath.String()), attribute.String("requestedContentType", rq.responseFormat)))
 	defer span.End()
 
-	pathMetadata, data, err := i.backend.GetBlock(ctx, rq.maybeResolvedPath())
+	pathMetadata, data, err := i.backend.GetBlock(ctx, rq.mostlyResolvedPath())
 	if !i.handleRequestErrors(w, r, rq.contentPath, err) {
 		return false
 	}
-	if rq.pathMetadata == nil {
-		rq.pathMetadata = &pathMetadata
-	}
 	defer data.Close()
 
-	setIpfsRootsHeader(w, *rq.pathMetadata)
+	setIpfsRootsHeader(w, rq, &pathMetadata)
 	return i.renderCodec(ctx, w, r, rq, data)
 }
 

gateway/handler_defaults.go

@@ -32,7 +32,7 @@ func (i *handler) serveDefaults(ctx context.Context, w http.ResponseWriter, r *h
 	switch r.Method {
 	case http.MethodHead:
 		var data files.Node
-		pathMetadata, data, err = i.backend.Head(ctx, rq.maybeResolvedPath())
+		pathMetadata, data, err = i.backend.Head(ctx, rq.mostlyResolvedPath())
 		if !i.handleRequestErrors(w, r, rq.contentPath, err) {
 			return false
 		}
@@ -62,10 +62,10 @@ func (i *handler) serveDefaults(ctx context.Context, w http.ResponseWriter, r *h
 		// allow backend to find  providers for parents, even when internal
 		// CIDs are not announced, and will provide better key for caching
 		// related DAGs.
-		pathMetadata, getResp, err = i.backend.Get(ctx, rq.maybeResolvedPath(), ranges...)
+		pathMetadata, getResp, err = i.backend.Get(ctx, rq.mostlyResolvedPath(), ranges...)
 		if err != nil {
 			if isWebRequest(rq.responseFormat) {
-				forwardedPath, continueProcessing := i.handleWebRequestErrors(w, r, rq.maybeResolvedPath(), rq.immutablePath, rq.contentPath, err, rq.logger)
+				forwardedPath, continueProcessing := i.handleWebRequestErrors(w, r, rq.mostlyResolvedPath(), rq.immutablePath, rq.contentPath, err, rq.logger)
 				if !continueProcessing {
 					return false
 				}
@@ -94,10 +94,7 @@ func (i *handler) serveDefaults(ctx context.Context, w http.ResponseWriter, r *h
 		return false
 	}
 
-	if rq.pathMetadata == nil {
-		rq.pathMetadata = &pathMetadata
-	}
-	setIpfsRootsHeader(w, *rq.pathMetadata)
+	setIpfsRootsHeader(w, rq, &pathMetadata)
 
 	resolvedPath := pathMetadata.LastSegment
 	switch mc.Code(resolvedPath.Cid().Prefix().Codec) {

gateway/handler_tar.go

@@ -21,16 +21,13 @@ func (i *handler) serveTAR(ctx context.Context, w http.ResponseWriter, r *http.R
 	defer cancel()
 
 	// Get Unixfs file (or directory)
-	pathMetadata, file, err := i.backend.GetAll(ctx, rq.maybeResolvedPath())
+	pathMetadata, file, err := i.backend.GetAll(ctx, rq.mostlyResolvedPath())
 	if !i.handleRequestErrors(w, r, rq.contentPath, err) {
 		return false
 	}
-	if rq.pathMetadata == nil {
-		rq.pathMetadata = &pathMetadata
-	}
 	defer file.Close()
 
-	setIpfsRootsHeader(w, *rq.pathMetadata)
+	setIpfsRootsHeader(w, rq, &pathMetadata)
 	rootCid := pathMetadata.LastSegment.Cid()
 
 	// Set Cache-Control and read optional Last-Modified time