Improve "merkledag: not found" error

[hsanjuan]

Based on #33, taking advantage of the rather new Is() methods from the errors
package to check for equality.

[Stebalien]

Returning ErrNotFoundCid{Cid} is already going to break anything comparing against ErrNotFound. Given that, I'd rather explicitly break it and replace ErrNotFound with type ErrNotFound struct { Cid }.

[hsanjuan]

Ok, but no-one returns ErrNotFoundCid (and we can grep-fix all of our things accordingly). So for everyone else things would still work. But yeah, let's play safe and break.

[Stebalien]

We should consider the comments form #33 and implement a NotFound method on the error. See: https://github.com/ipfs/go-datastore/blob/9e9a3a8350fe9b372e4d1cf8dc590d1f005a1540/datastore.go#L202-L213.

[Stebalien]

Note: we're definitely going to punt this till after 0.5 as it may have other consequences.

[hsanjuan]

We should consider the comments form #33 and implement a NotFound method on the error. See: https://github.com/ipfs/go-datastore/blob/9e9a3a8350fe9b372e4d1cf8dc590d1f005a1540/datastore.go#L202-L213.

What is the rationale for this approach? I don't fully understand it. Is the rationale here to be able to check if an error implements a NotFound interface and then use that method to decide? Who is supposed to call a .NotFound method?

I checked the blog post and my take is that wrapping and letting the original packages decide if it's one of their NotFound is the accepted approach. Particularly now that Go added wrap() and Is() methods natively. This allows both treating them as opaque and checking them when needed. It also allows every package to have their own ErrNotFound (which can wrap other ErrNotFounds).

Am I missing something?

[hsanjuan]

Exposed Cid thinking that this might be used from outside this module.

It is ugly though that you have to instantiate the struct whenever you want to check if errors.Is(whatever, ErrNotFound{}).

[Stebalien]

We can add an IsNotFound function.

[hsanjuan]

Added, and made it equivalent to Is.

[Stebalien]

From async discussion:

Really, there are two kinds of checks: "is" and "property".

• is: Useful when you want to check if an error was ultimately caused by some specific error. Honestly, that's probably what we want here.
• property: This is how, e.g., timeout errors usually work. You implement functions on your custom error type indicating the properties your error has. Then, you can call As(err, &targetInterface) on the error to tell if the error can have the given property. Finally, you invoke the actual function (in this case, NotFound() bool to determine if the error actually has that property.

TL;DR: NotFound() is probably a "nice to have". For our particular use-case, it's not really all that useful as we specifically want to know if the error was "IPLD node not found", not some random "datastore is broken because the index wasn't found" error.