Fix exception handling in network requests

base/src/main/java/app/tivi/extensions/RetrofitExtensions.kt

@@ -25,102 +25,90 @@ import kotlinx.coroutines.delay
 import retrofit2.Call
 import retrofit2.HttpException
 import retrofit2.Response
+import retrofit2.awaitResponse
 import java.io.IOException
 
 inline fun <T> Response<T>.bodyOrThrow(): T {
     if (!isSuccessful) throw HttpException(this)
     return body()!!
 }
 
-inline fun <T> Response<T>.toException() = HttpException(this)
-
-suspend inline fun <T> Call<T>.executeWithRetry(
+suspend fun <T> withRetry(
     defaultDelay: Long = 100,
     maxAttempts: Int = 3,
-    shouldRetry: (Exception) -> Boolean = ::defaultShouldRetry
-): Response<T> {
+    shouldRetry: (Throwable) -> Boolean = ::defaultShouldRetry,
+    block: suspend () -> Result<T>
+): Result<T> {
     repeat(maxAttempts) { attempt ->
-        var nextDelay = attempt * attempt * defaultDelay
+        when (val response = block()) {
+            is Success -> return response
+            is ErrorResult -> {
+                // The response failed, so lets see if we should retry again
+                if (attempt == maxAttempts - 1 || !shouldRetry(response.throwable)) {
+                    throw response.throwable
+                }
 
-        try {
-            // Clone a new ready call if needed
-            val call = if (isExecuted) clone() else this
-            return call.execute()
-        } catch (e: Exception) {
-            // The response failed, so lets see if we should retry again
-            if (attempt == (maxAttempts - 1) || !shouldRetry(e)) {
-                throw e
-            }
+                var nextDelay = attempt * attempt * defaultDelay
 
-            if (e is HttpException) {
-                // If we have a HttpException, check whether we have a Retry-After
-                // header to decide how long to delay
-                val retryAfterHeader = e.response()?.headers()?.get("Retry-After")
-                if (retryAfterHeader != null && retryAfterHeader.isNotEmpty()) {
-                    // Got a Retry-After value, try and parse it to an long
-                    try {
-                        nextDelay = (retryAfterHeader.toLong() + 10).coerceAtLeast(defaultDelay)
-                    } catch (nfe: NumberFormatException) {
-                        // Probably won't happen, ignore the value and use the generated default above
+                if (response.throwable is HttpException) {
+                    // If we have a HttpException, check whether we have a Retry-After
+                    // header to decide how long to delay
+                    response.throwable.retryAfter?.let {
+                        nextDelay = it.coerceAtLeast(defaultDelay)
                     }
                 }
+
+                delay(nextDelay)
             }
         }
-
-        delay(nextDelay)
     }
 
     // We should never hit here
     throw IllegalStateException("Unknown exception from executeWithRetry")
 }
 
-suspend inline fun <T> Call<T>.fetchBodyWithRetry(
-    firstDelay: Long = 100,
-    maxAttempts: Int = 3,
-    shouldRetry: (Exception) -> Boolean = ::defaultShouldRetry
-) = executeWithRetry(firstDelay, maxAttempts, shouldRetry).bodyOrThrow()
+private val HttpException.retryAfter: Long?
+    get() {
+        val retryAfterHeader = response()?.headers()?.get("Retry-After")
+        if (retryAfterHeader != null && retryAfterHeader.isNotEmpty()) {
+            // Got a Retry-After value, try and parse it to an long
+            try {
+                return retryAfterHeader.toLong() + 10
+            } catch (nfe: NumberFormatException) {
+                // Probably won't happen, ignore the value and use the generated default above
+            }
+        }
+        return null
+    }
 
-inline fun defaultShouldRetry(exception: Exception) = when (exception) {
-    is HttpException -> exception.code() == 429
+private fun defaultShouldRetry(throwable: Throwable) = when (throwable) {
+    is HttpException -> throwable.code() == 429
     is IOException -> true
     else -> false
 }
 
-inline fun <T> Response<T>.isFromNetwork(): Boolean {
-    return raw().cacheResponse == null
-}
-
-inline fun <T> Response<T>.isFromCache(): Boolean {
-    return raw().cacheResponse != null
-}
-
-inline fun <T> Response<T>.toResultUnit(): Result<Unit> = try {
-    if (isSuccessful) {
-        Success(data = Unit, responseModified = isFromNetwork())
-    } else {
-        ErrorResult(toException())
-    }
-} catch (e: Exception) {
-    ErrorResult(e)
-}
+private val Response<*>.isFromNetwork: Boolean
+    get() = raw().cacheResponse == null
 
-inline fun <T> Response<T>.toResult(): Result<T> = try {
-    if (isSuccessful) {
-        Success(data = bodyOrThrow(), responseModified = isFromNetwork())
-    } else {
-        ErrorResult(toException())
-    }
-} catch (e: Exception) {
-    ErrorResult(e)
+suspend fun <T> Call<T>.awaitUnit(): Result<Unit> = try {
+    Success(data = Unit, responseModified = awaitResponse().isFromNetwork)
+} catch (t: Throwable) {
+    ErrorResult(t)
 }
 
-@Suppress("REDUNDANT_INLINE_SUSPEND_FUNCTION_TYPE")
-suspend fun <T, E> Response<T>.toResult(mapper: suspend (T) -> E): Result<E> = try {
-    if (isSuccessful) {
-        Success(data = mapper(bodyOrThrow()), responseModified = isFromNetwork())
-    } else {
-        ErrorResult(toException())
+suspend fun <T, E> Call<T>.awaitResult(
+    mapper: suspend (T) -> E,
+): Result<E> = try {
+    awaitResponse().let {
+        if (it.isSuccessful) {
+            Success(
+                data = mapper(it.bodyOrThrow()),
+                responseModified = it.isFromNetwork
+            )
+        } else {
+            ErrorResult(HttpException(it))
+        }
     }
-} catch (e: Exception) {
-    ErrorResult(e)
+} catch (t: Throwable) {
+    ErrorResult(t)
 }

data-android/src/test/java/app/tivi/data/repositories/FollowedShowRepositoryTest.kt

@@ -36,6 +36,8 @@ import app.tivi.utils.insertShow
 import app.tivi.utils.show
 import app.tivi.utils.show2
 import com.google.common.truth.Truth.assertThat
+import com.uwetrottmann.trakt5.entities.ListIds
+import com.uwetrottmann.trakt5.entities.TraktList
 import dagger.hilt.android.testing.HiltAndroidTest
 import dagger.hilt.android.testing.UninstallModules
 import io.mockk.coEvery
@@ -66,7 +68,11 @@ class FollowedShowRepositoryTest : DatabaseTest() {
 
     @Test
     fun testSync() = testScope.runBlockingTest {
-        coEvery { traktDataSource.getFollowedListId() } returns Success(0)
+        coEvery { traktDataSource.getFollowedListId() } returns Success(
+            TraktList().apply {
+                ids = ListIds().apply { trakt = 0 }
+            }
+        )
         coEvery { traktDataSource.getListShows(0) } returns Success(listOf(followedShow1Network to show))
 
         repository.syncFollowedShows()
@@ -79,7 +85,11 @@ class FollowedShowRepositoryTest : DatabaseTest() {
     fun testSync_emptyResponse() = testScope.runBlockingTest {
         insertFollowedShow(database)
 
-        coEvery { traktDataSource.getFollowedListId() } returns Success(0)
+        coEvery { traktDataSource.getFollowedListId() } returns Success(
+            TraktList().apply {
+                ids = ListIds().apply { trakt = 0 }
+            }
+        )
         coEvery { traktDataSource.getListShows(0) } returns Success(emptyList())
 
         repository.syncFollowedShows()
@@ -91,7 +101,11 @@ class FollowedShowRepositoryTest : DatabaseTest() {
     fun testSync_responseDifferentShow() = testScope.runBlockingTest {
         insertFollowedShow(database)
 
-        coEvery { traktDataSource.getFollowedListId() } returns Success(0)
+        coEvery { traktDataSource.getFollowedListId() } returns Success(
+            TraktList().apply {
+                ids = ListIds().apply { trakt = 0 }
+            }
+        )
         coEvery { traktDataSource.getListShows(0) } returns Success(listOf(followedShow2Network to show2))
 
         repository.syncFollowedShows()

data/src/main/java/app/tivi/data/mappers/Mapper.kt

@@ -16,10 +16,10 @@
 
 package app.tivi.data.mappers
 
-interface Mapper<F, T> {
+fun interface Mapper<F, T> {
     suspend fun map(from: F): T
 }
 
-interface IndexedMapper<F, T> {
+fun interface IndexedMapper<F, T> {
     suspend fun map(index: Int, from: F): T
 }

data/src/main/java/app/tivi/data/repositories/episodes/TmdbEpisodeDataSource.kt

@@ -20,8 +20,8 @@ import app.tivi.data.entities.Episode
 import app.tivi.data.entities.Result
 import app.tivi.data.mappers.ShowIdToTmdbIdMapper
 import app.tivi.data.mappers.TmdbEpisodeToEpisode
-import app.tivi.extensions.executeWithRetry
-import app.tivi.extensions.toResult
+import app.tivi.extensions.awaitResult
+import app.tivi.extensions.withRetry
 import com.uwetrottmann.tmdb2.Tmdb
 import javax.inject.Inject
 
@@ -34,10 +34,9 @@ class TmdbEpisodeDataSource @Inject constructor(
         showId: Long,
         seasonNumber: Int,
         episodeNumber: Int
-    ): Result<Episode> {
-        return tmdb.tvEpisodesService()
+    ): Result<Episode> = withRetry {
+        tmdb.tvEpisodesService()
             .episode(tmdbIdMapper.map(showId), seasonNumber, episodeNumber, null)
-            .executeWithRetry()
-            .toResult(episodeMapper::map)
+            .awaitResult { episodeMapper.map(it) }
     }
 }

data/src/main/java/app/tivi/data/repositories/episodes/TraktEpisodeDataSource.kt

@@ -21,8 +21,8 @@ import app.tivi.data.entities.ErrorResult
 import app.tivi.data.entities.Result
 import app.tivi.data.mappers.ShowIdToTraktIdMapper
 import app.tivi.data.mappers.TraktEpisodeToEpisode
-import app.tivi.extensions.executeWithRetry
-import app.tivi.extensions.toResult
+import app.tivi.extensions.awaitResult
+import app.tivi.extensions.withRetry
 import com.uwetrottmann.trakt5.enums.Extended
 import com.uwetrottmann.trakt5.services.Episodes
 import javax.inject.Inject
@@ -40,9 +40,10 @@ class TraktEpisodeDataSource @Inject constructor(
     ): Result<Episode> {
         val traktId = traktIdMapper.map(showId)
             ?: return ErrorResult(IllegalArgumentException("No Trakt ID for show with ID: $showId"))
-
-        return service.get().summary(traktId.toString(), seasonNumber, episodeNumber, Extended.FULL)
-            .executeWithRetry()
-            .toResult(episodeMapper::map)
+        return withRetry {
+            service.get()
+                .summary(traktId.toString(), seasonNumber, episodeNumber, Extended.FULL)
+                .awaitResult { episodeMapper.map(it) }
+        }
     }
 }