feat: wrap parsing errors into ErrInvalidCid

[hacdias]

This PR wraps the parsing errors into ErrInvalidCid, a new custom error type I added. This allows depends of this package to test if the error they got includes a wrapped CID parsing error via errors.Is.

[rvagg]

a test or two would be nice

Is the custom Is necessary here? The Unwrap should be enough for errors.Is to work shouldn't it?

It's a shame we're not at minimum of Go 1.20 because fmt.Errorf("%w: %w", ErrInvalidCid, err) would be an excellent pattern to avoid the additional code here, ErrInvalidCid could just be an errors.New("invalid cid").

[hacdias]

@rvagg

Is the custom Is necessary here? The Unwrap should be enough for errors.Is to work shouldn't it?

Yes, it is necessary. You can try running the test I just added without the .Is. The Unwrap only gives you the underlying error. I want to errors.Is(err, &ErrInvalidCid{}) to match any parsing errors. That is not possible without having the .Is implemented.

It's a shame we're not at minimum of Go 1.20

Yes, I do agree that the multiple wrapping in Go 1.20 would solve much of the hassle and be easier. Perhaps when we finally move to Go 1.20 we can change this, but then it will be a breaking change as ErrInvalidCid will be its own error and not just a type.

[rvagg]

See fc27a0c which you can pull in or reimplement yourself which changes this away from pointers.

An error only needs to satisfy interface { Error() string }, it doesn't matter whether it's a pointer receiver or not as long as it's got that method in some form. To some degree whether you use pointers for errors or not is a matter of taste, but errors.Is and friends takes away arguments based on wanting to use pointers for identity (err1 == err2). When you make it a pointer you're going to be allocating on the heap and involving GC etc. We're not modifying anything in the struct, if the user wants to modify it then that's their call but it's done in their local version, not the version that gets bubbled all the way up from this base layer of the stack (these errors are going to be received by users many frames removed from these call sites given go-cid's location in the stack!) so there's lots of pieces in between that a modification of the error might impact; so having a value rather than a pointer takes away that risk too.

I'll see if I can get some more experienced gophers to weigh in in case I'm talking out of my butt here.

[kylehuntsman]

I'd like to propose a simpler style of wrapping all together.

It doesn't look like ErrInvalidCid actually holds anymore than the error it is wrapping, so there is no need for a struct.

var (
  ErrInvalidCid = errors.New("invalid cid")
  ErrCidTooShort = fmt.Errorf("%w: cid too short", ErrInvalidCid)
)

You'll typically find examples where the %w is placed after the child error message, but we can place it before the child error message to keep the nice invalid cid: reason for invalid cid messaging format when printing the error out.

Whenever you want to return a ErrCidTooShort, just return ErrCidTooShort.

func funcThatReturnsCidTooShort() error {
	return ErrCidTooShort
}

You can still check it's an ErrInvalidCid with errors.Is all without having to implement any of the functions around managing a struct with an internal Err or wrapping/unwrapping.

err := funcThatReturnsCidTooShort()
errors.Is(err, ErrInvalidCid) {
  // do whatever
}

A nice side affect of the way we declared the error variables is that this creates a nice UX when using the errors. You don't need to instantiate a struct and pass it to errors.Is, you can just use the exported variables cid.ErrInvalidCid and cid.ErrCidTooShort. Nice and clean!

You can do the same thing for one off ErrInvalidCid errors as well

func funcThatReturnsWrappedErrInvalidCid() error {
	err := errors.New("some other error from somewhere else")
	return fmt.Errorf("%w: %s", ErrInvalidCid, err)
}

Here's a golang playground that implements what I just described. Playground is for 1.19.

https://go.dev/play/p/zrCM0aQthTO?v=goprev

package main

import (
	"errors"
	"fmt"
)

var (
	ErrInvalidCid  = errors.New("invalid cid")
	ErrCidTooShort = fmt.Errorf("%w: cid too short", ErrInvalidCid)
)

func main() {
	err1 := funcThatReturnsCidTooShort()
	if errors.Is(err1, ErrInvalidCid) {
		fmt.Printf("err1 is an ErrInvalidCid, %s\n", err1) // Prints "err1 is an ErrInvalidCid, invalid cid: cid too short"
	}

	err2 := funcThatReturnsWrappedErrInvalidCid()
	if errors.Is(err2, ErrInvalidCid) {
		fmt.Printf("err2 is an ErrInvalidCid, %s\n", err2) // Prints "err2 is an ErrInvalidCid, invalid cid: some other error from somewhere else"
	}
}

func funcThatReturnsCidTooShort() error {
	return ErrCidTooShort
}

func funcThatReturnsWrappedErrInvalidCid() error {
	err := errors.New("some other error from somewhere else")
	return fmt.Errorf("%w: %s", ErrInvalidCid, err)
}

