Address comments

Author: mrober

firebase-sessions/api.txt

@@ -16,52 +16,3 @@ package com.google.firebase.sessions {
   }
 
 }
-
-package com.google.firebase.sessions.api {
-
-  public final class FirebaseSessionsDependencies {
-    method public void addDependency(@NonNull com.google.firebase.sessions.api.SessionSubscriber.Name subscriberName);
-    field @NonNull public static final com.google.firebase.sessions.api.FirebaseSessionsDependencies INSTANCE;
-  }
-
-  public interface SessionSubscriber {
-    method @NonNull public com.google.firebase.sessions.api.SessionSubscriber.Name getSessionSubscriberName();
-    method public boolean isDataCollectionEnabled();
-    method public void onSessionChanged(@NonNull com.google.firebase.sessions.api.SessionSubscriber.SessionDetails sessionDetails);
-    property public abstract boolean isDataCollectionEnabled;
-    property @NonNull public abstract com.google.firebase.sessions.api.SessionSubscriber.Name sessionSubscriberName;
-  }
-
-  public enum SessionSubscriber.Name {
-    method @NonNull public static com.google.firebase.sessions.api.SessionSubscriber.Name valueOf(@NonNull String name) throws java.lang.IllegalArgumentException;
-    method @NonNull public static com.google.firebase.sessions.api.SessionSubscriber.Name[] values();
-    enum_constant public static final com.google.firebase.sessions.api.SessionSubscriber.Name CRASHLYTICS;
-    enum_constant @Discouraged(message="This is for testing purposes only.") public static final com.google.firebase.sessions.api.SessionSubscriber.Name MATT_SAYS_HI;
-    enum_constant public static final com.google.firebase.sessions.api.SessionSubscriber.Name PERFORMANCE;
-  }
-
-  public static final class SessionSubscriber.SessionDetails {
-    ctor public SessionSubscriber.SessionDetails(@NonNull String sessionId);
-    method @NonNull public String component1();
-    method @NonNull public com.google.firebase.sessions.api.SessionSubscriber.SessionDetails copy(@NonNull String sessionId);
-    method @NonNull public String getSessionId();
-    property @NonNull public final String sessionId;
-  }
-
-}
-
-package com.google.firebase.sessions.settings {
-
-  public interface SettingsProvider {
-    method @Nullable public Double getSamplingRate();
-    method @Nullable public Boolean getSessionEnabled();
-    method @Nullable public kotlin.time.Duration getSessionRestartTimeout();
-    method public boolean isSettingsStale();
-    method public void updateSettings();
-    property @Nullable public abstract Double samplingRate;
-    property @Nullable public abstract Boolean sessionEnabled;
-    property @Nullable public abstract kotlin.time.Duration sessionRestartTimeout;
-  }
-
-}
-

firebase-sessions/src/main/kotlin/com/google/firebase/sessions/FirebaseSessions.kt

@@ -83,16 +83,11 @@ internal constructor(
   private fun initiateSessionStart() {
     val sessionDetails = sessionGenerator.generateNewSession()
 
-    if (!sessionGenerator.collectEvents) {
-      Log.d(TAG, "Sessions SDK has sampled this session")
-      return
-    }
-
     sessionStartScope.launch {
       val subscribers = FirebaseSessionsDependencies.getSubscribers()
 
       if (subscribers.isEmpty()) {
-        Log.e(
+        Log.d(
           TAG,
           "Sessions SDK did not have any dependent SDKs register as dependencies. Events will not be sent."
         )
@@ -112,6 +107,11 @@ internal constructor(
         }
       }
 
+      if (!sessionGenerator.collectEvents) {
+        Log.d(TAG, "Sessions SDK has sampled this session")
+        return@launch
+      }
+
       sessionCoordinator.attemptLoggingSessionEvent(
         SessionEvents.startSession(firebaseApp, sessionDetails, sessionSettings, timeProvider)
       )

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

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

firebase-sessions/src/test/kotlin/com/google/firebase/sessions/SessionCoordinatorTest.kt

@@ -43,7 +43,7 @@ class SessionCoordinatorTest {
     val firebaseInstallations = FakeFirebaseInstallations("FaKeFiD")
     val sessionCoordinator =
       SessionCoordinator(
-        firebaseInstallations = FakeFirebaseInstallations("FaKeFiD"),
+        firebaseInstallations,
         eventGDTLogger = fakeEventGDTLogger,
       )
 

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)
 }