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: https://github.com/ipld/go-ipld-prime/pull/494

Also Ref https://github.com/ipld/go-car/pull/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.

[rvagg]

Implemented the suggested change in https://github.com/ipld/go-ipld-prime/pull/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 https://github.com/ipld/go-ipld-prime/pull/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[bot]]

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.

[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 https://github.com/ipld/go-ipld-prime/pull/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?

[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.