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

Use standard not found error (probably no merge) #493

Closed
wants to merge 1 commit into from

Conversation

hannahhoward
Copy link
Collaborator

Goals

The memstore should return the standard "not found" error for stores that don't have a thing. Sadly, this is currently legacy go-ipld-format ErrNotFound.

implementation

Import go-ipld-format's ErrNotFound, use it. Obviously, this is terrible.

I think the proper solution is to move that error to this repo, then alias it in go-ipld-format.

Mainly wanted to raise this to provoke the discussion cause I was surprised for find format.IsNotFound not working with memstore in a test. I'll work around.

@rvagg
Copy link
Member

rvagg commented Feb 15, 2023

Yeah, you're right, this is not pleasant and I certainly don't want to pull that in here, especially when there's a good chance it's going to be sucked into the go-libipfs vortex.

But I proposed an alternative in https://github.com/ipld/go-car/blob/master/v2/storage/notfound.go and suggested that the implementation there be brought over here. Instead of using the concrete type we should be doing what the IsNotFound() does there:

	if nf, ok := err.(interface{ NotFound() bool }); ok {
		return nf.NotFound()
	}

Which would also work for go-block-format—the next step would be to update that error there with that. But it'd technically be a "breaking" change because it's not quite as strict, so it's not perfect and also means anyone using older go-block-format would not match the newer form because of the strictness.

rvagg added a commit that referenced this pull request 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
@rvagg rvagg mentioned this pull request Feb 15, 2023
rvagg added a commit that referenced this pull request 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
rvagg added a commit that referenced this pull request 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
@BigLep
Copy link

BigLep commented Feb 21, 2023

Closing in favor of #494

@BigLep BigLep closed this Feb 21, 2023
rvagg added a commit that referenced this pull request Mar 28, 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
rvagg added a commit that referenced this pull request Mar 31, 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
rvagg added a commit that referenced this pull request Jun 8, 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
rvagg added a commit that referenced this pull request Jun 13, 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: 🎉 Done
Development

Successfully merging this pull request may close these issues.

3 participants