-
Notifications
You must be signed in to change notification settings - Fork 34
Description
When Base64.decode(CharSequence in)
decodes a string, in some cases for "bad" strings in throws IllegalArgumentException
:
throw new IllegalArgumentException("invalid character to decode: " + c); |
, but in other cases it returns null
.
bytes-java/src/main/java/at/favre/lib/bytes/Base64.java
Lines 108 to 110 in e622b3b
if (lastWordChars == 1) { | |
// We read 1 char followed by "===". But 6 bits is a truncated byte! Fail. | |
return null; |
This makes the method harder to use as the caller needs to handle both cases for every call with potentially invalid input: catching IllegalArgumentException
and comparing the result with null
.
I think the best way to handle both types of invalid input would be to throw IllegalArgumentException
in both cases and never return null
.
This behavior is even tested:
assertNull(Base64.decode("a\r\t\n ")); |
It makes me think that this is the desired behavior, but why?
Btw, this behavior actually leads to the bug in the id-mask
library. That is how I actually found it.
When I do idMask.unmask(badlyEncodedString)
, then it eventually calls Base64.decode
, that returns null
, and then it tries to wrap it into Bytes
object
bytes-java/src/main/java/at/favre/lib/bytes/Bytes.java
Lines 155 to 157 in e622b3b
public static Bytes wrap(byte[] array, ByteOrder byteOrder) { | |
return new Bytes(Objects.requireNonNull(array, "passed array must not be null"), byteOrder); | |
} |
, that leads to java.lang.NullPointerException: passed array must not be null
. That is unexpected behavior I believe. idMask.unmask(badlyEncodedString)
should never throw NPE.
Of course, this problem could be solved on the id-mask
side (by catching NPE and throwing IllegalArgumentException
), but I think the root problem is the inconsistent behavior of Base64.decode
. I can't imagine a situation when it is the desired behavior to sometimes throw IllegalArgumentException
and sometimes return null
for basically the same case of "invalid input".
Why it is important to throw IllegalArgumentException
and not NullPointerException
? Because the 1st error means the error is on the caller side (probably someone called the API with a wrong encoded ID), while the 2nd one means that something wrong happened inside the library. NPE would probably be acceptable in the case when input
CharSequence is null
. But if it is a non-null string, then NPE should never be thrown except for a bug in the implementation.