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 81ab0bb68..8e89006c5 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 @@ -54,7 +54,14 @@ internal class SubscriptionOperationExecutor( val startingOp = operations.first() return if (startingOp is CreateSubscriptionOperation) { - createSubscription(startingOp, operations) + // SDK-4388: if the subscription already exists on the backend (non-local id), + // POSTing to /subscriptions with that id is silently treated as a no-op and + // drops the merged enabled/status payload. Dispatch as an update instead. + if (!IDManager.isLocalId(startingOp.subscriptionId)) { + updateExistingSubscriptionFromCreate(startingOp, operations) + } else { + createSubscription(startingOp, operations) + } } else if (operations.any { it is DeleteSubscriptionOperation }) { if (operations.size > 1) { throw Exception("Only supports one operation! Attempted operations:\n$operations") @@ -73,6 +80,29 @@ internal class SubscriptionOperationExecutor( } } + private suspend fun updateExistingSubscriptionFromCreate( + createOperation: CreateSubscriptionOperation, + operations: List, + ): ExecutionResponse { + // if there are any deletes all operations should be tossed, nothing to do. + if (operations.any { it is DeleteSubscriptionOperation }) { + return ExecutionResponse(ExecutionResult.SUCCESS) + } + + val lastUpdateOperation = operations.lastOrNull { it is UpdateSubscriptionOperation } as UpdateSubscriptionOperation? + val patchOp = + UpdateSubscriptionOperation( + createOperation.appId, + createOperation.onesignalId, + createOperation.subscriptionId, + createOperation.type, + lastUpdateOperation?.enabled ?: createOperation.enabled, + lastUpdateOperation?.address ?: createOperation.address, + lastUpdateOperation?.status ?: createOperation.status, + ) + return updateSubscription(patchOp, listOf(patchOp)) + } + private suspend fun createSubscription( createOperation: CreateSubscriptionOperation, operations: List, @@ -88,12 +118,11 @@ internal class SubscriptionOperationExecutor( val enabled = lastUpdateOperation?.enabled ?: createOperation.enabled val address = lastUpdateOperation?.address ?: createOperation.address val status = lastUpdateOperation?.status ?: createOperation.status - val subId = if (!IDManager.isLocalId(createOperation.subscriptionId)) createOperation.subscriptionId else null try { val subscription = SubscriptionObject( - id = subId, + id = null, convert(createOperation.type), address, enabled, 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 4ae305324..11aa367b6 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 @@ -105,11 +105,14 @@ class SubscriptionOperationExecutorTests : } } - test("create subscription includes the subscription ID if non-local") { + // SDK-4388: A CreateSubscriptionOperation can carry a non-local subscriptionId when + // a subscription that already exists on the backend is re-attached to a new local user + // (e.g. createAndSwitchToNewUser after login). POSTing to /subscriptions with that id is + // silently a no-op on the server, so we must PATCH the existing subscription instead. + test("create subscription PATCHes existing subscription when subscription ID is non-local") { // Given val mockSubscriptionBackendService = mockk() - coEvery { mockSubscriptionBackendService.createSubscription(any(), any(), any(), any()) } returns - Pair(remoteSubscriptionId, rywData) + coEvery { mockSubscriptionBackendService.updateSubscription(any(), any(), any()) } returns rywData val mockSubscriptionsModelStore = mockk() val subscriptionModel = SubscriptionModel() @@ -144,16 +147,22 @@ class SubscriptionOperationExecutorTests : ) // When - subscriptionOperationExecutor.execute(operations) + val response = subscriptionOperationExecutor.execute(operations) // Then + response.result shouldBe ExecutionResult.SUCCESS + coVerify(exactly = 0) { + mockSubscriptionBackendService.createSubscription(any(), any(), any(), any()) + } coVerify(exactly = 1) { - mockSubscriptionBackendService.createSubscription( + mockSubscriptionBackendService.updateSubscription( appId, - IdentityConstants.ONESIGNAL_ID, - remoteOneSignalId, + remoteSubscriptionId, withArg { - it.id shouldBe remoteSubscriptionId + it.type shouldBe SubscriptionObjectType.ANDROID_PUSH + it.enabled shouldBe true + it.token shouldBe "pushToken" + it.notificationTypes shouldBe SubscriptionStatus.SUBSCRIBED.value }, ) } @@ -823,4 +832,274 @@ class SubscriptionOperationExecutorTests : mockConsistencyManager.setRywData(remoteOneSignalId, IamFetchRywTokenKey.SUBSCRIPTION, rywData) } } + + // Repro for SDK-4388: when a CreateSubscriptionOperation is grouped with a follow-up + // UpdateSubscriptionOperation for a subscription that already exists on the server + // (non-local subscriptionId), the executor must PATCH the merged final state instead + // of POSTing (which the backend treats as a no-op, silently dropping enabled/status). + test("create subscription with non-local id PATCHes instead of POSTing when grouped with update") { + // Given + val mockSubscriptionBackendService = mockk() + coEvery { mockSubscriptionBackendService.updateSubscription(any(), any(), any()) } returns rywData + + val mockSubscriptionsModelStore = mockk() + val subscriptionModel = SubscriptionModel().apply { id = remoteSubscriptionId } + every { mockSubscriptionsModelStore.get(remoteSubscriptionId) } returns subscriptionModel + + val mockBuildUserService = mockk() + + val subscriptionOperationExecutor = + SubscriptionOperationExecutor( + mockSubscriptionBackendService, + MockHelper.deviceService(), + AndroidMockHelper.applicationService(), + mockSubscriptionsModelStore, + MockHelper.configModelStore(), + mockBuildUserService, + getNewRecordState(), + mockConsistencyManager, + ) + + val operations = + listOf( + CreateSubscriptionOperation( + appId, + remoteOneSignalId, + remoteSubscriptionId, + SubscriptionType.PUSH, + false, + "pushToken", + SubscriptionStatus.NO_PERMISSION, + ), + UpdateSubscriptionOperation( + appId, + remoteOneSignalId, + remoteSubscriptionId, + SubscriptionType.PUSH, + true, + "pushToken", + SubscriptionStatus.SUBSCRIBED, + ), + ) + + // When + val response = subscriptionOperationExecutor.execute(operations) + + // Then + response.result shouldBe ExecutionResult.SUCCESS + coVerify(exactly = 0) { + mockSubscriptionBackendService.createSubscription(any(), any(), any(), any()) + } + coVerify(exactly = 1) { + mockSubscriptionBackendService.updateSubscription( + appId, + remoteSubscriptionId, + withArg { + it.type shouldBe SubscriptionObjectType.ANDROID_PUSH + it.enabled shouldBe true + it.token shouldBe "pushToken" + it.notificationTypes shouldBe SubscriptionStatus.SUBSCRIBED.value + }, + ) + } + } + + // SDK-4388: when a CreateSubscriptionOperation with a non-local id is grouped with a + // DeleteSubscriptionOperation, the executor must short-circuit to SUCCESS without + // making any backend calls. Creating-then-deleting an already-existing subscription is + // a no-op, and a stray PATCH here would resurrect a subscription the caller intended + // to remove. + test("create subscription with non-local id then delete is a successful no-op") { + // Given + val mockSubscriptionBackendService = mockk() + + val mockSubscriptionsModelStore = mockk() + val subscriptionModel = SubscriptionModel().apply { id = remoteSubscriptionId } + every { mockSubscriptionsModelStore.get(remoteSubscriptionId) } returns subscriptionModel + + val mockBuildUserService = mockk() + + val subscriptionOperationExecutor = + SubscriptionOperationExecutor( + mockSubscriptionBackendService, + MockHelper.deviceService(), + AndroidMockHelper.applicationService(), + mockSubscriptionsModelStore, + MockHelper.configModelStore(), + mockBuildUserService, + getNewRecordState(), + mockConsistencyManager, + ) + + val operations = + listOf( + CreateSubscriptionOperation( + appId, + remoteOneSignalId, + remoteSubscriptionId, + SubscriptionType.PUSH, + true, + "pushToken", + SubscriptionStatus.SUBSCRIBED, + ), + DeleteSubscriptionOperation(appId, remoteOneSignalId, remoteSubscriptionId), + ) + + // When + val response = subscriptionOperationExecutor.execute(operations) + + // Then + response.result shouldBe ExecutionResult.SUCCESS + coVerify(exactly = 0) { + mockSubscriptionBackendService.createSubscription(any(), any(), any(), any()) + } + coVerify(exactly = 0) { + mockSubscriptionBackendService.updateSubscription(any(), any(), any()) + } + coVerify(exactly = 0) { + mockSubscriptionBackendService.deleteSubscription(any(), any()) + } + } + + // SDK-4388: when the PATCH dispatched for a non-local-id Create fails with a retryable + // network error, the executor must propagate FAIL_RETRY (and any retryAfterSeconds) so + // OperationRepo can re-execute, rather than swallowing the error or routing back to a + // POST that the backend would silently no-op. + test("create subscription with non-local id fails with retry when there is a network condition") { + // Given + val mockSubscriptionBackendService = mockk() + coEvery { mockSubscriptionBackendService.updateSubscription(any(), any(), any()) } throws + BackendException(408, retryAfterSeconds = 10) + + val mockSubscriptionsModelStore = mockk() + val mockBuildUserService = mockk() + + val subscriptionOperationExecutor = + SubscriptionOperationExecutor( + mockSubscriptionBackendService, + MockHelper.deviceService(), + AndroidMockHelper.applicationService(), + mockSubscriptionsModelStore, + MockHelper.configModelStore(), + mockBuildUserService, + getNewRecordState(), + mockConsistencyManager, + ) + + val operations = + listOf( + CreateSubscriptionOperation( + appId, + remoteOneSignalId, + remoteSubscriptionId, + SubscriptionType.PUSH, + true, + "pushToken", + SubscriptionStatus.SUBSCRIBED, + ), + ) + + // When + val response = subscriptionOperationExecutor.execute(operations) + + // Then + response.result shouldBe ExecutionResult.FAIL_RETRY + response.retryAfterSeconds shouldBe 10 + coVerify(exactly = 0) { + mockSubscriptionBackendService.createSubscription(any(), any(), any(), any()) + } + coVerify(exactly = 1) { + mockSubscriptionBackendService.updateSubscription( + appId, + remoteSubscriptionId, + withArg { + it.type shouldBe SubscriptionObjectType.ANDROID_PUSH + it.enabled shouldBe true + it.token shouldBe "pushToken" + it.notificationTypes shouldBe SubscriptionStatus.SUBSCRIBED.value + }, + ) + } + } + + // SDK-4388: when multiple UpdateSubscriptionOperations are batched with a non-local-id + // Create, the executor must collapse them into a single PATCH carrying the *last* + // update's values (not the create's, not the first update's). Locks in last-wins + // semantics so a future "merge updates" refactor can't silently change behavior on + // this path and re-introduce stale state on the backend. + test("create subscription with non-local id and multiple updates uses the last update's values") { + // Given + val mockSubscriptionBackendService = mockk() + coEvery { mockSubscriptionBackendService.updateSubscription(any(), any(), any()) } returns rywData + + val mockSubscriptionsModelStore = mockk() + val subscriptionModel = SubscriptionModel().apply { id = remoteSubscriptionId } + every { mockSubscriptionsModelStore.get(remoteSubscriptionId) } returns subscriptionModel + + val mockBuildUserService = mockk() + + val subscriptionOperationExecutor = + SubscriptionOperationExecutor( + mockSubscriptionBackendService, + MockHelper.deviceService(), + AndroidMockHelper.applicationService(), + mockSubscriptionsModelStore, + MockHelper.configModelStore(), + mockBuildUserService, + getNewRecordState(), + mockConsistencyManager, + ) + + val operations = + listOf( + CreateSubscriptionOperation( + appId, + remoteOneSignalId, + remoteSubscriptionId, + SubscriptionType.PUSH, + false, + "pushTokenCreate", + SubscriptionStatus.NO_PERMISSION, + ), + UpdateSubscriptionOperation( + appId, + remoteOneSignalId, + remoteSubscriptionId, + SubscriptionType.PUSH, + false, + "pushTokenIntermediate", + SubscriptionStatus.NO_PERMISSION, + ), + UpdateSubscriptionOperation( + appId, + remoteOneSignalId, + remoteSubscriptionId, + SubscriptionType.PUSH, + true, + "pushTokenFinal", + SubscriptionStatus.SUBSCRIBED, + ), + ) + + // When + val response = subscriptionOperationExecutor.execute(operations) + + // Then + response.result shouldBe ExecutionResult.SUCCESS + coVerify(exactly = 0) { + mockSubscriptionBackendService.createSubscription(any(), any(), any(), any()) + } + coVerify(exactly = 1) { + mockSubscriptionBackendService.updateSubscription( + appId, + remoteSubscriptionId, + withArg { + it.type shouldBe SubscriptionObjectType.ANDROID_PUSH + it.enabled shouldBe true + it.token shouldBe "pushTokenFinal" + it.notificationTypes shouldBe SubscriptionStatus.SUBSCRIBED.value + }, + ) + } + } })