From 731623f274896ae378a280fdec0beca2fbe4a38e Mon Sep 17 00:00:00 2001 From: Fadi George Date: Wed, 22 Apr 2026 22:00:38 -0700 Subject: [PATCH 1/3] fix(subscriptions): patch existing subscription instead of post --- .../SubscriptionOperationExecutor.kt | 20 +++- .../SubscriptionOperationExecutorTests.kt | 96 +++++++++++++++++-- 2 files changed, 106 insertions(+), 10 deletions(-) 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 81ab0bb687..74af46d091 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 @@ -88,12 +88,28 @@ 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 + + // 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, dropping the merged + // enabled/status payload. Re-dispatch as a PATCH on the existing subscription instead. + if (!IDManager.isLocalId(createOperation.subscriptionId)) { + val patchOp = + UpdateSubscriptionOperation( + createOperation.appId, + createOperation.onesignalId, + createOperation.subscriptionId, + createOperation.type, + enabled, + address, + status, + ) + return updateSubscription(patchOp, listOf(patchOp)) + } 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 4ae3053247..4d00ebf53b 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,75 @@ 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 + }, + ) + } + } }) From 3da01c0a9ed791041652d5d71cd5032fa65212a2 Mon Sep 17 00:00:00 2001 From: Fadi George Date: Wed, 22 Apr 2026 22:11:04 -0700 Subject: [PATCH 2/3] refactor(subscriptions): move non-local id check earlier --- .../SubscriptionOperationExecutor.kt | 49 ++++++++++++------- 1 file changed, 31 insertions(+), 18 deletions(-) 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 74af46d091..8e89006c5e 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, @@ -89,23 +119,6 @@ internal class SubscriptionOperationExecutor( val address = lastUpdateOperation?.address ?: createOperation.address val status = lastUpdateOperation?.status ?: createOperation.status - // 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, dropping the merged - // enabled/status payload. Re-dispatch as a PATCH on the existing subscription instead. - if (!IDManager.isLocalId(createOperation.subscriptionId)) { - val patchOp = - UpdateSubscriptionOperation( - createOperation.appId, - createOperation.onesignalId, - createOperation.subscriptionId, - createOperation.type, - enabled, - address, - status, - ) - return updateSubscription(patchOp, listOf(patchOp)) - } - try { val subscription = SubscriptionObject( From 24a362355b13442ff867bdeb2d4811470fcd3263 Mon Sep 17 00:00:00 2001 From: AR Abdul Azeez Date: Mon, 27 Apr 2026 14:40:32 -0400 Subject: [PATCH 3/3] test(subscriptions): add coverage for non-local id Create paths Adds three tests covering branches in `updateExistingSubscriptionFromCreate` that the SDK-4388 fix introduced but didn't fully exercise: 1. Create + Delete with non-local id is a successful no-op (locks in the delete short-circuit so a future refactor can't accidentally route to PATCH and resurrect a deleted subscription). 2. Network-retryable backend errors on the PATCH path propagate as FAIL_RETRY with retryAfterSeconds (matches the local-id Create behavior and ensures errors aren't swallowed). 3. Multiple batched updates with a non-local-id Create collapse into a single PATCH carrying the *last* update's values (locks in last-wins semantics; mirrors the existing local-id POST test). These complement the two existing non-local-id tests and bring coverage for the new path to parity with the local-id Create path. Made-with: Cursor --- .../SubscriptionOperationExecutorTests.kt | 199 ++++++++++++++++++ 1 file changed, 199 insertions(+) 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 4d00ebf53b..11aa367b6b 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 @@ -903,4 +903,203 @@ class SubscriptionOperationExecutorTests : ) } } + + // 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 + }, + ) + } + } })