Fix ignoredErrors, ignoredTransactions and ignoredCheckIns being unset by external options (#4207)

* When parsing ExternalOptions that are missing keep the value in SentryOptions for filter lists

* changelog

Author: adinauer

CHANGELOG.md

@@ -14,6 +14,9 @@
 ### Fixes
 
 - `SentryOptions.setTracePropagationTargets` is no longer marked internal ([#4170](https://github.com/getsentry/sentry-java/pull/4170))
+- Fix `ignoredErrors`, `ignoredTransactions` and `ignoredCheckIns` being unset by external options like `sentry.properties` or ENV vars ([#4207](https://github.com/getsentry/sentry-java/pull/4207))
+  - Whenever parsing of external options was enabled (`enableExternalConfiguration`), which is the default for many integrations, the values set on `SentryOptions` passed to `Sentry.init` would be lost
+  - Even if the value was not set in any external configuration it would still be set to an empty list
 
 ### Behavioural Changes
 

sentry/api/sentry.api

@@ -4029,6 +4029,7 @@ public abstract interface class io/sentry/config/PropertiesProvider {
 	public fun getBooleanProperty (Ljava/lang/String;)Ljava/lang/Boolean;
 	public fun getDoubleProperty (Ljava/lang/String;)Ljava/lang/Double;
 	public fun getList (Ljava/lang/String;)Ljava/util/List;
+	public fun getListOrNull (Ljava/lang/String;)Ljava/util/List;
 	public fun getLongProperty (Ljava/lang/String;)Ljava/lang/Long;
 	public abstract fun getMap (Ljava/lang/String;)Ljava/util/Map;
 	public abstract fun getProperty (Ljava/lang/String;)Ljava/lang/String;

sentry/src/main/java/io/sentry/ExternalOptions.java

@@ -128,7 +128,7 @@ public final class ExternalOptions {
     }
     options.setIdleTimeout(propertiesProvider.getLongProperty("idle-timeout"));
 
-    options.setIgnoredErrors(propertiesProvider.getList("ignored-errors"));
+    options.setIgnoredErrors(propertiesProvider.getListOrNull("ignored-errors"));
 
     options.setEnabled(propertiesProvider.getBooleanProperty("enabled"));
 
@@ -138,8 +138,8 @@ public final class ExternalOptions {
     options.setSendModules(propertiesProvider.getBooleanProperty("send-modules"));
     options.setSendDefaultPii(propertiesProvider.getBooleanProperty("send-default-pii"));
 
-    options.setIgnoredCheckIns(propertiesProvider.getList("ignored-checkins"));
-    options.setIgnoredTransactions(propertiesProvider.getList("ignored-transactions"));
+    options.setIgnoredCheckIns(propertiesProvider.getListOrNull("ignored-checkins"));
+    options.setIgnoredTransactions(propertiesProvider.getListOrNull("ignored-transactions"));
 
     options.setEnableBackpressureHandling(
         propertiesProvider.getBooleanProperty("enable-backpressure-handling"));

sentry/src/main/java/io/sentry/config/PropertiesProvider.java

@@ -38,6 +38,18 @@ default List<String> getList(final @NotNull String property) {
     return value != null ? Arrays.asList(value.split(",")) : Collections.emptyList();
   }
 
+  /**
+   * Resolves a list of values for a property given by it's name.
+   *
+   * @param property - the property name
+   * @return the list or null if not found
+   */
+  @Nullable
+  default List<String> getListOrNull(final @NotNull String property) {
+    final String value = getProperty(property);
+    return value != null ? Arrays.asList(value.split(",")) : null;
+  }
+
   /**
    * Resolves property given by it's name.
    *

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

@@ -218,6 +218,14 @@ class ExternalOptionsTest {
         }
     }
 
+    @Test
+    fun `creates options with null ignored errors if missing`() {
+        val logger = mock<ILogger>()
+        withPropertiesFile("Another .*", logger) { options ->
+            assertNull(options.ignoredErrors)
+        }
+    }
+
     @Test
     fun `creates options with single bundle ID using external properties`() {
         withPropertiesFile("bundle-ids=12ea7a02-46ac-44c0-a5bb-6d1fd9586411") { options ->
@@ -270,13 +278,29 @@ class ExternalOptionsTest {
         }
     }
 
+    @Test
+    fun `creates options with null ignoredCheckIns if missing`() {
+        val logger = mock<ILogger>()
+        withPropertiesFile("Another .*", logger) { options ->
+            assertNull(options.ignoredCheckIns)
+        }
+    }
+
     @Test
     fun `creates options with ignoredTransactions`() {
         withPropertiesFile("ignored-transactions=transactionName1,transactionName2") { options ->
             assertTrue(options.ignoredTransactions!!.containsAll(listOf("transactionName1", "transactionName2")))
         }
     }
 
+    @Test
+    fun `creates options with null ignoredTransactions if missing`() {
+        val logger = mock<ILogger>()
+        withPropertiesFile("Another .*", logger) { options ->
+            assertNull(options.ignoredTransactions)
+        }
+    }
+
     @Test
     fun `creates options with enableBackpressureHandling set to false`() {
         withPropertiesFile("enable-backpressure-handling=false") { options ->

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

@@ -668,4 +668,31 @@ class SentryOptionsTest {
     fun `when options is initialized, InitPriority is set to MEDIUM by default`() {
         assertEquals(SentryOptions().initPriority, InitPriority.MEDIUM)
     }
+
+    @Test
+    fun `merging options when ignoredErrors is not set preserves the previous value`() {
+        val externalOptions = ExternalOptions()
+        val options = SentryOptions()
+        options.setIgnoredErrors(listOf("error1", "error2"))
+        options.merge(externalOptions)
+        assertEquals(listOf(FilterString("error1"), FilterString("error2")), options.ignoredErrors)
+    }
+
+    @Test
+    fun `merging options when ignoredTransactions is not set preserves the previous value`() {
+        val externalOptions = ExternalOptions()
+        val options = SentryOptions()
+        options.setIgnoredTransactions(listOf("transaction1", "transaction2"))
+        options.merge(externalOptions)
+        assertEquals(listOf(FilterString("transaction1"), FilterString("transaction2")), options.ignoredTransactions)
+    }
+
+    @Test
+    fun `merging options when ignoredCheckIns is not set preserves the previous value`() {
+        val externalOptions = ExternalOptions()
+        val options = SentryOptions()
+        options.setIgnoredCheckIns(listOf("checkin1", "checkin2"))
+        options.merge(externalOptions)
+        assertEquals(listOf(FilterString("checkin1"), FilterString("checkin2")), options.ignoredCheckIns)
+    }
 }