From cbaa85eae479a44d7ad1eb7e066ae0141713556f Mon Sep 17 00:00:00 2001 From: Oscar Spruit Date: Wed, 3 Apr 2024 14:32:06 +0200 Subject: [PATCH 1/2] Correctly merge flows in StatusRepository and add debounce This fixes an issue where a status could be fetched multiple times at once, causing the /payments/details call to be triggered multiple times. COAND-883 --- .../internal/data/api/StatusRepository.kt | 65 ++++++--- .../data/api/DefaultStatusRepositoryTest.kt | 133 ++++++++++++++---- .../checkout/test/extensions/TestFlow.kt | 5 + 3 files changed, 154 insertions(+), 49 deletions(-) diff --git a/components-core/src/main/java/com/adyen/checkout/components/core/internal/data/api/StatusRepository.kt b/components-core/src/main/java/com/adyen/checkout/components/core/internal/data/api/StatusRepository.kt index ba23f8de61..4850b00e36 100644 --- a/components-core/src/main/java/com/adyen/checkout/components/core/internal/data/api/StatusRepository.kt +++ b/components-core/src/main/java/com/adyen/checkout/components/core/internal/data/api/StatusRepository.kt @@ -9,25 +9,34 @@ package com.adyen.checkout.components.core.internal.data.api import androidx.annotation.RestrictTo +import androidx.annotation.VisibleForTesting import com.adyen.checkout.components.core.internal.data.model.StatusRequest import com.adyen.checkout.components.core.internal.data.model.StatusResponse import com.adyen.checkout.components.core.internal.util.StatusResponseUtils +import com.adyen.checkout.components.core.internal.util.bufferedChannel import com.adyen.checkout.core.AdyenLogLevel import com.adyen.checkout.core.internal.util.adyenLog import com.adyen.checkout.core.internal.util.runSuspendCatching import kotlinx.coroutines.CoroutineDispatcher import kotlinx.coroutines.Dispatchers +import kotlinx.coroutines.FlowPreview import kotlinx.coroutines.cancel import kotlinx.coroutines.currentCoroutineContext import kotlinx.coroutines.delay import kotlinx.coroutines.flow.Flow -import kotlinx.coroutines.flow.MutableSharedFlow +import kotlinx.coroutines.flow.debounce import kotlinx.coroutines.flow.flow import kotlinx.coroutines.flow.map import kotlinx.coroutines.flow.merge +import kotlinx.coroutines.flow.onEach +import kotlinx.coroutines.flow.receiveAsFlow +import kotlinx.coroutines.flow.transform import kotlinx.coroutines.isActive import kotlinx.coroutines.withContext -import java.util.concurrent.TimeUnit +import kotlin.time.Duration.Companion.milliseconds +import kotlin.time.Duration.Companion.seconds +import kotlin.time.TimeMark +import kotlin.time.TimeSource @RestrictTo(RestrictTo.Scope.LIBRARY_GROUP) interface StatusRepository { @@ -41,19 +50,36 @@ interface StatusRepository { class DefaultStatusRepository( private val statusService: StatusService, private val clientKey: String, + private val timeSource: TimeSource = TimeSource.Monotonic, private val coroutineDispatcher: CoroutineDispatcher = Dispatchers.IO, ) : StatusRepository { private var delay: Long = 0 - private val refreshFlow: MutableSharedFlow = MutableSharedFlow(extraBufferCapacity = 1) + private val refreshFlow = bufferedChannel() + @OptIn(FlowPreview::class) override fun poll(paymentData: String, maxPollingDuration: Long): Flow> { - val startTime = System.currentTimeMillis() + val startTime = timeSource.markNow() + + updateDelay(startTime, maxPollingDuration) val pollingFlow = flow { while (currentCoroutineContext().isActive) { - val result = fetchStatus(paymentData) + emit(paymentData) + delay(delay) + } + } + + return merge( + pollingFlow, + refreshFlow.receiveAsFlow(), + ) + .debounce(DEBOUNCE_TIME) + .map { + fetchStatus(it) + } + .transform { result -> emit(result) if (result.isSuccess && StatusResponseUtils.isFinalResult(result.getOrThrow())) { @@ -61,18 +87,14 @@ class DefaultStatusRepository( } if (!updateDelay(startTime, maxPollingDuration)) { - emit(Result.failure(IllegalStateException("Max polling time has been exceeded."))) + adyenLog(AdyenLogLevel.DEBUG) { "Max polling time exceeded" } + emit(Result.failure(IllegalStateException("Max polling time exceeded."))) currentCoroutineContext().cancel() } - - delay(delay) } - } - - return merge( - pollingFlow, - refreshFlow.map { fetchStatus(it) }, - ) + .onEach { + adyenLog(AdyenLogLevel.DEBUG) { "Emitting status: ${it.getOrNull()?.resultCode}" } + } } private suspend fun fetchStatus(paymentData: String) = withContext(coroutineDispatcher) { @@ -84,8 +106,8 @@ class DefaultStatusRepository( /** * @return Returns if the delay time was updated. If not, that means the max polling time has been exceeded. */ - private fun updateDelay(startTime: Long, maxPollingDuration: Long): Boolean { - val elapsedTime = System.currentTimeMillis() - startTime + private fun updateDelay(startTime: TimeMark, maxPollingDuration: Long): Boolean { + val elapsedTime = startTime.elapsedNow().inWholeMilliseconds return when { elapsedTime <= POLLING_THRESHOLD -> { delay = POLLING_DELAY_FAST @@ -103,12 +125,15 @@ class DefaultStatusRepository( override fun refreshStatus(paymentData: String) { adyenLog(AdyenLogLevel.VERBOSE) { "refreshStatus" } - refreshFlow.tryEmit(paymentData) + refreshFlow.trySend(paymentData) } companion object { - private val POLLING_DELAY_FAST = TimeUnit.SECONDS.toMillis(2) - private val POLLING_DELAY_SLOW = TimeUnit.SECONDS.toMillis(10) - private val POLLING_THRESHOLD = TimeUnit.SECONDS.toMillis(60) + private val POLLING_DELAY_FAST = 2.seconds.inWholeMilliseconds + private val POLLING_DELAY_SLOW = 10.seconds.inWholeMilliseconds + private val POLLING_THRESHOLD = 60.seconds.inWholeMilliseconds + + @VisibleForTesting + internal val DEBOUNCE_TIME = 100.milliseconds.inWholeMilliseconds } } diff --git a/components-core/src/test/java/com/adyen/checkout/components/core/internal/data/api/DefaultStatusRepositoryTest.kt b/components-core/src/test/java/com/adyen/checkout/components/core/internal/data/api/DefaultStatusRepositoryTest.kt index 0c924c7721..cc529d68b5 100644 --- a/components-core/src/test/java/com/adyen/checkout/components/core/internal/data/api/DefaultStatusRepositoryTest.kt +++ b/components-core/src/test/java/com/adyen/checkout/components/core/internal/data/api/DefaultStatusRepositoryTest.kt @@ -8,72 +8,147 @@ package com.adyen.checkout.components.core.internal.data.api -import app.cash.turbine.test import com.adyen.checkout.components.core.internal.data.model.StatusResponse +import com.adyen.checkout.test.LoggingExtension import com.adyen.checkout.test.TestDispatcherExtension +import com.adyen.checkout.test.extensions.test +import kotlinx.coroutines.CancellationException import kotlinx.coroutines.ExperimentalCoroutinesApi +import kotlinx.coroutines.cancel +import kotlinx.coroutines.test.TestCoroutineScheduler +import kotlinx.coroutines.test.UnconfinedTestDispatcher +import kotlinx.coroutines.test.advanceUntilIdle import kotlinx.coroutines.test.runTest +import kotlinx.coroutines.test.testTimeSource import org.junit.jupiter.api.Assertions.assertEquals -import org.junit.jupiter.api.BeforeEach +import org.junit.jupiter.api.Assertions.assertInstanceOf +import org.junit.jupiter.api.Assertions.assertTrue import org.junit.jupiter.api.Test import org.junit.jupiter.api.extension.ExtendWith import org.mockito.Mock import org.mockito.junit.jupiter.MockitoExtension import org.mockito.kotlin.any import org.mockito.kotlin.doReturn +import org.mockito.kotlin.times +import org.mockito.kotlin.verify import org.mockito.kotlin.whenever -import java.util.concurrent.TimeUnit +import kotlin.time.Duration.Companion.minutes +import kotlin.time.ExperimentalTime +import kotlin.time.TimeSource -@OptIn(ExperimentalCoroutinesApi::class) -@ExtendWith(MockitoExtension::class, TestDispatcherExtension::class) +@OptIn(ExperimentalCoroutinesApi::class, ExperimentalTime::class) +@ExtendWith(MockitoExtension::class, TestDispatcherExtension::class, LoggingExtension::class) internal class DefaultStatusRepositoryTest( @Mock private val statusService: StatusService ) { private lateinit var statusRepository: DefaultStatusRepository - @BeforeEach - fun beforeEach() { - statusRepository = DefaultStatusRepository(statusService, "someclientkey") - } - @Test fun `when receiving the final result, then it should be emitted and the flow should end`() = runTest { + statusRepository = createRepository(testScheduler, testTimeSource) val response = StatusResponse(resultCode = "final") whenever(statusService.checkStatus(any(), any())) doReturn response - statusRepository - .poll("paymentData", DEFAULT_MAX_POLLING_DURATION) - .test { - val expected = Result.success(response) - assertEquals(expected, awaitItem()) + val statusFlow = statusRepository + .poll("paymentData", MAX_POLLING_DURATION) + .test(testScheduler) + + advanceUntilIdle() + + val expected = Result.success(response) + assertEquals(expected, statusFlow.latestValue) - cancelAndIgnoreRemainingEvents() - } + statusFlow.cancel() } @Test fun `when refreshing the status, then the result is emitted immediately`() = runTest { + statusRepository = createRepository(testScheduler, testTimeSource) val refreshResponse = StatusResponse(resultCode = "refresh") whenever(statusService.checkStatus(any(), any())) - // return final result first, so polling stops - .doReturn(StatusResponse(resultCode = "final"), refreshResponse) + .doReturn(StatusResponse(resultCode = "pending"), refreshResponse) - statusRepository - .poll("paymentData", DEFAULT_MAX_POLLING_DURATION) - .test { - skipItems(1) + val statusFlow = statusRepository + .poll("paymentData", MAX_POLLING_DURATION) + .test(testScheduler) - statusRepository.refreshStatus("test") + statusRepository.refreshStatus("test") + + advanceUntilIdle() + + val expected = Result.success(refreshResponse) + assertEquals(expected, statusFlow.latestValue) + + statusFlow.cancel() + } - val expected = Result.success(refreshResponse) - assertEquals(expected, awaitItem()) + @Test + fun `when fetching the status multiple times at the same time, then it is only fetched once`() = runTest { + statusRepository = createRepository(testScheduler, testTimeSource) + whenever(statusService.checkStatus(any(), any())) doReturn StatusResponse(resultCode = "pending") + + val statusFlow = statusRepository + .poll("paymentData", MAX_POLLING_DURATION) + .test(testScheduler) + + statusRepository.refreshStatus("test") + statusRepository.refreshStatus("test") + statusRepository.refreshStatus("test") + + testScheduler.advanceTimeBy(DefaultStatusRepository.DEBOUNCE_TIME + 1) + + verify(statusService, times(1)).checkStatus(any(), any()) + + statusFlow.cancel() + } + + @Test + fun `when polling result is final, then the flow is cancelled`() = runTest { + statusRepository = createRepository(testScheduler, testTimeSource) + whenever(statusService.checkStatus(any(), any())) doReturn StatusResponse(resultCode = "authorised") + + val statusFlow = statusRepository + .poll("paymentData", MAX_POLLING_DURATION) + .test(testScheduler) + + testScheduler.advanceTimeBy(DefaultStatusRepository.DEBOUNCE_TIME + 1) + + assertInstanceOf(CancellationException::class.java, statusFlow.completionThrowable) + + statusFlow.cancel() + } + + @Test + fun `when max polling time is exceeded, then a value is emitted and the flow is cancelled`() = runTest { + statusRepository = createRepository(testScheduler, testTimeSource) + whenever(statusService.checkStatus(any(), any())) doReturn StatusResponse(resultCode = "pending") + + val statusFlow = statusRepository + .poll("paymentData", MAX_POLLING_DURATION) + .test(testScheduler) + + testScheduler.advanceTimeBy(MAX_POLLING_DURATION + 2100) + + assertTrue(statusFlow.latestValue.isFailure) + assertInstanceOf(CancellationException::class.java, statusFlow.completionThrowable) + + statusFlow.cancel() + } - cancelAndIgnoreRemainingEvents() - } + private fun createRepository( + testScheduler: TestCoroutineScheduler, + testTimeSource: TimeSource + ): DefaultStatusRepository { + return DefaultStatusRepository( + statusService = statusService, + clientKey = "someclientkey", + timeSource = testTimeSource, + coroutineDispatcher = UnconfinedTestDispatcher(testScheduler), + ) } companion object { - private val DEFAULT_MAX_POLLING_DURATION = TimeUnit.MINUTES.toMillis(10) + private val MAX_POLLING_DURATION = 1.minutes.inWholeMilliseconds } } diff --git a/test-core/src/main/java/com/adyen/checkout/test/extensions/TestFlow.kt b/test-core/src/main/java/com/adyen/checkout/test/extensions/TestFlow.kt index 626b27e370..24ea9f77c2 100644 --- a/test-core/src/main/java/com/adyen/checkout/test/extensions/TestFlow.kt +++ b/test-core/src/main/java/com/adyen/checkout/test/extensions/TestFlow.kt @@ -13,6 +13,7 @@ import kotlinx.coroutines.ExperimentalCoroutinesApi import kotlinx.coroutines.Job import kotlinx.coroutines.flow.Flow import kotlinx.coroutines.flow.launchIn +import kotlinx.coroutines.flow.onCompletion import kotlinx.coroutines.flow.onEach import kotlinx.coroutines.test.TestCoroutineScheduler import kotlinx.coroutines.test.UnconfinedTestDispatcher @@ -35,9 +36,13 @@ class TestFlow internal constructor(flow: Flow, testScheduler: TestCorouti val latestValue: T get() = values.last() + var completionThrowable: Throwable? = null + private set + init { flow .onEach { _values.add(it) } + .onCompletion { completionThrowable = it } .launchIn(this) } } From de5ed251fecb29b3a744d23fd64824884a6e4141 Mon Sep 17 00:00:00 2001 From: Oscar Spruit Date: Wed, 3 Apr 2024 14:52:02 +0200 Subject: [PATCH 2/2] Add release note entry COAND-883 --- RELEASE_NOTES.md | 1 + 1 file changed, 1 insertion(+) diff --git a/RELEASE_NOTES.md b/RELEASE_NOTES.md index def4d92de8..a5f4a62ff7 100644 --- a/RELEASE_NOTES.md +++ b/RELEASE_NOTES.md @@ -13,6 +13,7 @@ - Overriding some of the XML styles without specifying a parent style no longer causes a build error. - Not defining `?android:attr/textColor` in your own theme will no longer crash. - The build output should no longer contain warnings about multiple substitutions specified in non-positional format in string resources. +- In some edge cases `onAdditionalDetails` was triggered multiple times, this no longer happens. ## Removed - The functions to get specific configurations from `CheckoutConfiguration` (such as `CheckoutConfiguration.getDropInConfiguration()` or `CheckoutConfiguration.getCardConfiguration()`) are no longer accessible. Pass the `CheckoutConfiguration` object as it is when starting Drop-in or Components.