Skip to content

Commit 46316e6

Browse files
jkasten2jkasten2
authored
Merge pull request #2046 from OneSignal/fix/operation-repo-missing-grouping-modify-key
[Fix] external_id is skipped and updates stop if something updates the User (such as addTag) shortly before login is called
2 parents 55c5443 + bc91777 commit 46316e6

File tree

16 files changed

+170
-34
lines changed

16 files changed

+170
-34
lines changed

‎OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/operations/IOperationRepo.kt

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,7 @@
11
package com.onesignal.core.internal.operations
22

3+
import kotlin.reflect.KClass
4+
35
/**
46
* The operation queue provides a mechanism to queue one or more [Operation] with the promise
57
* it will be executed in a background thread at some point in the future. Operations are
@@ -31,4 +33,13 @@ interface IOperationRepo {
3133
operation: Operation,
3234
flush: Boolean = false,
3335
): Boolean
36+
37+
/**
38+
* Check if the queue contains a specific operation type
39+
*/
40+
fun <T : Operation> containsInstanceOf(type: KClass<T>): Boolean
3441
}
42+
43+
// Extension function so the syntax containsInstanceOf<Operation>() can be used over
44+
// containsInstanceOf(Operation::class)
45+
inline fun <reified T : Operation> IOperationRepo.containsInstanceOf(): Boolean = containsInstanceOf(T::class)

‎OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/operations/impl/OperationRepo.kt

Lines changed: 17 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@ import com.onesignal.debug.internal.logging.Logging
1515
import kotlinx.coroutines.delay
1616
import kotlinx.coroutines.withTimeoutOrNull
1717
import java.util.UUID
18+
import kotlin.reflect.KClass
1819

1920
internal class OperationRepo(
2021
executors: List<IOperationExecutor>,
@@ -26,7 +27,11 @@ internal class OperationRepo(
2627
val operation: Operation,
2728
val waiter: WaiterWithValue<Boolean>? = null,
2829
var retries: Int = 0,
29-
)
30+
) {
31+
override fun toString(): String {
32+
return Pair(operation.toString(), retries).toString() + "\n"
33+
}
34+
}
3035

3136
private val executorsMap: Map<String, IOperationExecutor>
3237
private val queue = mutableListOf<OperationQueueItem>()
@@ -48,6 +53,12 @@ internal class OperationRepo(
4853
}
4954
}
5055

