From 34b7a3c8ee3baf8d86e38e4dfe86eb8ab8d88f66 Mon Sep 17 00:00:00 2001 From: Konrad Pozniak Date: Tue, 8 Mar 2022 21:39:59 +0100 Subject: [PATCH] Don't hide potential timeline bugs by catching all exceptions (#2372) * don't hide potential timeline bugs by catching all exceptions * fix NetworkTimelineRemoteMediatorTest * improve ifExpected function * fix code formatting --- .../components/timeline/util/TimelineUtils.kt | 17 +++++++++++++++++ .../viewmodel/CachedTimelineRemoteMediator.kt | 5 ++++- .../viewmodel/CachedTimelineViewModel.kt | 5 ++++- .../viewmodel/NetworkTimelineRemoteMediator.kt | 5 ++++- .../viewmodel/NetworkTimelineViewModel.kt | 7 ++++++- .../timeline/viewmodel/TimelineViewModel.kt | 16 +--------------- .../NetworkTimelineRemoteMediatorTest.kt | 6 +++--- 7 files changed, 39 insertions(+), 22 deletions(-) create mode 100644 app/src/main/java/com/keylesspalace/tusky/components/timeline/util/TimelineUtils.kt diff --git a/app/src/main/java/com/keylesspalace/tusky/components/timeline/util/TimelineUtils.kt b/app/src/main/java/com/keylesspalace/tusky/components/timeline/util/TimelineUtils.kt new file mode 100644 index 00000000..c8d95fd8 --- /dev/null +++ b/app/src/main/java/com/keylesspalace/tusky/components/timeline/util/TimelineUtils.kt @@ -0,0 +1,17 @@ +package com.keylesspalace.tusky.components.timeline.util + +import retrofit2.HttpException +import java.io.IOException + +fun Throwable.isExpected() = this is IOException || this is HttpException + +inline fun ifExpected( + t: Throwable, + cb: () -> T +): T { + if (t.isExpected()) { + return cb() + } else { + throw t + } +} diff --git a/app/src/main/java/com/keylesspalace/tusky/components/timeline/viewmodel/CachedTimelineRemoteMediator.kt b/app/src/main/java/com/keylesspalace/tusky/components/timeline/viewmodel/CachedTimelineRemoteMediator.kt index e9d81e59..1cd58f0a 100644 --- a/app/src/main/java/com/keylesspalace/tusky/components/timeline/viewmodel/CachedTimelineRemoteMediator.kt +++ b/app/src/main/java/com/keylesspalace/tusky/components/timeline/viewmodel/CachedTimelineRemoteMediator.kt @@ -23,6 +23,7 @@ import androidx.room.withTransaction import com.google.gson.Gson import com.keylesspalace.tusky.components.timeline.Placeholder import com.keylesspalace.tusky.components.timeline.toEntity +import com.keylesspalace.tusky.components.timeline.util.ifExpected import com.keylesspalace.tusky.db.AccountManager import com.keylesspalace.tusky.db.AppDatabase import com.keylesspalace.tusky.db.TimelineStatusEntity @@ -109,7 +110,9 @@ class CachedTimelineRemoteMediator( } return MediatorResult.Success(endOfPaginationReached = statuses.isEmpty()) } catch (e: Exception) { - return MediatorResult.Error(e) + return ifExpected(e) { + MediatorResult.Error(e) + } } } diff --git a/app/src/main/java/com/keylesspalace/tusky/components/timeline/viewmodel/CachedTimelineViewModel.kt b/app/src/main/java/com/keylesspalace/tusky/components/timeline/viewmodel/CachedTimelineViewModel.kt index 26691bf5..d71da9bb 100644 --- a/app/src/main/java/com/keylesspalace/tusky/components/timeline/viewmodel/CachedTimelineViewModel.kt +++ b/app/src/main/java/com/keylesspalace/tusky/components/timeline/viewmodel/CachedTimelineViewModel.kt @@ -34,6 +34,7 @@ import com.keylesspalace.tusky.appstore.ReblogEvent import com.keylesspalace.tusky.components.timeline.Placeholder import com.keylesspalace.tusky.components.timeline.toEntity import com.keylesspalace.tusky.components.timeline.toViewData +import com.keylesspalace.tusky.components.timeline.util.ifExpected import com.keylesspalace.tusky.db.AccountManager import com.keylesspalace.tusky.db.AppDatabase import com.keylesspalace.tusky.entity.Poll @@ -191,7 +192,9 @@ class CachedTimelineViewModel @Inject constructor( } } } catch (e: Exception) { - loadMoreFailed(placeholderId, e) + ifExpected(e) { + loadMoreFailed(placeholderId, e) + } } } } diff --git a/app/src/main/java/com/keylesspalace/tusky/components/timeline/viewmodel/NetworkTimelineRemoteMediator.kt b/app/src/main/java/com/keylesspalace/tusky/components/timeline/viewmodel/NetworkTimelineRemoteMediator.kt index 114faa92..7a4c779d 100644 --- a/app/src/main/java/com/keylesspalace/tusky/components/timeline/viewmodel/NetworkTimelineRemoteMediator.kt +++ b/app/src/main/java/com/keylesspalace/tusky/components/timeline/viewmodel/NetworkTimelineRemoteMediator.kt @@ -19,6 +19,7 @@ import androidx.paging.ExperimentalPagingApi import androidx.paging.LoadType import androidx.paging.PagingState import androidx.paging.RemoteMediator +import com.keylesspalace.tusky.components.timeline.util.ifExpected import com.keylesspalace.tusky.db.AccountManager import com.keylesspalace.tusky.util.HttpHeaderLink import com.keylesspalace.tusky.util.dec @@ -107,7 +108,9 @@ class NetworkTimelineRemoteMediator( viewModel.currentSource?.invalidate() return MediatorResult.Success(endOfPaginationReached = statuses.isEmpty()) } catch (e: Exception) { - return MediatorResult.Error(e) + return ifExpected(e) { + MediatorResult.Error(e) + } } } } diff --git a/app/src/main/java/com/keylesspalace/tusky/components/timeline/viewmodel/NetworkTimelineViewModel.kt b/app/src/main/java/com/keylesspalace/tusky/components/timeline/viewmodel/NetworkTimelineViewModel.kt index a750a366..bb226292 100644 --- a/app/src/main/java/com/keylesspalace/tusky/components/timeline/viewmodel/NetworkTimelineViewModel.kt +++ b/app/src/main/java/com/keylesspalace/tusky/components/timeline/viewmodel/NetworkTimelineViewModel.kt @@ -28,6 +28,7 @@ import com.keylesspalace.tusky.appstore.EventHub import com.keylesspalace.tusky.appstore.FavoriteEvent import com.keylesspalace.tusky.appstore.PinEvent import com.keylesspalace.tusky.appstore.ReblogEvent +import com.keylesspalace.tusky.components.timeline.util.ifExpected import com.keylesspalace.tusky.db.AccountManager import com.keylesspalace.tusky.entity.Poll import com.keylesspalace.tusky.entity.Status @@ -43,6 +44,7 @@ import kotlinx.coroutines.launch import kotlinx.coroutines.rx3.await import retrofit2.HttpException import retrofit2.Response +import java.io.IOException import javax.inject.Inject /** @@ -170,7 +172,9 @@ class NetworkTimelineViewModel @Inject constructor( currentSource?.invalidate() } catch (e: Exception) { - loadMoreFailed(placeholderId, e) + ifExpected(e) { + loadMoreFailed(placeholderId, e) + } } } } @@ -214,6 +218,7 @@ class NetworkTimelineViewModel @Inject constructor( currentSource?.invalidate() } + @Throws(IOException::class, HttpException::class) suspend fun fetchStatusesForKind( fromId: String?, uptoId: String?, diff --git a/app/src/main/java/com/keylesspalace/tusky/components/timeline/viewmodel/TimelineViewModel.kt b/app/src/main/java/com/keylesspalace/tusky/components/timeline/viewmodel/TimelineViewModel.kt index c7c95636..544d0818 100644 --- a/app/src/main/java/com/keylesspalace/tusky/components/timeline/viewmodel/TimelineViewModel.kt +++ b/app/src/main/java/com/keylesspalace/tusky/components/timeline/viewmodel/TimelineViewModel.kt @@ -33,6 +33,7 @@ import com.keylesspalace.tusky.appstore.PreferenceChangedEvent import com.keylesspalace.tusky.appstore.ReblogEvent import com.keylesspalace.tusky.appstore.StatusDeletedEvent import com.keylesspalace.tusky.appstore.UnfollowEvent +import com.keylesspalace.tusky.components.timeline.util.ifExpected import com.keylesspalace.tusky.db.AccountManager import com.keylesspalace.tusky.entity.Filter import com.keylesspalace.tusky.entity.Poll @@ -46,8 +47,6 @@ import kotlinx.coroutines.flow.Flow import kotlinx.coroutines.launch import kotlinx.coroutines.rx3.asFlow import kotlinx.coroutines.rx3.await -import retrofit2.HttpException -import java.io.IOException abstract class TimelineViewModel( private val timelineCases: TimelineCases, @@ -291,19 +290,6 @@ abstract class TimelineViewModel( } } - private fun isExpectedRequestException(t: Exception) = t is IOException || t is HttpException - - private inline fun ifExpected( - t: Exception, - cb: () -> Unit - ) { - if (isExpectedRequestException(t)) { - cb() - } else { - throw t - } - } - companion object { private const val TAG = "TimelineVM" internal const val LOAD_AT_ONCE = 30 diff --git a/app/src/test/java/com/keylesspalace/tusky/components/timeline/NetworkTimelineRemoteMediatorTest.kt b/app/src/test/java/com/keylesspalace/tusky/components/timeline/NetworkTimelineRemoteMediatorTest.kt index 601fc00a..456171ef 100644 --- a/app/src/test/java/com/keylesspalace/tusky/components/timeline/NetworkTimelineRemoteMediatorTest.kt +++ b/app/src/test/java/com/keylesspalace/tusky/components/timeline/NetworkTimelineRemoteMediatorTest.kt @@ -27,7 +27,7 @@ import org.junit.runner.RunWith import org.robolectric.annotation.Config import retrofit2.HttpException import retrofit2.Response -import java.lang.RuntimeException +import java.io.IOException @Config(sdk = [29]) @RunWith(AndroidJUnit4::class) @@ -66,7 +66,7 @@ class NetworkTimelineRemoteMediatorTest { val timelineViewModel: NetworkTimelineViewModel = mock { on { statusData } doReturn mutableListOf() - onBlocking { fetchStatusesForKind(anyOrNull(), anyOrNull(), anyOrNull()) } doThrow RuntimeException() + onBlocking { fetchStatusesForKind(anyOrNull(), anyOrNull(), anyOrNull()) } doThrow IOException() } val remoteMediator = NetworkTimelineRemoteMediator(accountManager, timelineViewModel) @@ -74,7 +74,7 @@ class NetworkTimelineRemoteMediatorTest { val result = runBlocking { remoteMediator.load(LoadType.REFRESH, state()) } assertTrue(result is RemoteMediator.MediatorResult.Error) - assertTrue((result as RemoteMediator.MediatorResult.Error).throwable is RuntimeException) + assertTrue((result as RemoteMediator.MediatorResult.Error).throwable is IOException) } @Test