Merge pull request #6122 from getsentry/fix/data-collection-explicit-opt-in

fix(core): [Data Collection 27] Make opt-in explicit

Author: adinauer

sentry-android-core/src/main/java/io/sentry/android/core/ManifestMetadataReader.java

@@ -834,7 +834,7 @@ static void applyMetadata(
       final @NotNull Object metadata,
       final @NotNull ILogger logger,
       final @NotNull DataCollection currentDataCollection) {
-    final @NotNull DataCollection dataCollection = new DataCollection(false);
+    final @NotNull DataCollection dataCollection = new DataCollection();
 
     if (containsKey(metadata, DATA_COLLECTION_USER_INFO)) {
       dataCollection.setUserInfo(readBool(metadata, logger, DATA_COLLECTION_USER_INFO, false));

sentry/api/sentry.api

@@ -387,7 +387,7 @@ public final class io/sentry/DataCategory : java/lang/Enum {
 
 public final class io/sentry/DataCollection {
 	public fun <init> ()V
-	public fun <init> (Z)V
+	public fun forceDataCollection ()V
 	public fun getCookies ()Lio/sentry/KeyValueCollectionBehavior;
 	public fun getDatabaseQueryData ()Ljava/lang/Boolean;
 	public fun getFilePaths ()Ljava/lang/Boolean;

sentry/src/main/java/io/sentry/DataCollection.java

@@ -21,13 +21,11 @@ public final class DataCollection {
   private final @NotNull HttpHeaders httpHeaders = new HttpHeaders();
   private final @NotNull Graphql graphql = new Graphql();
 
-  public DataCollection() {
-    this(true);
-  }
+  public DataCollection() {}
 
-  @ApiStatus.Internal
-  public DataCollection(final boolean forceDataCollection) {
-    this.forceDataCollection = forceDataCollection;
+  /** Opts into the documented Data Collection defaults when no individual option is configured. */
+  public void forceDataCollection() {
+    forceDataCollection = true;
   }
 
   public @Nullable Boolean getUserInfo() {

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

@@ -250,7 +250,7 @@ public final class ExternalOptions {
 
   private static @Nullable DataCollection parseDataCollection(
       final @NotNull PropertiesProvider propertiesProvider) {
-    final DataCollection dataCollection = new DataCollection(false);
+    final DataCollection dataCollection = new DataCollection();
 
     final Boolean userInfo = propertiesProvider.getBooleanProperty("data-collection.user-info");
     if (userInfo != null) {

sentry/src/main/java/io/sentry/SentryOptions.java

@@ -352,7 +352,7 @@ public class SentryOptions implements RateLimiterConfig {
   /** whether to send personal identifiable information along with events */
   private boolean sendDefaultPii = false;
 
-  private @NotNull DataCollection dataCollection = new DataCollection(false);
+  private @NotNull DataCollection dataCollection = new DataCollection();
 
   private final @NotNull DataCollectionResolver dataCollectionResolver =
       new DataCollectionResolver(this);
@@ -1776,7 +1776,9 @@ public void setSendDefaultPii(boolean sendDefaultPii) {
   /**
    * Replaces the configuration for data that the SDK collects automatically.
    *
-   * <p>Passing an empty {@link DataCollection} opts into the documented data-collection defaults.
+   * <p>This discards any Data Collection options already configured on this instance. To opt into
+   * the documented defaults while preserving them, call {@link
+   * DataCollection#forceDataCollection()} on the object returned by {@link #getDataCollection()}.
    */
   public void setDataCollection(final @NotNull DataCollection dataCollection) {
     if (dataCollection != null) {

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

@@ -371,7 +371,7 @@ class DataCollectionResolverTest {
 
   @Test
   fun `explicit empty data collection enables every HTTP body direction`() {
-    val options = SentryOptions().apply { dataCollection = DataCollection() }
+    val options = SentryOptions().apply { dataCollection.forceDataCollection() }
 
     assertThat(options.dataCollectionResolver.isIncomingRequestBody).isTrue()
     assertThat(options.dataCollectionResolver.isOutgoingRequestBody).isTrue()

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

@@ -6,7 +6,7 @@ import kotlin.test.assertFailsWith
 
 class DataCollectionTest {
   @Test
-  fun `public constructor forces Data Collection for empty configuration`() {
+  fun `public constructor does not force Data Collection for empty configuration`() {
     val dataCollection = DataCollection()
 
     assertThat(dataCollection.userInfo).isNull()
@@ -19,19 +19,21 @@ class DataCollectionTest {
     assertThat(dataCollection.httpHeaders.response).isNull()
     assertThat(dataCollection.graphql.document).isNull()
     assertThat(dataCollection.graphql.variables).isNull()
-    assertThat(dataCollection.isExplicitlyConfigured()).isTrue()
+    assertThat(dataCollection.isExplicitlyConfigured()).isFalse()
   }
 
   @Test
-  fun `SDK-owned configuration does not force Data Collection`() {
-    val dataCollection = DataCollection(false)
+  fun `force Data Collection makes empty configuration explicit`() {
+    val dataCollection = DataCollection()
 
-    assertThat(dataCollection.isExplicitlyConfigured()).isFalse()
+    dataCollection.forceDataCollection()
+
+    assertThat(dataCollection.isExplicitlyConfigured()).isTrue()
   }
 
   @Test
-  fun `nested override makes SDK-owned configuration explicit`() {
-    val dataCollection = DataCollection(false)
+  fun `nested override makes configuration explicit`() {
+    val dataCollection = DataCollection()
 
     dataCollection.graphql.setVariables(false)
 
@@ -40,7 +42,7 @@ class DataCollectionTest {
 
   @Test
   fun `explicit false is distinct from unset`() {
-    val dataCollection = DataCollection(false)
+    val dataCollection = DataCollection()
 
     dataCollection.setUserInfo(false)
 
@@ -50,7 +52,7 @@ class DataCollectionTest {
 
   @Test
   fun `nullable Boolean options are mutable Kotlin properties`() {
-    val dataCollection = DataCollection(false)
+    val dataCollection = DataCollection()
 
     dataCollection.userInfo = false
     dataCollection.databaseQueryData = false
@@ -81,7 +83,7 @@ class DataCollectionTest {
 
   @Test
   fun `empty HTTP body set is distinct from unset`() {
-    val dataCollection = DataCollection(false)
+    val dataCollection = DataCollection()
 
     dataCollection.setHttpBodies(emptySet())
 
@@ -105,7 +107,7 @@ class DataCollectionTest {
 
   @Test
   fun `database query data false is distinct from unset`() {
-    val dataCollection = DataCollection(false)
+    val dataCollection = DataCollection()
 
     dataCollection.setDatabaseQueryData(false)
 
@@ -115,7 +117,7 @@ class DataCollectionTest {
 
   @Test
   fun `file paths false is distinct from unset`() {
-    val dataCollection = DataCollection(false)
+    val dataCollection = DataCollection()
 
     dataCollection.setFilePaths(false)
 
@@ -125,7 +127,7 @@ class DataCollectionTest {
 
   @Test
   fun `nested HTTP header override marks configuration explicit`() {
-    val dataCollection = DataCollection(false)
+    val dataCollection = DataCollection()
     val behavior = KeyValueCollectionBehavior.denyList("authorization")
 
     dataCollection.httpHeaders.setRequest(behavior)
@@ -135,7 +137,7 @@ class DataCollectionTest {
 
   @Test
   fun `nested GraphQL false marks configuration explicit`() {
-    val dataCollection = DataCollection(false)
+    val dataCollection = DataCollection()
 
     dataCollection.graphql.setVariables(false)
 

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

@@ -48,14 +48,33 @@ class SentryOptionsTest {
   }
 
   @Test
-  fun `setting an empty data collection marks it explicitly configured`() {
+  fun `setting an empty data collection preserves legacy mode`() {
     val options = SentryOptions()
 
     options.dataCollection = DataCollection()
 
+    assertThat(options.dataCollection.isExplicitlyConfigured()).isFalse()
+  }
+
+  @Test
+  fun `forcing empty data collection marks it explicitly configured`() {
+    val options = SentryOptions()
+
+    options.dataCollection.forceDataCollection()
+
     assertThat(options.dataCollection.isExplicitlyConfigured()).isTrue()
   }
 
+  @Test
+  fun `forcing data collection preserves existing options`() {
+    val options = SentryOptions()
+    options.dataCollection.setUserInfo(false)
+
+    options.dataCollection.forceDataCollection()
+
+    assertThat(options.dataCollection.userInfo).isFalse()
+  }
+
   @Test
   fun `setting data collection replaces the default instance`() {
     val options = SentryOptions()