bytes-java

bytes-java copied to clipboard

When Base64.decode(CharSequence in) decodes a string, in some cases for "bad" strings in throws IllegalArgumentException:

https://github.com/patrickfav/bytes-java/blob/e622b3b4dfe0bca2b65d2dcd6dae78441ee2961a/src/main/java/at/favre/lib/bytes/Base64.java#L92

, but in other cases it returns null.

https://github.com/patrickfav/bytes-java/blob/e622b3b4dfe0bca2b65d2dcd6dae78441ee2961a/src/main/java/at/favre/lib/bytes/Base64.java#L108-L110

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:

https://github.com/patrickfav/bytes-java/blob/e622b3b4dfe0bca2b65d2dcd6dae78441ee2961a/src/test/java/at/favre/lib/bytes/Base64Test.java#L47

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

https://github.com/patrickfav/bytes-java/blob/e622b3b4dfe0bca2b65d2dcd6dae78441ee2961a/src/main/java/at/favre/lib/bytes/Bytes.java#L155-L157

, 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.