Add null check for BaseNCodec decodning

[mipo256]

Add null check for BaseNCodec decodning

[markt-asf]

Why?

[mipo256]

Becuase this code, for example : new Base64().decode(myString) will produce NPE in case of myString is null. This is becuase of inner delegation to public byte[] decode(final byte[] pArray) method (in BaseNCodec class), which cannot handle null values.

[markt-asf]

So don't call decode((String) null).

[gmshake]

Becuase this code, for example : new Base64().decode(myString) will produce NPE in case of myString is null. This is becuase of inner delegation to public byte[] decode(final byte[] pArray) method (in BaseNCodec class), which cannot handle null values.

It make sense to encode a empty string, but it does not make sense to encode or decode a null string.

[mipo256]

But guys, I am, as a framework user, expect that the framework will take care of this check. Do you really think I am doing the
decode((String) null) - I have a string in application, which can be null, and I, as a framework user, do not want to take care about making checks about it.

Indeed, fellows, I guess you have used Apache Commons. Take a look into its utility classes - they withdraw this boring responsibilities from the framework client.

[mipo256]

Becuase this code, for example : new Base64().decode(myString) will produce NPE in case of myString is null. This is becuase of inner delegation to public byte[] decode(final byte[] pArray) method (in BaseNCodec class), which cannot handle null values.

It make sense to encode a empty string, but it does not make sense to encode or decode a null string.

I agree with that, but it forces the client the be aware of the fact that string is not null. I am responsible to check it, as a Client - why? I genuinely do not understand the logic behind it

[markt-asf]

1. Please avoid use of gender specific terminology such as "guys" or "fellows" when referring to members of the Tomcat community.

2. This code is not part of a framework. It is Tomcat's internal API. Applications should not be calling this code.

The logic behind the current design is simple: Tomcat never calls that method with null so a null check is unnecessary.

[mipo256]

Ok, I am sorry, I will reference you directly :) I disagree with you, @markt-asf. The class Base64 is used directly by the clients. Just a couple of examples: this, and this, also this and e.t.c.) - and, as I already mentioned, Base64 delegates to BaseNCodec, which does not tolerate null.

@markt-asf I really do not understand why it is so hard to apply this minor, non-breaking change :) As I mentioned above, people are using this API in thier code (it is public b.t.w), so why not make their life easier?)

[markt-asf]

All those references you provided are for org.apache.commons.codec.binary.BaseNCodec not org.apache.tomcat.util.codec.binary.BaseNCodec

That the class is public is irrelevant. It has to be public as it is a utility class used by various Tomcat packages. Tomcat's public API is documented in the RELEASE-NOTES.

The change won't be applied because it isn't necessary and there is no desire to add unnecessary code bloat to Tomcat.

It your application wants to use Commons Codec then it should package it in WEB-INF/lib.

[mipo256]

Ok, that make sence. I will use the public API

[ChristopherSchultz]

@mikhail2048 Your patch also doesn't even compile, so it wouldn't have been accepted, anyway.