diff --git a/embrace-android-sdk/src/main/java/io/embrace/android/embracesdk/comms/api/ApiClient.kt b/embrace-android-sdk/src/main/java/io/embrace/android/embracesdk/comms/api/ApiClient.kt index 982d5862dc..8df73e8fb8 100644 --- a/embrace-android-sdk/src/main/java/io/embrace/android/embracesdk/comms/api/ApiClient.kt +++ b/embrace-android-sdk/src/main/java/io/embrace/android/embracesdk/comms/api/ApiClient.kt @@ -7,13 +7,13 @@ internal interface ApiClient { /** * Executes [ApiRequest] as a GET, returning the response as a [ApiResponse] */ - fun executeGet(request: ApiRequest): ApiResponse + fun executeGet(request: ApiRequest): ApiResponse /** * Executes [ApiRequest] as a POST with the given body defined by [payloadToCompress], returning the response as a [ApiResponse]. * The body will be gzip compressed. */ - fun executePost(request: ApiRequest, payloadToCompress: ByteArray): ApiResponse + fun executePost(request: ApiRequest, payloadToCompress: ByteArray): ApiResponse companion object { /** @@ -23,6 +23,8 @@ internal interface ApiClient { const val NO_HTTP_RESPONSE = -1 + const val TOO_MANY_REQUESTS = 429 + const val defaultTimeoutMs = 60 * 1000 } } diff --git a/embrace-android-sdk/src/main/java/io/embrace/android/embracesdk/comms/api/ApiClientImpl.kt b/embrace-android-sdk/src/main/java/io/embrace/android/embracesdk/comms/api/ApiClientImpl.kt index 36bf1b0bb9..4a3760c8b6 100644 --- a/embrace-android-sdk/src/main/java/io/embrace/android/embracesdk/comms/api/ApiClientImpl.kt +++ b/embrace-android-sdk/src/main/java/io/embrace/android/embracesdk/comms/api/ApiClientImpl.kt @@ -1,13 +1,16 @@ package io.embrace.android.embracesdk.comms.api import io.embrace.android.embracesdk.comms.api.ApiClient.Companion.NO_HTTP_RESPONSE +import io.embrace.android.embracesdk.comms.api.ApiClient.Companion.TOO_MANY_REQUESTS import io.embrace.android.embracesdk.comms.api.ApiClient.Companion.defaultTimeoutMs import io.embrace.android.embracesdk.logging.InternalEmbraceLogger import java.io.ByteArrayOutputStream import java.io.IOException import java.io.InputStream import java.io.InputStreamReader -import java.net.HttpURLConnection +import java.net.HttpURLConnection.HTTP_ENTITY_TOO_LARGE +import java.net.HttpURLConnection.HTTP_NOT_MODIFIED +import java.net.HttpURLConnection.HTTP_OK import java.util.zip.GZIPOutputStream /** @@ -24,16 +27,17 @@ internal class ApiClientImpl( private val logger: InternalEmbraceLogger ) : ApiClient { - override fun executeGet(request: ApiRequest): ApiResponse { + override fun executeGet(request: ApiRequest): ApiResponse { var connection: EmbraceConnection? = null - try { + return try { connection = request.toConnection() setTimeouts(connection) connection.connect() - return executeHttpRequest(connection) + val response = executeHttpRequest(connection) + response } catch (ex: Throwable) { - throw IllegalStateException(ex.localizedMessage ?: "", ex) + ApiResponse.Incomplete(IllegalStateException(ex.localizedMessage ?: "", ex)) } finally { runCatching { connection?.inputStream?.close() @@ -41,13 +45,13 @@ internal class ApiClientImpl( } } - override fun executePost(request: ApiRequest, payloadToCompress: ByteArray): ApiResponse = + override fun executePost(request: ApiRequest, payloadToCompress: ByteArray): ApiResponse = executeRawPost(request, gzip(payloadToCompress)) /** * Posts a payload according to the ApiRequest parameter. The payload will not be gzip compressed. */ - private fun executeRawPost(request: ApiRequest, payload: ByteArray?): ApiResponse { + private fun executeRawPost(request: ApiRequest, payload: ByteArray?): ApiResponse { logger.logDeveloper("ApiClient", request.httpMethod.toString() + " " + request.url) logger.logDeveloper("ApiClient", "Request details: $request") @@ -60,13 +64,10 @@ internal class ApiClientImpl( connection.outputStream?.write(payload) connection.connect() } - val response = executeHttpRequest(connection) - // pre-existing behavior. handle this better in future. - check(response.statusCode == HttpURLConnection.HTTP_OK) { "Failed to retrieve from Embrace server." } response } catch (ex: Throwable) { - throw IllegalStateException(ex.localizedMessage ?: "", ex) + ApiResponse.Incomplete(IllegalStateException(ex.localizedMessage ?: "", ex)) } finally { runCatching { connection?.inputStream?.close() @@ -83,17 +84,35 @@ internal class ApiClientImpl( * Executes a HTTP call using the specified connection, returning the response from the * server as a string. */ - private fun executeHttpRequest(connection: EmbraceConnection): ApiResponse { - try { + private fun executeHttpRequest(connection: EmbraceConnection): ApiResponse { + return try { val responseCode = readHttpResponseCode(connection) - val headers = readHttpResponseHeaders(connection) - return ApiResponse( - responseCode, - headers, - readResponseBodyAsString(connection.inputStream) - ) + val responseHeaders = readHttpResponseHeaders(connection) + + return when (responseCode) { + HTTP_OK -> { + val responseBody = readResponseBodyAsString(connection.inputStream) + ApiResponse.Success(responseBody, responseHeaders) + } + HTTP_NOT_MODIFIED -> { + ApiResponse.NotModified + } + HTTP_ENTITY_TOO_LARGE -> { + ApiResponse.PayloadTooLarge + } + TOO_MANY_REQUESTS -> { + val retryAfter = responseHeaders["Retry-After"]?.toLongOrNull() + ApiResponse.TooManyRequests(retryAfter) + } + NO_HTTP_RESPONSE -> { + ApiResponse.Incomplete(IllegalStateException("Connection failed or unexpected response code")) + } + else -> { + ApiResponse.Failure(responseCode, responseHeaders) + } + } } catch (exc: Throwable) { - throw IllegalStateException("Error occurred during HTTP request execution", exc) + ApiResponse.Incomplete(IllegalStateException("Error occurred during HTTP request execution", exc)) } } diff --git a/embrace-android-sdk/src/main/java/io/embrace/android/embracesdk/comms/api/ApiResponse.kt b/embrace-android-sdk/src/main/java/io/embrace/android/embracesdk/comms/api/ApiResponse.kt index 6bf734ce97..60bec72148 100644 --- a/embrace-android-sdk/src/main/java/io/embrace/android/embracesdk/comms/api/ApiResponse.kt +++ b/embrace-android-sdk/src/main/java/io/embrace/android/embracesdk/comms/api/ApiResponse.kt @@ -1,7 +1,36 @@ package io.embrace.android.embracesdk.comms.api -internal data class ApiResponse( - val statusCode: Int?, - val headers: Map, - val body: T? -) +/** + * ApiResponse is a sealed class that represents the result of an API call. + */ +internal sealed class ApiResponse { + /** + * Represents an API call that returned a 200 OK status code. + */ + data class Success(val body: String?, val headers: Map?) : ApiResponse() + + /** + * Represents an API call that returned a 304 Not Modified status code. + */ + object NotModified : ApiResponse() + + /** + * Represents an API call that returned a 413 Payload Too Large status code. + */ + object PayloadTooLarge : ApiResponse() + + /** + * Represents an API call that returned a 429 Too Many Requests status code. + */ + data class TooManyRequests(val retryAfter: Long?) : ApiResponse() + + /** + * Represents a failed API call. (status code 400-499 or 500-599 except 413 and 429) + */ + data class Failure(val code: Int, val headers: Map?) : ApiResponse() + + /** + * Represents an exception thrown while making the API call. + */ + data class Incomplete(val exception: Throwable) : ApiResponse() +} diff --git a/embrace-android-sdk/src/main/java/io/embrace/android/embracesdk/comms/api/EmbraceApiService.kt b/embrace-android-sdk/src/main/java/io/embrace/android/embracesdk/comms/api/EmbraceApiService.kt index 9aa6beaec0..afd48aebc4 100644 --- a/embrace-android-sdk/src/main/java/io/embrace/android/embracesdk/comms/api/EmbraceApiService.kt +++ b/embrace-android-sdk/src/main/java/io/embrace/android/embracesdk/comms/api/EmbraceApiService.kt @@ -15,7 +15,6 @@ import io.embrace.android.embracesdk.payload.BlobMessage import io.embrace.android.embracesdk.payload.EventMessage import io.embrace.android.embracesdk.payload.NetworkEvent import java.io.StringReader -import java.net.HttpURLConnection import java.util.concurrent.Future import java.util.concurrent.ScheduledExecutorService import java.util.concurrent.TimeUnit @@ -53,33 +52,39 @@ internal class EmbraceApiService( * @return a future containing the configuration. */ @Throws(IllegalStateException::class) + @Suppress("UseCheckOrError") override fun getConfig(): RemoteConfig? { var request = prepareConfigRequest(configUrl) val cachedResponse = cachedConfigProvider(configUrl, request) if (cachedResponse.isValid()) { // only bother if we have a useful response. request = request.copy(eTag = cachedResponse.eTag) } - val response = apiClient.executeGet(request) - return when (response.statusCode) { - HttpURLConnection.HTTP_OK -> { + return when (val response = apiClient.executeGet(request)) { + is ApiResponse.Success -> { logger.logInfo("Fetched new config successfully.") val jsonReader = JsonReader(StringReader(response.body)) serializer.loadObject(jsonReader, RemoteConfig::class.java) } - - HttpURLConnection.HTTP_NOT_MODIFIED -> { + is ApiResponse.NotModified -> { logger.logInfo("Confirmed config has not been modified.") cachedResponse.remoteConfig } - - ApiClient.NO_HTTP_RESPONSE -> { + is ApiResponse.TooManyRequests -> { + // TODO: We should retry after the retryAfter time or 3 seconds and apply exponential backoff. + logger.logWarning("Too many requests. ") + null + } + is ApiResponse.Failure -> { logger.logInfo("Failed to fetch config (no response).") null } - - else -> { - logger.logWarning("Unexpected status code when fetching config: ${response.statusCode}") + is ApiResponse.Incomplete -> { + logger.logWarning("Failed to fetch config.", response.exception) + throw response.exception + } + ApiResponse.PayloadTooLarge -> { + // Not expected to receive a 413 response for a GET request. null } } @@ -209,8 +214,32 @@ internal class EmbraceApiService( } } + @Suppress("UseCheckOrError") private fun executePost(request: ApiRequest, payload: ByteArray) { - apiClient.executePost(request, payload) + when (val response = apiClient.executePost(request, payload)) { + is ApiResponse.Success -> {} + is ApiResponse.TooManyRequests -> { + // Temporarily, we just throw an exception. In the future, + // we will use the retryAfter to schedule a retry. + val retryAfter = response.retryAfter ?: 3 + throw IllegalStateException("Too many requests. Will retry after $retryAfter seconds.") + } + is ApiResponse.PayloadTooLarge -> { + // We don't want to retry on PayloadTooLarge error, so we just log the error. + logger.logError("Payload too large. Dropping event.") + } + is ApiResponse.Failure -> { + // We don't want to retry on 4xx or 5xx errors, so we just log the error. + logger.logError("Failed to retrieve from Embrace server. Status code: ${response.code}") + } + is ApiResponse.Incomplete -> { + // Retry on incomplete response. + throw IllegalStateException("Failed to retrieve from Embrace server.") + } + is ApiResponse.NotModified -> { + // Not expected to receive a 304 response for a POST request. + } + } } companion object { diff --git a/embrace-android-sdk/src/test/java/io/embrace/android/embracesdk/comms/api/ApiClientImplTest.kt b/embrace-android-sdk/src/test/java/io/embrace/android/embracesdk/comms/api/ApiClientImplTest.kt index 803e7c01a3..664c540beb 100644 --- a/embrace-android-sdk/src/test/java/io/embrace/android/embracesdk/comms/api/ApiClientImplTest.kt +++ b/embrace-android-sdk/src/test/java/io/embrace/android/embracesdk/comms/api/ApiClientImplTest.kt @@ -12,12 +12,14 @@ import okhttp3.mockwebserver.MockWebServer import okhttp3.mockwebserver.RecordedRequest import org.junit.After import org.junit.Assert.assertEquals +import org.junit.Assert.assertTrue import org.junit.Before import org.junit.Test import java.net.SocketException import java.util.concurrent.Executors import java.util.concurrent.TimeUnit import java.util.zip.GZIPInputStream +import kotlin.IllegalStateException /** * Runs a [MockWebServer] and asserts against our network code to ensure that it @@ -42,65 +44,81 @@ internal class ApiClientImplTest { server.shutdown() } - @Test(expected = IllegalStateException::class) + @Test fun testUnreachableHost() { // attempt some unreachable port val request = ApiRequest(url = EmbraceUrl.create("http://localhost:1565")) - apiClient.executePost(request, "Hello world".toByteArray()) + val response = apiClient.executePost(request, "Hello world".toByteArray()) + check(response is ApiResponse.Incomplete) + assertTrue(response.exception is IllegalStateException) } @Test fun testGet200Response() { server.enqueue(response200) - val result = runGetRequest() + val response = runGetRequest() assertGetRequest(server.takeRequest()) - assertEquals(DEFAULT_RESPONSE_BODY, result.body) + check(response is ApiResponse.Success) + assertEquals(DEFAULT_RESPONSE_BODY, response.body) } @Test fun testPost200ResponseCompressed() { server.enqueue(response200) - val result = runPostRequest() - assertEquals(DEFAULT_RESPONSE_BODY, result.body) + val response = runPostRequest() + check(response is ApiResponse.Success) + assertEquals(DEFAULT_RESPONSE_BODY, response.body) val delivered = server.takeRequest() assertPostRequest(delivered) assertEquals(DEFAULT_REQUEST_BODY, delivered.readCompressedRequestBody()) } - @Test(expected = IllegalStateException::class) + @Test fun testGet400Response() { server.enqueue(response400) - runGetRequest() + val response = runGetRequest() + check(response is ApiResponse.Failure) + assertEquals(response.code, 400) } - @Test(expected = IllegalStateException::class) + @Test fun testPost400Response() { server.enqueue(response400) - runPostRequest() + val response = runPostRequest() + check(response is ApiResponse.Failure) + assertEquals(response.code, 400) } - @Test(expected = IllegalStateException::class) + @Test fun testGet500Response() { server.enqueue(response500) - runGetRequest() + val response = runGetRequest() + check(response is ApiResponse.Failure) + assertEquals(response.code, 500) } - @Test(expected = IllegalStateException::class) + @Test fun testPost500Response() { server.enqueue(response500) - runPostRequest() + val response = runPostRequest() + check(response is ApiResponse.Failure) + assertEquals(response.code, 500) } - @Test(expected = IllegalStateException::class) + @Test fun testGetConnectionThrows() { - apiClient.executeGet(createThrowingRequest()) + val response = apiClient.executeGet(createThrowingRequest()) + check(response is ApiResponse.Incomplete) + assertTrue(response.exception is java.lang.IllegalStateException) } - @Test(expected = IllegalStateException::class) + @Test fun testPostConnectionThrows() { - apiClient.executePost(createThrowingRequest(), DEFAULT_REQUEST_BODY.toByteArray()) + val response = apiClient.executePost(createThrowingRequest(), DEFAULT_REQUEST_BODY.toByteArray()) + check(response is ApiResponse.Incomplete) + assertTrue(response.exception is java.lang.IllegalStateException) } /** @@ -114,6 +132,7 @@ internal class ApiClientImplTest { val payload = createLargeSessionPayload() val result = runPostRequest(payload = payload.toByteArray()) + check(result is ApiResponse.Success) // assert on result parsed by ApiClient assertEquals(DEFAULT_RESPONSE_BODY, result.body) @@ -126,7 +145,7 @@ internal class ApiClientImplTest { /** * Simulates an I/O exception midway through a request. */ - @Test(expected = IllegalStateException::class) + @Test fun testIoExceptionMidRequest() { server.enqueue(MockResponse().throttleBody(1, 1000, TimeUnit.MILLISECONDS)) @@ -136,7 +155,9 @@ internal class ApiClientImplTest { }, 25, TimeUnit.MILLISECONDS) // fire off the api request - runPostRequest() + val response = runPostRequest() + check(response is ApiResponse.Incomplete) + assertTrue(response.exception is IllegalStateException) } @Test @@ -178,7 +199,7 @@ internal class ApiClientImplTest { ) } - private fun runGetRequest(): ApiResponse = + private fun runGetRequest(): ApiResponse = apiClient.executeGet( ApiRequest( url = EmbraceUrl.create(baseUrl), @@ -188,7 +209,7 @@ internal class ApiClientImplTest { private fun runPostRequest( payload: ByteArray = DEFAULT_REQUEST_BODY.toByteArray() - ): ApiResponse = + ): ApiResponse = apiClient.executePost( ApiRequest( url = EmbraceUrl.create(baseUrl), diff --git a/embrace-android-sdk/src/test/java/io/embrace/android/embracesdk/comms/api/EmbraceApiServiceTest.kt b/embrace-android-sdk/src/test/java/io/embrace/android/embracesdk/comms/api/EmbraceApiServiceTest.kt index c4de623c5d..99e12beac4 100644 --- a/embrace-android-sdk/src/test/java/io/embrace/android/embracesdk/comms/api/EmbraceApiServiceTest.kt +++ b/embrace-android-sdk/src/test/java/io/embrace/android/embracesdk/comms/api/EmbraceApiServiceTest.kt @@ -28,7 +28,6 @@ import org.junit.Assert.assertSame import org.junit.Assert.assertTrue import org.junit.Before import org.junit.Test -import java.net.SocketException import java.util.concurrent.ScheduledExecutorService internal class EmbraceApiServiceTest { @@ -64,8 +63,7 @@ internal class EmbraceApiServiceTest { @Test fun `test getConfig returns correct values in Response`() { fakeApiClient.queueResponse( - ApiResponse( - statusCode = 200, + ApiResponse.Success( headers = emptyMap(), body = defaultConfigResponseBody ) @@ -80,22 +78,19 @@ internal class EmbraceApiServiceTest { assertEquals(100, remoteConfig.threshold) } - @Test(expected = SocketException::class) - fun `getConfig rethrows an exception thrown by apiClient`() { - val killerResponse: ApiResponse = mockk(relaxed = true) - every { killerResponse.statusCode } answers { throw SocketException() } - fakeApiClient.queueResponse(killerResponse) + @Test(expected = IllegalStateException::class) + fun `getConfig throws an exception when receiving ApiResponse_Incomplete`() { + val incompleteResponse: ApiResponse.Incomplete = ApiResponse.Incomplete( + IllegalStateException("Connection failed") + ) + fakeApiClient.queueResponse(incompleteResponse) apiService.getConfig() } @Test fun `cached remote config returned when 304 received`() { fakeApiClient.queueResponse( - ApiResponse( - statusCode = 304, - headers = emptyMap(), - body = null, - ) + ApiResponse.NotModified ) assertEquals(cachedConfig.remoteConfig, apiService.getConfig()) } @@ -103,10 +98,9 @@ internal class EmbraceApiServiceTest { @Test fun `getConfig did not complete returns a null config`() { fakeApiClient.queueResponse( - ApiResponse( - statusCode = NO_HTTP_RESPONSE, - headers = emptyMap(), - body = null + ApiResponse.Failure( + code = NO_HTTP_RESPONSE, + headers = emptyMap() ) ) assertNull(apiService.getConfig()) @@ -115,10 +109,9 @@ internal class EmbraceApiServiceTest { @Test fun `getConfig results in unexpected response code returns a null config`() { fakeApiClient.queueResponse( - ApiResponse( - statusCode = 400, - headers = emptyMap(), - body = null + ApiResponse.Failure( + code = 400, + headers = emptyMap() ) ) assertNull(apiService.getConfig()) @@ -129,11 +122,7 @@ internal class EmbraceApiServiceTest { val cfg = RemoteConfig() cachedConfig = CachedConfig(cfg, "my_etag") fakeApiClient.queueResponse( - ApiResponse( - statusCode = 304, - headers = emptyMap(), - body = null - ) + ApiResponse.NotModified ) val remoteConfig = apiService.getConfig() assertSame(cfg, remoteConfig) @@ -291,8 +280,7 @@ internal class EmbraceApiServiceTest { private const val fakeDeviceId = "ajflkadsflkadslkfjds" private const val fakeAppVersionName = "6.1.0" private val defaultConfigResponseBody = ResourceReader.readResourceAsText("remote_config_response.json") - private val successfulPostResponse = ApiResponse( - statusCode = 200, + private val successfulPostResponse = ApiResponse.Success( headers = emptyMap(), body = "" ) diff --git a/embrace-android-sdk/src/test/java/io/embrace/android/embracesdk/fakes/FakeApiClient.kt b/embrace-android-sdk/src/test/java/io/embrace/android/embracesdk/fakes/FakeApiClient.kt index 4ddfab6cf7..8396a7778b 100644 --- a/embrace-android-sdk/src/test/java/io/embrace/android/embracesdk/fakes/FakeApiClient.kt +++ b/embrace-android-sdk/src/test/java/io/embrace/android/embracesdk/fakes/FakeApiClient.kt @@ -8,17 +8,17 @@ import java.util.Queue internal class FakeApiClient : ApiClient { val sentRequests: MutableList> = mutableListOf() - private val queuedResponses: Queue> = LinkedList() + private val queuedResponses: Queue = LinkedList() - override fun executeGet(request: ApiRequest): ApiResponse = getNext(request, null) + override fun executeGet(request: ApiRequest): ApiResponse = getNext(request, null) - override fun executePost(request: ApiRequest, payloadToCompress: ByteArray): ApiResponse = getNext(request, payloadToCompress) + override fun executePost(request: ApiRequest, payloadToCompress: ByteArray): ApiResponse = getNext(request, payloadToCompress) - fun queueResponse(response: ApiResponse) { + fun queueResponse(response: ApiResponse) { queuedResponses.add(response) } - private fun getNext(request: ApiRequest, bytes: ByteArray?): ApiResponse { + private fun getNext(request: ApiRequest, bytes: ByteArray?): ApiResponse { sentRequests.add(Pair(request, bytes)) return checkNotNull(queuedResponses.poll()) { "No response" } }