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

[rvagg]

This is a BREAKING CHANGE as it no longer strictly matches only this ErrNotFound type but any type implementing interface{ NotFound() bool }.

Ref: ipld/go-ipld-prime#494

Also Ref ipld/go-car#363 where this was first implemented and flagged as a generalised feature that would get pulled in to go-ipld-prime. One thing this allows us to do is remove one more central dependency just for a simple type—for most newer IPLD work, this repo doesn't have anything other than ErrNotFound, but that gets in the way when we're dealing with legacy Blockstore interfaces which still use the check here.

[willscott]

is there a reason not to attempt the same duck-typing here as in IsNotFound?

[rvagg]

I'm on the fence about this one, my thinking is that when you're doing an errors.Is() you have an expectation that the thing your checking IS the thing you are looking for, or at a minimum it's in the chain of wrapped errors somewhere. Changing it to check for NotFound() means that you're loosening that contract to be IS LIKE. I'm more comfortable doing it with IsNotFound() because that contract is local, not at the standard library level and because it has a very particular meaning - "is not found!".

But I could go the other way too, I can't think up a hypothetical case where you would want to differentiate. Maybe if you were checking to see whether you have an old-style Blockstore somewhere in your call stack .. for some reason?

@masih care to weigh in too? I don't mind going either way on this.

[masih]

Thinking out loud:

If the rationale is backward compatibility then my vote would be to have less verbose code and let Is just work without strict type matching.

As far as the go documentation on this interface goes, it reads very loose to me; the documentation does not explicitly dictate that the type should match. In fact it specifically uses loose language like "An error type might provide an Is method so it can be treated as equivalent to an existing error.".

[rvagg]

"equivalent" is the key word here, good catch that it's in the official docs!

I'll switch it out here and in go-ipld-prime so we get maximal compatibility and happiness.

[rvagg]

Implemented the suggested change in ipld/go-ipld-prime#494 but ran into some hurdled with the nature of errors.Is(), so I've taken it a step further with IsNotFound(). Will copy those changes here if/when we agree on the approach.

[aschmahmann]

@hsanjuan given you did the implementation and most of the bubbling work with NotFound here, any concerns or see any potential issues with this refactoring? For example, do you know of anyone doing typecasting after an Is/IsNotFound that would be problematic?

[Kubuxu]

Go stdlib discourages usage of standalone func Is$ErrorType, I don't think we should create requirements to use the older style of error handling where the Go stdlib suggests using only the new style:

example from stdlib: os/error.go:88

// This function predates errors.Is. It only supports errors returned by
// the os package. New code should use errors.Is(err, fs.ErrNotExist).
func IsNotExist(err error) bool {
	return underlyingErrorIs(err, ErrNotExist)
}

My proposal would be to create a minimal package containing sentinel error values for use within the ecosystem. Each package that exposes in its own errors can re-export for user convenience.

---

As a user, I will most likely forget about IsNotFound and use errors.Is(err. whateverpackageigottheresultfrom.ErrNotFound). Lotus (and my own code) especially make very heavy use of error wrapping so the errors.Is is preferred.

[hsanjuan]

I don't see that this would cause problems in our stack but it's been a while.

I used ipld.IsNotFound(err) all over the place instead of errors.Is(err, format.ErrNotFound{}) because it was weird to have to instantiate an empty struct (no idea it was discouraged, but then it was just a shorthand for exactly the same thing and not meant to behave differently at all).

I think changing in both Is() and IsNotFound() to do exactly the same is the most consistent, short of extracting to a minimal separate package (current trend is to reduce number of repos).

I think we can indeed consider anything that implements NotFound() equivalent for our purposes.

We might also choose to deprecate IsNotFound() and gently push everyone to move onto the errors.Is() syntax.

[Kubuxu]

format.ErrNotFound should probably be a sentinel instance of the error, but that would be even more breaking.

My primary point is that we should not encourage the usage of ipld.IsNotFound because it doesn't handle wrapping.

[rvagg]

OK, so I'd really like to progress this and ideally deprecate the ErrNotFound here because it's the main thing that has broad use from this library and that would be a solid step toward deprecating this whole library. The feedback so far seems to make that goal difficult to achieve with the changes proposed—i.e. the desire to make errors.Is() work nicely while also allowing a smooth migration path off this library.

