refactor: add contentPathToCarUrl and tests

this implements part of #160 (comment) that fixes the way we encode content paths, without changing the interface of fetcher in caboose.
ipfs-inactive · Jul 26, 2023 · 982535a · 982535a
1 parent 539d7ad
commit 982535a
Show file tree

Hide file tree

Showing 4 changed files with 108 additions and 28 deletions.
diff --git a/graph_gateway_test.go b/graph_gateway_test.go
@@ -328,7 +328,7 @@ func TestTarAtEndOfPath(t *testing.T) {
 			}
 		case 6:
 			// Expect a request for the rest of the multiblock file in the directory and give it
-			expectedUri := "/ipfs/bafybeigcisqd7m5nf3qmuvjdbakl5bdnh4ocrmacaqkpuh77qjvggmt2sa?dag-scope=entity&entity-bytes=768:*"
+			expectedUri := "/ipfs/bafybeigcisqd7m5nf3qmuvjdbakl5bdnh4ocrmacaqkpuh77qjvggmt2sa?format=car&dag-scope=entity&entity-bytes=768:*"
 			if request.RequestURI != expectedUri {
 				panic(fmt.Errorf("expected URI %s, got %s", expectedUri, request.RequestURI))
 			}

diff --git a/lib/gateway_utils.go b/lib/gateway_utils.go
@@ -0,0 +1,39 @@
+package lib
+
+import (
+	"net/url"
+	"strconv"
+	"strings"
+
+	"github.com/ipfs/boxo/gateway"
+)
+
+// contentPathToCarUrl returns an URL that allows retrieval of specified resource
+// from a trustless gateway that implements IPIP-402
+func contentPathToCarUrl(path gateway.ImmutablePath, params gateway.CarParams) *url.URL {
+	return &url.URL{
+		Path:     path.String(),
+		RawQuery: carParamsToString(params),
+	}
+}
+
+// carParamsToString converts CarParams to URL parameters compatible with IPIP-402
+func carParamsToString(params gateway.CarParams) string {
+	paramsBuilder := strings.Builder{}
+	paramsBuilder.WriteString("format=car") // always send explicit format in URL, this  makes debugging easier, even when Accept header was set
+	if params.Scope != "" {
+		paramsBuilder.WriteString("&dag-scope=")
+		paramsBuilder.WriteString(string(params.Scope))
+	}
+	if params.Range != nil {
+		paramsBuilder.WriteString("&entity-bytes=")
+		paramsBuilder.WriteString(strconv.FormatInt(params.Range.From, 10))
+		paramsBuilder.WriteString(":")
+		if params.Range.To != nil {
+			paramsBuilder.WriteString(strconv.FormatInt(*params.Range.To, 10))
+		} else {
+			paramsBuilder.WriteString("*")
+		}
+	}
+	return paramsBuilder.String()
+}
diff --git a/lib/gateway_utils_test.go b/lib/gateway_utils_test.go
@@ -0,0 +1,57 @@
+package lib
+
+import (
+	"testing"
+
+	ifacepath "github.com/ipfs/boxo/coreiface/path"
+	"github.com/ipfs/boxo/gateway"
+)
+
+func TestContentPathToCarUrl(t *testing.T) {
+	negativeOffset := int64(-42)
+	testCases := []struct {
+		contentPath string // to be turned into gateway.ImmutablePath
+		carParams   gateway.CarParams
+		expectedUrl string // url.URL.String()
+	}{
+		{
+			contentPath: "/ipfs/bafybeigdyrzt5sfp7udm7hu76uh7y26nf3efuylqabf3oclgtqy55fbzdi",
+			carParams:   gateway.CarParams{},
+			expectedUrl: "/ipfs/bafybeigdyrzt5sfp7udm7hu76uh7y26nf3efuylqabf3oclgtqy55fbzdi?format=car",
+		},
+		{
+			contentPath: "/ipfs/bafybeigdyrzt5sfp7udm7hu76uh7y26nf3efuylqabf3oclgtqy55fbzdi",
+			carParams:   gateway.CarParams{Scope: "entity", Range: &gateway.DagByteRange{From: 0, To: nil}},
+			expectedUrl: "/ipfs/bafybeigdyrzt5sfp7udm7hu76uh7y26nf3efuylqabf3oclgtqy55fbzdi?format=car&dag-scope=entity&entity-bytes=0:*",
+		},
+		{
+			contentPath: "/ipfs/bafybeigdyrzt5sfp7udm7hu76uh7y26nf3efuylqabf3oclgtqy55fbzdi",
+			carParams:   gateway.CarParams{Scope: "block"},
+			expectedUrl: "/ipfs/bafybeigdyrzt5sfp7udm7hu76uh7y26nf3efuylqabf3oclgtqy55fbzdi?format=car&dag-scope=block",
+		},
+		{
+			contentPath: "/ipfs/bafybeigdyrzt5sfp7udm7hu76uh7y26nf3efuylqabf3oclgtqy55fbzdi",
+			carParams:   gateway.CarParams{Scope: "entity", Range: &gateway.DagByteRange{From: 4, To: &negativeOffset}},
+			expectedUrl: "/ipfs/bafybeigdyrzt5sfp7udm7hu76uh7y26nf3efuylqabf3oclgtqy55fbzdi?format=car&dag-scope=entity&entity-bytes=4:-42",
+		},
+		{
+			// a regression test for case described in https://github.com/ipfs/gateway-conformance/issues/115
+			contentPath: "/ipfs/bafybeiaysi4s6lnjev27ln5icwm6tueaw2vdykrtjkwiphwekaywqhcjze/I/Auditorio_de_Tenerife%2C_Santa_Cruz_de_Tenerife%2C_España%2C_2012-12-15%2C_DD_02.jpg.webp",
+			carParams:   gateway.CarParams{Scope: "entity", Range: &gateway.DagByteRange{From: 0, To: nil}},
+			expectedUrl: "/ipfs/bafybeiaysi4s6lnjev27ln5icwm6tueaw2vdykrtjkwiphwekaywqhcjze/I/Auditorio_de_Tenerife%252C_Santa_Cruz_de_Tenerife%252C_Espa%C3%B1a%252C_2012-12-15%252C_DD_02.jpg.webp?format=car&dag-scope=entity&entity-bytes=0:*",
+		},
+	}
+
+	for _, tc := range testCases {
+		t.Run("TestContentPathToCarUrl", func(t *testing.T) {
+			contentPath, err := gateway.NewImmutablePath(ifacepath.New(tc.contentPath))
+			if err != nil {
+				t.Fatal(err)
+			}
+			result := contentPathToCarUrl(contentPath, tc.carParams).String()
+			if result != tc.expectedUrl {
+				t.Errorf("Expected %q, but got %q", tc.expectedUrl, result)
+			}
+		})
+	}
+}
diff --git a/lib/graph_gateway.go b/lib/graph_gateway.go
@@ -5,17 +5,17 @@ import (
 	"context"
 	"errors"
 	"fmt"
-	"github.com/hashicorp/go-multierror"
-	"github.com/ipfs/go-unixfsnode/hamt"
-	"github.com/ipld/go-ipld-prime/traversal"
 	"io"
 	"net/http"
-	"net/url"
 	gopath "path"
 	"strconv"
 	"strings"
 	"time"
 
+	"github.com/hashicorp/go-multierror"
+	"github.com/ipfs/go-unixfsnode/hamt"
+	"github.com/ipld/go-ipld-prime/traversal"
+
 	"github.com/filecoin-saturn/caboose"
 	nsopts "github.com/ipfs/boxo/coreiface/options/namesys"
 	ifacepath "github.com/ipfs/boxo/coreiface/path"
@@ -230,28 +230,8 @@ func registerGraphGatewayMetrics(registerer prometheus.Registerer) *GraphGateway
 	}
 }
 
