Should add null check when put data into concurrenthashmap

[jdneo]

The code here:
https://github.com/Microsoft/ApplicationInsights-Java/blob/v1.0.9/core/src/main/java/com/microsoft/applicationinsights/internal/util/MapUtil.java#L43

put data into a ConCurrentHashMap but does not have null check.

which will casue an NPE error if the entry.getValue() is null.

[dhaval24]

@jdneo thanks for pointing this out. By the way recent master also might have the same issue. Is it possible for you to send out a PR for this. Your contribution would be greatly appreciated. Let me know.

[jdneo]

Hi @dhaval24 Sure, I'll send a PR sooner.

[dhaval24]

@jdneo thank you very much. Just curios have you encountered this issue while SDK is running? Wondering how you caught this up.

[jdneo]

Yes, some of our users encounter this problem.

Here are some related links:
https://docs.microsoft.com/en-us/java/azure/spring-framework/deploy-spring-boot-java-app-with-maven-plugin#configure-maven-to-use-your-azure-service-principal
microsoft/azure-maven-plugins#73

[dhaval24]

I see. So just was checking the java docs and found that standard maps allow null keys and values but concurrent map doesn't allow them. Since this method is generic for any kind of maps it would make sense to apply check with condition on type too (source instance of ConcurrentHashMap && entry.getValue() != null)

[jdneo]

Yes, but if entry.getValue() is null, what should we do? Since we have no ide what type is for Value

[dhaval24]

In case of concurrent maps, key with null value doesn't make sense at all so we might skip the key itself. and then log a trace with the help of Internal SDK Logger. (InternalLogger.instance.trace("Skipping key %s while copying because of null value", value)

[jdneo]

@dhaval24 That's fine!

[dhaval24]

@jdneo please also do add a test case for it :-)