From 4173fcfc53962d5168b4a083a9a2ab98fc1cfa62 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Saleniuk?= Date: Mon, 14 Aug 2023 15:16:53 +0200 Subject: [PATCH 1/3] chore: add handling NoNetworkConnection in image loader --- .../com/wire/android/di/CoreLogicModule.kt | 2 +- .../com/wire/android/di/ImageLoadingModule.kt | 2 +- .../ui/home/conversations/mock/Mock.kt | 4 +-- .../wire/android/util/ui/AssetImageFetcher.kt | 26 +++++++++++++++---- .../android/util/ui/WireSessionImageLoader.kt | 25 ++++++++++++++---- kalium | 2 +- 6 files changed, 46 insertions(+), 15 deletions(-) diff --git a/app/src/main/kotlin/com/wire/android/di/CoreLogicModule.kt b/app/src/main/kotlin/com/wire/android/di/CoreLogicModule.kt index f5a6a00cb3a..6e7b1b7759d 100644 --- a/app/src/main/kotlin/com/wire/android/di/CoreLogicModule.kt +++ b/app/src/main/kotlin/com/wire/android/di/CoreLogicModule.kt @@ -125,7 +125,7 @@ import com.wire.kalium.logic.feature.user.UpdateEmailUseCase import com.wire.kalium.logic.feature.user.screenshotCensoring.ObserveScreenshotCensoringConfigUseCase import com.wire.kalium.logic.feature.user.screenshotCensoring.PersistScreenshotCensoringConfigUseCase import com.wire.kalium.logic.featureFlags.KaliumConfigs -import com.wire.kalium.logic.network.NetworkStateObserver +import com.wire.kalium.network.NetworkStateObserver import dagger.Module import dagger.Provides import dagger.hilt.InstallIn diff --git a/app/src/main/kotlin/com/wire/android/di/ImageLoadingModule.kt b/app/src/main/kotlin/com/wire/android/di/ImageLoadingModule.kt index 860f1c48f53..f661cba425f 100644 --- a/app/src/main/kotlin/com/wire/android/di/ImageLoadingModule.kt +++ b/app/src/main/kotlin/com/wire/android/di/ImageLoadingModule.kt @@ -25,7 +25,7 @@ import com.wire.android.util.ui.WireSessionImageLoader import com.wire.kalium.logic.feature.asset.DeleteAssetUseCase import com.wire.kalium.logic.feature.asset.GetAvatarAssetUseCase import com.wire.kalium.logic.feature.asset.GetMessageAssetUseCase -import com.wire.kalium.logic.network.NetworkStateObserver +import com.wire.kalium.network.NetworkStateObserver import dagger.Module import dagger.Provides import dagger.hilt.InstallIn diff --git a/app/src/main/kotlin/com/wire/android/ui/home/conversations/mock/Mock.kt b/app/src/main/kotlin/com/wire/android/ui/home/conversations/mock/Mock.kt index c53d47abf11..9c50a96780b 100644 --- a/app/src/main/kotlin/com/wire/android/ui/home/conversations/mock/Mock.kt +++ b/app/src/main/kotlin/com/wire/android/ui/home/conversations/mock/Mock.kt @@ -50,8 +50,8 @@ import com.wire.kalium.logic.data.message.Message import com.wire.kalium.logic.data.user.ConnectionState import com.wire.kalium.logic.data.user.UserAssetId import com.wire.kalium.logic.data.user.UserAvailabilityStatus -import com.wire.kalium.logic.network.NetworkState -import com.wire.kalium.logic.network.NetworkStateObserver +import com.wire.kalium.network.NetworkState +import com.wire.kalium.network.NetworkStateObserver import kotlinx.coroutines.flow.MutableStateFlow import kotlinx.coroutines.flow.StateFlow diff --git a/app/src/main/kotlin/com/wire/android/util/ui/AssetImageFetcher.kt b/app/src/main/kotlin/com/wire/android/util/ui/AssetImageFetcher.kt index c2c745cd173..adf2a624975 100644 --- a/app/src/main/kotlin/com/wire/android/util/ui/AssetImageFetcher.kt +++ b/app/src/main/kotlin/com/wire/android/util/ui/AssetImageFetcher.kt @@ -29,6 +29,8 @@ import coil.fetch.Fetcher import coil.request.Options import com.wire.android.model.ImageAsset import com.wire.android.util.toDrawable +import com.wire.kalium.logic.CoreFailure +import com.wire.kalium.logic.NetworkFailure import com.wire.kalium.logic.feature.asset.DeleteAssetUseCase import com.wire.kalium.logic.feature.asset.GetAvatarAssetUseCase import com.wire.kalium.logic.feature.asset.GetMessageAssetUseCase @@ -55,13 +57,13 @@ internal class AssetImageFetcher( return when (data) { is ImageAsset.UserAvatarAsset -> { val retryHash = options.parameters.value(OPTION_PARAMETER_RETRY_KEY) ?: DEFAULT_RETRY_ATTEMPT - if (retryHash >= RETRY_ATTEMPT_TO_DELETE_ASSET) { deleteAsset(data.userAssetId) } - when (val result = getPublicAsset(data.userAssetId)) { - is PublicAssetResult.Failure -> throw AssetImageException(result.isRetryNeeded) + is PublicAssetResult.Failure -> + throw AssetImageException(retryPolicy(result.isRetryNeeded, result.coreFailure)) + is PublicAssetResult.Success -> { drawableResultWrapper.toFetchResult(result.assetPath) } @@ -70,7 +72,9 @@ internal class AssetImageFetcher( is ImageAsset.PrivateAsset -> { when (val result = getPrivateAsset(data.conversationId, data.messageId).await()) { - is MessageAssetResult.Failure -> throw AssetImageException(result.isRetryNeeded) + is MessageAssetResult.Failure -> + throw AssetImageException(retryPolicy(result.isRetryNeeded, result.coreFailure)) + is MessageAssetResult.Success -> { drawableResultWrapper.toFetchResult(result.decodedAssetPath) } @@ -90,6 +94,12 @@ internal class AssetImageFetcher( } } + private fun retryPolicy(isRetryNeeded: Boolean, coreFailure: CoreFailure): AssetImageRetryPolicy = when { + !isRetryNeeded -> AssetImageRetryPolicy.DO_NOT_RETRY + coreFailure is NetworkFailure.NoNetworkConnection -> AssetImageRetryPolicy.RETRY_WHEN_CONNECTED + else -> AssetImageRetryPolicy.EXPONENTIAL_RETRY_WHEN_CONNECTED + } + class Factory( private val getPublicAssetUseCase: GetAvatarAssetUseCase, private val getPrivateAssetUseCase: GetMessageAssetUseCase, @@ -117,4 +127,10 @@ data class AssetFetcherParameters( val options: Options ) -data class AssetImageException(val isRetryNeeded: Boolean) : Exception("Load asset image exception") +data class AssetImageException(val retryPolicy: AssetImageRetryPolicy) : Exception("Load asset image exception") + +enum class AssetImageRetryPolicy { + RETRY_WHEN_CONNECTED, + EXPONENTIAL_RETRY_WHEN_CONNECTED, + DO_NOT_RETRY +} diff --git a/app/src/main/kotlin/com/wire/android/util/ui/WireSessionImageLoader.kt b/app/src/main/kotlin/com/wire/android/util/ui/WireSessionImageLoader.kt index 36939c1d0bc..c91d7a666b2 100644 --- a/app/src/main/kotlin/com/wire/android/util/ui/WireSessionImageLoader.kt +++ b/app/src/main/kotlin/com/wire/android/util/ui/WireSessionImageLoader.kt @@ -43,9 +43,13 @@ import com.wire.android.model.ImageAsset import com.wire.kalium.logic.feature.asset.DeleteAssetUseCase import com.wire.kalium.logic.feature.asset.GetAvatarAssetUseCase import com.wire.kalium.logic.feature.asset.GetMessageAssetUseCase -import com.wire.kalium.logic.network.NetworkState -import com.wire.kalium.logic.network.NetworkStateObserver +import com.wire.kalium.logic.util.ExponentialDurationHelperImpl +import com.wire.kalium.network.NetworkState +import com.wire.kalium.network.NetworkStateObserver +import kotlinx.coroutines.delay import kotlinx.coroutines.flow.firstOrNull +import kotlin.time.Duration.Companion.minutes +import kotlin.time.Duration.Companion.seconds /** * An ImageLoader that is able to load AssetIds supplied by Kalium. @@ -59,6 +63,8 @@ class WireSessionImageLoader( ) { private companion object { const val RETRY_INCREMENT_ATTEMPT_PER_STEP = 1 + val MIN_RETRY_DELAY = 1.seconds + val MAX_RETRY_DELAY = 10.minutes } /** @@ -74,6 +80,7 @@ class WireSessionImageLoader( fallbackData: Any? = null ): Painter { var retryHash by remember { mutableStateOf(0) } + val exponentialDurationHelper = remember { ExponentialDurationHelperImpl(MIN_RETRY_DELAY, MAX_RETRY_DELAY) } val painter = rememberAsyncImagePainter( model = ImageRequest.Builder(LocalContext.current) .memoryCacheKey(asset?.uniqueKey) @@ -85,18 +92,26 @@ class WireSessionImageLoader( ) .build(), error = (fallbackData as? Int)?.let { painterResource(id = it) }, + placeholder = (fallbackData as? Int)?.let { painterResource(id = it) }, imageLoader = coilImageLoader ) LaunchedEffect(painter.state) { if (painter.state is AsyncImagePainter.State.Error) { - val isRetryNeeded = - ((painter.state as AsyncImagePainter.State.Error).result.throwable as? AssetImageException)?.isRetryNeeded ?: false - if (isRetryNeeded) { + val retryPolicy = ((painter.state as AsyncImagePainter.State.Error).result.throwable as? AssetImageException)?.retryPolicy + ?: AssetImageRetryPolicy.DO_NOT_RETRY + + if (retryPolicy == AssetImageRetryPolicy.EXPONENTIAL_RETRY_WHEN_CONNECTED) { + delay(exponentialDurationHelper.next()) + } + + if (retryPolicy != AssetImageRetryPolicy.DO_NOT_RETRY) { networkStateObserver.observeNetworkState().firstOrNull { it == NetworkState.ConnectedWithInternet }.let { retryHash += RETRY_INCREMENT_ATTEMPT_PER_STEP } } + } else { + exponentialDurationHelper.reset() } } return painter diff --git a/kalium b/kalium index fe7a70e2aaa..e4061b547d9 160000 --- a/kalium +++ b/kalium @@ -1 +1 @@ -Subproject commit fe7a70e2aaa57b7b00785c834aa07f2d04b0bf58 +Subproject commit e4061b547d9dc3dfe1e6512b956fa6d4f3186a3c From 7efcc11ecfd0b437da052f3bd6b0603fd4ca7b7d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Saleniuk?= Date: Mon, 14 Aug 2023 15:20:01 +0200 Subject: [PATCH 2/3] change kalium ref --- kalium | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/kalium b/kalium index e4061b547d9..3948c6fa7be 160000 --- a/kalium +++ b/kalium @@ -1 +1 @@ -Subproject commit e4061b547d9dc3dfe1e6512b956fa6d4f3186a3c +Subproject commit 3948c6fa7be52ca5e2195e33e8c448babf394806 From 41d386b353913b0b725f76638f7f17df24a451d5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Saleniuk?= Date: Mon, 14 Aug 2023 16:43:53 +0200 Subject: [PATCH 3/3] add tests for handling failures in AssetImageFetcher --- .../android/util/ui/AssetImageFetcherTest.kt | 147 +++++++++++++++++- 1 file changed, 144 insertions(+), 3 deletions(-) diff --git a/app/src/test/kotlin/com/wire/android/util/ui/AssetImageFetcherTest.kt b/app/src/test/kotlin/com/wire/android/util/ui/AssetImageFetcherTest.kt index 9461492b507..4b1b07643d4 100644 --- a/app/src/test/kotlin/com/wire/android/util/ui/AssetImageFetcherTest.kt +++ b/app/src/test/kotlin/com/wire/android/util/ui/AssetImageFetcherTest.kt @@ -28,6 +28,7 @@ import com.wire.android.framework.FakeKaliumFileSystem import com.wire.android.model.ImageAsset import com.wire.android.util.ui.AssetImageFetcher.Companion.OPTION_PARAMETER_RETRY_KEY import com.wire.kalium.logic.CoreFailure +import com.wire.kalium.logic.NetworkFailure import com.wire.kalium.logic.data.id.ConversationId import com.wire.kalium.logic.data.user.AssetId import com.wire.kalium.logic.feature.asset.DeleteAssetUseCase @@ -43,6 +44,7 @@ import kotlinx.coroutines.ExperimentalCoroutinesApi import kotlinx.coroutines.test.runTest import okio.Path import okio.buffer +import org.junit.jupiter.api.Assertions.assertEquals import org.junit.jupiter.api.Test import org.junit.jupiter.api.assertThrows @@ -144,6 +146,141 @@ internal class AssetImageFetcherTest { coVerify(inverse = true) { arrangement.drawableResultWrapper.toFetchResult(any()) } } + @Test + fun givenAUserAvatarAssetData_WhenCallingFetchReturnsFailureWithRetryNotNeeded_ThenThrowExceptionWithDoNotRetryPolicy() = + runTest { + // Given + val someUserAssetId = AssetId("value", "domain") + val data = ImageAsset.UserAvatarAsset(mockk(), someUserAssetId) + val (arrangement, assetImageFetcher) = Arrangement() + .withErrorResponse( + data = data, + isRetryNeeded = false, + coreFailure = CoreFailure.Unknown(null) + ) + .arrange() + + // When + val exception = assertThrows { assetImageFetcher.fetch() } + + // Then + assertEquals(AssetImageRetryPolicy.DO_NOT_RETRY, exception.retryPolicy) + coVerify(inverse = true) { arrangement.drawableResultWrapper.toFetchResult(any()) } + } + + @Test + fun givenAUserAvatarAssetData_WhenCallingFetchReturnsNoConnectionFailure_ThenThrowExceptionWithRetryPolicy() = + runTest { + // Given + val someUserAssetId = AssetId("value", "domain") + val data = ImageAsset.UserAvatarAsset(mockk(), someUserAssetId) + val (arrangement, assetImageFetcher) = Arrangement() + .withErrorResponse( + data = data, + isRetryNeeded = true, + coreFailure = NetworkFailure.NoNetworkConnection(null) + ) + .arrange() + + // When + val exception = assertThrows { assetImageFetcher.fetch() } + + // Then + assertEquals(AssetImageRetryPolicy.RETRY_WHEN_CONNECTED, exception.retryPolicy) + coVerify(inverse = true) { arrangement.drawableResultWrapper.toFetchResult(any()) } + } + + @Test + fun givenAUserAvatarAssetData_WhenCallingFetchReturnsFailureWithRetryNeeded_ThenThrowExceptionWithExponentialRetryPolicy() = + runTest { + // Given + val someUserAssetId = AssetId("value", "domain") + val data = ImageAsset.UserAvatarAsset(mockk(), someUserAssetId) + val (arrangement, assetImageFetcher) = Arrangement() + .withErrorResponse( + data = data, + isRetryNeeded = true, + coreFailure = CoreFailure.Unknown(null) + ) + .arrange() + + // When + val exception = assertThrows { assetImageFetcher.fetch() } + + // Then + assertEquals(AssetImageRetryPolicy.EXPONENTIAL_RETRY_WHEN_CONNECTED, exception.retryPolicy) + coVerify(inverse = true) { arrangement.drawableResultWrapper.toFetchResult(any()) } + } + + @Test + fun givenAPrivateAssetImageData_WhenCallingFetchReturnsFailureWithRetryNotNeeded_ThenThrowExceptionWithDoNotRetryPolicy() = + runTest { + // Given + val someConversationId = ConversationId("some-value", "some-domain") + val someMessageId = "some-message-id" + val data = ImageAsset.PrivateAsset(mockk(), someConversationId, someMessageId, true) + val (arrangement, assetImageFetcher) = Arrangement() + .withErrorResponse( + data = data, + isRetryNeeded = false, + coreFailure = CoreFailure.Unknown(null) + ) + .arrange() + + // When + val exception = assertThrows { assetImageFetcher.fetch() } + + // Then + assertEquals(AssetImageRetryPolicy.DO_NOT_RETRY, exception.retryPolicy) + coVerify(inverse = true) { arrangement.drawableResultWrapper.toFetchResult(any()) } + } + + @Test + fun givenAPrivateAssetImageData_WhenCallingFetchReturnsNoConnectionFailure_ThenThrowExceptionWithRetryPolicy() = + runTest { + // Given + val someConversationId = ConversationId("some-value", "some-domain") + val someMessageId = "some-message-id" + val data = ImageAsset.PrivateAsset(mockk(), someConversationId, someMessageId, true) + val (arrangement, assetImageFetcher) = Arrangement() + .withErrorResponse( + data = data, + isRetryNeeded = true, + coreFailure = NetworkFailure.NoNetworkConnection(null) + ) + .arrange() + + // When + val exception = assertThrows { assetImageFetcher.fetch() } + + // Then + assertEquals(AssetImageRetryPolicy.RETRY_WHEN_CONNECTED, exception.retryPolicy) + coVerify(inverse = true) { arrangement.drawableResultWrapper.toFetchResult(any()) } + } + + @Test + fun givenAPrivateAssetImageData_WhenCallingFetchReturnsFailureWithRetryNeeded_ThenThrowExceptionWithExponentialRetryPolicy() = + runTest { + // Given + val someConversationId = ConversationId("some-value", "some-domain") + val someMessageId = "some-message-id" + val data = ImageAsset.PrivateAsset(mockk(), someConversationId, someMessageId, true) + val (arrangement, assetImageFetcher) = Arrangement() + .withErrorResponse( + data = data, + isRetryNeeded = true, + coreFailure = CoreFailure.Unknown(null) + ) + .arrange() + + // When + val exception = assertThrows { assetImageFetcher.fetch() } + + // Then + assertEquals(AssetImageRetryPolicy.EXPONENTIAL_RETRY_WHEN_CONNECTED, exception.retryPolicy) + coVerify(inverse = true) { arrangement.drawableResultWrapper.toFetchResult(any()) } + } + private class Arrangement { val getPublicAsset = mockk() val getPrivateAsset = mockk() @@ -198,11 +335,15 @@ internal class AssetImageFetcherTest { return this } - fun withErrorResponse(data: ImageAsset, isRetryNeeded: Boolean = false): Arrangement { + fun withErrorResponse( + data: ImageAsset, + isRetryNeeded: Boolean = false, + coreFailure: CoreFailure = CoreFailure.Unknown(null) + ): Arrangement { imageData = data coEvery { getPublicAsset.invoke((any())) }.returns( PublicAssetResult.Failure( - CoreFailure.Unknown(null), + coreFailure, isRetryNeeded ) ) @@ -214,7 +355,7 @@ internal class AssetImageFetcherTest { }.returns( CompletableDeferred( MessageAssetResult.Failure( - CoreFailure.Unknown(null), + coreFailure, isRetryNeeded ) )