From 805d4620ddd04e0497186d722e8eed6116899ac8 Mon Sep 17 00:00:00 2001 From: Rod Vagg Date: Fri, 17 Feb 2023 13:58:14 +1100 Subject: [PATCH] feat: Is() performs interface check, IsNotFound() unwraps --- storage/notfound.go | 64 +++++++++++++++++++++++++++++----------- storage/notfound_test.go | 55 +++++++++++++++++++++++++++++++--- 2 files changed, 98 insertions(+), 21 deletions(-) diff --git a/storage/notfound.go b/storage/notfound.go index f6779a9c..0795a5a6 100644 --- a/storage/notfound.go +++ b/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 } diff --git a/storage/notfound_test.go b/storage/notfound_test.go index 32ba046b..56d207d2 100644 --- a/storage/notfound_test.go +++ b/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" }