-func paramsToString(params gateway.CarParams) string {
-	paramsBuilder := strings.Builder{}
-	paramsBuilder.WriteString("dag-scope=")
-	paramsBuilder.WriteString(string(params.Scope))
-	if params.Range != nil {
-		paramsBuilder.WriteString("&entity-bytes=")
-		paramsBuilder.WriteString(strconv.FormatInt(params.Range.From, 10))
-		paramsBuilder.WriteString(":")
-		if params.Range.To != nil {
-			paramsBuilder.WriteString(strconv.FormatInt(*params.Range.To, 10))
-		} else {
-			paramsBuilder.WriteString("*")
-		}
-	}
-	return paramsBuilder.String()
-}
-
 func (api *GraphGateway) fetchCAR(ctx context.Context, path gateway.ImmutablePath, params gateway.CarParams, cb DataCallback) error {
-	escapedPath := url.PathEscape(path.String())
-	escapedPath = strings.ReplaceAll(escapedPath, "%2F", "/")
-	paramsStr := paramsToString(params)
-	urlWithoutHost := fmt.Sprintf("%s?%s", escapedPath, paramsStr)
+	urlWithoutHost := contentPathToCarUrl(path, params).String()
 
 	api.metrics.carFetchAttemptMetric.Inc()
 	var ipldError error
@@ -866,11 +846,15 @@ func fetchWithPartialRetries[T any](ctx context.Context, path gateway.ImmutableP
 			case closeErr := <-closeCh:
 				return closeErr
 			case req := <-sendRequest:
-				requestStr := fmt.Sprintf("/ipfs/%s?%s", req.c.String(), paramsToString(req.params))
 				// set path and params for next iteration
 				p = ipfspath.FromCid(req.c)
+				imPath, err := gateway.NewImmutablePath(ifacepath.New(p.String()))
+				if err != nil {
+					return err
+				}
 				params = req.params
-				return &ErrPartialResponse{StillNeed: []string{requestStr}}
+				remainderUrl := contentPathToCarUrl(imPath, params).String()
+				return &ErrPartialResponse{StillNeed: []string{remainderUrl}}
 			case <-cctx.Done():
 				return cctx.Err()
 			}