Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: fix storage timing issue, set sessionId before plugin.setup #190

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 25 additions & 9 deletions android/src/main/java/com/amplitude/android/Amplitude.kt
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand All @@ -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 {
Expand All @@ -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) {
Expand Down Expand Up @@ -94,7 +109,8 @@ open class Amplitude(
}
}

(timeline as Timeline).start()
// Start session before adding plugins
(timeline as Timeline).start(startTime)
}

/**
Expand Down
6 changes: 2 additions & 4 deletions android/src/main/java/com/amplitude/android/Timeline.kt
Original file line number Diff line number Diff line change
Expand Up @@ -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
)

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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()

Expand Down
11 changes: 8 additions & 3 deletions android/src/test/java/com/amplitude/android/AmplitudeTest.kt
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand All @@ -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)
}
}

Expand Down
3 changes: 1 addition & 2 deletions core/src/main/java/com/amplitude/core/Amplitude.kt
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down