Skip to content

Inconsistent result of failed Base64.decode (throws vs returns null) #65

@xak2000

Description

@xak2000

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.

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

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.

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions