Merge branch 'main' into td/record-exception

Author: tejasd

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

firebase-functions/CHANGELOG.md

@@ -1,5 +1,6 @@
 # Unreleased
-* [fixed] Fixed HttpsCallableResult.data resolution in Kotlin
+* [fixed] Minor internal infrastructure improvements. (#6544)
+* [fixed] Fixed HttpsCallableResult.data resolution in Kotlin. (#6530)
 
 # 21.1.0
 * [changed] Migrated to Kotlin
@@ -209,4 +210,3 @@ updates.
 optional region to override the default "us-central1".
 * [feature] New `useFunctionsEmulator` method allows testing against a local
 instance of the [Cloud Functions Emulator](https://firebase.google.com/docs/functions/local-emulator).
-

firebase-functions/src/main/java/com/google/firebase/functions/FirebaseContextProvider.kt

@@ -47,13 +47,16 @@ constructor(
     appCheckDeferred.whenAvailable { p: Provider<InteropAppCheckTokenProvider> ->
       val appCheck = p.get()
       appCheckRef.set(appCheck)
-      appCheck.addAppCheckTokenListener { unused: AppCheckTokenResult? -> }
+      appCheck.addAppCheckTokenListener {
+        // Do nothing; we just need to register a listener so that the App Check SDK knows
+        // to auto-refresh the token.
+      }
     }
   }
 
-  override fun getContext(limitedUseAppCheckToken: Boolean): Task<HttpsCallableContext?>? {
+  override fun getContext(getLimitedUseAppCheckToken: Boolean): Task<HttpsCallableContext?>? {
     val authToken = getAuthToken()
-    val appCheckToken = getAppCheckToken(limitedUseAppCheckToken)
+    val appCheckToken = getAppCheckToken(getLimitedUseAppCheckToken)
     return Tasks.whenAll(authToken, appCheckToken).onSuccessTask(executor) { _ ->
       Tasks.forResult(
         HttpsCallableContext(authToken.result, instanceId.get().token, appCheckToken.result)

firebase-functions/src/main/java/com/google/firebase/functions/FirebaseFunctions.kt

@@ -58,16 +58,16 @@ internal constructor(
   @UiThread uiExecutor: Executor
 ) {
   // The network client to use for HTTPS requests.
-  private val client: OkHttpClient
+  private val client: OkHttpClient = OkHttpClient()
 
   // A serializer to encode/decode parameters and return values.
-  private val serializer: Serializer
+  private val serializer: Serializer = Serializer()
 
   // A provider of client metadata to include with calls.
-  private val contextProvider: ContextProvider
+  private val contextProvider: ContextProvider = Preconditions.checkNotNull(contextProvider)
 
   // The projectId to use for all functions references.
-  private val projectId: String
+  private val projectId: String = Preconditions.checkNotNull(projectId)
 
   // The region to use for all function references.
   private var region: String? = null
@@ -82,12 +82,7 @@ internal constructor(
   private var emulatorSettings: EmulatedServiceSettings? = null
 
   init {
-    client = OkHttpClient()
-    serializer = Serializer()
-    this.contextProvider = Preconditions.checkNotNull(contextProvider)
-    this.projectId = Preconditions.checkNotNull(projectId)
-    val isRegion: Boolean
-    isRegion =
+    val isRegion: Boolean =
       try {
         URL(regionOrCustomDomain)
         false
@@ -182,9 +177,7 @@ internal constructor(
     options: HttpsCallOptions
   ): Task<HttpsCallableResult> {
     return providerInstalled.task
-      .continueWithTask(executor) { task: Task<Void>? ->
-        contextProvider.getContext(options.limitedUseAppCheckTokens)
-      }
+      .continueWithTask(executor) { contextProvider.getContext(options.limitedUseAppCheckTokens) }
       .continueWithTask(executor) { task: Task<HttpsCallableContext?> ->
         if (!task.isSuccessful) {
           return@continueWithTask Tasks.forException<HttpsCallableResult>(task.exception!!)
@@ -204,9 +197,7 @@ internal constructor(
    */
   internal fun call(url: URL, data: Any?, options: HttpsCallOptions): Task<HttpsCallableResult> {
     return providerInstalled.task
-      .continueWithTask(executor) { task: Task<Void>? ->
-        contextProvider.getContext(options.limitedUseAppCheckTokens)
-      }
+      .continueWithTask(executor) { contextProvider.getContext(options.limitedUseAppCheckTokens) }
       .continueWithTask(executor) { task: Task<HttpsCallableContext?> ->
         if (!task.isSuccessful) {
           return@continueWithTask Tasks.forException<HttpsCallableResult>(task.exception!!)
@@ -277,16 +268,15 @@ internal constructor(
         @Throws(IOException::class)
         override fun onResponse(ignored: Call, response: Response) {
           val code = fromHttpStatus(response.code())
-          val body = response.body()!!.string()
-          val exception = fromResponse(code, body, serializer)
+          val bodyAsString = response.body()!!.string()
+          val exception = fromResponse(code, bodyAsString, serializer)
           if (exception != null) {
             tcs.setException(exception)
             return
           }
-          val bodyJSON: JSONObject
-          bodyJSON =
+          val bodyAsJson: JSONObject =
             try {
-              JSONObject(body)
+              JSONObject(bodyAsString)
             } catch (je: JSONException) {
               val e: Exception =
                 FirebaseFunctionsException(
@@ -298,10 +288,10 @@ internal constructor(
               tcs.setException(e)
               return
             }
-          var dataJSON = bodyJSON.opt("data")
+          var dataJSON = bodyAsJson.opt("data")
           // TODO: Allow "result" instead of "data" for now, for backwards compatibility.
           if (dataJSON == null) {
-            dataJSON = bodyJSON.opt("result")
+            dataJSON = bodyAsJson.opt("result")
           }
           if (dataJSON == null) {
             val e: Exception =

firebase-functions/src/main/java/com/google/firebase/functions/FirebaseFunctionsException.kt

@@ -27,7 +27,7 @@ public class FirebaseFunctionsException : FirebaseException {
    * canonical error codes for Google APIs, as documented here:
    * https://github.com/googleapis/googleapis/blob/master/google/rpc/code.proto#L26
    */
-  public enum class Code(private val value: Int) {
+  public enum class Code(@Suppress("unused") private val value: Int) {
     /**
      * The operation completed successfully. FirebaseFunctionsException will never have a status of
      * OK.
@@ -199,21 +199,21 @@ public class FirebaseFunctionsException : FirebaseException {
       serializer: Serializer
     ): FirebaseFunctionsException? {
       // Start with reasonable defaults from the status code.
-      var code = code
-      var description = code.name
+      var actualCode = code
+      var description = actualCode.name
       var details: Any? = null
 
       // Then look through the body for explicit details.
       try {
-        val json = JSONObject(body)
+        val json = JSONObject(body ?: "")
         val error = json.getJSONObject("error")
         if (error.opt("status") is String) {
-          code = Code.valueOf(error.getString("status"))
+          actualCode = Code.valueOf(error.getString("status"))
           // TODO: Add better default descriptions for error enums.
           // The default description needs to be updated for the new code.
-          description = code.name
+          description = actualCode.name
         }
-        if (error.opt("message") is String && !error.getString("message").isEmpty()) {
+        if (error.opt("message") is String && error.getString("message").isNotEmpty()) {
           description = error.getString("message")
         }
         details = error.opt("details")
@@ -222,16 +222,16 @@ public class FirebaseFunctionsException : FirebaseException {
         }
       } catch (iae: IllegalArgumentException) {
         // This most likely means the status string was invalid, so consider this malformed.
-        code = Code.INTERNAL
-        description = code.name
+        actualCode = Code.INTERNAL
+        description = actualCode.name
       } catch (ioe: JSONException) {
         // If we couldn't parse explicit error data, that's fine.
       }
-      return if (code == Code.OK) {
+      return if (actualCode == Code.OK) {
         // Technically, there's an edge case where a developer could explicitly return an error code
         // of OK, and we will treat it as success, but that seems reasonable.
         null
-      } else FirebaseFunctionsException(description, code, details)
+      } else FirebaseFunctionsException(description, actualCode, details)
     }
   }
 }