Putting null keys on a MapMessage shouldn't be throwing exception

[SubieGold]

Looks like using MapMessage with null values causes a IllegalArgumentException to be thrown. I think that MapMessage should be able to accept nulls or the logger should be able to handle the exception.

logging-log4j2/log4j-api/src/main/java/org/apache/logging/log4j/message/MapMessage.java

Line 205 in 613b520

throw new IllegalArgumentException("No value provided for key " + candidateKey);

[vy]

@LoganDev99, you might indeed be right. Do you have a particular use case that caused this exception?

[SubieGold]

We were logging some data from another API that unfortunately returned a null. Logging the null would have been perfectly fine since the application would have just continued.

[vy]

Okay, I see your point. I don't know whether MapMessage (and its subclasses) should register an entry with null key or simply discard it. I would like to hear your thoughts on this too.

All in all, feel free to submit a PR. Though note that I would prefer to see this new behaviour in MapMessage subclasses too.

[SubieGold]

Perhaps it can accept the key and value and just log string null?

[ppkarwasz]

Okay, I see your point. I don't know whether MapMessage (and its subclasses) should register an entry with null key or simply discard it. I would like to hear your thoughts on this too.

For reference SLF4J discards all calls to MDC.pushByKey if the key is null (its implementation is based on ThreadLocalMapOfStacks).

I think that discarding it is the safest choice: if the context data is copied into a map that does not accept nulls it could cause problems downstream.

[rgoers]

@ppkarwasz Comparing how SLF4J handles a ThreadContext map entry vs how we handle a MapMessage should not be considered to have the same behavior. That said, I don't believe we should always accept nulls. At best, it is fair to say that the behavior when a null value is passed is undefined (or up to the implementation).

[ppkarwasz]

Agreed,

I was a little bit bluffing with the null key. As far as our MDCAdapter implementation goes, I use a null key passed to pushByKey/popByKey to allow users to access our NDC.