Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat!: use NotFound feature-test in IsNotFound() #76

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

feat!: use NotFound feature-test in IsNotFound() #76

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
07b3870
feat!: use NotFound feature-test in IsNotFound()
rvagg Feb 16, 2023
7f024d7
chore!: deprecate ErrNotFound and alias to github.com/ipld/go-ipld-pr…
rvagg Mar 28, 2023
d826cf7
fix: remove deprecation notes
rvagg Mar 29, 2023
54480b8
fix: update to ABI stable go-ipld-prime ErrNotFound
rvagg Jun 8, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions batch_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ func (d *testDag) Get(ctx context.Context, cid cid.Cid) (Node, error) {
if n, ok := d.nodes[cid.KeyString()]; ok {
return n, nil
}
return nil, ErrNotFound{cid}
return nil, ErrNotFound{Cid: cid}
}

func (d *testDag) GetMany(ctx context.Context, cids []cid.Cid) <-chan *NodeOption {
Expand All @@ -37,7 +37,7 @@ func (d *testDag) GetMany(ctx context.Context, cids []cid.Cid) <-chan *NodeOptio
if n, ok := d.nodes[c.KeyString()]; ok {
out <- &NodeOption{Node: n}
} else {
out <- &NodeOption{Err: ErrNotFound{c}}
out <- &NodeOption{Err: ErrNotFound{Cid: c}}
}
}
close(out)
Expand Down
4 changes: 2 additions & 2 deletions coding_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ func TestDecode(t *testing.T) {
id, err := cid.Prefix{
Version: 1,
Codec: cid.Raw,
MhType: mh.ID,
MhType: mh.IDENTITY,
MhLength: 0,
}.Sum(nil)

Expand Down Expand Up @@ -59,7 +59,7 @@ func TestRegistryDecode(t *testing.T) {
id, err := cid.Prefix{
Version: 1,
Codec: cid.Raw,
MhType: mh.ID,
MhType: mh.IDENTITY,
MhLength: 0,
}.Sum(nil)

Expand Down
2 changes: 1 addition & 1 deletion format_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ func (n *EmptyNode) Cid() cid.Cid {
id, err := cid.Prefix{
Version: 1,
Codec: cid.Raw,
MhType: mh.ID,
MhType: mh.IDENTITY,
MhLength: 0,
}.Sum(nil)

Expand Down
19 changes: 11 additions & 8 deletions go.mod
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,21 +2,24 @@ module github.com/ipfs/go-ipld-format

require (
github.com/ipfs/go-block-format v0.0.2
github.com/ipfs/go-cid v0.0.2
github.com/multiformats/go-multihash v0.0.1
github.com/ipfs/go-cid v0.4.1
github.com/ipld/go-ipld-prime v0.20.1-0.20230608045340-64d7e0ea9c05
github.com/multiformats/go-multihash v0.2.2
)

require (
github.com/gxed/hashland/keccakpg v0.0.1 // indirect
github.com/gxed/hashland/murmur3 v0.0.1 // indirect
github.com/ipfs/go-ipfs-util v0.0.1 // indirect
github.com/minio/blake2b-simd v0.0.0-20160723061019-3f5f724cb5b1 // indirect
github.com/minio/sha256-simd v0.0.0-20190131020904-2d45a736cd16 // indirect
github.com/mr-tron/base58 v1.1.0 // indirect
github.com/klauspost/cpuid/v2 v2.0.9 // indirect
github.com/minio/sha256-simd v1.0.0 // indirect
github.com/mr-tron/base58 v1.2.0 // indirect
github.com/multiformats/go-base32 v0.0.3 // indirect
github.com/multiformats/go-multibase v0.0.1 // indirect
github.com/multiformats/go-base36 v0.1.0 // indirect
github.com/multiformats/go-multibase v0.0.3 // indirect
github.com/multiformats/go-varint v0.0.6 // indirect
github.com/spaolacci/murmur3 v1.1.0 // indirect
golang.org/x/crypto v0.1.0 // indirect
golang.org/x/sys v0.1.0 // indirect
lukechampine.com/blake3 v1.1.6 // indirect
)

go 1.19
32 changes: 23 additions & 9 deletions go.sum
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,29 +1,43 @@
github.com/gxed/hashland/keccakpg v0.0.1 h1:wrk3uMNaMxbXiHibbPO4S0ymqJMm41WiudyFSs7UnsU=
github.com/gxed/hashland/keccakpg v0.0.1/go.mod h1:kRzw3HkwxFU1mpmPP8v1WyQzwdGfmKFJ6tItnhQ67kU=
github.com/gxed/hashland/murmur3 v0.0.1 h1:SheiaIt0sda5K+8FLz952/1iWS9zrnKsEJaOJu4ZbSc=
github.com/gxed/hashland/murmur3 v0.0.1/go.mod h1:KjXop02n4/ckmZSnY2+HKcLud/tcmvhST0bie/0lS48=
github.com/ipfs/go-block-format v0.0.2 h1:qPDvcP19izTjU8rgo6p7gTXZlkMkF5bz5G3fqIsSCPE=
github.com/ipfs/go-block-format v0.0.2/go.mod h1:AWR46JfpcObNfg3ok2JHDUfdiHRgWhJgCQF+KIgOPJY=
github.com/ipfs/go-cid v0.0.1/go.mod h1:GHWU/WuQdMPmIosc4Yn1bcCT7dSeX4lBafM7iqUPQvM=
github.com/ipfs/go-cid v0.0.2 h1:tuuKaZPU1M6HcejsO3AcYWW8sZ8MTvyxfc4uqB4eFE8=
github.com/ipfs/go-cid v0.0.2/go.mod h1:GHWU/WuQdMPmIosc4Yn1bcCT7dSeX4lBafM7iqUPQvM=
github.com/ipfs/go-cid v0.4.1 h1:A/T3qGvxi4kpKWWcPC/PgbvDA2bjVLO7n4UeVwnbs/s=
github.com/ipfs/go-cid v0.4.1/go.mod h1:uQHwDeX4c6CtyrFwdqyhpNcxVewur1M7l7fNU7LKwZk=
github.com/ipfs/go-ipfs-util v0.0.1 h1:Wz9bL2wB2YBJqggkA4dD7oSmqB4cAnpNbGrlHJulv50=
github.com/ipfs/go-ipfs-util v0.0.1/go.mod h1:spsl5z8KUnrve+73pOhSVZND1SIxPW5RyBCNzQxlJBc=
github.com/minio/blake2b-simd v0.0.0-20160723061019-3f5f724cb5b1 h1:lYpkrQH5ajf0OXOcUbGjvZxxijuBwbbmlSxLiuofa+g=
github.com/ipld/go-ipld-prime v0.20.1-0.20230608045340-64d7e0ea9c05 h1:yZ1HyYSgpB2euE1+GbnOkYoJVVHHobIBQGAvkyzay7Q=
github.com/ipld/go-ipld-prime v0.20.1-0.20230608045340-64d7e0ea9c05/go.mod h1:BJhRzV15Xxt1AsWdreTrLoVm5iOndyt/dVgXj67HabE=
github.com/klauspost/cpuid/v2 v2.0.4/go.mod h1:FInQzS24/EEf25PyTYn52gqo7WaD8xa0213Md/qVLRg=
github.com/klauspost/cpuid/v2 v2.0.9 h1:lgaqFMSdTdQYdZ04uHyN2d/eKdOMyi2YLSvlQIBFYa4=
github.com/klauspost/cpuid/v2 v2.0.9/go.mod h1:FInQzS24/EEf25PyTYn52gqo7WaD8xa0213Md/qVLRg=
github.com/minio/blake2b-simd v0.0.0-20160723061019-3f5f724cb5b1/go.mod h1:pD8RvIylQ358TN4wwqatJ8rNavkEINozVn9DtGI3dfQ=
github.com/minio/sha256-simd v0.0.0-20190131020904-2d45a736cd16 h1:5W7KhL8HVF3XCFOweFD3BNESdnO8ewyYTFT2R+/b8FQ=
github.com/minio/sha256-simd v0.0.0-20190131020904-2d45a736cd16/go.mod h1:2FMWW+8GMoPweT6+pI63m9YE3Lmw4J71hV56Chs1E/U=
github.com/mr-tron/base58 v1.1.0 h1:Y51FGVJ91WBqCEabAi5OPUz38eAx8DakuAm5svLcsfQ=
github.com/minio/sha256-simd v1.0.0 h1:v1ta+49hkWZyvaKwrQB8elexRqm6Y0aMLjCNsrYxo6g=
github.com/minio/sha256-simd v1.0.0/go.mod h1:OuYzVNI5vcoYIAmbIvHPl3N3jUzVedXbKy5RFepssQM=
github.com/mr-tron/base58 v1.1.0/go.mod h1:xcD2VGqlgYjBdcBLw+TuYLr8afG+Hj8g2eTVqeSzSU8=
github.com/mr-tron/base58 v1.2.0 h1:T/HDJBh4ZCPbU39/+c3rRvE0uKBQlU27+QI8LJ4t64o=
github.com/mr-tron/base58 v1.2.0/go.mod h1:BinMc/sQntlIE1frQmRFPUoPA1Zkr8VRgBdjWI2mNwc=
github.com/multiformats/go-base32 v0.0.3 h1:tw5+NhuwaOjJCC5Pp82QuXbrmLzWg7uxlMFp8Nq/kkI=
github.com/multiformats/go-base32 v0.0.3/go.mod h1:pLiuGC8y0QR3Ue4Zug5UzK9LjgbkL8NSQj0zQ5Nz/AA=
github.com/multiformats/go-multibase v0.0.1 h1:PN9/v21eLywrFWdFNsFKaU04kLJzuYzmrJR+ubhT9qA=
github.com/multiformats/go-base36 v0.1.0 h1:JR6TyF7JjGd3m6FbLU2cOxhC0Li8z8dLNGQ89tUg4F4=
github.com/multiformats/go-base36 v0.1.0/go.mod h1:kFGE83c6s80PklsHO9sRn2NCoffoRdUUOENyW/Vv6sM=
github.com/multiformats/go-multibase v0.0.1/go.mod h1:bja2MqRZ3ggyXtZSEDKpl0uO/gviWFaSteVbWT51qgs=
github.com/multiformats/go-multihash v0.0.1 h1:HHwN1K12I+XllBCrqKnhX949Orn4oawPkegHMu2vDqQ=
github.com/multiformats/go-multibase v0.0.3 h1:l/B6bJDQjvQ5G52jw4QGSYeOTZoAwIO77RblWplfIqk=
github.com/multiformats/go-multibase v0.0.3/go.mod h1:5+1R4eQrT3PkYZ24C3W2Ue2tPwIdYQD509ZjSb5y9Oc=
github.com/multiformats/go-multihash v0.0.1/go.mod h1:w/5tugSrLEbWqlcgJabL3oHFKTwfvkofsjW2Qa1ct4U=
github.com/multiformats/go-multihash v0.2.2 h1:Uu7LWs/PmWby1gkj1S1DXx3zyd3aVabA4FiMKn/2tAc=
github.com/multiformats/go-multihash v0.2.2/go.mod h1:dXgKXCXjBzdscBLk9JkjINiEsCKRVch90MdaGiKsvSM=
github.com/multiformats/go-varint v0.0.6 h1:gk85QWKxh3TazbLxED/NlDVv8+q+ReFJk7Y2W/KhfNY=
github.com/multiformats/go-varint v0.0.6/go.mod h1:3Ls8CIEsrijN6+B7PbrXRPxHRPuXSrVKRY101jdMZYE=
github.com/spaolacci/murmur3 v1.1.0 h1:7c1g84S4BPRrfL5Xrdp6fOJ206sU9y293DDHaoy0bLI=
github.com/spaolacci/murmur3 v1.1.0/go.mod h1:JwIasOWyU6f++ZhiEuf87xNszmSA2myDM2Kzu9HwQUA=
golang.org/x/crypto v0.0.0-20190211182817-74369b46fc67/go.mod h1:6SG95UA2DQfeDnfUPMdvaQW0Q7yPrPDi9nlGo2tz2b4=
golang.org/x/crypto v0.1.0 h1:MDRAIl0xIo9Io2xV565hzXHw3zVseKrJKodhohM5CjU=
golang.org/x/crypto v0.1.0/go.mod h1:RecgLatLF4+eUMCP1PoPZQb+cVrJcOPbHkTkbkB9sbw=
golang.org/x/sys v0.0.0-20190219092855-153ac476189d/go.mod h1:STP8DvDyc/dI5b8T5hshtkjS+E42TnysNCUPdjciGhY=
golang.org/x/sys v0.1.0 h1:kunALQeHf1/185U1i0GOB/fy1IPRDDpuoOOqRReG57U=
golang.org/x/sys v0.1.0/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg=
lukechampine.com/blake3 v1.1.6 h1:H3cROdztr7RCfoaTpGZFQsrqvweFLrqS73j7L7cmR5c=
lukechampine.com/blake3 v1.1.6/go.mod h1:tkKEOtDkNtklkXtLNEOGNq5tcV90tJiA1vAA12R78LA=
49 changes: 14 additions & 35 deletions merkledag.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,50 +2,29 @@ package format

import (
"context"
"errors"

cid "github.com/ipfs/go-cid"
"github.com/ipld/go-ipld-prime/storage"
)

// ErrNotFound is used to signal when a Node could not be found. The specific
// meaning will depend on the DAGService implementation, which may be trying
// to read nodes locally but also, trying to find them remotely.
//
// The Cid field can be filled in to provide additional context.
type ErrNotFound struct {
Cid cid.Cid
}

// Error implements the error interface and returns a human-readable
// message for this error.
func (e ErrNotFound) Error() string {
if e.Cid == cid.Undef {
return "ipld: could not find node"
}

return "ipld: could not find " + e.Cid.String()
}
type ErrNotFound = storage.ErrNotFound
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd like to make progress with this if possible but don't want to upset people by forging ahead if you disagree with the approach.

Thanks @rvagg I appreciate you both pushing on this and the thought here.

Some concerns/comments here:

NotFound() bool being too generic

There are a bunch of NotFound errors in the Go ecosystem that would match the check in the go-ipld-prime PR https://grep.app/search?q=%20notfound%28%29%20bool&filter[lang][0]=Go. Depending on how these errors are being used this could lead to bugs.

Adding the go-ipld-prime dependency

AFAICT the only change in this PR is to move where the error type lives since it could just as easily live on go-ipld-format as go-ipld-prime. The go-ipld-prime PR adds the changes which are mostly modifying Is() to apply to any NotFound() bool. If so does that just mean that we're at "go-ipld-prime doesn't want to import go-ipld-format, and go-ipld-format doesn't want to import go-ipld-prime"? If so it doesn't appear that there's much real benefit in moving this error.

In the meantime, I think this PR at least puts things on equal footing. It is super frustrating to HAVE to include this library to get a not found error to be recognized by our stack. Especially if you are using go-ipld-prime.

This ends up being false, since the PR just inverts which package would need to depend on which.

I don't personally care a ton here, since it's not like importing a couple variables from a package and re-exporting them is going to be a serious problem if the package is small and doesn't import too many dependencies, but it we're at the point of debating which library the error should live in I'm calling out that neither is automatically better.

Maybe it does end up being worthwhile (as was mentioned earlier in this PR) to create a new package. If it offends people's sensibilities to put the new package in either of go-ipld-format or go-ipld-prime then another repo is doable too. IMO this isn't going to be the last time we're going to be dealing with these kinds of error inconsistencies.

For example, in some recent gateway work in boxo we've ended up with https://github.com/ipfs/boxo/blob/1ec05d754868ab80af629b4a9401ee575cecbe1e/gateway/errors.go#L180-L208 which is pretty annoying and something that may pop up for other IPLD users.

In this case we'd end up with at least NotFound and InvalidTraversal errors and we'd make them IPLD specific.

Thoughts?


// Is allows to check whether any error is of this ErrNotFound type.
// Do not use this directly, but rather errors.Is(yourError, ErrNotFound).
func (e ErrNotFound) Is(err error) bool {
switch err.(type) {
case ErrNotFound:
return true
default:
return false
}
}

// NotFound returns true.
func (e ErrNotFound) NotFound() bool {
return true
}

// IsNotFound returns if the given error is or wraps an ErrNotFound
// (equivalent to errors.Is(err, ErrNotFound{}))
// IsNotFound returns true if the error is a ErrNotFound. As it uses a
// feature-test, it is also compatible with other NotFound error types,
// including github.com/ipld/go-ipld-prime/storage#ErrNotFound.
//
// errors.Is() should be preferred as the standard Go way to test for errors;
// however due to the move of the legacy ErrNotFound to
// github.com/ipld/go-ipld-prime/storage, it may not report correctly where
// older block storage packages emit the legacy ErrNotFound. The IsNotFound()
// function provides a maximally compatible matching function that should be
// able to determine whether an ErrNotFound, either new or legacy, exists within
// a wrapped error chain.
func IsNotFound(err error) bool {
return errors.Is(err, ErrNotFound{})
return storage.IsNotFound(err)
}

// Either a node or an error.
Expand Down