Fix: Do not crash when passing null values to @nullable methods, eg U…

…ser and Scope (#1133)

CHANGELOG.md

@@ -9,6 +9,7 @@
 * Enhancement: Read tracesSampleRate from AndroidManifest
 * Fix: Initialize Logback after context refreshes (#1129)
 * Ref: Return NoOpTransaction instead of null (#1126)
+* Fix: Do not crash when passing null values to @Nullable methods, eg User and Scope
 
 # 4.0.0-alpha.2
 

sentry-test-support/src/main/kotlin/io/sentry/test/reflection.kt

@@ -5,3 +5,8 @@ inline fun <reified T : Any> T.injectForField(name: String, value: Any?) {
         .apply { isAccessible = true }
         .set(this, value)
 }
+
+inline fun <reified T : Any> T.callMethod(name: String, parameterTypes: Class<*>, value: Any?) {
+    T::class.java.getDeclaredMethod(name, parameterTypes)
+            .invoke(this, value)
+}

sentry/src/main/java/io/sentry/Scope.java

@@ -83,7 +83,7 @@ public Scope(final @NotNull SentryOptions options) {
    *
    * @param level the SentryLevel
    */
-  public void setLevel(@Nullable SentryLevel level) {
+  public void setLevel(final @Nullable SentryLevel level) {
     this.level = level;
   }
 
@@ -150,7 +150,7 @@ public void setTransaction(final @NotNull ITransaction transaction) {
    *
    * @param user the user
    */
-  public void setUser(@Nullable User user) {
+  public void setUser(final @Nullable User user) {
     this.user = user;
 
     if (options.isEnableScopeSync()) {
@@ -175,7 +175,10 @@ List<String> getFingerprint() {
    *
    * @param fingerprint the fingerprint list
    */
-  public void setFingerprint(@NotNull List<String> fingerprint) {
+  public void setFingerprint(final @NotNull List<String> fingerprint) {
+    if (fingerprint == null) {
+      return;
+    }
     this.fingerprint = fingerprint;
   }
 
@@ -251,7 +254,7 @@ public void addBreadcrumb(@NotNull Breadcrumb breadcrumb, final @Nullable Object
    *
    * @param breadcrumb the breadcrumb
    */
-  public void addBreadcrumb(@NotNull Breadcrumb breadcrumb) {
+  public void addBreadcrumb(final @NotNull Breadcrumb breadcrumb) {
     addBreadcrumb(breadcrumb, null);
   }
 
@@ -305,7 +308,7 @@ Map<String, String> getTags() {
    * @param key the key
    * @param value the value
    */
-  public void setTag(@NotNull String key, @NotNull String value) {
+  public void setTag(final @NotNull String key, final @NotNull String value) {
     this.tags.put(key, value);
 
     if (options.isEnableScopeSync()) {
@@ -320,7 +323,7 @@ public void setTag(@NotNull String key, @NotNull String value) {
    *
    * @param key the key
    */
-  public void removeTag(@NotNull String key) {
+  public void removeTag(final @NotNull String key) {
     this.tags.remove(key);
 
     if (options.isEnableScopeSync()) {
@@ -346,7 +349,7 @@ Map<String, Object> getExtras() {
    * @param key the key
    * @param value the value
    */
-  public void setExtra(@NotNull String key, @NotNull String value) {
+  public void setExtra(final @NotNull String key, final @NotNull String value) {
     this.extra.put(key, value);
 
     if (options.isEnableScopeSync()) {
@@ -361,7 +364,7 @@ public void setExtra(@NotNull String key, @NotNull String value) {
    *
    * @param key the key
    */
-  public void removeExtra(@NotNull String key) {
+  public void removeExtra(final @NotNull String key) {
     this.extra.remove(key);
 
     if (options.isEnableScopeSync()) {
@@ -542,7 +545,7 @@ List<EventProcessor> getEventProcessors() {
    *
    * @param eventProcessor the event processor
    */
-  public void addEventProcessor(@NotNull EventProcessor eventProcessor) {
+  public void addEventProcessor(final @NotNull EventProcessor eventProcessor) {
     eventProcessors.add(eventProcessor);
   }
 
@@ -553,7 +556,7 @@ public void addEventProcessor(@NotNull EventProcessor eventProcessor) {
    * @return a clone of the Session after executing the callback and mutating the session
    */
   @Nullable
-  Session withSession(@NotNull IWithSession sessionCallback) {
+  Session withSession(final @NotNull IWithSession sessionCallback) {
     Session cloneSession = null;
     synchronized (sessionLock) {
       sessionCallback.accept(session);

sentry/src/main/java/io/sentry/protocol/User.java

@@ -52,7 +52,7 @@ public final class User implements Cloneable, IUnknownPropertiesConsumer {
    *
    * @param email the e-mail.
    */
-  public void setEmail(@Nullable String email) {
+  public void setEmail(final @Nullable String email) {
     this.email = email;
   }
 
@@ -70,7 +70,7 @@ public void setEmail(@Nullable String email) {
    *
    * @param id the user id.
    */
-  public void setId(@Nullable String id) {
+  public void setId(final @Nullable String id) {
     this.id = id;
   }
 
@@ -88,7 +88,7 @@ public void setId(@Nullable String id) {
    *
    * @param username the username.
    */
-  public void setUsername(@Nullable String username) {
+  public void setUsername(final @Nullable String username) {
     this.username = username;
   }
 
@@ -106,7 +106,7 @@ public void setUsername(@Nullable String username) {
    *
    * @param ipAddress the IP address of the user.
    */
-  public void setIpAddress(@Nullable String ipAddress) {
+  public void setIpAddress(final @Nullable String ipAddress) {
     this.ipAddress = ipAddress;
   }
 
@@ -124,8 +124,12 @@ public void setIpAddress(@Nullable String ipAddress) {
    *
    * @param other the other user related data..
    */
-  public void setOthers(@Nullable Map<String, String> other) {
-    this.other = new ConcurrentHashMap<>(other);
+  public void setOthers(final @Nullable Map<String, String> other) {
+    if (other != null) {
+      this.other = new ConcurrentHashMap<>(other);
+    } else {
+      this.other = null;
+    }
   }
 
   /**
@@ -135,7 +139,7 @@ public void setOthers(@Nullable Map<String, String> other) {
    */
   @ApiStatus.Internal
   @Override
-  public void acceptUnknownProperties(Map<String, Object> unknown) {
+  public void acceptUnknownProperties(final @NotNull Map<String, Object> unknown) {
     this.unknown = new ConcurrentHashMap<>(unknown);
   }
 
@@ -145,6 +149,7 @@ public void acceptUnknownProperties(Map<String, Object> unknown) {
    * @return the unknown map
    */
   @TestOnly
+  @Nullable
   Map<String, Object> getUnknown() {
     return unknown;
   }

sentry/src/test/java/io/sentry/ScopeTest.kt

@@ -6,6 +6,7 @@ import com.nhaarman.mockitokotlin2.mock
 import com.nhaarman.mockitokotlin2.never
 import com.nhaarman.mockitokotlin2.verify
 import io.sentry.protocol.User
+import io.sentry.test.callMethod
 import java.util.concurrent.CopyOnWriteArrayList
 import kotlin.test.Test
 import kotlin.test.assertEquals
@@ -660,4 +661,15 @@ class ScopeTest {
         assertNotSame(scope.attachments, scope.attachments,
                 "Scope.attachments must return a new instance on each call.")
     }
+
+    @Test
+    fun `setting null fingerprint do not overwrite current value`() {
+        val scope = Scope(SentryOptions())
+        // sanity check
+        assertNotNull(scope.fingerprint)
+
+        scope.callMethod("setFingerprint", List::class.java, null)
+
+        assertNotNull(scope.fingerprint)
+    }
 }

sentry/src/test/java/io/sentry/protocol/UserTest.kt

@@ -4,20 +4,12 @@ import kotlin.test.Test
 import kotlin.test.assertEquals
 import kotlin.test.assertNotNull
 import kotlin.test.assertNotSame
+import kotlin.test.assertNull
 
 class UserTest {
     @Test
     fun `cloning user wont have the same references`() {
-        val user = User()
-        user.email = "a@a.com"
-        user.id = "123"
-        user.ipAddress = "123.x"
-        user.username = "userName"
-        val others = mutableMapOf(Pair("others", "others"))
-        user.others = others
-        val unknown = mapOf(Pair("unknown", "unknown"))
-        user.acceptUnknownProperties(unknown)
-
+        val user = createUser()
         val clone = user.clone()
 
         assertNotNull(clone)
@@ -30,38 +22,20 @@ class UserTest {
 
     @Test
     fun `cloning user will have the same values`() {
-        val user = User()
-        user.email = "a@a.com"
-        user.id = "123"
-        user.ipAddress = "123.x"
-        user.username = "userName"
-        val others = mutableMapOf(Pair("others", "others"))
-        user.others = others
-        val unknown = mapOf(Pair("unknown", "unknown"))
-        user.acceptUnknownProperties(unknown)
-
+        val user = createUser()
         val clone = user.clone()
 
         assertEquals("a@a.com", clone.email)
         assertEquals("123", clone.id)
         assertEquals("123.x", clone.ipAddress)
         assertEquals("userName", clone.username)
         assertEquals("others", clone.others!!["others"])
-        assertEquals("unknown", clone.unknown["unknown"])
+        assertEquals("unknown", clone.unknown!!["unknown"])
     }
 
     @Test
     fun `cloning user and changing the original values wont change the clone values`() {
-        val user = User()
-        user.email = "a@a.com"
-        user.id = "123"
-        user.ipAddress = "123.x"
-        user.username = "userName"
-        val others = mutableMapOf(Pair("others", "others"))
-        user.others = others
-        val unknown = mapOf(Pair("unknown", "unknown"))
-        user.acceptUnknownProperties(unknown)
-
+        val user = createUser()
         val clone = user.clone()
 
         user.email = "b@b.com"
@@ -79,7 +53,28 @@ class UserTest {
         assertEquals("userName", clone.username)
         assertEquals("others", clone.others!!["others"])
         assertEquals(1, clone.others!!.size)
-        assertEquals("unknown", clone.unknown["unknown"])
-        assertEquals(1, clone.unknown.size)
+        assertEquals("unknown", clone.unknown!!["unknown"])
+        assertEquals(1, clone.unknown!!.size)
+    }
+
+    @Test
+    fun `setting null others do not crash`() {
+        val user = createUser()
+        user.others = null
+
+        assertNull(user.others)
+    }
+
+    private fun createUser(): User {
+        return User().apply {
+            email = "a@a.com"
+            id = "123"
+            ipAddress = "123.x"
+            username = "userName"
+            val others = mutableMapOf(Pair("others", "others"))
+            setOthers(others)
+            val unknown = mapOf(Pair("unknown", "unknown"))
+            acceptUnknownProperties(unknown)
+        }
     }
 }