fix: fix storage timing issue from last commit, set sessionId before …

…plugin.setup (#190)
amplitude · Mar 27, 2024 · 84b4b9d · 84b4b9d
1 parent c4db161
commit 84b4b9d
Show file tree

Hide file tree

Showing 6 changed files with 43 additions and 24 deletions.
diff --git a/android/src/main/java/com/amplitude/android/Amplitude.kt b/android/src/main/java/com/amplitude/android/Amplitude.kt
@@ -9,13 +9,13 @@ import com.amplitude.android.plugins.AndroidContextPlugin
 import com.amplitude.android.plugins.AndroidLifecyclePlugin
 import com.amplitude.android.plugins.AndroidNetworkConnectivityCheckerPlugin
 import com.amplitude.android.utilities.Session
+import com.amplitude.android.utilities.SystemTime
 import com.amplitude.core.Amplitude
 import com.amplitude.core.events.BaseEvent
 import com.amplitude.core.platform.plugins.AmplitudeDestination
 import com.amplitude.core.platform.plugins.GetAmpliExtrasPlugin
 import com.amplitude.core.utilities.FileStorage
 import com.amplitude.id.IdentityConfiguration
-import kotlinx.coroutines.Deferred
 import kotlinx.coroutines.launch
 
 open class Amplitude(
@@ -25,10 +25,14 @@ open class Amplitude(
     internal var inForeground = false
     private lateinit var androidContextPlugin: AndroidContextPlugin
 
+    private var _initialSessionId = Session.EMPTY_SESSION_ID
     val sessionId: Long
         get() {
-            return if (timeline == null) Session.EMPTY_SESSION_ID
-            else (timeline as Timeline).sessionId
+            return try {
+                (timeline as Timeline).sessionId
+            } catch (e: Throwable) {
+                _initialSessionId
+            }
         }
 
     init {
@@ -52,16 +56,27 @@ open class Amplitude(
         )
     }
 
-    override fun build(): Deferred<Boolean> {
-        val deferred = super.build()
+    private suspend fun loadInitialSessionId(startTime: Long) {
+        // Load in existing session info
+        val session = Session(configuration as Configuration, storage, store)
 
-        // Start session before adding plugins
-        (timeline as Timeline).initSession()
+        // disconnect from storages
+        session.storage = null
+        session.state = null
+
+        // Get the next session id without changing storage values
+        session.startNewSessionIfNeeded(startTime, configuration.sessionId)
 
-        return deferred
+        // Set the initial sessionId before plugins are setup
+        _initialSessionId = session.sessionId
     }
 
     override suspend fun buildInternal(identityConfiguration: IdentityConfiguration) {
+        val startTime = SystemTime.getCurrentTimeMillis()
+
+        // Set the initial sessionId before plugins are setup
+        loadInitialSessionId(startTime)
+
         // Migrations
         ApiKeyStorageMigration(this).execute()
         if ((configuration as Configuration).migrateLegacyData) {
@@ -94,7 +109,8 @@ open class Amplitude(
             }
         }
 
-        (timeline as Timeline).start()
+        // Start session before adding plugins
+        (timeline as Timeline).start(startTime)
     }
 
     /**

diff --git a/android/src/main/java/com/amplitude/android/Timeline.kt b/android/src/main/java/com/amplitude/android/Timeline.kt
@@ -26,17 +26,15 @@ class Timeline : Timeline() {
     internal var sessionId: Long = Session.EMPTY_SESSION_ID
         get() = if (session == null) Session.EMPTY_SESSION_ID else session.sessionId
 
-    internal fun initSession() {
+    internal suspend fun start(timestamp: Long? = null) {
         this.session = Session(
             amplitude.configuration as Configuration,
             amplitude.storage,
             amplitude.store
         )
-    }
 
-    internal suspend fun start() {
         val sessionEvents = session.startNewSessionIfNeeded(
-            SystemTime.getCurrentTimeMillis(),
+            timestamp ?: SystemTime.getCurrentTimeMillis(),
             amplitude.configuration.sessionId
         )
 

diff --git a/android/src/main/java/com/amplitude/android/utilities/Session.kt b/android/src/main/java/com/amplitude/android/utilities/Session.kt
@@ -9,8 +9,8 @@ import java.util.concurrent.atomic.AtomicLong
 
 class Session(
     private var configuration: Configuration,
-    private var storage: Storage? = null,
-    private var state: State? = null,
+    internal var storage: Storage? = null,
+    internal var state: State? = null,
 ) {
     companion object {
         const val EMPTY_SESSION_ID = -1L

diff --git a/android/src/test/java/com/amplitude/android/AmplitudeSessionTest.kt b/android/src/test/java/com/amplitude/android/AmplitudeSessionTest.kt
@@ -482,11 +482,12 @@ class AmplitudeSessionTest {
 
     @Test
     fun amplitude_explicitSessionForEventShouldBePreserved() = runTest {
-        val amplitude = Amplitude(createConfiguration())
-        setDispatcher(amplitude, testScheduler)
-
+        val config = createConfiguration()
         val mockedPlugin = spyk(StubPlugin())
-        amplitude.add(mockedPlugin)
+        config.plugins = listOf(mockedPlugin)
+
+        val amplitude = Amplitude(config)
+        setDispatcher(amplitude, testScheduler)
 
         amplitude.isBuilt.await()
 

diff --git a/android/src/test/java/com/amplitude/android/AmplitudeTest.kt b/android/src/test/java/com/amplitude/android/AmplitudeTest.kt
@@ -234,6 +234,8 @@ class AmplitudeTest {
 
     @Test
     fun amplitude_should_set_sessionId_before_plugin_setup() = runTest {
+        var setupSessionId: Long? = null
+
         class SessionIdPlugin() : DestinationPlugin() {
             override val type: Plugin.Type = Plugin.Type.Destination
 
@@ -244,23 +246,26 @@ class AmplitudeTest {
 
                 this.amplitude = amplitude
 
-                val sessionId = (amplitude as Amplitude).sessionId
+                setupSessionId = (amplitude as Amplitude).sessionId
 
-                Assertions.assertNotNull(sessionId)
+                Assertions.assertNotNull(setupSessionId)
+                Assertions.assertNotEquals(-1L, setupSessionId)
             }
         }
 
         // set session Id in the config
         val config = createConfiguration()
         // isolate storage from other tests
         config.instanceName = "session-id-for-plugin-setup"
+        config.plugins = listOf(SessionIdPlugin())
         val amp = Amplitude(config)
-        amp.add(SessionIdPlugin())
 
         setDispatcher(testScheduler)
 
         if (amp?.isBuilt!!.await()) {
             Assertions.assertNotNull(amp?.sessionId)
+            Assertions.assertNotEquals(-1L, amp?.sessionId)
+            Assertions.assertEquals(setupSessionId, amp?.sessionId)
         }
     }
 

diff --git a/core/src/main/java/com/amplitude/core/Amplitude.kt b/core/src/main/java/com/amplitude/core/Amplitude.kt
@@ -100,10 +100,9 @@ open class Amplitude internal constructor(
     protected open fun build(): Deferred<Boolean> {
         val amplitude = this
 
-        storage = configuration.storageProvider.getStorage(amplitude)
-
         val built =
             amplitudeScope.async(amplitudeDispatcher, CoroutineStart.LAZY) {
+                storage = configuration.storageProvider.getStorage(amplitude)
                 identifyInterceptStorage =
                     configuration.identifyInterceptStorageProvider.getStorage(
                         amplitude,