From ec026f58968d9dbf948420f0db8534db3148f201 Mon Sep 17 00:00:00 2001 From: Bala Mani Nayar D Date: Tue, 13 Oct 2026 16:10:42 +0530 Subject: [PATCH 01/15] feat(SharedUtils): add size-limited binary body support to HttpClient Image downloads (and other binary payloads) need a way to fetch raw bytes without corrupting them through a String round-trip, and with a hard cap on body size so a malicious or misbehaving server can't exhaust memory. sendRequestForBytes/executeRequestForBytes stream the body via okio and enforce maxBodyBytes even when the response omits a Content-Length (e.g. chunked transfer). Co-Authored-By: Claude Sonnet 5 --- .../api/sharedutils/HttpClient.kt | 15 ++++ .../OkHttpSynchronousHttpClient.kt | 47 ++++++++++ .../api/sharedutils/HttpClientUnitTest.kt | 30 +++++++ .../OkHttpSynchronousHttpClientTest.kt | 85 +++++++++++++++++++ 4 files changed, 177 insertions(+) diff --git a/SharedUtils/src/main/java/com/braintreepayments/api/sharedutils/HttpClient.kt b/SharedUtils/src/main/java/com/braintreepayments/api/sharedutils/HttpClient.kt index 3698965e16..675eec0ffe 100644 --- a/SharedUtils/src/main/java/com/braintreepayments/api/sharedutils/HttpClient.kt +++ b/SharedUtils/src/main/java/com/braintreepayments/api/sharedutils/HttpClient.kt @@ -23,4 +23,19 @@ class HttpClient internal constructor( okHttpSynchronousHttpClient.executeRequest(request) } } + + /** + * Sends [request] and returns the raw response body, for binary payloads such as images. + * + * @param maxBodyBytes Optional limit on the response body size, enforced even if the response omits + * `Content-Length` (e.g. chunked transfer). + * @throws java.io.IOException if the network request fails, the response is unsuccessful or has no body, or the + * body size exceeds [maxBodyBytes] + * @throws IllegalArgumentException if the request URL is not a valid http or https URL + */ + suspend fun sendRequestForBytes(request: OkHttpRequest, maxBodyBytes: Long? = null): ByteArray { + return withContext(ioDispatcher) { + okHttpSynchronousHttpClient.executeRequestForBytes(request, maxBodyBytes) + } + } } diff --git a/SharedUtils/src/main/java/com/braintreepayments/api/sharedutils/OkHttpSynchronousHttpClient.kt b/SharedUtils/src/main/java/com/braintreepayments/api/sharedutils/OkHttpSynchronousHttpClient.kt index 01f6c85d3b..a53fba5cc8 100644 --- a/SharedUtils/src/main/java/com/braintreepayments/api/sharedutils/OkHttpSynchronousHttpClient.kt +++ b/SharedUtils/src/main/java/com/braintreepayments/api/sharedutils/OkHttpSynchronousHttpClient.kt @@ -5,6 +5,8 @@ import okhttp3.MediaType.Companion.toMediaTypeOrNull import okhttp3.OkHttpClient import okhttp3.Request import okhttp3.RequestBody.Companion.toRequestBody +import okhttp3.ResponseBody +import okio.Buffer import java.io.IOException import java.util.concurrent.TimeUnit @@ -52,6 +54,51 @@ internal class OkHttpSynchronousHttpClient( } } + /** + * Executes a synchronous HTTP request using OkHttp and returns the raw response body. + * + * Use this for binary payloads such as images, which [executeRequest] would corrupt by decoding the body as a + * [String]. + * + * @param okHttpRequest The request data to execute. + * @param maxBodyBytes Optional limit on the response body size. Rejected as soon as the declared + * `Content-Length` exceeds it, or, if undeclared (e.g. chunked transfer), as soon as actual bytes read exceed + * it — the body is never buffered past [maxBodyBytes] + 1 bytes. + * @return The response body bytes. + * @throws IOException if the request fails, the response is unsuccessful or has no body, or the body size + * exceeds [maxBodyBytes]. + * @throws IllegalArgumentException if the request URL is not a valid http or https URL. + */ + @Throws(IOException::class) + fun executeRequestForBytes(okHttpRequest: OkHttpRequest, maxBodyBytes: Long? = null): ByteArray { + val request = okHttpRequest.toRequest() + + okHttpClient.newCall(request).execute().use { response -> + val responseBody = response.body?.takeIf { response.isSuccessful } + ?: throw IOException("Unexpected code ${response.code} for ${request.url}") + if (maxBodyBytes == null) return responseBody.bytes() + return responseBody.readBoundedBytes(maxBodyBytes, request.url.toString()) + } + } + + // Bounded regardless of the declared Content-Length, so a response that omits or understates it (e.g. chunked + // transfer) still can't be buffered unbounded in memory. + private fun ResponseBody.readBoundedBytes(maxBodyBytes: Long, url: String): ByteArray { + if (contentLength() > maxBodyBytes) throwBodyTooLarge(maxBodyBytes, url) + + val limit = maxBodyBytes + 1 + val buffer = Buffer() + val source = source() + while (buffer.size < limit && source.read(buffer, limit - buffer.size) != -1L) { + // keep reading until EOF or the limit is reached + } + if (buffer.size > maxBodyBytes) throwBodyTooLarge(maxBodyBytes, url) + return buffer.readByteArray() + } + + private fun throwBodyTooLarge(maxBodyBytes: Long, url: String): Nothing = + throw IOException("Response body exceeds $maxBodyBytes bytes for $url") + /** * Converts an [OkHttpRequest] to an OkHttp [Request]. * diff --git a/SharedUtils/src/test/java/com/braintreepayments/api/sharedutils/HttpClientUnitTest.kt b/SharedUtils/src/test/java/com/braintreepayments/api/sharedutils/HttpClientUnitTest.kt index dd53a29c19..b5234d4d5d 100644 --- a/SharedUtils/src/test/java/com/braintreepayments/api/sharedutils/HttpClientUnitTest.kt +++ b/SharedUtils/src/test/java/com/braintreepayments/api/sharedutils/HttpClientUnitTest.kt @@ -6,6 +6,7 @@ import io.mockk.verify import kotlinx.coroutines.ExperimentalCoroutinesApi import kotlinx.coroutines.test.StandardTestDispatcher import kotlinx.coroutines.test.runTest +import org.junit.Assert.assertArrayEquals import org.junit.Assert.assertEquals import org.junit.Assert.assertNotNull import org.junit.Before @@ -94,4 +95,33 @@ class HttpClientUnitTest { val sut = HttpClient() assertNotNull(sut) } + + @Test + fun `when sendRequestForBytes is called, returns bytes and forwards maxBodyBytes`() = + runTest(testDispatcher) { + val request = mockk() + val bytes = byteArrayOf(1, 2, 3) + + every { mockOkHttpClient.executeRequestForBytes(request, any()) } returns bytes + + assertArrayEquals(bytes, sut.sendRequestForBytes(request, maxBodyBytes = 1024L)) + sut.sendRequestForBytes(request) + + verify { mockOkHttpClient.executeRequestForBytes(request, 1024L) } + verify { mockOkHttpClient.executeRequestForBytes(request, null) } + } + + @Test + fun `when sendRequestForBytes fails with IOException, throws IOException`() = + runTest(testDispatcher) { + val request = mockk() + + every { mockOkHttpClient.executeRequestForBytes(request, null) } throws java.io.IOException("Not found") + + val thrownException = assertFailsWith { + sut.sendRequestForBytes(request) + } + + assertEquals("Not found", thrownException.message) + } } diff --git a/SharedUtils/src/test/java/com/braintreepayments/api/sharedutils/OkHttpSynchronousHttpClientTest.kt b/SharedUtils/src/test/java/com/braintreepayments/api/sharedutils/OkHttpSynchronousHttpClientTest.kt index 80d6f8004f..5b66cbdc32 100644 --- a/SharedUtils/src/test/java/com/braintreepayments/api/sharedutils/OkHttpSynchronousHttpClientTest.kt +++ b/SharedUtils/src/test/java/com/braintreepayments/api/sharedutils/OkHttpSynchronousHttpClientTest.kt @@ -11,12 +11,16 @@ import okhttp3.OkHttpClient import okhttp3.Request import okhttp3.Response import okhttp3.ResponseBody +import okhttp3.ResponseBody.Companion.toResponseBody +import okio.Buffer import org.junit.After +import org.junit.Assert.assertArrayEquals import org.junit.Assert.assertEquals import org.junit.Assert.assertTrue import org.junit.Before import org.junit.Test import java.io.IOException +import kotlin.test.assertFailsWith class OkHttpSynchronousHttpClientTest { @@ -105,4 +109,85 @@ class OkHttpSynchronousHttpClientTest { assertEquals("Bearer token", capturedRequest.header("Authorization")) assertEquals("Value", capturedRequest.header("Custom")) } + + @Test + fun `when response is successful, executeRequestForBytes sends a GET and returns the body bytes unchanged`() { + // PNG signature plus bytes that are invalid UTF-8, which a String round-trip would corrupt + val bytes = byteArrayOf(0x89.toByte(), 0x50, 0x4E, 0x47, 0xFF.toByte(), 0xFE.toByte(), 0x00) + givenBytesResponse(isSuccessful = true, body = bytes.toResponseBody()) + + val result = sut.executeRequestForBytes(OkHttpRequest(URL, Method.Get)) + + assertArrayEquals(bytes, result) + val slot = slot() + verify { okHttpClient.newCall(capture(slot)) } + assertEquals("$URL/", slot.captured.url.toString()) + assertEquals("GET", slot.captured.method) + } + + @Test + fun `when response is unsuccessful or has no body, executeRequestForBytes throws IOException`() { + givenBytesResponse(isSuccessful = false, body = byteArrayOf(1).toResponseBody()) + assertFailsWith { sut.executeRequestForBytes(OkHttpRequest(URL, Method.Get)) } + + givenBytesResponse(isSuccessful = true, body = null) + assertFailsWith { sut.executeRequestForBytes(OkHttpRequest(URL, Method.Get)) } + } + + @Test + fun `when declared body size exceeds maxBodyBytes, executeRequestForBytes throws without reading the body`() { + givenBytesResponse(isSuccessful = true, body = responseBody) + every { responseBody.contentLength() } returns 2048L + + assertFailsWith { + sut.executeRequestForBytes(OkHttpRequest(URL, Method.Get), maxBodyBytes = 1024L) + } + verify(exactly = 0) { responseBody.bytes() } + verify(exactly = 0) { responseBody.source() } + } + + @Test + fun `when declared body size is within maxBodyBytes, executeRequestForBytes returns the body`() { + val bytes = ByteArray(1024) { 7 } + givenBytesResponse(isSuccessful = true, body = bytes.toResponseBody()) + + val result = sut.executeRequestForBytes(OkHttpRequest(URL, Method.Get), maxBodyBytes = 1024L) + + assertArrayEquals(bytes, result) + } + + @Test + fun `when Content-Length is not declared and actual body is within maxBodyBytes, returns the body`() { + val bytes = ByteArray(1024) { 7 } + givenBytesResponse(isSuccessful = true, body = responseBody) + every { responseBody.contentLength() } returns -1L + every { responseBody.source() } returns Buffer().write(bytes) + + val result = sut.executeRequestForBytes(OkHttpRequest(URL, Method.Get), maxBodyBytes = 1024L) + + assertArrayEquals(bytes, result) + } + + @Test + fun `when Content-Length is not declared but actual body exceeds maxBodyBytes, throws without buffering it all`() { + val bytes = ByteArray(2048) { 7 } + givenBytesResponse(isSuccessful = true, body = responseBody) + every { responseBody.contentLength() } returns -1L + every { responseBody.source() } returns Buffer().write(bytes) + + assertFailsWith { + sut.executeRequestForBytes(OkHttpRequest(URL, Method.Get), maxBodyBytes = 1024L) + } + } + + private fun givenBytesResponse(isSuccessful: Boolean, body: ResponseBody?) { + every { okHttpClient.newCall(any()) } returns call + every { call.execute() } returns response + every { response.isSuccessful } returns isSuccessful + every { response.body } returns body + } + + companion object { + private const val URL = "https://example.com" + } } From 4381b29ed4e89c1a11ad7d9551a0936d302d6a3c Mon Sep 17 00:00:00 2001 From: Bala Mani Nayar D Date: Tue, 13 Oct 2026 16:10:51 +0530 Subject: [PATCH 02/15] feat(PayPalSavedPaymentMethod): add ImageLoader and Compose LoaderImage Saved-payment-method icons need to be fetched, decoded, and cached without blocking the main thread. ImageLoader de-dupes concurrent requests for the same URL, bounds parallel bitmap decoding, caches decoded bitmaps in an LruCache, and binds results into an ImageView with placeholder/cancel/re-attach-resume support for RecyclerView reuse. LoaderImage wraps it for Compose call sites. Co-Authored-By: Claude Sonnet 5 --- .../imageloader/ImageLoader.kt | 203 ++++++++++++++++++ .../imageloader/LoaderImage.kt | 49 +++++ .../imageloader/ImageLoaderUnitTest.kt | 168 +++++++++++++++ .../ImageLoaderViewBindingUnitTest.kt | 145 +++++++++++++ .../imageloader/TestImages.kt | 13 ++ 5 files changed, 578 insertions(+) create mode 100644 PayPalSavedPaymentMethod/src/main/java/com/braintreepayments/api/paypalsavedpaymentmethod/imageloader/ImageLoader.kt create mode 100644 PayPalSavedPaymentMethod/src/main/java/com/braintreepayments/api/paypalsavedpaymentmethod/imageloader/LoaderImage.kt create mode 100644 PayPalSavedPaymentMethod/src/test/java/com/braintreepayments/api/paypalsavedpaymentmethod/imageloader/ImageLoaderUnitTest.kt create mode 100644 PayPalSavedPaymentMethod/src/test/java/com/braintreepayments/api/paypalsavedpaymentmethod/imageloader/ImageLoaderViewBindingUnitTest.kt create mode 100644 PayPalSavedPaymentMethod/src/test/java/com/braintreepayments/api/paypalsavedpaymentmethod/imageloader/TestImages.kt diff --git a/PayPalSavedPaymentMethod/src/main/java/com/braintreepayments/api/paypalsavedpaymentmethod/imageloader/ImageLoader.kt b/PayPalSavedPaymentMethod/src/main/java/com/braintreepayments/api/paypalsavedpaymentmethod/imageloader/ImageLoader.kt new file mode 100644 index 0000000000..70de2b5180 --- /dev/null +++ b/PayPalSavedPaymentMethod/src/main/java/com/braintreepayments/api/paypalsavedpaymentmethod/imageloader/ImageLoader.kt @@ -0,0 +1,203 @@ +package com.braintreepayments.api.paypalsavedpaymentmethod.imageloader + +import android.graphics.Bitmap +import android.graphics.BitmapFactory +import android.util.LruCache +import android.view.View +import android.widget.ImageView +import androidx.annotation.DrawableRes +import com.braintreepayments.api.sharedutils.HttpClient +import com.braintreepayments.api.sharedutils.Method +import com.braintreepayments.api.sharedutils.OkHttpRequest +import kotlinx.coroutines.CoroutineDispatcher +import kotlinx.coroutines.CoroutineScope +import kotlinx.coroutines.CoroutineStart +import kotlinx.coroutines.Deferred +import kotlinx.coroutines.Dispatchers +import kotlinx.coroutines.Job +import kotlinx.coroutines.SupervisorJob +import kotlinx.coroutines.async +import kotlinx.coroutines.job +import kotlinx.coroutines.launch +import kotlinx.coroutines.sync.Semaphore +import kotlinx.coroutines.sync.withPermit +import kotlinx.coroutines.withContext +import java.io.IOException +import java.lang.ref.WeakReference +import java.util.Collections +import java.util.WeakHashMap +import java.util.concurrent.ConcurrentHashMap + +/** + * Loads funding-instrument icons into memory, de-duplicating concurrent requests for the same URL and bounding how + * many bitmaps are decoded at once. Bytes are fetched through the SDK's pinned [HttpClient]. Dependencies are + * injectable for tests; production code uses [getInstance]. + */ +internal class ImageLoader internal constructor( + private val httpClient: HttpClient = HttpClient(), + private val defaultDispatcher: CoroutineDispatcher = Dispatchers.Default, + private val mainDispatcher: CoroutineDispatcher = Dispatchers.Main.immediate, + private val memoryCache: LruCache = createMemoryCache(), + private val decodeSemaphore: Semaphore = Semaphore(DEFAULT_MAX_PARALLEL_DECODES), +) { + // Deliberately unscoped to any lifecycle: ImageLoader is a process-wide singleton (see getInstance()), so + // in-flight downloads outlive any single screen and are only ever torn down by process death. + private val loaderScope = CoroutineScope(SupervisorJob() + defaultDispatcher) + private val inFlightRequests = ConcurrentHashMap>() + + // Weak keys: the loader is a process-wide singleton and must never keep a view alive. + private val viewRequests = Collections.synchronizedMap(WeakHashMap()) + + private val attachStateListener = object : View.OnAttachStateChangeListener { + override fun onViewAttachedToWindow(v: View) { + val imageView = v as? ImageView ?: return + // Resume a load whose wait was cancelled by a detach, e.g. a RecyclerView row re-attached without re-bind. + val request = viewRequests[imageView] + if (request != null && request.job.isCancelled) startViewRequest(imageView, request.url) + } + + override fun onViewDetachedFromWindow(v: View) { + val imageView = v as? ImageView ?: return + // Keep the record so a re-attach can resume; only the wait is cancelled, the download keeps running. + viewRequests[imageView]?.job?.cancel() + } + } + + /** Returns the cached bitmap for [url] without touching the network, or `null` on a cache miss. */ + fun peek(url: String): Bitmap? = memoryCache.get(url) + + /** + * Returns the bitmap for [url], from memory when cached. Throws [IllegalArgumentException] if [url] is blank or + * malformed, and [IOException] if the download or decode fails. + */ + suspend fun load(url: String): Bitmap { + // TODO: validate with asHttpsUrlOrNull() from UrlValidation.kt (PR #1718) once it is on this branch, so only + // https URLs with a host reach the network. + require(url.isNotBlank()) { "url must not be blank" } + memoryCache.get(url)?.let { return it } + return inFlightRequest(url).await() + } + + /** Binds [url] into [imageView], showing [placeholder] until the image is ready, or permanently if it fails. */ + fun load( + url: String?, + imageView: ImageView, + @DrawableRes placeholder: Int?, + ) { + cancel(imageView) + // TODO: validate with asHttpsUrlOrNull() from UrlValidation.kt (PR #1718) instead of a blank check. + val validUrl = url?.takeIf { it.isNotBlank() } + val cached = validUrl?.let { memoryCache.get(it) } + if (cached != null) { + imageView.setImageBitmap(cached) + return + } + placeholder?.let { imageView.setImageResource(it) } + if (validUrl == null) return + + // Remove-then-add keeps exactly one listener per view, however many times the view is re-bound. + imageView.removeOnAttachStateChangeListener(attachStateListener) + imageView.addOnAttachStateChangeListener(attachStateListener) + startViewRequest(imageView, validUrl) + } + + fun cancel(imageView: ImageView) { + // Symmetric with the add in load(): once a request is fully cancelled there is nothing left to resume on + // a future re-attach, so the view shouldn't keep carrying the listener. + imageView.removeOnAttachStateChangeListener(attachStateListener) + viewRequests.remove(imageView)?.job?.cancel() + } + + private fun inFlightRequest(url: String): Deferred = + inFlightRequests[url] ?: synchronized(inFlightRequests) { + inFlightRequests[url] ?: loaderScope.async(start = CoroutineStart.LAZY) { + fetchAndDecode(url) + }.also { deferred -> + inFlightRequests[url] = deferred + // Registered after the put and before start, so a download that finishes immediately cannot leave + // a stale (possibly failed) entry behind. Waiter cancellation never removes the entry. + deferred.invokeOnCompletion { inFlightRequests.remove(url, deferred) } + deferred.start() + } + } + + private fun startViewRequest(imageView: ImageView, url: String) { + val target = WeakReference(imageView) + val job = loaderScope.launch(mainDispatcher, start = CoroutineStart.LAZY) { + val bitmap = try { + load(url) + } catch (ignored: IOException) { + null // keep the placeholder; no automatic retry + } catch (ignored: IllegalArgumentException) { + null // malformed URL rejected before any request; see the URL validation TODO in load() + } + val view = target.get() ?: return@launch + // Drop the result if the view has been re-bound to a different request since this one started. + if (viewRequests[view]?.job !== coroutineContext.job) return@launch + viewRequests.remove(view) + bitmap?.let(view::setImageBitmap) + } + // Recorded before start so the identity check above always sees this request. + viewRequests[imageView] = ViewRequest(url, job) + job.start() + } + + private suspend fun fetchAndDecode(url: String): Bitmap { + val bytes = httpClient.sendRequestForBytes(OkHttpRequest(url, Method.Get), MAX_IMAGE_BYTES) + val bitmap = withContext(defaultDispatcher) { + decodeSemaphore.withPermit { decodeBitmap(bytes, url) } + } + memoryCache.put(url, bitmap) + return bitmap + } + + // Two-pass decode: read bounds only, then decode a power-of-two subsample close to the display size. + // Reference: https://developer.android.com/topic/performance/graphics/load-bitmap + private fun decodeBitmap(bytes: ByteArray, url: String): Bitmap { + val bounds = BitmapFactory.Options().apply { inJustDecodeBounds = true } + BitmapFactory.decodeByteArray(bytes, 0, bytes.size, bounds) + + val options = BitmapFactory.Options().apply { + inSampleSize = calculateInSampleSize(bounds, MAX_DECODE_DIMENSION_PX, MAX_DECODE_DIMENSION_PX) + } + return BitmapFactory.decodeByteArray(bytes, 0, bytes.size, options) + ?: throw IOException("Unable to decode image from $url") + } + + private fun calculateInSampleSize( + options: BitmapFactory.Options, + reqWidth: Int, + reqHeight: Int, + ): Int { + val height = options.outHeight + val width = options.outWidth + var inSampleSize = 1 + + while (height / inSampleSize > reqHeight || width / inSampleSize > reqWidth) { + inSampleSize *= 2 + } + + return inSampleSize + } + + private class ViewRequest(val url: String, val job: Job) + + companion object { + private const val DEFAULT_CACHE_SIZE_BYTES = 4 * 1024 * 1024 + private const val DEFAULT_MAX_PARALLEL_DECODES = 4 + private const val MAX_DECODE_DIMENSION_PX = 128 + private const val MAX_IMAGE_BYTES = 1024 * 1024L + + private fun createMemoryCache() = object : LruCache(DEFAULT_CACHE_SIZE_BYTES) { + override fun sizeOf(key: String, value: Bitmap): Int = value.byteCount + } + + @Volatile + private var instance: ImageLoader? = null + + fun getInstance(): ImageLoader = + instance ?: synchronized(this) { + instance ?: ImageLoader().also { instance = it } + } + } +} diff --git a/PayPalSavedPaymentMethod/src/main/java/com/braintreepayments/api/paypalsavedpaymentmethod/imageloader/LoaderImage.kt b/PayPalSavedPaymentMethod/src/main/java/com/braintreepayments/api/paypalsavedpaymentmethod/imageloader/LoaderImage.kt new file mode 100644 index 0000000000..fcc29d76b2 --- /dev/null +++ b/PayPalSavedPaymentMethod/src/main/java/com/braintreepayments/api/paypalsavedpaymentmethod/imageloader/LoaderImage.kt @@ -0,0 +1,49 @@ +package com.braintreepayments.api.paypalsavedpaymentmethod.imageloader + +import android.graphics.Bitmap +import androidx.compose.foundation.Image +import androidx.compose.runtime.Composable +import androidx.compose.runtime.LaunchedEffect +import androidx.compose.runtime.getValue +import androidx.compose.runtime.mutableStateOf +import androidx.compose.runtime.remember +import androidx.compose.runtime.setValue +import androidx.compose.ui.Modifier +import androidx.compose.ui.graphics.asImageBitmap +import androidx.compose.ui.graphics.painter.Painter +import java.io.IOException + +/** + * Renders the funding-instrument icon at [url] via [ImageLoader], falling back to [placeholder] + * until the load completes, if it fails, or if [url] is `null` or blank. + * + * The icon is decorative (the FI label is rendered next to it), so it has no content description. + */ +@Composable +internal fun LoaderImage( + url: String?, + placeholder: Painter, + modifier: Modifier = Modifier, + imageLoader: ImageLoader = remember { ImageLoader.getInstance() }, +) { + // Seed from the memory cache so a cached icon renders on the first frame instead of flashing the placeholder. + var bitmap by remember(url) { mutableStateOf(url?.let(imageLoader::peek)) } + + LaunchedEffect(url) { + if (bitmap != null || url.isNullOrBlank()) return@LaunchedEffect + bitmap = try { + imageLoader.load(url) + } catch (ignored: IOException) { + null + } catch (ignored: IllegalArgumentException) { + null + } + } + + val loadedBitmap = bitmap + if (loadedBitmap != null) { + Image(bitmap = loadedBitmap.asImageBitmap(), contentDescription = null, modifier = modifier) + } else { + Image(painter = placeholder, contentDescription = null, modifier = modifier) + } +} diff --git a/PayPalSavedPaymentMethod/src/test/java/com/braintreepayments/api/paypalsavedpaymentmethod/imageloader/ImageLoaderUnitTest.kt b/PayPalSavedPaymentMethod/src/test/java/com/braintreepayments/api/paypalsavedpaymentmethod/imageloader/ImageLoaderUnitTest.kt new file mode 100644 index 0000000000..c5b33f27f7 --- /dev/null +++ b/PayPalSavedPaymentMethod/src/test/java/com/braintreepayments/api/paypalsavedpaymentmethod/imageloader/ImageLoaderUnitTest.kt @@ -0,0 +1,168 @@ +package com.braintreepayments.api.paypalsavedpaymentmethod.imageloader + +import android.graphics.Bitmap +import android.util.LruCache +import com.braintreepayments.api.sharedutils.HttpClient +import com.braintreepayments.api.sharedutils.Method +import com.braintreepayments.api.sharedutils.OkHttpRequest +import io.mockk.coEvery +import io.mockk.coVerify +import io.mockk.mockk +import io.mockk.slot +import kotlinx.coroutines.CompletableDeferred +import kotlinx.coroutines.ExperimentalCoroutinesApi +import kotlinx.coroutines.async +import kotlinx.coroutines.awaitAll +import kotlinx.coroutines.launch +import kotlinx.coroutines.sync.Semaphore +import kotlinx.coroutines.test.StandardTestDispatcher +import kotlinx.coroutines.test.advanceUntilIdle +import kotlinx.coroutines.test.runCurrent +import kotlinx.coroutines.test.runTest +import org.junit.Assert.assertEquals +import org.junit.Assert.assertFalse +import org.junit.Assert.assertNull +import org.junit.Assert.assertSame +import org.junit.Test +import org.junit.runner.RunWith +import org.robolectric.RobolectricTestRunner +import org.robolectric.annotation.GraphicsMode +import java.io.IOException +import kotlin.test.assertFailsWith + +@OptIn(ExperimentalCoroutinesApi::class) +@RunWith(RobolectricTestRunner::class) +@GraphicsMode(GraphicsMode.Mode.NATIVE) +class ImageLoaderUnitTest { + + private val testDispatcher = StandardTestDispatcher() + private val httpClient = mockk() + private var sut = createImageLoader() + + @Test + fun `load sends a size-limited GET and returns the bitmap without upscaling`() = runTest(testDispatcher) { + val request = slot() + coEvery { httpClient.sendRequestForBytes(capture(request), 1024 * 1024L) } returns pngBytes(32, 32) + + val bitmap = sut.load(URL) + + assertEquals(32, bitmap.width) + assertEquals(URL, request.captured.url) + assertEquals(Method.Get, request.captured.method) + } + + @Test + fun `load serves a repeated url from the memory cache`() = runTest(testDispatcher) { + givenImage(32) + assertNull(sut.peek(URL)) + + val first = sut.load(URL) + + assertSame(first, sut.peek(URL)) + assertSame(first, sut.load(URL)) + coVerify(exactly = 1) { httpClient.sendRequestForBytes(any(), any()) } + } + + @Test + fun `concurrent loads of the same url share one request`() = runTest(testDispatcher) { + givenImage(32) + + val results = listOf(async { sut.load(URL) }, async { sut.load(URL) }).awaitAll() + + assertSame(results[0], results[1]) + coVerify(exactly = 1) { httpClient.sendRequestForBytes(any(), any()) } + } + + @Test + fun `cancelling a waiter does not start a second request for the same url`() = runTest(testDispatcher) { + val gate = CompletableDeferred() + coEvery { httpClient.sendRequestForBytes(any(), any()) } coAnswers { + gate.await() + pngBytes(32, 32) + } + + val firstWaiter = launch { sut.load(URL) } + runCurrent() + firstWaiter.cancel() + val secondWaiter = async { sut.load(URL) } + runCurrent() + gate.complete(Unit) + + assertEquals(32, secondWaiter.await().width) + coVerify(exactly = 1) { httpClient.sendRequestForBytes(any(), any()) } + } + + @Test + fun `a failed load is not cached and the next load makes a new request`() = runTest(testDispatcher) { + var calls = 0 + coEvery { httpClient.sendRequestForBytes(any(), any()) } answers { + if (++calls == 1) throw IOException("Unexpected code 500") else pngBytes(32, 32) + } + + assertFailsWith { sut.load(URL) } + + assertEquals(32, sut.load(URL).width) + coVerify(exactly = 2) { httpClient.sendRequestForBytes(any(), any()) } + } + + @Test + fun `load throws IOException and caches nothing when the bytes cannot be decoded`() = runTest(testDispatcher) { + coEvery { httpClient.sendRequestForBytes(any(), any()) } returns byteArrayOf(1, 2, 3) + + assertFailsWith { sut.load(URL) } + assertNull(sut.peek(URL)) + } + + @Test + fun `load throws IllegalArgumentException for a blank url without making a request`() = runTest(testDispatcher) { + assertFailsWith { sut.load("") } + assertFailsWith { sut.load(" ") } + + coVerify(exactly = 0) { httpClient.sendRequestForBytes(any(), any()) } + } + + @Test + fun `load downsamples a large image to at most 128 px`() = runTest(testDispatcher) { + givenImage(1024) + + val bitmap = sut.load(URL) + + assertEquals(128, bitmap.width) + assertEquals(128, bitmap.height) + } + + @Test + fun `decoding waits for a decode permit`() = runTest(testDispatcher) { + val decodeSemaphore = Semaphore(1).apply { acquire() } + sut = createImageLoader(decodeSemaphore) + givenImage(32) + + val result = async { sut.load(URL) } + advanceUntilIdle() + assertFalse(result.isCompleted) + + decodeSemaphore.release() + assertEquals(32, result.await().width) + } + + @Test + fun `getInstance always returns the same loader`() { + assertSame(ImageLoader.getInstance(), ImageLoader.getInstance()) + } + + private fun givenImage(size: Int) { + coEvery { httpClient.sendRequestForBytes(any(), any()) } returns pngBytes(size, size) + } + + private fun createImageLoader(decodeSemaphore: Semaphore = Semaphore(4)) = ImageLoader( + httpClient = httpClient, + defaultDispatcher = testDispatcher, + mainDispatcher = testDispatcher, + memoryCache = LruCache(4 * 1024 * 1024), + decodeSemaphore = decodeSemaphore, + ) + + companion object { + private const val URL = "https://www.paypalobjects.com/images/visa.png" + } +} diff --git a/PayPalSavedPaymentMethod/src/test/java/com/braintreepayments/api/paypalsavedpaymentmethod/imageloader/ImageLoaderViewBindingUnitTest.kt b/PayPalSavedPaymentMethod/src/test/java/com/braintreepayments/api/paypalsavedpaymentmethod/imageloader/ImageLoaderViewBindingUnitTest.kt new file mode 100644 index 0000000000..abeadaf480 --- /dev/null +++ b/PayPalSavedPaymentMethod/src/test/java/com/braintreepayments/api/paypalsavedpaymentmethod/imageloader/ImageLoaderViewBindingUnitTest.kt @@ -0,0 +1,145 @@ +package com.braintreepayments.api.paypalsavedpaymentmethod.imageloader + +import android.app.Activity +import android.graphics.Bitmap +import android.graphics.drawable.BitmapDrawable +import android.util.LruCache +import android.widget.FrameLayout +import android.widget.ImageView +import com.braintreepayments.api.paypalsavedpaymentmethod.R +import com.braintreepayments.api.sharedutils.HttpClient +import io.mockk.coEvery +import io.mockk.coVerify +import io.mockk.mockk +import kotlinx.coroutines.ExperimentalCoroutinesApi +import kotlinx.coroutines.sync.Semaphore +import kotlinx.coroutines.test.StandardTestDispatcher +import kotlinx.coroutines.test.runTest +import org.junit.Assert.assertEquals +import org.junit.Assert.assertFalse +import org.junit.Assert.assertNotNull +import org.junit.Test +import org.junit.runner.RunWith +import org.robolectric.Robolectric +import org.robolectric.RobolectricTestRunner +import org.robolectric.annotation.GraphicsMode +import java.io.IOException + +@OptIn(ExperimentalCoroutinesApi::class) +@RunWith(RobolectricTestRunner::class) +@GraphicsMode(GraphicsMode.Mode.NATIVE) +class ImageLoaderViewBindingUnitTest { + + private val testDispatcher = StandardTestDispatcher() + private val httpClient = mockk() + private val sut = ImageLoader( + httpClient = httpClient, + defaultDispatcher = testDispatcher, + mainDispatcher = testDispatcher, + memoryCache = LruCache(4 * 1024 * 1024), + decodeSemaphore = Semaphore(4), + ) + private val activity = Robolectric.buildActivity(Activity::class.java).setup().get() + private val imageView = ImageView(activity).also { activity.setContentView(it) } + + @Test + fun `load shows the placeholder and then the downloaded bitmap`() { + givenImage(32) + + sut.load(URL, imageView, PLACEHOLDER) + assertShowsPlaceholder() + + advanceUntilIdle() + assertShowsBitmapOfWidth(32) + } + + @Test + fun `load sets a cached bitmap immediately`() = runTest(testDispatcher) { + givenImage(32) + sut.load(URL) + + sut.load(URL, imageView, PLACEHOLDER) + + assertShowsBitmapOfWidth(32) + } + + @Test + fun `load keeps the placeholder for a null or blank url without making a request`() { + listOf(null, " ").forEach { url -> + sut.load(url, imageView, PLACEHOLDER) + advanceUntilIdle() + assertShowsPlaceholder() + } + coVerify(exactly = 0) { httpClient.sendRequestForBytes(any(), any()) } + } + + @Test + fun `load keeps the placeholder when the download fails or the url is rejected as malformed`() { + listOf(IOException("Unexpected code 404"), IllegalArgumentException("Invalid URL host")).forEach { error -> + coEvery { httpClient.sendRequestForBytes(any(), any()) } throws error + + sut.load(URL, imageView, PLACEHOLDER) + advanceUntilIdle() + + assertShowsPlaceholder() + } + } + + @Test + fun `re-binding the view before the first load completes shows only the latest image`() { + coEvery { httpClient.sendRequestForBytes(match { it.url == URL }, any()) } returns pngBytes(16, 16) + coEvery { httpClient.sendRequestForBytes(match { it.url == OTHER_URL }, any()) } returns pngBytes(48, 48) + + sut.load(URL, imageView, PLACEHOLDER) + sut.load(OTHER_URL, imageView, PLACEHOLDER) + advanceUntilIdle() + + assertShowsBitmapOfWidth(48) + } + + @Test + fun `a detached view resumes its load when re-attached`() { + givenImage(32) + + sut.load(URL, imageView, PLACEHOLDER) + activity.setContentView(FrameLayout(activity)) + advanceUntilIdle() + assertShowsPlaceholder() + + activity.setContentView(imageView) + advanceUntilIdle() + assertShowsBitmapOfWidth(32) + } + + @Test + fun `cancel stops the bitmap from being set`() { + givenImage(32) + + sut.load(URL, imageView, PLACEHOLDER) + sut.cancel(imageView) + advanceUntilIdle() + + assertShowsPlaceholder() + } + + private fun givenImage(size: Int) { + coEvery { httpClient.sendRequestForBytes(any(), any()) } returns pngBytes(size, size) + } + + private fun advanceUntilIdle() = testDispatcher.scheduler.advanceUntilIdle() + + private fun assertShowsPlaceholder() { + assertNotNull(imageView.drawable) + assertFalse(imageView.drawable is BitmapDrawable) + } + + private fun assertShowsBitmapOfWidth(width: Int) { + assertEquals(width, (imageView.drawable as BitmapDrawable).bitmap.width) + } + + companion object { + private const val URL = "https://www.paypalobjects.com/images/visa.png" + private const val OTHER_URL = "https://www.paypalobjects.com/images/bank.png" + private val PLACEHOLDER = R.drawable.ic_fi_card_placeholder + } +} diff --git a/PayPalSavedPaymentMethod/src/test/java/com/braintreepayments/api/paypalsavedpaymentmethod/imageloader/TestImages.kt b/PayPalSavedPaymentMethod/src/test/java/com/braintreepayments/api/paypalsavedpaymentmethod/imageloader/TestImages.kt new file mode 100644 index 0000000000..912e298f21 --- /dev/null +++ b/PayPalSavedPaymentMethod/src/test/java/com/braintreepayments/api/paypalsavedpaymentmethod/imageloader/TestImages.kt @@ -0,0 +1,13 @@ +package com.braintreepayments.api.paypalsavedpaymentmethod.imageloader + +import android.graphics.Bitmap +import java.io.ByteArrayOutputStream + +/** Encodes a solid [width] x [height] PNG. Requires Robolectric's native graphics mode to produce real bytes. */ +internal fun pngBytes(width: Int, height: Int): ByteArray { + val bitmap = Bitmap.createBitmap(width, height, Bitmap.Config.ARGB_8888) + return ByteArrayOutputStream().use { stream -> + bitmap.compress(Bitmap.CompressFormat.PNG, 100, stream) + stream.toByteArray() + } +} From d2ac1b4fc266bcea8121a4d55fbc802208502be9 Mon Sep 17 00:00:00 2001 From: Bala Mani Nayar D Date: Wed, 14 Oct 2026 10:44:20 +0530 Subject: [PATCH 03/15] fix(PayPalSavedPaymentMethod): address PR review feedback on ImageLoader Clear the ImageView when no placeholder is given and nothing is cached, instead of leaving a recycled row showing a stale image while the new URL loads. Document the rationale behind the cache size, decode concurrency, decode dimension, and body size constants. Addresses review comments from @anibalb2500 on PR #1721. Co-Authored-By: Claude Sonnet 5 --- .../imageloader/ImageLoader.kt | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/PayPalSavedPaymentMethod/src/main/java/com/braintreepayments/api/paypalsavedpaymentmethod/imageloader/ImageLoader.kt b/PayPalSavedPaymentMethod/src/main/java/com/braintreepayments/api/paypalsavedpaymentmethod/imageloader/ImageLoader.kt index 70de2b5180..e2e176a7df 100644 --- a/PayPalSavedPaymentMethod/src/main/java/com/braintreepayments/api/paypalsavedpaymentmethod/imageloader/ImageLoader.kt +++ b/PayPalSavedPaymentMethod/src/main/java/com/braintreepayments/api/paypalsavedpaymentmethod/imageloader/ImageLoader.kt @@ -92,7 +92,12 @@ internal class ImageLoader internal constructor( imageView.setImageBitmap(cached) return } - placeholder?.let { imageView.setImageResource(it) } + // Clear any stale image (e.g. from a recycled RecyclerView row) when no placeholder is given. + if (placeholder != null) { + imageView.setImageResource(placeholder) + } else { + imageView.setImageDrawable(null) + } if (validUrl == null) return // Remove-then-add keeps exactly one listener per view, however many times the view is re-bound. @@ -183,9 +188,16 @@ internal class ImageLoader internal constructor( private class ViewRequest(val url: String, val job: Job) companion object { + // 4 MB keeps a reasonable number of small FI icons resident without pressuring low-memory devices. private const val DEFAULT_CACHE_SIZE_BYTES = 4 * 1024 * 1024 + + // Bounds concurrent BitmapFactory decodes so many icons loading at once can't spike memory/CPU. private const val DEFAULT_MAX_PARALLEL_DECODES = 4 + + // FI icons never render larger than this in any current layout; decoding beyond it wastes memory. private const val MAX_DECODE_DIMENSION_PX = 128 + + // Generous ceiling for a small icon; guards against an unexpectedly large or malicious response. private const val MAX_IMAGE_BYTES = 1024 * 1024L private fun createMemoryCache() = object : LruCache(DEFAULT_CACHE_SIZE_BYTES) { From 154b7b09aee326677753db5a336e7e358d013dd2 Mon Sep 17 00:00:00 2001 From: Bala Mani Nayar D Date: Wed, 14 Oct 2026 18:25:56 +0530 Subject: [PATCH 04/15] fix(PayPalSavedPaymentMethod): address PR review feedback on ImageLoader - Catch Exception (rethrowing CancellationException) in the view-binding load path so an unexpected failure (e.g. a RuntimeException from a malformed decode) keeps the placeholder instead of crashing the host app under SupervisorJob. - Document that the loader is raster-only (no SVG support), matching standard Android image loading in general. - Document that load() and cancel() must be called on the main thread. - Add a CHANGELOG entry for SharedUtils#sendRequestForBytes. Addresses review comments from @Bhuvana-S100 on PR #1721. Co-Authored-By: Claude Sonnet 5 --- CHANGELOG.md | 2 ++ .../imageloader/ImageLoader.kt | 15 ++++++++++++++- 2 files changed, 16 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index cd93cf3a21..0acb74a006 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,8 @@ * Add optional `resultKey` parameter to `ThreeDSecureLauncher` constructors, allowing a unique registration key per launcher instance * SEPADirectDebit * Add Compose constructor for `SEPADirectDebitLauncher` +* SharedUtils + * Add `HttpClient#sendRequestForBytes` for fetching binary payloads with a size cap, enforced via streaming even when the response omits `Content-Length` ## 5.33.0 (2026-09-16) diff --git a/PayPalSavedPaymentMethod/src/main/java/com/braintreepayments/api/paypalsavedpaymentmethod/imageloader/ImageLoader.kt b/PayPalSavedPaymentMethod/src/main/java/com/braintreepayments/api/paypalsavedpaymentmethod/imageloader/ImageLoader.kt index e2e176a7df..72818e5255 100644 --- a/PayPalSavedPaymentMethod/src/main/java/com/braintreepayments/api/paypalsavedpaymentmethod/imageloader/ImageLoader.kt +++ b/PayPalSavedPaymentMethod/src/main/java/com/braintreepayments/api/paypalsavedpaymentmethod/imageloader/ImageLoader.kt @@ -27,11 +27,16 @@ import java.lang.ref.WeakReference import java.util.Collections import java.util.WeakHashMap import java.util.concurrent.ConcurrentHashMap +import kotlin.coroutines.cancellation.CancellationException /** * Loads funding-instrument icons into memory, de-duplicating concurrent requests for the same URL and bounding how * many bitmaps are decoded at once. Bytes are fetched through the SDK's pinned [HttpClient]. Dependencies are * injectable for tests; production code uses [getInstance]. + * + * Decodes raster formats only (PNG, JPEG, etc.) via [android.graphics.BitmapFactory]. There is currently no image + * loader in Braintree Native, and this custom implementation does not support SVG, matching standard Android image + * loading in general (BitmapFactory, and Coil/Glide without an SVG add-on). */ internal class ImageLoader internal constructor( private val httpClient: HttpClient = HttpClient(), @@ -78,7 +83,11 @@ internal class ImageLoader internal constructor( return inFlightRequest(url).await() } - /** Binds [url] into [imageView], showing [placeholder] until the image is ready, or permanently if it fails. */ + /** + * Binds [url] into [imageView], showing [placeholder] until the image is ready, or permanently if it fails. + * + * Must be called on the main thread; it updates [imageView] directly. + */ fun load( url: String?, imageView: ImageView, @@ -106,6 +115,7 @@ internal class ImageLoader internal constructor( startViewRequest(imageView, validUrl) } + /** Cancels any pending load for [imageView]. Must be called on the main thread. */ fun cancel(imageView: ImageView) { // Symmetric with the add in load(): once a request is fully cancelled there is nothing left to resume on // a future re-attach, so the view shouldn't keep carrying the listener. @@ -135,6 +145,9 @@ internal class ImageLoader internal constructor( null // keep the placeholder; no automatic retry } catch (ignored: IllegalArgumentException) { null // malformed URL rejected before any request; see the URL validation TODO in load() + } catch (ignored: Exception) { + if (ignored is CancellationException) throw ignored + null // unexpected failure (e.g. a malformed response the decoder can't handle); keep the placeholder } val view = target.get() ?: return@launch // Drop the result if the view has been re-bound to a different request since this one started. From fc93da3e47b17aeaed56134cd648211d0cad8fa2 Mon Sep 17 00:00:00 2001 From: Bala Mani Nayar D Date: Wed, 14 Oct 2026 18:26:18 +0530 Subject: [PATCH 05/15] test(PayPalSavedPaymentMethod): add Compose tests for LoaderImage Adds androidx.compose.ui:ui-test-junit4/ui-test-manifest as test-only dependencies (Robolectric-based, reusing the existing MockK/HttpClient test setup) and a LoaderImageUnitTest covering placeholder-to-image, cached first frame (no placeholder flash), load failure, and a URL change triggering a fresh load. LoaderImage gets two test-only testTag modifiers (loaded vs placeholder) since both Image() calls intentionally use contentDescription = null for this decorative icon, leaving no other way to distinguish them. Addresses review comment from @Bhuvana-S100 on PR #1721. Co-Authored-By: Claude Sonnet 5 --- PayPalSavedPaymentMethod/build.gradle | 3 + .../imageloader/LoaderImage.kt | 17 ++- .../imageloader/LoaderImageUnitTest.kt | 119 ++++++++++++++++++ 3 files changed, 137 insertions(+), 2 deletions(-) create mode 100644 PayPalSavedPaymentMethod/src/test/java/com/braintreepayments/api/paypalsavedpaymentmethod/imageloader/LoaderImageUnitTest.kt diff --git a/PayPalSavedPaymentMethod/build.gradle b/PayPalSavedPaymentMethod/build.gradle index 28ee3610ff..d8d7d133cf 100644 --- a/PayPalSavedPaymentMethod/build.gradle +++ b/PayPalSavedPaymentMethod/build.gradle @@ -71,6 +71,9 @@ dependencies { testImplementation libs.test.parameter.injector testImplementation project(':TestUtils') testImplementation libs.coroutines.test + testImplementation composeBom + testImplementation libs.androidx.compose.ui.test.junit4 + testImplementation libs.androidx.compose.ui.test.manifest debugImplementation libs.androidx.ui.tooling androidTestImplementation project(':TestUtils') diff --git a/PayPalSavedPaymentMethod/src/main/java/com/braintreepayments/api/paypalsavedpaymentmethod/imageloader/LoaderImage.kt b/PayPalSavedPaymentMethod/src/main/java/com/braintreepayments/api/paypalsavedpaymentmethod/imageloader/LoaderImage.kt index fcc29d76b2..68a16408ef 100644 --- a/PayPalSavedPaymentMethod/src/main/java/com/braintreepayments/api/paypalsavedpaymentmethod/imageloader/LoaderImage.kt +++ b/PayPalSavedPaymentMethod/src/main/java/com/braintreepayments/api/paypalsavedpaymentmethod/imageloader/LoaderImage.kt @@ -11,6 +11,7 @@ import androidx.compose.runtime.setValue import androidx.compose.ui.Modifier import androidx.compose.ui.graphics.asImageBitmap import androidx.compose.ui.graphics.painter.Painter +import androidx.compose.ui.platform.testTag import java.io.IOException /** @@ -42,8 +43,20 @@ internal fun LoaderImage( val loadedBitmap = bitmap if (loadedBitmap != null) { - Image(bitmap = loadedBitmap.asImageBitmap(), contentDescription = null, modifier = modifier) + Image( + bitmap = loadedBitmap.asImageBitmap(), + contentDescription = null, + modifier = modifier.testTag(LOADED_IMAGE_TEST_TAG) + ) } else { - Image(painter = placeholder, contentDescription = null, modifier = modifier) + Image( + painter = placeholder, + contentDescription = null, + modifier = modifier.testTag(PLACEHOLDER_IMAGE_TEST_TAG) + ) } } + +// No semantic distinction exists otherwise, since the icon is decorative (contentDescription = null above). +internal const val LOADED_IMAGE_TEST_TAG = "loader_image_loaded" +internal const val PLACEHOLDER_IMAGE_TEST_TAG = "loader_image_placeholder" diff --git a/PayPalSavedPaymentMethod/src/test/java/com/braintreepayments/api/paypalsavedpaymentmethod/imageloader/LoaderImageUnitTest.kt b/PayPalSavedPaymentMethod/src/test/java/com/braintreepayments/api/paypalsavedpaymentmethod/imageloader/LoaderImageUnitTest.kt new file mode 100644 index 0000000000..f930b2dc0c --- /dev/null +++ b/PayPalSavedPaymentMethod/src/test/java/com/braintreepayments/api/paypalsavedpaymentmethod/imageloader/LoaderImageUnitTest.kt @@ -0,0 +1,119 @@ +package com.braintreepayments.api.paypalsavedpaymentmethod.imageloader + +import android.graphics.Bitmap +import android.util.LruCache +import androidx.compose.runtime.getValue +import androidx.compose.runtime.mutableStateOf +import androidx.compose.runtime.setValue +import androidx.compose.ui.graphics.Color +import androidx.compose.ui.graphics.painter.ColorPainter +import androidx.compose.ui.test.junit4.createComposeRule +import androidx.compose.ui.test.onNodeWithTag +import com.braintreepayments.api.sharedutils.HttpClient +import io.mockk.coEvery +import io.mockk.coVerify +import io.mockk.mockk +import kotlinx.coroutines.ExperimentalCoroutinesApi +import kotlinx.coroutines.sync.Semaphore +import kotlinx.coroutines.test.StandardTestDispatcher +import kotlinx.coroutines.test.runTest +import org.junit.Rule +import org.junit.Test +import org.junit.runner.RunWith +import org.robolectric.RobolectricTestRunner +import org.robolectric.annotation.GraphicsMode +import java.io.IOException + +@OptIn(ExperimentalCoroutinesApi::class) +@RunWith(RobolectricTestRunner::class) +@GraphicsMode(GraphicsMode.Mode.NATIVE) +class LoaderImageUnitTest { + + @get:Rule + val composeTestRule = createComposeRule() + + private val testDispatcher = StandardTestDispatcher() + private val httpClient = mockk() + private val sut = ImageLoader( + httpClient = httpClient, + defaultDispatcher = testDispatcher, + mainDispatcher = testDispatcher, + memoryCache = LruCache(4 * 1024 * 1024), + decodeSemaphore = Semaphore(4), + ) + private val placeholder = ColorPainter(Color.Red) + + @Test + fun `shows the placeholder then the loaded image`() { + coEvery { httpClient.sendRequestForBytes(any(), any()) } returns pngBytes(32, 32) + + composeTestRule.setContent { + LoaderImage(url = URL, placeholder = placeholder, imageLoader = sut) + } + + composeTestRule.onNodeWithTag(PLACEHOLDER_IMAGE_TEST_TAG).assertExists() + composeTestRule.onNodeWithTag(LOADED_IMAGE_TEST_TAG).assertDoesNotExist() + + testDispatcher.scheduler.advanceUntilIdle() + composeTestRule.waitForIdle() + + composeTestRule.onNodeWithTag(LOADED_IMAGE_TEST_TAG).assertExists() + composeTestRule.onNodeWithTag(PLACEHOLDER_IMAGE_TEST_TAG).assertDoesNotExist() + } + + @Test + fun `shows the cached image on the first frame without flashing the placeholder`() = runTest(testDispatcher) { + coEvery { httpClient.sendRequestForBytes(any(), any()) } returns pngBytes(32, 32) + sut.load(URL) + + composeTestRule.setContent { + LoaderImage(url = URL, placeholder = placeholder, imageLoader = sut) + } + + composeTestRule.onNodeWithTag(LOADED_IMAGE_TEST_TAG).assertExists() + composeTestRule.onNodeWithTag(PLACEHOLDER_IMAGE_TEST_TAG).assertDoesNotExist() + } + + @Test + fun `keeps the placeholder when the load fails`() { + coEvery { httpClient.sendRequestForBytes(any(), any()) } throws IOException("boom") + + composeTestRule.setContent { + LoaderImage(url = URL, placeholder = placeholder, imageLoader = sut) + } + testDispatcher.scheduler.advanceUntilIdle() + composeTestRule.waitForIdle() + + composeTestRule.onNodeWithTag(PLACEHOLDER_IMAGE_TEST_TAG).assertExists() + composeTestRule.onNodeWithTag(LOADED_IMAGE_TEST_TAG).assertDoesNotExist() + } + + @Test + fun `loads the new image after the url changes`() { + coEvery { httpClient.sendRequestForBytes(match { it.url == URL }, any()) } returns pngBytes(16, 16) + coEvery { httpClient.sendRequestForBytes(match { it.url == OTHER_URL }, any()) } returns pngBytes(48, 48) + + var currentUrl by mutableStateOf(URL) + composeTestRule.setContent { + LoaderImage(url = currentUrl, placeholder = placeholder, imageLoader = sut) + } + testDispatcher.scheduler.advanceUntilIdle() + composeTestRule.waitForIdle() + composeTestRule.onNodeWithTag(LOADED_IMAGE_TEST_TAG).assertExists() + + currentUrl = OTHER_URL + // Recomposition from an external state write only happens once Compose processes a frame; only then does + // the new LaunchedEffect start and queue work on testDispatcher for advanceUntilIdle() to drain. + composeTestRule.waitForIdle() + testDispatcher.scheduler.advanceUntilIdle() + composeTestRule.waitForIdle() + + composeTestRule.onNodeWithTag(LOADED_IMAGE_TEST_TAG).assertExists() + coVerify(exactly = 1) { httpClient.sendRequestForBytes(match { it.url == OTHER_URL }, any()) } + } + + private companion object { + const val URL = "https://www.paypalobjects.com/images/visa.png" + const val OTHER_URL = "https://www.paypalobjects.com/images/bank.png" + } +} From 2d1228c47e13392a8e1704a3f0420310d9fa4943 Mon Sep 17 00:00:00 2001 From: Bala Mani Nayar D Date: Fri, 16 Oct 2026 09:56:14 +0530 Subject: [PATCH 06/15] fix(PayPalSavedPaymentMethod): address Copilot review on ImageLoader - ImageLoader re-checks the memory cache before starting a download, so a URL cached just after the caller's cache miss isn't downloaded again - executeRequestForBytes fails if an https request is redirected to a non-https URL - the maxBodyBytes read limit saturates at Long.MAX_VALUE instead of overflowing --- .../imageloader/ImageLoader.kt | 3 ++- .../OkHttpSynchronousHttpClient.kt | 11 ++++++--- .../OkHttpSynchronousHttpClientTest.kt | 23 ++++++++++++++++++- 3 files changed, 32 insertions(+), 5 deletions(-) diff --git a/PayPalSavedPaymentMethod/src/main/java/com/braintreepayments/api/paypalsavedpaymentmethod/imageloader/ImageLoader.kt b/PayPalSavedPaymentMethod/src/main/java/com/braintreepayments/api/paypalsavedpaymentmethod/imageloader/ImageLoader.kt index 72818e5255..56346966f5 100644 --- a/PayPalSavedPaymentMethod/src/main/java/com/braintreepayments/api/paypalsavedpaymentmethod/imageloader/ImageLoader.kt +++ b/PayPalSavedPaymentMethod/src/main/java/com/braintreepayments/api/paypalsavedpaymentmethod/imageloader/ImageLoader.kt @@ -126,7 +126,8 @@ internal class ImageLoader internal constructor( private fun inFlightRequest(url: String): Deferred = inFlightRequests[url] ?: synchronized(inFlightRequests) { inFlightRequests[url] ?: loaderScope.async(start = CoroutineStart.LAZY) { - fetchAndDecode(url) + // Another download may have cached this URL after the caller's cache miss and before this started. + memoryCache.get(url) ?: fetchAndDecode(url) }.also { deferred -> inFlightRequests[url] = deferred // Registered after the put and before start, so a download that finishes immediately cannot leave diff --git a/SharedUtils/src/main/java/com/braintreepayments/api/sharedutils/OkHttpSynchronousHttpClient.kt b/SharedUtils/src/main/java/com/braintreepayments/api/sharedutils/OkHttpSynchronousHttpClient.kt index a53fba5cc8..8e5325f2a1 100644 --- a/SharedUtils/src/main/java/com/braintreepayments/api/sharedutils/OkHttpSynchronousHttpClient.kt +++ b/SharedUtils/src/main/java/com/braintreepayments/api/sharedutils/OkHttpSynchronousHttpClient.kt @@ -65,8 +65,8 @@ internal class OkHttpSynchronousHttpClient( * `Content-Length` exceeds it, or, if undeclared (e.g. chunked transfer), as soon as actual bytes read exceed * it — the body is never buffered past [maxBodyBytes] + 1 bytes. * @return The response body bytes. - * @throws IOException if the request fails, the response is unsuccessful or has no body, or the body size - * exceeds [maxBodyBytes]. + * @throws IOException if the request fails, the response is unsuccessful or has no body, an https request is + * redirected to a non-https URL, or the body size exceeds [maxBodyBytes]. * @throws IllegalArgumentException if the request URL is not a valid http or https URL. */ @Throws(IOException::class) @@ -76,6 +76,10 @@ internal class OkHttpSynchronousHttpClient( okHttpClient.newCall(request).execute().use { response -> val responseBody = response.body?.takeIf { response.isSuccessful } ?: throw IOException("Unexpected code ${response.code} for ${request.url}") + // OkHttp follows redirects, including https to http; never accept a downgraded download. + if (request.url.isHttps && !response.request.url.isHttps) { + throw IOException("Redirected from https to a non-https URL for ${request.url}") + } if (maxBodyBytes == null) return responseBody.bytes() return responseBody.readBoundedBytes(maxBodyBytes, request.url.toString()) } @@ -86,7 +90,8 @@ internal class OkHttpSynchronousHttpClient( private fun ResponseBody.readBoundedBytes(maxBodyBytes: Long, url: String): ByteArray { if (contentLength() > maxBodyBytes) throwBodyTooLarge(maxBodyBytes, url) - val limit = maxBodyBytes + 1 + // Saturated so a limit of Long.MAX_VALUE can't overflow to a negative read limit. + val limit = if (maxBodyBytes == Long.MAX_VALUE) maxBodyBytes else maxBodyBytes + 1 val buffer = Buffer() val source = source() while (buffer.size < limit && source.read(buffer, limit - buffer.size) != -1L) { diff --git a/SharedUtils/src/test/java/com/braintreepayments/api/sharedutils/OkHttpSynchronousHttpClientTest.kt b/SharedUtils/src/test/java/com/braintreepayments/api/sharedutils/OkHttpSynchronousHttpClientTest.kt index 5b66cbdc32..9184f81c03 100644 --- a/SharedUtils/src/test/java/com/braintreepayments/api/sharedutils/OkHttpSynchronousHttpClientTest.kt +++ b/SharedUtils/src/test/java/com/braintreepayments/api/sharedutils/OkHttpSynchronousHttpClientTest.kt @@ -180,11 +180,32 @@ class OkHttpSynchronousHttpClientTest { } } - private fun givenBytesResponse(isSuccessful: Boolean, body: ResponseBody?) { + @Test + fun `when an https request is redirected to http, executeRequestForBytes throws IOException`() { + givenBytesResponse(isSuccessful = true, body = byteArrayOf(1).toResponseBody(), finalUrl = "http://example.com") + + assertFailsWith { sut.executeRequestForBytes(OkHttpRequest(URL, Method.Get)) } + } + + @Test + fun `when maxBodyBytes is Long MAX_VALUE, executeRequestForBytes returns the body`() { + val bytes = ByteArray(1024) { 7 } + givenBytesResponse(isSuccessful = true, body = responseBody) + every { responseBody.contentLength() } returns -1L + every { responseBody.source() } returns Buffer().write(bytes) + + val result = sut.executeRequestForBytes(OkHttpRequest(URL, Method.Get), maxBodyBytes = Long.MAX_VALUE) + + assertArrayEquals(bytes, result) + } + + private fun givenBytesResponse(isSuccessful: Boolean, body: ResponseBody?, finalUrl: String = URL) { every { okHttpClient.newCall(any()) } returns call every { call.execute() } returns response every { response.isSuccessful } returns isSuccessful every { response.body } returns body + // The request OkHttp ended on after following any redirects. + every { response.request } returns Request.Builder().url(finalUrl).build() } companion object { From c06a61b53acf1f58cc065e12d64d82d886716d0c Mon Sep 17 00:00:00 2001 From: Bala Mani Nayar D Date: Mon, 19 Oct 2026 18:36:36 +0530 Subject: [PATCH 07/15] fix(PayPalSavedPaymentMethod): show a loading state instead of the fallback icon while an image loads LoaderImage had one "no image" state, so the fallback icon showed for the whole download. It now has three: loading draws a slot that is blank by default, and the placeholder shows only when the load fails or there is no URL. The ImageView load() gets the same split: loadingPlaceholder while loading and errorPlaceholder on failure or a missing URL, each blank when null. --- .../imageloader/ImageLoader.kt | 36 ++++++----- .../imageloader/LoaderImage.kt | 47 ++++++++++----- .../ImageLoaderViewBindingUnitTest.kt | 57 +++++++++++++----- .../imageloader/LoaderImageUnitTest.kt | 59 ++++++++++++++++++- 4 files changed, 153 insertions(+), 46 deletions(-) diff --git a/PayPalSavedPaymentMethod/src/main/java/com/braintreepayments/api/paypalsavedpaymentmethod/imageloader/ImageLoader.kt b/PayPalSavedPaymentMethod/src/main/java/com/braintreepayments/api/paypalsavedpaymentmethod/imageloader/ImageLoader.kt index 56346966f5..f47795145c 100644 --- a/PayPalSavedPaymentMethod/src/main/java/com/braintreepayments/api/paypalsavedpaymentmethod/imageloader/ImageLoader.kt +++ b/PayPalSavedPaymentMethod/src/main/java/com/braintreepayments/api/paypalsavedpaymentmethod/imageloader/ImageLoader.kt @@ -58,7 +58,9 @@ internal class ImageLoader internal constructor( val imageView = v as? ImageView ?: return // Resume a load whose wait was cancelled by a detach, e.g. a RecyclerView row re-attached without re-bind. val request = viewRequests[imageView] - if (request != null && request.job.isCancelled) startViewRequest(imageView, request.url) + if (request != null && request.job.isCancelled) { + startViewRequest(imageView, request.url, request.errorPlaceholder) + } } override fun onViewDetachedFromWindow(v: View) { @@ -84,14 +86,17 @@ internal class ImageLoader internal constructor( } /** - * Binds [url] into [imageView], showing [placeholder] until the image is ready, or permanently if it fails. + * Binds [url] into [imageView]. While the image loads, the view shows [loadingPlaceholder], or stays blank when it + * is `null`. If [url] is missing or blank, or the load fails, it shows [errorPlaceholder], or stays blank when it + * is `null`. * * Must be called on the main thread; it updates [imageView] directly. */ fun load( url: String?, imageView: ImageView, - @DrawableRes placeholder: Int?, + @DrawableRes loadingPlaceholder: Int? = null, + @DrawableRes errorPlaceholder: Int? = null, ) { cancel(imageView) // TODO: validate with asHttpsUrlOrNull() from UrlValidation.kt (PR #1718) instead of a blank check. @@ -101,18 +106,16 @@ internal class ImageLoader internal constructor( imageView.setImageBitmap(cached) return } - // Clear any stale image (e.g. from a recycled RecyclerView row) when no placeholder is given. - if (placeholder != null) { - imageView.setImageResource(placeholder) - } else { - imageView.setImageDrawable(null) + if (validUrl == null) { + imageView.setPlaceholder(errorPlaceholder) + return } - if (validUrl == null) return + imageView.setPlaceholder(loadingPlaceholder) // Remove-then-add keeps exactly one listener per view, however many times the view is re-bound. imageView.removeOnAttachStateChangeListener(attachStateListener) imageView.addOnAttachStateChangeListener(attachStateListener) - startViewRequest(imageView, validUrl) + startViewRequest(imageView, validUrl, errorPlaceholder) } /** Cancels any pending load for [imageView]. Must be called on the main thread. */ @@ -137,7 +140,7 @@ internal class ImageLoader internal constructor( } } - private fun startViewRequest(imageView: ImageView, url: String) { + private fun startViewRequest(imageView: ImageView, url: String, @DrawableRes errorPlaceholder: Int?) { val target = WeakReference(imageView) val job = loaderScope.launch(mainDispatcher, start = CoroutineStart.LAZY) { val bitmap = try { @@ -154,10 +157,10 @@ internal class ImageLoader internal constructor( // Drop the result if the view has been re-bound to a different request since this one started. if (viewRequests[view]?.job !== coroutineContext.job) return@launch viewRequests.remove(view) - bitmap?.let(view::setImageBitmap) + if (bitmap != null) view.setImageBitmap(bitmap) else view.setPlaceholder(errorPlaceholder) } // Recorded before start so the identity check above always sees this request. - viewRequests[imageView] = ViewRequest(url, job) + viewRequests[imageView] = ViewRequest(url, errorPlaceholder, job) job.start() } @@ -199,7 +202,12 @@ internal class ImageLoader internal constructor( return inSampleSize } - private class ViewRequest(val url: String, val job: Job) + // Also clears a stale image (e.g. from a recycled RecyclerView row) when there is no placeholder. + private fun ImageView.setPlaceholder(@DrawableRes placeholder: Int?) { + if (placeholder != null) setImageResource(placeholder) else setImageDrawable(null) + } + + private class ViewRequest(val url: String, @DrawableRes val errorPlaceholder: Int?, val job: Job) companion object { // 4 MB keeps a reasonable number of small FI icons resident without pressuring low-memory devices. diff --git a/PayPalSavedPaymentMethod/src/main/java/com/braintreepayments/api/paypalsavedpaymentmethod/imageloader/LoaderImage.kt b/PayPalSavedPaymentMethod/src/main/java/com/braintreepayments/api/paypalsavedpaymentmethod/imageloader/LoaderImage.kt index 68a16408ef..12e28ba3f5 100644 --- a/PayPalSavedPaymentMethod/src/main/java/com/braintreepayments/api/paypalsavedpaymentmethod/imageloader/LoaderImage.kt +++ b/PayPalSavedPaymentMethod/src/main/java/com/braintreepayments/api/paypalsavedpaymentmethod/imageloader/LoaderImage.kt @@ -2,6 +2,7 @@ package com.braintreepayments.api.paypalsavedpaymentmethod.imageloader import android.graphics.Bitmap import androidx.compose.foundation.Image +import androidx.compose.foundation.layout.Spacer import androidx.compose.runtime.Composable import androidx.compose.runtime.LaunchedEffect import androidx.compose.runtime.getValue @@ -15,41 +16,44 @@ import androidx.compose.ui.platform.testTag import java.io.IOException /** - * Renders the funding-instrument icon at [url] via [ImageLoader], falling back to [placeholder] - * until the load completes, if it fails, or if [url] is `null` or blank. + * Renders the funding-instrument icon at [url] via [ImageLoader]. While the image loads it shows [loading], which is + * blank by default. It shows [placeholder] only when there is no image to show: if the load fails, or if [url] is + * `null` or blank. * * The icon is decorative (the FI label is rendered next to it), so it has no content description. + * + * @param loading drawn while the image loads. It must apply the [Modifier] it receives so it takes the icon's size. */ @Composable internal fun LoaderImage( url: String?, placeholder: Painter, modifier: Modifier = Modifier, + loading: @Composable (Modifier) -> Unit = { Spacer(it) }, imageLoader: ImageLoader = remember { ImageLoader.getInstance() }, ) { - // Seed from the memory cache so a cached icon renders on the first frame instead of flashing the placeholder. - var bitmap by remember(url) { mutableStateOf(url?.let(imageLoader::peek)) } + // Seeded from the memory cache so a cached icon renders on the first frame instead of flashing the loading state. + var state by remember(url) { mutableStateOf(initialState(url, imageLoader)) } LaunchedEffect(url) { - if (bitmap != null || url.isNullOrBlank()) return@LaunchedEffect - bitmap = try { - imageLoader.load(url) + if (state != LoadState.Loading || url == null) return@LaunchedEffect + state = try { + LoadState.Loaded(imageLoader.load(url)) } catch (ignored: IOException) { - null + LoadState.Failed } catch (ignored: IllegalArgumentException) { - null + LoadState.Failed } } - val loadedBitmap = bitmap - if (loadedBitmap != null) { - Image( - bitmap = loadedBitmap.asImageBitmap(), + when (val current = state) { + LoadState.Loading -> loading(modifier.testTag(LOADING_TEST_TAG)) + is LoadState.Loaded -> Image( + bitmap = current.bitmap.asImageBitmap(), contentDescription = null, modifier = modifier.testTag(LOADED_IMAGE_TEST_TAG) ) - } else { - Image( + LoadState.Failed -> Image( painter = placeholder, contentDescription = null, modifier = modifier.testTag(PLACEHOLDER_IMAGE_TEST_TAG) @@ -57,6 +61,19 @@ internal fun LoaderImage( } } +private sealed interface LoadState { + data object Loading : LoadState + data class Loaded(val bitmap: Bitmap) : LoadState + data object Failed : LoadState +} + +// A missing URL has nothing to load, so it goes straight to the placeholder instead of showing the loading state. +private fun initialState(url: String?, imageLoader: ImageLoader): LoadState = when { + url.isNullOrBlank() -> LoadState.Failed + else -> imageLoader.peek(url)?.let(LoadState::Loaded) ?: LoadState.Loading +} + // No semantic distinction exists otherwise, since the icon is decorative (contentDescription = null above). +internal const val LOADING_TEST_TAG = "loader_image_loading" internal const val LOADED_IMAGE_TEST_TAG = "loader_image_loaded" internal const val PLACEHOLDER_IMAGE_TEST_TAG = "loader_image_placeholder" diff --git a/PayPalSavedPaymentMethod/src/test/java/com/braintreepayments/api/paypalsavedpaymentmethod/imageloader/ImageLoaderViewBindingUnitTest.kt b/PayPalSavedPaymentMethod/src/test/java/com/braintreepayments/api/paypalsavedpaymentmethod/imageloader/ImageLoaderViewBindingUnitTest.kt index abeadaf480..ae29269517 100644 --- a/PayPalSavedPaymentMethod/src/test/java/com/braintreepayments/api/paypalsavedpaymentmethod/imageloader/ImageLoaderViewBindingUnitTest.kt +++ b/PayPalSavedPaymentMethod/src/test/java/com/braintreepayments/api/paypalsavedpaymentmethod/imageloader/ImageLoaderViewBindingUnitTest.kt @@ -18,6 +18,7 @@ import kotlinx.coroutines.test.runTest import org.junit.Assert.assertEquals import org.junit.Assert.assertFalse import org.junit.Assert.assertNotNull +import org.junit.Assert.assertNull import org.junit.Test import org.junit.runner.RunWith import org.robolectric.Robolectric @@ -43,10 +44,21 @@ class ImageLoaderViewBindingUnitTest { private val imageView = ImageView(activity).also { activity.setContentView(it) } @Test - fun `load shows the placeholder and then the downloaded bitmap`() { + fun `load stays blank while loading and then shows the downloaded bitmap`() { givenImage(32) - sut.load(URL, imageView, PLACEHOLDER) + sut.load(URL, imageView, errorPlaceholder = ERROR_PLACEHOLDER) + assertBlank() + + advanceUntilIdle() + assertShowsBitmapOfWidth(32) + } + + @Test + fun `load shows the loading placeholder while loading`() { + givenImage(32) + + sut.load(URL, imageView, loadingPlaceholder = LOADING_PLACEHOLDER) assertShowsPlaceholder() advanceUntilIdle() @@ -58,15 +70,15 @@ class ImageLoaderViewBindingUnitTest { givenImage(32) sut.load(URL) - sut.load(URL, imageView, PLACEHOLDER) + sut.load(URL, imageView, LOADING_PLACEHOLDER, ERROR_PLACEHOLDER) assertShowsBitmapOfWidth(32) } @Test - fun `load keeps the placeholder for a null or blank url without making a request`() { + fun `load shows the error placeholder for a null or blank url without making a request`() { listOf(null, " ").forEach { url -> - sut.load(url, imageView, PLACEHOLDER) + sut.load(url, imageView, LOADING_PLACEHOLDER, ERROR_PLACEHOLDER) advanceUntilIdle() assertShowsPlaceholder() } @@ -74,24 +86,36 @@ class ImageLoaderViewBindingUnitTest { } @Test - fun `load keeps the placeholder when the download fails or the url is rejected as malformed`() { + fun `load shows the error placeholder when the download fails or the url is rejected as malformed`() { listOf(IOException("Unexpected code 404"), IllegalArgumentException("Invalid URL host")).forEach { error -> coEvery { httpClient.sendRequestForBytes(any(), any()) } throws error - sut.load(URL, imageView, PLACEHOLDER) + sut.load(URL, imageView, errorPlaceholder = ERROR_PLACEHOLDER) + assertBlank() advanceUntilIdle() assertShowsPlaceholder() } } + @Test + fun `load goes blank when the download fails and there is no error placeholder`() { + coEvery { httpClient.sendRequestForBytes(any(), any()) } throws IOException("Unexpected code 404") + + sut.load(URL, imageView, loadingPlaceholder = LOADING_PLACEHOLDER) + assertShowsPlaceholder() + advanceUntilIdle() + + assertBlank() + } + @Test fun `re-binding the view before the first load completes shows only the latest image`() { coEvery { httpClient.sendRequestForBytes(match { it.url == URL }, any()) } returns pngBytes(16, 16) coEvery { httpClient.sendRequestForBytes(match { it.url == OTHER_URL }, any()) } returns pngBytes(48, 48) - sut.load(URL, imageView, PLACEHOLDER) - sut.load(OTHER_URL, imageView, PLACEHOLDER) + sut.load(URL, imageView, errorPlaceholder = ERROR_PLACEHOLDER) + sut.load(OTHER_URL, imageView, errorPlaceholder = ERROR_PLACEHOLDER) advanceUntilIdle() assertShowsBitmapOfWidth(48) @@ -101,10 +125,10 @@ class ImageLoaderViewBindingUnitTest { fun `a detached view resumes its load when re-attached`() { givenImage(32) - sut.load(URL, imageView, PLACEHOLDER) + sut.load(URL, imageView, errorPlaceholder = ERROR_PLACEHOLDER) activity.setContentView(FrameLayout(activity)) advanceUntilIdle() - assertShowsPlaceholder() + assertBlank() activity.setContentView(imageView) advanceUntilIdle() @@ -115,11 +139,11 @@ class ImageLoaderViewBindingUnitTest { fun `cancel stops the bitmap from being set`() { givenImage(32) - sut.load(URL, imageView, PLACEHOLDER) + sut.load(URL, imageView, errorPlaceholder = ERROR_PLACEHOLDER) sut.cancel(imageView) advanceUntilIdle() - assertShowsPlaceholder() + assertBlank() } private fun givenImage(size: Int) { @@ -128,6 +152,10 @@ class ImageLoaderViewBindingUnitTest { private fun advanceUntilIdle() = testDispatcher.scheduler.advanceUntilIdle() + private fun assertBlank() { + assertNull(imageView.drawable) + } + private fun assertShowsPlaceholder() { assertNotNull(imageView.drawable) assertFalse(imageView.drawable is BitmapDrawable) @@ -140,6 +168,7 @@ class ImageLoaderViewBindingUnitTest { companion object { private const val URL = "https://www.paypalobjects.com/images/visa.png" private const val OTHER_URL = "https://www.paypalobjects.com/images/bank.png" - private val PLACEHOLDER = R.drawable.ic_fi_card_placeholder + private val LOADING_PLACEHOLDER = R.drawable.ic_fi_bank_placeholder + private val ERROR_PLACEHOLDER = R.drawable.ic_fi_card_placeholder } } diff --git a/PayPalSavedPaymentMethod/src/test/java/com/braintreepayments/api/paypalsavedpaymentmethod/imageloader/LoaderImageUnitTest.kt b/PayPalSavedPaymentMethod/src/test/java/com/braintreepayments/api/paypalsavedpaymentmethod/imageloader/LoaderImageUnitTest.kt index f930b2dc0c..a25493a766 100644 --- a/PayPalSavedPaymentMethod/src/test/java/com/braintreepayments/api/paypalsavedpaymentmethod/imageloader/LoaderImageUnitTest.kt +++ b/PayPalSavedPaymentMethod/src/test/java/com/braintreepayments/api/paypalsavedpaymentmethod/imageloader/LoaderImageUnitTest.kt @@ -2,11 +2,14 @@ package com.braintreepayments.api.paypalsavedpaymentmethod.imageloader import android.graphics.Bitmap import android.util.LruCache +import androidx.compose.foundation.layout.Box import androidx.compose.runtime.getValue import androidx.compose.runtime.mutableStateOf import androidx.compose.runtime.setValue +import androidx.compose.ui.Modifier import androidx.compose.ui.graphics.Color import androidx.compose.ui.graphics.painter.ColorPainter +import androidx.compose.ui.platform.testTag import androidx.compose.ui.test.junit4.createComposeRule import androidx.compose.ui.test.onNodeWithTag import com.braintreepayments.api.sharedutils.HttpClient @@ -44,23 +47,48 @@ class LoaderImageUnitTest { private val placeholder = ColorPainter(Color.Red) @Test - fun `shows the placeholder then the loaded image`() { + fun `shows the loading state, not the placeholder, while the image loads`() { coEvery { httpClient.sendRequestForBytes(any(), any()) } returns pngBytes(32, 32) composeTestRule.setContent { LoaderImage(url = URL, placeholder = placeholder, imageLoader = sut) } - composeTestRule.onNodeWithTag(PLACEHOLDER_IMAGE_TEST_TAG).assertExists() + composeTestRule.onNodeWithTag(LOADING_TEST_TAG).assertExists() + composeTestRule.onNodeWithTag(PLACEHOLDER_IMAGE_TEST_TAG).assertDoesNotExist() composeTestRule.onNodeWithTag(LOADED_IMAGE_TEST_TAG).assertDoesNotExist() testDispatcher.scheduler.advanceUntilIdle() composeTestRule.waitForIdle() composeTestRule.onNodeWithTag(LOADED_IMAGE_TEST_TAG).assertExists() + composeTestRule.onNodeWithTag(LOADING_TEST_TAG).assertDoesNotExist() composeTestRule.onNodeWithTag(PLACEHOLDER_IMAGE_TEST_TAG).assertDoesNotExist() } + @Test + fun `draws the loading slot while the image loads`() { + coEvery { httpClient.sendRequestForBytes(any(), any()) } returns pngBytes(32, 32) + + composeTestRule.setContent { + LoaderImage( + url = URL, + placeholder = placeholder, + loading = { Box(it) { Box(Modifier.testTag(CUSTOM_LOADING_TEST_TAG)) } }, + imageLoader = sut + ) + } + + composeTestRule.onNodeWithTag(CUSTOM_LOADING_TEST_TAG).assertExists() + composeTestRule.onNodeWithTag(PLACEHOLDER_IMAGE_TEST_TAG).assertDoesNotExist() + + testDispatcher.scheduler.advanceUntilIdle() + composeTestRule.waitForIdle() + + composeTestRule.onNodeWithTag(CUSTOM_LOADING_TEST_TAG).assertDoesNotExist() + composeTestRule.onNodeWithTag(LOADED_IMAGE_TEST_TAG).assertExists() + } + @Test fun `shows the cached image on the first frame without flashing the placeholder`() = runTest(testDispatcher) { coEvery { httpClient.sendRequestForBytes(any(), any()) } returns pngBytes(32, 32) @@ -71,11 +99,12 @@ class LoaderImageUnitTest { } composeTestRule.onNodeWithTag(LOADED_IMAGE_TEST_TAG).assertExists() + composeTestRule.onNodeWithTag(LOADING_TEST_TAG).assertDoesNotExist() composeTestRule.onNodeWithTag(PLACEHOLDER_IMAGE_TEST_TAG).assertDoesNotExist() } @Test - fun `keeps the placeholder when the load fails`() { + fun `shows the placeholder when the load fails`() { coEvery { httpClient.sendRequestForBytes(any(), any()) } throws IOException("boom") composeTestRule.setContent { @@ -85,9 +114,20 @@ class LoaderImageUnitTest { composeTestRule.waitForIdle() composeTestRule.onNodeWithTag(PLACEHOLDER_IMAGE_TEST_TAG).assertExists() + composeTestRule.onNodeWithTag(LOADING_TEST_TAG).assertDoesNotExist() composeTestRule.onNodeWithTag(LOADED_IMAGE_TEST_TAG).assertDoesNotExist() } + @Test + fun `shows the placeholder without a loading state for a null url`() { + assertPlaceholderWithoutLoading(url = null) + } + + @Test + fun `shows the placeholder without a loading state for a blank url`() { + assertPlaceholderWithoutLoading(url = " ") + } + @Test fun `loads the new image after the url changes`() { coEvery { httpClient.sendRequestForBytes(match { it.url == URL }, any()) } returns pngBytes(16, 16) @@ -112,8 +152,21 @@ class LoaderImageUnitTest { coVerify(exactly = 1) { httpClient.sendRequestForBytes(match { it.url == OTHER_URL }, any()) } } + private fun assertPlaceholderWithoutLoading(url: String?) { + composeTestRule.setContent { + LoaderImage(url = url, placeholder = placeholder, imageLoader = sut) + } + testDispatcher.scheduler.advanceUntilIdle() + composeTestRule.waitForIdle() + + composeTestRule.onNodeWithTag(PLACEHOLDER_IMAGE_TEST_TAG).assertExists() + composeTestRule.onNodeWithTag(LOADING_TEST_TAG).assertDoesNotExist() + coVerify(exactly = 0) { httpClient.sendRequestForBytes(any(), any()) } + } + private companion object { const val URL = "https://www.paypalobjects.com/images/visa.png" const val OTHER_URL = "https://www.paypalobjects.com/images/bank.png" + const val CUSTOM_LOADING_TEST_TAG = "custom_loading" } } From 58f5bd45edfa9af969beab2f2ab842df5d89be54 Mon Sep 17 00:00:00 2001 From: Bala Mani Nayar D Date: Tue, 20 Oct 2026 19:20:38 +0530 Subject: [PATCH 08/15] fix(PayPalSavedPaymentMethod): validate image URLs with asHttpsUrlOrNull Replace the blank-only checks in ImageLoader with asHttpsUrlOrNull() now that UrlValidation.kt (#1718) is on the feature branch, so only https URLs with a host reach the network. LoaderImage applies the same rule up front, so an invalid URL shows the placeholder without a loading state. Co-Authored-By: Claude Opus 5.5 (1M context) --- .../imageloader/ImageLoader.kt | 22 +++++++++---------- .../imageloader/LoaderImage.kt | 11 +++++----- .../imageloader/ImageLoaderUnitTest.kt | 11 +++++----- .../ImageLoaderViewBindingUnitTest.kt | 4 ++-- .../imageloader/LoaderImageUnitTest.kt | 5 +++++ 5 files changed, 29 insertions(+), 24 deletions(-) diff --git a/PayPalSavedPaymentMethod/src/main/java/com/braintreepayments/api/paypalsavedpaymentmethod/imageloader/ImageLoader.kt b/PayPalSavedPaymentMethod/src/main/java/com/braintreepayments/api/paypalsavedpaymentmethod/imageloader/ImageLoader.kt index f47795145c..2d70f02d48 100644 --- a/PayPalSavedPaymentMethod/src/main/java/com/braintreepayments/api/paypalsavedpaymentmethod/imageloader/ImageLoader.kt +++ b/PayPalSavedPaymentMethod/src/main/java/com/braintreepayments/api/paypalsavedpaymentmethod/imageloader/ImageLoader.kt @@ -6,6 +6,7 @@ import android.util.LruCache import android.view.View import android.widget.ImageView import androidx.annotation.DrawableRes +import com.braintreepayments.api.paypalsavedpaymentmethod.asHttpsUrlOrNull import com.braintreepayments.api.sharedutils.HttpClient import com.braintreepayments.api.sharedutils.Method import com.braintreepayments.api.sharedutils.OkHttpRequest @@ -74,21 +75,19 @@ internal class ImageLoader internal constructor( fun peek(url: String): Bitmap? = memoryCache.get(url) /** - * Returns the bitmap for [url], from memory when cached. Throws [IllegalArgumentException] if [url] is blank or - * malformed, and [IOException] if the download or decode fails. + * Returns the bitmap for [url], from memory when cached. Throws [IllegalArgumentException] if [url] is not an https + * URL with a host, and [IOException] if the download or decode fails. */ suspend fun load(url: String): Bitmap { - // TODO: validate with asHttpsUrlOrNull() from UrlValidation.kt (PR #1718) once it is on this branch, so only - // https URLs with a host reach the network. - require(url.isNotBlank()) { "url must not be blank" } - memoryCache.get(url)?.let { return it } - return inFlightRequest(url).await() + val validUrl = requireNotNull(url.asHttpsUrlOrNull()) { "url must be an https URL with a host" } + memoryCache.get(validUrl)?.let { return it } + return inFlightRequest(validUrl).await() } /** * Binds [url] into [imageView]. While the image loads, the view shows [loadingPlaceholder], or stays blank when it - * is `null`. If [url] is missing or blank, or the load fails, it shows [errorPlaceholder], or stays blank when it - * is `null`. + * is `null`. If [url] is missing or not an https URL with a host, or the load fails, it shows [errorPlaceholder], + * or stays blank when it is `null`. * * Must be called on the main thread; it updates [imageView] directly. */ @@ -99,8 +98,7 @@ internal class ImageLoader internal constructor( @DrawableRes errorPlaceholder: Int? = null, ) { cancel(imageView) - // TODO: validate with asHttpsUrlOrNull() from UrlValidation.kt (PR #1718) instead of a blank check. - val validUrl = url?.takeIf { it.isNotBlank() } + val validUrl = url.asHttpsUrlOrNull() val cached = validUrl?.let { memoryCache.get(it) } if (cached != null) { imageView.setImageBitmap(cached) @@ -148,7 +146,7 @@ internal class ImageLoader internal constructor( } catch (ignored: IOException) { null // keep the placeholder; no automatic retry } catch (ignored: IllegalArgumentException) { - null // malformed URL rejected before any request; see the URL validation TODO in load() + null // non-https or host-less URL rejected before any request } catch (ignored: Exception) { if (ignored is CancellationException) throw ignored null // unexpected failure (e.g. a malformed response the decoder can't handle); keep the placeholder diff --git a/PayPalSavedPaymentMethod/src/main/java/com/braintreepayments/api/paypalsavedpaymentmethod/imageloader/LoaderImage.kt b/PayPalSavedPaymentMethod/src/main/java/com/braintreepayments/api/paypalsavedpaymentmethod/imageloader/LoaderImage.kt index 12e28ba3f5..6b5b28af24 100644 --- a/PayPalSavedPaymentMethod/src/main/java/com/braintreepayments/api/paypalsavedpaymentmethod/imageloader/LoaderImage.kt +++ b/PayPalSavedPaymentMethod/src/main/java/com/braintreepayments/api/paypalsavedpaymentmethod/imageloader/LoaderImage.kt @@ -13,12 +13,13 @@ import androidx.compose.ui.Modifier import androidx.compose.ui.graphics.asImageBitmap import androidx.compose.ui.graphics.painter.Painter import androidx.compose.ui.platform.testTag +import com.braintreepayments.api.paypalsavedpaymentmethod.asHttpsUrlOrNull import java.io.IOException /** * Renders the funding-instrument icon at [url] via [ImageLoader]. While the image loads it shows [loading], which is * blank by default. It shows [placeholder] only when there is no image to show: if the load fails, or if [url] is - * `null` or blank. + * `null` or not an https URL with a host. * * The icon is decorative (the FI label is rendered next to it), so it has no content description. * @@ -67,10 +68,10 @@ private sealed interface LoadState { data object Failed : LoadState } -// A missing URL has nothing to load, so it goes straight to the placeholder instead of showing the loading state. -private fun initialState(url: String?, imageLoader: ImageLoader): LoadState = when { - url.isNullOrBlank() -> LoadState.Failed - else -> imageLoader.peek(url)?.let(LoadState::Loaded) ?: LoadState.Loading +// A missing or non-https URL has nothing to load, so it goes straight to the placeholder instead of showing loading. +private fun initialState(url: String?, imageLoader: ImageLoader): LoadState { + val validUrl = url.asHttpsUrlOrNull() ?: return LoadState.Failed + return imageLoader.peek(validUrl)?.let(LoadState::Loaded) ?: LoadState.Loading } // No semantic distinction exists otherwise, since the icon is decorative (contentDescription = null above). diff --git a/PayPalSavedPaymentMethod/src/test/java/com/braintreepayments/api/paypalsavedpaymentmethod/imageloader/ImageLoaderUnitTest.kt b/PayPalSavedPaymentMethod/src/test/java/com/braintreepayments/api/paypalsavedpaymentmethod/imageloader/ImageLoaderUnitTest.kt index c5b33f27f7..cc52acaa6d 100644 --- a/PayPalSavedPaymentMethod/src/test/java/com/braintreepayments/api/paypalsavedpaymentmethod/imageloader/ImageLoaderUnitTest.kt +++ b/PayPalSavedPaymentMethod/src/test/java/com/braintreepayments/api/paypalsavedpaymentmethod/imageloader/ImageLoaderUnitTest.kt @@ -114,12 +114,13 @@ class ImageLoaderUnitTest { } @Test - fun `load throws IllegalArgumentException for a blank url without making a request`() = runTest(testDispatcher) { - assertFailsWith { sut.load("") } - assertFailsWith { sut.load(" ") } + fun `load throws IllegalArgumentException for a blank or non-https url without making a request`() = + runTest(testDispatcher) { + listOf("", " ", "http://www.paypalobjects.com/images/visa.png", "javascript:alert(1)", "https://") + .forEach { url -> assertFailsWith { sut.load(url) } } - coVerify(exactly = 0) { httpClient.sendRequestForBytes(any(), any()) } - } + coVerify(exactly = 0) { httpClient.sendRequestForBytes(any(), any()) } + } @Test fun `load downsamples a large image to at most 128 px`() = runTest(testDispatcher) { diff --git a/PayPalSavedPaymentMethod/src/test/java/com/braintreepayments/api/paypalsavedpaymentmethod/imageloader/ImageLoaderViewBindingUnitTest.kt b/PayPalSavedPaymentMethod/src/test/java/com/braintreepayments/api/paypalsavedpaymentmethod/imageloader/ImageLoaderViewBindingUnitTest.kt index ae29269517..ca673a281b 100644 --- a/PayPalSavedPaymentMethod/src/test/java/com/braintreepayments/api/paypalsavedpaymentmethod/imageloader/ImageLoaderViewBindingUnitTest.kt +++ b/PayPalSavedPaymentMethod/src/test/java/com/braintreepayments/api/paypalsavedpaymentmethod/imageloader/ImageLoaderViewBindingUnitTest.kt @@ -76,8 +76,8 @@ class ImageLoaderViewBindingUnitTest { } @Test - fun `load shows the error placeholder for a null or blank url without making a request`() { - listOf(null, " ").forEach { url -> + fun `load shows the error placeholder for a null, blank or non-https url without making a request`() { + listOf(null, " ", "http://www.paypalobjects.com/images/visa.png").forEach { url -> sut.load(url, imageView, LOADING_PLACEHOLDER, ERROR_PLACEHOLDER) advanceUntilIdle() assertShowsPlaceholder() diff --git a/PayPalSavedPaymentMethod/src/test/java/com/braintreepayments/api/paypalsavedpaymentmethod/imageloader/LoaderImageUnitTest.kt b/PayPalSavedPaymentMethod/src/test/java/com/braintreepayments/api/paypalsavedpaymentmethod/imageloader/LoaderImageUnitTest.kt index a25493a766..369247a828 100644 --- a/PayPalSavedPaymentMethod/src/test/java/com/braintreepayments/api/paypalsavedpaymentmethod/imageloader/LoaderImageUnitTest.kt +++ b/PayPalSavedPaymentMethod/src/test/java/com/braintreepayments/api/paypalsavedpaymentmethod/imageloader/LoaderImageUnitTest.kt @@ -128,6 +128,11 @@ class LoaderImageUnitTest { assertPlaceholderWithoutLoading(url = " ") } + @Test + fun `shows the placeholder without a loading state for a non-https url`() { + assertPlaceholderWithoutLoading(url = "http://www.paypalobjects.com/images/visa.png") + } + @Test fun `loads the new image after the url changes`() { coEvery { httpClient.sendRequestForBytes(match { it.url == URL }, any()) } returns pngBytes(16, 16) From 68ef661fbe0ce0080ed2b1babb71a90c27fdf35e Mon Sep 17 00:00:00 2001 From: Bala Mani Nayar D Date: Wed, 21 Oct 2026 14:43:41 +0530 Subject: [PATCH 09/15] fix(SharedUtils): bound binary downloads by default and address review feedback on the image loader - HttpClient.sendRequestForBytes and OkHttpSynchronousHttpClient.executeRequestForBytes default to a 5 MB body limit instead of being unbounded - executeRequestForBytes limits the whole call, body included, to 30 seconds, so a server that trickles bytes can't hold a download open - LoaderImage shows the placeholder for any unexpected exception instead of crashing the composition, rethrowing CancellationException - document why the process-wide loader scope is safe, and update the CHANGELOG entry --- CHANGELOG.md | 2 +- .../imageloader/ImageLoader.kt | 4 +++- .../imageloader/LoaderImage.kt | 5 ++++ .../imageloader/LoaderImageUnitTest.kt | 14 +++++++++++ .../api/sharedutils/HttpClient.kt | 7 ++++-- .../OkHttpSynchronousHttpClient.kt | 24 +++++++++++++------ .../api/sharedutils/HttpClientUnitTest.kt | 6 ++--- .../OkHttpSynchronousHttpClientTest.kt | 22 +++++++++++++++++ 8 files changed, 70 insertions(+), 14 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 0acb74a006..2ef1aa356e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,7 +9,7 @@ * SEPADirectDebit * Add Compose constructor for `SEPADirectDebitLauncher` * SharedUtils - * Add `HttpClient#sendRequestForBytes` for fetching binary payloads with a size cap, enforced via streaming even when the response omits `Content-Length` + * Add `HttpClient#sendRequestForBytes` for fetching binary payloads with a size cap (5 MB by default), enforced via streaming even when the response omits `Content-Length`, and a 30 second limit on the whole call ## 5.33.0 (2026-09-16) diff --git a/PayPalSavedPaymentMethod/src/main/java/com/braintreepayments/api/paypalsavedpaymentmethod/imageloader/ImageLoader.kt b/PayPalSavedPaymentMethod/src/main/java/com/braintreepayments/api/paypalsavedpaymentmethod/imageloader/ImageLoader.kt index 2d70f02d48..c68f55b206 100644 --- a/PayPalSavedPaymentMethod/src/main/java/com/braintreepayments/api/paypalsavedpaymentmethod/imageloader/ImageLoader.kt +++ b/PayPalSavedPaymentMethod/src/main/java/com/braintreepayments/api/paypalsavedpaymentmethod/imageloader/ImageLoader.kt @@ -47,7 +47,9 @@ internal class ImageLoader internal constructor( private val decodeSemaphore: Semaphore = Semaphore(DEFAULT_MAX_PARALLEL_DECODES), ) { // Deliberately unscoped to any lifecycle: ImageLoader is a process-wide singleton (see getInstance()), so - // in-flight downloads outlive any single screen and are only ever torn down by process death. + // in-flight downloads outlive any single screen and are only ever torn down by process death. That is safe because + // each download is short and bounded (30 s call timeout, size cap) and failures aren't cached, so nothing + // accumulates. private val loaderScope = CoroutineScope(SupervisorJob() + defaultDispatcher) private val inFlightRequests = ConcurrentHashMap>() diff --git a/PayPalSavedPaymentMethod/src/main/java/com/braintreepayments/api/paypalsavedpaymentmethod/imageloader/LoaderImage.kt b/PayPalSavedPaymentMethod/src/main/java/com/braintreepayments/api/paypalsavedpaymentmethod/imageloader/LoaderImage.kt index 6b5b28af24..99764ebb2a 100644 --- a/PayPalSavedPaymentMethod/src/main/java/com/braintreepayments/api/paypalsavedpaymentmethod/imageloader/LoaderImage.kt +++ b/PayPalSavedPaymentMethod/src/main/java/com/braintreepayments/api/paypalsavedpaymentmethod/imageloader/LoaderImage.kt @@ -15,6 +15,7 @@ import androidx.compose.ui.graphics.painter.Painter import androidx.compose.ui.platform.testTag import com.braintreepayments.api.paypalsavedpaymentmethod.asHttpsUrlOrNull import java.io.IOException +import kotlin.coroutines.cancellation.CancellationException /** * Renders the funding-instrument icon at [url] via [ImageLoader]. While the image loads it shows [loading], which is @@ -44,6 +45,10 @@ internal fun LoaderImage( LoadState.Failed } catch (ignored: IllegalArgumentException) { LoadState.Failed + } catch (ignored: Exception) { + if (ignored is CancellationException) throw ignored + // Unexpected failure (e.g. a response the decoder can't handle): show the placeholder, don't crash. + LoadState.Failed } } diff --git a/PayPalSavedPaymentMethod/src/test/java/com/braintreepayments/api/paypalsavedpaymentmethod/imageloader/LoaderImageUnitTest.kt b/PayPalSavedPaymentMethod/src/test/java/com/braintreepayments/api/paypalsavedpaymentmethod/imageloader/LoaderImageUnitTest.kt index 369247a828..cad87d0aeb 100644 --- a/PayPalSavedPaymentMethod/src/test/java/com/braintreepayments/api/paypalsavedpaymentmethod/imageloader/LoaderImageUnitTest.kt +++ b/PayPalSavedPaymentMethod/src/test/java/com/braintreepayments/api/paypalsavedpaymentmethod/imageloader/LoaderImageUnitTest.kt @@ -118,6 +118,20 @@ class LoaderImageUnitTest { composeTestRule.onNodeWithTag(LOADED_IMAGE_TEST_TAG).assertDoesNotExist() } + @Test + fun `shows the placeholder when the load fails with an unexpected exception`() { + coEvery { httpClient.sendRequestForBytes(any(), any()) } throws IllegalStateException("boom") + + composeTestRule.setContent { + LoaderImage(url = URL, placeholder = placeholder, imageLoader = sut) + } + testDispatcher.scheduler.advanceUntilIdle() + composeTestRule.waitForIdle() + + composeTestRule.onNodeWithTag(PLACEHOLDER_IMAGE_TEST_TAG).assertExists() + composeTestRule.onNodeWithTag(LOADING_TEST_TAG).assertDoesNotExist() + } + @Test fun `shows the placeholder without a loading state for a null url`() { assertPlaceholderWithoutLoading(url = null) diff --git a/SharedUtils/src/main/java/com/braintreepayments/api/sharedutils/HttpClient.kt b/SharedUtils/src/main/java/com/braintreepayments/api/sharedutils/HttpClient.kt index 675eec0ffe..2dda914a9d 100644 --- a/SharedUtils/src/main/java/com/braintreepayments/api/sharedutils/HttpClient.kt +++ b/SharedUtils/src/main/java/com/braintreepayments/api/sharedutils/HttpClient.kt @@ -27,13 +27,16 @@ class HttpClient internal constructor( /** * Sends [request] and returns the raw response body, for binary payloads such as images. * - * @param maxBodyBytes Optional limit on the response body size, enforced even if the response omits + * @param maxBodyBytes Limit on the response body size, 5 MB by default, enforced even if the response omits * `Content-Length` (e.g. chunked transfer). * @throws java.io.IOException if the network request fails, the response is unsuccessful or has no body, or the * body size exceeds [maxBodyBytes] * @throws IllegalArgumentException if the request URL is not a valid http or https URL */ - suspend fun sendRequestForBytes(request: OkHttpRequest, maxBodyBytes: Long? = null): ByteArray { + suspend fun sendRequestForBytes( + request: OkHttpRequest, + maxBodyBytes: Long = OkHttpSynchronousHttpClient.DEFAULT_MAX_BODY_BYTES + ): ByteArray { return withContext(ioDispatcher) { okHttpSynchronousHttpClient.executeRequestForBytes(request, maxBodyBytes) } diff --git a/SharedUtils/src/main/java/com/braintreepayments/api/sharedutils/OkHttpSynchronousHttpClient.kt b/SharedUtils/src/main/java/com/braintreepayments/api/sharedutils/OkHttpSynchronousHttpClient.kt index 8e5325f2a1..f4e1441bcd 100644 --- a/SharedUtils/src/main/java/com/braintreepayments/api/sharedutils/OkHttpSynchronousHttpClient.kt +++ b/SharedUtils/src/main/java/com/braintreepayments/api/sharedutils/OkHttpSynchronousHttpClient.kt @@ -61,26 +61,30 @@ internal class OkHttpSynchronousHttpClient( * [String]. * * @param okHttpRequest The request data to execute. - * @param maxBodyBytes Optional limit on the response body size. Rejected as soon as the declared - * `Content-Length` exceeds it, or, if undeclared (e.g. chunked transfer), as soon as actual bytes read exceed - * it — the body is never buffered past [maxBodyBytes] + 1 bytes. + * @param maxBodyBytes Limit on the response body size, [DEFAULT_MAX_BODY_BYTES] by default. Rejected as soon as the + * declared `Content-Length` exceeds it, or, if undeclared (e.g. chunked transfer), as soon as actual bytes read + * exceed it — the body is never buffered past [maxBodyBytes] + 1 bytes. * @return The response body bytes. * @throws IOException if the request fails, the response is unsuccessful or has no body, an https request is - * redirected to a non-https URL, or the body size exceeds [maxBodyBytes]. + * redirected to a non-https URL, the body size exceeds [maxBodyBytes], or the whole call, body included, takes + * longer than [BYTES_CALL_TIMEOUT_SECONDS]. * @throws IllegalArgumentException if the request URL is not a valid http or https URL. */ @Throws(IOException::class) - fun executeRequestForBytes(okHttpRequest: OkHttpRequest, maxBodyBytes: Long? = null): ByteArray { + fun executeRequestForBytes(okHttpRequest: OkHttpRequest, maxBodyBytes: Long = DEFAULT_MAX_BODY_BYTES): ByteArray { val request = okHttpRequest.toRequest() - okHttpClient.newCall(request).execute().use { response -> + // The connect and read timeouts only bound each step, so a server that trickles bytes could hold the call open + // for as long as it keeps sending; this bounds the whole call. + val call = okHttpClient.newCall(request) + call.timeout().timeout(BYTES_CALL_TIMEOUT_SECONDS, TimeUnit.SECONDS) + call.execute().use { response -> val responseBody = response.body?.takeIf { response.isSuccessful } ?: throw IOException("Unexpected code ${response.code} for ${request.url}") // OkHttp follows redirects, including https to http; never accept a downgraded download. if (request.url.isHttps && !response.request.url.isHttps) { throw IOException("Redirected from https to a non-https URL for ${request.url}") } - if (maxBodyBytes == null) return responseBody.bytes() return responseBody.readBoundedBytes(maxBodyBytes, request.url.toString()) } } @@ -133,5 +137,11 @@ internal class OkHttpSynchronousHttpClient( companion object { private const val THIRTY = 30L + + /** How long [executeRequestForBytes] may take in total, from connecting to reading the last byte. */ + private const val BYTES_CALL_TIMEOUT_SECONDS = THIRTY + + /** The largest response body [executeRequestForBytes] reads unless the caller asks for a different limit. */ + const val DEFAULT_MAX_BODY_BYTES = 5L * 1024L * 1024L } } diff --git a/SharedUtils/src/test/java/com/braintreepayments/api/sharedutils/HttpClientUnitTest.kt b/SharedUtils/src/test/java/com/braintreepayments/api/sharedutils/HttpClientUnitTest.kt index b5234d4d5d..9ba1823c7b 100644 --- a/SharedUtils/src/test/java/com/braintreepayments/api/sharedutils/HttpClientUnitTest.kt +++ b/SharedUtils/src/test/java/com/braintreepayments/api/sharedutils/HttpClientUnitTest.kt @@ -97,7 +97,7 @@ class HttpClientUnitTest { } @Test - fun `when sendRequestForBytes is called, returns bytes and forwards maxBodyBytes`() = + fun `when sendRequestForBytes is called, returns bytes and forwards maxBodyBytes or the default limit`() = runTest(testDispatcher) { val request = mockk() val bytes = byteArrayOf(1, 2, 3) @@ -108,7 +108,7 @@ class HttpClientUnitTest { sut.sendRequestForBytes(request) verify { mockOkHttpClient.executeRequestForBytes(request, 1024L) } - verify { mockOkHttpClient.executeRequestForBytes(request, null) } + verify { mockOkHttpClient.executeRequestForBytes(request, OkHttpSynchronousHttpClient.DEFAULT_MAX_BODY_BYTES) } } @Test @@ -116,7 +116,7 @@ class HttpClientUnitTest { runTest(testDispatcher) { val request = mockk() - every { mockOkHttpClient.executeRequestForBytes(request, null) } throws java.io.IOException("Not found") + every { mockOkHttpClient.executeRequestForBytes(request, any()) } throws java.io.IOException("Not found") val thrownException = assertFailsWith { sut.sendRequestForBytes(request) diff --git a/SharedUtils/src/test/java/com/braintreepayments/api/sharedutils/OkHttpSynchronousHttpClientTest.kt b/SharedUtils/src/test/java/com/braintreepayments/api/sharedutils/OkHttpSynchronousHttpClientTest.kt index 9184f81c03..066d85a8c7 100644 --- a/SharedUtils/src/test/java/com/braintreepayments/api/sharedutils/OkHttpSynchronousHttpClientTest.kt +++ b/SharedUtils/src/test/java/com/braintreepayments/api/sharedutils/OkHttpSynchronousHttpClientTest.kt @@ -13,6 +13,7 @@ import okhttp3.Response import okhttp3.ResponseBody import okhttp3.ResponseBody.Companion.toResponseBody import okio.Buffer +import okio.Timeout import org.junit.After import org.junit.Assert.assertArrayEquals import org.junit.Assert.assertEquals @@ -20,6 +21,7 @@ import org.junit.Assert.assertTrue import org.junit.Before import org.junit.Test import java.io.IOException +import java.util.concurrent.TimeUnit import kotlin.test.assertFailsWith class OkHttpSynchronousHttpClientTest { @@ -146,6 +148,26 @@ class OkHttpSynchronousHttpClientTest { verify(exactly = 0) { responseBody.source() } } + @Test + fun `executeRequestForBytes limits the whole call to 30 seconds`() { + val callTimeout = mockk(relaxed = true) + givenBytesResponse(isSuccessful = true, body = byteArrayOf(1).toResponseBody()) + every { call.timeout() } returns callTimeout + + sut.executeRequestForBytes(OkHttpRequest(URL, Method.Get)) + + verify { callTimeout.timeout(30L, TimeUnit.SECONDS) } + } + + @Test + fun `when maxBodyBytes is not given, executeRequestForBytes rejects a body over the default limit`() { + givenBytesResponse(isSuccessful = true, body = responseBody) + every { responseBody.contentLength() } returns OkHttpSynchronousHttpClient.DEFAULT_MAX_BODY_BYTES + 1 + + assertFailsWith { sut.executeRequestForBytes(OkHttpRequest(URL, Method.Get)) } + verify(exactly = 0) { responseBody.source() } + } + @Test fun `when declared body size is within maxBodyBytes, executeRequestForBytes returns the body`() { val bytes = ByteArray(1024) { 7 } From 1abdc8cf6a1ca79dd6b1c93cfec758dc154f9ddb Mon Sep 17 00:00:00 2001 From: Bala Mani Nayar D Date: Wed, 21 Oct 2026 14:54:57 +0530 Subject: [PATCH 10/15] test(SharedUtils): add instrumentation tests for binary downloads The unit tests mock OkHttp, so they can't check real chunked transfer, binary integrity over TLS or the https-to-http redirect check. Adds MockWebServer tests for executeRequestForBytes (binary body, declared and chunked bodies over and within the limit, 404, https-to-http and https-to-https redirects) and for sendRequestForBytes (returns the bytes, runs off the main thread). The androidTest manifest allows cleartext traffic so the redirect test can reach a plain http server, and the mock server uses HTTP/1.1 because HTTP/2 has no chunked transfer encoding. --- .../src/androidTest/AndroidManifest.xml | 3 +- .../api/sharedutils/HttpClientTest.kt | 36 +++++++ .../OkHttpSynchronousHttpClientTest.kt | 102 ++++++++++++++++++ 3 files changed, 140 insertions(+), 1 deletion(-) diff --git a/SharedUtils/src/androidTest/AndroidManifest.xml b/SharedUtils/src/androidTest/AndroidManifest.xml index 8485581508..bcb6205b67 100644 --- a/SharedUtils/src/androidTest/AndroidManifest.xml +++ b/SharedUtils/src/androidTest/AndroidManifest.xml @@ -3,7 +3,8 @@ - + + diff --git a/SharedUtils/src/androidTest/java/com/braintreepayments/api/sharedutils/HttpClientTest.kt b/SharedUtils/src/androidTest/java/com/braintreepayments/api/sharedutils/HttpClientTest.kt index 5b5d06e7af..a745405422 100644 --- a/SharedUtils/src/androidTest/java/com/braintreepayments/api/sharedutils/HttpClientTest.kt +++ b/SharedUtils/src/androidTest/java/com/braintreepayments/api/sharedutils/HttpClientTest.kt @@ -9,7 +9,9 @@ import okhttp3.mockwebserver.MockResponse import okhttp3.mockwebserver.MockWebServer import okhttp3.tls.HandshakeCertificates import okhttp3.tls.HeldCertificate +import okio.Buffer import org.junit.After +import org.junit.Assert.assertArrayEquals import org.junit.Assert.assertFalse import org.junit.Assert.assertNotNull import org.junit.Before @@ -84,6 +86,40 @@ class HttpClientTest { ) } + @Test + fun sendRequestForBytes_returnsBodyAndDispatchesNetworkCallOffCallingThread() = runTest { + var networkThreadName = "" + val callingThreadName = Thread.currentThread().name + val okHttpClient = OkHttpClient.Builder() + .sslSocketFactory( + clientCertificates.sslSocketFactory(), + clientCertificates.trustManager + ) + .addInterceptor { chain -> + networkThreadName = Thread.currentThread().name + chain.proceed(chain.request()) + } + .build() + val sut = HttpClient( + okHttpSynchronousHttpClient = OkHttpSynchronousHttpClient(okHttpClient = okHttpClient) + ) + // PNG signature plus bytes that are invalid UTF-8, which a String round-trip would corrupt + val bytes = byteArrayOf(0x89.toByte(), 0x50, 0x4E, 0x47, 0xFF.toByte(), 0xFE.toByte(), 0x00) + mockWebServer.enqueue(MockResponse().setBody(Buffer().write(bytes)).setResponseCode(200)) + + val result = sut.sendRequestForBytes(OkHttpRequest(mockWebServer.url("/image").toString(), Method.Get)) + + assertArrayEquals(bytes, result) + assertFalse( + "Network call should execute on a different thread", + networkThreadName == callingThreadName + ) + assertFalse( + "Network call should not execute on main thread", + Looper.getMainLooper().thread.name == networkThreadName + ) + } + @Test fun defaultConstructor_createsWorkingInstance() { val client = HttpClient() diff --git a/SharedUtils/src/androidTest/java/com/braintreepayments/api/sharedutils/OkHttpSynchronousHttpClientTest.kt b/SharedUtils/src/androidTest/java/com/braintreepayments/api/sharedutils/OkHttpSynchronousHttpClientTest.kt index a0d12c09c2..a68fd36338 100644 --- a/SharedUtils/src/androidTest/java/com/braintreepayments/api/sharedutils/OkHttpSynchronousHttpClientTest.kt +++ b/SharedUtils/src/androidTest/java/com/braintreepayments/api/sharedutils/OkHttpSynchronousHttpClientTest.kt @@ -2,11 +2,14 @@ package com.braintreepayments.api.sharedutils import androidx.test.internal.runner.junit4.AndroidJUnit4ClassRunner import okhttp3.OkHttpClient +import okhttp3.Protocol import okhttp3.mockwebserver.MockResponse import okhttp3.mockwebserver.MockWebServer import okhttp3.tls.HandshakeCertificates import okhttp3.tls.HeldCertificate +import okio.Buffer import org.junit.After +import org.junit.Assert.assertArrayEquals import org.junit.Assert.assertEquals import org.junit.Assert.assertTrue import org.junit.Before @@ -35,6 +38,8 @@ class OkHttpSynchronousHttpClientTest { .build() mockWebServer = MockWebServer() + // HTTP/2 has no chunked transfer encoding, so the chunked-body tests need HTTP/1.1. + mockWebServer.protocols = listOf(Protocol.HTTP_1_1) mockWebServer.useHttps(serverCertificates.sslSocketFactory(), false) mockWebServer.start() @@ -105,6 +110,91 @@ class OkHttpSynchronousHttpClientTest { assertTrue(response.timing.endTime >= response.timing.startTime) } + @Test + fun executeRequestForBytes_returnsBinaryBodyUnchanged() { + // PNG signature plus bytes that are invalid UTF-8, which a String round-trip would corrupt + val bytes = byteArrayOf(0x89.toByte(), 0x50, 0x4E, 0x47, 0xFF.toByte(), 0xFE.toByte(), 0x00) + mockWebServer.enqueue(MockResponse().setBody(Buffer().write(bytes)).setResponseCode(200)) + + val result = sut.executeRequestForBytes(bytesRequest("/image")) + + assertArrayEquals(bytes, result) + } + + @Test + fun executeRequestForBytes_declaredLengthOverLimit_throwsIOException() { + mockWebServer.enqueue(MockResponse().setBody(Buffer().write(ByteArray(2048))).setResponseCode(200)) + + val error = assertThrows { sut.executeRequestForBytes(bytesRequest("/big"), maxBodyBytes = 1024L) } + + assertTrue(error.message, error.message.orEmpty().contains("exceeds 1024 bytes")) + } + + @Test + fun executeRequestForBytes_chunkedBodyOverLimit_throwsIOException() { + // A chunked response carries no Content-Length, so only the streaming bound can reject it. + mockWebServer.enqueue(MockResponse().setChunkedBody(Buffer().write(ByteArray(4096)), 512)) + + val error = assertThrows { + sut.executeRequestForBytes(bytesRequest("/chunked"), maxBodyBytes = 1024L) + } + + assertTrue(error.message, error.message.orEmpty().contains("exceeds 1024 bytes")) + } + + @Test + fun executeRequestForBytes_chunkedBodyWithinLimit_returnsBody() { + val bytes = ByteArray(4096) { (it % 251).toByte() } + mockWebServer.enqueue(MockResponse().setChunkedBody(Buffer().write(bytes), 512)) + + val result = sut.executeRequestForBytes(bytesRequest("/chunked"), maxBodyBytes = 8192L) + + assertArrayEquals(bytes, result) + } + + @Test + fun executeRequestForBytes_unsuccessfulResponse_throwsIOException() { + mockWebServer.enqueue(MockResponse().setResponseCode(404).setBody("not found")) + + val error = assertThrows { sut.executeRequestForBytes(bytesRequest("/missing")) } + + assertTrue(error.message, error.message.orEmpty().contains("404")) + } + + @Test + fun executeRequestForBytes_httpsRedirectedToHttp_throwsIOException() { + val plainServer = MockWebServer() + plainServer.enqueue(MockResponse().setBody("downgraded").setResponseCode(200)) + plainServer.start() + try { + mockWebServer.enqueue( + MockResponse() + .setResponseCode(302) + .setHeader("Location", plainServer.url("/downgraded").toString()) + ) + + val error = assertThrows { sut.executeRequestForBytes(bytesRequest("/redirect")) } + + // Asserting the message rules out the request failing for another reason, such as cleartext being blocked. + assertTrue(error.message, error.message.orEmpty().contains("Redirected from https to a non-https URL")) + } finally { + plainServer.shutdown() + } + } + + @Test + fun executeRequestForBytes_httpsRedirectedToHttps_returnsBody() { + val bytes = byteArrayOf(1, 2, 3) + mockWebServer.enqueue( + MockResponse().setResponseCode(302).setHeader("Location", mockWebServer.url("/final").toString()) + ) + mockWebServer.enqueue(MockResponse().setBody(Buffer().write(bytes)).setResponseCode(200)) + + val result = sut.executeRequestForBytes(bytesRequest("/redirect")) + + assertArrayEquals(bytes, result) + } + @Test fun executeRequest_sendsCustomHeaders() { mockWebServer.enqueue(MockResponse().setBody("ok").setResponseCode(200)) @@ -118,4 +208,16 @@ class OkHttpSynchronousHttpClientTest { assertEquals("custom-value", recordedRequest.getHeader("X-Custom-Header")) assertEquals("Bearer token123", recordedRequest.getHeader("Authorization")) } + + private fun bytesRequest(path: String) = OkHttpRequest(mockWebServer.url(path).toString(), Method.Get) + + private inline fun assertThrows(block: () -> Unit): T { + try { + block() + } catch (e: Throwable) { + if (e is T) return e + throw e + } + throw AssertionError("Expected ${T::class.java.simpleName} to be thrown") + } } From eb738405ee222343fe8b0451a42bf1b71d378b1e Mon Sep 17 00:00:00 2001 From: Bala Mani Nayar D Date: Thu, 22 Oct 2026 12:01:35 +0530 Subject: [PATCH 11/15] fix(ImageLoader): bound concurrent icon fetches, add clearCache for memory pressure Address review feedback: a burst of distinct icon URLs could previously open one network download per icon with no limit, since only decode (not fetch) was semaphore-bounded. Add a separate fetchSemaphore around the HttpClient call. Also add clearCache(), since the loader holds no Context and can't listen for onTrimMemory itself -- host apps can wire it in from Application or ComponentCallbacks2. --- .../imageloader/ImageLoader.kt | 28 ++++++++++++++-- .../imageloader/ImageLoaderUnitTest.kt | 32 ++++++++++++++++++- 2 files changed, 56 insertions(+), 4 deletions(-) diff --git a/PayPalSavedPaymentMethod/src/main/java/com/braintreepayments/api/paypalsavedpaymentmethod/imageloader/ImageLoader.kt b/PayPalSavedPaymentMethod/src/main/java/com/braintreepayments/api/paypalsavedpaymentmethod/imageloader/ImageLoader.kt index c68f55b206..23fac13779 100644 --- a/PayPalSavedPaymentMethod/src/main/java/com/braintreepayments/api/paypalsavedpaymentmethod/imageloader/ImageLoader.kt +++ b/PayPalSavedPaymentMethod/src/main/java/com/braintreepayments/api/paypalsavedpaymentmethod/imageloader/ImageLoader.kt @@ -32,12 +32,15 @@ import kotlin.coroutines.cancellation.CancellationException /** * Loads funding-instrument icons into memory, de-duplicating concurrent requests for the same URL and bounding how - * many bitmaps are decoded at once. Bytes are fetched through the SDK's pinned [HttpClient]. Dependencies are - * injectable for tests; production code uses [getInstance]. + * many bitmaps are fetched and decoded at once. Bytes are fetched through the SDK's pinned [HttpClient]. Dependencies + * are injectable for tests; production code uses [getInstance]. * * Decodes raster formats only (PNG, JPEG, etc.) via [android.graphics.BitmapFactory]. There is currently no image * loader in Braintree Native, and this custom implementation does not support SVG, matching standard Android image * loading in general (BitmapFactory, and Coil/Glide without an SVG add-on). + * + * Call [clearCache] from the host app's memory-pressure callback to release cached bitmaps; see [clearCache] for why + * this loader can't do that on its own. */ internal class ImageLoader internal constructor( private val httpClient: HttpClient = HttpClient(), @@ -45,6 +48,7 @@ internal class ImageLoader internal constructor( private val mainDispatcher: CoroutineDispatcher = Dispatchers.Main.immediate, private val memoryCache: LruCache = createMemoryCache(), private val decodeSemaphore: Semaphore = Semaphore(DEFAULT_MAX_PARALLEL_DECODES), + private val fetchSemaphore: Semaphore = Semaphore(DEFAULT_MAX_PARALLEL_FETCHES), ) { // Deliberately unscoped to any lifecycle: ImageLoader is a process-wide singleton (see getInstance()), so // in-flight downloads outlive any single screen and are only ever torn down by process death. That is safe because @@ -76,6 +80,16 @@ internal class ImageLoader internal constructor( /** Returns the cached bitmap for [url] without touching the network, or `null` on a cache miss. */ fun peek(url: String): Bitmap? = memoryCache.get(url) + /** + * Evicts every cached bitmap. The loader holds no [android.content.Context] and so cannot register for memory + * callbacks itself; call this from the host app's [android.content.ComponentCallbacks2.onTrimMemory] (e.g. + * `Application.onTrimMemory`) to release the cache under memory pressure. Safe to call at any time -- in-flight + * downloads are unaffected and simply re-populate the cache on completion. + */ + fun clearCache() { + memoryCache.evictAll() + } + /** * Returns the bitmap for [url], from memory when cached. Throws [IllegalArgumentException] if [url] is not an https * URL with a host, and [IOException] if the download or decode fails. @@ -165,7 +179,12 @@ internal class ImageLoader internal constructor( } private suspend fun fetchAndDecode(url: String): Bitmap { - val bytes = httpClient.sendRequestForBytes(OkHttpRequest(url, Method.Get), MAX_IMAGE_BYTES) + // Bounds how many icon downloads run over the network at once; distinct URLs are not deduplicated like + // repeat requests for the same URL are (see inFlightRequest), so without this a burst of distinct icons + // (e.g. a list screen) could open one connection per icon at the same time. + val bytes = fetchSemaphore.withPermit { + httpClient.sendRequestForBytes(OkHttpRequest(url, Method.Get), MAX_IMAGE_BYTES) + } val bitmap = withContext(defaultDispatcher) { decodeSemaphore.withPermit { decodeBitmap(bytes, url) } } @@ -216,6 +235,9 @@ internal class ImageLoader internal constructor( // Bounds concurrent BitmapFactory decodes so many icons loading at once can't spike memory/CPU. private const val DEFAULT_MAX_PARALLEL_DECODES = 4 + // Bounds concurrent icon downloads so a burst of distinct URLs can't open unbounded connections at once. + private const val DEFAULT_MAX_PARALLEL_FETCHES = 4 + // FI icons never render larger than this in any current layout; decoding beyond it wastes memory. private const val MAX_DECODE_DIMENSION_PX = 128 diff --git a/PayPalSavedPaymentMethod/src/test/java/com/braintreepayments/api/paypalsavedpaymentmethod/imageloader/ImageLoaderUnitTest.kt b/PayPalSavedPaymentMethod/src/test/java/com/braintreepayments/api/paypalsavedpaymentmethod/imageloader/ImageLoaderUnitTest.kt index cc52acaa6d..273a1de0d4 100644 --- a/PayPalSavedPaymentMethod/src/test/java/com/braintreepayments/api/paypalsavedpaymentmethod/imageloader/ImageLoaderUnitTest.kt +++ b/PayPalSavedPaymentMethod/src/test/java/com/braintreepayments/api/paypalsavedpaymentmethod/imageloader/ImageLoaderUnitTest.kt @@ -146,6 +146,32 @@ class ImageLoaderUnitTest { assertEquals(32, result.await().width) } + @Test + fun `fetching waits for a fetch permit`() = runTest(testDispatcher) { + val fetchSemaphore = Semaphore(1).apply { acquire() } + sut = createImageLoader(fetchSemaphore = fetchSemaphore) + givenImage(32) + + val result = async { sut.load(URL) } + advanceUntilIdle() + assertFalse(result.isCompleted) + coVerify(exactly = 0) { httpClient.sendRequestForBytes(any(), any()) } + + fetchSemaphore.release() + assertEquals(32, result.await().width) + } + + @Test + fun `clearCache evicts cached bitmaps`() = runTest(testDispatcher) { + givenImage(32) + sut.load(URL) + assertEquals(32, sut.peek(URL)?.width) + + sut.clearCache() + + assertNull(sut.peek(URL)) + } + @Test fun `getInstance always returns the same loader`() { assertSame(ImageLoader.getInstance(), ImageLoader.getInstance()) @@ -155,12 +181,16 @@ class ImageLoaderUnitTest { coEvery { httpClient.sendRequestForBytes(any(), any()) } returns pngBytes(size, size) } - private fun createImageLoader(decodeSemaphore: Semaphore = Semaphore(4)) = ImageLoader( + private fun createImageLoader( + decodeSemaphore: Semaphore = Semaphore(4), + fetchSemaphore: Semaphore = Semaphore(4), + ) = ImageLoader( httpClient = httpClient, defaultDispatcher = testDispatcher, mainDispatcher = testDispatcher, memoryCache = LruCache(4 * 1024 * 1024), decodeSemaphore = decodeSemaphore, + fetchSemaphore = fetchSemaphore, ) companion object { From 8771cd1e214102359a5173a7d787ef04e92e164d Mon Sep 17 00:00:00 2001 From: Bala Mani Nayar D Date: Thu, 22 Oct 2026 12:04:05 +0530 Subject: [PATCH 12/15] docs(CHANGELOG): simplify sendRequestForBytes entry per review suggestion --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 2ef1aa356e..f86cef0b45 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,7 +9,7 @@ * SEPADirectDebit * Add Compose constructor for `SEPADirectDebitLauncher` * SharedUtils - * Add `HttpClient#sendRequestForBytes` for fetching binary payloads with a size cap (5 MB by default), enforced via streaming even when the response omits `Content-Length`, and a 30 second limit on the whole call + * Add `sendRequestForBytes` to `HttpClient` for fetching binary payloads ## 5.33.0 (2026-09-16) From 8e26d86d61928493f7e9941cd4f113eda5a80deb Mon Sep 17 00:00:00 2001 From: Bala Mani Nayar D Date: Thu, 22 Oct 2026 15:29:46 +0530 Subject: [PATCH 13/15] fix(PayPalSavedPaymentMethod): move calculateInSampleSize out of ImageLoader to fix detekt TooManyFunctions ImageLoader reached 11 functions after clearCache was added. calculateInSampleSize uses nothing from the loader, so it is now a private top-level function. --- .../imageloader/ImageLoader.kt | 33 ++++++++++--------- 1 file changed, 17 insertions(+), 16 deletions(-) diff --git a/PayPalSavedPaymentMethod/src/main/java/com/braintreepayments/api/paypalsavedpaymentmethod/imageloader/ImageLoader.kt b/PayPalSavedPaymentMethod/src/main/java/com/braintreepayments/api/paypalsavedpaymentmethod/imageloader/ImageLoader.kt index 23fac13779..46da4ba5e6 100644 --- a/PayPalSavedPaymentMethod/src/main/java/com/braintreepayments/api/paypalsavedpaymentmethod/imageloader/ImageLoader.kt +++ b/PayPalSavedPaymentMethod/src/main/java/com/braintreepayments/api/paypalsavedpaymentmethod/imageloader/ImageLoader.kt @@ -205,22 +205,6 @@ internal class ImageLoader internal constructor( ?: throw IOException("Unable to decode image from $url") } - private fun calculateInSampleSize( - options: BitmapFactory.Options, - reqWidth: Int, - reqHeight: Int, - ): Int { - val height = options.outHeight - val width = options.outWidth - var inSampleSize = 1 - - while (height / inSampleSize > reqHeight || width / inSampleSize > reqWidth) { - inSampleSize *= 2 - } - - return inSampleSize - } - // Also clears a stale image (e.g. from a recycled RecyclerView row) when there is no placeholder. private fun ImageView.setPlaceholder(@DrawableRes placeholder: Int?) { if (placeholder != null) setImageResource(placeholder) else setImageDrawable(null) @@ -257,3 +241,20 @@ internal class ImageLoader internal constructor( } } } + +// Top level rather than a member: it uses nothing from the loader. +private fun calculateInSampleSize( + options: BitmapFactory.Options, + reqWidth: Int, + reqHeight: Int, +): Int { + val height = options.outHeight + val width = options.outWidth + var inSampleSize = 1 + + while (height / inSampleSize > reqHeight || width / inSampleSize > reqWidth) { + inSampleSize *= 2 + } + + return inSampleSize +} From 314d93fc70fe80d148eb5940a07f7d39b7dc6efa Mon Sep 17 00:00:00 2001 From: Bala Mani Nayar D Date: Fri, 23 Oct 2026 00:26:36 +0530 Subject: [PATCH 14/15] fix(PayPalSavedPaymentMethod): block https-to-http image redirects and handle decode OOM Byte downloads use a client that doesn't follow redirects between https and http, so no cleartext request is made. An OutOfMemoryError while decoding an icon now fails like any other decode error, so the view keeps its error placeholder. --- .../imageloader/ImageLoader.kt | 10 +++++-- .../imageloader/ImageLoaderUnitTest.kt | 24 +++++++++++++++++ .../OkHttpSynchronousHttpClientTest.kt | 7 ++--- .../OkHttpSynchronousHttpClient.kt | 19 +++++++------ .../OkHttpSynchronousHttpClientTest.kt | 27 ++++++++++++++----- 5 files changed, 68 insertions(+), 19 deletions(-) diff --git a/PayPalSavedPaymentMethod/src/main/java/com/braintreepayments/api/paypalsavedpaymentmethod/imageloader/ImageLoader.kt b/PayPalSavedPaymentMethod/src/main/java/com/braintreepayments/api/paypalsavedpaymentmethod/imageloader/ImageLoader.kt index 46da4ba5e6..08f5c4c03c 100644 --- a/PayPalSavedPaymentMethod/src/main/java/com/braintreepayments/api/paypalsavedpaymentmethod/imageloader/ImageLoader.kt +++ b/PayPalSavedPaymentMethod/src/main/java/com/braintreepayments/api/paypalsavedpaymentmethod/imageloader/ImageLoader.kt @@ -201,8 +201,14 @@ internal class ImageLoader internal constructor( val options = BitmapFactory.Options().apply { inSampleSize = calculateInSampleSize(bounds, MAX_DECODE_DIMENSION_PX, MAX_DECODE_DIMENSION_PX) } - return BitmapFactory.decodeByteArray(bytes, 0, bytes.size, options) - ?: throw IOException("Unable to decode image from $url") + // An optional icon must never crash the host app, so an out-of-memory decode fails like any other decode + // failure and the view keeps its error placeholder. + val bitmap = try { + BitmapFactory.decodeByteArray(bytes, 0, bytes.size, options) + } catch (e: OutOfMemoryError) { + throw IOException("Not enough memory to decode image from $url", e) + } + return bitmap ?: throw IOException("Unable to decode image from $url") } // Also clears a stale image (e.g. from a recycled RecyclerView row) when there is no placeholder. diff --git a/PayPalSavedPaymentMethod/src/test/java/com/braintreepayments/api/paypalsavedpaymentmethod/imageloader/ImageLoaderUnitTest.kt b/PayPalSavedPaymentMethod/src/test/java/com/braintreepayments/api/paypalsavedpaymentmethod/imageloader/ImageLoaderUnitTest.kt index 273a1de0d4..f6890f53be 100644 --- a/PayPalSavedPaymentMethod/src/test/java/com/braintreepayments/api/paypalsavedpaymentmethod/imageloader/ImageLoaderUnitTest.kt +++ b/PayPalSavedPaymentMethod/src/test/java/com/braintreepayments/api/paypalsavedpaymentmethod/imageloader/ImageLoaderUnitTest.kt @@ -1,14 +1,18 @@ package com.braintreepayments.api.paypalsavedpaymentmethod.imageloader import android.graphics.Bitmap +import android.graphics.BitmapFactory import android.util.LruCache import com.braintreepayments.api.sharedutils.HttpClient import com.braintreepayments.api.sharedutils.Method import com.braintreepayments.api.sharedutils.OkHttpRequest import io.mockk.coEvery import io.mockk.coVerify +import io.mockk.every import io.mockk.mockk +import io.mockk.mockkStatic import io.mockk.slot +import io.mockk.unmockkStatic import kotlinx.coroutines.CompletableDeferred import kotlinx.coroutines.ExperimentalCoroutinesApi import kotlinx.coroutines.async @@ -23,6 +27,7 @@ import org.junit.Assert.assertEquals import org.junit.Assert.assertFalse import org.junit.Assert.assertNull import org.junit.Assert.assertSame +import org.junit.Assert.assertTrue import org.junit.Test import org.junit.runner.RunWith import org.robolectric.RobolectricTestRunner @@ -113,6 +118,25 @@ class ImageLoaderUnitTest { assertNull(sut.peek(URL)) } + @Test + fun `load throws IOException and caches nothing when decoding runs out of memory`() = runTest(testDispatcher) { + givenImage(32) + mockkStatic(BitmapFactory::class) + try { + every { + BitmapFactory.decodeByteArray(any(), any(), any(), match { !it.inJustDecodeBounds }) + } throws OutOfMemoryError() + + val error = assertFailsWith { sut.load(URL) } + + // Checked by message: coroutine stack-trace recovery may wrap the exception in a copy. + assertTrue(error.message.orEmpty().contains("Not enough memory")) + assertNull(sut.peek(URL)) + } finally { + unmockkStatic(BitmapFactory::class) + } + } + @Test fun `load throws IllegalArgumentException for a blank or non-https url without making a request`() = runTest(testDispatcher) { diff --git a/SharedUtils/src/androidTest/java/com/braintreepayments/api/sharedutils/OkHttpSynchronousHttpClientTest.kt b/SharedUtils/src/androidTest/java/com/braintreepayments/api/sharedutils/OkHttpSynchronousHttpClientTest.kt index a68fd36338..d598148620 100644 --- a/SharedUtils/src/androidTest/java/com/braintreepayments/api/sharedutils/OkHttpSynchronousHttpClientTest.kt +++ b/SharedUtils/src/androidTest/java/com/braintreepayments/api/sharedutils/OkHttpSynchronousHttpClientTest.kt @@ -162,7 +162,7 @@ class OkHttpSynchronousHttpClientTest { } @Test - fun executeRequestForBytes_httpsRedirectedToHttp_throwsIOException() { + fun executeRequestForBytes_httpsRedirectedToHttp_throwsIOExceptionWithoutCleartextRequest() { val plainServer = MockWebServer() plainServer.enqueue(MockResponse().setBody("downgraded").setResponseCode(200)) plainServer.start() @@ -175,8 +175,9 @@ class OkHttpSynchronousHttpClientTest { val error = assertThrows { sut.executeRequestForBytes(bytesRequest("/redirect")) } - // Asserting the message rules out the request failing for another reason, such as cleartext being blocked. - assertTrue(error.message, error.message.orEmpty().contains("Redirected from https to a non-https URL")) + // The redirect is never followed, so the cleartext server is never contacted. + assertTrue(error.message, error.message.orEmpty().contains("302")) + assertEquals(0, plainServer.requestCount) } finally { plainServer.shutdown() } diff --git a/SharedUtils/src/main/java/com/braintreepayments/api/sharedutils/OkHttpSynchronousHttpClient.kt b/SharedUtils/src/main/java/com/braintreepayments/api/sharedutils/OkHttpSynchronousHttpClient.kt index f4e1441bcd..d86a5c5e99 100644 --- a/SharedUtils/src/main/java/com/braintreepayments/api/sharedutils/OkHttpSynchronousHttpClient.kt +++ b/SharedUtils/src/main/java/com/braintreepayments/api/sharedutils/OkHttpSynchronousHttpClient.kt @@ -28,6 +28,13 @@ internal class OkHttpSynchronousHttpClient( .build(), ) { + // OkHttp follows redirects between https and http by default. Byte downloads must never make a cleartext hop, so + // they use a client that doesn't follow a redirect that changes the scheme; it shares the connection pool and TLS + // setup of [okHttpClient]. + private val bytesOkHttpClient: OkHttpClient by lazy { + okHttpClient.newBuilder().followSslRedirects(false).build() + } + /** * Executes a synchronous HTTP request using OkHttp. * @@ -65,9 +72,9 @@ internal class OkHttpSynchronousHttpClient( * declared `Content-Length` exceeds it, or, if undeclared (e.g. chunked transfer), as soon as actual bytes read * exceed it — the body is never buffered past [maxBodyBytes] + 1 bytes. * @return The response body bytes. - * @throws IOException if the request fails, the response is unsuccessful or has no body, an https request is - * redirected to a non-https URL, the body size exceeds [maxBodyBytes], or the whole call, body included, takes - * longer than [BYTES_CALL_TIMEOUT_SECONDS]. + * @throws IOException if the request fails, the response is unsuccessful or has no body (including a redirect + * between https and http, which is never followed), the body size exceeds [maxBodyBytes], or the whole call, body + * included, takes longer than [BYTES_CALL_TIMEOUT_SECONDS]. * @throws IllegalArgumentException if the request URL is not a valid http or https URL. */ @Throws(IOException::class) @@ -76,15 +83,11 @@ internal class OkHttpSynchronousHttpClient( // The connect and read timeouts only bound each step, so a server that trickles bytes could hold the call open // for as long as it keeps sending; this bounds the whole call. - val call = okHttpClient.newCall(request) + val call = bytesOkHttpClient.newCall(request) call.timeout().timeout(BYTES_CALL_TIMEOUT_SECONDS, TimeUnit.SECONDS) call.execute().use { response -> val responseBody = response.body?.takeIf { response.isSuccessful } ?: throw IOException("Unexpected code ${response.code} for ${request.url}") - // OkHttp follows redirects, including https to http; never accept a downgraded download. - if (request.url.isHttps && !response.request.url.isHttps) { - throw IOException("Redirected from https to a non-https URL for ${request.url}") - } return responseBody.readBoundedBytes(maxBodyBytes, request.url.toString()) } } diff --git a/SharedUtils/src/test/java/com/braintreepayments/api/sharedutils/OkHttpSynchronousHttpClientTest.kt b/SharedUtils/src/test/java/com/braintreepayments/api/sharedutils/OkHttpSynchronousHttpClientTest.kt index 066d85a8c7..76ac007a8a 100644 --- a/SharedUtils/src/test/java/com/braintreepayments/api/sharedutils/OkHttpSynchronousHttpClientTest.kt +++ b/SharedUtils/src/test/java/com/braintreepayments/api/sharedutils/OkHttpSynchronousHttpClientTest.kt @@ -27,6 +27,7 @@ import kotlin.test.assertFailsWith class OkHttpSynchronousHttpClientTest { private lateinit var okHttpClient: OkHttpClient + private lateinit var bytesOkHttpClientBuilder: OkHttpClient.Builder private lateinit var certificatePinner: CertificatePinner private lateinit var sut: OkHttpSynchronousHttpClient @@ -38,6 +39,10 @@ class OkHttpSynchronousHttpClientTest { fun setUp() { okHttpClient = mockk(relaxed = true) certificatePinner = mockk(relaxed = true) + bytesOkHttpClientBuilder = mockk() + every { okHttpClient.newBuilder() } returns bytesOkHttpClientBuilder + every { bytesOkHttpClientBuilder.followSslRedirects(false) } returns bytesOkHttpClientBuilder + every { bytesOkHttpClientBuilder.build() } returns okHttpClient sut = OkHttpSynchronousHttpClient( okHttpClient = okHttpClient @@ -203,10 +208,22 @@ class OkHttpSynchronousHttpClientTest { } @Test - fun `when an https request is redirected to http, executeRequestForBytes throws IOException`() { - givenBytesResponse(isSuccessful = true, body = byteArrayOf(1).toResponseBody(), finalUrl = "http://example.com") + fun `executeRequestForBytes uses a client that does not follow redirects between https and http`() { + givenBytesResponse(isSuccessful = true, body = byteArrayOf(1).toResponseBody()) - assertFailsWith { sut.executeRequestForBytes(OkHttpRequest(URL, Method.Get)) } + sut.executeRequestForBytes(OkHttpRequest(URL, Method.Get)) + + verify { bytesOkHttpClientBuilder.followSslRedirects(false) } + } + + @Test + fun `when the response is a redirect that was not followed, executeRequestForBytes throws IOException`() { + givenBytesResponse(isSuccessful = false, body = byteArrayOf(1).toResponseBody()) + every { response.code } returns 302 + + val error = assertFailsWith { sut.executeRequestForBytes(OkHttpRequest(URL, Method.Get)) } + + assertTrue(error.message.orEmpty().contains("302")) } @Test @@ -221,13 +238,11 @@ class OkHttpSynchronousHttpClientTest { assertArrayEquals(bytes, result) } - private fun givenBytesResponse(isSuccessful: Boolean, body: ResponseBody?, finalUrl: String = URL) { + private fun givenBytesResponse(isSuccessful: Boolean, body: ResponseBody?) { every { okHttpClient.newCall(any()) } returns call every { call.execute() } returns response every { response.isSuccessful } returns isSuccessful every { response.body } returns body - // The request OkHttp ended on after following any redirects. - every { response.request } returns Request.Builder().url(finalUrl).build() } companion object { From 9f2d0d829bac373bb67f837d4386908f42e5a67e Mon Sep 17 00:00:00 2001 From: Bala Mani Nayar D Date: Fri, 23 Oct 2026 00:31:51 +0530 Subject: [PATCH 15/15] docs(SharedUtils): document unfollowed redirects and call timeout for sendRequestForBytes Align the HttpClient.sendRequestForBytes KDoc with the underlying client, and note that an out-of-memory decode fails ImageLoader.load with an IOException. --- .../api/paypalsavedpaymentmethod/imageloader/ImageLoader.kt | 2 +- .../java/com/braintreepayments/api/sharedutils/HttpClient.kt | 5 +++-- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/PayPalSavedPaymentMethod/src/main/java/com/braintreepayments/api/paypalsavedpaymentmethod/imageloader/ImageLoader.kt b/PayPalSavedPaymentMethod/src/main/java/com/braintreepayments/api/paypalsavedpaymentmethod/imageloader/ImageLoader.kt index 08f5c4c03c..817ba71239 100644 --- a/PayPalSavedPaymentMethod/src/main/java/com/braintreepayments/api/paypalsavedpaymentmethod/imageloader/ImageLoader.kt +++ b/PayPalSavedPaymentMethod/src/main/java/com/braintreepayments/api/paypalsavedpaymentmethod/imageloader/ImageLoader.kt @@ -92,7 +92,7 @@ internal class ImageLoader internal constructor( /** * Returns the bitmap for [url], from memory when cached. Throws [IllegalArgumentException] if [url] is not an https - * URL with a host, and [IOException] if the download or decode fails. + * URL with a host, and [IOException] if the download or decode fails, including when decoding runs out of memory. */ suspend fun load(url: String): Bitmap { val validUrl = requireNotNull(url.asHttpsUrlOrNull()) { "url must be an https URL with a host" } diff --git a/SharedUtils/src/main/java/com/braintreepayments/api/sharedutils/HttpClient.kt b/SharedUtils/src/main/java/com/braintreepayments/api/sharedutils/HttpClient.kt index 2dda914a9d..3cad79e4ec 100644 --- a/SharedUtils/src/main/java/com/braintreepayments/api/sharedutils/HttpClient.kt +++ b/SharedUtils/src/main/java/com/braintreepayments/api/sharedutils/HttpClient.kt @@ -29,8 +29,9 @@ class HttpClient internal constructor( * * @param maxBodyBytes Limit on the response body size, 5 MB by default, enforced even if the response omits * `Content-Length` (e.g. chunked transfer). - * @throws java.io.IOException if the network request fails, the response is unsuccessful or has no body, or the - * body size exceeds [maxBodyBytes] + * @throws java.io.IOException if the network request fails, the response is unsuccessful or has no body + * (including a redirect between https and http, which is never followed), the body size exceeds [maxBodyBytes], + * or the whole call takes longer than 30 seconds * @throws IllegalArgumentException if the request URL is not a valid http or https URL */ suspend fun sendRequestForBytes(