So, I've proposed a new approach in the latest change:

• Alias ErrNotFound to github.com/ipld/go-ipld-prime/storage.ErrNotFound and have it be the single, sentinel error type. That is being proposed over in feat: add ErrNotFound ipld/go-ipld-prime#494 and is very similar to what I was originally proposing here.
• Alias IsNotFound() to storage.IsNotFound()
• Put deprecation notices on both here.

Where we end up with is:

• An IsNotFound() that is universally safe, it should be able to detect any not-found error conforming to interface{NotFound() bool} within a wrapped error chain.
• An Is() that will work with errors.Is() BUT with the catch that because errors.Is works in reverse (to what I think it should), by using the source's Is() to check against the target, there may be cases where using errors.Is on "legacy" forms of ErrNotFound don't get recognised.

So a safe migration path would look like: use IsNotFound() until we've either flushed out older forms of ErrNotFound from the stack or you're sure you're only working with the newer forms. We (I) can probably chase down the main uses of this error type and get them updated, then when people go through the upgrade-all-my-dependencies dance they'll be less likely to end up in a situation where it matters.

[hsanjuan]

solid step toward deprecating this whole library

What I would like to see is a similar upgrade path for every type in this library. Whatever the reasons are to deprecate this lib, this is a library that does not get in the middle, is easy to use and compact, something that go-ipld-prime was not last time I looked.

main thing that has broad use from this library

DAGService defined here is very important and I use it in a bunch of places. Node and Links as defined here are very important. There is currently no replacement, to my knowledge, that is not absolutely complex and overkill to how things are defined here.

I personally see 0 value in deprecating this library. I am not sure what I gain as a developer, other than tedious upgrade work. Perhaps that should be reasoned somewhere before embarking on such efforts.

[hannahhoward]

@hsanjuan if people want to use this library, that's great. And realistically, it'd be silly to try to fully deprecate this library or go-merkledag any time soon.

That said, as someone who has followed it for a long time, go-ipld-prime has come a long, long way. 3 years ago, it was a playground of experimentation that prioritized theoretical correctness over practicality. Today, I honestly enjoy using it, love the power you get. I'm sad a lot of the waters are poisoned from people being turned off by what it was in the past. The emerging tools built on top of it are actually quite nice. For example, go-unixfsnode is a library that I honestly find much easier to use than go-unixfs these days.

Rather than framing this as a "should we deprecate this library" which puts everyone on the defensive cause no one likes upgrade work, I'd rather frame this as "why would you choose one or the other?" and use it to identify some of the remaining rough edges in go-ipld-prime (I definitely am aware of a few). And also to enumerate some of the great things about that library (like the ability to work more seamlessly IMO with non-UnixFS data).

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.

[github-actions]

Suggested version: v0.5.0

Comparing to: v0.4.0 (diff)

Changes in go.mod file(s):

diff --git a/go.mod b/go.mod
index 105c7c1..e131c85 100644
--- a/go.mod
+++ b/go.mod
@@ -6,4 +6,17 @@ require (
 	github.com/multiformats/go-multihash v0.0.1
 )
 
-go 1.16
+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/multiformats/go-base32 v0.0.3 // indirect
+	github.com/multiformats/go-multibase v0.0.1 // indirect
+	golang.org/x/crypto v0.0.0-20190211182817-74369b46fc67 // indirect
+	golang.org/x/sys v0.0.0-20190219092855-153ac476189d // indirect
+)
+
+go 1.19

gorelease says:

# summary
Suggested version: v0.5.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.

[rvagg]

Ok, I stepped on a mine with that one, I apologise. I've removed the deprecation notice here so it's just a plain upgrade and won't impact ongoing use of this library.

