Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: add ErrNotFound #494

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

feat: add ErrNotFound #494

wants to merge 4 commits into from

Conversation

rvagg
Copy link
Member

@rvagg rvagg commented Feb 15, 2023

  • 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: ipld/go-car#363
Ref: #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 filecoin-project/lassie#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 rvagg force-pushed the rvagg/notfound branch 2 times, most recently from 07c5309 to 7b069ee Compare February 15, 2023 08:30
Copy link
Member

@masih masih left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Joy 🎉 thank you

rvagg added a commit to ipfs/go-ipld-format that referenced this pull request Feb 16, 2023
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
rvagg added a commit to ipfs/go-ipld-format that referenced this pull request Feb 16, 2023
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
@rvagg
Copy link
Member Author

rvagg commented Feb 16, 2023

go-ipld-format version of this ipfs/go-ipld-format#76

@rvagg
Copy link
Member Author

rvagg commented Feb 17, 2023

@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 ipfs/go-ipld-format#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?

@willscott
Copy link
Member

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

@rvagg rvagg force-pushed the rvagg/notfound branch 2 times, most recently from d5ef532 to eee16ca Compare March 31, 2023 03:55
@rvagg rvagg force-pushed the rvagg/notfound branch from eee16ca to 64d7e0e Compare June 8, 2023 04:54
rvagg added a commit to ipfs/go-ipld-format that referenced this pull request Jun 8, 2023
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
@rvagg rvagg force-pushed the rvagg/notfound branch from 64d7e0e to 9846442 Compare June 8, 2023 05:00
@rvagg
Copy link
Member Author

rvagg commented Jun 8, 2023

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 added 4 commits June 13, 2023 21:08
* 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: ipld/go-car#363
Ref: #493
ABI compatible with github.com/ipfs/go-ipld-format#ErrNotFound for smoother
upgrade path
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: 🏃‍♀️ In Progress
Development

Successfully merging this pull request may close these issues.

3 participants