From 81f725667e949d0276abb4c2e3d2a5418bc088e3 Mon Sep 17 00:00:00 2001 From: Nik Clayton Date: Sat, 18 Mar 2023 10:25:41 +0100 Subject: [PATCH] Show better errors when loading notifications fails (#3448) * Show better errors with notification loading fails The errors are returned as a JSON object, parse it, and show the error message it contains. Handle the cases where there might be no error message, or the JSON may be malformed. Add tests. Fixes #3445 * Lint --- .../NotificationsPagingSource.kt | 20 ++- .../notifications/NotificationsRepository.kt | 6 +- .../com/keylesspalace/tusky/entity/Error.kt | 24 +++ .../NotificationsPagingSourceTest.kt | 145 ++++++++++++++++++ 4 files changed, 192 insertions(+), 3 deletions(-) create mode 100644 app/src/main/java/com/keylesspalace/tusky/entity/Error.kt create mode 100644 app/src/test/java/com/keylesspalace/tusky/components/notifications/NotificationsPagingSourceTest.kt diff --git a/app/src/main/java/com/keylesspalace/tusky/components/notifications/NotificationsPagingSource.kt b/app/src/main/java/com/keylesspalace/tusky/components/notifications/NotificationsPagingSource.kt index 90d24fed9..5f7eafb0e 100644 --- a/app/src/main/java/com/keylesspalace/tusky/components/notifications/NotificationsPagingSource.kt +++ b/app/src/main/java/com/keylesspalace/tusky/components/notifications/NotificationsPagingSource.kt @@ -20,6 +20,7 @@ package com.keylesspalace.tusky.components.notifications import android.util.Log import androidx.paging.PagingSource import androidx.paging.PagingState +import com.google.gson.Gson import com.keylesspalace.tusky.entity.Notification import com.keylesspalace.tusky.network.MastodonApi import com.keylesspalace.tusky.util.HttpHeaderLink @@ -35,6 +36,7 @@ data class Links(val next: String?, val prev: String?) /** [PagingSource] for Mastodon Notifications, identified by the Notification ID */ class NotificationsPagingSource @Inject constructor( private val mastodonApi: MastodonApi, + private val gson: Gson, private val notificationFilter: Set ) : PagingSource() { override suspend fun load(params: LoadParams): LoadResult { @@ -58,7 +60,23 @@ class NotificationsPagingSource @Inject constructor( } if (!response.isSuccessful) { - return LoadResult.Error(Throwable(response.errorBody()?.string())) + val code = response.code() + + val msg = response.errorBody()?.string()?.let { errorBody -> + if (errorBody.isBlank()) return@let "no reason given" + + val error = try { + gson.fromJson(errorBody, com.keylesspalace.tusky.entity.Error::class.java) + } catch (e: Exception) { + return@let "$errorBody ($e)" + } + + when (val desc = error.error_description) { + null -> error.error + else -> "${error.error}: $desc" + } + } ?: "no reason given" + return LoadResult.Error(Throwable("HTTP $code: $msg")) } val links = getPageLinks(response.headers()["link"]) diff --git a/app/src/main/java/com/keylesspalace/tusky/components/notifications/NotificationsRepository.kt b/app/src/main/java/com/keylesspalace/tusky/components/notifications/NotificationsRepository.kt index 25c8458ac..b933a9a3f 100644 --- a/app/src/main/java/com/keylesspalace/tusky/components/notifications/NotificationsRepository.kt +++ b/app/src/main/java/com/keylesspalace/tusky/components/notifications/NotificationsRepository.kt @@ -23,6 +23,7 @@ import androidx.paging.Pager import androidx.paging.PagingConfig import androidx.paging.PagingData import androidx.paging.PagingSource +import com.google.gson.Gson import com.keylesspalace.tusky.entity.Notification import com.keylesspalace.tusky.network.MastodonApi import kotlinx.coroutines.flow.Flow @@ -31,7 +32,8 @@ import retrofit2.Response import javax.inject.Inject class NotificationsRepository @Inject constructor( - private val mastodonApi: MastodonApi + private val mastodonApi: MastodonApi, + private val gson: Gson ) { private var factory: InvalidatingPagingSourceFactory? = null @@ -47,7 +49,7 @@ class NotificationsRepository @Inject constructor( Log.d(TAG, "getNotificationsStream(), filtering: $filter") factory = InvalidatingPagingSourceFactory { - NotificationsPagingSource(mastodonApi, filter) + NotificationsPagingSource(mastodonApi, gson, filter) } return Pager( diff --git a/app/src/main/java/com/keylesspalace/tusky/entity/Error.kt b/app/src/main/java/com/keylesspalace/tusky/entity/Error.kt new file mode 100644 index 000000000..f78cafacd --- /dev/null +++ b/app/src/main/java/com/keylesspalace/tusky/entity/Error.kt @@ -0,0 +1,24 @@ +/* + * Copyright 2023 Tusky Contributors + * + * This file is a part of Tusky. + * + * This program is free software; you can redistribute it and/or modify it under the terms of the + * GNU General Public License as published by the Free Software Foundation; either version 3 of the + * License, or (at your option) any later version. + * + * Tusky is distributed in the hope that it will be useful, but WITHOUT ANY WARRANTY; without even + * the implied warranty of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU General + * Public License for more details. + * + * You should have received a copy of the GNU General Public License along with Tusky; if not, + * see . + */ + +package com.keylesspalace.tusky.entity + +/** @see [Error](https://docs.joinmastodon.org/entities/Error/) */ +data class Error( + val error: String, + val error_description: String? +) diff --git a/app/src/test/java/com/keylesspalace/tusky/components/notifications/NotificationsPagingSourceTest.kt b/app/src/test/java/com/keylesspalace/tusky/components/notifications/NotificationsPagingSourceTest.kt new file mode 100644 index 000000000..450f702a7 --- /dev/null +++ b/app/src/test/java/com/keylesspalace/tusky/components/notifications/NotificationsPagingSourceTest.kt @@ -0,0 +1,145 @@ +/* + * Copyright 2023 Tusky Contributors + * + * This file is a part of Tusky. + * + * This program is free software; you can redistribute it and/or modify it under the terms of the + * GNU General Public License as published by the Free Software Foundation; either version 3 of the + * License, or (at your option) any later version. + * + * Tusky is distributed in the hope that it will be useful, but WITHOUT ANY WARRANTY; without even + * the implied warranty of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU General + * Public License for more details. + * + * You should have received a copy of the GNU General Public License along with Tusky; if not, + * see . + */ + +package com.keylesspalace.tusky.components.notifications + +import androidx.paging.PagingSource +import androidx.test.ext.junit.runners.AndroidJUnit4 +import com.google.gson.Gson +import com.keylesspalace.tusky.entity.Notification +import com.keylesspalace.tusky.network.MastodonApi +import kotlinx.coroutines.ExperimentalCoroutinesApi +import kotlinx.coroutines.test.runTest +import okhttp3.ResponseBody.Companion.toResponseBody +import org.junit.Assert.assertEquals +import org.junit.Assert.assertTrue +import org.junit.Test +import org.junit.runner.RunWith +import org.mockito.kotlin.any +import org.mockito.kotlin.anyOrNull +import org.mockito.kotlin.doReturn +import org.mockito.kotlin.mock +import org.robolectric.annotation.Config +import retrofit2.Response + +@Config(sdk = [28]) +@RunWith(AndroidJUnit4::class) +@OptIn(ExperimentalCoroutinesApi::class) +class NotificationsPagingSourceTest { + @Test + fun `load() returns error message on HTTP error`() = runTest { + // Given + val jsonError = "{error: 'This is an error'}".toResponseBody() + val mockApi: MastodonApi = mock { + onBlocking { notifications(anyOrNull(), anyOrNull(), anyOrNull(), anyOrNull()) } doReturn Response.error(429, jsonError) + onBlocking { notification(any()) } doReturn Response.error(429, jsonError) + } + + val filter = emptySet() + val gson = Gson() + val pagingSource = NotificationsPagingSource(mockApi, gson, filter) + val loadingParams = PagingSource.LoadParams.Refresh("0", 5, false) + + // When + val loadResult = pagingSource.load(loadingParams) + + // Then + assertTrue(loadResult is PagingSource.LoadResult.Error) + assertEquals( + "HTTP 429: This is an error", + (loadResult as PagingSource.LoadResult.Error).throwable.message + ) + } + + // As previous, but with `error_description` field as well. + @Test + fun `load() returns extended error message on HTTP error`() = runTest { + // Given + val jsonError = "{error: 'This is an error', error_description: 'Description of the error'}".toResponseBody() + val mockApi: MastodonApi = mock { + onBlocking { notifications(anyOrNull(), anyOrNull(), anyOrNull(), anyOrNull()) } doReturn Response.error(429, jsonError) + onBlocking { notification(any()) } doReturn Response.error(429, jsonError) + } + + val filter = emptySet() + val gson = Gson() + val pagingSource = NotificationsPagingSource(mockApi, gson, filter) + val loadingParams = PagingSource.LoadParams.Refresh("0", 5, false) + + // When + val loadResult = pagingSource.load(loadingParams) + + // Then + assertTrue(loadResult is PagingSource.LoadResult.Error) + assertEquals( + "HTTP 429: This is an error: Description of the error", + (loadResult as PagingSource.LoadResult.Error).throwable.message + ) + } + + // As previous, but no error JSON, so expect default response + @Test + fun `load() returns default error message on empty HTTP error`() = runTest { + // Given + val jsonError = "{}".toResponseBody() + val mockApi: MastodonApi = mock { + onBlocking { notifications(anyOrNull(), anyOrNull(), anyOrNull(), anyOrNull()) } doReturn Response.error(429, jsonError) + onBlocking { notification(any()) } doReturn Response.error(429, jsonError) + } + + val filter = emptySet() + val gson = Gson() + val pagingSource = NotificationsPagingSource(mockApi, gson, filter) + val loadingParams = PagingSource.LoadParams.Refresh("0", 5, false) + + // When + val loadResult = pagingSource.load(loadingParams) + + // Then + assertTrue(loadResult is PagingSource.LoadResult.Error) + assertEquals( + "HTTP 429: no reason given", + (loadResult as PagingSource.LoadResult.Error).throwable.message + ) + } + + // As previous, but malformed JSON, so expect response with enough information to troubleshoot + @Test + fun `load() returns useful error message on malformed HTTP error`() = runTest { + // Given + val jsonError = "{'malformedjson}".toResponseBody() + val mockApi: MastodonApi = mock { + onBlocking { notifications(anyOrNull(), anyOrNull(), anyOrNull(), anyOrNull()) } doReturn Response.error(429, jsonError) + onBlocking { notification(any()) } doReturn Response.error(429, jsonError) + } + + val filter = emptySet() + val gson = Gson() + val pagingSource = NotificationsPagingSource(mockApi, gson, filter) + val loadingParams = PagingSource.LoadParams.Refresh("0", 5, false) + + // When + val loadResult = pagingSource.load(loadingParams) + + // Then + assertTrue(loadResult is PagingSource.LoadResult.Error) + assertEquals( + "HTTP 429: {'malformedjson} (com.google.gson.JsonSyntaxException: com.google.gson.stream.MalformedJsonException: Unterminated string at line 1 column 17 path \$.)", + (loadResult as PagingSource.LoadResult.Error).throwable.message + ) + } +}