Address comments

Author: mrober

firebase-sessions/src/main/kotlin/com/google/firebase/sessions/api/FirebaseSessionsDependencies.kt

@@ -16,6 +16,7 @@
 
 package com.google.firebase.sessions.api
 
+import android.util.Log
 import androidx.annotation.VisibleForTesting
 import java.util.Collections.synchronizedMap
 import kotlinx.coroutines.sync.Mutex
@@ -29,6 +30,8 @@ import kotlinx.coroutines.sync.withLock
  * This is important because the Sessions SDK starts up before dependent SDKs.
  */
 object FirebaseSessionsDependencies {
+  private const val TAG = "SessionsDependencies"
+
   private val dependencies = synchronizedMap(mutableMapOf<SessionSubscriber.Name, Dependency>())
 
   /**
@@ -37,7 +40,8 @@ object FirebaseSessionsDependencies {
    */
   fun addDependency(subscriberName: SessionSubscriber.Name) {
     if (dependencies.containsKey(subscriberName)) {
-      throw IllegalArgumentException("Dependency $subscriberName already added.")
+      Log.d(TAG, "Dependency $subscriberName already added.")
+      return
     }
 
     // The dependency is locked until the subscriber registers itself.
@@ -50,7 +54,8 @@ object FirebaseSessionsDependencies {
     val dependency = getDependency(subscriberName)
 
     dependency.subscriber?.run {
-      throw IllegalArgumentException("Subscriber $subscriberName already registered.")
+      Log.d(TAG, "Subscriber $subscriberName already registered.")
+      return
     }
     dependency.subscriber = subscriber
 
@@ -60,24 +65,21 @@ object FirebaseSessionsDependencies {
 
   /** Gets the subscribers safely, blocks until all the subscribers are registered. */
   internal suspend fun getSubscribers(): Map<SessionSubscriber.Name, SessionSubscriber> {
-    // The call to get will never throw because the mutex guarantees it's been registered.
+    // The call to getSubscriber will never throw because the mutex guarantees it's been registered.
     return dependencies.mapValues { (subscriberName, dependency) ->
-      dependency.mutex.withLock { get(subscriberName) }
+      dependency.mutex.withLock { getSubscriber(subscriberName) }
     }
   }
 
-  /** Gets the subscriber regardless of being registered. This is exposed for testing. */
+  /** Gets the subscriber, regardless of being registered. This is exposed for testing. */
   @VisibleForTesting
-  internal operator fun get(subscriberName: SessionSubscriber.Name): SessionSubscriber {
+  internal fun getSubscriber(subscriberName: SessionSubscriber.Name): SessionSubscriber {
     return getDependency(subscriberName).subscriber
       ?: throw IllegalStateException("Subscriber $subscriberName has not been registered.")
   }
 
-  /** Resets the registered subscribers for testing purposes. */
-  @VisibleForTesting
-  internal fun reset() {
-    dependencies.keys.forEach { dependencies[it] = Dependency(Mutex(locked = true)) }
-  }
+  /** Resets all the dependencies for testing purposes. */
+  @VisibleForTesting internal fun reset() = dependencies.clear()
 
   private fun getDependency(subscriberName: SessionSubscriber.Name): Dependency {
     return dependencies.getOrElse(subscriberName) {

firebase-sessions/src/main/kotlin/com/google/firebase/sessions/api/SessionSubscriber.kt

@@ -31,5 +31,5 @@ interface SessionSubscriber {
 
   val isDataCollectionEnabled: Boolean
 
-  val sessionSubscriberName: Name
+  val sessionSubscriberName: SessionSubscriber.Name
 }

firebase-sessions/src/test/kotlin/com/google/firebase/sessions/api/FirebaseSessionsDependenciesTest.kt

@@ -39,89 +39,88 @@ import org.junit.runner.RunWith
 class FirebaseSessionsDependenciesTest {
   @After
   fun cleanUp() {
-    // Reset any registered subscribers after each test.
+    // Reset all dependencies after each test.
     FirebaseSessionsDependencies.reset()
   }
 
   @Test
   fun register_dependencyAdded_canGet() {
+    val crashlyticsSubscriber = FakeSessionSubscriber(sessionSubscriberName = CRASHLYTICS)
+    FirebaseSessionsDependencies.addDependency(CRASHLYTICS)
     FirebaseSessionsDependencies.register(crashlyticsSubscriber)
 
-    assertThat(FirebaseSessionsDependencies[CRASHLYTICS]).isEqualTo(crashlyticsSubscriber)
+    assertThat(FirebaseSessionsDependencies.getSubscriber(CRASHLYTICS))
+      .isEqualTo(crashlyticsSubscriber)
   }
 
   @Test
-  fun register_alreadyRegistered_throws() {
+  fun register_alreadyRegisteredSameName_ignoresSecondSubscriber() {
+    val firstSubscriber = FakeSessionSubscriber(sessionSubscriberName = CRASHLYTICS)
+    val secondSubscriber = FakeSessionSubscriber(sessionSubscriberName = CRASHLYTICS)
+
+    FirebaseSessionsDependencies.addDependency(CRASHLYTICS)
+
     // Register the first time, no problem.
-    FirebaseSessionsDependencies.register(crashlyticsSubscriber)
+    FirebaseSessionsDependencies.register(firstSubscriber)
 
-    val thrown =
-      assertThrows(IllegalArgumentException::class.java) {
-        // Attempt to register the same subscriber a second time.
-        FirebaseSessionsDependencies.register(crashlyticsSubscriber)
-      }
+    // Attempt to register a second subscriber with the same name.
+    FirebaseSessionsDependencies.register(secondSubscriber)
 
-    assertThat(thrown).hasMessageThat().contains("Subscriber CRASHLYTICS already registered")
+    assertThat(FirebaseSessionsDependencies.getSubscriber(CRASHLYTICS)).isEqualTo(firstSubscriber)
   }
 
   @Test
   fun getSubscriber_dependencyAdded_notRegistered_throws() {
-    val thrown =
-      assertThrows(IllegalStateException::class.java) { FirebaseSessionsDependencies[CRASHLYTICS] }
+    FirebaseSessionsDependencies.addDependency(PERFORMANCE)
 
-    assertThat(thrown).hasMessageThat().contains("Subscriber CRASHLYTICS has not been registered")
-  }
-
-  @Test
-  fun getSubscriber_notDepended_throws() {
     val thrown =
       assertThrows(IllegalStateException::class.java) {
-        // Performance was never added as a dependency.
-        FirebaseSessionsDependencies[PERFORMANCE]
+        FirebaseSessionsDependencies.getSubscriber(PERFORMANCE)
       }
 
-    assertThat(thrown).hasMessageThat().contains("Cannot get dependency PERFORMANCE")
+    assertThat(thrown).hasMessageThat().contains("Subscriber PERFORMANCE has not been registered")
   }
 
   @Test
-  fun addDependencyTwice_throws() {
+  fun getSubscriber_notDepended_throws() {
     val thrown =
-      assertThrows(IllegalArgumentException::class.java) {
-        // CRASHLYTICS has already been added. Attempt to add it again.
-        FirebaseSessionsDependencies.addDependency(CRASHLYTICS)
+      assertThrows(IllegalStateException::class.java) {
+        // Crashlytics was never added as a dependency.
+        FirebaseSessionsDependencies.getSubscriber(CRASHLYTICS)
       }
 
-    assertThat(thrown).hasMessageThat().contains("Dependency CRASHLYTICS already added")
+    assertThat(thrown).hasMessageThat().contains("Cannot get dependency CRASHLYTICS")
   }
 
   @Test
   fun getSubscribers_waitsForRegister(): Unit = runBlocking {
+    val crashlyticsSubscriber = FakeSessionSubscriber(sessionSubscriberName = CRASHLYTICS)
+    FirebaseSessionsDependencies.addDependency(CRASHLYTICS)
+
     // Wait a few seconds and then register.
     launch {
       delay(2.seconds)
       FirebaseSessionsDependencies.register(crashlyticsSubscriber)
     }
 
     // Block until the register happens.
-    val subscribers = runBlocking { FirebaseSessionsDependencies.getSubscribers() }
+    val subscribers = FirebaseSessionsDependencies.getSubscribers()
 
     assertThat(subscribers).containsExactly(CRASHLYTICS, crashlyticsSubscriber)
   }
 
+  @Test
+  fun getSubscribers_noDependencies() = runTest {
+    val subscribers = FirebaseSessionsDependencies.getSubscribers()
+
+    assertThat(subscribers).isEmpty()
+  }
+
   @Test(expected = TimeoutCancellationException::class)
   fun getSubscribers_neverRegister_waitsForever() = runTest {
+    FirebaseSessionsDependencies.addDependency(CRASHLYTICS)
+
     // The register never happens, wait until the timeout.
     withTimeout(2.seconds) { FirebaseSessionsDependencies.getSubscribers() }
   }
-
-  companion object {
-    init {
-      // Add only Crashlytics as a dependency, not Performance.
-      // This is similar to how 1P SDKs will add themselves as dependencies. Note that this
-      // dependency is added for all unit tests after this class is loaded into memory.
-      FirebaseSessionsDependencies.addDependency(CRASHLYTICS)
-    }
-  }
-
-  private val crashlyticsSubscriber = FakeSessionSubscriber(sessionSubscriberName = CRASHLYTICS)
 }