refactor: simplify processing of CarParams

- adds more type safety: DuplicateBlocksPolicy is no longer a pointer to
  bool, and together with DagOrder has an explicit *Unspecified state.
- removes passing pointer and reliance on mutation. now mutation
  happens only once, in buildCarParams func, where implicit defaults
  are set when order or dups are unspecified in the request
- moved car version validation and other parameter related logic
  into the same build funcs
- fixed a bug where ?format=car eclipsed params from Accept header

Author: lidel

gateway/blocks_backend.go

@@ -232,25 +232,7 @@ func (bb *BlocksBackend) Head(ctx context.Context, path ImmutablePath) (ContentP
 	return md, fileNode, nil
 }
 
-func (bb *BlocksBackend) GetCAR(ctx context.Context, p ImmutablePath, params *CarParams) (ContentPathMetadata, io.ReadCloser, error) {
-	// Check if we support the request order. On unknown, change it to DFS. We change
-	// the parameter directly, which means that the caller can use the value to later construct
-	// the Content-Type header.
-	switch params.Order {
-	case DagOrderUnknown:
-		params.Order = DagOrderDFS
-	case DagOrderDFS:
-		// Do nothing
-	default:
-		return ContentPathMetadata{}, nil, fmt.Errorf("unsupported application/vnd.ipld.car block order parameter: %q", params.Order)
-	}
-
-	// Similarly, if params.Duplicates is not set, let's set it to false.
-	if params.Duplicates == nil {
-		v := false
-		params.Duplicates = &v
-	}
-
+func (bb *BlocksBackend) GetCAR(ctx context.Context, p ImmutablePath, params CarParams) (ContentPathMetadata, io.ReadCloser, error) {
 	pathMetadata, err := bb.ResolvePath(ctx, p)
 	if err != nil {
 		return ContentPathMetadata{}, nil, err
@@ -267,7 +249,7 @@ func (bb *BlocksBackend) GetCAR(ctx context.Context, p ImmutablePath, params *Ca
 			w,
 			[]cid.Cid{pathMetadata.LastSegment.Cid()},
 			car.WriteAsCarV1(true),
-			car.AllowDuplicatePuts(*params.Duplicates),
+			car.AllowDuplicatePuts(params.Duplicates.Bool()),
 		)
 		if err != nil {
 			// io.PipeWriter.CloseWithError always returns nil.
@@ -302,7 +284,7 @@ func (bb *BlocksBackend) GetCAR(ctx context.Context, p ImmutablePath, params *Ca
 }
 
 // walkGatewaySimpleSelector walks the subgraph described by the path and terminal element parameters
-func walkGatewaySimpleSelector(ctx context.Context, p ipfspath.Path, params *CarParams, lsys *ipld.LinkSystem, pathResolver resolver.Resolver) error {
+func walkGatewaySimpleSelector(ctx context.Context, p ipfspath.Path, params CarParams, lsys *ipld.LinkSystem, pathResolver resolver.Resolver) error {
 	// First resolve the path since we always need to.
 	lastCid, remainder, err := pathResolver.ResolveToLastNode(ctx, p)
 	if err != nil {
@@ -335,7 +317,7 @@ func walkGatewaySimpleSelector(ctx context.Context, p ipfspath.Path, params *Car
 				Ctx:                            ctx,
 				LinkSystem:                     *lsys,
 				LinkTargetNodePrototypeChooser: bsfetcher.DefaultPrototypeChooser,
-				LinkVisitOnlyOnce:              !*params.Duplicates,
+				LinkVisitOnlyOnce:              !params.Duplicates.Bool(),
 			},
 		}
 

gateway/gateway.go

@@ -124,7 +124,7 @@ type CarParams struct {
 	Range      *DagByteRange
 	Scope      DagScope
 	Order      DagOrder
-	Duplicates *bool
+	Duplicates DuplicateBlocksPolicy
 }
 
 // DagByteRange describes a range request within a UnixFS file. "From" and
@@ -194,10 +194,47 @@ const (
 type DagOrder string
 
 const (
-	DagOrderDFS     DagOrder = "dfs"
-	DagOrderUnknown DagOrder = "unk"
+	DagOrderUnspecified DagOrder = ""
+	DagOrderUnknown     DagOrder = "unk"
+	DagOrderDFS         DagOrder = "dfs"
 )
 
+// DuplicateBlocksPolicy represents the content type parameter 'dups' (IPIP-412)
+type DuplicateBlocksPolicy int
+
+const (
+	DuplicateBlocksUnspecified DuplicateBlocksPolicy = iota // 0 - implicit default
+	DuplicateBlocksIncluded                                 // 1 - explicitly include duplicates
+	DuplicateBlocksExcluded                                 // 2 - explicitly NOT include duplicates
+)
+
+// NewDuplicateBlocksPolicy returns DuplicateBlocksPolicy based on the content type parameter 'dups' (IPIP-412)
+func NewDuplicateBlocksPolicy(dupsValue string) DuplicateBlocksPolicy {
+	switch dupsValue {
+	case "y":
+		return DuplicateBlocksIncluded
+	case "n":
+		return DuplicateBlocksExcluded
+	}
+	return DuplicateBlocksUnspecified
+}
+
+func (d DuplicateBlocksPolicy) Bool() bool {
+	// duplicates should be returned only when explicitly requested,
+	// so any other state than DuplicateBlocksIncluded should return false
+	return d == DuplicateBlocksIncluded
+}
+
+func (d DuplicateBlocksPolicy) String() string {
+	switch d {
+	case DuplicateBlocksIncluded:
+		return "y"
+	case DuplicateBlocksExcluded:
+		return "n"
+	}
+	return ""
+}
+
 type ContentPathMetadata struct {
 	PathSegmentRoots []cid.Cid
 	LastSegment      path.Resolved
@@ -287,11 +324,8 @@ type IPFSBackend interface {
 	ResolvePath(context.Context, ImmutablePath) (ContentPathMetadata, error)
 
 	// GetCAR returns a CAR file for the given immutable path. It returns an error
-	// if there was an issue before the CAR streaming begins. If [CarParams.Duplicates]
-	// is nil, or if [CaraParams.Order] is Unknown, the implementer should change it
-	// such that the caller can form the response "Content-Type" header with the most
-	// amount of information.
-	GetCAR(context.Context, ImmutablePath, *CarParams) (ContentPathMetadata, io.ReadCloser, error)
+	// if there was an issue before the CAR streaming begins.
+	GetCAR(context.Context, ImmutablePath, CarParams) (ContentPathMetadata, io.ReadCloser, error)
 
 	// IsCached returns whether or not the path exists locally.
 	IsCached(context.Context, path.Path) bool

gateway/gateway_test.go

@@ -669,7 +669,7 @@ func (mb *errorMockBackend) Head(ctx context.Context, path ImmutablePath) (Conte
 	return ContentPathMetadata{}, nil, mb.err
 }
 
-func (mb *errorMockBackend) GetCAR(ctx context.Context, path ImmutablePath, params *CarParams) (ContentPathMetadata, io.ReadCloser, error) {
+func (mb *errorMockBackend) GetCAR(ctx context.Context, path ImmutablePath, params CarParams) (ContentPathMetadata, io.ReadCloser, error) {
 	return ContentPathMetadata{}, nil, mb.err
 }
 
@@ -753,7 +753,7 @@ func (mb *panicMockBackend) Head(ctx context.Context, immutablePath ImmutablePat
 	panic("i am panicking")
 }
 
-func (mb *panicMockBackend) GetCAR(ctx context.Context, immutablePath ImmutablePath, params *CarParams) (ContentPathMetadata, io.ReadCloser, error) {
+func (mb *panicMockBackend) GetCAR(ctx context.Context, immutablePath ImmutablePath, params CarParams) (ContentPathMetadata, io.ReadCloser, error) {
 	panic("i am panicking")
 }
 

gateway/handler.go

@@ -637,28 +637,9 @@ const (
 
 // return explicit response format if specified in request as query parameter or via Accept HTTP header
 func customResponseFormat(r *http.Request) (mediaType string, params map[string]string, err error) {
-	// Translate query param to a content type, if present.
-	if formatParam := r.URL.Query().Get("format"); formatParam != "" {
-		switch formatParam {
-		case "raw":
-			return rawResponseFormat, nil, nil
-		case "car":
-			return carResponseFormat, nil, nil
-		case "tar":
-			return tarResponseFormat, nil, nil
-		case "json":
-			return jsonResponseFormat, nil, nil
-		case "cbor":
-			return cborResponseFormat, nil, nil
-		case "dag-json":
-			return dagJsonResponseFormat, nil, nil
-		case "dag-cbor":
-			return dagCborResponseFormat, nil, nil
-		case "ipns-record":
-			return ipnsRecordResponseFormat, nil, nil
-		}
-	}
-
+	// First, inspect Accept header, as it may not only include content type, but also optional parameters.
+	// such as CAR version or additional ones from IPIP-412.
+	//
 	// Browsers and other user agents will send Accept header with generic types like:
 	// Accept:text/html,application/xhtml+xml,application/xml;q=0.9,image/avif,image/webp,*/*;q=0.8
 	// We only care about explicit, vendor-specific content-types and respond to the first match (in order).
@@ -681,6 +662,28 @@ func customResponseFormat(r *http.Request) (mediaType string, params map[string]
 		}
 	}
 
+	// If no Accept header, translate query param to a content type, if present.
+	if formatParam := r.URL.Query().Get("format"); formatParam != "" {
+		switch formatParam {
+		case "raw":
+			return rawResponseFormat, nil, nil
+		case "car":
+			return carResponseFormat, nil, nil
+		case "tar":
+			return tarResponseFormat, nil, nil
+		case "json":
+			return jsonResponseFormat, nil, nil
+		case "cbor":
+			return cborResponseFormat, nil, nil
+		case "dag-json":
+			return dagJsonResponseFormat, nil, nil
+		case "dag-cbor":
+			return dagCborResponseFormat, nil, nil
+		case "ipns-record":
+			return ipnsRecordResponseFormat, nil, nil
+		}
+	}
+
 	// If none of special-cased content types is found, return empty string
 	// to indicate default, implicit UnixFS response should be prepared
 	return "", nil, nil

gateway/handler_car.go

@@ -30,16 +30,7 @@ func (i *handler) serveCAR(ctx context.Context, w http.ResponseWriter, r *http.R
 	ctx, cancel := context.WithCancel(ctx)
 	defer cancel()
 
-	switch rq.responseParams["version"] {
-	case "": // noop, client does not care about version
-	case "1": // noop, we support this
-	default:
-		err := fmt.Errorf("unsupported CAR version: only version=1 is supported")
-		i.webError(w, r, err, http.StatusBadRequest)
-		return false
-	}
-
-	params, err := getCarParams(r, rq.responseParams)
+	params, err := buildCarParams(r, rq.responseParams)
 	if err != nil {
 		i.webError(w, r, err, http.StatusBadRequest)
 		return false
@@ -90,7 +81,7 @@ func (i *handler) serveCAR(ctx context.Context, w http.ResponseWriter, r *http.R
 	// sub-DAGs and IPLD selectors: https://github.com/ipfs/go-ipfs/issues/8769
 	w.Header().Set("Accept-Ranges", "none")
 
-	w.Header().Set("Content-Type", getContentTypeFromCarParams(params))
+	w.Header().Set("Content-Type", buildContentTypeFromCarParams(params))
 	w.Header().Set("X-Content-Type-Options", "nosniff") // no funny business in the browsers :^)
 
 	_, copyErr := io.Copy(w, carFile)
@@ -113,7 +104,15 @@ func (i *handler) serveCAR(ctx context.Context, w http.ResponseWriter, r *http.R
 	return true
 }
 
-func getCarParams(r *http.Request, formatParams map[string]string) (*CarParams, error) {
+// buildCarParams returns CarParams based on the request, any optional parameters
+// passed in URL, Accept header and the implicit defaults specific to boxo
+// implementation, such as block order and duplicates status.
+//
+// If any of the optional content type parameters (e.g., CAR order or
+// duplicates) are unspecified or empty, the function will automatically infer
+// default values.
+func buildCarParams(r *http.Request, contentTypeParams map[string]string) (CarParams, error) {
+	// URL query parameters
 	queryParams := r.URL.Query()
 	rangeStr, hasRange := queryParams.Get(carRangeBytesKey), queryParams.Has(carRangeBytesKey)
 	scopeStr, hasScope := queryParams.Get(carTerminalElementTypeKey), queryParams.Has(carTerminalElementTypeKey)
@@ -123,7 +122,7 @@ func getCarParams(r *http.Request, formatParams map[string]string) (*CarParams,
 		rng, err := NewDagByteRange(rangeStr)
 		if err != nil {
 			err = fmt.Errorf("invalid application/vnd.ipld.car entity-bytes URL parameter: %w", err)
-			return nil, err
+			return CarParams{}, err
 		}
 		params.Range = &rng
 	}
@@ -134,55 +133,72 @@ func getCarParams(r *http.Request, formatParams map[string]string) (*CarParams,
 			params.Scope = s
 		default:
 			err := fmt.Errorf("unsupported application/vnd.ipld.car dag-scope URL parameter: %q", scopeStr)
-			return nil, err
+			return CarParams{}, err
 		}
 	} else {
 		params.Scope = DagScopeAll
 	}
 
-	switch order := DagOrder(formatParams["order"]); order {
-	case DagOrderUnknown, DagOrderDFS:
-		params.Order = order
-	case "":
-		params.Order = DagOrderUnknown
-	default:
-		return nil, fmt.Errorf("unsupported application/vnd.ipld.car content type order parameter: %q", order)
-	}
-
-	switch dups := formatParams["dups"]; dups {
-	case "y":
-		v := true
-		params.Duplicates = &v
-	case "n":
-		v := false
-		params.Duplicates = &v
-	case "":
-		// Acceptable, we do not set anything.
+	// application/vnd.ipld.car content type parameters from Accept header
+
+	// version of CAR format
+	switch contentTypeParams["version"] {
+	case "": // noop, client does not care about version
+	case "1": // noop, we support this
 	default:
-		return nil, fmt.Errorf("unsupported application/vnd.ipld.car content type  dups parameter:  %q", dups)
+		return CarParams{}, fmt.Errorf("unsupported application/vnd.ipld.car version: only version=1 is supported")
+	}
+
+	// optional order from IPIP-412
+	if order := DagOrder(contentTypeParams["order"]); order != DagOrderUnspecified {
+		switch order {
+		case DagOrderUnknown, DagOrderDFS:
+			params.Order = order
+		default:
+			return CarParams{}, fmt.Errorf("unsupported application/vnd.ipld.car content type order parameter: %q", order)
+		}
+	} else {
+		// when order is not specified, we use DFS as the implicit default
+		// as this has always been the default behavior and we should not break
+		// legacy clients
+		params.Order = DagOrderDFS
+	}
+
+	// optional dups from IPIP-412
+	if dups := NewDuplicateBlocksPolicy(contentTypeParams["dups"]); dups != DuplicateBlocksUnspecified {
+		switch dups {
+		case DuplicateBlocksExcluded, DuplicateBlocksIncluded:
+			params.Duplicates = dups
+		default:
+			return CarParams{}, fmt.Errorf("unsupported application/vnd.ipld.car content type dups parameter:  %q", dups)
+		}
+	} else {
+		// when duplicate block preference is not specified, we set it to
+		// false, as this has always been the default behavior, we should
+		// not break legacy clients, and responses to requests made via ?format=car
+		// should benefit from block deduplication
+		params.Duplicates = DuplicateBlocksExcluded
+
 	}
 
-	return &params, nil
+	return params, nil
 }
 
-func getContentTypeFromCarParams(params *CarParams) string {
+// buildContentTypeFromCarParams returns a string for Content-Type header.
+// It does not change any values, CarParams are respected as-is.
+func buildContentTypeFromCarParams(params CarParams) string {
 	h := strings.Builder{}
 	h.WriteString(carResponseFormat)
-	h.WriteString("; version=1; order=")
+	h.WriteString("; version=1")
 
-	if params.Order != "" {
+	if params.Order != DagOrderUnspecified {
+		h.WriteString("; order=")
 		h.WriteString(string(params.Order))
-	} else {
-		h.WriteString(string(DagOrderUnknown))
 	}
 
-	if params.Duplicates != nil {
+	if params.Duplicates != DuplicateBlocksUnspecified {
 		h.WriteString("; dups=")
-		if *params.Duplicates {
-			h.WriteString("y")
-		} else {
-			h.WriteString("n")
-		}
+		h.WriteString(params.Duplicates.String())
 	}
 
 	return h.String()
@@ -209,17 +225,28 @@ func getCarRootCidAndLastSegment(imPath ImmutablePath) (cid.Cid, string, error)
 	return rootCid, lastSegment, err
 }
 
-func getCarEtag(imPath ImmutablePath, params *CarParams, rootCid cid.Cid) string {
+func getCarEtag(imPath ImmutablePath, params CarParams, rootCid cid.Cid) string {
 	data := imPath.String()
 	if params.Scope != DagScopeAll {
-		data += "." + string(params.Scope)
+		data += string(params.Scope)
+	}
+
+	// 'order' from IPIP-412 impact Etag only if set to something else
+	// than DFS (which is the implicit default)
+	if params.Order != DagOrderDFS {
+		data += string(params.Order)
+	}
+
+	// 'dups' from IPIP-412 impact Etag only if 'y'
+	if dups := params.Duplicates.String(); dups == "y" {
+		data += dups
 	}
 
 	if params.Range != nil {
 		if params.Range.From != 0 || params.Range.To != nil {
-			data += "." + strconv.FormatInt(params.Range.From, 10)
+			data += strconv.FormatInt(params.Range.From, 10)
 			if params.Range.To != nil {
-				data += "." + strconv.FormatInt(*params.Range.To, 10)
+				data += strconv.FormatInt(*params.Range.To, 10)
 			}
 		}
 	}