From 549c8febf35eacd75fdafae327f2cba45424c0d2 Mon Sep 17 00:00:00 2001 From: Zsolt Bertalan Date: Mon, 16 Dec 2024 20:32:34 +0000 Subject: [PATCH] Fix #5 Incorporate kotlin-result-retry --- .../main/kotlin/data-convention.gradle.kts | 1 + config/detekt/default-detekt-config.yml | 2 +- gradle/libs.versions.toml | 3 + .../movies/data/repository/MoviesAccessor.kt | 4 + shared-data/build.gradle.kts | 6 +- .../data/getresult/fetchCacheThenNetwork.kt | 228 ------------------ .../data/getresult/fetchCacheThenRemote.kt | 133 ++++++++++ .../data/getresult/fetchNetworkFirst.kt | 5 +- .../flickslate/shared/data/retry/Retry.kt | 52 ++++ .../FetchCacheThenRemoteLaterTest.kt | 45 ++++ .../getresult/FetchCacheThenRemoteOnceTest.kt | 46 ++++ .../getresult/FetchCacheThenRemoteTest.kt | 44 ++++ .../data/getresult/FetchRemoteFirstTest.kt | 47 ++++ ...workRequest.kt => NetworkRequestMother.kt} | 18 ++ .../flickslate/shared/compose/design/Color.kt | 2 +- .../compose/util/recomposeHighlighter.kt | 7 +- .../tv/data/repository/TvAccessor.kt | 2 + 17 files changed, 411 insertions(+), 234 deletions(-) delete mode 100644 shared-data/src/main/java/com/zsoltbertalan/flickslate/shared/data/getresult/fetchCacheThenNetwork.kt create mode 100644 shared-data/src/main/java/com/zsoltbertalan/flickslate/shared/data/getresult/fetchCacheThenRemote.kt create mode 100644 shared-data/src/main/java/com/zsoltbertalan/flickslate/shared/data/retry/Retry.kt rename shared-data/src/test/java/com/zsoltbertalan/flickslate/shared/data/getresult/{NetworkRequest.kt => NetworkRequestMother.kt} (71%) diff --git a/build-logic/convention/src/main/kotlin/data-convention.gradle.kts b/build-logic/convention/src/main/kotlin/data-convention.gradle.kts index 9a68008..1edf08e 100644 --- a/build-logic/convention/src/main/kotlin/data-convention.gradle.kts +++ b/build-logic/convention/src/main/kotlin/data-convention.gradle.kts @@ -22,6 +22,7 @@ dependencies { "implementation"(libs.findLibrary("kotlinx.coroutines.core").get()) "implementation"(libs.findLibrary("kotlinx.serialization.core").get()) "implementation"(libs.findLibrary("kotlinResult.result").get()) + "implementation"(libs.findLibrary("kotlinRetry").get()) "implementation"(libs.findLibrary("kotlinx.collections.immutable.jvm").get()) "api"(libs.findLibrary("squareUp.retrofit2.retrofit").get()) "implementation"(libs.findLibrary("squareUp.okhttp3.okhttp").get()) diff --git a/config/detekt/default-detekt-config.yml b/config/detekt/default-detekt-config.yml index 95c287c..f65626f 100644 --- a/config/detekt/default-detekt-config.yml +++ b/config/detekt/default-detekt-config.yml @@ -610,7 +610,7 @@ style: - '1' - '2' ignoreHashCodeFunction: true - ignorePropertyDeclaration: true + ignorePropertyDeclaration: false ignoreLocalVariableDeclaration: false ignoreConstantDeclaration: true ignoreCompanionObjectPropertyDeclaration: true diff --git a/gradle/libs.versions.toml b/gradle/libs.versions.toml index faab681..674a82e 100644 --- a/gradle/libs.versions.toml +++ b/gradle/libs.versions.toml @@ -42,6 +42,7 @@ junit = "4.13.2" #jUnit5 = "5.10.2" kotest = "5.9.1" kotlinResult = "1.1.21" +kotlinRetry = "2.0.1" kotlinx-collections = "0.3.8" #koin = "3.5.3" #ktor = "2.3.6" @@ -305,6 +306,8 @@ detekt-compose = { module = "io.nlopez.compose.rules:detekt", version = "0.4.22" kotlinResult-result = { module = "com.michael-bull.kotlin-result:kotlin-result", version.ref = "kotlinResult" } kotlinResult-coroutines = { module = "com.michael-bull.kotlin-result:kotlin-result-coroutines", version.ref = "kotlinResult" } +kotlinRetry = { module = "com.michael-bull.kotlin-retry:kotlin-retry", version.ref = "kotlinRetry" } +#kotlinRetryResult = { module = "com.michael-bull.kotlin-retry:kotlin-retry-result", version.ref = "kotlinRetry" } #view-collapsingToolbar = "me.onebone:toolbar-compose:2.3.5" coil-base = "io.coil-kt:coil-base:2.7.0" diff --git a/movies/movies-data/src/main/java/com/zsoltbertalan/flickslate/movies/data/repository/MoviesAccessor.kt b/movies/movies-data/src/main/java/com/zsoltbertalan/flickslate/movies/data/repository/MoviesAccessor.kt index e6b5a8e..19ca4a9 100644 --- a/movies/movies-data/src/main/java/com/zsoltbertalan/flickslate/movies/data/repository/MoviesAccessor.kt +++ b/movies/movies-data/src/main/java/com/zsoltbertalan/flickslate/movies/data/repository/MoviesAccessor.kt @@ -7,6 +7,7 @@ import com.zsoltbertalan.flickslate.movies.data.network.MoviesService import com.zsoltbertalan.flickslate.movies.data.network.model.toMovieDetail import com.zsoltbertalan.flickslate.movies.domain.api.MoviesRepository import com.zsoltbertalan.flickslate.movies.domain.model.MovieDetail +import com.zsoltbertalan.flickslate.shared.data.getresult.backoffRetryPolicy import com.zsoltbertalan.flickslate.shared.data.getresult.fetchCacheThenRemote import com.zsoltbertalan.flickslate.shared.data.util.runCatchingApi import com.zsoltbertalan.flickslate.shared.model.Movie @@ -53,6 +54,7 @@ class MoviesAccessor @Inject constructor( ) popularMoviesDataSource.insertPopularMovies(moviesReply, page) }, + retryPolicy = backoffRetryPolicy, ) } @@ -70,6 +72,7 @@ class MoviesAccessor @Inject constructor( ) upcomingMoviesDataSource.insertUpcomingMovies(moviesReply, page) }, + retryPolicy = backoffRetryPolicy, ) } @@ -87,6 +90,7 @@ class MoviesAccessor @Inject constructor( ) nowPlayingMoviesDataSource.insertNowPlayingMovies(moviesReply, page) }, + retryPolicy = backoffRetryPolicy, ) } diff --git a/shared-data/build.gradle.kts b/shared-data/build.gradle.kts index c3f25fb..83d2169 100644 --- a/shared-data/build.gradle.kts +++ b/shared-data/build.gradle.kts @@ -39,6 +39,7 @@ dependencies { api(libs.google.dagger.core) api(libs.inject) implementation(libs.kotlinResult.result) + api(libs.kotlinRetry) api(libs.kotlinx.collections.immutable.jvm) api(libs.kotlinx.coroutines.core) api(libs.kotlinx.serialization.core) @@ -67,5 +68,8 @@ dependencies { detektPlugins(libs.detekt.compose) +} -} \ No newline at end of file +tasks.withType().configureEach { + compilerOptions.freeCompilerArgs.add("-opt-in=kotlin.contracts.ExperimentalContracts") +} diff --git a/shared-data/src/main/java/com/zsoltbertalan/flickslate/shared/data/getresult/fetchCacheThenNetwork.kt b/shared-data/src/main/java/com/zsoltbertalan/flickslate/shared/data/getresult/fetchCacheThenNetwork.kt deleted file mode 100644 index 296b1f1..0000000 --- a/shared-data/src/main/java/com/zsoltbertalan/flickslate/shared/data/getresult/fetchCacheThenNetwork.kt +++ /dev/null @@ -1,228 +0,0 @@ -package com.zsoltbertalan.flickslate.shared.data.getresult - -import com.github.michaelbull.result.Err -import com.github.michaelbull.result.Ok -import com.github.michaelbull.result.andThen -import com.github.michaelbull.result.map -import com.github.michaelbull.result.recoverIf -import com.zsoltbertalan.flickslate.shared.data.getresult.STRATEGY.CACHE_FIRST_NETWORK_LATER -import com.zsoltbertalan.flickslate.shared.data.getresult.STRATEGY.CACHE_FIRST_NETWORK_ONCE -import com.zsoltbertalan.flickslate.shared.data.getresult.STRATEGY.CACHE_FIRST_NETWORK_SECOND -import com.zsoltbertalan.flickslate.shared.data.util.runCatchingApi -import com.zsoltbertalan.flickslate.shared.data.util.runCatchingUnit -import com.zsoltbertalan.flickslate.shared.model.Failure -import com.zsoltbertalan.flickslate.shared.util.Outcome -import kotlinx.coroutines.flow.Flow -import kotlinx.coroutines.flow.emitAll -import kotlinx.coroutines.flow.first -import kotlinx.coroutines.flow.flow -import kotlinx.coroutines.flow.flowOf -import retrofit2.Response - -/** - * A generic Repository function to handle **cache first** strategy as described here: - * https://herrbert74.github.io/posts/caching-strategies-in-android/#network-first-aka-stale-if-error - * - * Recovers from not modified HTTP response if we add eTag through @Header("If-None-Match") to the request, and if - * there is no update, or if the cache contains any data. In these cases it recovers by emitting null. - * - * Based on Flower: - * https://github.com/hadiyarajesh/flower/blob/master/flower-core/src/commonMain/kotlin/com/hadiyarajesh/flower_core/DBBoundResource.kt - * - * Difference to Flower - * Some unused function parameters were removed. - * KotlinResult library is used instead of custom monad. Loading should be handled by UI. - * - * @param fetchFromLocal A function to retrieve [DOMAIN] data from local database. Must be nullable! - * - * @param shouldMakeNetworkRequest Whether or not to make network request - * - * @param makeNetworkRequest A function to make network request and retrieve data mapped to [DOMAIN]. - * [retrofit2.Response] can be used as part of the call. It is needed when we want to extract information from the - * header (like ETags) or the error body. - * - * @param saveResponseData A function to save network reply coming in [DOMAIN] format from [makeNetworkRequest] - * - * @param strategy How to deal with network requests and results. See [STRATEGY] - * - * @return Result<[DOMAIN]> type - */ -inline fun fetchCacheThenRemote( - crossinline fetchFromLocal: () -> Flow, - crossinline shouldMakeNetworkRequest: (DOMAIN?) -> Boolean = { true }, - crossinline makeNetworkRequest: suspend () -> Outcome, - noinline saveResponseData: suspend (DOMAIN) -> Unit = { }, - strategy: STRATEGY = CACHE_FIRST_NETWORK_SECOND, -) = flow> { - - val localData = fetchFromLocal().first() - localData?.let { emit(Ok(it)) } - - val networkOnlyOnceAndAlreadyCached = strategy == CACHE_FIRST_NETWORK_ONCE && localData != null - - if (shouldMakeNetworkRequest(localData) && networkOnlyOnceAndAlreadyCached.not()) { - - val newResult = makeNetworkRequest().andThen { domain -> - saveResponseData(domain) - Ok(domain) - }.recoverIf( - { failure -> - failure == Failure.NotModified || localData != null }, - { null } - ) - - if (shouldEmitNetworkResult(newResult, strategy, localData == null)) { - emitAll( - flowOf( - newResult.map { it!! } - ) - ) - } - } -} - -/** - * For [retrofit2.Response] version see [fetchCacheThenNetworkResponse]. Response is needed when we want to extract - * information from the header (like ETags) or the error body. - * - * Recovers from network errors if the cache contains any data by emitting null. - * - * Fetch the data from local database (if available), - * emit if it's available, - * perform a network request (if instructed), - * and if not null, emit the response after saving it to local database. - * @param fetchFromLocal - A function to retrieve [DOMAIN] data from local database. Must be nullable! - * @param shouldMakeNetworkRequest - Whether or not to make network request - * @param makeNetworkRequest - A function to make network request and retrieve [REMOTE] data - * @param saveResponseData - A function to save network reply coming in [REMOTE] format - * @param mapper - An extension function to convert [REMOTE] to [DOMAIN] format - * @param strategy - How to deal with network requests and results. See [STRATEGY] - * @return Result<[DOMAIN]> type - */ -inline fun fetchCacheThenNetwork( - crossinline fetchFromLocal: () -> Flow, - crossinline shouldMakeNetworkRequest: (DOMAIN?) -> Boolean = { true }, - crossinline makeNetworkRequest: suspend () -> REMOTE, - noinline saveResponseData: suspend (REMOTE) -> Unit = { }, - crossinline mapper: REMOTE.() -> DOMAIN, - strategy: STRATEGY = CACHE_FIRST_NETWORK_SECOND, -) = flow> { - - val localData = fetchFromLocal().first() - localData?.let { emit(Ok(it)) } - - val networkOnlyOnceAndAlreadyCached = strategy == CACHE_FIRST_NETWORK_ONCE && localData != null - - if (shouldMakeNetworkRequest(localData) && networkOnlyOnceAndAlreadyCached.not()) { - - val result = runCatchingApi { - makeNetworkRequest() - }.andThen { dto -> - saveResponseData(dto) - Ok(dto) - }.map { - it.mapper() - }.recoverIf( - { _ -> localData != null }, - { null } - ) - - if (shouldEmitNetworkResult(result, strategy, localData == null)) { - emitAll( - flowOf( - result.map { it!! } - ) - ) - } - } - -} - -/** - * [retrofit2.Response] version of [fetchCacheThenNetwork]. Response is needed when we want to extract information - * from the header (like ETags) or the error body. Otherwise use the simpler version above. - * - * Recovers from not modified HTTP response if we add eTag through @Header("If-None-Match") to the request, and it - * there is no update, or if the cache contains any data. In these cases it recovers by emitting null. - * - * Fetch the data from local database (if available), - * emit if it's available, - * perform a network request (if instructed), - * and if not null, emit the response after saving it to local database. - * @param fetchFromLocal - A function to retrieve [DOMAIN] data from local database. Must be nullable! - * @param shouldMakeNetworkRequest - Whether or not to make network request - * @param makeNetworkRequest - A function to make network request and retrieve [REMOTE] data wrapped in [retrofit2.Response] - * @param saveResponseData - A function to save network reply coming in [REMOTE] format wrapped in [retrofit2.Response] - * @param mapper - An extension function to convert [REMOTE] to [DOMAIN] format - * @param strategy - How to deal with network requests and results. See [STRATEGY] - * @return Result<[DOMAIN]> type - */ -inline fun fetchCacheThenNetworkResponse( - crossinline fetchFromLocal: () -> Flow, - crossinline shouldMakeNetworkRequest: (DOMAIN?) -> Boolean = { true }, - crossinline makeNetworkRequest: suspend () -> Response, - noinline saveResponseData: suspend (Response) -> Unit = { }, - crossinline mapper: REMOTE.() -> DOMAIN, - strategy: STRATEGY = CACHE_FIRST_NETWORK_SECOND, -) = flow> { - val localData = fetchFromLocal().first() - localData?.let { emit(Ok(it)) } - val networkOnlyOnceAndAlreadyCached = strategy == CACHE_FIRST_NETWORK_ONCE && localData != null - if (shouldMakeNetworkRequest(localData) && networkOnlyOnceAndAlreadyCached.not()) { - val result = runCatchingApi { - makeNetworkRequest() - }.andThen { response -> - if (response.isSuccessful) { - runCatchingUnit { saveResponseData(response) } - Ok(response.body()) - } else { - Err(response.handle()) - } - }.map { - it?.mapper() - }.recoverIf( - { failure -> failure == Failure.NotModified || localData != null }, - { null } - ) - - if (shouldEmitNetworkResult(result, strategy, localData == null)) { - emitAll( - flowOf( - result.map { it!! } - ) - ) - } - } - -} - -fun shouldEmitNetworkResult( - result: Outcome, - strategy: STRATEGY, - isLocalNull: Boolean -): Boolean { - return when (strategy) { - CACHE_FIRST_NETWORK_LATER -> isLocalNull - else -> result.isErr || (result.isOk && result.component1() != null) - } -} - -/** - * Cache first strategies to deal with network requests and results. - */ -enum class STRATEGY { - /** - * Always emit network result. - */ - CACHE_FIRST_NETWORK_SECOND, - - /** - * Save but do not emit network result, unless there is no cache result. - */ - CACHE_FIRST_NETWORK_LATER, - - /** - * Make network request only if there is no cache result. - */ - CACHE_FIRST_NETWORK_ONCE, -} diff --git a/shared-data/src/main/java/com/zsoltbertalan/flickslate/shared/data/getresult/fetchCacheThenRemote.kt b/shared-data/src/main/java/com/zsoltbertalan/flickslate/shared/data/getresult/fetchCacheThenRemote.kt new file mode 100644 index 0000000..fc7dca7 --- /dev/null +++ b/shared-data/src/main/java/com/zsoltbertalan/flickslate/shared/data/getresult/fetchCacheThenRemote.kt @@ -0,0 +1,133 @@ +package com.zsoltbertalan.flickslate.shared.data.getresult + +import com.github.michaelbull.result.Ok +import com.github.michaelbull.result.andThen +import com.github.michaelbull.result.map +import com.github.michaelbull.result.recoverIf +import com.github.michaelbull.retry.policy.RetryPolicy +import com.github.michaelbull.retry.policy.binaryExponentialBackoff +import com.github.michaelbull.retry.policy.constantDelay +import com.github.michaelbull.retry.policy.continueIf +import com.github.michaelbull.retry.policy.plus +import com.github.michaelbull.retry.policy.stopAtAttempts +import com.zsoltbertalan.flickslate.shared.data.getresult.STRATEGY.CACHE_FIRST_NETWORK_LATER +import com.zsoltbertalan.flickslate.shared.data.getresult.STRATEGY.CACHE_FIRST_NETWORK_ONCE +import com.zsoltbertalan.flickslate.shared.data.getresult.STRATEGY.CACHE_FIRST_NETWORK_SECOND +import com.zsoltbertalan.flickslate.shared.data.retry.retry +import com.zsoltbertalan.flickslate.shared.model.Failure +import com.zsoltbertalan.flickslate.shared.util.Outcome +import kotlinx.coroutines.flow.Flow +import kotlinx.coroutines.flow.emitAll +import kotlinx.coroutines.flow.first +import kotlinx.coroutines.flow.flow +import kotlinx.coroutines.flow.flowOf + +const val RETRY_DELAY = 5000L +const val RETRY_ATTEMPTS = 5 + +/** + * There are four retry policies, which can be combined through the plus operator. + * They are: Backoff, Predicate, Delay and Stop. I tried to add at least one each to the below examples. + * See https://github.com/michaelbull/kotlin-retry/blob/master/kotlin-retry/src/commonMain/kotlin/com/github/michaelbull/retry/policy/Backoff.kt + */ +val defaultNoRetryPolicy = continueIf { false } +val defaultRetryPolicy = constantDelay(RETRY_DELAY) + stopAtAttempts(RETRY_ATTEMPTS) +val backoffRetryPolicy = binaryExponentialBackoff(min = 10L, max = 5000L) + stopAtAttempts(RETRY_ATTEMPTS) + +/** + * A generic Repository function to handle **cache first** strategy as described here: + * https://herrbert74.github.io/posts/caching-strategies-in-android/#network-first-aka-stale-if-error + * + * Recovers from not modified HTTP response if we add eTag through @Header("If-None-Match") to the request, and if + * there is no update, or if the cache contains any data. In these cases it recovers by emitting null. + * + * Based on Flower: + * https://github.com/hadiyarajesh/flower/blob/master/flower-core/src/commonMain/kotlin/com/hadiyarajesh/flower_core/DBBoundResource.kt + * + * Difference to Flower + * Some unused function parameters were removed. + * KotlinResult library is used instead of custom monad. Loading should be handled by UI. + * + * @param fetchFromLocal A function to retrieve [DOMAIN] data from local database. Must be nullable! + * + * @param shouldMakeNetworkRequest Whether or not to make network request. + * + * @param makeNetworkRequest A function to make network request and retrieve data mapped to [DOMAIN]. + * [retrofit2.Response] can be used as part of the call. It is needed when we want to extract information from the + * header (like ETags) or the error body. + * + * @param saveResponseData A function to save network reply coming in [DOMAIN] format from [makeNetworkRequest]. + * + * @param strategy How to deal with network requests and results. See [STRATEGY]. + * + * @param retryPolicy How to do retries after a failed network request. Default is to do nothing. See [RetryPolicy]. + * + * @return Result<[DOMAIN]> type + */ +inline fun fetchCacheThenRemote( + crossinline fetchFromLocal: () -> Flow, + crossinline shouldMakeNetworkRequest: (DOMAIN?) -> Boolean = { true }, + crossinline makeNetworkRequest: suspend () -> Outcome, + noinline saveResponseData: suspend (DOMAIN) -> Unit = { }, + strategy: STRATEGY = CACHE_FIRST_NETWORK_SECOND, + retryPolicy: RetryPolicy = defaultNoRetryPolicy, +) = flow> { + + val localData = fetchFromLocal().first() + localData?.let { emit(Ok(it)) } + + val networkOnlyOnceAndAlreadyCached = strategy == CACHE_FIRST_NETWORK_ONCE && localData != null + + if (shouldMakeNetworkRequest(localData) && networkOnlyOnceAndAlreadyCached.not()) { + + val newResult = retry(retryPolicy) { makeNetworkRequest() }.andThen { domain -> + saveResponseData(domain) + Ok(domain) + }.recoverIf( + { failure -> + failure == Failure.NotModified || localData != null + }, + { null } + ) + + if (shouldEmitNetworkResult(newResult, strategy, localData == null)) { + emitAll( + flowOf( + newResult.map { it!! } + ) + ) + } + } +} + +fun shouldEmitNetworkResult( + result: Outcome, + strategy: STRATEGY, + isLocalNull: Boolean +): Boolean { + return when (strategy) { + CACHE_FIRST_NETWORK_LATER -> isLocalNull + else -> result.isErr || (result.isOk && result.component1() != null) + } +} + +/** + * Cache first strategies to deal with network requests and results. + */ +enum class STRATEGY { + + /** + * Always emit network result. + */ + CACHE_FIRST_NETWORK_SECOND, + + /** + * Save but do not emit network result, unless there is no cache result. + */ + CACHE_FIRST_NETWORK_LATER, + + /** + * Make network request only if there is no cache result. + */ + CACHE_FIRST_NETWORK_ONCE, +} diff --git a/shared-data/src/main/java/com/zsoltbertalan/flickslate/shared/data/getresult/fetchNetworkFirst.kt b/shared-data/src/main/java/com/zsoltbertalan/flickslate/shared/data/getresult/fetchNetworkFirst.kt index b2a77a2..5ff43bf 100644 --- a/shared-data/src/main/java/com/zsoltbertalan/flickslate/shared/data/getresult/fetchNetworkFirst.kt +++ b/shared-data/src/main/java/com/zsoltbertalan/flickslate/shared/data/getresult/fetchNetworkFirst.kt @@ -4,6 +4,8 @@ import com.github.michaelbull.result.Ok import com.github.michaelbull.result.andThen import com.github.michaelbull.result.map import com.github.michaelbull.result.recoverIf +import com.github.michaelbull.retry.policy.RetryPolicy +import com.zsoltbertalan.flickslate.shared.data.retry.retry import com.zsoltbertalan.flickslate.shared.data.util.runCatchingUnit import com.zsoltbertalan.flickslate.shared.model.Failure import com.zsoltbertalan.flickslate.shared.util.Outcome @@ -41,11 +43,12 @@ inline fun fetchRemoteFirst( crossinline fetchFromLocal: () -> Flow, crossinline makeNetworkRequest: suspend () -> Outcome, noinline saveResponseData: suspend (DOMAIN) -> Unit = { }, + retryPolicy: RetryPolicy = defaultNoRetryPolicy, ) = flow> { var localData: DOMAIN? = null - val result = makeNetworkRequest().andThen { domain -> + val result = retry(retryPolicy) { makeNetworkRequest() }.andThen { domain -> runCatchingUnit { saveResponseData(domain) } Ok(domain) }.recoverIf( diff --git a/shared-data/src/main/java/com/zsoltbertalan/flickslate/shared/data/retry/Retry.kt b/shared-data/src/main/java/com/zsoltbertalan/flickslate/shared/data/retry/Retry.kt new file mode 100644 index 0000000..871cd6a --- /dev/null +++ b/shared-data/src/main/java/com/zsoltbertalan/flickslate/shared/data/retry/Retry.kt @@ -0,0 +1,52 @@ +package com.zsoltbertalan.flickslate.shared.data.retry + +import com.github.michaelbull.result.Err +import com.github.michaelbull.result.Result +import com.github.michaelbull.result.asErr +import com.github.michaelbull.retry.attempt.Attempt +import com.github.michaelbull.retry.attempt.firstAttempt +import com.github.michaelbull.retry.instruction.ContinueRetrying +import com.github.michaelbull.retry.instruction.RetryInstruction +import com.github.michaelbull.retry.instruction.StopRetrying +import com.github.michaelbull.retry.policy.RetryPolicy +import kotlinx.coroutines.delay +import kotlin.contracts.InvocationKind +import kotlin.contracts.contract + +/** + * Calls the specified function [block] and returns its [Result], handling any [Err] returned from the [block] function + * execution retrying the invocation according to [instructions][RetryInstruction] from the [policy]. + */ +suspend inline fun retry(policy: RetryPolicy, block: () -> Result): Result { + contract { + callsInPlace(block, InvocationKind.AT_LEAST_ONCE) + } + + var attempt: Attempt? = null + + while (true) { + val result = block() + + if (result.isOk) { + return result + } else { + if (attempt == null) { + attempt = firstAttempt() + } + + val failedAttempt = attempt.failedWith(result.error) + + when (val instruction = policy(failedAttempt)) { + StopRetrying -> return result.asErr() + + ContinueRetrying -> attempt.retryImmediately() + + else -> { + val (delayMillis) = instruction + delay(delayMillis) + attempt.retryAfter(delayMillis) + } + } + } + } +} diff --git a/shared-data/src/test/java/com/zsoltbertalan/flickslate/shared/data/getresult/FetchCacheThenRemoteLaterTest.kt b/shared-data/src/test/java/com/zsoltbertalan/flickslate/shared/data/getresult/FetchCacheThenRemoteLaterTest.kt index 5887ce8..1459f73 100644 --- a/shared-data/src/test/java/com/zsoltbertalan/flickslate/shared/data/getresult/FetchCacheThenRemoteLaterTest.kt +++ b/shared-data/src/test/java/com/zsoltbertalan/flickslate/shared/data/getresult/FetchCacheThenRemoteLaterTest.kt @@ -15,12 +15,18 @@ import kotlinx.coroutines.flow.flowOf import kotlinx.coroutines.flow.toList import kotlinx.coroutines.launch import kotlinx.coroutines.test.runTest +import org.junit.After import org.junit.Test class FetchCacheThenRemoteLaterTest { val mockSaveResponseData: (List) -> Unit = mockk(relaxed = true) + @After + fun after() { + NetworkRequestMother.resetAttempts() + } + @Test fun `when cache has data and network has data then emit once`() = runTest { @@ -142,4 +148,43 @@ class FetchCacheThenRemoteLaterTest { } + @Test + fun `given default retry policy when cache has NO data and network has data after retry then emit once`() = + runTest { + + val fetchFromLocal = { flowOf(null) } + + val flow = fetchCacheThenRemote( + fetchFromLocal = fetchFromLocal, + makeNetworkRequest = NetworkRequestMother.makeNetworkRequestResultAfterRetry(), + saveResponseData = mockSaveResponseData, + strategy = STRATEGY.CACHE_FIRST_NETWORK_LATER, + retryPolicy = defaultRetryPolicy + ) + + flow.toList() shouldBe listOf( + Ok(GenreMother.createGenreList()) + ) + verify(exactly = 1) { mockSaveResponseData(any()) } + + } + + @Test + fun `given default no retry policy is when cache has NO data and network has data after retry then emit error`() = + runTest { + + val fetchFromLocal = { flowOf(null) } + + val flow = fetchCacheThenRemote( + fetchFromLocal = fetchFromLocal, + makeNetworkRequest = NetworkRequestMother.makeNetworkRequestResultAfterRetry(), + saveResponseData = mockSaveResponseData, + strategy = STRATEGY.CACHE_FIRST_NETWORK_LATER, + ) + + flow.first() shouldBe Err(Failure.ServerError("Invalid id: The pre-requisite id is invalid or not found.")) + verify(exactly = 0) { mockSaveResponseData(any()) } + + } + } diff --git a/shared-data/src/test/java/com/zsoltbertalan/flickslate/shared/data/getresult/FetchCacheThenRemoteOnceTest.kt b/shared-data/src/test/java/com/zsoltbertalan/flickslate/shared/data/getresult/FetchCacheThenRemoteOnceTest.kt index 0c4d7dc..61ba2e7 100644 --- a/shared-data/src/test/java/com/zsoltbertalan/flickslate/shared/data/getresult/FetchCacheThenRemoteOnceTest.kt +++ b/shared-data/src/test/java/com/zsoltbertalan/flickslate/shared/data/getresult/FetchCacheThenRemoteOnceTest.kt @@ -15,12 +15,18 @@ import kotlinx.coroutines.flow.flowOf import kotlinx.coroutines.flow.toList import kotlinx.coroutines.launch import kotlinx.coroutines.test.runTest +import org.junit.After import org.junit.Test class FetchCacheThenRemoteOnceTest { val mockSaveResponseData: (List) -> Unit = mockk(relaxed = true) + @After + fun after() { + NetworkRequestMother.resetAttempts() + } + @Test fun `when cache has data and network has data then emit once`() = runTest { @@ -143,4 +149,44 @@ class FetchCacheThenRemoteOnceTest { } + + @Test + fun `given default retry policy when cache has NO data and network has data after retry then emit once`() = + runTest { + + val fetchFromLocal = { flowOf(null) } + + val flow = fetchCacheThenRemote( + fetchFromLocal = fetchFromLocal, + makeNetworkRequest = NetworkRequestMother.makeNetworkRequestResultAfterRetry(), + saveResponseData = mockSaveResponseData, + strategy = STRATEGY.CACHE_FIRST_NETWORK_ONCE, + retryPolicy = defaultRetryPolicy + ) + + flow.toList() shouldBe listOf( + Ok(GenreMother.createGenreList()) + ) + verify(exactly = 1) { mockSaveResponseData(any()) } + + } + + @Test + fun `given default no retry policy when cache has NO data and network has data after retry then emit error`() = + runTest { + + val fetchFromLocal = { flowOf(null) } + + val flow = fetchCacheThenRemote( + fetchFromLocal = fetchFromLocal, + makeNetworkRequest = NetworkRequestMother.makeNetworkRequestResultAfterRetry(), + saveResponseData = mockSaveResponseData, + strategy = STRATEGY.CACHE_FIRST_NETWORK_ONCE, + ) + + flow.first() shouldBe Err(Failure.ServerError("Invalid id: The pre-requisite id is invalid or not found.")) + verify(exactly = 0) { mockSaveResponseData(any()) } + + } + } diff --git a/shared-data/src/test/java/com/zsoltbertalan/flickslate/shared/data/getresult/FetchCacheThenRemoteTest.kt b/shared-data/src/test/java/com/zsoltbertalan/flickslate/shared/data/getresult/FetchCacheThenRemoteTest.kt index 0040120..568e1b1 100644 --- a/shared-data/src/test/java/com/zsoltbertalan/flickslate/shared/data/getresult/FetchCacheThenRemoteTest.kt +++ b/shared-data/src/test/java/com/zsoltbertalan/flickslate/shared/data/getresult/FetchCacheThenRemoteTest.kt @@ -15,12 +15,18 @@ import kotlinx.coroutines.flow.flowOf import kotlinx.coroutines.flow.toList import kotlinx.coroutines.launch import kotlinx.coroutines.test.runTest +import org.junit.After import org.junit.Test class FetchCacheThenRemoteTest { val mockSaveResponseData: (List) -> Unit = mockk(relaxed = true) + @After + fun after() { + NetworkRequestMother.resetAttempts() + } + @Test fun `when cache has data and network has data then emit twice`() = runTest { @@ -137,4 +143,42 @@ class FetchCacheThenRemoteTest { } + + @Test + fun `given default retry policy when cache has NO data and network has data after retry then emit once`() = + runTest { + + val fetchFromLocal = { flowOf(null) } + + val flow = fetchCacheThenRemote( + fetchFromLocal = fetchFromLocal, + makeNetworkRequest = NetworkRequestMother.makeNetworkRequestResultAfterRetry(), + saveResponseData = mockSaveResponseData, + retryPolicy = defaultRetryPolicy + ) + + flow.toList() shouldBe listOf( + Ok(GenreMother.createGenreList()) + ) + verify(exactly = 1) { mockSaveResponseData(any()) } + + } + + @Test + fun `given default no retry policy is when cache has NO data and network has data after retry then emit error`() = + runTest { + + val fetchFromLocal = { flowOf(null) } + + val flow = fetchCacheThenRemote( + fetchFromLocal = fetchFromLocal, + makeNetworkRequest = NetworkRequestMother.makeNetworkRequestResultAfterRetry(), + saveResponseData = mockSaveResponseData, + ) + + flow.first() shouldBe Err(Failure.ServerError("Invalid id: The pre-requisite id is invalid or not found.")) + verify(exactly = 0) { mockSaveResponseData(any()) } + + } + } diff --git a/shared-data/src/test/java/com/zsoltbertalan/flickslate/shared/data/getresult/FetchRemoteFirstTest.kt b/shared-data/src/test/java/com/zsoltbertalan/flickslate/shared/data/getresult/FetchRemoteFirstTest.kt index a4b8524..d1c6285 100644 --- a/shared-data/src/test/java/com/zsoltbertalan/flickslate/shared/data/getresult/FetchRemoteFirstTest.kt +++ b/shared-data/src/test/java/com/zsoltbertalan/flickslate/shared/data/getresult/FetchRemoteFirstTest.kt @@ -9,16 +9,26 @@ import com.zsoltbertalan.flickslate.shared.model.PageData import com.zsoltbertalan.flickslate.shared.model.PagingReply import com.zsoltbertalan.flickslate.shared.util.Outcome import io.kotest.matchers.shouldBe +import io.mockk.mockk +import io.mockk.verify import kotlinx.coroutines.delay import kotlinx.coroutines.flow.first import kotlinx.coroutines.flow.flowOf import kotlinx.coroutines.flow.toList import kotlinx.coroutines.launch import kotlinx.coroutines.test.runTest +import org.junit.After import org.junit.Test class FetchRemoteFirstTest { + val mockSaveResponseData: (List) -> Unit = mockk(relaxed = true) + + @After + fun after() { + NetworkRequestMother.resetAttempts() + } + @Test fun `when network has data and cache has data then emit once`() = runTest { @@ -120,4 +130,41 @@ class FetchRemoteFirstTest { } + @Test + fun `given default retry policy when cache has NO data and network has data after retry then emit once`() = + runTest { + + val fetchFromLocal = { flowOf(null) } + + val flow = fetchRemoteFirst( + fetchFromLocal = fetchFromLocal, + makeNetworkRequest = NetworkRequestMother.makeNetworkRequestResultAfterRetry(), + saveResponseData = mockSaveResponseData, + retryPolicy = defaultRetryPolicy + ) + + flow.toList() shouldBe listOf( + Ok(GenreMother.createGenreList()) + ) + verify(exactly = 1) { mockSaveResponseData(any()) } + + } + + @Test + fun `given default no retry policy is when cache has NO data and network has data after retry then emit error`() = + runTest { + + val fetchFromLocal = { flowOf(null) } + + val flow = fetchRemoteFirst( + fetchFromLocal = fetchFromLocal, + makeNetworkRequest = NetworkRequestMother.makeNetworkRequestResultAfterRetry(), + saveResponseData = mockSaveResponseData, + ) + + flow.first() shouldBe Err(Failure.ServerError("Invalid id: The pre-requisite id is invalid or not found.")) + verify(exactly = 0) { mockSaveResponseData(any()) } + + } + } diff --git a/shared-data/src/test/java/com/zsoltbertalan/flickslate/shared/data/getresult/NetworkRequest.kt b/shared-data/src/test/java/com/zsoltbertalan/flickslate/shared/data/getresult/NetworkRequestMother.kt similarity index 71% rename from shared-data/src/test/java/com/zsoltbertalan/flickslate/shared/data/getresult/NetworkRequest.kt rename to shared-data/src/test/java/com/zsoltbertalan/flickslate/shared/data/getresult/NetworkRequestMother.kt index 0b4dee6..7d4bb43 100644 --- a/shared-data/src/test/java/com/zsoltbertalan/flickslate/shared/data/getresult/NetworkRequest.kt +++ b/shared-data/src/test/java/com/zsoltbertalan/flickslate/shared/data/getresult/NetworkRequestMother.kt @@ -21,3 +21,21 @@ fun makeNetworkRequestDelayedResponse(): suspend () -> Outcome> = su delay(1000) Ok(GenreMother.createGenreList()) } + +object NetworkRequestMother { + + private var attempt = 0 + + fun resetAttempts() { + attempt = 0 + } + + fun makeNetworkRequestResultAfterRetry(): suspend () -> Outcome> = suspend { + if (attempt == 0) { + attempt++ + Err(Failure.ServerError("Invalid id: The pre-requisite id is invalid or not found.")) + } else { + Ok(GenreMother.createGenreList()) + } + } +} diff --git a/shared/src/main/java/com/zsoltbertalan/flickslate/shared/compose/design/Color.kt b/shared/src/main/java/com/zsoltbertalan/flickslate/shared/compose/design/Color.kt index 8721ef5..988aa25 100644 --- a/shared/src/main/java/com/zsoltbertalan/flickslate/shared/compose/design/Color.kt +++ b/shared/src/main/java/com/zsoltbertalan/flickslate/shared/compose/design/Color.kt @@ -1,4 +1,4 @@ -@file:Suppress("unused") +@file:Suppress("unused", "MagicNumber") package com.zsoltbertalan.flickslate.shared.compose.design diff --git a/shared/src/main/java/com/zsoltbertalan/flickslate/shared/compose/util/recomposeHighlighter.kt b/shared/src/main/java/com/zsoltbertalan/flickslate/shared/compose/util/recomposeHighlighter.kt index 83879c0..e59928d 100644 --- a/shared/src/main/java/com/zsoltbertalan/flickslate/shared/compose/util/recomposeHighlighter.kt +++ b/shared/src/main/java/com/zsoltbertalan/flickslate/shared/compose/util/recomposeHighlighter.kt @@ -19,6 +19,9 @@ import androidx.compose.ui.unit.dp import kotlinx.coroutines.delay import kotlin.math.min +const val RECOMPOSE_TIMEOUT = 3000L +const val RECOMPOSE_HIGHLIGHT_DENOMINATOR = 100f + /** * A [Modifier] that draws a border around elements that are recomposing. The border increases in * size and interpolates from red to green as more recompositions occur before a timeout. @@ -43,7 +46,7 @@ private val recomposeModifier = // Start the timeout, and reset everytime there's a recomposition. (Using totalCompositions // as the key is really just to cause the timer to restart every composition). LaunchedEffect(totalCompositions[0]) { - delay(3000) + delay(RECOMPOSE_TIMEOUT) totalCompositionsAtLastTimeout.longValue = totalCompositions[0] } @@ -77,7 +80,7 @@ private val recomposeModifier = else -> lerp( Color.Yellow.copy(alpha = 0.8f), Color.Red.copy(alpha = 0.5f), - min(1f, (numCompositionsSinceTimeout - 1).toFloat() / 100f) + min(1f, (numCompositionsSinceTimeout - 1).toFloat() / RECOMPOSE_HIGHLIGHT_DENOMINATOR) ) to numCompositionsSinceTimeout.toInt().dp.toPx() } diff --git a/tv/tv-data/src/main/java/com/zsoltbertalan/flickslate/tv/data/repository/TvAccessor.kt b/tv/tv-data/src/main/java/com/zsoltbertalan/flickslate/tv/data/repository/TvAccessor.kt index b047d77..6db9d9f 100644 --- a/tv/tv-data/src/main/java/com/zsoltbertalan/flickslate/tv/data/repository/TvAccessor.kt +++ b/tv/tv-data/src/main/java/com/zsoltbertalan/flickslate/tv/data/repository/TvAccessor.kt @@ -1,5 +1,6 @@ package com.zsoltbertalan.flickslate.tv.data.repository +import com.zsoltbertalan.flickslate.shared.data.getresult.backoffRetryPolicy import com.zsoltbertalan.flickslate.shared.data.getresult.fetchCacheThenRemote import com.zsoltbertalan.flickslate.shared.data.util.runCatchingApi import com.zsoltbertalan.flickslate.shared.model.PagingReply @@ -37,6 +38,7 @@ class TvAccessor @Inject constructor( ) tvDataSource.insertTv(topRatedTvReply, page) }, + retryPolicy = backoffRetryPolicy, ) }