refactor(gw): merge IPFSBackend.Get and GetRange

#240 (review)

gateway/blocks_gateway.go

@@ -139,7 +139,7 @@ func NewBlocksGateway(blockService blockservice.BlockService, opts ...BlockGatew
 	}, nil
 }
 
-func (api *BlocksGateway) Get(ctx context.Context, path ImmutablePath) (ContentPathMetadata, *GetResponse, error) {
+func (api *BlocksGateway) Get(ctx context.Context, path ImmutablePath, ranges ...ByteRange) (ContentPathMetadata, *GetResponse, error) {
 	md, nd, err := api.getNode(ctx, path)
 	if err != nil {
 		return md, nil, err
@@ -180,10 +180,6 @@ func (api *BlocksGateway) Get(ctx context.Context, path ImmutablePath) (ContentP
 	return ContentPathMetadata{}, nil, fmt.Errorf("data was not a valid file or directory: %w", ErrInternalServerError) // TODO: should there be a gateway invalid content type to abstract over the various IPLD error types?
 }
 
-func (api *BlocksGateway) GetRange(ctx context.Context, path ImmutablePath, ranges ...GetRange) (ContentPathMetadata, *GetResponse, error) {
-	return api.Get(ctx, path)
-}
-
 func (api *BlocksGateway) GetAll(ctx context.Context, path ImmutablePath) (ContentPathMetadata, files.Node, error) {
 	md, nd, err := api.getNode(ctx, path)
 	if err != nil {

gateway/gateway.go

@@ -54,11 +54,13 @@ type ContentPathMetadata struct {
 	ContentType      string // Only used for UnixFS requests
 }
 
-// GetRange describes a range request within a UnixFS file. From and To mostly follow HTTP Range Request semantics.
+// ByteRange describes a range request within a UnixFS file. From and To mostly follow [HTTP Byte Range] Request semantics.
 // From >= 0 and To = nil: Get the file (From, Length)
 // From >= 0 and To >= 0: Get the range (From, To)
 // From >= 0 and To <0: Get the range (From, Length - To)
-type GetRange struct {
+//
+// [HTTP Byte Range]: https://httpwg.org/specs/rfc9110.html#rfc.section.14.1.2
+type ByteRange struct {
 	From uint64
 	To   *int64
 }
@@ -86,16 +88,24 @@ func NewGetResponseFromDirectoryListing(dagSize uint64, entries <-chan unixfs.Li
 // There are also some existing error types that the gateway code knows how to handle (e.g. context.DeadlineExceeded
 // and various IPLD pathing related errors).
 type IPFSBackend interface {
-	// Get returns a UnixFS file, UnixFS directory, or an IPLD block depending on what the path is that has been
-	// requested. Directories' files.DirEntry objects do not need to contain content, but must contain Name,
-	// Size, and Cid.
-	Get(context.Context, ImmutablePath) (ContentPathMetadata, *GetResponse, error)
-
-	// GetRange returns a full UnixFS file object, or a UnixFS directory. Ranges passed in are advisory for pre-fetching
-	// data, however consumers of this function may require extra data beyond the passed ranges (e.g. the initial bit of
-	// the file might be used for content type sniffing even if only the end of the file is requested).
-	// Note: there is currently no semantic meaning attached to a range request for a directory
-	GetRange(context.Context, ImmutablePath, ...GetRange) (ContentPathMetadata, *GetResponse, error)
+
+	// Get returns a GetResponse with UnixFS file, directory or a block in IPLD
+	// format e.g., (DAG-)CBOR/JSON.
+	//
+	// Returned Directories are preferably a minimum info required for enumeration: Name, Size, and Cid.
+	//
+	// Optional ranges follow [HTTP Byte Ranges] notation and can be used for
+	// pre-fetching specific sections of a file or a block.
+	//
+	// Range notes:
+	//   - Generating response to a range request may require additional data
+	//     beyond the passed ranges (e.g. a single byte range from the middle of a
+	//     file will still need magic bytes from the very beginning for content
+	//     type sniffing).
+	//   - A range request for a directory currently holds no semantic meaning.
+	//
+	// [HTTP Byte Ranges]: https://httpwg.org/specs/rfc9110.html#rfc.section.14.1.2
+	Get(context.Context, ImmutablePath, ...ByteRange) (ContentPathMetadata, *GetResponse, error)
 
 	// GetAll returns a UnixFS file or directory depending on what the path is that has been requested. Directories should
 	// include all content recursively.

gateway/gateway_test.go

@@ -107,12 +107,8 @@ func newMockAPI(t *testing.T) (*mockAPI, cid.Cid) {
 	}, cids[0]
 }
 
-func (api *mockAPI) Get(ctx context.Context, immutablePath ImmutablePath) (ContentPathMetadata, *GetResponse, error) {
-	return api.gw.Get(ctx, immutablePath)
-}
-
-func (api *mockAPI) GetRange(ctx context.Context, immutablePath ImmutablePath, ranges ...GetRange) (ContentPathMetadata, *GetResponse, error) {
-	return api.gw.GetRange(ctx, immutablePath, ranges...)
+func (api *mockAPI) Get(ctx context.Context, immutablePath ImmutablePath, ranges ...ByteRange) (ContentPathMetadata, *GetResponse, error) {
+	return api.gw.Get(ctx, immutablePath, ranges...)
 }
 
 func (api *mockAPI) GetAll(ctx context.Context, immutablePath ImmutablePath) (ContentPathMetadata, files.Node, error) {

gateway/handler_defaults.go

@@ -29,7 +29,7 @@ func (i *handler) serveDefaults(ctx context.Context, w http.ResponseWriter, r *h
 		isDirectoryHeadRequest bool
 		directoryMetadata      *directoryMetadata
 		err                    error
-		ranges                 []GetRange
+		ranges                 []ByteRange
 	)
 
 	switch r.Method {
@@ -60,23 +60,19 @@ func (i *handler) serveDefaults(ctx context.Context, w http.ResponseWriter, r *h
 		}
 
 		var getResp *GetResponse
-		// TODO: passing resolved path here, instead of contentPath is harming content routing. Knowing original immutableContentPath will allow backend to find  providers for parents, even when internal CIDs are not announced, and will provide better key for caching related DAGs.
-		if len(ranges) == 0 {
-			pathMetadata, getResp, err = i.api.Get(ctx, maybeResolvedImPath)
-		} else {
-			pathMetadata, getResp, err = i.api.GetRange(ctx, maybeResolvedImPath, ranges...)
-		}
+		// TODO: passing only resolved path here, instead of contentPath is
+		// harming content routing. Knowing original immutableContentPath will
+		// 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.api.Get(ctx, maybeResolvedImPath, ranges...)
 		if err != nil {
 			if isWebRequest(requestedContentType) {
 				forwardedPath, continueProcessing := i.handleWebRequestErrors(w, r, maybeResolvedImPath, immutableContentPath, contentPath, err, logger)
 				if !continueProcessing {
 					return false
 				}
-				if len(ranges) == 0 {
-					pathMetadata, getResp, err = i.api.Get(ctx, forwardedPath)
-				} else {
-					pathMetadata, getResp, err = i.api.GetRange(ctx, forwardedPath, ranges...)
-				}
+				pathMetadata, getResp, err = i.api.Get(ctx, forwardedPath, ranges...)
 				if err != nil {
 					err = fmt.Errorf("failed to resolve %s: %w", debugStr(contentPath.String()), err)
 					webError(w, err, http.StatusInternalServerError)
@@ -100,7 +96,7 @@ func (i *handler) serveDefaults(ctx context.Context, w http.ResponseWriter, r *h
 		return false
 	}
 
-	// TODO: check if we have a bug when maybeResolvedImPath is resolved and i.setIpfsRootsHeader works with pathMetadata returned by GetRange(maybeResolvedImPath)
+	// TODO: check if we have a bug when maybeResolvedImPath is resolved and i.setIpfsRootsHeader works with pathMetadata returned by Get(maybeResolvedImPath)
 	if err := i.setIpfsRootsHeader(w, pathMetadata); err != nil {
 		webRequestError(w, err)
 		return false
@@ -138,15 +134,15 @@ func (i *handler) serveDefaults(ctx context.Context, w http.ResponseWriter, r *h
 }
 
 // parseRange parses a Range header string as per RFC 7233.
-func parseRange(s string) ([]GetRange, error) {
+func parseRange(s string) ([]ByteRange, error) {
 	if s == "" {
 		return nil, nil // header not present
 	}
 	const b = "bytes="
 	if !strings.HasPrefix(s, b) {
 		return nil, errors.New("invalid range")
 	}
-	var ranges []GetRange
+	var ranges []ByteRange
 	for _, ra := range strings.Split(s[len(b):], ",") {
 		ra = textproto.TrimString(ra)
 		if ra == "" {
@@ -157,7 +153,7 @@ func parseRange(s string) ([]GetRange, error) {
 			return nil, errors.New("invalid range")
 		}
 		start, end = textproto.TrimString(start), textproto.TrimString(end)
-		var r GetRange
+		var r ByteRange
 		if start == "" {
 			r.From = 0
 			// If no start is specified, end specifies the

gateway/handler_test.go

@@ -45,11 +45,7 @@ type errorMockAPI struct {
 	err error
 }
 
-func (api *errorMockAPI) Get(ctx context.Context, path ImmutablePath) (ContentPathMetadata, *GetResponse, error) {
-	return ContentPathMetadata{}, nil, api.err
-}
-
-func (api *errorMockAPI) GetRange(ctx context.Context, path ImmutablePath, getRange ...GetRange) (ContentPathMetadata, *GetResponse, error) {
+func (api *errorMockAPI) Get(ctx context.Context, path ImmutablePath, getRange ...ByteRange) (ContentPathMetadata, *GetResponse, error) {
 	return ContentPathMetadata{}, nil, api.err
 }
 
@@ -161,11 +157,7 @@ type panicMockAPI struct {
 	panicOnHostnameHandler bool
 }
 
-func (api *panicMockAPI) Get(ctx context.Context, immutablePath ImmutablePath) (ContentPathMetadata, *GetResponse, error) {
-	panic("i am panicking")
-}
-
-func (api *panicMockAPI) GetRange(ctx context.Context, immutablePath ImmutablePath, ranges ...GetRange) (ContentPathMetadata, *GetResponse, error) {
+func (api *panicMockAPI) Get(ctx context.Context, immutablePath ImmutablePath, ranges ...ByteRange) (ContentPathMetadata, *GetResponse, error) {
 	panic("i am panicking")
 }
 

gateway/handler_unixfs_dir.go

@@ -23,7 +23,7 @@ import (
 // serveDirectory returns the best representation of UnixFS directory
 //
 // It will return index.html if present, or generate directory listing otherwise.
-func (i *handler) serveDirectory(ctx context.Context, w http.ResponseWriter, r *http.Request, resolvedPath ipath.Resolved, contentPath ipath.Path, isHeadRequest bool, directoryMetadata *directoryMetadata, ranges []GetRange, begin time.Time, logger *zap.SugaredLogger) bool {
+func (i *handler) serveDirectory(ctx context.Context, w http.ResponseWriter, r *http.Request, resolvedPath ipath.Resolved, contentPath ipath.Path, isHeadRequest bool, directoryMetadata *directoryMetadata, ranges []ByteRange, begin time.Time, logger *zap.SugaredLogger) bool {
 	ctx, span := spanTrace(ctx, "ServeDirectory", trace.WithAttributes(attribute.String("path", resolvedPath.String())))
 	defer span.End()
 
@@ -81,11 +81,7 @@ func (i *handler) serveDirectory(ctx context.Context, w http.ResponseWriter, r *
 		}
 	} else {
 		var getResp *GetResponse
-		if len(ranges) == 0 {
-			_, getResp, err = i.api.Get(ctx, imIndexPath)
-		} else {
-			_, getResp, err = i.api.GetRange(ctx, imIndexPath, ranges...)
-		}
+		_, getResp, err = i.api.Get(ctx, imIndexPath, ranges...)
 		if err == nil {
 			if getResp.bytes == nil {
 				webError(w, fmt.Errorf("%q could not be read: %w", imIndexPath, files.ErrNotReader), http.StatusUnprocessableEntity)

gateway/metrics.go

@@ -55,25 +55,13 @@ func (b *ipfsBackendWithMetrics) updateApiCallMetric(name string, err error, beg
 	}
 }
 
-func (b *ipfsBackendWithMetrics) Get(ctx context.Context, path ImmutablePath) (ContentPathMetadata, *GetResponse, error) {
+func (b *ipfsBackendWithMetrics) Get(ctx context.Context, path ImmutablePath, ranges ...ByteRange) (ContentPathMetadata, *GetResponse, error) {
 	begin := time.Now()
 	name := "IPFSBackend.Get"
-	ctx, span := spanTrace(ctx, name, trace.WithAttributes(attribute.String("path", path.String())))
-	defer span.End()
-
-	md, n, err := b.api.Get(ctx, path)
-
-	b.updateApiCallMetric(name, err, begin)
-	return md, n, err
-}
-
-func (b *ipfsBackendWithMetrics) GetRange(ctx context.Context, path ImmutablePath, ranges ...GetRange) (ContentPathMetadata, *GetResponse, error) {
-	begin := time.Now()
-	name := "IPFSBackend.GetRange"
-	ctx, span := spanTrace(ctx, name, trace.WithAttributes(attribute.String("path", path.String())))
+	ctx, span := spanTrace(ctx, name, trace.WithAttributes(attribute.String("path", path.String()), attribute.Int("ranges", len(ranges))))
 	defer span.End()
 
-	md, f, err := b.api.GetRange(ctx, path, ranges...)
+	md, f, err := b.api.Get(ctx, path, ranges...)
 
 	b.updateApiCallMetric(name, err, begin)
 	return md, f, err