feat: Is() performs interface check, IsNotFound() unwraps

storage/notfound.go

@@ -1,11 +1,10 @@
 package storage
 
-import "github.com/ipfs/go-cid"
+import (
+	"errors"
 
-// compatible with the go-ipld-format ErrNotFound, match against
-// interface{NotFound() bool}
-// this could go into go-ipld-prime, but for now we'll just exercise the
-// feature-test pattern
+	"github.com/ipfs/go-cid"
+)
 
 // ErrNotFound is a 404, but for block storage systems. It is returned when
 // a block is not found. The Key is typically the binary form of a CID
@@ -16,6 +15,13 @@ import "github.com/ipfs/go-cid"
 // The IsNotFound() function here will test for this and therefore be compatible
 // with this ErrNotFound, and the legacy ErrNotFound. The same is not true for
 // the legacy github.com/ipfs/go-ipld-format#IsNotFound.
+//
+// errors.Is() should be preferred as the standard Go way to test for errors;
+// however due to the move of the legacy ErrNotFound to this package, 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.
 type ErrNotFound struct {
 	Key string
 }
@@ -39,22 +45,46 @@ func (e ErrNotFound) NotFound() bool {
 	return true
 }
 
-// Is allows errors.Is to work with this error type.
+// Is allows errors.Is to work with this error type. It is compatible with the
+// legacy github.com/ipfs/go-ipld-format#ErrNotFound, and other related error
+// types as it uses a feature-test on the NotFound() method.
+//
+// It is important to note that because errors.Is() performs a reverse match,
+// whereby the Is() of the error being checked is called on the target,
+// the legacy ErrNotFound#Is will perform a strict type match, which will fail
+// where the original error is of the legacy type. Where compatibility is
+// required across multiple block storage systems that may return legacy error
+// types, use the IsNotFound() function instead.
 func (e ErrNotFound) Is(err error) bool {
-	switch err.(type) {
-	case ErrNotFound:
-		return true
-	default:
-		return false
+	if v, ok := err.(interface{ NotFound() bool }); ok && v.NotFound() {
+		return v.NotFound()
 	}
+	return false
 }
 
-// IsNotFound returns true if the error is a ErrNotFound. As it uses a
-// feature-test, it is also compatible with the legacy
-// github.com/ipfs/go-ipld-format#ErrNotFound.
+var enf = ErrNotFound{}
+
+// IsNotFound returns true if the error is a ErrNotFound, or compatible with an
+// ErrNotFound, or wraps such an error. Compatibility is determined by the
+// type implementing the NotFound() method which returns true.
+// It is compatible with the legacy github.com/ipfs/go-ipld-format#ErrNotFound,
+// and other related error types.
+//
+// This is NOT the same as errors.Is(err, storage.ErrNotFound{}) which relies on
+// the Is() of the original err rather than the target. IsNotFound() uses the
+// Is() of storage.ErrNotFound to perform the check. The difference being that
+// the err being checked doesn't need to have a feature-testing Is() method for
+// this to succeed, it only needs to have a NotFound() method that returns true.
+//
+// Prefer this method for maximal compatibility, including wrapped errors, that
+// implement the minimal interface{ NotFound() true }.
 func IsNotFound(err error) bool {
-	if nf, ok := err.(interface{ NotFound() bool }); ok {
-		return nf.NotFound()
+	for {
+		if enf.Is(err) {
+			return true
+		}
+		if err = errors.Unwrap(err); err == nil {
+			return false
+		}
 	}
-	return false
 }

storage/notfound_test.go

@@ -1,6 +1,8 @@
 package storage_test
 
 import (
+	"errors"
+	"fmt"
 	"testing"
 
 	"github.com/ipfs/go-cid"
@@ -12,6 +14,9 @@ func TestNotFound(t *testing.T) {
 	if !storage.IsNotFound(nf) {
 		t.Fatal("expected ErrNotFound to be a NotFound error")
 	}
+	if !errors.Is(nf, storage.ErrNotFound{}) {
+		t.Fatal("expected ErrNotFound to be a NotFound error")
+	}
 	if nf.Error() != "ipld: could not find foo" {
 		t.Fatal("unexpected error message")
 	}
@@ -20,19 +25,47 @@ func TestNotFound(t *testing.T) {
 	if !storage.IsNotFound(nf) {
 		t.Fatal("expected ErrNotFound to be a NotFound error")
 	}
+	if !errors.Is(nf, storage.ErrNotFound{}) {
+		t.Fatal("expected ErrNotFound to be a NotFound error")
+	}
 	if nf.Error() != "ipld: could not find bafybeigdyrzt5sfp7udm7hu76uh7y26nf3efuylqabf3oclgtqy55fbzdi" {
 		t.Fatal("unexpected error message")
 	}
 
-	wnf := &weirdNotFoundError{}
+	wrappedNf := fmt.Errorf("wrapped outer: %w", fmt.Errorf("wrapped inner: %w", nf))
+	if !storage.IsNotFound(wrappedNf) {
+		t.Fatal("expected wrapped ErrNotFound to be a NotFound error")
+	}
+	if !errors.Is(wrappedNf, storage.ErrNotFound{}) {
+		t.Fatal("expected wrapped ErrNotFound to be a NotFound error")
+	}
+
+	fmt.Println("WeirdNotFoundErr")
+	wnf := weirdNotFoundError{}
 	if !storage.IsNotFound(wnf) {
 		t.Fatal("expected weirdNotFoundError to be a NotFound error")
 	}
+	if !errors.Is(wnf, storage.ErrNotFound{}) {
+		t.Fatal("expected weirdNotFoundError to be a NotFound error")
+	}
 
-	// a weirder case, this one implements `NotFound()` but it returns false, so
-	// this shouldn't be a NotFound error
-	wnnf := &weirdNotNotFoundError{}
+	// a weirder case, this one implements `NotFound()` but it returns false; but
+	// it also implements the same Is() that will claim it's a not-found, so
+	// it should work one way around, but not the other, when it's being asked
+	// whether an error is or not
+	wnnf := weirdNotNotFoundError{}
 	if storage.IsNotFound(wnnf) {
+		// this shouldn't be true because we test NotFound()==true
+		t.Fatal("expected weirdNotNotFoundError to NOT be a NotFound error")
+	}
+	if !errors.Is(wnnf, storage.ErrNotFound{}) {
+		// this should be true, because weirdNotNotFoundError.Is() performs the
+		// check on storage.ErrNotFound{}.NotFound() which does return true.
+		t.Fatal("expected weirdNotNotFoundError to be a NotFound error")
+	}
+	if errors.Is(nf, weirdNotNotFoundError{}) {
+		// switch them around and we get the same result as storage.IsNotFound, but
+		// won't work with wrapped weirdNotNotFoundError errors.
 		t.Fatal("expected weirdNotNotFoundError to NOT be a NotFound error")
 	}
 }
@@ -43,6 +76,13 @@ func (weirdNotFoundError) NotFound() bool {
 	return true
 }
 
+func (weirdNotFoundError) Is(err error) bool {
+	if v, ok := err.(interface{ NotFound() bool }); ok && v.NotFound() {
+		return v.NotFound()
+	}
+	return false
+}
+
 func (weirdNotFoundError) Error() string {
 	return "weird not found error"
 }
@@ -53,6 +93,13 @@ func (weirdNotNotFoundError) NotFound() bool {
 	return false
 }
 
+func (weirdNotNotFoundError) Is(err error) bool {
+	if v, ok := err.(interface{ NotFound() bool }); ok && v.NotFound() {
+		return v.NotFound()
+	}
+	return false
+}
+
 func (weirdNotNotFoundError) Error() string {
 	return "weird not NOT found error"
 }