Avoid NPEs from SDK in API like setTag, setData, setContext

CHANGELOG.md

@@ -2,13 +2,14 @@
 
 ## Unreleased
 
-### Behavioural Changes
-
-- Use `java.net.URI` for parsing URLs in `UrlUtils` ([#4210](https://github.com/getsentry/sentry-java/pull/4210))
-  - This could affect grouping for issues with messages containing URLs that fall in known corner cases that were handled incorrectly previously (e.g. email in URL path)
-
 ### Fixes
 
+- The SDK now handles `null` on many APIs instead of expecting a non `null` value ([#4245](https://github.com/getsentry/sentry-java/pull/4245))
+  - Certain APIs like `setTag`, `setData`, `setExtra`, `setContext` previously caused a `NullPointerException` when invoked with either `null` key or value.
+  - The SDK now tries to have a sane fallback when `null` is passed and no longer throws `NullPointerException`
+  - If `null` is passed, the SDK will
+    - do nothing if a `null` key is passed, returning `null` for non void methods
+    - remove any previous value if the new value is set to `null`
 - Add support for setting in-app-includes/in-app-excludes via AndroidManifest.xml ([#4240](https://github.com/getsentry/sentry-java/pull/4240))
 - Modifications to OkHttp requests are now properly propagated to the affected span / breadcrumbs ([#4238](https://github.com/getsentry/sentry-java/pull/4238))
   - Please ensure the SentryOkHttpInterceptor is added last to your OkHttpClient, as otherwise changes to the `Request`  by subsequent interceptors won't be considered
@@ -22,6 +23,11 @@
   - Set `sentry.capture-open-telemetry-events=true` in Springs `application.properties` to enable it
   - Set `sentry.captureOpenTelemetryEvents: true` in Springs `application.yml` to enable it
 
+### Behavioural Changes
+
+- Use `java.net.URI` for parsing URLs in `UrlUtils` ([#4210](https://github.com/getsentry/sentry-java/pull/4210))
+  - This could affect grouping for issues with messages containing URLs that fall in known corner cases that were handled incorrectly previously (e.g. email in URL path)
+
 ### Internal
 
 - Also use port when checking if a request is made to Sentry DSN ([#4231](https://github.com/getsentry/sentry-java/pull/4231))

sentry-android-core/src/test/java/io/sentry/android/core/InternalSentrySdkTest.kt

@@ -319,7 +319,7 @@ class InternalSentrySdkTest {
     fun `serializeScope provides fallback app data if none is set`() {
         val options = SentryAndroidOptions()
         val scope = Scope(options)
-        scope.setContexts("app", null)
+        scope.setContexts("app", null as Any?)
 
         val serializedScope = InternalSentrySdk.serializeScope(context, options, scope)
         assertTrue(((serializedScope["contexts"] as Map<*, *>)["app"] as Map<*, *>).containsKey("app_name"))

sentry-opentelemetry/sentry-opentelemetry-bootstrap/src/main/java/io/sentry/opentelemetry/OtelStrongRefSpanWrapper.java

@@ -225,12 +225,12 @@ public void setThrowable(@Nullable Throwable throwable) {
   }
 
   @Override
-  public void setTag(@NotNull String key, @NotNull String value) {
+  public void setTag(@Nullable String key, @Nullable String value) {
     delegate.setTag(key, value);
   }
 
   @Override
-  public @Nullable String getTag(@NotNull String key) {
+  public @Nullable String getTag(@Nullable String key) {
     return delegate.getTag(key);
   }
 
@@ -240,12 +240,12 @@ public boolean isFinished() {
   }
 
   @Override
-  public void setData(@NotNull String key, @NotNull Object value) {
+  public void setData(@Nullable String key, @Nullable Object value) {
     delegate.setData(key, value);
   }
 
   @Override
-  public @Nullable Object getData(@NotNull String key) {
+  public @Nullable Object getData(@Nullable String key) {
     return delegate.getData(key);
   }
 
@@ -281,7 +281,7 @@ public boolean isNoOp() {
   }
 
   @Override
-  public void setContext(@NotNull String key, @NotNull Object context) {
+  public void setContext(@Nullable String key, @Nullable Object context) {
     delegate.setContext(key, context);
   }
 

sentry-opentelemetry/sentry-opentelemetry-bootstrap/src/main/java/io/sentry/opentelemetry/OtelTransactionSpanForwarder.java

@@ -152,12 +152,12 @@ public void setThrowable(@Nullable Throwable throwable) {
   }
 
   @Override
-  public void setTag(@NotNull String key, @NotNull String value) {
+  public void setTag(@Nullable String key, @Nullable String value) {
     rootSpan.setTag(key, value);
   }
 
   @Override
-  public @Nullable String getTag(@NotNull String key) {
+  public @Nullable String getTag(@Nullable String key) {
     return rootSpan.getTag(key);
   }
 
@@ -167,12 +167,12 @@ public boolean isFinished() {
   }
 
   @Override
-  public void setData(@NotNull String key, @NotNull Object value) {
+  public void setData(@Nullable String key, @Nullable Object value) {
     rootSpan.setData(key, value);
   }
 
   @Override
-  public @Nullable Object getData(@NotNull String key) {
+  public @Nullable Object getData(@Nullable String key) {
     return rootSpan.getData(key);
   }
 
@@ -277,7 +277,7 @@ public void finish(
   }
 
   @Override
-  public void setContext(@NotNull String key, @NotNull Object context) {
+  public void setContext(@Nullable String key, @Nullable Object context) {
     // thoughts:
     // - span would have to save it on global storage too since we can't add complex data to otel
     // span

sentry-opentelemetry/sentry-opentelemetry-core/src/main/java/io/sentry/opentelemetry/OtelSpanWrapper.java

@@ -330,12 +330,15 @@ public void setThrowable(@Nullable Throwable throwable) {
   }
 
   @Override
-  public void setTag(@NotNull String key, @NotNull String value) {
+  public void setTag(@Nullable String key, @Nullable String value) {
     context.setTag(key, value);
   }
 
   @Override
-  public @Nullable String getTag(@NotNull String key) {
+  public @Nullable String getTag(@Nullable String key) {
+    if (key == null) {
+      return null;
+    }
     return context.getTags().get(key);
   }
 
@@ -357,12 +360,22 @@ public boolean isFinished() {
   }
 
   @Override
-  public void setData(@NotNull String key, @NotNull Object value) {
-    data.put(key, value);
+  public void setData(@Nullable String key, @Nullable Object value) {
+    if (key == null) {
+      return;
+    }
+    if (value == null) {
+      data.remove(key);
+    } else {
+      data.put(key, value);
+    }
   }
 
   @Override
-  public @Nullable Object getData(@NotNull String key) {
+  public @Nullable Object getData(@Nullable String key) {
+    if (key == null) {
+      return null;
+    }
     return data.get(key);
   }
 
@@ -422,7 +435,7 @@ public boolean isNoOp() {
   }
 
   @Override
-  public void setContext(@NotNull String key, @NotNull Object context) {
+  public void setContext(@Nullable String key, @Nullable Object context) {
     contexts.put(key, context);
   }
 

sentry-system-test-support/src/main/kotlin/io/sentry/systemtest/util/LoggingInsecureRestClient.kt

@@ -10,6 +10,7 @@ import okhttp3.RequestBody
 import okhttp3.RequestBody.Companion.toRequestBody
 import okhttp3.Response
 import org.slf4j.LoggerFactory
+import java.util.concurrent.TimeUnit
 
 open class LoggingInsecureRestClient {
     val logger = LoggerFactory.getLogger(LoggingInsecureRestClient::class.java)
@@ -55,6 +56,10 @@ open class LoggingInsecureRestClient {
 
     protected fun client(): OkHttpClient {
         return OkHttpClient.Builder()
+            .callTimeout(60, TimeUnit.SECONDS)
+            .connectTimeout(60, TimeUnit.SECONDS)
+            .readTimeout(60, TimeUnit.SECONDS)
+            .writeTimeout(60, TimeUnit.SECONDS)
             .build()
     }
 

sentry/api/sentry.api

@@ -238,8 +238,11 @@ public final class io/sentry/CombinedContextsView : io/sentry/protocol/Contexts
 	public fun isEmpty ()Z
 	public fun keys ()Ljava/util/Enumeration;
 	public fun put (Ljava/lang/String;Ljava/lang/Object;)Ljava/lang/Object;
+	public fun putAll (Lio/sentry/protocol/Contexts;)V
+	public fun putAll (Ljava/util/Map;)V
 	public fun remove (Ljava/lang/Object;)Ljava/lang/Object;
 	public fun serialize (Lio/sentry/ObjectWriter;Lio/sentry/ILogger;)V
+	public fun set (Ljava/lang/String;Ljava/lang/Object;)Ljava/lang/Object;
 	public fun setApp (Lio/sentry/protocol/App;)V
 	public fun setBrowser (Lio/sentry/protocol/Browser;)V
 	public fun setDevice (Lio/sentry/protocol/Device;)V

sentry/src/main/java/io/sentry/Breadcrumb.java

@@ -616,7 +616,10 @@ public Map<String, Object> getData() {
    * @return the value or null
    */
   @Nullable
-  public Object getData(final @NotNull String key) {
+  public Object getData(final @Nullable String key) {
+    if (key == null) {
+      return null;
+    }
     return data.get(key);
   }
 
@@ -626,16 +629,26 @@ public Object getData(final @NotNull String key) {
    * @param key the key
    * @param value the value
    */
-  public void setData(@NotNull String key, @NotNull Object value) {
-    data.put(key, value);
+  public void setData(@Nullable String key, @Nullable Object value) {
+    if (key == null) {
+      return;
+    }
+    if (value == null) {
+      removeData(key);
+    } else {
+      data.put(key, value);
+    }
   }
 
   /**
    * Removes an entry from the data's map
    *
    * @param key the key
    */
-  public void removeData(@NotNull String key) {
+  public void removeData(@Nullable String key) {
+    if (key == null) {
+      return;
+    }
     data.remove(key);
   }
 

sentry/src/main/java/io/sentry/CombinedContextsView.java

@@ -241,14 +241,14 @@ public boolean isEmpty() {
   }
 
   @Override
-  public boolean containsKey(final @NotNull Object key) {
+  public boolean containsKey(final @Nullable Object key) {
     return globalContexts.containsKey(key)
         || isolationContexts.containsKey(key)
         || currentContexts.containsKey(key);
   }
 
   @Override
-  public @Nullable Object get(final @NotNull Object key) {
+  public @Nullable Object get(final @Nullable Object key) {
     final @Nullable Object current = currentContexts.get(key);
     if (current != null) {
       return current;
@@ -261,12 +261,12 @@ public boolean containsKey(final @NotNull Object key) {
   }
 
   @Override
-  public @Nullable Object put(final @NotNull String key, final @Nullable Object value) {
+  public @Nullable Object put(final @Nullable String key, final @Nullable Object value) {
     return getDefaultContexts().put(key, value);
   }
 
   @Override
-  public @Nullable Object remove(final @NotNull Object key) {
+  public @Nullable Object remove(final @Nullable Object key) {
     return getDefaultContexts().remove(key);
   }
 
@@ -281,10 +281,26 @@ public boolean containsKey(final @NotNull Object key) {
   }
 
   @Override
-  public void serialize(@NotNull ObjectWriter writer, @NotNull ILogger logger) throws IOException {
+  public void serialize(final @NotNull ObjectWriter writer, final @NotNull ILogger logger)
+      throws IOException {
     mergeContexts().serialize(writer, logger);
   }
 
+  @Override
+  public @Nullable Object set(@Nullable String key, @Nullable Object value) {
+    return put(key, value);
+  }
+
+  @Override
+  public void putAll(@Nullable Map<? extends String, ?> m) {
+    getDefaultContexts().putAll(m);
+  }
+
+  @Override
+  public void putAll(@Nullable Contexts contexts) {
+    getDefaultContexts().putAll(contexts);
+  }
+
   private @NotNull Contexts mergeContexts() {
     final @NotNull Contexts allContexts = new Contexts();
     allContexts.putAll(globalContexts);

sentry/src/main/java/io/sentry/CombinedScopeView.java

@@ -229,12 +229,12 @@ public void clear() {
   }
 
   @Override
-  public void setTag(@NotNull String key, @NotNull String value) {
+  public void setTag(@Nullable String key, @Nullable String value) {
     getDefaultWriteScope().setTag(key, value);
   }
 
   @Override
-  public void removeTag(@NotNull String key) {
+  public void removeTag(@Nullable String key) {
     getDefaultWriteScope().removeTag(key);
   }
 
@@ -248,12 +248,12 @@ public void removeTag(@NotNull String key) {
   }
 
   @Override
-  public void setExtra(@NotNull String key, @NotNull String value) {
+  public void setExtra(@Nullable String key, @Nullable String value) {
     getDefaultWriteScope().setExtra(key, value);
   }
 
   @Override
-  public void removeExtra(@NotNull String key) {
+  public void removeExtra(@Nullable String key) {
     getDefaultWriteScope().removeExtra(key);
   }
 
@@ -267,42 +267,42 @@ public void removeExtra(@NotNull String key) {
   }
 
   @Override
-  public void setContexts(@NotNull String key, @NotNull Object value) {
+  public void setContexts(@Nullable String key, @Nullable Object value) {
     getDefaultWriteScope().setContexts(key, value);
   }
 
   @Override
-  public void setContexts(@NotNull String key, @NotNull Boolean value) {
+  public void setContexts(@Nullable String key, @Nullable Boolean value) {
     getDefaultWriteScope().setContexts(key, value);
   }
 
   @Override
-  public void setContexts(@NotNull String key, @NotNull String value) {
+  public void setContexts(@Nullable String key, @Nullable String value) {
     getDefaultWriteScope().setContexts(key, value);
   }
 
   @Override
-  public void setContexts(@NotNull String key, @NotNull Number value) {
+  public void setContexts(@Nullable String key, @Nullable Number value) {
     getDefaultWriteScope().setContexts(key, value);
   }
 
   @Override
-  public void setContexts(@NotNull String key, @NotNull Collection<?> value) {
+  public void setContexts(@Nullable String key, @Nullable Collection<?> value) {
     getDefaultWriteScope().setContexts(key, value);
   }
 
   @Override
-  public void setContexts(@NotNull String key, @NotNull Object[] value) {
+  public void setContexts(@Nullable String key, @Nullable Object[] value) {
     getDefaultWriteScope().setContexts(key, value);
   }
 
   @Override
-  public void setContexts(@NotNull String key, @NotNull Character value) {
+  public void setContexts(@Nullable String key, @Nullable Character value) {
     getDefaultWriteScope().setContexts(key, value);
   }
 
   @Override
-  public void removeContexts(@NotNull String key) {
+  public void removeContexts(@Nullable String key) {
     getDefaultWriteScope().removeContexts(key);
   }