From fbd69659e3c4db3a1dd298dc21ce0b8fc1e53b02 Mon Sep 17 00:00:00 2001 From: Chen Cen <79880926+ccen-stripe@users.noreply.github.com> Date: Mon, 8 Nov 2021 11:27:11 -0800 Subject: [PATCH] Replace FraudDetectionDataRequestExecutor with StripeNetworkClient (#4360) --- .../android/FraudDetectionDataRepository.kt | 42 ++++--- .../FraudDetectionDataRequestExecutor.kt | 44 -------- .../FraudDetectionDataRepositoryTest.kt | 103 +++++++++++------- .../FraudDetectionDataRequestExecutorTest.kt | 96 ---------------- 4 files changed, 92 insertions(+), 193 deletions(-) delete mode 100644 payments-core/src/main/java/com/stripe/android/networking/FraudDetectionDataRequestExecutor.kt delete mode 100644 payments-core/src/test/java/com/stripe/android/networking/FraudDetectionDataRequestExecutorTest.kt diff --git a/payments-core/src/main/java/com/stripe/android/FraudDetectionDataRepository.kt b/payments-core/src/main/java/com/stripe/android/FraudDetectionDataRepository.kt index f5fcb2b1fbd..f1db2d282fb 100644 --- a/payments-core/src/main/java/com/stripe/android/FraudDetectionDataRepository.kt +++ b/payments-core/src/main/java/com/stripe/android/FraudDetectionDataRepository.kt @@ -1,10 +1,13 @@ package com.stripe.android import android.content.Context -import com.stripe.android.networking.DefaultFraudDetectionDataRequestExecutor +import com.stripe.android.core.networking.DefaultStripeNetworkClient +import com.stripe.android.core.networking.StripeNetworkClient +import com.stripe.android.core.networking.StripeResponse +import com.stripe.android.core.networking.responseJson +import com.stripe.android.model.parsers.FraudDetectionDataJsonParser import com.stripe.android.networking.DefaultFraudDetectionDataRequestFactory import com.stripe.android.networking.FraudDetectionData -import com.stripe.android.networking.FraudDetectionDataRequestExecutor import com.stripe.android.networking.FraudDetectionDataRequestFactory import kotlinx.coroutines.CoroutineScope import kotlinx.coroutines.Dispatchers @@ -32,18 +35,18 @@ internal interface FraudDetectionDataRepository { fun save(fraudDetectionData: FraudDetectionData) } +private val timestampSupplier: () -> Long = { + Calendar.getInstance().timeInMillis +} + internal class DefaultFraudDetectionDataRepository( private val localStore: FraudDetectionDataStore, private val fraudDetectionDataRequestFactory: FraudDetectionDataRequestFactory, - private val fraudDetectionDataRequestExecutor: FraudDetectionDataRequestExecutor, + private val stripeNetworkClient: StripeNetworkClient, private val workContext: CoroutineContext ) : FraudDetectionDataRepository { private var cachedFraudDetectionData: FraudDetectionData? = null - private val timestampSupplier: () -> Long = { - Calendar.getInstance().timeInMillis - } - @JvmOverloads constructor( context: Context, @@ -51,9 +54,7 @@ internal class DefaultFraudDetectionDataRepository( ) : this( localStore = DefaultFraudDetectionDataStore(context, workContext), fraudDetectionDataRequestFactory = DefaultFraudDetectionDataRequestFactory(context), - fraudDetectionDataRequestExecutor = DefaultFraudDetectionDataRequestExecutor( - workContext = workContext - ), + stripeNetworkClient = DefaultStripeNetworkClient(workContext = workContext), workContext = workContext ) @@ -70,11 +71,14 @@ internal class DefaultFraudDetectionDataRepository( if (localFraudDetectionData == null || localFraudDetectionData.isExpired(timestampSupplier()) ) { - fraudDetectionDataRequestExecutor.execute( - request = fraudDetectionDataRequestFactory.create( - localFraudDetectionData - ) - ) + // fraud detection data request failures should be non-fatal + runCatching { + stripeNetworkClient.executeRequest( + fraudDetectionDataRequestFactory.create( + localFraudDetectionData + ) + ).fraudDetectionData() + }.getOrNull() } else { localFraudDetectionData } @@ -98,3 +102,11 @@ internal class DefaultFraudDetectionDataRepository( localStore.save(fraudDetectionData) } } + +private val fraudDetectionJsonParser = FraudDetectionDataJsonParser(timestampSupplier) + +/** + * Internal extension to convert the [String] body of [StripeResponse] to a [FraudDetectionData]. + */ +private fun StripeResponse.fraudDetectionData(): FraudDetectionData? = + takeIf { isOk }?.let { fraudDetectionJsonParser.parse(it.responseJson()) } diff --git a/payments-core/src/main/java/com/stripe/android/networking/FraudDetectionDataRequestExecutor.kt b/payments-core/src/main/java/com/stripe/android/networking/FraudDetectionDataRequestExecutor.kt deleted file mode 100644 index 2133b1086ff..00000000000 --- a/payments-core/src/main/java/com/stripe/android/networking/FraudDetectionDataRequestExecutor.kt +++ /dev/null @@ -1,44 +0,0 @@ -package com.stripe.android.networking - -import com.stripe.android.core.networking.ConnectionFactory -import com.stripe.android.core.networking.responseJson -import com.stripe.android.model.parsers.FraudDetectionDataJsonParser -import kotlinx.coroutines.Dispatchers -import kotlinx.coroutines.withContext -import java.util.Calendar -import kotlin.coroutines.CoroutineContext - -internal interface FraudDetectionDataRequestExecutor { - suspend fun execute( - request: FraudDetectionDataRequest - ): FraudDetectionData? -} - -internal class DefaultFraudDetectionDataRequestExecutor( - private val connectionFactory: ConnectionFactory = ConnectionFactory.Default, - private val workContext: CoroutineContext = Dispatchers.IO -) : FraudDetectionDataRequestExecutor { - private val timestampSupplier = { - Calendar.getInstance().timeInMillis - } - - override suspend fun execute( - request: FraudDetectionDataRequest - ) = withContext(workContext) { - // fraud detection data request failures should be non-fatal - runCatching { - executeInternal(request) - }.getOrNull() - } - - private fun executeInternal(request: FraudDetectionDataRequest): FraudDetectionData? { - connectionFactory.create(request).use { conn -> - return runCatching { - conn.response.takeIf { it.isOk }?.let { - FraudDetectionDataJsonParser(timestampSupplier) - .parse(it.responseJson()) - } - }.getOrNull() - } - } -} diff --git a/payments-core/src/test/java/com/stripe/android/FraudDetectionDataRepositoryTest.kt b/payments-core/src/test/java/com/stripe/android/FraudDetectionDataRepositoryTest.kt index db804768b1d..74865386942 100644 --- a/payments-core/src/test/java/com/stripe/android/FraudDetectionDataRepositoryTest.kt +++ b/payments-core/src/test/java/com/stripe/android/FraudDetectionDataRepositoryTest.kt @@ -3,10 +3,10 @@ package com.stripe.android import android.content.Context import androidx.test.core.app.ApplicationProvider import com.google.common.truth.Truth.assertThat +import com.stripe.android.core.networking.StripeNetworkClient +import com.stripe.android.core.networking.StripeResponse import com.stripe.android.networking.DefaultFraudDetectionDataRequestFactory import com.stripe.android.networking.FraudDetectionData -import com.stripe.android.networking.FraudDetectionDataRequest -import com.stripe.android.networking.FraudDetectionDataRequestExecutor import com.stripe.android.networking.FraudDetectionDataRequestFactory import kotlinx.coroutines.ExperimentalCoroutinesApi import kotlinx.coroutines.test.TestCoroutineDispatcher @@ -16,8 +16,11 @@ import org.mockito.kotlin.any import org.mockito.kotlin.mock import org.mockito.kotlin.never import org.mockito.kotlin.verify +import org.mockito.kotlin.whenever import org.robolectric.RobolectricTestRunner +import java.net.HttpURLConnection.HTTP_OK import java.util.Calendar +import java.util.UUID import java.util.concurrent.TimeUnit import kotlin.test.AfterTest import kotlin.test.Test @@ -27,7 +30,6 @@ import kotlin.test.Test class FraudDetectionDataRepositoryTest { private val context = ApplicationProvider.getApplicationContext() private val testDispatcher = TestCoroutineDispatcher() - private val fraudDetectionDataRequestExecutor: FraudDetectionDataRequestExecutor = mock() @AfterTest fun after() { @@ -46,46 +48,71 @@ class FraudDetectionDataRepositoryTest { } @Test - fun `get() when FraudDetectionData is expired should request new remote FraudDetectionData`() { - val expectedFraudDetectionData = createFraudDetectionData() - val repository = DefaultFraudDetectionDataRepository( - localStore = DefaultFraudDetectionDataStore( - context, - testDispatcher - ), - fraudDetectionDataRequestFactory = DefaultFraudDetectionDataRequestFactory(context), - fraudDetectionDataRequestExecutor = object : FraudDetectionDataRequestExecutor { - override suspend fun execute(request: FraudDetectionDataRequest) = expectedFraudDetectionData - }, - workContext = testDispatcher - ) - repository.save(createFraudDetectionData(elapsedTime = -60L)) - repository.refresh() - val actualFraudDetectionData = repository.getCached() + fun `get() when FraudDetectionData is expired should request new remote FraudDetectionData`() = + testDispatcher.runBlockingTest { + val mockStripeNetworkClient = mock() + val expectedGUID = UUID.randomUUID().toString() + val expectedMUID = UUID.randomUUID().toString() + val expectedSID = UUID.randomUUID().toString() - assertThat(actualFraudDetectionData) - .isEqualTo(expectedFraudDetectionData) - } + whenever(mockStripeNetworkClient.executeRequest(any())).thenReturn( + StripeResponse( + code = HTTP_OK, + body = + """ + { + "guid": "$expectedGUID", + "muid": "$expectedMUID", + "sid": "$expectedSID" + } + """.trimIndent() + ) + ) + + val repository = DefaultFraudDetectionDataRepository( + localStore = DefaultFraudDetectionDataStore( + context, + testDispatcher + ), + fraudDetectionDataRequestFactory = DefaultFraudDetectionDataRequestFactory(context), + stripeNetworkClient = mockStripeNetworkClient, + workContext = testDispatcher + ) + val expiredFraudDetectionData = createFraudDetectionData(elapsedTime = -60L) + repository.save(expiredFraudDetectionData) + repository.refresh() + val actualFraudDetectionData = repository.getCached() + + assertThat(expiredFraudDetectionData.guid).isNotEqualTo(expectedGUID) + assertThat(expiredFraudDetectionData.muid).isNotEqualTo(expectedMUID) + assertThat(expiredFraudDetectionData.sid).isNotEqualTo(expectedSID) + + assertThat(requireNotNull(actualFraudDetectionData).guid).isEqualTo(expectedGUID) + assertThat(actualFraudDetectionData.muid).isEqualTo(expectedMUID) + assertThat(actualFraudDetectionData.sid).isEqualTo(expectedSID) + } @Test - fun `refresh() when advancedFraudSignals is disabled should not fetch FraudDetectionData`() = testDispatcher.runBlockingTest { - Stripe.advancedFraudSignalsEnabled = false + fun `refresh() when advancedFraudSignals is disabled should not fetch FraudDetectionData`() = + testDispatcher.runBlockingTest { + Stripe.advancedFraudSignalsEnabled = false + val mockStripeNetworkClient = mock() - val store: FraudDetectionDataStore = mock() - val fraudDetectionDataRequestFactory: FraudDetectionDataRequestFactory = mock() - val repository = DefaultFraudDetectionDataRepository( - localStore = store, - fraudDetectionDataRequestFactory = fraudDetectionDataRequestFactory, - fraudDetectionDataRequestExecutor = fraudDetectionDataRequestExecutor, - workContext = testDispatcher - ) - repository.refresh() + val store: FraudDetectionDataStore = mock() + val fraudDetectionDataRequestFactory: FraudDetectionDataRequestFactory = mock() + val repository = DefaultFraudDetectionDataRepository( + localStore = store, + fraudDetectionDataRequestFactory = fraudDetectionDataRequestFactory, + stripeNetworkClient = mockStripeNetworkClient, + workContext = testDispatcher + ) + repository.refresh() - verify(store, never()).get() - verify(store, never()).save(any()) - verify(fraudDetectionDataRequestFactory, never()).create(any()) - verify(fraudDetectionDataRequestExecutor, never()).execute(any()) - } + verify(store, never()).get() + verify(store, never()).save(any()) + verify(fraudDetectionDataRequestFactory, never()).create(any()) + verify(mockStripeNetworkClient, never()).executeRequest(any()) + } private companion object { fun createFraudDetectionData(elapsedTime: Long = 0L): FraudDetectionData { diff --git a/payments-core/src/test/java/com/stripe/android/networking/FraudDetectionDataRequestExecutorTest.kt b/payments-core/src/test/java/com/stripe/android/networking/FraudDetectionDataRequestExecutorTest.kt deleted file mode 100644 index 9cb520f2e97..00000000000 --- a/payments-core/src/test/java/com/stripe/android/networking/FraudDetectionDataRequestExecutorTest.kt +++ /dev/null @@ -1,96 +0,0 @@ -package com.stripe.android.networking - -import androidx.test.core.app.ApplicationProvider -import com.google.common.truth.Truth.assertThat -import com.stripe.android.FraudDetectionDataFixtures -import com.stripe.android.core.networking.ConnectionFactory -import com.stripe.android.core.networking.StripeConnection -import kotlinx.coroutines.ExperimentalCoroutinesApi -import kotlinx.coroutines.test.TestCoroutineDispatcher -import kotlinx.coroutines.test.runBlockingTest -import org.junit.runner.RunWith -import org.mockito.kotlin.mock -import org.mockito.kotlin.whenever -import org.robolectric.RobolectricTestRunner -import java.io.IOException -import kotlin.test.AfterTest -import kotlin.test.Test - -@RunWith(RobolectricTestRunner::class) -@ExperimentalCoroutinesApi -class FraudDetectionDataRequestExecutorTest { - private val testDispatcher = TestCoroutineDispatcher() - - private val fraudDetectionDataRequestFactory = DefaultFraudDetectionDataRequestFactory( - context = ApplicationProvider.getApplicationContext() - ) - - @AfterTest - fun cleanup() { - testDispatcher.cleanupTestCoroutines() - } - - @Test - fun `execute() when successful should return non-empty values`() { - testDispatcher.runBlockingTest { - val remoteFraudDetectionData = requireNotNull( - createFraudDetectionDataRequestExecutor().execute( - request = fraudDetectionDataRequestFactory.create(FRAUD_DETECTION_DATA) - ) - ) - - assertThat(remoteFraudDetectionData.guid) - .isNotEmpty() - assertThat(remoteFraudDetectionData.muid) - .isNotEmpty() - assertThat(remoteFraudDetectionData.sid) - .isNotEmpty() - } - } - - @Test - fun `execute() when successful should return null`() { - val request = fraudDetectionDataRequestFactory.create(FRAUD_DETECTION_DATA) - val connection = mock>().also { - whenever(it.responseCode).thenReturn(500) - } - - val connectionFactory = mock().also { - whenever(it.create(request)) - .thenReturn(connection) - } - - testDispatcher.runBlockingTest { - assertThat( - createFraudDetectionDataRequestExecutor(connectionFactory = connectionFactory) - .execute(request = request) - ).isNull() - } - } - - @Test - fun `execute() when connection exception should return null`() { - val request = fraudDetectionDataRequestFactory.create(FRAUD_DETECTION_DATA) - val connectionFactory = mock().also { - whenever(it.create(request)).thenThrow(IOException()) - } - - testDispatcher.runBlockingTest { - assertThat( - createFraudDetectionDataRequestExecutor(connectionFactory = connectionFactory) - .execute(request = request) - ).isNull() - } - } - - private fun createFraudDetectionDataRequestExecutor( - connectionFactory: ConnectionFactory = ConnectionFactory.Default - ) = DefaultFraudDetectionDataRequestExecutor( - connectionFactory = connectionFactory, - workContext = testDispatcher - ) - - private companion object { - private val FRAUD_DETECTION_DATA = FraudDetectionDataFixtures.create() - } -}