go-ipld-prime icon indicating copy to clipboard operation
go-ipld-prime copied to clipboard

feat: add ErrNotFound

Open rvagg opened this issue 2 years ago • 4 comments

  • Intended to replace github.com/ipfs/go-ipld-format#ErrNotFound
  • A new IsNotFound() that uses feature detection rather than type checking so it's compatible with old and new forms.

Ref: https://github.com/ipld/go-car/pull/363 Ref: https://github.com/ipld/go-ipld-prime/pull/493


I flagged this in the go-car PR where I put a form of this in v2/storage, which has a note about potentially doing this here: https://github.com/ipld/go-car/blob/a2a8d2f9f60fba6cb0c0b985d9599bcc03a3e2f4/v2/storage/notfound.go#L5-L8

@hannahhoward has flagged the need for this in #493 since it's getting increasingly awkward when moving between LinkSystem and Blockstore and back again, as demonstrated in https://github.com/filecoin-project/lassie/pull/86.

Unfortunately, in its current form, we can't just make the "legacy" ErrNotFound be an alias for this one because I'm using a Key property here rather than Cid, because the storage interfaces here are all key string based. https://github.com/ipfs/go-ipld-format/blob/e379dec580e9a158eaa5311f6cf6ab644c0b3ca2/merkledag.go#L15

Instead, my proposal for now is to just update the IsNotFound() over there to match this one, so it feature-detects too and they can play nicely together. We could also put a deprectation notice on it and point people here.

Other suggestions?

rvagg avatar Feb 15 '23 07:02 rvagg

go-ipld-format version of this https://github.com/ipfs/go-ipld-format/pull/76

rvagg avatar Feb 16 '23 06:02 rvagg

@masih @willscott: continuing discussion from https://github.com/ipfs/go-ipld-format/pull/76/files#r1108834553, I've implemented the change discussed here first and will copy wholesale over there afterward.

The main goal is to make errors.Is() perform the lose matching using the feature detection, rather than strictly type detection.

But, the annoying challenge with this is that errors.Is(err, target) uses the Is() on err, not target to do the check. So while we can do nice checking here, it's not going to be relaxed enough for legacy errors to match. e.g. errors.Is(legacyErr, storage.ErrNotFound{}) is going to return false, until legacyErr gets its own Is() that does type checking—but that's not going to happen until (a) we merge & release https://github.com/ipfs/go-ipld-format/pull/76 and (b) that new version bubbles up through everywhere that needs it. Which is very annoying, because errors.Is() handles unwrapping nicely, but it still only uses the unwrapped error's Is() to do the check.

So what I've also done is changed IsNotFound(err) to do something like the inverse. It'll take err, do a feature-test and return true if that passes. If not, it'll unwrap and continue. So it ends up being the universal check, and when we put the same thing in go-ipld-format, it'll do the same thing, for old and new errors, wrapped and unwrapped, from whatever repo. But errors.Is() still has a limitation.

Thoughts?

rvagg avatar Feb 17 '23 03:02 rvagg

As long as we have a recommendation between the two in comments on the two function, i think it's fine to move forward here

willscott avatar Feb 21 '23 21:02 willscott

Slight change of approach here - instead of using a Key property to be compatible with the storage API notion of key string in all of its operations, I've switched it to Cid so now it's ABI compatible with github.com/ipfs/go-ipld-format#ErrNotFound. It's a slight compromise, but it's also not the end of the world if your "not found" error doesn't have the key in it that you were looking up AND those operations are unlikely to be very common with these interfaces anyway.

rvagg avatar Jun 08 '23 05:06 rvagg