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: check that the CidBuilder hasher is usable #91

Merged
merged 1 commit into from
Sep 30, 2022

Conversation

rvagg
Copy link
Member

@rvagg rvagg commented Sep 28, 2022

Ref: #90

When using SetCidBuilder you can set up for a hasher that will not be found when a Sum() is eventually called in EncodeProtobuf(), and the error from this doesn't bubble up through RawData(), it just panics.

So this change adds an error return to SetCidBuilder() and will fail to use the builder if it's going to result in that error. If the error isn't handled then you'll get a silent default to a (presumably) working builder. If it's checked then at least you'll have an error to bubble up to the user or some place helpful.

However, the caveat here is that we are not actually testing Sum() will work, only the obvious path to the error. If we are given a custom CidBuilder that doesn't match Prefix, or if there is a custom Sum() that doesn't use MhType then this isn't going to work. But I'm not sure what that situation is.

This is a soft option for dealing with #90.

Copy link
Contributor

@Jorropo Jorropo left a comment

Choose a reason for hiding this comment

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

This is better, now all consumers are protected to what we found in Kubo.

@rvagg
Copy link
Member Author

rvagg commented Sep 29, 2022

@Jorropo got an opinion on version bump for this? I'm not really sure how a non forward-breaking in a <0 semver situation would normally be handled. In >1 I'd probably just bump minor version for this so I'm tempted to say in <0 that it's a patch bump and let static analysis pick up the change for consumers.

@rvagg
Copy link
Member Author

rvagg commented Sep 29, 2022

Needs to be a semver-minor bump because it's a method so it impacts the type's interface.

I'm also considering further testing that if the builder isn't a cid.Prefix running a Sum() on it with a simple value (empty byte array?) to test that it actually works.

@Jorropo
Copy link
Contributor

Jorropo commented Sep 29, 2022

@rvagg I don't fully understand where is the API changing here ?
Because it wouldn't work either way, before it would panic later, but now it's an error earlier.
AFAIT there is no way to use this code with broken hasher, before it would panic or error when getting bytes, now it's when setting a build.

I'm also considering further testing that if the builder isn't a cid.Prefix running a Sum() on it with a simple value (empty byte array?) to test that it actually works.

Yeah, this would also work in-case a custom builder is passed, I think it's fine.

node.go Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants