diff --git a/core/network/src/main/kotlin/app/pachli/core/network/retrofit/apiresult/ApiResult.kt b/core/network/src/main/kotlin/app/pachli/core/network/retrofit/apiresult/ApiResult.kt index 850f455cc..587a8c8ad 100644 --- a/core/network/src/main/kotlin/app/pachli/core/network/retrofit/apiresult/ApiResult.kt +++ b/core/network/src/main/kotlin/app/pachli/core/network/retrofit/apiresult/ApiResult.kt @@ -28,6 +28,7 @@ import com.squareup.moshi.JsonDataException import java.io.IOException import java.lang.reflect.Type import okhttp3.Headers +import okhttp3.Request import retrofit2.HttpException import retrofit2.Response @@ -53,125 +54,134 @@ data class ApiResponse( * A failed response from an API call. * * @param resourceId String resource ID of the error message. + * @param request The request that failed * @param throwable The [Throwable] that caused this error. The server * message (if it exists), or [Throwable.getLocalizedMessage] will be - * interpolated in to this string at `%1$s`. + * interpolated in to this string at `%1$s`. The request's method and + * URL is appended to make error reports more useful. */ sealed class ApiError( @StringRes override val resourceId: Int, - val throwable: Throwable, + open val request: Request, + open val throwable: Throwable, ) : PachliError { - override val formatArgs: Array? = ( - throwable.getServerErrorMessage() ?: throwable.localizedMessage?.trim() - )?.let { arrayOf(it) } + override val formatArgs: Array? by lazy { + ( + throwable.getServerErrorMessage() ?: throwable.localizedMessage?.trim() + )?.let { arrayOf("$it: ${request.method} ${request.url}") } ?: arrayOf("${request.method} ${request.url}") + } override val cause: PachliError? = null companion object { - fun from(throwable: Throwable): ApiError { + fun from(request: Request, throwable: Throwable): ApiError { return when (throwable) { is HttpException -> when (throwable.code()) { - in 400..499 -> ClientError.from(throwable) - in 500..599 -> ServerError.from(throwable) - else -> Unknown(throwable) + in 400..499 -> ClientError.from(request, throwable) + in 500..599 -> ServerError.from(request, throwable) + else -> Unknown(request, throwable) } - is JsonDataException -> JsonParseError(throwable) - is IOException -> IoError(throwable) - else -> Unknown(throwable) + is JsonDataException -> JsonParseError(request, throwable) + is IOException -> IoError(request, throwable) + else -> Unknown(request, throwable) } } } - data class Unknown(val exception: Throwable) : ApiError( + data class Unknown(override val request: Request, override val throwable: Throwable) : ApiError( R.string.error_generic_fmt, - exception, + request, + throwable, ) } sealed class HttpError( @StringRes override val resourceId: Int, + override val request: Request, open val exception: HttpException, -) : ApiError(resourceId, exception) +) : ApiError(resourceId, request, exception) /** 4xx errors */ sealed class ClientError( @StringRes override val resourceId: Int, + request: Request, exception: HttpException, -) : HttpError(resourceId, exception) { +) : HttpError(resourceId, request, exception) { companion object { - fun from(exception: HttpException): ClientError { + fun from(request: Request, exception: HttpException): ClientError { return when (exception.code()) { - 400 -> BadRequest(exception) - 401 -> Unauthorized(exception) - 404 -> NotFound(exception) - 410 -> Gone(exception) - 429 -> RateLimit(exception) - else -> UnknownClientError(exception) + 400 -> BadRequest(request, exception) + 401 -> Unauthorized(request, exception) + 404 -> NotFound(request, exception) + 410 -> Gone(request, exception) + 429 -> RateLimit(request, exception) + else -> UnknownClientError(request, exception) } } } /** 400 Bad request */ - data class BadRequest(override val exception: HttpException) : - ClientError(R.string.error_generic_fmt, exception) + data class BadRequest(override val request: Request, override val exception: HttpException) : + ClientError(R.string.error_generic_fmt, request, exception) /** 401 Unauthorized */ - data class Unauthorized(override val exception: HttpException) : - ClientError(R.string.error_generic_fmt, exception) + data class Unauthorized(override val request: Request, override val exception: HttpException) : + ClientError(R.string.error_generic_fmt, request, exception) /** 404 Not found */ - data class NotFound(override val exception: HttpException) : - ClientError(R.string.error_404_not_found_fmt, exception) + data class NotFound(override val request: Request, override val exception: HttpException) : + ClientError(R.string.error_404_not_found_fmt, request, exception) /** 410 Gone */ - data class Gone(override val exception: HttpException) : - ClientError(R.string.error_generic_fmt, exception) + data class Gone(override val request: Request, override val exception: HttpException) : + ClientError(R.string.error_generic_fmt, request, exception) /** 429 Rate limit */ - data class RateLimit(override val exception: HttpException) : - ClientError(R.string.error_429_rate_limit_fmt, exception) + data class RateLimit(override val request: Request, override val exception: HttpException) : + ClientError(R.string.error_429_rate_limit_fmt, request, exception) /** All other 4xx client errors */ - data class UnknownClientError(override val exception: HttpException) : - ClientError(R.string.error_generic_fmt, exception) + data class UnknownClientError(override val request: Request, override val exception: HttpException) : + ClientError(R.string.error_generic_fmt, request, exception) } /** 5xx errors */ sealed class ServerError( @StringRes override val resourceId: Int, + request: Request, exception: HttpException, -) : HttpError(resourceId, exception) { +) : HttpError(resourceId, request, exception) { companion object { - fun from(exception: HttpException): ServerError { + fun from(request: Request, exception: HttpException): ServerError { return when (exception.code()) { - 500 -> Internal(exception) - 501 -> NotImplemented(exception) - 502 -> BadGateway(exception) - 503 -> ServiceUnavailable(exception) - else -> UnknownServerError(exception) + 500 -> Internal(request, exception) + 501 -> NotImplemented(request, exception) + 502 -> BadGateway(request, exception) + 503 -> ServiceUnavailable(request, exception) + else -> UnknownServerError(request, exception) } } } /** 500 Internal error */ - data class Internal(override val exception: HttpException) : - ServerError(R.string.error_generic_fmt, exception) + data class Internal(override val request: Request, override val exception: HttpException) : + ServerError(R.string.error_generic_fmt, request, exception) /** 501 Not implemented */ - data class NotImplemented(override val exception: HttpException) : - ServerError(R.string.error_404_not_found_fmt, exception) + data class NotImplemented(override val request: Request, override val exception: HttpException) : + ServerError(R.string.error_404_not_found_fmt, request, exception) /** 502 Bad gateway */ - data class BadGateway(override val exception: HttpException) : - ServerError(R.string.error_generic_fmt, exception) + data class BadGateway(override val request: Request, override val exception: HttpException) : + ServerError(R.string.error_generic_fmt, request, exception) /** 503 Service unavailable */ - data class ServiceUnavailable(override val exception: HttpException) : - ServerError(R.string.error_generic_fmt, exception) + data class ServiceUnavailable(override val request: Request, override val exception: HttpException) : + ServerError(R.string.error_generic_fmt, request, exception) /** All other 5xx server errors */ - data class UnknownServerError(override val exception: HttpException) : - ServerError(R.string.error_generic_fmt, exception) + data class UnknownServerError(override val request: Request, override val exception: HttpException) : + ServerError(R.string.error_generic_fmt, request, exception) } /** @@ -179,41 +189,40 @@ sealed class ServerError( * response in [exception] may be a success, as the server may have sent a 2xx * without a content-type. */ -data class MissingContentType(val exception: HttpException) : - ApiError(R.string.error_missing_content_type_fmt, exception) +data class MissingContentType(override val request: Request, val exception: HttpException) : + ApiError(R.string.error_missing_content_type_fmt, request, exception) /** * The server sent a response with the wrong content type (not "application/json") * Note that the underlying response in [exception] may be a success, as the server * may have sent a 2xx with the wrong content-type. */ -data class WrongContentType(val contentType: String, val exception: HttpException) : - ApiError(R.string.error_wrong_content_type_fmt, exception) { +data class WrongContentType(override val request: Request, val contentType: String, val exception: HttpException) : + ApiError(R.string.error_wrong_content_type_fmt, request, exception) { override val formatArgs: Array get() = super.formatArgs?.let { arrayOf(contentType, *it) } ?: arrayOf(contentType) } -data class JsonParseError(val exception: JsonDataException) : - ApiError(R.string.error_json_data_fmt, exception) +data class JsonParseError(override val request: Request, val exception: JsonDataException) : + ApiError(R.string.error_json_data_fmt, request, exception) -data class IoError(val exception: IOException) : - ApiError(R.string.error_network_fmt, exception) +data class IoError(override val request: Request, val exception: IOException) : + ApiError(R.string.error_network_fmt, request, exception) /** * Creates an [ApiResult] from a [Response]. */ -fun Result.Companion.from(response: Response, successType: Type): ApiResult { +fun Result.Companion.from(request: Request, response: Response, successType: Type): ApiResult { response.headers()["content-type"]?.let { contentType -> if (!contentType.startsWith("application/json")) { - return Err(WrongContentType(contentType, HttpException(response))) + return Err(WrongContentType(request, contentType, HttpException(response))) } - } ?: return Err(MissingContentType(HttpException(response))) + } ?: return Err(MissingContentType(request, HttpException(response))) if (!response.isSuccessful) { - val err = ApiError.from(HttpException(response)) - return Err(err) + return Err(ApiError.from(request, HttpException(response))) } // Skip body processing for successful responses expecting Unit @@ -226,5 +235,5 @@ fun Result.Companion.from(response: Response, successType: Type): ApiResu return Ok(ApiResponse(response.headers(), body, response.code())) } - return Err(ApiError.from(HttpException(response))) + return Err(ApiError.from(request, HttpException(response))) } diff --git a/core/network/src/main/kotlin/app/pachli/core/network/retrofit/apiresult/ApiResultCall.kt b/core/network/src/main/kotlin/app/pachli/core/network/retrofit/apiresult/ApiResultCall.kt index 43a1272d7..617978048 100644 --- a/core/network/src/main/kotlin/app/pachli/core/network/retrofit/apiresult/ApiResultCall.kt +++ b/core/network/src/main/kotlin/app/pachli/core/network/retrofit/apiresult/ApiResultCall.kt @@ -34,12 +34,12 @@ internal class ApiResultCall( override fun onResponse(call: Call, response: Response) { callback.onResponse( this@ApiResultCall, - Response.success(ApiResult.from(response, successType)), + Response.success(ApiResult.from(call.request(), response, successType)), ) } override fun onFailure(call: Call, throwable: Throwable) { - val err: ApiResult = Err(ApiError.from(throwable)) + val err: ApiResult = Err(ApiError.from(call.request(), throwable)) callback.onResponse(this@ApiResultCall, Response.success(err)) } diff --git a/core/network/src/main/kotlin/app/pachli/core/network/retrofit/apiresult/SyncApiResultCallAdapter.kt b/core/network/src/main/kotlin/app/pachli/core/network/retrofit/apiresult/SyncApiResultCallAdapter.kt index 5de4b9a88..ce36a5702 100644 --- a/core/network/src/main/kotlin/app/pachli/core/network/retrofit/apiresult/SyncApiResultCallAdapter.kt +++ b/core/network/src/main/kotlin/app/pachli/core/network/retrofit/apiresult/SyncApiResultCallAdapter.kt @@ -36,9 +36,9 @@ internal class SyncApiResultCallAdapter( override fun adapt(call: Call): ApiResult { return try { - ApiResult.from(call.execute(), successType) + ApiResult.from(call.request(), call.execute(), successType) } catch (e: Exception) { - Err(ApiError.from(e)) + Err(ApiError.from(call.request(), e)) } } } diff --git a/core/network/src/test/kotlin/app/pachli/core/network/retrofit/apiresult/ApiResultCallAdapterFactoryTest.kt b/core/network/src/test/kotlin/app/pachli/core/network/retrofit/apiresult/ApiResultCallAdapterFactoryTest.kt index 67b7390f3..e0a5eb2be 100644 --- a/core/network/src/test/kotlin/app/pachli/core/network/retrofit/apiresult/ApiResultCallAdapterFactoryTest.kt +++ b/core/network/src/test/kotlin/app/pachli/core/network/retrofit/apiresult/ApiResultCallAdapterFactoryTest.kt @@ -10,7 +10,7 @@ import retrofit2.Call import retrofit2.Retrofit class ApiResultCallAdapterFactoryTest { - private val retrofit = Retrofit.Builder().baseUrl("http://example.com").build() + private val retrofit = Retrofit.Builder().baseUrl("https://example.com").build() @OptIn(ExperimentalStdlibApi::class) @Test diff --git a/core/network/src/test/kotlin/app/pachli/core/network/retrofit/apiresult/ApiResultCallTest.kt b/core/network/src/test/kotlin/app/pachli/core/network/retrofit/apiresult/ApiResultCallTest.kt index 24bc792ac..0ba8b36e1 100644 --- a/core/network/src/test/kotlin/app/pachli/core/network/retrofit/apiresult/ApiResultCallTest.kt +++ b/core/network/src/test/kotlin/app/pachli/core/network/retrofit/apiresult/ApiResultCallTest.kt @@ -23,6 +23,7 @@ import com.github.michaelbull.result.unwrapError import com.google.common.truth.Truth.assertThat import java.io.IOException import okhttp3.Headers +import okhttp3.Request import org.junit.Assert.assertThrows import org.junit.Test import retrofit2.Call @@ -73,7 +74,15 @@ class ApiResultCallTest { object : Callback> { override fun onResponse(call: Call>, response: Response>) { assertThat(response.isSuccessful).isTrue() - assertThat(response.body()).isEqualTo(ApiResult.from(okResponse, String::class.java)) + assertThat(response.body()).isEqualTo( + ApiResult.from( + Request.Builder() + .url("https://example.com/") + .build(), + okResponse, + String::class.java, + ), + ) } override fun onFailure(call: Call>, t: Throwable) { @@ -152,7 +161,7 @@ class ApiResultCallTest { val exception = (error as ClientError.NotFound).exception assertThat(exception).isInstanceOf(HttpException::class.java) assertThat(exception.code()).isEqualTo(404) - assertThat(error.formatArgs).isEqualTo(arrayOf("HTTP 404 Not Found")) + assertThat(error.formatArgs).isEqualTo(arrayOf("HTTP 404 Not Found: GET https://example.com/")) } override fun onFailure(call: Call>, t: Throwable) { @@ -180,7 +189,7 @@ class ApiResultCallTest { val exception = (error as ClientError.NotFound).exception assertThat(exception).isInstanceOf(HttpException::class.java) assertThat(exception.code()).isEqualTo(404) - assertThat(error.formatArgs).isEqualTo(arrayOf(errorMsg)) + assertThat(error.formatArgs).isEqualTo(arrayOf("$errorMsg: GET https://example.com/")) } override fun onFailure(call: Call>, t: Throwable) { @@ -209,7 +218,7 @@ class ApiResultCallTest { val exception = (error as ClientError.NotFound).exception assertThat(exception).isInstanceOf(HttpException::class.java) assertThat(exception.code()).isEqualTo(404) - assertThat(error.formatArgs).isEqualTo(arrayOf("$errorMsg: $descriptionMsg")) + assertThat(error.formatArgs).isEqualTo(arrayOf("$errorMsg: $descriptionMsg: GET https://example.com/")) } override fun onFailure(call: Call>, t: Throwable) { @@ -223,12 +232,16 @@ class ApiResultCallTest { @Test fun `should parse call with IOException as ApiResult-failure`() { - val error = Err(IoError(IOException())) + val exception = IOException() networkApiResultCall.enqueue( object : Callback> { override fun onResponse(call: Call>, response: Response>) { - assertThat(response.body()).isEqualTo(error) + val err = response.body() as? Err + assertThat(err).isInstanceOf(Err::class.java) + assertThat(err?.error?.request).isEqualTo(call.request()) + assertThat(err?.error?.throwable).isEqualTo(exception) + assertThat(err?.error?.formatArgs).isEqualTo(arrayOf("GET https://example.com/")) } override fun onFailure(call: Call>, t: Throwable) { @@ -237,6 +250,6 @@ class ApiResultCallTest { }, ) - backingCall.completeWithException(error.error.throwable) + backingCall.completeWithException(exception) } } diff --git a/core/network/src/test/kotlin/app/pachli/core/network/retrofit/apiresult/TestCall.kt b/core/network/src/test/kotlin/app/pachli/core/network/retrofit/apiresult/TestCall.kt index 60c91c61e..5672fe926 100644 --- a/core/network/src/test/kotlin/app/pachli/core/network/retrofit/apiresult/TestCall.kt +++ b/core/network/src/test/kotlin/app/pachli/core/network/retrofit/apiresult/TestCall.kt @@ -28,7 +28,7 @@ class TestCall : Call { private var executed = false private var canceled = false private var callback: Callback? = null - private var request = Request.Builder().url("http://example.com").build() + private var request = Request.Builder().url("https://example.com").build() fun completeWithException(t: Throwable) { synchronized(this) { diff --git a/core/testing/src/main/kotlin/app/pachli/core/testing/ApiResult.kt b/core/testing/src/main/kotlin/app/pachli/core/testing/ApiResult.kt index fd0797122..b6190a3eb 100644 --- a/core/testing/src/main/kotlin/app/pachli/core/testing/ApiResult.kt +++ b/core/testing/src/main/kotlin/app/pachli/core/testing/ApiResult.kt @@ -25,6 +25,7 @@ import com.github.michaelbull.result.Ok import okhttp3.Headers import okhttp3.Protocol import okhttp3.Request +import okhttp3.RequestBody.Companion.toRequestBody import okhttp3.ResponseBody.Companion.toResponseBody import retrofit2.HttpException import retrofit2.Response @@ -45,11 +46,30 @@ fun success(data: T, code: Int = 200, vararg headers: String): ApiResult * failure. * * @param code HTTP failure code. - * @param body Data to use as the HTTP response body. + * @param responseBody Data to use as the HTTP response body. * @param message (optional) String to use as the HTTP status message. + * @param method (optional) String to use as the request method + * @param url (optional) String to use as the request URL + * @param requestBody (optional) String to use as the request body */ -fun failure(code: Int = 404, body: String = "", message: String = code.httpStatusMessage()): ApiResult = - Err(ApiError.from(HttpException(jsonError(code, body, message)))) +fun failure( + code: Int = 404, + responseBody: String = "", + message: String = code.httpStatusMessage(), + method: String = "GET", + url: String = "https://example.com", + requestBody: String? = null, +): ApiResult { + return Err( + ApiError.from( + Request.Builder() + .method(method, requestBody?.toRequestBody()) + .url(url) + .build(), + HttpException(jsonError(code, responseBody, message)), + ), + ) +} /** * Equivalent to [Response.error] with the content-type set to `application/json`.