Skip to content

Commit c033fe5

Browse files
lidelhacdias
lidel
authored and
hacdias
committed
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
1 parent 0fa23f5 commit c033fe5

8 files changed

Lines changed: 186 additions & 139 deletions

File tree

‎gateway/blocks_backend.go‎

Lines changed: 4 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -232,25 +232,7 @@ func (bb *BlocksBackend) Head(ctx context.Context, path ImmutablePath) (ContentP
232232
return md, fileNode, nil
233233
}
234234

235-
func (bb *BlocksBackend) GetCAR(ctx context.Context, p ImmutablePath, params *CarParams) (ContentPathMetadata, io.ReadCloser, error) {
236-
// Check if we support the request order. On unknown, change it to DFS. We change
237-
// the parameter directly, which means that the caller can use the value to later construct
238-
// the Content-Type header.
239-
switch params.Order {
240-
case DagOrderUnknown:
241-
params.Order = DagOrderDFS
242-
case DagOrderDFS:
243-
// Do nothing
244-
default:
245-
return ContentPathMetadata{}, nil, fmt.Errorf("unsupported application/vnd.ipld.car block order parameter: %q", params.Order)
246-
}
247-
248-
// Similarly, if params.Duplicates is not set, let's set it to false.
249-
if params.Duplicates == nil {
250-
v := false
251-
params.Duplicates = &v
252-
}
253-
235+
func (bb *BlocksBackend) GetCAR(ctx context.Context, p ImmutablePath, params CarParams) (ContentPathMetadata, io.ReadCloser, error) {
254236
pathMetadata, err := bb.ResolvePath(ctx, p)
255237
if err != nil {
256238
return ContentPathMetadata{}, nil, err
@@ -267,7 +249,7 @@ func (bb *BlocksBackend) GetCAR(ctx context.Context, p ImmutablePath, params *Ca
267249
w,
268250
[]cid.Cid{pathMetadata.LastSegment.Cid()},
269251
car.WriteAsCarV1(true),
270-
car.AllowDuplicatePuts(*params.Duplicates),
252+
car.AllowDuplicatePuts(params.Duplicates.Bool()),
271253
)
272254
if err != nil {
273255
// io.PipeWriter.CloseWithError always returns nil.
@@ -302,7 +284,7 @@ func (bb *BlocksBackend) GetCAR(ctx context.Context, p ImmutablePath, params *Ca
302284
}
303285

304286
// walkGatewaySimpleSelector walks the subgraph described by the path and terminal element parameters
305-
func walkGatewaySimpleSelector(ctx context.Context, p ipfspath.Path, params *CarParams, lsys *ipld.LinkSystem, pathResolver resolver.Resolver) error {
287+
func walkGatewaySimpleSelector(ctx context.Context, p ipfspath.Path, params CarParams, lsys *ipld.LinkSystem, pathResolver resolver.Resolver) error {
306288
// First resolve the path since we always need to.
307289
lastCid, remainder, err := pathResolver.ResolveToLastNode(ctx, p)
308290
if err != nil {
@@ -335,7 +317,7 @@ func walkGatewaySimpleSelector(ctx context.Context, p ipfspath.Path, params *Car
335317
Ctx: ctx,
336318
LinkSystem: *lsys,
337319
LinkTargetNodePrototypeChooser: bsfetcher.DefaultPrototypeChooser,
338-
LinkVisitOnlyOnce: !*params.Duplicates,
320+
LinkVisitOnlyOnce: !params.Duplicates.Bool(),
339321
},
340322
}
341323

‎gateway/gateway.go‎

Lines changed: 42 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -124,7 +124,7 @@ type CarParams struct {
124124
Range *DagByteRange
125125
Scope DagScope
126126
Order DagOrder
127-
Duplicates *bool
127+
Duplicates DuplicateBlocksPolicy
128128
}
129129

130130
// DagByteRange describes a range request within a UnixFS file. "From" and
@@ -194,10 +194,47 @@ const (
194194
type DagOrder string
195195

196196
const (
197-
DagOrderDFS DagOrder = "dfs"
198-
DagOrderUnknown DagOrder = "unk"
197+
DagOrderUnspecified DagOrder = ""
198+
DagOrderUnknown DagOrder = "unk"
199+
DagOrderDFS DagOrder = "dfs"
199200
)
200201

202+
// DuplicateBlocksPolicy represents the content type parameter 'dups' (IPIP-412)
203+
type DuplicateBlocksPolicy int
204+
205+
const (
206+
DuplicateBlocksUnspecified DuplicateBlocksPolicy = iota // 0 - implicit default
207+
DuplicateBlocksIncluded // 1 - explicitly include duplicates
208+
DuplicateBlocksExcluded // 2 - explicitly NOT include duplicates
209+
)
210+
211+
// NewDuplicateBlocksPolicy returns DuplicateBlocksPolicy based on the content type parameter 'dups' (IPIP-412)
212+
func NewDuplicateBlocksPolicy(dupsValue string) DuplicateBlocksPolicy {
213+
switch dupsValue {
214+
case "y":
215+
return DuplicateBlocksIncluded
216+
case "n":
217+
return DuplicateBlocksExcluded
218+
}
219+
return DuplicateBlocksUnspecified
220+
}
221+
222+
func (d DuplicateBlocksPolicy) Bool() bool {
223+
// duplicates should be returned only when explicitly requested,
224+
// so any other state than DuplicateBlocksIncluded should return false
225+
return d == DuplicateBlocksIncluded
226+
}
227+
228+
func (d DuplicateBlocksPolicy) String() string {
229+
switch d {
230+
case DuplicateBlocksIncluded:
231+
return "y"
232+
case DuplicateBlocksExcluded:
233+
return "n"
234+
}
235+
return ""
236+
}
237+
201238
type ContentPathMetadata struct {
202239
PathSegmentRoots []cid.Cid
203240
LastSegment path.Resolved
@@ -287,11 +324,8 @@ type IPFSBackend interface {
287324
ResolvePath(context.Context, ImmutablePath) (ContentPathMetadata, error)
288325

289326
// GetCAR returns a CAR file for the given immutable path. It returns an error
290-
// if there was an issue before the CAR streaming begins. If [CarParams.Duplicates]
291-
// is nil, or if [CaraParams.Order] is Unknown, the implementer should change it
292-
// such that the caller can form the response "Content-Type" header with the most
293-
// amount of information.
294-
GetCAR(context.Context, ImmutablePath, *CarParams) (ContentPathMetadata, io.ReadCloser, error)
327+
// if there was an issue before the CAR streaming begins.
328+
GetCAR(context.Context, ImmutablePath, CarParams) (ContentPathMetadata, io.ReadCloser, error)
295329

296330
// IsCached returns whether or not the path exists locally.
297331
IsCached(context.Context, path.Path) bool

‎gateway/gateway_test.go‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -669,7 +669,7 @@ func (mb *errorMockBackend) Head(ctx context.Context, path ImmutablePath) (Conte
669669
return ContentPathMetadata{}, nil, mb.err
670670
}
671671

672-
func (mb *errorMockBackend) GetCAR(ctx context.Context, path ImmutablePath, params *CarParams) (ContentPathMetadata, io.ReadCloser, error) {
672+
func (mb *errorMockBackend) GetCAR(ctx context.Context, path ImmutablePath, params CarParams) (ContentPathMetadata, io.ReadCloser, error) {
673673
return ContentPathMetadata{}, nil, mb.err
674674
}
675675

@@ -753,7 +753,7 @@ func (mb *panicMockBackend) Head(ctx context.Context, immutablePath ImmutablePat
753753
panic("i am panicking")
754754
}
755755

756-
func (mb *panicMockBackend) GetCAR(ctx context.Context, immutablePath ImmutablePath, params *CarParams) (ContentPathMetadata, io.ReadCloser, error) {
756+
func (mb *panicMockBackend) GetCAR(ctx context.Context, immutablePath ImmutablePath, params CarParams) (ContentPathMetadata, io.ReadCloser, error) {
757757
panic("i am panicking")
758758
}
759759

‎gateway/handler.go‎

Lines changed: 25 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -637,28 +637,9 @@ const (
637637

638638
// return explicit response format if specified in request as query parameter or via Accept HTTP header
639639
func customResponseFormat(r *http.Request) (mediaType string, params map[string]string, err error) {
640-
// Translate query param to a content type, if present.
641-
if formatParam := r.URL.Query().Get("format"); formatParam != "" {
642-
switch formatParam {
643-
case "raw":
644-
return rawResponseFormat, nil, nil
645-
case "car":
646-
return carResponseFormat, nil, nil
647-
case "tar":
648-
return tarResponseFormat, nil, nil
649-
case "json":
650-
return jsonResponseFormat, nil, nil
651-
case "cbor":
652-
return cborResponseFormat, nil, nil
653-
case "dag-json":
654-
return dagJsonResponseFormat, nil, nil
655-
case "dag-cbor":
656-
return dagCborResponseFormat, nil, nil
657-
case "ipns-record":
658-
return ipnsRecordResponseFormat, nil, nil
659-
}
660-
}
661-
640+
// First, inspect Accept header, as it may not only include content type, but also optional parameters.
641+
// such as CAR version or additional ones from IPIP-412.
642+
//
662643
// Browsers and other user agents will send Accept header with generic types like:
663644
// Accept:text/html,application/xhtml+xml,application/xml;q=0.9,image/avif,image/webp,*/*;q=0.8
664645
// 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]
681662
}
682663
}
683664

665+
// If no Accept header, translate query param to a content type, if present.
666+
if formatParam := r.URL.Query().Get("format"); formatParam != "" {
667+
switch formatParam {
668+
case "raw":
669+
return rawResponseFormat, nil, nil
670+
case "car":
671+
return carResponseFormat, nil, nil
672+
case "tar":
673+
return tarResponseFormat, nil, nil
674+
case "json":
675+
return jsonResponseFormat, nil, nil
676+
case "cbor":
677+
return cborResponseFormat, nil, nil
678+
case "dag-json":
679+
return dagJsonResponseFormat, nil, nil
680+
case "dag-cbor":
681+
return dagCborResponseFormat, nil, nil
682+
case "ipns-record":
683+
return ipnsRecordResponseFormat, nil, nil
684+
}
685+
}
686+
684687
// If none of special-cased content types is found, return empty string
685688
// to indicate default, implicit UnixFS response should be prepared
686689
return "", nil, nil

‎gateway/handler_car.go‎

Lines changed: 76 additions & 49 deletions
Original file line numberDiff line numberDiff line change
@@ -30,16 +30,7 @@ func (i *handler) serveCAR(ctx context.Context, w http.ResponseWriter, r *http.R
3030
ctx, cancel := context.WithCancel(ctx)
3131
defer cancel()
3232

33-
switch rq.responseParams["version"] {
34-
case "": // noop, client does not care about version
35-
case "1": // noop, we support this
36-
default:
37-
err := fmt.Errorf("unsupported CAR version: only version=1 is supported")
38-
i.webError(w, r, err, http.StatusBadRequest)
39-
return false
40-
}
41-
42-
params, err := getCarParams(r, rq.responseParams)
33+
params, err := buildCarParams(r, rq.responseParams)
4334
if err != nil {
4435
i.webError(w, r, err, http.StatusBadRequest)
4536
return false
@@ -90,7 +81,7 @@ func (i *handler) serveCAR(ctx context.Context, w http.ResponseWriter, r *http.R
9081
// sub-DAGs and IPLD selectors: https://github.com/ipfs/go-ipfs/issues/8769
9182
w.Header().Set("Accept-Ranges", "none")
9283

93-
w.Header().Set("Content-Type", getContentTypeFromCarParams(params))
84+
w.Header().Set("Content-Type", buildContentTypeFromCarParams(params))
9485
w.Header().Set("X-Content-Type-Options", "nosniff") // no funny business in the browsers :^)
9586

9687
_, copyErr := io.Copy(w, carFile)
@@ -113,7 +104,15 @@ func (i *handler) serveCAR(ctx context.Context, w http.ResponseWriter, r *http.R
113104
return true
114105
}
115106

116-
func getCarParams(r *http.Request, formatParams map[string]string) (*CarParams, error) {
107+
// buildCarParams returns CarParams based on the request, any optional parameters
108+
// passed in URL, Accept header and the implicit defaults specific to boxo
109+
// implementation, such as block order and duplicates status.
110+
//
111+
// If any of the optional content type parameters (e.g., CAR order or
112+
// duplicates) are unspecified or empty, the function will automatically infer
113+
// default values.
114+
func buildCarParams(r *http.Request, contentTypeParams map[string]string) (CarParams, error) {
115+
// URL query parameters
117116
queryParams := r.URL.Query()
118117
rangeStr, hasRange := queryParams.Get(carRangeBytesKey), queryParams.Has(carRangeBytesKey)
119118
scopeStr, hasScope := queryParams.Get(carTerminalElementTypeKey), queryParams.Has(carTerminalElementTypeKey)
@@ -123,7 +122,7 @@ func getCarParams(r *http.Request, formatParams map[string]string) (*CarParams,
123122
rng, err := NewDagByteRange(rangeStr)
124123
if err != nil {
125124
err = fmt.Errorf("invalid application/vnd.ipld.car entity-bytes URL parameter: %w", err)
126-
return nil, err
125+
return CarParams{}, err
127126
}
128127
params.Range = &rng
129128
}
@@ -134,55 +133,72 @@ func getCarParams(r *http.Request, formatParams map[string]string) (*CarParams,
134133
params.Scope = s
135134
default:
136135
err := fmt.Errorf("unsupported application/vnd.ipld.car dag-scope URL parameter: %q", scopeStr)
137-
return nil, err
136+
return CarParams{}, err
138137
}
139138
} else {
140139
params.Scope = DagScopeAll
141140
}
142141

143-
switch order := DagOrder(formatParams["order"]); order {
144-
case DagOrderUnknown, DagOrderDFS:
145-
params.Order = order
146-
case "":
147-
params.Order = DagOrderUnknown
148-
default:
149-
return nil, fmt.Errorf("unsupported application/vnd.ipld.car content type order parameter: %q", order)
150-
}
151-
152-
switch dups := formatParams["dups"]; dups {
153-
case "y":
154-
v := true
155-
params.Duplicates = &v
156-
case "n":
157-
v := false
158-
params.Duplicates = &v
159-
case "":
160-
// Acceptable, we do not set anything.
142+
// application/vnd.ipld.car content type parameters from Accept header
143+
144+
// version of CAR format
145+
switch contentTypeParams["version"] {
146+
case "": // noop, client does not care about version
147+
case "1": // noop, we support this
161148
default:
162-
return nil, fmt.Errorf("unsupported application/vnd.ipld.car content type dups parameter: %q", dups)
149+
return CarParams{}, fmt.Errorf("unsupported application/vnd.ipld.car version: only version=1 is supported")
150+
}
151+
152+
// optional order from IPIP-412
153+
if order := DagOrder(contentTypeParams["order"]); order != DagOrderUnspecified {
154+
switch order {
155+
case DagOrderUnknown, DagOrderDFS:
156+
params.Order = order
157+
default:
158+
return CarParams{}, fmt.Errorf("unsupported application/vnd.ipld.car content type order parameter: %q", order)
159+
}
160+
} else {
161+
// when order is not specified, we use DFS as the implicit default
162+
// as this has always been the default behavior and we should not break
163+
// legacy clients
164+
params.Order = DagOrderDFS
165+
}
166+
167+
// optional dups from IPIP-412
168+
if dups := NewDuplicateBlocksPolicy(contentTypeParams["dups"]); dups != DuplicateBlocksUnspecified {
169+
switch dups {
170+
case DuplicateBlocksExcluded, DuplicateBlocksIncluded:
171+
params.Duplicates = dups
172+
default:
173+
return CarParams{}, fmt.Errorf("unsupported application/vnd.ipld.car content type dups parameter: %q", dups)
174+
}
175+
} else {
176+
// when duplicate block preference is not specified, we set it to
177+
// false, as this has always been the default behavior, we should
178+
// not break legacy clients, and responses to requests made via ?format=car
179+
// should benefit from block deduplication
180+
params.Duplicates = DuplicateBlocksExcluded
181+
163182
}
164183

165-
return &params, nil
184+
return params, nil
166185
}
167186

168-
func getContentTypeFromCarParams(params *CarParams) string {
187+
// buildContentTypeFromCarParams returns a string for Content-Type header.
188+
// It does not change any values, CarParams are respected as-is.
189+
func buildContentTypeFromCarParams(params CarParams) string {
169190
h := strings.Builder{}
170191
h.WriteString(carResponseFormat)
171-
h.WriteString("; version=1; order=")
192+
h.WriteString("; version=1")
172193

173-
if params.Order != "" {
194+
if params.Order != DagOrderUnspecified {
195+
h.WriteString("; order=")
174196
h.WriteString(string(params.Order))
175-
} else {
176-
h.WriteString(string(DagOrderUnknown))
177197
}
178198

179-
if params.Duplicates != nil {
199+
if params.Duplicates != DuplicateBlocksUnspecified {
180200
h.WriteString("; dups=")
181-
if *params.Duplicates {
182-
h.WriteString("y")
183-
} else {
184-
h.WriteString("n")
185-
}
201+
h.WriteString(params.Duplicates.String())
186202
}
187203

188204
return h.String()
@@ -209,17 +225,28 @@ func getCarRootCidAndLastSegment(imPath ImmutablePath) (cid.Cid, string, error)
209225
return rootCid, lastSegment, err
210226
}
211227

212-
func getCarEtag(imPath ImmutablePath, params *CarParams, rootCid cid.Cid) string {
228+
func getCarEtag(imPath ImmutablePath, params CarParams, rootCid cid.Cid) string {
213229
data := imPath.String()
214230
if params.Scope != DagScopeAll {
215-
data += "." + string(params.Scope)
231+
data += string(params.Scope)
232+
}
233+
234+
// 'order' from IPIP-412 impact Etag only if set to something else
235+
// than DFS (which is the implicit default)
236+
if params.Order != DagOrderDFS {
237+
data += string(params.Order)
238+
}
239+
240+
// 'dups' from IPIP-412 impact Etag only if 'y'
241+
if dups := params.Duplicates.String(); dups == "y" {
242+
data += dups
216243
}
217244

218245
if params.Range != nil {
219246
if params.Range.From != 0 || params.Range.To != nil {
220-
data += "." + strconv.FormatInt(params.Range.From, 10)
247+
data += strconv.FormatInt(params.Range.From, 10)
221248
if params.Range.To != nil {
222-
data += "." + strconv.FormatInt(*params.Range.To, 10)
249+
data += strconv.FormatInt(*params.Range.To, 10)
223250
}
224251
}
225252
}

0 commit comments

Comments
 (0)