From 83fa82b0ade6bbfddf6f500eccda3b033168b5eb Mon Sep 17 00:00:00 2001 From: Josh Kasten Date: Wed, 17 Apr 2024 16:38:02 -0400 Subject: [PATCH 1/6] create test to prove we don't wait after create This is to account for the case where we create a User or Subscription and attempt to immediately access it (via GET or PATCH). A delay is needed as the backend may incorrectly 404 otherwise, due to a small delay in it's server replication. A cold down period like this also helps improve batching as well. --- .../core/internal/config/ConfigModel.kt | 13 +++++ .../internal/operations/OperationRepoTests.kt | 51 +++++++++++++++++++ 2 files changed, 64 insertions(+) diff --git a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/config/ConfigModel.kt b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/config/ConfigModel.kt index 22b3297931..a2d283a133 100644 --- a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/config/ConfigModel.kt +++ b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/config/ConfigModel.kt @@ -141,6 +141,19 @@ class ConfigModel : Model() { setLongProperty(::opRepoPostWakeDelay.name, value) } + /** + * The number milliseconds to delay after an operation completes + * that creates or changes ids. + * This is a "cold down" period to avoid a caveat with OneSignal's backend + * replication, where you may incorrectly get a 404 when attempting a GET + * or PATCH REST API call on something just after it is created. + */ + var opRepoPostCreateDelay: Long + get() = getLongProperty(::opRepoPostCreateDelay.name) { 5_000 } + set(value) { + setLongProperty(::opRepoPostCreateDelay.name, value) + } + /** * The minimum number of milliseconds required to pass to allow the fetching of IAM to occur. */ 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 243e539622..0f6030307d 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 @@ -23,7 +23,9 @@ import io.mockk.slot import io.mockk.spyk import io.mockk.verify import kotlinx.coroutines.delay +import kotlinx.coroutines.launch import kotlinx.coroutines.withTimeoutOrNull +import kotlinx.coroutines.yield // Mocks used by every test in this file private class Mocks { @@ -485,6 +487,54 @@ class OperationRepoTests : FunSpec({ executor.execute(withArg { it[0] shouldBe secondOp }) } } + + // This is to account for the case where we create a User or Subscription + // and attempt to immediately access it (via GET or PATCH). A delay is + // needed as the backend may incorrectly 404 otherwise, due to a small + // delay in it's server replication. + // A cold down period like this also helps improve batching as well. + test("execution of an operation with translation IDs delays follow up operations") { + // Given + val mocks = Mocks() + mocks.configModelStore.model.opRepoPostCreateDelay = 100 + val operation1 = mockOperation(groupComparisonType = GroupComparisonType.NONE) + val operation2 = mockOperation(groupComparisonType = GroupComparisonType.NONE, applyToRecordId = "id2") + val operation3 = mockOperation(groupComparisonType = GroupComparisonType.NONE) + coEvery { + mocks.executor.execute(listOf(operation1)) + } returns ExecutionResponse(ExecutionResult.SUCCESS, mapOf("local-id1" to "id2")) + + // When + mocks.operationRepo.start() + mocks.operationRepo.enqueue(operation1) + val job = launch { mocks.operationRepo.enqueueAndWait(operation2) }.also { yield() } + mocks.operationRepo.enqueue(operation3) + job.join() + + // Then + coVerifyOrder { + mocks.executor.execute( + withArg { + it.count() shouldBe 1 + it[0] shouldBe operation1 + }, + ) + operation2.translateIds(mapOf("local-id1" to "id2")) + mocks.executor.execute( + withArg { + it.count() shouldBe 1 + it[0] shouldBe operation3 + }, + ) + // Ensure operation2 runs after operation3 as it has to wait for the create delay + mocks.executor.execute( + withArg { + it.count() shouldBe 1 + it[0] shouldBe operation2 + }, + ) + } + } }) { companion object { private fun mockOperation( @@ -495,6 +545,7 @@ class OperationRepoTests : FunSpec({ createComparisonKey: String = "create-key", modifyComparisonKey: String = "modify-key", operationIdSlot: CapturingSlot? = null, + applyToRecordId: String = "", ): Operation { val operation = mockk() val opIdSlot = operationIdSlot ?: slot() From 59fd1df66b5c29bbe2ce2faec5c137c6f00a7684 Mon Sep 17 00:00:00 2001 From: Josh Kasten Date: Wed, 17 Apr 2024 17:21:39 -0400 Subject: [PATCH 2/6] Add opRepoPostCreateDelay This is a new behavior which wakes a specific amount of time after something is created (such as a User or Subscription) before make a network call to get or update it. The main motivation is that the OneSignal's server may return a 404 if you attempt to GET or PATCH on something that was just created. This is due fact that OneSignal's backend server replication sometimes has a delay under load. This may be considered a bug in the backend, but on the flip side the SDK is being inefficient if a follow up network request is made any way, which was the 2nd motivation for this change. --- .../core/internal/operations/Operation.kt | 6 ++++ .../internal/operations/impl/OperationRepo.kt | 33 ++++++++++++++----- .../java/com/onesignal/user/UserModule.kt | 4 +++ .../operations/CreateSubscriptionOperation.kt | 1 + .../operations/DeleteAliasOperation.kt | 1 + .../operations/DeleteSubscriptionOperation.kt | 1 + .../internal/operations/DeleteTagOperation.kt | 1 + .../LoginUserFromSubscriptionOperation.kt | 1 + .../internal/operations/LoginUserOperation.kt | 1 + .../operations/RefreshUserOperation.kt | 1 + .../internal/operations/SetAliasOperation.kt | 1 + .../operations/SetPropertyOperation.kt | 1 + .../internal/operations/SetTagOperation.kt | 1 + .../operations/TrackPurchaseOperation.kt | 1 + .../operations/TrackSessionEndOperation.kt | 1 + .../operations/TrackSessionStartOperation.kt | 1 + .../TransferSubscriptionOperation.kt | 1 + .../operations/UpdateSubscriptionOperation.kt | 1 + .../operations/impl/states/NewRecordsState.kt | 27 +++++++++++++++ .../internal/operations/OperationRepoTests.kt | 4 +++ .../user/internal/operations/ExecutorMocks.kt | 12 +++++++ .../java/com/onesignal/mocks/MockHelper.kt | 1 + 22 files changed, 93 insertions(+), 9 deletions(-) create mode 100644 OneSignalSDK/onesignal/core/src/main/java/com/onesignal/user/internal/operations/impl/states/NewRecordsState.kt create mode 100644 OneSignalSDK/onesignal/core/src/test/java/com/onesignal/user/internal/operations/ExecutorMocks.kt diff --git a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/operations/Operation.kt b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/operations/Operation.kt index 42fdd62374..76f51994ab 100644 --- a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/operations/Operation.kt +++ b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/operations/Operation.kt @@ -20,6 +20,12 @@ abstract class Operation(name: String) : Model() { this.name = name } + /** + * This is a unique id that points to a record this operation will affect. + * Example: If the operation is updating tags on a User this will be the onesignalId. + */ + abstract val applyToRecordId: String + /** * The key of this operation for when the starting operation has a [groupComparisonType] * of [GroupComparisonType.CREATE] 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 e94de1be20..222035560d 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 @@ -1,7 +1,6 @@ package com.onesignal.core.internal.operations.impl import com.onesignal.common.threading.WaiterWithValue -import com.onesignal.common.threading.suspendifyOnThread import com.onesignal.core.internal.config.ConfigModelStore import com.onesignal.core.internal.operations.ExecutionResult import com.onesignal.core.internal.operations.GroupComparisonType @@ -12,7 +11,11 @@ import com.onesignal.core.internal.startup.IStartableService import com.onesignal.core.internal.time.ITime import com.onesignal.debug.LogLevel import com.onesignal.debug.internal.logging.Logging +import com.onesignal.user.internal.operations.impl.states.NewRecordsState +import kotlinx.coroutines.CoroutineScope import kotlinx.coroutines.delay +import kotlinx.coroutines.launch +import kotlinx.coroutines.newSingleThreadContext import kotlinx.coroutines.withTimeoutOrNull import java.util.UUID import kotlin.reflect.KClass @@ -22,6 +25,7 @@ internal class OperationRepo( private val _operationModelStore: OperationModelStore, private val _configModelStore: ConfigModelStore, private val _time: ITime, + private val _newRecordState: NewRecordsState, ) : IOperationRepo, IStartableService { internal class OperationQueueItem( val operation: Operation, @@ -38,6 +42,7 @@ internal class OperationRepo( private val queue = mutableListOf() private val waiter = WaiterWithValue() private var paused = false + private var coroutineScope = CoroutineScope(newSingleThreadContext(name = "OpRepo")) /** *** Buckets *** * Purpose: Bucketing is a pattern we are using to help save network @@ -56,13 +61,11 @@ internal class OperationRepo( * network calls. */ private var enqueueIntoBucket = 0 - private val executeBucket: Int get() { - return if (enqueueIntoBucket == 0) 0 else enqueueIntoBucket - 1 - } + private val executeBucket get() = + if (enqueueIntoBucket == 0) 0 else enqueueIntoBucket - 1 init { val executorsMap: MutableMap = mutableMapOf() - for (executor in executors) { for (operation in executor.operations) { executorsMap[operation] = executor @@ -83,9 +86,7 @@ internal class OperationRepo( override fun start() { paused = false - suspendifyOnThread(name = "OpRepo") { - processQueueForever() - } + coroutineScope.launch { processQueueForever() } } override fun enqueue( @@ -195,6 +196,18 @@ internal class OperationRepo( synchronized(queue) { queue.forEach { it.operation.translateIds(response.idTranslations) } } + response.idTranslations.values.forEach { _newRecordState.add(it) } + coroutineScope.launch { + delay(_configModelStore.model.opRepoPostCreateDelay) + synchronized(queue) { + // NOTE: Even if the queue is not empty we may wake + // when not needed, as those operations may not have + // depended on these ids. This however should be very + // rare and the side-effect is only a bit less + // batching. + if (queue.isNotEmpty()) waiter.wake(false) + } + } } when (response.result) { @@ -278,7 +291,9 @@ internal class OperationRepo( return synchronized(queue) { val startingOp = queue.firstOrNull { - it.operation.canStartExecute && it.bucket <= bucketFilter + it.operation.canStartExecute && + _newRecordState.canAccess(it.operation.applyToRecordId) && + it.bucket <= bucketFilter } if (startingOp != null) { 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 c1b463286e..7e2c5daacb 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 @@ -25,6 +25,7 @@ import com.onesignal.user.internal.operations.impl.executors.UpdateUserOperation import com.onesignal.user.internal.operations.impl.listeners.IdentityModelStoreListener import com.onesignal.user.internal.operations.impl.listeners.PropertiesModelStoreListener import com.onesignal.user.internal.operations.impl.listeners.SubscriptionModelStoreListener +import com.onesignal.user.internal.operations.impl.states.NewRecordsState import com.onesignal.user.internal.properties.PropertiesModelStore import com.onesignal.user.internal.service.UserRefreshService import com.onesignal.user.internal.subscriptions.ISubscriptionManager @@ -68,5 +69,8 @@ internal class UserModule : IModule { builder.register().provides() builder.register().provides() + + // Shared state between Executors + builder.register().provides() } } diff --git a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/user/internal/operations/CreateSubscriptionOperation.kt b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/user/internal/operations/CreateSubscriptionOperation.kt index 59c463b7f8..3fe1b718b2 100644 --- a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/user/internal/operations/CreateSubscriptionOperation.kt +++ b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/user/internal/operations/CreateSubscriptionOperation.kt @@ -86,6 +86,7 @@ class CreateSubscriptionOperation() : Operation(SubscriptionOperationExecutor.CR override val modifyComparisonKey: String get() = "$appId.User.$onesignalId.Subscription.$subscriptionId" override val groupComparisonType: GroupComparisonType = GroupComparisonType.ALTER override val canStartExecute: Boolean get() = !IDManager.isLocalId(onesignalId) + override val applyToRecordId: String get() = onesignalId constructor(appId: String, onesignalId: String, subscriptionId: String, type: SubscriptionType, enabled: Boolean, address: String, status: SubscriptionStatus) : this() { this.appId = appId diff --git a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/user/internal/operations/DeleteAliasOperation.kt b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/user/internal/operations/DeleteAliasOperation.kt index c2ae81f5bb..1595a6de2b 100644 --- a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/user/internal/operations/DeleteAliasOperation.kt +++ b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/user/internal/operations/DeleteAliasOperation.kt @@ -41,6 +41,7 @@ class DeleteAliasOperation() : Operation(IdentityOperationExecutor.DELETE_ALIAS) override val modifyComparisonKey: String get() = "$appId.User.$onesignalId.Alias.$label" override val groupComparisonType: GroupComparisonType = GroupComparisonType.NONE override val canStartExecute: Boolean get() = !IDManager.isLocalId(onesignalId) + override val applyToRecordId: String get() = onesignalId constructor(appId: String, onesignalId: String, label: String) : this() { this.appId = appId diff --git a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/user/internal/operations/DeleteSubscriptionOperation.kt b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/user/internal/operations/DeleteSubscriptionOperation.kt index b7724a7404..40ee092d8b 100644 --- a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/user/internal/operations/DeleteSubscriptionOperation.kt +++ b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/user/internal/operations/DeleteSubscriptionOperation.kt @@ -42,6 +42,7 @@ class DeleteSubscriptionOperation() : Operation(SubscriptionOperationExecutor.DE override val modifyComparisonKey: String get() = "$appId.User.$onesignalId.Subscription.$subscriptionId" override val groupComparisonType: GroupComparisonType = GroupComparisonType.NONE override val canStartExecute: Boolean get() = !IDManager.isLocalId(onesignalId) && !IDManager.isLocalId(onesignalId) + override val applyToRecordId: String get() = subscriptionId constructor(appId: String, onesignalId: String, subscriptionId: String) : this() { this.appId = appId 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 023187cbac..f88ae3c568 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 @@ -42,6 +42,7 @@ class DeleteTagOperation() : Operation(UpdateUserOperationExecutor.DELETE_TAG) { override val modifyComparisonKey: String get() = "$appId.User.$onesignalId" override val groupComparisonType: GroupComparisonType = GroupComparisonType.ALTER override val canStartExecute: Boolean get() = !IDManager.isLocalId(onesignalId) + override val applyToRecordId: String get() = onesignalId constructor(appId: String, onesignalId: String, key: String) : this() { this.appId = appId diff --git a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/user/internal/operations/LoginUserFromSubscriptionOperation.kt b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/user/internal/operations/LoginUserFromSubscriptionOperation.kt index a8aa8af7e4..9597283f75 100644 --- a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/user/internal/operations/LoginUserFromSubscriptionOperation.kt +++ b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/user/internal/operations/LoginUserFromSubscriptionOperation.kt @@ -40,6 +40,7 @@ class LoginUserFromSubscriptionOperation() : Operation(LoginUserFromSubscription override val modifyComparisonKey: String get() = "$appId.Subscription.$subscriptionId.Login" override val groupComparisonType: GroupComparisonType = GroupComparisonType.NONE override val canStartExecute: Boolean = true + override val applyToRecordId: String get() = subscriptionId constructor(appId: String, onesignalId: String, subscriptionId: String) : this() { this.appId = appId diff --git a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/user/internal/operations/LoginUserOperation.kt b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/user/internal/operations/LoginUserOperation.kt index 67eb10ea9a..e03390537d 100644 --- a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/user/internal/operations/LoginUserOperation.kt +++ b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/user/internal/operations/LoginUserOperation.kt @@ -56,6 +56,7 @@ class LoginUserOperation() : Operation(LoginUserOperationExecutor.LOGIN_USER) { override val modifyComparisonKey: String = "" override val groupComparisonType: GroupComparisonType = GroupComparisonType.CREATE override val canStartExecute: Boolean get() = existingOnesignalId == null || !IDManager.isLocalId(existingOnesignalId!!) + override val applyToRecordId: String get() = onesignalId constructor(appId: String, onesignalId: String, externalId: String?, existingOneSignalId: String? = null) : this() { this.appId = appId diff --git a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/user/internal/operations/RefreshUserOperation.kt b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/user/internal/operations/RefreshUserOperation.kt index bba5e1708c..953cbe7b9c 100644 --- a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/user/internal/operations/RefreshUserOperation.kt +++ b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/user/internal/operations/RefreshUserOperation.kt @@ -33,6 +33,7 @@ class RefreshUserOperation() : Operation(RefreshUserOperationExecutor.REFRESH_US override val modifyComparisonKey: String get() = "$appId.User.$onesignalId.Refresh" override val groupComparisonType: GroupComparisonType = GroupComparisonType.CREATE override val canStartExecute: Boolean get() = !IDManager.isLocalId(onesignalId) + override val applyToRecordId: String get() = onesignalId constructor(appId: String, onesignalId: String) : this() { this.appId = appId diff --git a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/user/internal/operations/SetAliasOperation.kt b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/user/internal/operations/SetAliasOperation.kt index 206c6d6556..c88c23e46f 100644 --- a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/user/internal/operations/SetAliasOperation.kt +++ b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/user/internal/operations/SetAliasOperation.kt @@ -51,6 +51,7 @@ class SetAliasOperation() : Operation(IdentityOperationExecutor.SET_ALIAS) { override val modifyComparisonKey: String get() = "$appId.User.$onesignalId.Identity.$label" override val groupComparisonType: GroupComparisonType = GroupComparisonType.ALTER override val canStartExecute: Boolean get() = !IDManager.isLocalId(onesignalId) + override val applyToRecordId: String get() = onesignalId constructor(appId: String, onesignalId: String, label: String, value: String) : this() { this.appId = appId 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 94dc51fbff..2aff9c174a 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 @@ -50,6 +50,7 @@ class SetPropertyOperation() : Operation(UpdateUserOperationExecutor.SET_PROPERT override val modifyComparisonKey: String get() = "$appId.User.$onesignalId" override val groupComparisonType: GroupComparisonType = GroupComparisonType.ALTER override val canStartExecute: Boolean get() = !IDManager.isLocalId(onesignalId) + override val applyToRecordId: String get() = onesignalId constructor(appId: String, onesignalId: String, property: String, value: Any?) : this() { this.appId = appId 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 25826bf7f3..88bfa06eda 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 @@ -51,6 +51,7 @@ class SetTagOperation() : Operation(UpdateUserOperationExecutor.SET_TAG) { override val modifyComparisonKey: String get() = "$appId.User.$onesignalId" override val groupComparisonType: GroupComparisonType = GroupComparisonType.ALTER override val canStartExecute: Boolean get() = !IDManager.isLocalId(onesignalId) + override val applyToRecordId: String get() = onesignalId constructor(appId: String, onesignalId: String, key: String, value: String) : this() { this.appId = appId diff --git a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/user/internal/operations/TrackPurchaseOperation.kt b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/user/internal/operations/TrackPurchaseOperation.kt index 1d7dafbfb1..78da8cfb0a 100644 --- a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/user/internal/operations/TrackPurchaseOperation.kt +++ b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/user/internal/operations/TrackPurchaseOperation.kt @@ -63,6 +63,7 @@ class TrackPurchaseOperation() : Operation(UpdateUserOperationExecutor.TRACK_PUR override val modifyComparisonKey: String get() = "$appId.User.$onesignalId" override val groupComparisonType: GroupComparisonType = GroupComparisonType.ALTER override val canStartExecute: Boolean get() = !IDManager.isLocalId(onesignalId) + override val applyToRecordId: String get() = onesignalId constructor(appId: String, onesignalId: String, treatNewAsExisting: Boolean, amountSpent: BigDecimal, purchases: List) : this() { this.appId = appId diff --git a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/user/internal/operations/TrackSessionEndOperation.kt b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/user/internal/operations/TrackSessionEndOperation.kt index 7b5ef70442..940051e91b 100644 --- a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/user/internal/operations/TrackSessionEndOperation.kt +++ b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/user/internal/operations/TrackSessionEndOperation.kt @@ -41,6 +41,7 @@ class TrackSessionEndOperation() : Operation(UpdateUserOperationExecutor.TRACK_S override val modifyComparisonKey: String get() = "$appId.User.$onesignalId" override val groupComparisonType: GroupComparisonType = GroupComparisonType.ALTER override val canStartExecute: Boolean get() = !IDManager.isLocalId(onesignalId) + override val applyToRecordId: String get() = onesignalId constructor(appId: String, onesignalId: String, sessionTime: Long) : this() { this.appId = appId diff --git a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/user/internal/operations/TrackSessionStartOperation.kt b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/user/internal/operations/TrackSessionStartOperation.kt index 4512857491..e5b9e0f29c 100644 --- a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/user/internal/operations/TrackSessionStartOperation.kt +++ b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/user/internal/operations/TrackSessionStartOperation.kt @@ -32,6 +32,7 @@ class TrackSessionStartOperation() : Operation(UpdateUserOperationExecutor.TRACK override val modifyComparisonKey: String get() = "$appId.User.$onesignalId" override val groupComparisonType: GroupComparisonType = GroupComparisonType.ALTER override val canStartExecute: Boolean get() = !IDManager.isLocalId(onesignalId) + override val applyToRecordId: String get() = onesignalId constructor(appId: String, onesignalId: String) : this() { this.appId = appId diff --git a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/user/internal/operations/TransferSubscriptionOperation.kt b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/user/internal/operations/TransferSubscriptionOperation.kt index fce653c323..0122bf8729 100644 --- a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/user/internal/operations/TransferSubscriptionOperation.kt +++ b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/user/internal/operations/TransferSubscriptionOperation.kt @@ -42,6 +42,7 @@ class TransferSubscriptionOperation() : Operation(SubscriptionOperationExecutor. override val modifyComparisonKey: String get() = "$appId.Subscription.$subscriptionId.Transfer" override val groupComparisonType: GroupComparisonType = GroupComparisonType.NONE override val canStartExecute: Boolean get() = !IDManager.isLocalId(onesignalId) && !IDManager.isLocalId(subscriptionId) + override val applyToRecordId: String get() = subscriptionId constructor(appId: String, subscriptionId: String, onesignalId: String) : this() { this.appId = appId diff --git a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/user/internal/operations/UpdateSubscriptionOperation.kt b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/user/internal/operations/UpdateSubscriptionOperation.kt index 38f3fa2108..59a9e1c1b7 100644 --- a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/user/internal/operations/UpdateSubscriptionOperation.kt +++ b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/user/internal/operations/UpdateSubscriptionOperation.kt @@ -85,6 +85,7 @@ class UpdateSubscriptionOperation() : Operation(SubscriptionOperationExecutor.UP override val modifyComparisonKey: String get() = "$appId.User.$onesignalId.Subscription.$subscriptionId" override val groupComparisonType: GroupComparisonType = GroupComparisonType.ALTER override val canStartExecute: Boolean get() = !IDManager.isLocalId(onesignalId) && !IDManager.isLocalId(onesignalId) + override val applyToRecordId: String get() = subscriptionId constructor(appId: String, onesignalId: String, subscriptionId: String, type: SubscriptionType, enabled: Boolean, address: String, status: SubscriptionStatus) : this() { this.appId = appId diff --git a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/user/internal/operations/impl/states/NewRecordsState.kt b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/user/internal/operations/impl/states/NewRecordsState.kt new file mode 100644 index 0000000000..fd3e2183a2 --- /dev/null +++ b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/user/internal/operations/impl/states/NewRecordsState.kt @@ -0,0 +1,27 @@ +package com.onesignal.user.internal.operations.impl.states + +import com.onesignal.core.internal.config.ConfigModelStore +import com.onesignal.core.internal.time.ITime + +/** + * Purpose: Keeps track of ids that were just created on the backend. + * This list gets used to delay network calls to ensure upcoming + * requests are ready to be accepted by the backend. + */ +class NewRecordsState( + private val _time: ITime, + private val _configModelStore: ConfigModelStore, +) { + // Key = a string id + // Value = A Timestamp in ms of when the id was created + private val records: MutableMap = mutableMapOf() + + fun add(key: String) { + records[key] = _time.currentTimeMillis + } + + fun canAccess(key: String): Boolean { + val timeLastMovedOrCreated = records[key] ?: return true + return _time.currentTimeMillis - timeLastMovedOrCreated > _configModelStore.model.opRepoPostCreateDelay + } +} 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 0f6030307d..7a05f270ee 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 @@ -9,6 +9,7 @@ import com.onesignal.core.internal.time.impl.Time import com.onesignal.debug.LogLevel import com.onesignal.debug.internal.logging.Logging import com.onesignal.mocks.MockHelper +import com.onesignal.user.internal.operations.ExecutorMocks.Companion.getNewRecordState import io.kotest.core.spec.style.FunSpec import io.kotest.matchers.shouldBe import io.mockk.CapturingSlot @@ -55,6 +56,7 @@ private class Mocks { operationModelStore, configModelStore, Time(), + getNewRecordState(configModelStore), ), ) } @@ -75,6 +77,7 @@ class OperationRepoTests : FunSpec({ override val modifyComparisonKey = "" override val groupComparisonType = GroupComparisonType.NONE override val canStartExecute = false + override val applyToRecordId = "" } class MyOperation2 : MyOperation() @@ -558,6 +561,7 @@ class OperationRepoTests : FunSpec({ every { operation.createComparisonKey } returns createComparisonKey every { operation.modifyComparisonKey } returns modifyComparisonKey every { operation.translateIds(any()) } just runs + every { operation.applyToRecordId } returns applyToRecordId return operation } diff --git a/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/user/internal/operations/ExecutorMocks.kt b/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/user/internal/operations/ExecutorMocks.kt new file mode 100644 index 0000000000..9890563333 --- /dev/null +++ b/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/user/internal/operations/ExecutorMocks.kt @@ -0,0 +1,12 @@ +package com.onesignal.user.internal.operations + +import com.onesignal.core.internal.config.ConfigModelStore +import com.onesignal.core.internal.time.impl.Time +import com.onesignal.mocks.MockHelper +import com.onesignal.user.internal.operations.impl.states.NewRecordsState + +class ExecutorMocks { + companion object { + fun getNewRecordState(configModelStore: ConfigModelStore = MockHelper.configModelStore()) = NewRecordsState(Time(), configModelStore) + } +} diff --git a/OneSignalSDK/onesignal/testhelpers/src/main/java/com/onesignal/mocks/MockHelper.kt b/OneSignalSDK/onesignal/testhelpers/src/main/java/com/onesignal/mocks/MockHelper.kt index 27901d92bd..bf6bde7879 100644 --- a/OneSignalSDK/onesignal/testhelpers/src/main/java/com/onesignal/mocks/MockHelper.kt +++ b/OneSignalSDK/onesignal/testhelpers/src/main/java/com/onesignal/mocks/MockHelper.kt @@ -46,6 +46,7 @@ object MockHelper { configModel.opRepoExecutionInterval = 1 configModel.opRepoPostWakeDelay = 1 + configModel.opRepoPostCreateDelay = 1 configModel.appId = DEFAULT_APP_ID From 5ffa8a319844b03741f9ad839c1d4b34b1e1a4ca Mon Sep 17 00:00:00 2001 From: Josh Kasten Date: Wed, 17 Apr 2024 19:46:55 -0400 Subject: [PATCH 3/6] Retry on 404/410 within short window after create In summary, this is fallback logic to opRepoPostCreateDelay. The opRepoPostCreateDelay is a balance between not waiting too long where the SDK isn't being response, but long enough where the likelihood of running into the backend replica delay issue is low. Given the above, we can further lower the chance of this issue by retrying requests that fail with 404 or 410 for Subscriptions. We don't want to do this with all 404/410 requests so we utilize the same NewRecordsState list, but added a new isInMissingRetryWindow method and opRepoPostCreateRetryUpTo value to drive the window value. Added tests to executors to ensure the new 404 logic is working. Some executors were missing 404 handling altogether so we filled in those gaps in this commit as well. --- .../core/internal/config/ConfigModel.kt | 15 ++- .../executors/IdentityOperationExecutor.kt | 24 +++- .../executors/RefreshUserOperationExecutor.kt | 5 + .../SubscriptionOperationExecutor.kt | 26 +++- .../executors/UpdateUserOperationExecutor.kt | 5 + .../operations/impl/states/NewRecordsState.kt | 8 +- .../IdentityOperationExecutorTests.kt | 109 ++++++++++++++- .../RefreshUserOperationExecutorTests.kt | 83 +++++++++++- .../SubscriptionOperationExecutorTests.kt | 127 ++++++++++++++++++ .../UpdateUserOperationExecutorTests.kt | 67 +++++++++ .../java/com/onesignal/mocks/MockHelper.kt | 1 + 11 files changed, 450 insertions(+), 20 deletions(-) diff --git a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/config/ConfigModel.kt b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/config/ConfigModel.kt index a2d283a133..81328efb54 100644 --- a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/config/ConfigModel.kt +++ b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/config/ConfigModel.kt @@ -142,7 +142,7 @@ class ConfigModel : Model() { } /** - * The number milliseconds to delay after an operation completes + * The number of milliseconds to delay after an operation completes * that creates or changes ids. * This is a "cold down" period to avoid a caveat with OneSignal's backend * replication, where you may incorrectly get a 404 when attempting a GET @@ -154,6 +154,19 @@ class ConfigModel : Model() { setLongProperty(::opRepoPostCreateDelay.name, value) } + /** + * The number of milliseconds to retry operations for new models. + * This is a fallback to opRepoPostCreateDelay, where it's delay may + * not be enough. The server may be unusually overloaded so we will + * retry these (back-off rules apply to all retries) as we only want + * to re-create records as a last resort. + */ + var opRepoPostCreateRetryUpTo: Long + get() = getLongProperty(::opRepoPostCreateRetryUpTo.name) { 60_000 } + set(value) { + setLongProperty(::opRepoPostCreateRetryUpTo.name, value) + } + /** * The minimum number of milliseconds required to pass to allow the fetching of IAM to occur. */ 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 93b90faaa1..88a98f3586 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 @@ -14,11 +14,13 @@ import com.onesignal.user.internal.builduser.IRebuildUserService import com.onesignal.user.internal.identity.IdentityModelStore import com.onesignal.user.internal.operations.DeleteAliasOperation import com.onesignal.user.internal.operations.SetAliasOperation +import com.onesignal.user.internal.operations.impl.states.NewRecordsState internal class IdentityOperationExecutor( private val _identityBackend: IIdentityBackendService, private val _identityModelStore: IdentityModelStore, private val _buildUserService: IRebuildUserService, + private val _newRecordState: NewRecordsState, ) : IOperationExecutor { override val operations: List get() = listOf(SET_ALIAS, DELETE_ALIAS) @@ -67,11 +69,15 @@ internal class IdentityOperationExecutor( NetworkUtils.ResponseStatusType.UNAUTHORIZED -> ExecutionResponse(ExecutionResult.FAIL_UNAUTHORIZED) NetworkUtils.ResponseStatusType.MISSING -> { - val operations = _buildUserService.getRebuildOperationsIfCurrentUser(lastOperation.appId, lastOperation.onesignalId) - if (operations == null) { + if (_newRecordState.isInMissingRetryWindow(lastOperation.onesignalId)) { + return ExecutionResponse(ExecutionResult.FAIL_RETRY) + } + + val rebuildOps = _buildUserService.getRebuildOperationsIfCurrentUser(lastOperation.appId, lastOperation.onesignalId) + if (rebuildOps == null) { return ExecutionResponse(ExecutionResult.FAIL_NORETRY) } else { - return ExecutionResponse(ExecutionResult.FAIL_RETRY, operations = operations) + return ExecutionResponse(ExecutionResult.FAIL_RETRY, operations = rebuildOps) } } } @@ -103,10 +109,14 @@ internal class IdentityOperationExecutor( NetworkUtils.ResponseStatusType.UNAUTHORIZED -> ExecutionResponse(ExecutionResult.FAIL_UNAUTHORIZED) NetworkUtils.ResponseStatusType.MISSING -> { - // This means either the User or the Alias was already - // deleted, either way the end state is the same, the - // alias no longer exists on that User. - ExecutionResponse(ExecutionResult.SUCCESS) + return if (_newRecordState.isInMissingRetryWindow(lastOperation.onesignalId)) { + ExecutionResponse(ExecutionResult.FAIL_RETRY) + } else { + // This means either the User or the Alias was already + // deleted, either way the end state is the same, the + // alias no longer exists on that User. + ExecutionResponse(ExecutionResult.SUCCESS) + } } } } 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 4266ab9037..ba8c433aa7 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 @@ -17,6 +17,7 @@ import com.onesignal.user.internal.builduser.IRebuildUserService import com.onesignal.user.internal.identity.IdentityModel import com.onesignal.user.internal.identity.IdentityModelStore import com.onesignal.user.internal.operations.RefreshUserOperation +import com.onesignal.user.internal.operations.impl.states.NewRecordsState import com.onesignal.user.internal.properties.PropertiesModel import com.onesignal.user.internal.properties.PropertiesModelStore import com.onesignal.user.internal.subscriptions.SubscriptionModel @@ -31,6 +32,7 @@ internal class RefreshUserOperationExecutor( private val _subscriptionsModelStore: SubscriptionModelStore, private val _configModelStore: ConfigModelStore, private val _buildUserService: IRebuildUserService, + private val _newRecordState: NewRecordsState, ) : IOperationExecutor { override val operations: List get() = listOf(REFRESH_USER) @@ -135,6 +137,9 @@ internal class RefreshUserOperationExecutor( NetworkUtils.ResponseStatusType.UNAUTHORIZED -> ExecutionResponse(ExecutionResult.FAIL_UNAUTHORIZED) NetworkUtils.ResponseStatusType.MISSING -> { + if (_newRecordState.isInMissingRetryWindow(op.onesignalId)) { + return ExecutionResponse(ExecutionResult.FAIL_RETRY) + } val operations = _buildUserService.getRebuildOperationsIfCurrentUser(op.appId, op.onesignalId) if (operations == null) { return ExecutionResponse(ExecutionResult.FAIL_NORETRY) 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 a9cdf9fe97..b9261af744 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 @@ -26,6 +26,7 @@ import com.onesignal.user.internal.operations.CreateSubscriptionOperation import com.onesignal.user.internal.operations.DeleteSubscriptionOperation import com.onesignal.user.internal.operations.TransferSubscriptionOperation import com.onesignal.user.internal.operations.UpdateSubscriptionOperation +import com.onesignal.user.internal.operations.impl.states.NewRecordsState import com.onesignal.user.internal.subscriptions.SubscriptionModel import com.onesignal.user.internal.subscriptions.SubscriptionModelStore import com.onesignal.user.internal.subscriptions.SubscriptionType @@ -37,6 +38,7 @@ internal class SubscriptionOperationExecutor( private val _subscriptionModelStore: SubscriptionModelStore, private val _configModelStore: ConfigModelStore, private val _buildUserService: IRebuildUserService, + private val _newRecordState: NewRecordsState, ) : IOperationExecutor { override val operations: List get() = listOf(CREATE_SUBSCRIPTION, UPDATE_SUBSCRIPTION, DELETE_SUBSCRIPTION, TRANSFER_SUBSCRIPTION) @@ -136,6 +138,9 @@ internal class SubscriptionOperationExecutor( NetworkUtils.ResponseStatusType.UNAUTHORIZED -> ExecutionResponse(ExecutionResult.FAIL_UNAUTHORIZED) NetworkUtils.ResponseStatusType.MISSING -> { + if (_newRecordState.isInMissingRetryWindow(createOperation.onesignalId)) { + return ExecutionResponse(ExecutionResult.FAIL_RETRY) + } val operations = _buildUserService.getRebuildOperationsIfCurrentUser(createOperation.appId, createOperation.onesignalId) if (operations == null) { return ExecutionResponse(ExecutionResult.FAIL_NORETRY) @@ -177,7 +182,14 @@ internal class SubscriptionOperationExecutor( return when (responseType) { NetworkUtils.ResponseStatusType.RETRYABLE -> ExecutionResponse(ExecutionResult.FAIL_RETRY) - NetworkUtils.ResponseStatusType.MISSING -> + NetworkUtils.ResponseStatusType.MISSING -> { + if (listOf( + lastOperation.onesignalId, + lastOperation.subscriptionId, + ).any { _newRecordState.isInMissingRetryWindow(it) } + ) { + return ExecutionResponse(ExecutionResult.FAIL_RETRY) + } // toss this, but create an identical CreateSubscriptionOperation to re-create the subscription being updated. ExecutionResponse( ExecutionResult.FAIL_NORETRY, @@ -194,6 +206,7 @@ internal class SubscriptionOperationExecutor( ), ), ) + } else -> ExecutionResponse(ExecutionResult.FAIL_NORETRY) } @@ -248,9 +261,14 @@ internal class SubscriptionOperationExecutor( val responseType = NetworkUtils.getResponseStatusType(ex.statusCode) return when (responseType) { - NetworkUtils.ResponseStatusType.MISSING -> - // if the subscription is missing, we are good! - ExecutionResponse(ExecutionResult.SUCCESS) + NetworkUtils.ResponseStatusType.MISSING -> { + if (listOf(op.onesignalId, op.subscriptionId).any { _newRecordState.isInMissingRetryWindow(it) }) { + ExecutionResponse(ExecutionResult.FAIL_RETRY) + } else { + // if the subscription is missing, we are good! + ExecutionResponse(ExecutionResult.SUCCESS) + } + } NetworkUtils.ResponseStatusType.RETRYABLE -> ExecutionResponse(ExecutionResult.FAIL_RETRY) else -> 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 1002ee1f0f..a6419612bf 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 @@ -22,6 +22,7 @@ import com.onesignal.user.internal.operations.SetTagOperation import com.onesignal.user.internal.operations.TrackPurchaseOperation import com.onesignal.user.internal.operations.TrackSessionEndOperation import com.onesignal.user.internal.operations.TrackSessionStartOperation +import com.onesignal.user.internal.operations.impl.states.NewRecordsState import com.onesignal.user.internal.properties.PropertiesModelStore internal class UpdateUserOperationExecutor( @@ -29,6 +30,7 @@ internal class UpdateUserOperationExecutor( private val _identityModelStore: IdentityModelStore, private val _propertiesModelStore: PropertiesModelStore, private val _buildUserService: IRebuildUserService, + private val _newRecordState: NewRecordsState, ) : IOperationExecutor { override val operations: List get() = listOf(SET_TAG, DELETE_TAG, SET_PROPERTY, TRACK_SESSION_START, TRACK_SESSION_END, TRACK_PURCHASE) @@ -163,6 +165,9 @@ internal class UpdateUserOperationExecutor( NetworkUtils.ResponseStatusType.UNAUTHORIZED -> ExecutionResponse(ExecutionResult.FAIL_UNAUTHORIZED) NetworkUtils.ResponseStatusType.MISSING -> { + if (_newRecordState.isInMissingRetryWindow(onesignalId)) { + return ExecutionResponse(ExecutionResult.FAIL_RETRY) + } val operations = _buildUserService.getRebuildOperationsIfCurrentUser(appId, onesignalId) if (operations == null) { return ExecutionResponse(ExecutionResult.FAIL_NORETRY) diff --git a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/user/internal/operations/impl/states/NewRecordsState.kt b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/user/internal/operations/impl/states/NewRecordsState.kt index fd3e2183a2..e884fc7f6d 100644 --- a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/user/internal/operations/impl/states/NewRecordsState.kt +++ b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/user/internal/operations/impl/states/NewRecordsState.kt @@ -6,7 +6,8 @@ import com.onesignal.core.internal.time.ITime /** * Purpose: Keeps track of ids that were just created on the backend. * This list gets used to delay network calls to ensure upcoming - * requests are ready to be accepted by the backend. + * requests are ready to be accepted by the backend. Also used for retries + * as a fallback if the server is under extra load. */ class NewRecordsState( private val _time: ITime, @@ -24,4 +25,9 @@ class NewRecordsState( val timeLastMovedOrCreated = records[key] ?: return true return _time.currentTimeMillis - timeLastMovedOrCreated > _configModelStore.model.opRepoPostCreateDelay } + + fun isInMissingRetryWindow(key: String): Boolean { + val timeLastMovedOrCreated = records[key] ?: return false + return _time.currentTimeMillis - timeLastMovedOrCreated <= _configModelStore.model.opRepoPostCreateRetryUpTo + } } diff --git a/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/user/internal/operations/IdentityOperationExecutorTests.kt b/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/user/internal/operations/IdentityOperationExecutorTests.kt index 4721945a8c..0a9e53603f 100644 --- a/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/user/internal/operations/IdentityOperationExecutorTests.kt +++ b/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/user/internal/operations/IdentityOperationExecutorTests.kt @@ -10,6 +10,7 @@ import com.onesignal.user.internal.backend.IdentityConstants import com.onesignal.user.internal.builduser.IRebuildUserService import com.onesignal.user.internal.identity.IdentityModel import com.onesignal.user.internal.identity.IdentityModelStore +import com.onesignal.user.internal.operations.ExecutorMocks.Companion.getNewRecordState import com.onesignal.user.internal.operations.impl.executors.IdentityOperationExecutor import io.kotest.core.spec.style.FunSpec import io.kotest.matchers.shouldBe @@ -37,7 +38,8 @@ class IdentityOperationExecutorTests : FunSpec({ val mockBuildUserService = mockk() - val identityOperationExecutor = IdentityOperationExecutor(mockIdentityBackendService, mockIdentityModelStore, mockBuildUserService) + val identityOperationExecutor = + IdentityOperationExecutor(mockIdentityBackendService, mockIdentityModelStore, mockBuildUserService, getNewRecordState()) val operations = listOf(SetAliasOperation("appId", "onesignalId", "aliasKey1", "aliasValue1")) // When @@ -59,7 +61,8 @@ class IdentityOperationExecutorTests : FunSpec({ val mockIdentityModelStore = MockHelper.identityModelStore() val mockBuildUserService = mockk() - val identityOperationExecutor = IdentityOperationExecutor(mockIdentityBackendService, mockIdentityModelStore, mockBuildUserService) + val identityOperationExecutor = + IdentityOperationExecutor(mockIdentityBackendService, mockIdentityModelStore, mockBuildUserService, getNewRecordState()) val operations = listOf(SetAliasOperation("appId", "onesignalId", "aliasKey1", "aliasValue1")) // When @@ -78,7 +81,8 @@ class IdentityOperationExecutorTests : FunSpec({ val mockIdentityModelStore = MockHelper.identityModelStore() val mockBuildUserService = mockk() - val identityOperationExecutor = IdentityOperationExecutor(mockIdentityBackendService, mockIdentityModelStore, mockBuildUserService) + val identityOperationExecutor = + IdentityOperationExecutor(mockIdentityBackendService, mockIdentityModelStore, mockBuildUserService, getNewRecordState()) val operations = listOf(SetAliasOperation("appId", "onesignalId", "aliasKey1", "aliasValue1")) // When @@ -89,6 +93,50 @@ class IdentityOperationExecutorTests : FunSpec({ response.result shouldBe ExecutionResult.FAIL_NORETRY } + test("execution of set alias operation with MISSING error") { + // Given + val mockIdentityBackendService = mockk() + coEvery { mockIdentityBackendService.setAlias(any(), any(), any(), any()) } throws BackendException(404) + + val mockIdentityModelStore = MockHelper.identityModelStore() + val mockBuildUserService = mockk() + every { mockBuildUserService.getRebuildOperationsIfCurrentUser(any(), any()) } returns null + + val identityOperationExecutor = + IdentityOperationExecutor(mockIdentityBackendService, mockIdentityModelStore, mockBuildUserService, getNewRecordState()) + val operations = listOf(SetAliasOperation("appId", "onesignalId", "aliasKey1", "aliasValue1")) + + // When + + val response = identityOperationExecutor.execute(operations) + + // Then + response.result shouldBe ExecutionResult.FAIL_NORETRY + } + + test("execution of set alias operation with MISSING error, but isInMissingRetryWindow") { + // Given + val mockIdentityBackendService = mockk() + coEvery { mockIdentityBackendService.setAlias(any(), any(), any(), any()) } throws BackendException(404) + + val mockIdentityModelStore = MockHelper.identityModelStore() + val mockBuildUserService = mockk() + every { mockBuildUserService.getRebuildOperationsIfCurrentUser(any(), any()) } returns null + + val mockConfigModelStore = MockHelper.configModelStore().also { it.model.opRepoPostCreateRetryUpTo = 1_000 } + val newRecordState = getNewRecordState(mockConfigModelStore).also { it.add("onesignalId") } + val identityOperationExecutor = + IdentityOperationExecutor(mockIdentityBackendService, mockIdentityModelStore, mockBuildUserService, newRecordState) + val operations = listOf(SetAliasOperation("appId", "onesignalId", "aliasKey1", "aliasValue1")) + + // When + + val response = identityOperationExecutor.execute(operations) + + // Then + response.result shouldBe ExecutionResult.FAIL_RETRY + } + test("execution of delete alias operation") { // Given val mockIdentityBackendService = mockk() @@ -103,7 +151,8 @@ class IdentityOperationExecutorTests : FunSpec({ val mockBuildUserService = mockk() - val identityOperationExecutor = IdentityOperationExecutor(mockIdentityBackendService, mockIdentityModelStore, mockBuildUserService) + val identityOperationExecutor = + IdentityOperationExecutor(mockIdentityBackendService, mockIdentityModelStore, mockBuildUserService, getNewRecordState()) val operations = listOf(DeleteAliasOperation("appId", "onesignalId", "aliasKey1")) // When @@ -125,7 +174,8 @@ class IdentityOperationExecutorTests : FunSpec({ val mockIdentityModelStore = MockHelper.identityModelStore() val mockBuildUserService = mockk() - val identityOperationExecutor = IdentityOperationExecutor(mockIdentityBackendService, mockIdentityModelStore, mockBuildUserService) + val identityOperationExecutor = + IdentityOperationExecutor(mockIdentityBackendService, mockIdentityModelStore, mockBuildUserService, getNewRecordState()) val operations = listOf(DeleteAliasOperation("appId", "onesignalId", "aliasKey1")) // When @@ -144,7 +194,8 @@ class IdentityOperationExecutorTests : FunSpec({ val mockIdentityModelStore = MockHelper.identityModelStore() val mockBuildUserService = mockk() - val identityOperationExecutor = IdentityOperationExecutor(mockIdentityBackendService, mockIdentityModelStore, mockBuildUserService) + val identityOperationExecutor = + IdentityOperationExecutor(mockIdentityBackendService, mockIdentityModelStore, mockBuildUserService, getNewRecordState()) val operations = listOf(DeleteAliasOperation("appId", "onesignalId", "aliasKey1")) // When @@ -154,4 +205,50 @@ class IdentityOperationExecutorTests : FunSpec({ // Then response.result shouldBe ExecutionResult.FAIL_NORETRY } + + // If we get a 404 then either the Alias and/or the User doesn't exist, + // ether way that Alias doesn't exist on the User currently so consider it as successful + test("execution of delete alias operation with MISSING error") { + // Given + val mockIdentityBackendService = mockk() + coEvery { mockIdentityBackendService.deleteAlias(any(), any(), any(), any()) } throws BackendException(404) + + val mockIdentityModelStore = MockHelper.identityModelStore() + val mockBuildUserService = mockk() + + val identityOperationExecutor = + IdentityOperationExecutor(mockIdentityBackendService, mockIdentityModelStore, mockBuildUserService, getNewRecordState()) + val operations = listOf(DeleteAliasOperation("appId", "onesignalId", "aliasKey1")) + + // When + + val response = identityOperationExecutor.execute(operations) + + // Then + response.result shouldBe ExecutionResult.SUCCESS + } + + // The server may return a 404 incorrectly if we just created the User + // and it's replication is behind. We should retry if we are in the + // uncertainty window. + test("execution of delete alias operation with MISSING error, but isInMissingRetryWindow") { + // Given + val mockIdentityBackendService = mockk() + coEvery { mockIdentityBackendService.deleteAlias(any(), any(), any(), any()) } throws BackendException(404) + + val mockIdentityModelStore = MockHelper.identityModelStore() + val mockBuildUserService = mockk() + + val mockConfigModelStore = MockHelper.configModelStore().also { it.model.opRepoPostCreateRetryUpTo = 1_000 } + val newRecordState = getNewRecordState(mockConfigModelStore).also { it.add("onesignalId") } + val identityOperationExecutor = + IdentityOperationExecutor(mockIdentityBackendService, mockIdentityModelStore, mockBuildUserService, newRecordState) + val operations = listOf(DeleteAliasOperation("appId", "onesignalId", "aliasKey1")) + + // When + val response = identityOperationExecutor.execute(operations) + + // Then + response.result shouldBe ExecutionResult.FAIL_RETRY + } }) diff --git a/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/user/internal/operations/RefreshUserOperationExecutorTests.kt b/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/user/internal/operations/RefreshUserOperationExecutorTests.kt index 95ebac44e5..18681f4a10 100644 --- a/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/user/internal/operations/RefreshUserOperationExecutorTests.kt +++ b/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/user/internal/operations/RefreshUserOperationExecutorTests.kt @@ -13,6 +13,7 @@ import com.onesignal.user.internal.backend.SubscriptionObject import com.onesignal.user.internal.backend.SubscriptionObjectType import com.onesignal.user.internal.builduser.IRebuildUserService import com.onesignal.user.internal.identity.IdentityModel +import com.onesignal.user.internal.operations.ExecutorMocks.Companion.getNewRecordState import com.onesignal.user.internal.operations.impl.executors.RefreshUserOperationExecutor import com.onesignal.user.internal.properties.PropertiesModel import com.onesignal.user.internal.subscriptions.SubscriptionModelStore @@ -40,7 +41,11 @@ class RefreshUserOperationExecutorTests : FunSpec({ CreateUserResponse( mapOf(IdentityConstants.ONESIGNAL_ID to remoteOneSignalId, "aliasLabel1" to "aliasValue1"), PropertiesObject(country = "US"), - listOf(SubscriptionObject(existingSubscriptionId1, SubscriptionObjectType.ANDROID_PUSH, enabled = true, token = "pushToken1"), SubscriptionObject(remoteSubscriptionId1, SubscriptionObjectType.ANDROID_PUSH, enabled = true, token = "pushToken2"), SubscriptionObject(remoteSubscriptionId2, SubscriptionObjectType.EMAIL, token = "name@company.com")), + listOf( + SubscriptionObject(existingSubscriptionId1, SubscriptionObjectType.ANDROID_PUSH, enabled = true, token = "pushToken1"), + SubscriptionObject(remoteSubscriptionId1, SubscriptionObjectType.ANDROID_PUSH, enabled = true, token = "pushToken2"), + SubscriptionObject(remoteSubscriptionId2, SubscriptionObjectType.EMAIL, token = "name@company.com"), + ), ) // Given @@ -76,6 +81,7 @@ class RefreshUserOperationExecutorTests : FunSpec({ mockSubscriptionsModelStore, mockConfigModelStore, mockBuildUserService, + getNewRecordState(), ) val operations = listOf(RefreshUserOperation(appId, remoteOneSignalId)) @@ -150,6 +156,7 @@ class RefreshUserOperationExecutorTests : FunSpec({ mockSubscriptionsModelStore, MockHelper.configModelStore(), mockBuildUserService, + getNewRecordState(), ) val operations = listOf(RefreshUserOperation(appId, remoteOneSignalId)) @@ -186,6 +193,7 @@ class RefreshUserOperationExecutorTests : FunSpec({ mockSubscriptionsModelStore, MockHelper.configModelStore(), mockBuildUserService, + getNewRecordState(), ) val operations = listOf(RefreshUserOperation(appId, remoteOneSignalId)) @@ -219,6 +227,7 @@ class RefreshUserOperationExecutorTests : FunSpec({ mockSubscriptionsModelStore, MockHelper.configModelStore(), mockBuildUserService, + getNewRecordState(), ) val operations = listOf(RefreshUserOperation(appId, remoteOneSignalId)) @@ -232,4 +241,76 @@ class RefreshUserOperationExecutorTests : FunSpec({ mockUserBackendService.getUser(appId, IdentityConstants.ONESIGNAL_ID, remoteOneSignalId) } } + + test("refresh user fails without retry when backend returns MISSING") { + // Given + val mockUserBackendService = mockk() + coEvery { mockUserBackendService.getUser(appId, IdentityConstants.ONESIGNAL_ID, remoteOneSignalId) } throws BackendException(404) + + // Given + val mockIdentityModelStore = MockHelper.identityModelStore() + val mockPropertiesModelStore = MockHelper.propertiesModelStore() + val mockSubscriptionsModelStore = mockk() + val mockBuildUserService = mockk() + every { mockBuildUserService.getRebuildOperationsIfCurrentUser(any(), any()) } returns null + + val loginUserOperationExecutor = + RefreshUserOperationExecutor( + mockUserBackendService, + mockIdentityModelStore, + mockPropertiesModelStore, + mockSubscriptionsModelStore, + MockHelper.configModelStore(), + mockBuildUserService, + getNewRecordState(), + ) + + val operations = listOf(RefreshUserOperation(appId, remoteOneSignalId)) + + // When + val response = loginUserOperationExecutor.execute(operations) + + // Then + response.result shouldBe ExecutionResult.FAIL_NORETRY + coVerify(exactly = 1) { + mockUserBackendService.getUser(appId, IdentityConstants.ONESIGNAL_ID, remoteOneSignalId) + } + } + + test("refresh user is retried when backend returns MISSING, but isInMissingRetryWindow") { + // Given + val mockUserBackendService = mockk() + coEvery { mockUserBackendService.getUser(appId, IdentityConstants.ONESIGNAL_ID, remoteOneSignalId) } throws BackendException(404) + + // Given + val mockIdentityModelStore = MockHelper.identityModelStore() + val mockPropertiesModelStore = MockHelper.propertiesModelStore() + val mockSubscriptionsModelStore = mockk() + val mockBuildUserService = mockk() + + val mockConfigModelStore = MockHelper.configModelStore().also { it.model.opRepoPostCreateRetryUpTo = 1_000 } + val newRecordState = getNewRecordState(mockConfigModelStore).also { it.add(remoteOneSignalId) } + + val loginUserOperationExecutor = + RefreshUserOperationExecutor( + mockUserBackendService, + mockIdentityModelStore, + mockPropertiesModelStore, + mockSubscriptionsModelStore, + MockHelper.configModelStore(), + mockBuildUserService, + newRecordState, + ) + + val operations = listOf(RefreshUserOperation(appId, remoteOneSignalId)) + + // When + val response = loginUserOperationExecutor.execute(operations) + + // Then + response.result shouldBe ExecutionResult.FAIL_RETRY + coVerify(exactly = 1) { + mockUserBackendService.getUser(appId, IdentityConstants.ONESIGNAL_ID, remoteOneSignalId) + } + } }) 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 471e69f6fb..2d4f044ff1 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 @@ -10,6 +10,7 @@ import com.onesignal.user.internal.backend.ISubscriptionBackendService import com.onesignal.user.internal.backend.IdentityConstants import com.onesignal.user.internal.backend.SubscriptionObjectType import com.onesignal.user.internal.builduser.IRebuildUserService +import com.onesignal.user.internal.operations.ExecutorMocks.Companion.getNewRecordState import com.onesignal.user.internal.operations.impl.executors.SubscriptionOperationExecutor import com.onesignal.user.internal.subscriptions.SubscriptionModel import com.onesignal.user.internal.subscriptions.SubscriptionModelStore @@ -52,6 +53,7 @@ class SubscriptionOperationExecutorTests : FunSpec({ mockSubscriptionsModelStore, MockHelper.configModelStore(), mockBuildUserService, + getNewRecordState(), ) val operations = @@ -104,6 +106,7 @@ class SubscriptionOperationExecutorTests : FunSpec({ mockSubscriptionsModelStore, MockHelper.configModelStore(), mockBuildUserService, + getNewRecordState(), ) val operations = @@ -156,6 +159,7 @@ class SubscriptionOperationExecutorTests : FunSpec({ mockSubscriptionsModelStore, MockHelper.configModelStore(), mockBuildUserService, + getNewRecordState(), ) val operations = @@ -191,6 +195,47 @@ class SubscriptionOperationExecutorTests : FunSpec({ } } + test("create subscription fails with retry when the backend returns MISSING, when isInMissingRetryWindow") { + // Given + val mockSubscriptionBackendService = mockk() + coEvery { mockSubscriptionBackendService.createSubscription(any(), any(), any(), any()) } throws BackendException(404) + + val mockSubscriptionsModelStore = mockk() + val mockBuildUserService = mockk() + val mockConfigModelStore = MockHelper.configModelStore().also { it.model.opRepoPostCreateRetryUpTo = 1_000 } + val newRecordState = getNewRecordState(mockConfigModelStore).also { it.add(remoteOneSignalId) } + + val subscriptionOperationExecutor = + SubscriptionOperationExecutor( + mockSubscriptionBackendService, + MockHelper.deviceService(), + AndroidMockHelper.applicationService(), + mockSubscriptionsModelStore, + MockHelper.configModelStore(), + mockBuildUserService, + newRecordState, + ) + + val operations = + listOf( + CreateSubscriptionOperation( + appId, + remoteOneSignalId, + localSubscriptionId, + SubscriptionType.PUSH, + true, + "pushToken", + SubscriptionStatus.SUBSCRIBED, + ), + ) + + // When + val response = subscriptionOperationExecutor.execute(operations) + + // Then + response.result shouldBe ExecutionResult.FAIL_RETRY + } + test("create subscription then delete subscription is a successful no-op") { // Given val mockSubscriptionBackendService = mockk() @@ -210,6 +255,7 @@ class SubscriptionOperationExecutorTests : FunSpec({ mockSubscriptionsModelStore, MockHelper.configModelStore(), mockBuildUserService, + getNewRecordState(), ) val operations = @@ -253,6 +299,7 @@ class SubscriptionOperationExecutorTests : FunSpec({ mockSubscriptionsModelStore, MockHelper.configModelStore(), mockBuildUserService, + getNewRecordState(), ) val operations = @@ -319,6 +366,7 @@ class SubscriptionOperationExecutorTests : FunSpec({ mockSubscriptionsModelStore, MockHelper.configModelStore(), mockBuildUserService, + getNewRecordState(), ) val operations = @@ -378,6 +426,7 @@ class SubscriptionOperationExecutorTests : FunSpec({ mockSubscriptionsModelStore, MockHelper.configModelStore(), mockBuildUserService, + getNewRecordState(), ) val operations = @@ -428,6 +477,7 @@ class SubscriptionOperationExecutorTests : FunSpec({ mockSubscriptionsModelStore, MockHelper.configModelStore(), mockBuildUserService, + getNewRecordState(), ) val operations = @@ -462,6 +512,47 @@ class SubscriptionOperationExecutorTests : FunSpec({ } } + test("update subscription fails with retry when the backend returns MISSING, when isInMissingRetryWindow") { + // Given + val mockSubscriptionBackendService = mockk() + coEvery { mockSubscriptionBackendService.updateSubscription(any(), any(), any()) } throws BackendException(404) + + val mockSubscriptionsModelStore = mockk() + val mockBuildUserService = mockk() + val mockConfigModelStore = MockHelper.configModelStore().also { it.model.opRepoPostCreateRetryUpTo = 1_000 } + val newRecordState = getNewRecordState(mockConfigModelStore).also { it.add(remoteOneSignalId) } + + val subscriptionOperationExecutor = + SubscriptionOperationExecutor( + mockSubscriptionBackendService, + MockHelper.deviceService(), + AndroidMockHelper.applicationService(), + mockSubscriptionsModelStore, + MockHelper.configModelStore(), + mockBuildUserService, + newRecordState, + ) + + val operations = + listOf( + UpdateSubscriptionOperation( + appId, + remoteOneSignalId, + remoteSubscriptionId, + SubscriptionType.PUSH, + true, + "pushToken2", + SubscriptionStatus.SUBSCRIBED, + ), + ) + + // When + val response = subscriptionOperationExecutor.execute(operations) + + // Then + response.result shouldBe ExecutionResult.FAIL_RETRY + } + test("delete subscription successfully deletes subscription") { // Given val mockSubscriptionBackendService = mockk() @@ -480,6 +571,7 @@ class SubscriptionOperationExecutorTests : FunSpec({ mockSubscriptionsModelStore, MockHelper.configModelStore(), mockBuildUserService, + getNewRecordState(), ) val operations = @@ -512,6 +604,7 @@ class SubscriptionOperationExecutorTests : FunSpec({ mockSubscriptionsModelStore, MockHelper.configModelStore(), mockBuildUserService, + getNewRecordState(), ) val operations = @@ -545,6 +638,7 @@ class SubscriptionOperationExecutorTests : FunSpec({ mockSubscriptionsModelStore, MockHelper.configModelStore(), mockBuildUserService, + getNewRecordState(), ) val operations = @@ -559,4 +653,37 @@ class SubscriptionOperationExecutorTests : FunSpec({ response.result shouldBe ExecutionResult.SUCCESS coVerify(exactly = 1) { mockSubscriptionBackendService.deleteSubscription(appId, remoteSubscriptionId) } } + + test("delete subscription fails with retry when the backend returns MISSING, when isInMissingRetryWindow") { + // Given + val mockSubscriptionBackendService = mockk() + coEvery { mockSubscriptionBackendService.deleteSubscription(any(), any()) } throws BackendException(404) + + val mockSubscriptionsModelStore = mockk() + val mockBuildUserService = mockk() + val mockConfigModelStore = MockHelper.configModelStore().also { it.model.opRepoPostCreateRetryUpTo = 1_000 } + val newRecordState = getNewRecordState(mockConfigModelStore).also { it.add(remoteOneSignalId) } + + val subscriptionOperationExecutor = + SubscriptionOperationExecutor( + mockSubscriptionBackendService, + MockHelper.deviceService(), + AndroidMockHelper.applicationService(), + mockSubscriptionsModelStore, + MockHelper.configModelStore(), + mockBuildUserService, + newRecordState, + ) + + val operations = + listOf( + DeleteSubscriptionOperation(appId, remoteOneSignalId, remoteSubscriptionId), + ) + + // When + val response = subscriptionOperationExecutor.execute(operations) + + // Then + response.result shouldBe ExecutionResult.FAIL_RETRY + } }) diff --git a/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/user/internal/operations/UpdateUserOperationExecutorTests.kt b/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/user/internal/operations/UpdateUserOperationExecutorTests.kt index bad53be5d8..b2f916bf04 100644 --- a/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/user/internal/operations/UpdateUserOperationExecutorTests.kt +++ b/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/user/internal/operations/UpdateUserOperationExecutorTests.kt @@ -1,11 +1,13 @@ package com.onesignal.user.internal.operations +import com.onesignal.common.exceptions.BackendException import com.onesignal.core.internal.operations.ExecutionResult import com.onesignal.core.internal.operations.Operation import com.onesignal.mocks.MockHelper import com.onesignal.user.internal.backend.IUserBackendService import com.onesignal.user.internal.backend.IdentityConstants import com.onesignal.user.internal.builduser.IRebuildUserService +import com.onesignal.user.internal.operations.ExecutorMocks.Companion.getNewRecordState import com.onesignal.user.internal.operations.impl.executors.UpdateUserOperationExecutor import com.onesignal.user.internal.properties.PropertiesModel import io.kotest.core.spec.style.FunSpec @@ -13,6 +15,7 @@ import io.kotest.matchers.shouldBe import io.kotest.matchers.shouldNotBe import io.mockk.coEvery import io.mockk.coVerify +import io.mockk.every import io.mockk.just import io.mockk.mockk import io.mockk.runs @@ -39,6 +42,7 @@ class UpdateUserOperationExecutorTests : FunSpec({ mockIdentityModelStore, mockPropertiesModelStore, mockBuildUserService, + getNewRecordState(), ) val operations = listOf(SetTagOperation(appId, remoteOneSignalId, "tagKey1", "tagValue1")) @@ -77,6 +81,7 @@ class UpdateUserOperationExecutorTests : FunSpec({ mockIdentityModelStore, mockPropertiesModelStore, mockBuildUserService, + getNewRecordState(), ) val operations = listOf( @@ -137,6 +142,7 @@ class UpdateUserOperationExecutorTests : FunSpec({ mockIdentityModelStore, mockPropertiesModelStore, mockBuildUserService, + getNewRecordState(), ) val operations = listOf( @@ -180,6 +186,7 @@ class UpdateUserOperationExecutorTests : FunSpec({ mockIdentityModelStore, mockPropertiesModelStore, mockBuildUserService, + getNewRecordState(), ) val operations = listOf( @@ -243,6 +250,7 @@ class UpdateUserOperationExecutorTests : FunSpec({ mockIdentityModelStore, mockPropertiesModelStore, mockBuildUserService, + getNewRecordState(), ) val operations = listOf( @@ -271,4 +279,63 @@ class UpdateUserOperationExecutorTests : FunSpec({ ) } } + + test("update user single operation fails with MISSING") { + // Given + val mockUserBackendService = mockk() + coEvery { mockUserBackendService.updateUser(any(), any(), any(), any(), any(), any()) } throws BackendException(404) + + // Given + val mockIdentityModelStore = MockHelper.identityModelStore() + val mockPropertiesModelStore = MockHelper.propertiesModelStore() + val mockBuildUserService = mockk() + every { mockBuildUserService.getRebuildOperationsIfCurrentUser(any(), any()) } returns null + + val loginUserOperationExecutor = + UpdateUserOperationExecutor( + mockUserBackendService, + mockIdentityModelStore, + mockPropertiesModelStore, + mockBuildUserService, + getNewRecordState(), + ) + val operations = listOf(SetTagOperation(appId, remoteOneSignalId, "tagKey1", "tagValue1")) + + // When + val response = loginUserOperationExecutor.execute(operations) + + // Then + response.result shouldBe ExecutionResult.FAIL_NORETRY + } + + test("update user single operation fails with MISSING, but isInMissingRetryWindow") { + // Given + val mockUserBackendService = mockk() + coEvery { mockUserBackendService.updateUser(any(), any(), any(), any(), any(), any()) } throws BackendException(404) + + // Given + val mockIdentityModelStore = MockHelper.identityModelStore() + val mockPropertiesModelStore = MockHelper.propertiesModelStore() + val mockBuildUserService = mockk() + every { mockBuildUserService.getRebuildOperationsIfCurrentUser(any(), any()) } returns null + + val mockConfigModelStore = MockHelper.configModelStore().also { it.model.opRepoPostCreateRetryUpTo = 1_000 } + val newRecordState = getNewRecordState(mockConfigModelStore).also { it.add(remoteOneSignalId) } + + val loginUserOperationExecutor = + UpdateUserOperationExecutor( + mockUserBackendService, + mockIdentityModelStore, + mockPropertiesModelStore, + mockBuildUserService, + newRecordState, + ) + val operations = listOf(SetTagOperation(appId, remoteOneSignalId, "tagKey1", "tagValue1")) + + // When + val response = loginUserOperationExecutor.execute(operations) + + // Then + response.result shouldBe ExecutionResult.FAIL_RETRY + } }) diff --git a/OneSignalSDK/onesignal/testhelpers/src/main/java/com/onesignal/mocks/MockHelper.kt b/OneSignalSDK/onesignal/testhelpers/src/main/java/com/onesignal/mocks/MockHelper.kt index bf6bde7879..a29345a5f8 100644 --- a/OneSignalSDK/onesignal/testhelpers/src/main/java/com/onesignal/mocks/MockHelper.kt +++ b/OneSignalSDK/onesignal/testhelpers/src/main/java/com/onesignal/mocks/MockHelper.kt @@ -47,6 +47,7 @@ object MockHelper { configModel.opRepoExecutionInterval = 1 configModel.opRepoPostWakeDelay = 1 configModel.opRepoPostCreateDelay = 1 + configModel.opRepoPostCreateRetryUpTo = 1 configModel.appId = DEFAULT_APP_ID From fb107585ee1562113dfb28fff6bc4304cac6ff92 Mon Sep 17 00:00:00 2001 From: Josh Kasten Date: Wed, 17 Apr 2024 21:57:02 -0400 Subject: [PATCH 4/6] correct applyToRecordId for login operation We need to check the existingOnesignalId first as most of the time the login executor tries to do an operation to the User, add Alias, first instead of creating a new User. This makes sure our opRepoPostCreateDelay applies. --- .../onesignal/user/internal/operations/LoginUserOperation.kt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/user/internal/operations/LoginUserOperation.kt b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/user/internal/operations/LoginUserOperation.kt index e03390537d..b283cc3da0 100644 --- a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/user/internal/operations/LoginUserOperation.kt +++ b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/user/internal/operations/LoginUserOperation.kt @@ -56,7 +56,7 @@ class LoginUserOperation() : Operation(LoginUserOperationExecutor.LOGIN_USER) { override val modifyComparisonKey: String = "" override val groupComparisonType: GroupComparisonType = GroupComparisonType.CREATE override val canStartExecute: Boolean get() = existingOnesignalId == null || !IDManager.isLocalId(existingOnesignalId!!) - override val applyToRecordId: String get() = onesignalId + override val applyToRecordId: String get() = existingOnesignalId ?: onesignalId constructor(appId: String, onesignalId: String, externalId: String?, existingOneSignalId: String? = null) : this() { this.appId = appId From 1c2bf827f7b550336bc8a5cb1f7b2ce6f5a086a2 Mon Sep 17 00:00:00 2001 From: Josh Kasten Date: Wed, 17 Apr 2024 22:05:12 -0400 Subject: [PATCH 5/6] prevent adding executionInterval & postCreateDelay After we wait for the opRepoPostCreateDelay time call to waiter.wake() would cause waitForNewOperationAndExecutionInterval() to wake up as we expect, however it was then waiting for the full time on opRepoExecutionInterval as well. To solve this we simply subtract the time we already waited from opRepoExecutionInterval. With the current values this means we fully skip opRepoExecutionInterval so we only wait opRepoPostCreateDelay. --- .../internal/operations/impl/OperationRepo.kt | 27 ++++++++++--------- 1 file changed, 14 insertions(+), 13 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 222035560d..70c09ce50f 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 @@ -38,9 +38,14 @@ internal class OperationRepo( } } + internal class LoopWaiterMessage( + val force: Boolean, + val previousWaitedTime: Long = 0, + ) + private val executorsMap: Map private val queue = mutableListOf() - private val waiter = WaiterWithValue() + private val waiter = WaiterWithValue() private var paused = false private var coroutineScope = CoroutineScope(newSingleThreadContext(name = "OpRepo")) @@ -124,7 +129,7 @@ internal class OperationRepo( } } - waiter.wake(flush) + waiter.wake(LoopWaiterMessage(flush, 0)) } /** @@ -161,16 +166,16 @@ internal class OperationRepo( */ private suspend fun waitForNewOperationAndExecutionInterval() { // 1. Wait for an operation to be enqueued - var force = waiter.waitForWake() + var wakeMessage = waiter.waitForWake() // 2. Wait at least the time defined in opRepoExecutionInterval // so operations can be grouped, unless one of them used // flush=true (AKA force) var lastTime = _time.currentTimeMillis - var remainingTime = _configModelStore.model.opRepoExecutionInterval - while (!force && remainingTime > 0) { + var remainingTime = _configModelStore.model.opRepoExecutionInterval - wakeMessage.previousWaitedTime + while (!wakeMessage.force && remainingTime > 0) { withTimeoutOrNull(remainingTime) { - force = waiter.waitForWake() + wakeMessage = waiter.waitForWake() } remainingTime -= _time.currentTimeMillis - lastTime lastTime = _time.currentTimeMillis @@ -198,14 +203,10 @@ internal class OperationRepo( } response.idTranslations.values.forEach { _newRecordState.add(it) } coroutineScope.launch { - delay(_configModelStore.model.opRepoPostCreateDelay) + val waitTime = _configModelStore.model.opRepoPostCreateDelay + delay(waitTime) synchronized(queue) { - // NOTE: Even if the queue is not empty we may wake - // when not needed, as those operations may not have - // depended on these ids. This however should be very - // rare and the side-effect is only a bit less - // batching. - if (queue.isNotEmpty()) waiter.wake(false) + if (queue.isNotEmpty()) waiter.wake(LoopWaiterMessage(false, waitTime)) } } } From a9dfc0933993563fe40d860bea744aef630c82cf Mon Sep 17 00:00:00 2001 From: Josh Kasten Date: Fri, 19 Apr 2024 17:18:07 -0400 Subject: [PATCH 6/6] only consider retrying on 404, don't include 410 If the backend returns 410 then it indicates it knows it existed at one point and it is deleted now. Where a 404 is more abstract. --- .../impl/executors/IdentityOperationExecutor.kt | 4 ++-- .../impl/executors/RefreshUserOperationExecutor.kt | 2 +- .../impl/executors/SubscriptionOperationExecutor.kt | 12 +++++++++--- .../impl/executors/UpdateUserOperationExecutor.kt | 2 +- 4 files changed, 13 insertions(+), 7 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 88a98f3586..fa55a55757 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 @@ -69,7 +69,7 @@ internal class IdentityOperationExecutor( NetworkUtils.ResponseStatusType.UNAUTHORIZED -> ExecutionResponse(ExecutionResult.FAIL_UNAUTHORIZED) NetworkUtils.ResponseStatusType.MISSING -> { - if (_newRecordState.isInMissingRetryWindow(lastOperation.onesignalId)) { + if (ex.statusCode == 404 && _newRecordState.isInMissingRetryWindow(lastOperation.onesignalId)) { return ExecutionResponse(ExecutionResult.FAIL_RETRY) } @@ -109,7 +109,7 @@ internal class IdentityOperationExecutor( NetworkUtils.ResponseStatusType.UNAUTHORIZED -> ExecutionResponse(ExecutionResult.FAIL_UNAUTHORIZED) NetworkUtils.ResponseStatusType.MISSING -> { - return if (_newRecordState.isInMissingRetryWindow(lastOperation.onesignalId)) { + return if (ex.statusCode == 404 && _newRecordState.isInMissingRetryWindow(lastOperation.onesignalId)) { ExecutionResponse(ExecutionResult.FAIL_RETRY) } else { // This means either the User or the Alias was already 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 ba8c433aa7..36e1650dc9 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 @@ -137,7 +137,7 @@ internal class RefreshUserOperationExecutor( NetworkUtils.ResponseStatusType.UNAUTHORIZED -> ExecutionResponse(ExecutionResult.FAIL_UNAUTHORIZED) NetworkUtils.ResponseStatusType.MISSING -> { - if (_newRecordState.isInMissingRetryWindow(op.onesignalId)) { + if (ex.statusCode == 404 && _newRecordState.isInMissingRetryWindow(op.onesignalId)) { return ExecutionResponse(ExecutionResult.FAIL_RETRY) } val operations = _buildUserService.getRebuildOperationsIfCurrentUser(op.appId, op.onesignalId) 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 b9261af744..08bfd48be0 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 @@ -138,7 +138,7 @@ internal class SubscriptionOperationExecutor( NetworkUtils.ResponseStatusType.UNAUTHORIZED -> ExecutionResponse(ExecutionResult.FAIL_UNAUTHORIZED) NetworkUtils.ResponseStatusType.MISSING -> { - if (_newRecordState.isInMissingRetryWindow(createOperation.onesignalId)) { + if (ex.statusCode == 404 && _newRecordState.isInMissingRetryWindow(createOperation.onesignalId)) { return ExecutionResponse(ExecutionResult.FAIL_RETRY) } val operations = _buildUserService.getRebuildOperationsIfCurrentUser(createOperation.appId, createOperation.onesignalId) @@ -183,7 +183,8 @@ internal class SubscriptionOperationExecutor( NetworkUtils.ResponseStatusType.RETRYABLE -> ExecutionResponse(ExecutionResult.FAIL_RETRY) NetworkUtils.ResponseStatusType.MISSING -> { - if (listOf( + if (ex.statusCode == 404 && + listOf( lastOperation.onesignalId, lastOperation.subscriptionId, ).any { _newRecordState.isInMissingRetryWindow(it) } @@ -262,7 +263,12 @@ internal class SubscriptionOperationExecutor( return when (responseType) { NetworkUtils.ResponseStatusType.MISSING -> { - if (listOf(op.onesignalId, op.subscriptionId).any { _newRecordState.isInMissingRetryWindow(it) }) { + if (ex.statusCode == 404 && + listOf( + op.onesignalId, + op.subscriptionId, + ).any { _newRecordState.isInMissingRetryWindow(it) } + ) { ExecutionResponse(ExecutionResult.FAIL_RETRY) } else { // if the subscription is missing, we are good! 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 a6419612bf..a512c8756f 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 @@ -165,7 +165,7 @@ internal class UpdateUserOperationExecutor( NetworkUtils.ResponseStatusType.UNAUTHORIZED -> ExecutionResponse(ExecutionResult.FAIL_UNAUTHORIZED) NetworkUtils.ResponseStatusType.MISSING -> { - if (_newRecordState.isInMissingRetryWindow(onesignalId)) { + if (ex.statusCode == 404 && _newRecordState.isInMissingRetryWindow(onesignalId)) { return ExecutionResponse(ExecutionResult.FAIL_RETRY) } val operations = _buildUserService.getRebuildOperationsIfCurrentUser(appId, onesignalId)