[kylehuntsman]

It's a shame we're not at minimum of Go 1.20 because fmt.Errorf("%w: %w", ErrInvalidCid, err) would be an excellent pattern to avoid the additional code here, ErrInvalidCid could just be an errors.New("invalid cid").

I just saw Rod's comment about this. This should work since go 1.13 no? I feel dumb now for writing out this whole thing and then realizing you guys might have already dismissed this because of the go version. Whoops 🤷.

I see that go 1.20 supports wrapping of multiple errors, as Rod already pointed out. What is the reason that the code below doesn't work? Is it if err is a ErrCustom and then stringified, then you lose the ability to check if the returned error is also a ErrCustom?

return fmt.Errorf("%w, %s", ErrInvalidCid, err)

Update

So yeah, if err in the above snippit is a ErrCidTooShort, errors.Is returns false if checking for it.

[rvagg]

What is the reason that the code below doesn't work?

All of the cases where there's currently this: &ErrInvalidCid{err} - wrapping an error returned from another library where you don't really get to tell the user they don't need the ability to errors.Is that error. Without multiple wrapping you lose the ability to errors.Is() on whatever error is coming out of go-multibase or go-multihash or whatever hasher is involved here that may have some special error type or sentinel worth unwrapping.

Same reason ErrInvalidCid = errors.New("invalid cid") and %w isn't going to work here, because you need %w: %w to get both of them in the wrapping chain .. which you can't do (unless we pull in multierr but more dependencies mean more problems), so maybe we just refactor this after Go 1.21 comes out and we get to drop Go 1.19.

[kylehuntsman]

... so maybe we just refactor this after Go 1.21 comes out and we get to drop Go 1.19.

This might make sense. In ipfs/go-libipfs/pull/205, the usage is:

errors.Is(err, &cid.ErrInvalidCid{}) {
  ...
}

If we don't wait for multiple wrapping, then the above breaks when multiple wrapping is implemented.

We could implement a IsErrInvalidCid(), doing the hard work for determining if any given error is in fact a ErrInvalidCid. Easily backwards compatible and doesn't change the interface once multiple wraps are supported. We can let users know they could use errors.Is once we support multiple wraps. 🤔

[rvagg]

Conversation here about Is*Err() functions (tl;dr: no): ipfs/go-ipld-format#76

[kylehuntsman]

Yeah okay, I see how this goes in circles back to implementing Is(), etc.

[hacdias]

@rvagg what you propose is also fine, I just wanted to understand the reasoning behind it. I applied your commit.

If we don't wait for multiple wrapping, then the above breaks when multiple wrapping is implemented.

@kylehuntsman yes, it breaks if we implement multiple wrapping here and release it as a patch release. If we do a 0.X release it should be fine to say there's a breaking change. I wouldn't consider that the most important piece right now.

[masih]

Minor comments otherwise LGTM

[rvagg]

Thanks for jumping through the hoops @hacdias, sorry for the pile-on, we're at the bottom of the stack so it's nice to get this RIGHT!

Let's bump a 0.4.0 for this, it's technically breaking because it removes the ability to do things like if err == mh.ErrInvalidMultihash {} which would work with the current version. There's probably a bunch of other sentinel errors under here too, like mbase.ErrUnsupportedEncoding.

[github-actions]

Suggested version: v0.4.0

Comparing to: v0.3.2 (diff)

Changes in go.mod file(s):

diff --git a/go.mod b/go.mod
index 44e2a51..c06cc5c 100644
--- a/go.mod
+++ b/go.mod
@@ -13,8 +13,8 @@ require (
 	github.com/mr-tron/base58 v1.2.0 // indirect
 	github.com/multiformats/go-base32 v0.0.3 // indirect
 	github.com/multiformats/go-base36 v0.1.0 // indirect
-	golang.org/x/crypto v0.0.0-20210506145944-38f3c27a63bf // indirect
-	golang.org/x/sys v0.0.0-20210309074719-68d13333faf2 // indirect
+	golang.org/x/crypto v0.1.0 // indirect
+	golang.org/x/sys v0.1.0 // indirect
 )
 
-go 1.18
+go 1.19

gorelease says:

# summary
Suggested version: v0.4.0

gocompat says:

Your branch is up to date with 'origin/master'.

Cutting a Release (and modifying non-markdown files)

This PR is modifying both version.json and non-markdown files.
The Release Checker is not able to analyse files that are not checked in to master. This might cause the above analysis to be inaccurate.
Please consider performing all the code changes in a separate PR before cutting the release.

Automatically created GitHub Release

A draft GitHub Release has been created.
It is going to be published when this PR is merged.
You can modify its' body to include any release notes you wish to include with the release.