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
This commit is contained in:
parent
4d8289b245
commit
34b7a3c8ee
7 changed files with 39 additions and 22 deletions
|
@ -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 <T> ifExpected(
|
||||
t: Throwable,
|
||||
cb: () -> T
|
||||
): T {
|
||||
if (t.isExpected()) {
|
||||
return cb()
|
||||
} else {
|
||||
throw t
|
||||
}
|
||||
}
|
|
@ -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)
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
|
|
|
@ -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)
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
|
|
|
@ -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)
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
|
|
|
@ -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?,
|
||||
|
|
|
@ -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
|
||||
|
|
|
@ -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
|
||||
|
|
Loading…
Reference in a new issue