From c4cf7d4b34d4d0854574a963b53aec0147dbeff9 Mon Sep 17 00:00:00 2001 From: Henrique Dias Date: Thu, 7 Sep 2023 10:19:38 +0200 Subject: [PATCH] refactor!: remove .Remainder, coreiface.ResolvePath returns remainder --- coreiface/coreapi.go | 6 ++- coreiface/tests/block.go | 2 +- coreiface/tests/dag.go | 2 +- coreiface/tests/path.go | 12 +++--- gateway/blocks_backend.go | 55 ++++++++++++++++----------- gateway/gateway.go | 7 ++-- gateway/handler_codec.go | 7 ++-- path/path.go | 79 ++++++++++++++++++++------------------- path/path_test.go | 24 ++++++------ 9 files changed, 104 insertions(+), 90 deletions(-) diff --git a/coreiface/coreapi.go b/coreiface/coreapi.go index 19a1d5ebba..25e54a37b7 100644 --- a/coreiface/coreapi.go +++ b/coreiface/coreapi.go @@ -46,8 +46,10 @@ type CoreAPI interface { // Routing returns an implementation of Routing API Routing() RoutingAPI - // ResolvePath resolves the path using Unixfs resolver - ResolvePath(context.Context, path.Path) (path.ImmutablePath, error) + // ResolvePath resolves the path using UnixFS resolver, and returns the resolved + // immutable path, and the remainder of the path segments that cannot be resolved + // within UnixFS. + ResolvePath(context.Context, path.Path) (path.ImmutablePath, []string, error) // ResolveNode resolves the path (if not resolved already) using Unixfs // resolver, gets and returns the resolved Node diff --git a/coreiface/tests/block.go b/coreiface/tests/block.go index 4647165d17..e825d3e615 100644 --- a/coreiface/tests/block.go +++ b/coreiface/tests/block.go @@ -220,7 +220,7 @@ func (tp *TestSuite) TestBlockGet(t *testing.T) { p := path.NewIPFSPath(res.Path().Cid()) - rp, err := api.ResolvePath(ctx, p) + rp, _, err := api.ResolvePath(ctx, p) if err != nil { t.Fatal(err) } diff --git a/coreiface/tests/dag.go b/coreiface/tests/dag.go index 425c8b4178..767fc3b422 100644 --- a/coreiface/tests/dag.go +++ b/coreiface/tests/dag.go @@ -116,7 +116,7 @@ func (tp *TestSuite) TestDagPath(t *testing.T) { t.Fatal(err) } - rp, err := api.ResolvePath(ctx, p) + rp, _, err := api.ResolvePath(ctx, p) if err != nil { t.Fatal(err) } diff --git a/coreiface/tests/path.go b/coreiface/tests/path.go index d356de39a7..0a63ee427d 100644 --- a/coreiface/tests/path.go +++ b/coreiface/tests/path.go @@ -55,9 +55,9 @@ func (tp *TestSuite) TestPathRemainder(t *testing.T) { p, err := path.Join(path.NewIPFSPath(nd.Cid()), "foo", "bar") require.NoError(t, err) - rp1, err := api.ResolvePath(ctx, p) + _, remainder, err := api.ResolvePath(ctx, p) require.NoError(t, err) - require.Equal(t, "/foo/bar", rp1.Remainder()) + require.Equal(t, "/foo/bar", path.SegmentsToString(remainder...)) } func (tp *TestSuite) TestEmptyPathRemainder(t *testing.T) { @@ -74,9 +74,9 @@ func (tp *TestSuite) TestEmptyPathRemainder(t *testing.T) { err = api.Dag().Add(ctx, nd) require.NoError(t, err) - rp1, err := api.ResolvePath(ctx, path.NewIPFSPath(nd.Cid())) + _, remainder, err := api.ResolvePath(ctx, path.NewIPFSPath(nd.Cid())) require.NoError(t, err) - require.Empty(t, rp1.Remainder()) + require.Empty(t, remainder) } func (tp *TestSuite) TestInvalidPathRemainder(t *testing.T) { @@ -96,7 +96,7 @@ func (tp *TestSuite) TestInvalidPathRemainder(t *testing.T) { p, err := path.Join(path.NewIPLDPath(nd.Cid()), "/bar/baz") require.NoError(t, err) - _, err = api.ResolvePath(ctx, p) + _, _, err = api.ResolvePath(ctx, p) require.NotNil(t, err) require.ErrorContains(t, err, `no link named "bar"`) } @@ -122,7 +122,7 @@ func (tp *TestSuite) TestPathRoot(t *testing.T) { p, err := path.Join(path.NewIPLDPath(nd.Cid()), "/foo") require.NoError(t, err) - rp, err := api.ResolvePath(ctx, p) + rp, _, err := api.ResolvePath(ctx, p) require.NoError(t, err) require.Equal(t, rp.Cid().String(), blk.Path().Cid().String()) } diff --git a/gateway/blocks_backend.go b/gateway/blocks_backend.go index e5ddf87626..26d60c393e 100644 --- a/gateway/blocks_backend.go +++ b/gateway/blocks_backend.go @@ -485,14 +485,15 @@ func walkGatewaySimpleSelector(ctx context.Context, p path.Path, params CarParam } func (bb *BlocksBackend) getNode(ctx context.Context, path path.ImmutablePath) (ContentPathMetadata, format.Node, error) { - roots, lastSeg, err := bb.getPathRoots(ctx, path) + roots, lastSeg, remainder, err := bb.getPathRoots(ctx, path) if err != nil { return ContentPathMetadata{}, nil, err } md := ContentPathMetadata{ - PathSegmentRoots: roots, - LastSegment: lastSeg, + PathSegmentRoots: roots, + LastSegment: lastSeg, + LastSegmentRemainder: remainder, } lastRoot := lastSeg.Cid() @@ -505,7 +506,7 @@ func (bb *BlocksBackend) getNode(ctx context.Context, path path.ImmutablePath) ( return md, nd, err } -func (bb *BlocksBackend) getPathRoots(ctx context.Context, contentPath path.ImmutablePath) ([]cid.Cid, path.ImmutablePath, error) { +func (bb *BlocksBackend) getPathRoots(ctx context.Context, contentPath path.ImmutablePath) ([]cid.Cid, path.ImmutablePath, []string, error) { /* These are logical roots where each CID represent one path segment and resolves to either a directory or the root block of a file. @@ -529,7 +530,10 @@ func (bb *BlocksBackend) getPathRoots(ctx context.Context, contentPath path.Immu contentPathStr := contentPath.String() pathSegments := strings.Split(contentPathStr[6:], "/") sp.WriteString(contentPathStr[:5]) // /ipfs or /ipns - var lastPath path.ImmutablePath + var ( + lastPath path.ImmutablePath + remainder []string + ) for _, root := range pathSegments { if root == "" { continue @@ -538,23 +542,24 @@ func (bb *BlocksBackend) getPathRoots(ctx context.Context, contentPath path.Immu sp.WriteString(root) p, err := path.NewPath(sp.String()) if err != nil { - return nil, nil, err + return nil, nil, nil, err } - resolvedSubPath, err := bb.resolvePath(ctx, p) + resolvedSubPath, remainderSubPath, err := bb.resolvePath(ctx, p) if err != nil { // TODO: should we be more explicit here and is this part of the IPFSBackend contract? // The issue here was that we returned datamodel.ErrWrongKind instead of this resolver error if isErrNotFound(err) { - return nil, nil, resolver.ErrNoLink{Name: root, Node: lastPath.Cid()} + return nil, nil, nil, resolver.ErrNoLink{Name: root, Node: lastPath.Cid()} } - return nil, nil, err + return nil, nil, nil, err } lastPath = resolvedSubPath + remainder = remainderSubPath pathRoots = append(pathRoots, lastPath.Cid()) } pathRoots = pathRoots[:len(pathRoots)-1] - return pathRoots, lastPath, nil + return pathRoots, lastPath, remainder, nil } func (bb *BlocksBackend) ResolveMutable(ctx context.Context, p path.Path) (path.ImmutablePath, error) { @@ -605,7 +610,7 @@ func (bb *BlocksBackend) GetDNSLinkRecord(ctx context.Context, hostname string) } func (bb *BlocksBackend) IsCached(ctx context.Context, p path.Path) bool { - rp, err := bb.resolvePath(ctx, p) + rp, _, err := bb.resolvePath(ctx, p) if err != nil { return false } @@ -615,41 +620,47 @@ func (bb *BlocksBackend) IsCached(ctx context.Context, p path.Path) bool { } func (bb *BlocksBackend) ResolvePath(ctx context.Context, path path.ImmutablePath) (ContentPathMetadata, error) { - roots, lastSeg, err := bb.getPathRoots(ctx, path) + roots, lastSeg, remainder, err := bb.getPathRoots(ctx, path) if err != nil { return ContentPathMetadata{}, err } md := ContentPathMetadata{ - PathSegmentRoots: roots, - LastSegment: lastSeg, + PathSegmentRoots: roots, + LastSegment: lastSeg, + LastSegmentRemainder: remainder, } return md, nil } -func (bb *BlocksBackend) resolvePath(ctx context.Context, p path.Path) (path.ImmutablePath, error) { +func (bb *BlocksBackend) resolvePath(ctx context.Context, p path.Path) (path.ImmutablePath, []string, error) { var err error if p.Namespace() == path.IPNSNamespace { p, err = resolve.ResolveIPNS(ctx, bb.namesys, p) if err != nil { - return nil, err + return nil, nil, err } } if p.Namespace() != path.IPFSNamespace { - return nil, fmt.Errorf("unsupported path namespace: %s", p.Namespace()) + return nil, nil, fmt.Errorf("unsupported path namespace: %s", p.Namespace()) } - node, rest, err := bb.resolver.ResolveToLastNode(ctx, p) + node, remainder, err := bb.resolver.ResolveToLastNode(ctx, p) if err != nil { - return nil, err + return nil, nil, err } - p, err = path.Join(path.NewIPFSPath(node), rest...) + p, err = path.Join(path.NewIPFSPath(node), remainder...) if err != nil { - return nil, err + return nil, nil, err + } + + imPath, err := path.NewImmutablePath(p) + if err != nil { + return nil, nil, err } - return path.NewImmutablePath(p) + return imPath, remainder, nil } type nodeGetterToCarExporer struct { diff --git a/gateway/gateway.go b/gateway/gateway.go index 3be6c10f31..039c3a0663 100644 --- a/gateway/gateway.go +++ b/gateway/gateway.go @@ -204,9 +204,10 @@ func (d DuplicateBlocksPolicy) String() string { } type ContentPathMetadata struct { - PathSegmentRoots []cid.Cid - LastSegment path.ImmutablePath - ContentType string // Only used for UnixFS requests + PathSegmentRoots []cid.Cid + LastSegment path.ImmutablePath + LastSegmentRemainder []string + ContentType string // Only used for UnixFS requests } // ByteRange describes a range request within a UnixFS file. "From" and "To" mostly diff --git a/gateway/handler_codec.go b/gateway/handler_codec.go index df222a7c21..f1a41e8ead 100644 --- a/gateway/handler_codec.go +++ b/gateway/handler_codec.go @@ -84,9 +84,10 @@ func (i *handler) renderCodec(ctx context.Context, w http.ResponseWriter, r *htt // If the resolved path still has some remainder, return error for now. // TODO: handle this when we have IPLD Patch (https://ipld.io/specs/patch/) via HTTP PUT // TODO: (depends on https://github.com/ipfs/kubo/issues/4801 and https://github.com/ipfs/kubo/issues/4782) - if resolvedPath.Remainder() != "" { - path := strings.TrimSuffix(resolvedPath.String(), resolvedPath.Remainder()) - err := fmt.Errorf("%q of %q could not be returned: reading IPLD Kinds other than Links (CBOR Tag 42) is not implemented: try reading %q instead", resolvedPath.Remainder(), resolvedPath.String(), path) + if len(rq.pathMetadata.LastSegmentRemainder) != 0 { + remainderStr := path.SegmentsToString(rq.pathMetadata.LastSegmentRemainder...) + path := strings.TrimSuffix(resolvedPath.String(), remainderStr) + err := fmt.Errorf("%q of %q could not be returned: reading IPLD Kinds other than Links (CBOR Tag 42) is not implemented: try reading %q instead", remainderStr, resolvedPath.String(), path) i.webError(w, r, err, http.StatusNotImplemented) return false } diff --git a/path/path.go b/path/path.go index 172f9dce53..314470c87e 100644 --- a/path/path.go +++ b/path/path.go @@ -83,11 +83,7 @@ func (p path) Namespace() Namespace { } func (p path) Segments() []string { - // Trim slashes from beginning and end, such that we do not return empty segments. - str := strings.TrimSuffix(p.str, "/") - str = strings.TrimPrefix(str, "/") - - return strings.Split(str, "/") + return StringToSegments(p.str) } // ImmutablePath is a [Path] which is guaranteed to have an immutable [Namespace]. @@ -96,9 +92,6 @@ type ImmutablePath interface { // Cid returns the [cid.Cid] of the root object of the path. Cid() cid.Cid - - // Remainder returns the unresolved parts of the path. - Remainder() string } var _ Path = immutablePath{} @@ -139,14 +132,6 @@ func (ip immutablePath) Cid() cid.Cid { return ip.cid } -func (ip immutablePath) Remainder() string { - remainder := strings.Join(ip.Segments()[2:], "/") - if remainder != "" { - remainder = "/" + remainder - } - return remainder -} - // NewIPFSPath returns a new "/ipfs" path with the provided CID. func NewIPFSPath(cid cid.Cid) ImmutablePath { return immutablePath{ @@ -174,34 +159,31 @@ func NewIPLDPath(cid cid.Cid) ImmutablePath { // trailing slash. This function returns an error when the given string is not // a valid content path. func NewPath(str string) (Path, error) { - cleaned := gopath.Clean(str) - components := strings.Split(cleaned, "/") + segments := StringToSegments(str) - if strings.HasSuffix(str, "/") { - // Do not forget to store the trailing slash! - cleaned += "/" + // Shortest valid path is "/{namespace}/{root}". That yields at least two + // segments: ["{namespace}" "{root}"]. Therefore, here we check if the original + // string begins with "/" (any path must), if we have at least two segments, and if + // the root is non-empty. The namespace is checked further below. + if !strings.HasPrefix(str, "/") || len(segments) < 2 || segments[1] == "" { + return nil, &ErrInvalidPath{err: ErrInsufficientComponents, path: str} } - // Shortest valid path is "/{namespace}/{element}". That yields at least three - // components: [" " "{namespace}" "{element}"]. The first component must therefore - // be empty. - if len(components) < 3 || components[0] != "" { - return nil, &ErrInvalidPath{err: ErrInsufficientComponents, path: str} + cleaned := SegmentsToString(segments...) + if strings.HasSuffix(str, "/") { + // Do not forget to preserve the trailing slash! + cleaned += "/" } - switch components[1] { + switch segments[0] { case "ipfs", "ipld": - if components[2] == "" { - return nil, &ErrInvalidPath{err: ErrInsufficientComponents, path: str} - } - - cid, err := cid.Decode(components[2]) + cid, err := cid.Decode(segments[1]) if err != nil { return nil, &ErrInvalidPath{err: err, path: str} } ns := IPFSNamespace - if components[1] == "ipld" { + if segments[0] == "ipld" { ns = IPLDNamespace } @@ -213,16 +195,12 @@ func NewPath(str string) (Path, error) { cid: cid, }, nil case "ipns": - if components[2] == "" { - return nil, &ErrInvalidPath{err: ErrInsufficientComponents, path: str} - } - return path{ str: cleaned, namespace: IPNSNamespace, }, nil default: - return nil, &ErrInvalidPath{err: fmt.Errorf("%w: %q", ErrUnknownNamespace, components[1]), path: str} + return nil, &ErrInvalidPath{err: fmt.Errorf("%w: %q", ErrUnknownNamespace, segments[0]), path: str} } } @@ -231,7 +209,7 @@ func NewPath(str string) (Path, error) { // using a forward slash "/" as separator. Please see [Path.Segments] for more // information about how segments must be structured. func NewPathFromSegments(segments ...string) (Path, error) { - return NewPath("/" + strings.Join(segments, "/")) + return NewPath(SegmentsToString(segments...)) } // Join joins a [Path] with certain segments and returns a new [Path]. @@ -240,3 +218,26 @@ func Join(p Path, segments ...string) (Path, error) { s = append(s, segments...) return NewPathFromSegments(s...) } + +// SegmentsToString converts an array of segments into a string. The returned string +// will always be prefixed with a "/" if there are any segments. For example, if the +// given segments array is ["foo", "bar"], the returned value will be "/foo/bar". +// Given an empty array, an empty string is returned. +func SegmentsToString(segments ...string) string { + str := strings.Join(segments, "/") + if str != "" { + str = "/" + str + } + return str +} + +// StringToSegments converts a string into an array of segments. This function follows +// the rules of [Path.Segments]: the path is first cleaned through [gopath.Clean] and +// no empty segments are returned. +func StringToSegments(str string) []string { + str = gopath.Clean(str) + // Trim slashes from beginning and end, such that we do not return empty segments. + str = strings.TrimSuffix(str, "/") + str = strings.TrimPrefix(str, "/") + return strings.Split(str, "/") +} diff --git a/path/path_test.go b/path/path_test.go index 08b692f39f..e7300ff22e 100644 --- a/path/path_test.go +++ b/path/path_test.go @@ -224,21 +224,20 @@ func TestNewImmutablePath(t *testing.T) { t.Run("Succeeds on Immutable Path", func(t *testing.T) { testCases := []struct { - path string - cid cid.Cid - remainder string + path string + cid cid.Cid }{ - {"/ipfs/QmdfTbBqBPQ7VNxZEYEj14VmRuZBkqFbiwReogJgS1zR1n", cid.MustParse("QmdfTbBqBPQ7VNxZEYEj14VmRuZBkqFbiwReogJgS1zR1n"), ""}, - {"/ipfs/QmdfTbBqBPQ7VNxZEYEj14VmRuZBkqFbiwReogJgS1zR1n/a/b", cid.MustParse("QmdfTbBqBPQ7VNxZEYEj14VmRuZBkqFbiwReogJgS1zR1n"), "/a/b"}, - {"/ipfs/QmdfTbBqBPQ7VNxZEYEj14VmRuZBkqFbiwReogJgS1zR1n/a/b/", cid.MustParse("QmdfTbBqBPQ7VNxZEYEj14VmRuZBkqFbiwReogJgS1zR1n"), "/a/b"}, + {"/ipfs/QmdfTbBqBPQ7VNxZEYEj14VmRuZBkqFbiwReogJgS1zR1n", cid.MustParse("QmdfTbBqBPQ7VNxZEYEj14VmRuZBkqFbiwReogJgS1zR1n")}, + {"/ipfs/QmdfTbBqBPQ7VNxZEYEj14VmRuZBkqFbiwReogJgS1zR1n/a/b", cid.MustParse("QmdfTbBqBPQ7VNxZEYEj14VmRuZBkqFbiwReogJgS1zR1n")}, + {"/ipfs/QmdfTbBqBPQ7VNxZEYEj14VmRuZBkqFbiwReogJgS1zR1n/a/b/", cid.MustParse("QmdfTbBqBPQ7VNxZEYEj14VmRuZBkqFbiwReogJgS1zR1n")}, - {"/ipfs/bafybeihdwdcefgh4dqkjv67uzcmw7ojee6xedzdetojuzjevtenxquvyku", cid.MustParse("bafybeihdwdcefgh4dqkjv67uzcmw7ojee6xedzdetojuzjevtenxquvyku"), ""}, - {"/ipfs/bafybeihdwdcefgh4dqkjv67uzcmw7ojee6xedzdetojuzjevtenxquvyku/a/b", cid.MustParse("bafybeihdwdcefgh4dqkjv67uzcmw7ojee6xedzdetojuzjevtenxquvyku"), "/a/b"}, - {"/ipfs/bafybeihdwdcefgh4dqkjv67uzcmw7ojee6xedzdetojuzjevtenxquvyku/a/b/", cid.MustParse("bafybeihdwdcefgh4dqkjv67uzcmw7ojee6xedzdetojuzjevtenxquvyku"), "/a/b"}, + {"/ipfs/bafybeihdwdcefgh4dqkjv67uzcmw7ojee6xedzdetojuzjevtenxquvyku", cid.MustParse("bafybeihdwdcefgh4dqkjv67uzcmw7ojee6xedzdetojuzjevtenxquvyku")}, + {"/ipfs/bafybeihdwdcefgh4dqkjv67uzcmw7ojee6xedzdetojuzjevtenxquvyku/a/b", cid.MustParse("bafybeihdwdcefgh4dqkjv67uzcmw7ojee6xedzdetojuzjevtenxquvyku")}, + {"/ipfs/bafybeihdwdcefgh4dqkjv67uzcmw7ojee6xedzdetojuzjevtenxquvyku/a/b/", cid.MustParse("bafybeihdwdcefgh4dqkjv67uzcmw7ojee6xedzdetojuzjevtenxquvyku")}, - {"/ipld/bafybeihdwdcefgh4dqkjv67uzcmw7ojee6xedzdetojuzjevtenxquvyku", cid.MustParse("bafybeihdwdcefgh4dqkjv67uzcmw7ojee6xedzdetojuzjevtenxquvyku"), ""}, - {"/ipld/bafybeihdwdcefgh4dqkjv67uzcmw7ojee6xedzdetojuzjevtenxquvyku/a/b", cid.MustParse("bafybeihdwdcefgh4dqkjv67uzcmw7ojee6xedzdetojuzjevtenxquvyku"), "/a/b"}, - {"/ipld/bafybeihdwdcefgh4dqkjv67uzcmw7ojee6xedzdetojuzjevtenxquvyku/a/b/", cid.MustParse("bafybeihdwdcefgh4dqkjv67uzcmw7ojee6xedzdetojuzjevtenxquvyku"), "/a/b"}, + {"/ipld/bafybeihdwdcefgh4dqkjv67uzcmw7ojee6xedzdetojuzjevtenxquvyku", cid.MustParse("bafybeihdwdcefgh4dqkjv67uzcmw7ojee6xedzdetojuzjevtenxquvyku")}, + {"/ipld/bafybeihdwdcefgh4dqkjv67uzcmw7ojee6xedzdetojuzjevtenxquvyku/a/b", cid.MustParse("bafybeihdwdcefgh4dqkjv67uzcmw7ojee6xedzdetojuzjevtenxquvyku")}, + {"/ipld/bafybeihdwdcefgh4dqkjv67uzcmw7ojee6xedzdetojuzjevtenxquvyku/a/b/", cid.MustParse("bafybeihdwdcefgh4dqkjv67uzcmw7ojee6xedzdetojuzjevtenxquvyku")}, } for _, testCase := range testCases { @@ -249,7 +248,6 @@ func TestNewImmutablePath(t *testing.T) { assert.NoError(t, err) assert.Equal(t, testCase.path, ip.String()) assert.Equal(t, testCase.cid, ip.Cid()) - assert.Equal(t, testCase.remainder, ip.Remainder()) } }) }