Skip to content

Commit 0139681

Browse files
polbinspolbins
committed
address review comments
1 parent 46f0f80 commit 0139681

File tree

4 files changed

+88
-45
lines changed

4 files changed

+88
-45
lines changed

‎android/src/main/java/com/amplitude/android/Amplitude.kt‎

Lines changed: 11 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@ import com.amplitude.android.utilities.ActivityLifecycleObserver
1313
import com.amplitude.core.State
1414
import com.amplitude.core.platform.plugins.AmplitudeDestination
1515
import com.amplitude.core.platform.plugins.GetAmpliExtrasPlugin
16+
import com.amplitude.id.Identity
1617
import com.amplitude.id.IdentityConfiguration
1718
import kotlinx.coroutines.CoroutineDispatcher
1819
import kotlinx.coroutines.CoroutineScope
@@ -39,7 +40,7 @@ open class Amplitude internal constructor(
3940
) {
4041
constructor(configuration: Configuration) : this(configuration, State())
4142

42-
internal lateinit var androidContextPlugin: AndroidContextPlugin
43+
private lateinit var androidContextPlugin: AndroidContextPlugin
4344

4445
val sessionId: Long
4546
get() {
@@ -95,7 +96,7 @@ open class Amplitude internal constructor(
9596
}
9697
androidContextPlugin = AndroidContextPlugin()
9798
add(androidContextPlugin)
98-
setDeviceId(androidContextPlugin.generateDeviceId())
99+
setDeviceId(androidContextPlugin.generateDeviceId(forceNew = false))
99100
add(GetAmpliExtrasPlugin())
100101
add(AndroidLifecyclePlugin(activityLifecycleCallbacks))
101102
add(AnalyticsConnectorIdentityPlugin())
@@ -112,29 +113,25 @@ open class Amplitude internal constructor(
112113
* @return the Amplitude instance
113114
*/
114115
override fun reset(): Amplitude {
115-
// IMMEDIATE: Clear identity first so generateDeviceId() creates a new one
116-
store.userId = null
117-
store.deviceId = null
118-
// Generate new deviceId (won't reuse store since we cleared it)
119-
val newDeviceId = androidContextPlugin.generateDeviceId()
120-
store.deviceId = newDeviceId
121-
// QUEUED: For event ordering + persistence
122-
(timeline as Timeline).queueSetIdentity(store.identity)
116+
if (!isBuilt.isCompleted) {
117+
logger.error("Cannot reset identity before Amplitude is initialized.")
118+
return this
119+
}
120+
val newDeviceId = androidContextPlugin.generateDeviceId(forceNew = true)
121+
val newIdentity = Identity(userId = null, deviceId = newDeviceId)
122+
store.identity = newIdentity
123+
(timeline as Timeline).queueSetIdentity(newIdentity)
123124
return this
124125
}
125126

126127
override fun setUserId(userId: String?): Amplitude {
127-
// IMMEDIATE: Update state, triggers ObservePlugins
128128
store.userId = userId
129-
// QUEUED: For event ordering + persistence
130129
(timeline as Timeline).queueSetIdentity(store.identity)
131130
return this
132131
}
133132

134133
override fun setDeviceId(deviceId: String): Amplitude {
135-
// IMMEDIATE: Update state, triggers ObservePlugins
136134
store.deviceId = deviceId
137-
// QUEUED: For event ordering + persistence
138135
(timeline as Timeline).queueSetIdentity(store.identity)
139136
return this
140137
}

‎android/src/main/java/com/amplitude/android/plugins/AndroidContextPlugin.kt‎

Lines changed: 11 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -31,20 +31,23 @@ open class AndroidContextPlugin : ContextPlugin() {
3131

3232
/**
3333
* Generate a device ID using Android-specific logic.
34-
* Priority: configuration -> store -> advertising ID -> app set ID -> random UUID
34+
* Priority: configuration -> store (if not forceNew) -> advertising ID -> app set ID -> random UUID
3535
*
36+
* @param forceNew If true, skip checking the store and generate a fresh deviceId
3637
* @return the generated device ID (never null for Android)
3738
*/
38-
override fun generateDeviceId(): String {
39+
fun generateDeviceId(forceNew: Boolean = false): String {
3940
val configuration = amplitude.configuration as Configuration
4041

41-
// Check configuration
42+
// Check configuration (always respected, even with forceNew)
4243
configuration.deviceId?.let { return it }
4344

44-
// Check store
45-
amplitude.store.deviceId?.let { deviceId ->
46-
if (validDeviceId(deviceId) && !deviceId.endsWith("S")) {
47-
return deviceId
45+
// Check store (skip if forcing new)
46+
if (!forceNew) {
47+
amplitude.store.deviceId?.let { deviceId ->
48+
if (validDeviceId(deviceId) && !deviceId.endsWith("S")) {
49+
return deviceId
50+
}
4851
}
4952
}
5053

@@ -73,13 +76,12 @@ open class AndroidContextPlugin : ContextPlugin() {
7376
return UUID.randomUUID().toString() + "R"
7477
}
7578

76-
@Suppress("UNUSED_PARAMETER")
7779
@Deprecated(
7880
message = "Use generateDeviceId() instead. Amplitude now handles setting the deviceId.",
7981
replaceWith = ReplaceWith("generateDeviceId()"),
8082
)
8183
fun initializeDeviceId(configuration: Configuration) {
82-
amplitude.setDeviceId(generateDeviceId())
84+
amplitude.setDeviceId(generateDeviceId(forceNew = false))
8385
}
8486

8587
@Deprecated(

‎android/src/test/kotlin/com/amplitude/android/AmplitudeTest.kt‎

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -469,6 +469,29 @@ class AmplitudeTest {
469469
)
470470
}
471471

472+
/**
473+
* Test that reset() does nothing when called before SDK initialization completes.
474+
*/
475+
@Test
476+
fun amplitude_reset_before_build_should_do_nothing() =
477+
runTest {
478+
val amplitude =
479+
createFakeAmplitude(
480+
scheduler = testScheduler,
481+
configuration = createConfiguration(),
482+
)
483+
484+
// Call reset immediately before waiting for build - should do nothing
485+
amplitude.reset()
486+
487+
// Now wait for build to complete
488+
amplitude.isBuilt.await()
489+
advanceUntilIdle()
490+
491+
// Verify identity was NOT reset (initial deviceId should still be present)
492+
assertNotNull("deviceId should exist", amplitude.store.deviceId)
493+
}
494+
472495
companion object {
473496
private const val INSTANCE_NAME = "testInstance"
474497
}

‎core/src/main/java/com/amplitude/core/State.kt‎

Lines changed: 43 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -9,51 +9,72 @@ class State {
99

1010
var identity: Identity
1111
get() = synchronized(lock) { _identity }
12-
set(value) = setIdentityInternal(value)
12+
set(value) {
13+
val changeInfo =
14+
synchronized(lock) {
15+
computeIdentityChange(value)
16+
} ?: return
17+
notifyObservers(changeInfo)
18+
}
1319

1420
// Delegating property for backwards compatibility
1521
var userId: String?
1622
get() = synchronized(lock) { _identity.userId }
1723
set(value) {
18-
synchronized(lock) {
19-
applyIdentityChange(Identity(userId = value, deviceId = _identity.deviceId))
20-
}
24+
val changeInfo =
25+
synchronized(lock) {
26+
computeIdentityChange(Identity(userId = value, deviceId = _identity.deviceId))
27+
} ?: return
28+
notifyObservers(changeInfo)
2129
}
2230

2331
// Delegating property for backwards compatibility
2432
var deviceId: String?
2533
get() = synchronized(lock) { _identity.deviceId }
2634
set(value) {
27-
synchronized(lock) {
28-
applyIdentityChange(Identity(userId = _identity.userId, deviceId = value))
29-
}
30-
}
31-
32-
private fun setIdentityInternal(newIdentity: Identity) {
33-
synchronized(lock) {
34-
applyIdentityChange(newIdentity)
35+
val changeInfo =
36+
synchronized(lock) {
37+
computeIdentityChange(Identity(userId = _identity.userId, deviceId = value))
38+
} ?: return
39+
notifyObservers(changeInfo)
3540
}
36-
}
3741

3842
/**
39-
* Apply an identity change. Must be called while holding [lock].
43+
* Compute identity change info. Must be called while holding [lock].
44+
* Returns null if no change occurred (idempotent).
4045
*/
41-
private fun applyIdentityChange(newIdentity: Identity) {
46+
private fun computeIdentityChange(newIdentity: Identity): IdentityChangeInfo? {
4247
val oldIdentity = _identity
43-
if (oldIdentity == newIdentity) return // Idempotent
48+
if (oldIdentity == newIdentity) return null
4449

4550
_identity = newIdentity
4651

47-
val userIdChanged = oldIdentity.userId != newIdentity.userId
48-
val deviceIdChanged = oldIdentity.deviceId != newIdentity.deviceId
52+
return IdentityChangeInfo(
53+
newIdentity = newIdentity,
54+
plugins = plugins.toList(),
55+
userIdChanged = oldIdentity.userId != newIdentity.userId,
56+
deviceIdChanged = oldIdentity.deviceId != newIdentity.deviceId,
57+
)
58+
}
4959

50-
plugins.toList().forEach { plugin ->
51-
if (userIdChanged) plugin.onUserIdChanged(newIdentity.userId)
52-
if (deviceIdChanged) plugin.onDeviceIdChanged(newIdentity.deviceId)
53-
plugin.onIdentityChanged(newIdentity)
60+
/**
61+
* Notify observers about identity change. Called outside the lock.
62+
*/
63+
private fun notifyObservers(changeInfo: IdentityChangeInfo) {
64+
changeInfo.plugins.forEach { plugin ->
65+
if (changeInfo.userIdChanged) plugin.onUserIdChanged(changeInfo.newIdentity.userId)
66+
if (changeInfo.deviceIdChanged) plugin.onDeviceIdChanged(changeInfo.newIdentity.deviceId)
67+
plugin.onIdentityChanged(changeInfo.newIdentity)
5468
}
5569
}
5670

71+
private data class IdentityChangeInfo(
72+
val newIdentity: Identity,
73+
val plugins: List<ObservePlugin>,
74+
val userIdChanged: Boolean,
75+
val deviceIdChanged: Boolean,
76+
)
77+
5778
val plugins: MutableList<ObservePlugin> = mutableListOf()
5879

5980
fun add(

0 commit comments

Comments
 (0)