From 7fe2010dd1326b1a1e47619c0f3ee17b919aa221 Mon Sep 17 00:00:00 2001 From: Josh Kasten Date: Mon, 8 Apr 2024 19:19:45 -0400 Subject: [PATCH 1/7] fix missing modify keys on some operations All operations must have at least a create or modify key, this is required so operations are not grouped together when they should not be. This fixes a bug where calling addTag() back-to-back with login() would cause the login to be dropped. This is because SetTagOperation.modifyComparisonKey was "" which matched LoginUserOperation.modifyComparisonKey as it was also "" and both these operations we returned by OperationRepo.getGroupableOperations. Both operations were given to UpdateUserOperationExecutor which it would skip the LoginUserOperation silently. This mistake originated from f1d204e3ebaff51aa8f4a7239bc681e918d75078 which wasn't in the 5.0.0-beta4 release, but was introduced in 5.0.0. The intent of PR #1794 is still preserved in this commit, we just simply fixed the bug noted above. --- .../onesignal/user/internal/operations/DeleteTagOperation.kt | 2 +- .../onesignal/user/internal/operations/SetPropertyOperation.kt | 2 +- .../com/onesignal/user/internal/operations/SetTagOperation.kt | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/user/internal/operations/DeleteTagOperation.kt b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/user/internal/operations/DeleteTagOperation.kt index 9ec0d70a5..023187cba 100644 --- a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/user/internal/operations/DeleteTagOperation.kt +++ b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/user/internal/operations/DeleteTagOperation.kt @@ -39,7 +39,7 @@ class DeleteTagOperation() : Operation(UpdateUserOperationExecutor.DELETE_TAG) { } override val createComparisonKey: String get() = "" - override val modifyComparisonKey: String get() = createComparisonKey + override val modifyComparisonKey: String get() = "$appId.User.$onesignalId" override val groupComparisonType: GroupComparisonType = GroupComparisonType.ALTER override val canStartExecute: Boolean get() = !IDManager.isLocalId(onesignalId) diff --git a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/user/internal/operations/SetPropertyOperation.kt b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/user/internal/operations/SetPropertyOperation.kt index c441504e3..94dc51fbf 100644 --- a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/user/internal/operations/SetPropertyOperation.kt +++ b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/user/internal/operations/SetPropertyOperation.kt @@ -47,7 +47,7 @@ class SetPropertyOperation() : Operation(UpdateUserOperationExecutor.SET_PROPERT } override val createComparisonKey: String get() = "" - override val modifyComparisonKey: String get() = createComparisonKey + override val modifyComparisonKey: String get() = "$appId.User.$onesignalId" override val groupComparisonType: GroupComparisonType = GroupComparisonType.ALTER override val canStartExecute: Boolean get() = !IDManager.isLocalId(onesignalId) diff --git a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/user/internal/operations/SetTagOperation.kt b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/user/internal/operations/SetTagOperation.kt index 733d0efcb..25826bf7f 100644 --- a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/user/internal/operations/SetTagOperation.kt +++ b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/user/internal/operations/SetTagOperation.kt @@ -48,7 +48,7 @@ class SetTagOperation() : Operation(UpdateUserOperationExecutor.SET_TAG) { } override val createComparisonKey: String get() = "" - override val modifyComparisonKey: String get() = createComparisonKey + override val modifyComparisonKey: String get() = "$appId.User.$onesignalId" override val groupComparisonType: GroupComparisonType = GroupComparisonType.ALTER override val canStartExecute: Boolean get() = !IDManager.isLocalId(onesignalId) From 80dbf09971158f11d92c762f33a04c388caf453b Mon Sep 17 00:00:00 2001 From: Josh Kasten Date: Mon, 8 Apr 2024 20:32:34 -0400 Subject: [PATCH 2/7] add operation checks to all executors To ensure operations are not silently skipped each executor now checks if it can process all the operations. This is important since bugs can go unnoticed for a while and without this it's hard to debug too. --- .../impl/executors/IdentityOperationExecutor.kt | 10 ++++++++++ .../LoginUserFromSubscriptionOperationExecutor.kt | 4 ++++ .../impl/executors/LoginUserOperationExecutor.kt | 3 ++- .../impl/executors/RefreshUserOperationExecutor.kt | 4 ++++ .../impl/executors/SubscriptionOperationExecutor.kt | 9 ++++++++- .../impl/executors/UpdateUserOperationExecutor.kt | 1 + 6 files changed, 29 insertions(+), 2 deletions(-) diff --git a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/user/internal/operations/impl/executors/IdentityOperationExecutor.kt b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/user/internal/operations/impl/executors/IdentityOperationExecutor.kt index 80b991b44..68ce3663d 100644 --- a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/user/internal/operations/impl/executors/IdentityOperationExecutor.kt +++ b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/user/internal/operations/impl/executors/IdentityOperationExecutor.kt @@ -26,6 +26,16 @@ internal class IdentityOperationExecutor( override suspend fun execute(operations: List): ExecutionResponse { Logging.debug("IdentityOperationExecutor(operations: $operations)") + if (operations.any { it !is SetAliasOperation && it !is DeleteAliasOperation }) { + throw Exception("Unrecognized operation(s)! Attempted operations:\n$operations") + } + + if (operations.any { it is SetAliasOperation } && + operations.any { it is DeleteAliasOperation } + ) { + throw Exception("Can't process SetAliasOperation and DeleteAliasOperation at the same time.") + } + // An alias group is an appId/onesignalId/aliasLabel combo, so we only care // about the last operation in the group, as that will be the effective end // state to this specific alias for this user. diff --git a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/user/internal/operations/impl/executors/LoginUserFromSubscriptionOperationExecutor.kt b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/user/internal/operations/impl/executors/LoginUserFromSubscriptionOperationExecutor.kt index e60ee5b8d..9a1178999 100644 --- a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/user/internal/operations/impl/executors/LoginUserFromSubscriptionOperationExecutor.kt +++ b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/user/internal/operations/impl/executors/LoginUserFromSubscriptionOperationExecutor.kt @@ -27,6 +27,10 @@ internal class LoginUserFromSubscriptionOperationExecutor( override suspend fun execute(operations: List): ExecutionResponse { Logging.debug("LoginUserFromSubscriptionOperationExecutor(operation: $operations)") + if (operations.size > 1) { + throw Exception("Only supports one operation! Attempted operations:\n$operations") + } + val startingOp = operations.first() if (startingOp is LoginUserFromSubscriptionOperation) { diff --git a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/user/internal/operations/impl/executors/LoginUserOperationExecutor.kt b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/user/internal/operations/impl/executors/LoginUserOperationExecutor.kt index 3f29b9097..280207803 100644 --- a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/user/internal/operations/impl/executors/LoginUserOperationExecutor.kt +++ b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/user/internal/operations/impl/executors/LoginUserOperationExecutor.kt @@ -55,7 +55,7 @@ internal class LoginUserOperationExecutor( val startingOp = operations.first() if (startingOp is LoginUserOperation) { - return loginUser(startingOp, operations) + return loginUser(startingOp, operations.drop(1)) } throw Exception("Unrecognized operation: $startingOp") @@ -153,6 +153,7 @@ internal class LoginUserOperationExecutor( is TransferSubscriptionOperation -> subscriptions = createSubscriptionsFromOperation(operation, subscriptions) is UpdateSubscriptionOperation -> subscriptions = createSubscriptionsFromOperation(operation, subscriptions) is DeleteSubscriptionOperation -> subscriptions = createSubscriptionsFromOperation(operation, subscriptions) + else -> throw Exception("Unrecognized operation: $operation") } } diff --git a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/user/internal/operations/impl/executors/RefreshUserOperationExecutor.kt b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/user/internal/operations/impl/executors/RefreshUserOperationExecutor.kt index d71991de3..4266ab903 100644 --- a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/user/internal/operations/impl/executors/RefreshUserOperationExecutor.kt +++ b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/user/internal/operations/impl/executors/RefreshUserOperationExecutor.kt @@ -38,6 +38,10 @@ internal class RefreshUserOperationExecutor( override suspend fun execute(operations: List): ExecutionResponse { Logging.log(LogLevel.DEBUG, "RefreshUserOperationExecutor(operation: $operations)") + if (operations.any { it !is RefreshUserOperation }) { + throw Exception("Unrecognized operation(s)! Attempted operations:\n$operations") + } + val startingOp = operations.first() if (startingOp is RefreshUserOperation) { return getUser(startingOp) diff --git a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/user/internal/operations/impl/executors/SubscriptionOperationExecutor.kt b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/user/internal/operations/impl/executors/SubscriptionOperationExecutor.kt index a13f47c3f..a9cdf9fe9 100644 --- a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/user/internal/operations/impl/executors/SubscriptionOperationExecutor.kt +++ b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/user/internal/operations/impl/executors/SubscriptionOperationExecutor.kt @@ -49,10 +49,17 @@ internal class SubscriptionOperationExecutor( return if (startingOp is CreateSubscriptionOperation) { createSubscription(startingOp, operations) } else if (operations.any { it is DeleteSubscriptionOperation }) { - deleteSubscription(operations.first { it is DeleteSubscriptionOperation } as DeleteSubscriptionOperation) + if (operations.size > 1) { + throw Exception("Only supports one operation! Attempted operations:\n$operations") + } + val deleteSubOps = operations.filterIsInstance() + deleteSubscription(deleteSubOps.first()) } else if (startingOp is UpdateSubscriptionOperation) { updateSubscription(startingOp, operations) } else if (startingOp is TransferSubscriptionOperation) { + if (operations.size > 1) { + throw Exception("TransferSubscriptionOperation only supports one operation! Attempted operations:\n$operations") + } transferSubscription(startingOp) } else { throw Exception("Unrecognized operation: $startingOp") diff --git a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/user/internal/operations/impl/executors/UpdateUserOperationExecutor.kt b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/user/internal/operations/impl/executors/UpdateUserOperationExecutor.kt index 19551eaca..1002ee1f0 100644 --- a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/user/internal/operations/impl/executors/UpdateUserOperationExecutor.kt +++ b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/user/internal/operations/impl/executors/UpdateUserOperationExecutor.kt @@ -114,6 +114,7 @@ internal class UpdateUserOperationExecutor( deltasObject = PropertiesDeltasObject(deltasObject.sessionTime, deltasObject.sessionCount, amountSpent, purchasesArray) } + else -> throw Exception("Unrecognized operation: $operation") } } From 20075f717c305d639f7cf22327df4d5d32302e41 Mon Sep 17 00:00:00 2001 From: Josh Kasten Date: Mon, 8 Apr 2024 20:34:34 -0400 Subject: [PATCH 3/7] debugging, print out the full operation details --- .../core/internal/operations/impl/OperationRepo.kt | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/operations/impl/OperationRepo.kt b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/operations/impl/OperationRepo.kt index f339de234..3cf8759b8 100644 --- a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/operations/impl/OperationRepo.kt +++ b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/operations/impl/OperationRepo.kt @@ -26,7 +26,11 @@ internal class OperationRepo( val operation: Operation, val waiter: WaiterWithValue? = null, var retries: Int = 0, - ) + ) { + override fun toString(): String { + return Pair(operation.toString(), retries).toString() + "\n" + } + } private val executorsMap: Map private val queue = mutableListOf() @@ -105,7 +109,7 @@ internal class OperationRepo( } val ops = getNextOps() - Logging.debug("processQueueForever:ops:$ops") + Logging.debug("processQueueForever:ops:\n$ops") if (ops != null) { executeOperations(ops) From 4f0be52729b1bb553c79f4e74227e535920fdbb2 Mon Sep 17 00:00:00 2001 From: Josh Kasten Date: Mon, 8 Apr 2024 21:11:32 -0400 Subject: [PATCH 4/7] fix mrege operations login test This test was missed when support for this was droped in commit f1d204e3ebaff51aa8f4a7239bc681e918d75078 --- .../LoginUserOperationExecutorTests.kt | 18 ------------------ 1 file changed, 18 deletions(-) diff --git a/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/user/internal/operations/LoginUserOperationExecutorTests.kt b/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/user/internal/operations/LoginUserOperationExecutorTests.kt index 92db84b76..b992fdc28 100644 --- a/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/user/internal/operations/LoginUserOperationExecutorTests.kt +++ b/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/user/internal/operations/LoginUserOperationExecutorTests.kt @@ -333,10 +333,6 @@ class LoginUserOperationExecutorTests : FunSpec({ val operations = listOf( LoginUserOperation(appId, localOneSignalId, null, null), - SetAliasOperation(appId, localOneSignalId, "aliasLabel1", "aliasValue1-1"), - SetAliasOperation(appId, localOneSignalId, "aliasLabel1", "aliasValue1-2"), - SetAliasOperation(appId, localOneSignalId, "aliasLabel2", "aliasValue2"), - DeleteAliasOperation(appId, localOneSignalId, "aliasLabel2"), CreateSubscriptionOperation( appId, localOneSignalId, @@ -365,20 +361,6 @@ class LoginUserOperationExecutorTests : FunSpec({ SubscriptionStatus.SUBSCRIBED, ), DeleteSubscriptionOperation(appId, localOneSignalId, "subscriptionId2"), - SetTagOperation(appId, localOneSignalId, "tagKey1", "tagValue1-1"), - SetTagOperation(appId, localOneSignalId, "tagKey1", "tagValue1-2"), - SetTagOperation(appId, localOneSignalId, "tagKey2", "tagValue2"), - DeleteTagOperation(appId, localOneSignalId, "tagKey2"), - SetPropertyOperation(appId, localOneSignalId, PropertiesModel::language.name, "lang1"), - SetPropertyOperation(appId, localOneSignalId, PropertiesModel::language.name, "lang2"), - SetPropertyOperation(appId, localOneSignalId, PropertiesModel::timezone.name, "timezone"), - SetPropertyOperation(appId, localOneSignalId, PropertiesModel::country.name, "country"), - SetPropertyOperation(appId, localOneSignalId, PropertiesModel::locationLatitude.name, 123.45), - SetPropertyOperation(appId, localOneSignalId, PropertiesModel::locationLongitude.name, 678.90), - SetPropertyOperation(appId, localOneSignalId, PropertiesModel::locationType.name, 1), - SetPropertyOperation(appId, localOneSignalId, PropertiesModel::locationAccuracy.name, 0.15), - SetPropertyOperation(appId, localOneSignalId, PropertiesModel::locationBackground.name, true), - SetPropertyOperation(appId, localOneSignalId, PropertiesModel::locationTimestamp.name, 1111L), ) // When From 29a7fd9e3800114a3f30caafa307edd62c57b00c Mon Sep 17 00:00:00 2001 From: Josh Kasten Date: Mon, 8 Apr 2024 21:15:31 -0400 Subject: [PATCH 5/7] fix delete subscription test This test had an extra update subscription operation. 1. This test shouldn't contain an update since it is testing a delete 2. An update and a delete could never be executed together as it's keys are different and therefore would never be grouped together. --- .../operations/SubscriptionOperationExecutorTests.kt | 9 --------- 1 file changed, 9 deletions(-) diff --git a/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/user/internal/operations/SubscriptionOperationExecutorTests.kt b/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/user/internal/operations/SubscriptionOperationExecutorTests.kt index 1b235738b..471e69f6f 100644 --- a/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/user/internal/operations/SubscriptionOperationExecutorTests.kt +++ b/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/user/internal/operations/SubscriptionOperationExecutorTests.kt @@ -484,15 +484,6 @@ class SubscriptionOperationExecutorTests : FunSpec({ val operations = listOf( - UpdateSubscriptionOperation( - appId, - remoteOneSignalId, - remoteSubscriptionId, - SubscriptionType.PUSH, - true, - "pushToken2", - SubscriptionStatus.SUBSCRIBED, - ), DeleteSubscriptionOperation(appId, remoteOneSignalId, remoteSubscriptionId), ) From cbe6f16cc7c0555ef5ad3eab7807fef8feb73f3b Mon Sep 17 00:00:00 2001 From: Josh Kasten Date: Mon, 8 Apr 2024 21:45:22 -0400 Subject: [PATCH 6/7] Add check that both comparison keys are not blank We want to know if this happens as soon as possible, as it will almost always result in operations being combined when they should not. --- .../onesignal/core/internal/operations/impl/OperationRepo.kt | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/operations/impl/OperationRepo.kt b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/operations/impl/OperationRepo.kt index 3cf8759b8..167d8d1a4 100644 --- a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/operations/impl/OperationRepo.kt +++ b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/operations/impl/OperationRepo.kt @@ -278,6 +278,10 @@ internal class OperationRepo( val itemKey = if (startingOp.operation.groupComparisonType == GroupComparisonType.CREATE) item.operation.createComparisonKey else item.operation.modifyComparisonKey + if (itemKey == "" && startingKey == "") { + throw Exception("Both comparison keys can not be blank!") + } + if (itemKey == startingKey) { queue.remove(item) ops.add(item) From bc917772972ae5e652567d1fcb904d9516e40aff Mon Sep 17 00:00:00 2001 From: Josh Kasten Date: Tue, 9 Apr 2024 16:52:24 -0400 Subject: [PATCH 7/7] recover from a stalled user in the OperationRepo Commit 7fe2010dd1326b1a1e47619c0f3ee17b919aa221 fixed a bug with login operations being drop in some scenarios, however it doesn't fix those already in the bad state. This comment get us out of the bad state by generating a replacement LoginUserOperation. See the comments in the code for more details. This comment was manually tested, ensuring it gets us out of the bad state, and updates works afterwords. --- .../internal/operations/IOperationRepo.kt | 11 +++ .../internal/operations/impl/OperationRepo.kt | 7 ++ .../java/com/onesignal/user/UserModule.kt | 3 + .../migrations/RecoverFromDroppedLoginBug.kt | 85 +++++++++++++++++++ .../internal/operations/OperationRepoTests.kt | 22 +++++ 5 files changed, 128 insertions(+) create mode 100644 OneSignalSDK/onesignal/core/src/main/java/com/onesignal/user/internal/migrations/RecoverFromDroppedLoginBug.kt diff --git a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/operations/IOperationRepo.kt b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/operations/IOperationRepo.kt index 9c28d1e81..9cc6c2d4b 100644 --- a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/operations/IOperationRepo.kt +++ b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/operations/IOperationRepo.kt @@ -1,5 +1,7 @@ package com.onesignal.core.internal.operations +import kotlin.reflect.KClass + /** * The operation queue provides a mechanism to queue one or more [Operation] with the promise * it will be executed in a background thread at some point in the future. Operations are @@ -31,4 +33,13 @@ interface IOperationRepo { operation: Operation, flush: Boolean = false, ): Boolean + + /** + * Check if the queue contains a specific operation type + */ + fun containsInstanceOf(type: KClass): Boolean } + +// Extension function so the syntax containsInstanceOf() can be used over +// containsInstanceOf(Operation::class) +inline fun IOperationRepo.containsInstanceOf(): Boolean = containsInstanceOf(T::class) diff --git a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/operations/impl/OperationRepo.kt b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/operations/impl/OperationRepo.kt index 167d8d1a4..fd82c22ac 100644 --- a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/operations/impl/OperationRepo.kt +++ b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/operations/impl/OperationRepo.kt @@ -15,6 +15,7 @@ import com.onesignal.debug.internal.logging.Logging import kotlinx.coroutines.delay import kotlinx.coroutines.withTimeoutOrNull import java.util.UUID +import kotlin.reflect.KClass internal class OperationRepo( executors: List, @@ -52,6 +53,12 @@ internal class OperationRepo( } } + override fun containsInstanceOf(type: KClass): Boolean { + synchronized(queue) { + return queue.any { type.isInstance(it.operation) } + } + } + override fun start() { paused = false suspendifyOnThread(name = "OpRepo") { diff --git a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/user/UserModule.kt b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/user/UserModule.kt index f371e6d39..c1b463286 100644 --- a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/user/UserModule.kt +++ b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/user/UserModule.kt @@ -15,6 +15,7 @@ import com.onesignal.user.internal.backend.impl.UserBackendService import com.onesignal.user.internal.builduser.IRebuildUserService import com.onesignal.user.internal.builduser.impl.RebuildUserService import com.onesignal.user.internal.identity.IdentityModelStore +import com.onesignal.user.internal.migrations.RecoverFromDroppedLoginBug import com.onesignal.user.internal.operations.impl.executors.IdentityOperationExecutor import com.onesignal.user.internal.operations.impl.executors.LoginUserFromSubscriptionOperationExecutor import com.onesignal.user.internal.operations.impl.executors.LoginUserOperationExecutor @@ -65,5 +66,7 @@ internal class UserModule : IModule { builder.register().provides() builder.register().provides() + + builder.register().provides() } } diff --git a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/user/internal/migrations/RecoverFromDroppedLoginBug.kt b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/user/internal/migrations/RecoverFromDroppedLoginBug.kt new file mode 100644 index 000000000..573a72f29 --- /dev/null +++ b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/user/internal/migrations/RecoverFromDroppedLoginBug.kt @@ -0,0 +1,85 @@ +package com.onesignal.user.internal.migrations + +import com.onesignal.common.IDManager +import com.onesignal.core.internal.config.ConfigModelStore +import com.onesignal.core.internal.operations.IOperationRepo +import com.onesignal.core.internal.operations.containsInstanceOf +import com.onesignal.core.internal.startup.IStartableService +import com.onesignal.debug.internal.logging.Logging +import com.onesignal.user.internal.identity.IdentityModelStore +import com.onesignal.user.internal.operations.LoginUserOperation + +/** + * Purpose: Automatically recovers a stalled User in the OperationRepo due + * to a bug in the SDK from 5.0.0 to 5.1.7. + * + * Issue: Some calls to OneSignal.login() would not be reflected on the + * backend and would stall the the queue for that User. This would result + * in User and Subscription operations to not be processed by + * OperationRepo. + * See PR #2046 for more details. + * + * Even if the developer called OneSignal.login() again with the same + * externalId it would not correct the stalled User. + * - Only calling logout() or login() with different externalId would + * have un-stalled the OperationRepo. And then only after logging + * back to the stalled user would it have recover all the unsent + * operations they may exist. + */ +class RecoverFromDroppedLoginBug( + private val _operationRepo: IOperationRepo, + private val _identityModelStore: IdentityModelStore, + private val _configModelStore: ConfigModelStore, +) : IStartableService { + override fun start() { + if (isInBadState()) { + Logging.warn( + "User with externalId:" + + "${_identityModelStore.model.externalId} " + + "was in a bad state, causing it to not update on OneSignal's " + + "backend! We are recovering and replaying all unsent " + + "operations now.", + ) + recoverByAddingBackDroppedLoginOperation() + } + } + + // We are in the bad state if ALL are true: + // 1. externalId is set (because OneSignal.login was called) + // 2. We don't have a real yet onesignalId + // - We haven't made a successful user create call yet. + // 3. There is no attempt to create the User left in the + // OperationRepo's queue. + private fun isInBadState(): Boolean { + val externalId = _identityModelStore.model.externalId + val onesignalId = _identityModelStore.model.onesignalId + + // NOTE: We are not accounting a more rare (and less important case) + // where a previously logged in User was never created. + // That is, if another user already logged in successfully, but + // the last user still has stuck pending operations due to the + // User never being created on the OneSignal's backend. + return externalId != null && + IDManager.isLocalId(onesignalId) && + !_operationRepo.containsInstanceOf() + } + + private fun recoverByAddingBackDroppedLoginOperation() { + // This is the operation that was dropped by mistake in some cases, + // once it is added to the queue all and it gets executed, all + // operations waiting on it will be sent. + + // This enqueues at the end, however this is ok, since + // the OperationRepo is designed find the first operation that is + // executable. + _operationRepo.enqueue( + LoginUserOperation( + _configModelStore.model.appId, + _identityModelStore.model.onesignalId, + _identityModelStore.model.externalId, + null, + ), + true, + ) + } +} diff --git a/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/core/internal/operations/OperationRepoTests.kt b/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/core/internal/operations/OperationRepoTests.kt index e4f0f6809..f456d3479 100644 --- a/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/core/internal/operations/OperationRepoTests.kt +++ b/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/core/internal/operations/OperationRepoTests.kt @@ -62,6 +62,28 @@ class OperationRepoTests : FunSpec({ Logging.logLevel = LogLevel.NONE } + test("containsInstanceOf") { + // Given + val operationRepo = Mocks().operationRepo + + open class MyOperation : Operation("MyOp") { + override val createComparisonKey = "" + override val modifyComparisonKey = "" + override val groupComparisonType = GroupComparisonType.NONE + override val canStartExecute = false + } + + class MyOperation2 : MyOperation() + + // When + operationRepo.start() + operationRepo.enqueue(MyOperation()) + + // Then + operationRepo.containsInstanceOf() shouldBe true + operationRepo.containsInstanceOf() shouldBe false + } + // Ensures we are not continuously waking the CPU test("ensure processQueueForever suspends when queue is empty") { // Given