Fix inefficiency in the setCustomKeys Kotlin extension (#6536)

Fixed inefficiency in the Kotlin `FirebaseCrashlytics.setCustomKeys`
extension.

The old implementation called `setCustomKey` for every key/value pair,
which serializes to disk, and is not an intuitive behaviour for
customers. This now calls `setCustomKeys` once with the entire built
map.

This proxies to the existing Java `CustomKeysAndValues.Builder` but
gives nice Kotlin syntax. The old class had a public ctor, I plan to
make this internal since customers shouldn't instantiate this class
directly. This is done in a way to not break the api, even though the
api is internal.

pair=tejasd

Author: mrober

firebase-crashlytics/CHANGELOG.md

@@ -1,4 +1,5 @@
 # Unreleased
+* [fixed] Fixed inefficiency in the Kotlin `FirebaseCrashlytics.setCustomKeys` extension.
 * [fixed] Execute failure listener outside the main thread  [#6535]
 
 # 19.2.1
@@ -634,3 +635,4 @@ The following release notes describe changes in the new SDK.
   from your `AndroidManifest.xml` file.
 * [removed] The `fabric.properties` and `crashlytics.properties` files are no
   longer supported. Remove them from your app.
+

firebase-crashlytics/src/androidTest/java/com/google/firebase/crashlytics/CrashlyticsTests.kt

@@ -59,6 +59,31 @@ class CrashlyticsTests {
     Firebase.app.get(UserAgentPublisher::class.java)
   }
 
+  @Test
+  fun keyValueBuilder() {
+    val keyValueBuilder = KeyValueBuilder()
+    keyValueBuilder.key("hello", "world")
+    keyValueBuilder.key("hello2", 23)
+    keyValueBuilder.key("hello3", 0.1)
+
+    val result: Map<String, String> = keyValueBuilder.build().keysAndValues
+
+    assertThat(result).containsExactly("hello", "world", "hello2", "23", "hello3", "0.1")
+  }
+
+  @Test
+  fun keyValueBuilder_withCrashlyticsInstance() {
+    @Suppress("DEPRECATION") val keyValueBuilder = KeyValueBuilder(Firebase.crashlytics)
+    keyValueBuilder.key("hello", "world")
+    keyValueBuilder.key("hello2", 23)
+    keyValueBuilder.key("hello3", 0.1)
+
+    val result: Map<String, String> = keyValueBuilder.build().keysAndValues
+
+    // The result is empty because it called crashlytics.setCustomKey for every key.
+    assertThat(result).isEmpty()
+  }
+
   companion object {
     private const val APP_ID = "1:1:android:1a"
     private const val API_KEY = "API-KEY-API-KEY-API-KEY-API-KEY-API-KEY"

firebase-crashlytics/src/androidTest/java/com/google/firebase/crashlytics/internal/metadata/MetaDataStoreTest.java

@@ -118,13 +118,13 @@ public void testWriteUserData_noFields() throws Exception {
   }
 
   @Test
-  // TODO(b/375437048): Let @daymxn know if this test fails. It's flaky and running away from me:(
   public void testWriteUserData_singleField() throws Exception {
     crashlyticsWorkers.diskWrite.submit(
-        () -> {
-          storeUnderTest.writeUserData(SESSION_ID_1, metadataWithUserId(SESSION_ID_1).getUserId());
-        });
+        () ->
+            storeUnderTest.writeUserData(
+                SESSION_ID_1, metadataWithUserId(SESSION_ID_1).getUserId()));
     crashlyticsWorkers.diskWrite.await();
+    Thread.sleep(10);
     UserMetadata userData =
         UserMetadata.loadFromExistingSession(SESSION_ID_1, fileStore, crashlyticsWorkers);
     assertEquals(USER_ID, userData.getUserId());
@@ -168,17 +168,15 @@ public void testWriteUserData_unicode() throws Exception {
   }
 
   @Test
-  // TODO(b/375437048): Let @daymxn know if this test fails. It's flaky and running away from me:(
   public void testWriteUserData_escaped() throws Exception {
     crashlyticsWorkers.diskWrite.submit(
-        () -> {
-          storeUnderTest.writeUserData(
-              SESSION_ID_1, metadataWithUserId(SESSION_ID_1, ESCAPED).getUserId());
-        });
+        () ->
+            storeUnderTest.writeUserData(
+                SESSION_ID_1, metadataWithUserId(SESSION_ID_1, ESCAPED).getUserId()));
     crashlyticsWorkers.diskWrite.await();
+    Thread.sleep(10);
     UserMetadata userData =
         UserMetadata.loadFromExistingSession(SESSION_ID_1, fileStore, crashlyticsWorkers);
-    Thread.sleep(10);
     assertEquals(ESCAPED.trim(), userData.getUserId());
   }
 

firebase-crashlytics/src/main/java/com/google/firebase/crashlytics/FirebaseCrashlytics.kt

@@ -27,10 +27,8 @@ val Firebase.crashlytics: FirebaseCrashlytics
   get() = FirebaseCrashlytics.getInstance()
 
 /** Associates all key-value parameters with the reports */
-fun FirebaseCrashlytics.setCustomKeys(init: KeyValueBuilder.() -> Unit) {
-  val builder = KeyValueBuilder(this)
-  builder.init()
-}
+fun FirebaseCrashlytics.setCustomKeys(init: KeyValueBuilder.() -> Unit) =
+  setCustomKeys(KeyValueBuilder().apply(init).build())
 
 /** @suppress */
 @Keep

firebase-crashlytics/src/main/java/com/google/firebase/crashlytics/KeyValueBuilder.kt

@@ -16,24 +16,48 @@
 
 package com.google.firebase.crashlytics
 
-/** Helper class to enable fluent syntax in [setCustomKeys] */
-class KeyValueBuilder(private val crashlytics: FirebaseCrashlytics) {
+/** Helper class to enable convenient syntax in [setCustomKeys] */
+class KeyValueBuilder
+private constructor(
+  private val crashlytics: FirebaseCrashlytics?,
+  private val builder: CustomKeysAndValues.Builder,
+) {
+  @Deprecated(
+    "Do not construct this directly. Use `setCustomKeys` instead. To be removed in the next major release."
+  )
+  constructor(crashlytics: FirebaseCrashlytics) : this(crashlytics, CustomKeysAndValues.Builder())
+
+  internal constructor() : this(crashlytics = null, CustomKeysAndValues.Builder())
+
+  internal fun build(): CustomKeysAndValues = builder.build()
 
   /** Sets a custom key and value that are associated with reports. */
-  fun key(key: String, value: Boolean) = crashlytics.setCustomKey(key, value)
+  fun key(key: String, value: Boolean) {
+    crashlytics?.setCustomKey(key, value) ?: builder.putBoolean(key, value)
+  }
 
   /** Sets a custom key and value that are associated with reports. */
-  fun key(key: String, value: Double) = crashlytics.setCustomKey(key, value)
+  fun key(key: String, value: Double) {
+    crashlytics?.setCustomKey(key, value) ?: builder.putDouble(key, value)
+  }
 
   /** Sets a custom key and value that are associated with reports. */
-  fun key(key: String, value: Float) = crashlytics.setCustomKey(key, value)
+  fun key(key: String, value: Float) {
+    crashlytics?.setCustomKey(key, value) ?: builder.putFloat(key, value)
+  }
 
   /** Sets a custom key and value that are associated with reports. */
-  fun key(key: String, value: Int) = crashlytics.setCustomKey(key, value)
+  fun key(key: String, value: Int) {
+    crashlytics?.setCustomKey(key, value) ?: builder.putInt(key, value)
+  }
 
   /** Sets a custom key and value that are associated with reports. */
-  fun key(key: String, value: Long) = crashlytics.setCustomKey(key, value)
+  fun key(key: String, value: Long) {
+    crashlytics?.setCustomKey(key, value) ?: builder.putLong(key, value)
+  }
 
   /** Sets a custom key and value that are associated with reports. */
-  fun key(key: String, value: String) = crashlytics.setCustomKey(key, value)
+  fun key(key: String, value: String) {
+    crashlytics?.setCustomKey(key, value) ?: builder.putString(key, value)
+  }
 }

firebase-crashlytics/src/main/java/com/google/firebase/crashlytics/internal/common/CrashlyticsCore.java

@@ -368,7 +368,9 @@ public void setCustomKey(String key, String value) {
    * @throws NullPointerException if any key in keysAndValues is null.
    */
   public void setCustomKeys(Map<String, String> keysAndValues) {
-    crashlyticsWorkers.common.submit(() -> controller.setCustomKeys(keysAndValues));
+    if (!keysAndValues.isEmpty()) {
+      crashlyticsWorkers.common.submit(() -> controller.setCustomKeys(keysAndValues));
+    }
   }
 
   // endregion