Add in new methods which don't raise, and unify bang semantics across RbNaCl #90
Conversation
|
I guess my main question is: what advantage do the non-raising versions confer? I think we can all agree that the raising versions will help prevent silent errors. Why have a non-raising option? I would say that a failing verifier is always an exceptional case. If things are working correctly, we should never seen exceptions raised. |
|
To allow for situations where exceptions are not used for flow control (but with the default being to raise). This is more true for checking of signatures and authenticators than for ciphertexts, but it could be true for ciphertexts too. If there are only raising versions, you will end up with a lot of cases where all cases are wrapped in an immediate |
|
Upon further reflection, I think this is a really bad change. I understand you don't like the ugliness of Worse, the semantics you are using here are the opposite of the "Rails" convention. I understand your motivation, and think it's the "right thing" per Matz's philosophy of bang methods being dangerous. I am worried that by having a non-raising version that does the opposite of the existing convention, a user of the library could get confused and have an unchecked verification failure because the bang method returned false instead of raising. I'd really rather just not include this. I think it has nebulous gains in terms of code quality while also reducing the safety of the library. |
|
It makes me so sad that rails is why we can't have nice things :/ : I find it hard to argue with you, because I can see the potential for breakage, too. Ah well. |
|
Though if you're set on this, we should remove the non-raising signature verification and authenticator verification as it currently exists in the API |
|
I would suggest shipping 2.0 without them, then maybe looping back on it and adding them in 2.1 if we can agree on the API. What you're trying to do is a purely additive change so it wouldn't break semantic versioning to add an API like this in a subsequent release. Totally agree on changing the verify APIs that utilize bang methods now to raise by default on the non-bang APIs (and getting rid of the bang versions) prior to shipping 2.0. Alternatively we could just adopt the Rails convention and call it good :| |
|
The problem with the rails convention is that has the "normal" methods unsafe, with an explicit need to check return values, instead of being "safe". This is why it makes me so sad rails has confused the matter. I'll work this over to making the defaults all raise, inc documenting that. |
|
+1 on that. If the Rails convention is screwed up we should just make the safe thing the only option. |
This now exposes the libsodium version as: * RbNaCl::Sodium::Version::STRING * RbNaCl::Sodium::Version::MAJOR * RbNaCl::Sodium::Version::MINOR * RbNaCl::Sodium::Version::PATCH The version check now compares the required version against the installed version by first parsing them as integers and then using the spaceship operator. The minimum version of libsodium has been changed to 0.4.3.
Now all three (authenticators, signatures and boxes) will raise if there are errors in verifying them. This ensures that errors are dealt with, or the program comes crashing to a halt. Self-tests are also updated.
|
Squshing messed this up, closing |
Now all authenticatos, signatures and boxes have a non-raising version of their check/open function, which is the bang method, and a raising version which is the plain one.
This is a switch for authenticators and signatures, but it is consistent with the bang methods being dangerous (i.e. they could let an invalid signature pass unchecked and unnoticed - the side-effect is a possible lack of security.)