If this is still too offensive to pull in go-ipld-prime (although you might be surprised how it undergirds so much our stack these days so you're already using it, just hidden behind wrappers to make our older libraries continue to feel the same), then another alternative is to make an entirely new repo just for this error. I'm fine with that too. I just want to stop pulling in this library just for an error type, which is the only reason I, and many others, have to touch go-ipld-format.

[hsanjuan]

Ok, I stepped on a mine with that one, I apologise. I've removed the deprecation notice here so it's just a plain upgrade and won't impact ongoing use of this library.

If this is still too offensive to pull in go-ipld-prime (although you might be surprised how it undergirds so much our stack these days so you're already using it, just hidden behind wrappers to make our older libraries continue to feel the same), then another alternative is to make an entirely new repo just for this error. I'm fine with that too. I just want to stop pulling in this library just for an error type, which is the only reason I, and many others, have to touch go-ipld-format.

Sorry, I didn't mean that you shouldn't deprecate these types. I meant about the library as a whole (particularly without an easy upgrade path to just rename a couple of things and have everything working).

You can deprecate these error types just fine. It is not so much of a problem for someone that pulls go-ipld-format to pull go-ipld-prime and rename some errors (we do already). What Hannah said makes sense.

[aschmahmann]

This is a BREAKING CHANGE as it no longer strictly matches only this ErrNotFound type but any type implementing interface{ NotFound() bool }.

It also breaks here because the field and value of the error have changed.

What's the upgrade plan here for all the impacted repos if these PRs were to get merged? Is it easier/harder than:

1. Just having go-ipld-prime's error type alias this one (since IIUC you are introducing a new type for go-ipld-prime as opposed to this one which is already in use)
2. Making a new type in go-ipld-prime, adding the NotFound() bool detection to both types and fixing any stragglers comparing directly with this type rather than using a function for determining equality

Is the idea here that there are more places doing equality checks (or type casts) vs creating or getting data from the error type, that the compiler will be louder here which will make this change easier than the alternatives, or something else?

[rvagg]

1. The original impetus behind this exercise is to remove dependency on go-ipld-format, because as far as a large amount of new code is concerned this repo only exists for this error type. I think there is a path to eventually deprecating this repo, but this error type will remain the main blocker to that kind of cleanup.
2. That was my original proposal, but the objection (further up here) was that errors.Is should be the preferred method of error checking, operating on a single, sentinel error type. Finding a single solution is a bit challenging.

I don't think I've seen any code directly doing type or equality checks with this type. I may not be looking in the right places. The vast majority of uses call directly into IsNotFound, which we can keep stable easily enough. (From a GitHub search that there's ~3 places (not forks) using errors.Is()).

Yet another possibility here is closer to my original commit here: Don't alias the error type, leave it alone, but make the implementations here and in go-ipld-prime roughly the same (probably with different field names) - performing the same feature test. As we move dependent code forward, both errors.Is() and IsNotFound() will pick up both forms, just using the idea that a "not found error conforms to a strict contract". That option just removes the singular sentinel form, which was requested above but I'm fine with that because it gives us all room to move I think.

[willscott]

• I think I agree with @Kubuxu that we should be trying to get to a world where errors.Is is the way to be determining a generic NotFound state across various ipld's.
• I'm personally okay migrating the the ErrNotFound here to storage.ErrNotFound as you did above, and committing to propagating changes in use to consumers.
• I'm okay limiting the current change to this error type, which I have also encountered frustration with, and defer any determination of other parts of the library until later.

[rvagg]

Take 5 at charting a middle path: I've updated ipld/go-ipld-prime#494 to be ABI compatible with this type, by changing the property from Key string to Cid cid.Cid. So now the aliasing here to that error type should be stable for users and I believe the change here should be entirely transparent.
There is a world in which old versions of this library performing an errors.Is() on their copy of this type are not going to catch it, but we (I) can work on getting as many users of this library upgraded as possible to avoid the awkwardness and also it's going to be better than the state we have now of having divergent "NotFound" error types already in play. If we can get both of these PRs merged, then work on cleaning all of the references up (I can spend a day or two doing this work), then we're in a very clean place, and users of this package that are only pulling it in for this error type can switch to go-ipld-prime, which they are much more likely to be already using, at least as a transitive.

@aschmahmann @willscott would you two mind taking a look and letting me know if you disagree with this as a path forward?

[willscott]

This looks fine to me

[rvagg]

ping @aschmahmann & anyone else interested in this thread - 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.

[aschmahmann]

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?