Fix: NPE when adding Context Data with null values for log4j2 (#1465)

Fixes #1386
getsentry · May 7, 2021 · 5fa4b52 · 5fa4b52
1 parent ada5080
commit 5fa4b52
Show file tree

Hide file tree

Showing 10 changed files with 134 additions and 10 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -5,6 +5,8 @@
 * Fix: sentry-android-timber package sets sentry.java.android.timber as SDK name (#1456)
 * Fix: When AppLifecycleIntegration is closed, it should remove observer using UI thread (#1459)
 * Bump: AGP to 4.2.0 (#1460)
+* Fix: NPE when adding Context Data with null values for log4j2 (#1465)
+
 
 Breaking Changes:
 

diff --git a/sentry-jul/src/main/java/io/sentry/jul/SentryHandler.java b/sentry-jul/src/main/java/io/sentry/jul/SentryHandler.java
@@ -8,6 +8,7 @@
 import io.sentry.SentryOptions;
 import io.sentry.protocol.Message;
 import io.sentry.protocol.SdkVersion;
+import io.sentry.util.CollectionUtils;
 import java.text.MessageFormat;
 import java.util.ArrayList;
 import java.util.Date;
@@ -19,7 +20,6 @@
 import java.util.logging.Level;
 import java.util.logging.LogManager;
 import java.util.logging.LogRecord;
-import java.util.stream.Collectors;
 import org.jetbrains.annotations.NotNull;
 import org.jetbrains.annotations.Nullable;
 import org.jetbrains.annotations.TestOnly;
@@ -188,9 +188,7 @@ SentryEvent createEvent(final @NotNull LogRecord record) {
     Map<String, String> mdcProperties = MDC.getMDCAdapter().getCopyOfContextMap();
     if (mdcProperties != null) {
       mdcProperties =
-          mdcProperties.entrySet().stream()
-              .filter(it -> it.getValue() != null)
-              .collect(Collectors.toMap(Map.Entry::getKey, Map.Entry::getValue));
+          CollectionUtils.filterMapEntries(mdcProperties, entry -> entry.getValue() != null);
       if (!mdcProperties.isEmpty()) {
         event.getContexts().put("MDC", mdcProperties);
       }

diff --git a/sentry-jul/src/test/kotlin/io/sentry/jul/SentryHandlerTest.kt b/sentry-jul/src/test/kotlin/io/sentry/jul/SentryHandlerTest.kt
@@ -19,6 +19,7 @@ import kotlin.test.Test
 import kotlin.test.assertEquals
 import kotlin.test.assertFalse
 import kotlin.test.assertNotNull
+import kotlin.test.assertNull
 import kotlin.test.assertTrue
 import org.awaitility.kotlin.await
 import org.slf4j.MDC
@@ -290,12 +291,31 @@ class SentryHandlerTest {
     @Test
     fun `ignore set tags with null values from MDC`() {
         fixture = Fixture(minimumEventLevel = Level.WARNING)
-        MDC.put("key", null)
+        MDC.put("key1", null)
+        MDC.put("key2", "value")
         fixture.logger.warning("testing MDC tags")
 
         await.untilAsserted {
             verify(fixture.transport).send(checkEvent { event ->
-                assertFalse(event.contexts.containsKey("MDC"))
+                assertNotNull(event.contexts["MDC"]) {
+                    val contextData = it as Map<*, *>
+                    assertNull(contextData["key1"])
+                    assertEquals("value", contextData["key2"])
+                }
+            }, anyOrNull())
+        }
+    }
+
+    @Test
+    fun `does not set MDC if all context entries are null`() {
+        fixture = Fixture(minimumEventLevel = Level.WARNING)
+        MDC.put("key1", null)
+        MDC.put("key2", null)
+        fixture.logger.warning("testing MDC tags")
+
+        await.untilAsserted {
+            verify(fixture.transport).send(checkEvent { event ->
+                assertNull(event.contexts["MDC"])
             }, anyOrNull())
         }
     }

diff --git a/sentry-log4j2/src/main/java/io/sentry/log4j2/SentryAppender.java b/sentry-log4j2/src/main/java/io/sentry/log4j2/SentryAppender.java
@@ -149,7 +149,8 @@ public void append(final @NotNull LogEvent eventObject) {
     }
 
     final Map<String, String> contextData =
-        CollectionUtils.shallowCopy(loggingEvent.getContextData().toMap());
+        CollectionUtils.filterMapEntries(
+            loggingEvent.getContextData().toMap(), entry -> entry.getValue() != null);
     if (!contextData.isEmpty()) {
       event.getContexts().put("Context Data", contextData);
     }

diff --git a/sentry-log4j2/src/test/kotlin/io/sentry/log4j2/SentryAppenderTest.kt b/sentry-log4j2/src/test/kotlin/io/sentry/log4j2/SentryAppenderTest.kt
@@ -20,6 +20,7 @@ import kotlin.test.Test
 import kotlin.test.assertEquals
 import kotlin.test.assertFalse
 import kotlin.test.assertNotNull
+import kotlin.test.assertNull
 import kotlin.test.assertTrue
 import org.apache.logging.log4j.Level
 import org.apache.logging.log4j.LogManager
@@ -227,6 +228,38 @@ class SentryAppenderTest {
         }
     }
 
+    @Test
+    fun `ignore set tags with null values from ThreadContext`() {
+        val logger = fixture.getSut(minimumEventLevel = Level.WARN)
+        ThreadContext.put("key1", null)
+        ThreadContext.put("key2", "value")
+        logger.warn("testing MDC tags")
+
+        await.untilAsserted {
+            verify(fixture.transport).send(checkEvent { event ->
+                assertNotNull(event.contexts["Context Data"]) {
+                    val contextData = it as Map<*, *>
+                    assertNull(contextData["key1"])
+                    assertEquals("value", contextData["key2"])
+                }
+            }, anyOrNull())
+        }
+    }
+
+    @Test
+    fun `does not set context data if all context entries are null`() {
+        val logger = fixture.getSut(minimumEventLevel = Level.WARN)
+        ThreadContext.put("key1", null)
+        ThreadContext.put("key2", null)
+        logger.warn("testing MDC tags")
+
+        await.untilAsserted {
+            verify(fixture.transport).send(checkEvent { event ->
+                assertNull(event.contexts["Context Data"])
+            }, anyOrNull())
+        }
+    }
+
     @Test
     fun `does not create MDC context when no MDC tags are set`() {
         val logger = fixture.getSut(minimumEventLevel = Level.WARN)

diff --git a/sentry-logback/src/main/java/io/sentry/logback/SentryAppender.java b/sentry-logback/src/main/java/io/sentry/logback/SentryAppender.java
@@ -13,6 +13,7 @@
 import io.sentry.SentryOptions;
 import io.sentry.protocol.Message;
 import io.sentry.protocol.SdkVersion;
+import io.sentry.util.CollectionUtils;
 import java.util.Arrays;
 import java.util.Collections;
 import java.util.List;
@@ -92,9 +93,8 @@ protected void append(@NotNull ILoggingEvent eventObject) {
 
     // remove keys with null values, there is no sense to send these keys to Sentry
     final Map<String, String> mdcProperties =
-        loggingEvent.getMDCPropertyMap().entrySet().stream()
-            .filter(it -> it.getValue() != null)
-            .collect(Collectors.toMap(Map.Entry::getKey, Map.Entry::getValue));
+        CollectionUtils.filterMapEntries(
+            loggingEvent.getMDCPropertyMap(), entry -> entry.getValue() != null);
     if (!mdcProperties.isEmpty()) {
       event.getContexts().put("MDC", mdcProperties);
     }

diff --git a/sentry-logback/src/test/kotlin/io/sentry/logback/SentryAppenderTest.kt b/sentry-logback/src/test/kotlin/io/sentry/logback/SentryAppenderTest.kt
@@ -207,6 +207,24 @@ class SentryAppenderTest {
 
     @Test
     fun `ignore set tags with null values from MDC`() {
+        fixture = Fixture(minimumEventLevel = Level.WARN)
+        MDC.put("key1", null)
+        MDC.put("key2", "value")
+        fixture.logger.warn("testing MDC tags")
+
+        await.untilAsserted {
+            verify(fixture.transport).send(checkEvent { event ->
+                assertNotNull(event.contexts["MDC"]) {
+                    val contextData = it as Map<*, *>
+                    assertNull(contextData["key1"])
+                    assertEquals("value", contextData["key2"])
+                }
+            }, anyOrNull())
+        }
+    }
+
+    @Test
+    fun `does not set MDC if all context entries are null`() {
         fixture = Fixture(minimumEventLevel = Level.WARN)
         MDC.put("key1", null)
         MDC.put("key2", null)

diff --git a/sentry/api/sentry.api b/sentry/api/sentry.api
@@ -1860,10 +1860,15 @@ public final class io/sentry/util/ApplyScopeUtils {
 }
 
 public final class io/sentry/util/CollectionUtils {
+	public static fun filterMapEntries (Ljava/util/Map;Lio/sentry/util/CollectionUtils$Predicate;)Ljava/util/Map;
 	public static fun shallowCopy (Ljava/util/Map;)Ljava/util/Map;
 	public static fun size (Ljava/lang/Iterable;)I
 }
 
+public abstract interface class io/sentry/util/CollectionUtils$Predicate {
+	public abstract fun test (Ljava/lang/Object;)Z
+}
+
 public final class io/sentry/util/LogUtils {
 	public fun <init> ()V
 	public static fun logIfNotFlushable (Lio/sentry/ILogger;Ljava/lang/Object;)V

diff --git a/sentry/src/main/java/io/sentry/util/CollectionUtils.java b/sentry/src/main/java/io/sentry/util/CollectionUtils.java
@@ -1,6 +1,7 @@
 package io.sentry.util;
 
 import java.util.Collection;
+import java.util.HashMap;
 import java.util.Map;
 import java.util.concurrent.ConcurrentHashMap;
 import org.jetbrains.annotations.ApiStatus;
@@ -45,4 +46,33 @@ public static int size(Iterable<?> data) {
       return null;
     }
   }
+
+  /**
+   * Returns a new map which entries match a predicate specified by a parameter.
+   *
+   * @param map - the map to filter
+   * @param predicate - the predicate
+   * @param <K> - map entry key type
+   * @param <V> - map entry value type
+   * @return a new map
+   */
+  public static @NotNull <K, V> Map<K, V> filterMapEntries(
+      final @NotNull Map<K, V> map, final @NotNull Predicate<Map.Entry<K, V>> predicate) {
+    final Map<K, V> filteredMap = new HashMap<>();
+    for (final Map.Entry<K, V> entry : map.entrySet()) {
+      if (predicate.test(entry)) {
+        filteredMap.put(entry.getKey(), entry.getValue());
+      }
+    }
+    return filteredMap;
+  }
+
+  /**
+   * A simplified copy of Java 8 Predicate.
+   *
+   * @param <T> the type
+   */
+  public interface Predicate<T> {
+    boolean test(T t);
+  }
 }
diff --git a/sentry/src/test/java/io/sentry/util/CollectionUtilsTest.kt b/sentry/src/test/java/io/sentry/util/CollectionUtilsTest.kt
@@ -0,0 +1,17 @@
+package io.sentry.util
+
+import kotlin.test.Test
+import kotlin.test.assertEquals
+
+class CollectionUtilsTest {
+
+    @Test
+    fun `filters out map not maching predicate`() {
+        val map = mapOf("key1" to 1, "key2" to 2, "key3" to 3)
+        val result = CollectionUtils.filterMapEntries(map) {
+            it.value % 2 == 0
+        }
+        assertEquals(2, result["key2"])
+        assertEquals(1, result.size)
+    }
+}