go-ipld-prime
go-ipld-prime copied to clipboard
feat: add ErrNotFound
- 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?
go-ipld-format version of this https://github.com/ipfs/go-ipld-format/pull/76
@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?
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
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.