From 6e9b690484bc3dd39301ab700e44e3b8ffab4da3 Mon Sep 17 00:00:00 2001 From: Josh Kasten Date: Mon, 6 May 2024 20:25:31 -0400 Subject: [PATCH] ensure internalEnqueue can't add the same operation This fixes a bug where loadSavedOperations would duplicate operations in OperationRepo.queue. This is because it is possible for operations to be added before it is started and these are also persisted to disk. Improved OperationRepoTests by making mockOperationModelStore modify a in memory list. This was required to write a test to ensure loadSavedOperations wasn't duplicating operations. --- .../internal/operations/impl/OperationRepo.kt | 10 ++++- .../internal/operations/OperationRepoTests.kt | 38 +++++++++++++++---- 2 files changed, 38 insertions(+), 10 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 7309227e18..f402233a92 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 @@ -48,7 +48,7 @@ internal class OperationRepo( ) private val executorsMap: Map - private val queue = mutableListOf() + internal val queue = mutableListOf() private val waiter = WaiterWithValue() private var paused = false private var coroutineScope = CoroutineScope(newSingleThreadContext(name = "OpRepo")) @@ -143,6 +143,12 @@ internal class OperationRepo( addToStore: Boolean, index: Int? = null, ) { + val hasExisting = queue.any { it.operation.id == queueItem.operation.id } + if (hasExisting) { + Logging.debug("OperationRepo: internalEnqueue - operation.id: ${queueItem.operation.id} already exists in the queue.") + return + } + synchronized(queue) { if (index != null) { queue.add(index, queueItem) @@ -402,7 +408,7 @@ internal class OperationRepo( * NOTE: Sometimes the loading might take longer than expected due to I/O reads from disk * Any I/O implies executing time will vary greatly. */ - private fun loadSavedOperations() { + internal fun loadSavedOperations() { _operationModelStore.loadOperations() for (operation in _operationModelStore.list().withIndex()) { internalEnqueue( 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 1041e06170..4976ec6436 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 @@ -27,6 +27,7 @@ import kotlinx.coroutines.delay import kotlinx.coroutines.launch import kotlinx.coroutines.withTimeoutOrNull import kotlinx.coroutines.yield +import java.util.UUID // Mocks used by every test in this file private class Mocks { @@ -34,11 +35,16 @@ private class Mocks { val operationModelStore: OperationModelStore = run { + val operationStoreList = mutableListOf() val mockOperationModelStore = mockk() every { mockOperationModelStore.loadOperations() } just runs - every { mockOperationModelStore.list() } returns listOf() - every { mockOperationModelStore.add(any()) } just runs - every { mockOperationModelStore.remove(any()) } just runs + every { mockOperationModelStore.list() } returns operationStoreList + every { mockOperationModelStore.add(any()) } answers { operationStoreList.add(firstArg()) } + every { mockOperationModelStore.remove(any()) } answers { + val id = firstArg() + val op = operationStoreList.firstOrNull { it.id == id } + operationStoreList.remove(op) + } mockOperationModelStore } @@ -117,9 +123,9 @@ class OperationRepoTests : FunSpec({ test("enqueue operation executes and is removed when executed") { // Given val mocks = Mocks() - val operationIdSlot = slot() val operation = mockOperation(operationIdSlot = operationIdSlot) + val opId = operation.id // When mocks.operationRepo.start() @@ -136,7 +142,7 @@ class OperationRepoTests : FunSpec({ it[0] shouldBe operation }, ) - mocks.operationModelStore.remove("operationId") + mocks.operationModelStore.remove(opId) } } @@ -151,6 +157,7 @@ class OperationRepoTests : FunSpec({ val operationIdSlot = slot() val operation = mockOperation(operationIdSlot = operationIdSlot) + val opId = operation.id // When opRepo.start() @@ -174,7 +181,7 @@ class OperationRepoTests : FunSpec({ it[0] shouldBe operation }, ) - mocks.operationModelStore.remove("operationId") + mocks.operationModelStore.remove(opId) } } @@ -210,6 +217,7 @@ class OperationRepoTests : FunSpec({ val operationIdSlot = slot() val operation = mockOperation(operationIdSlot = operationIdSlot) + val opId = operation.id // When mocks.operationRepo.start() @@ -226,7 +234,7 @@ class OperationRepoTests : FunSpec({ it[0] shouldBe operation }, ) - mocks.operationModelStore.remove("operationId") + mocks.operationModelStore.remove(opId) } } @@ -608,10 +616,24 @@ class OperationRepoTests : FunSpec({ spyListener.onOperationRepoLoaded() } } + + test("ensure loadSavedOperations doesn't duplicate existing OperationItems") { + // Given + val mocks = Mocks() + val op = mockOperation() + mocks.operationRepo.enqueue(op) + + // When + mocks.operationRepo.loadSavedOperations() + + // Then + mocks.operationRepo.queue.size shouldBe 1 + mocks.operationRepo.queue.first().operation shouldBe op + } }) { companion object { private fun mockOperation( - id: String = "operationId", + id: String = UUID.randomUUID().toString(), name: String = "DUMMY_OPERATION", canStartExecute: Boolean = true, groupComparisonType: GroupComparisonType = GroupComparisonType.NONE,