ipfs / go-cid

Content ID v1 implemented in go
MIT License
156 stars 47 forks source link

CidFromReader should not wrap valid EOF return. #151

Closed gammazero closed 1 year ago

gammazero commented 1 year ago

When reading from an io.Reader that has no data, the io.EOF error should not be wrapped in ErrInvalidCid. This is not an invalid CID, and is not the same as a partial read which is indicated by io.ErrUnexpectedEOF.

This fix is needed because existing code that uses CidFromReader may check for the end of an input stream by if err == io.EOF instead of the preferred if errors.Is(err, io.EOF), and that code will break at runtime after upgrading to go-cid v0.4.0.

Fixes #152

rvagg commented 1 year ago

This is a tough call, arguably errors.Is(err, io.EOF) is the right way to deal with this but your code really has limited it to the first byte, so specifically the case where you have a zero-byte reader. I think this deserves some comments which I'll suggest inline. Would you also mind adding a case for >1 byte that fails on the varint call but doesn't return an error - []byte{0x80, 0x01} should be good enough and should return an ErrInvalidCId wrapping an ErrUnexpectedEOF.

rvagg commented 1 year ago

Unfortunately I can't suggest inline for the godoc comment because it's too far out of the diff, but here's what I suggest for clearning up that:

// It's recommended to supply a reader that buffers and implements io.ByteReader,
// as CidFromReader has to do many single-byte reads to decode varints.
// If the argument only implements io.Reader, single-byte Read calls are used instead.
//
// If the Reader is found to yield zero bytes, an io.EOF error is returned directly, in all
// other error cases, an ErrInvalidCid, wrapping the original error, is returned.
func CidFromReader(r io.Reader) (int, Cid, error) {
gammazero commented 1 year ago

@rvagg Added test for ErrUnexpectedEOF and suggested comments.

github-actions[bot] commented 1 year ago

Suggested version: v0.4.1

Comparing to: v0.4.0 (diff)

Changes in go.mod file(s):

(empty)

gorelease says:

# summary
Suggested version: v0.4.1

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.