56+
override fun <T : Operation> containsInstanceOf(type: KClass<T>): Boolean {
57+
synchronized(queue) {
58+
return queue.any { type.isInstance(it.operation) }
59+
}
60+
}
61+
5162
override fun start() {
5263
paused = false
5364
suspendifyOnThread(name = "OpRepo") {
@@ -105,7 +116,7 @@ internal class OperationRepo(
105116
}
106117

107118
val ops = getNextOps()
108-
Logging.debug("processQueueForever:ops:$ops")
119+
Logging.debug("processQueueForever:ops:\n$ops")
109120

110121
if (ops != null) {
111122
executeOperations(ops)
@@ -274,6 +285,10 @@ internal class OperationRepo(
274285
val itemKey =
275286
if (startingOp.operation.groupComparisonType == GroupComparisonType.CREATE) item.operation.createComparisonKey else item.operation.modifyComparisonKey
276287

288+
if (itemKey == "" && startingKey == "") {
289+
throw Exception("Both comparison keys can not be blank!")
290+
}
291+
277292
if (itemKey == startingKey) {
278293
queue.remove(item)
279294
ops.add(item)

‎OneSignalSDK/onesignal/core/src/main/java/com/onesignal/user/UserModule.kt

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@ import com.onesignal.user.internal.backend.impl.UserBackendService
1515
import com.onesignal.user.internal.builduser.IRebuildUserService
1616
import com.onesignal.user.internal.builduser.impl.RebuildUserService
1717
import com.onesignal.user.internal.identity.IdentityModelStore
18+
import com.onesignal.user.internal.migrations.RecoverFromDroppedLoginBug
1819
import com.onesignal.user.internal.operations.impl.executors.IdentityOperationExecutor
1920
import com.onesignal.user.internal.operations.impl.executors.LoginUserFromSubscriptionOperationExecutor
2021
import com.onesignal.user.internal.operations.impl.executors.LoginUserOperationExecutor
@@ -65,5 +66,7 @@ internal class UserModule : IModule {
6566
builder.register<UserManager>().provides<IUserManager>()
6667

6768
builder.register<UserRefreshService>().provides<IStartableService>()
69+
70+
builder.register<RecoverFromDroppedLoginBug>().provides<IStartableService>()
6871
}
6972
}

‎OneSignalSDK/onesignal/core/src/main/java/com/onesignal/user/internal/migrations/RecoverFromDroppedLoginBug.kt

Lines changed: 85 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,85 @@
1+
package com.onesignal.user.internal.migrations
2+
3+
import com.onesignal.common.IDManager
4+
import com.onesignal.core.internal.config.ConfigModelStore
5+
import com.onesignal.core.internal.operations.IOperationRepo
6+
import com.onesignal.core.internal.operations.containsInstanceOf
7+
import com.onesignal.core.internal.startup.IStartableService
8+
import com.onesignal.debug.internal.logging.Logging
9+
import com.onesignal.user.internal.identity.IdentityModelStore
10+
import com.onesignal.user.internal.operations.LoginUserOperation
11+
12+
/**
13+
* Purpose: Automatically recovers a stalled User in the OperationRepo due
14+
* to a bug in the SDK from 5.0.0 to 5.1.7.
15+
*
16+
* Issue: Some calls to OneSignal.login() would not be reflected on the
17+
* backend and would stall the the queue for that User. This would result
18+
* in User and Subscription operations to not be processed by
19+
* OperationRepo.
20+
* See PR #2046 for more details.
21+
*
22+
* Even if the developer called OneSignal.login() again with the same
23+
* externalId it would not correct the stalled User.
24+
* - Only calling logout() or login() with different externalId would
25+
* have un-stalled the OperationRepo. And then only after logging
26+
* back to the stalled user would it have recover all the unsent
27+
* operations they may exist.
28+
*/
29+
class RecoverFromDroppedLoginBug(
30+
private val _operationRepo: IOperationRepo,
31+
private val _identityModelStore: IdentityModelStore,
32+
private val _configModelStore: ConfigModelStore,
33+
) : IStartableService {
34+
override fun start() {
35+
if (isInBadState()) {
36+
Logging.warn(
37+
"User with externalId:" +
38+
"${_identityModelStore.model.externalId} " +
39+
"was in a bad state, causing it to not update on OneSignal's " +
40+
"backend! We are recovering and replaying all unsent " +
41+
"operations now.",
42+
)
43+
recoverByAddingBackDroppedLoginOperation()
44+
}
45+
}
46+
47+
// We are in the bad state if ALL are true:
48+
// 1. externalId is set (because OneSignal.login was called)
49+
// 2. We don't have a real yet onesignalId
50+
// - We haven't made a successful user create call yet.
51+
// 3. There is no attempt to create the User left in the
52+
// OperationRepo's queue.
53+
private fun isInBadState(): Boolean {
54+
val externalId = _identityModelStore.model.externalId
55+
val onesignalId = _identityModelStore.model.onesignalId
56+
57+
// NOTE: We are not accounting a more rare (and less important case)
58+
// where a previously logged in User was never created.
59+
// That is, if another user already logged in successfully, but
60+
// the last user still has stuck pending operations due to the
61+
// User never being created on the OneSignal's backend.
62+
return externalId != null &&
63+
IDManager.isLocalId(onesignalId) &&
64+
!_operationRepo.containsInstanceOf<LoginUserOperation>()
65+
}
66+
67+
private fun recoverByAddingBackDroppedLoginOperation() {
68+
// This is the operation that was dropped by mistake in some cases,
69+
// once it is added to the queue all and it gets executed, all
70+
// operations waiting on it will be sent.
71+
72+
// This enqueues at the end, however this is ok, since
73+
// the OperationRepo is designed find the first operation that is
74+
// executable.
75+
_operationRepo.enqueue(
76+
LoginUserOperation(
77+
_configModelStore.model.appId,
78+
_identityModelStore.model.onesignalId,
79+
_identityModelStore.model.externalId,
80+
null,
81+
),
82+
true,
83+
)
84+
}
85+
}

‎OneSignalSDK/onesignal/core/src/main/java/com/onesignal/user/internal/operations/DeleteTagOperation.kt

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -39,7 +39,7 @@ class DeleteTagOperation() : Operation(UpdateUserOperationExecutor.DELETE_TAG) {
3939
}
4040

4141
override val createComparisonKey: String get() = ""
42-
override val modifyComparisonKey: String get() = createComparisonKey
42+
override val modifyComparisonKey: String get() = "$appId.User.$onesignalId"
4343
override val groupComparisonType: GroupComparisonType = GroupComparisonType.ALTER
4444
override val canStartExecute: Boolean get() = !IDManager.isLocalId(onesignalId)
4545

‎OneSignalSDK/onesignal/core/src/main/java/com/onesignal/user/internal/operations/SetPropertyOperation.kt

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -47,7 +47,7 @@ class SetPropertyOperation() : Operation(UpdateUserOperationExecutor.SET_PROPERT
4747
}
4848

4949
override val createComparisonKey: String get() = ""
50-
override val modifyComparisonKey: String get() = createComparisonKey
50+
override val modifyComparisonKey: String get() = "$appId.User.$onesignalId"
5151
override val groupComparisonType: GroupComparisonType = GroupComparisonType.ALTER
5252
override val canStartExecute: Boolean get() = !IDManager.isLocalId(onesignalId)
5353

‎OneSignalSDK/onesignal/core/src/main/java/com/onesignal/user/internal/operations/SetTagOperation.kt

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -48,7 +48,7 @@ class SetTagOperation() : Operation(UpdateUserOperationExecutor.SET_TAG) {
4848
}
4949

5050
override val createComparisonKey: String get() = ""
51-
override val modifyComparisonKey: String get() = createComparisonKey
51+
override val modifyComparisonKey: String get() = "$appId.User.$onesignalId"
5252
override val groupComparisonType: GroupComparisonType = GroupComparisonType.ALTER
5353
override val canStartExecute: Boolean get() = !IDManager.isLocalId(onesignalId)
5454

‎OneSignalSDK/onesignal/core/src/main/java/com/onesignal/user/internal/operations/impl/executors/IdentityOperationExecutor.kt

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,16 @@ internal class IdentityOperationExecutor(
2626
override suspend fun execute(operations: List<Operation>): ExecutionResponse {
2727
Logging.debug("IdentityOperationExecutor(operations: $operations)")
2828

29+
if (operations.any { it !is SetAliasOperation && it !is DeleteAliasOperation }) {
30+
throw Exception("Unrecognized operation(s)! Attempted operations:\n$operations")
31+
}
32+
33+
if (operations.any { it is SetAliasOperation } &&
34+
operations.any { it is DeleteAliasOperation }
35+
) {
36+
throw Exception("Can't process SetAliasOperation and DeleteAliasOperation at the same time.")
37+
}
38+
2939
// An alias group is an appId/onesignalId/aliasLabel combo, so we only care
3040
// about the last operation in the group, as that will be the effective end
3141
// state to this specific alias for this user.

‎OneSignalSDK/onesignal/core/src/main/java/com/onesignal/user/internal/operations/impl/executors/LoginUserFromSubscriptionOperationExecutor.kt

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,10 @@ internal class LoginUserFromSubscriptionOperationExecutor(
2727
override suspend fun execute(operations: List<Operation>): ExecutionResponse {
2828
Logging.debug("LoginUserFromSubscriptionOperationExecutor(operation: $operations)")
2929

30+
if (operations.size > 1) {
31+
throw Exception("Only supports one operation! Attempted operations:\n$operations")
32+
}
33+
3034
val startingOp = operations.first()
3135

3236
if (startingOp is LoginUserFromSubscriptionOperation) {

‎OneSignalSDK/onesignal/core/src/main/java/com/onesignal/user/internal/operations/impl/executors/LoginUserOperationExecutor.kt

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -55,7 +55,7 @@ internal class LoginUserOperationExecutor(
5555
val startingOp = operations.first()
5656

5757
if (startingOp is LoginUserOperation) {
58-
return loginUser(startingOp, operations)
58+
return loginUser(startingOp, operations.drop(1))
5959
}
6060

6161
throw Exception("Unrecognized operation: $startingOp")
@@ -153,6 +153,7 @@ internal class LoginUserOperationExecutor(
153153
is TransferSubscriptionOperation -> subscriptions = createSubscriptionsFromOperation(operation, subscriptions)
154154
is UpdateSubscriptionOperation -> subscriptions = createSubscriptionsFromOperation(operation, subscriptions)
155155
is DeleteSubscriptionOperation -> subscriptions = createSubscriptionsFromOperation(operation, subscriptions)
156+
else -> throw Exception("Unrecognized operation: $operation")
156157
}
157158
}
158159

‎OneSignalSDK/onesignal/core/src/main/java/com/onesignal/user/internal/operations/impl/executors/RefreshUserOperationExecutor.kt

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -38,6 +38,10 @@ internal class RefreshUserOperationExecutor(
3838
override suspend fun execute(operations: List<Operation>): ExecutionResponse {
3939
Logging.log(LogLevel.DEBUG, "RefreshUserOperationExecutor(operation: $operations)")
4040

41+
if (operations.any { it !is RefreshUserOperation }) {
42+
throw Exception("Unrecognized operation(s)! Attempted operations:\n$operations")
43+
}
44+
4145
val startingOp = operations.first()
4246
if (startingOp is RefreshUserOperation) {
4347
return getUser(startingOp)

‎OneSignalSDK/onesignal/core/src/main/java/com/onesignal/user/internal/operations/impl/executors/SubscriptionOperationExecutor.kt

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -49,10 +49,17 @@ internal class SubscriptionOperationExecutor(
4949
return if (startingOp is CreateSubscriptionOperation) {
5050
createSubscription(startingOp, operations)
5151
} else if (operations.any { it is DeleteSubscriptionOperation }) {
52-
deleteSubscription(operations.first { it is DeleteSubscriptionOperation } as DeleteSubscriptionOperation)
52+
if (operations.size > 1) {
53+
throw Exception("Only supports one operation! Attempted operations:\n$operations")
54+
}
55+
val deleteSubOps = operations.filterIsInstance<DeleteSubscriptionOperation>()
56+
deleteSubscription(deleteSubOps.first())
5357
} else if (startingOp is UpdateSubscriptionOperation) {
5458
updateSubscription(startingOp, operations)
5559
} else if (startingOp is TransferSubscriptionOperation) {
60+
if (operations.size > 1) {
61+
throw Exception("TransferSubscriptionOperation only supports one operation! Attempted operations:\n$operations")
62+
}
5663
transferSubscription(startingOp)
5764
} else {
5865
throw Exception("Unrecognized operation: $startingOp")

‎OneSignalSDK/onesignal/core/src/main/java/com/onesignal/user/internal/operations/impl/executors/UpdateUserOperationExecutor.kt

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -114,6 +114,7 @@ internal class UpdateUserOperationExecutor(
114114

115115
deltasObject = PropertiesDeltasObject(deltasObject.sessionTime, deltasObject.sessionCount, amountSpent, purchasesArray)
116116
}
117+
else -> throw Exception("Unrecognized operation: $operation")
117118
}
118119
}
119120

‎OneSignalSDK/onesignal/core/src/test/java/com/onesignal/core/internal/operations/OperationRepoTests.kt

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -62,6 +62,28 @@ class OperationRepoTests : FunSpec({
6262
Logging.logLevel = LogLevel.NONE
6363
}
6464

65+
test("containsInstanceOf") {
66+
// Given
67+
val operationRepo = Mocks().operationRepo
68+
69+
open class MyOperation : Operation("MyOp") {
70+
override val createComparisonKey = ""
71+
override val modifyComparisonKey = ""
72+
override val groupComparisonType = GroupComparisonType.NONE
73+
override val canStartExecute = false
74+
}
75+
76+
class MyOperation2 : MyOperation()
77+
78+
// When
79+
operationRepo.start()
80+
operationRepo.enqueue(MyOperation())
81+
82+
// Then
83+
operationRepo.containsInstanceOf<MyOperation>() shouldBe true
84+
operationRepo.containsInstanceOf<MyOperation2>() shouldBe false
85+
}
86+
6587
// Ensures we are not continuously waking the CPU
6688
test("ensure processQueueForever suspends when queue is empty") {
6789
// Given

‎OneSignalSDK/onesignal/core/src/test/java/com/onesignal/user/internal/operations/LoginUserOperationExecutorTests.kt

Lines changed: 0 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -333,10 +333,6 @@ class LoginUserOperationExecutorTests : FunSpec({
333333
val operations =
334334
listOf<Operation>(
335335
LoginUserOperation(appId, localOneSignalId, null, null),
336-
SetAliasOperation(appId, localOneSignalId, "aliasLabel1", "aliasValue1-1"),
337-
SetAliasOperation(appId, localOneSignalId, "aliasLabel1", "aliasValue1-2"),
338-
SetAliasOperation(appId, localOneSignalId, "aliasLabel2", "aliasValue2"),
339-
DeleteAliasOperation(appId, localOneSignalId, "aliasLabel2"),
340336
CreateSubscriptionOperation(
341337
appId,
342338
localOneSignalId,
@@ -365,20 +361,6 @@ class LoginUserOperationExecutorTests : FunSpec({
365361
SubscriptionStatus.SUBSCRIBED,
366362
),
367363
DeleteSubscriptionOperation(appId, localOneSignalId, "subscriptionId2"),
368-
SetTagOperation(appId, localOneSignalId, "tagKey1", "tagValue1-1"),
369-
SetTagOperation(appId, localOneSignalId, "tagKey1", "tagValue1-2"),
370-
SetTagOperation(appId, localOneSignalId, "tagKey2", "tagValue2"),
371-
DeleteTagOperation(appId, localOneSignalId, "tagKey2"),
372-
SetPropertyOperation(appId, localOneSignalId, PropertiesModel::language.name, "lang1"),
373-
SetPropertyOperation(appId, localOneSignalId, PropertiesModel::language.name, "lang2"),
374-
SetPropertyOperation(appId, localOneSignalId, PropertiesModel::timezone.name, "timezone"),
375-
SetPropertyOperation(appId, localOneSignalId, PropertiesModel::country.name, "country"),
376-
SetPropertyOperation(appId, localOneSignalId, PropertiesModel::locationLatitude.name, 123.45),
377-
SetPropertyOperation(appId, localOneSignalId, PropertiesModel::locationLongitude.name, 678.90),
378-
SetPropertyOperation(appId, localOneSignalId, PropertiesModel::locationType.name, 1),
379-
SetPropertyOperation(appId, localOneSignalId, PropertiesModel::locationAccuracy.name, 0.15),
380-
SetPropertyOperation(appId, localOneSignalId, PropertiesModel::locationBackground.name, true),
381-
SetPropertyOperation(appId, localOneSignalId, PropertiesModel::locationTimestamp.name, 1111L),
382364
)
383365

384366
// When

‎OneSignalSDK/onesignal/core/src/test/java/com/onesignal/user/internal/operations/SubscriptionOperationExecutorTests.kt

Lines changed: 0 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -484,15 +484,6 @@ class SubscriptionOperationExecutorTests : FunSpec({
484484

485485
val operations =
486486
listOf<Operation>(
487-
UpdateSubscriptionOperation(
488-
appId,
489-
remoteOneSignalId,
490-
remoteSubscriptionId,
491-
SubscriptionType.PUSH,
492-
true,
493-
"pushToken2",
494-
SubscriptionStatus.SUBSCRIBED,
495-
),
496487
DeleteSubscriptionOperation(appId, remoteOneSignalId, remoteSubscriptionId),
497488
)
498489

0 commit comments

Comments
 (0)