From cfd3bb8eeaa06aac3adf14570ad3ec322e63b18d Mon Sep 17 00:00:00 2001 From: Izaaz Yunus Date: Fri, 15 Mar 2024 17:08:42 -0700 Subject: [PATCH] fix: Only parse response body when a body is expected (#185) * fix: Only parse response body when a body is expected --- .../amplitude/core/utilities/HttpClient.kt | 3 +- .../com/amplitude/core/utilities/Response.kt | 27 +++++--- .../core/utilities/HttpClientTest.kt | 61 +++++++++++++++++++ 3 files changed, 82 insertions(+), 9 deletions(-) diff --git a/core/src/main/java/com/amplitude/core/utilities/HttpClient.kt b/core/src/main/java/com/amplitude/core/utilities/HttpClient.kt index 62e3a527..34dde549 100644 --- a/core/src/main/java/com/amplitude/core/utilities/HttpClient.kt +++ b/core/src/main/java/com/amplitude/core/utilities/HttpClient.kt @@ -3,7 +3,6 @@ package com.amplitude.core.utilities import com.amplitude.core.Configuration import com.amplitude.core.Constants import com.amplitude.core.ServerZone -import org.json.JSONObject import java.io.BufferedReader import java.io.Closeable import java.io.IOException @@ -38,7 +37,7 @@ internal class HttpClient( try { inputStream = getInputStream(this.connection) responseBody = inputStream.bufferedReader().use(BufferedReader::readText) - this.response = HttpResponse.createHttpResponse(responseCode, JSONObject(responseBody)) + this.response = HttpResponse.createHttpResponse(responseCode, responseBody) } catch (e: IOException) { this.response = HttpResponse.createHttpResponse(408, null) } finally { diff --git a/core/src/main/java/com/amplitude/core/utilities/Response.kt b/core/src/main/java/com/amplitude/core/utilities/Response.kt index 0600f9bc..fdad0f2a 100644 --- a/core/src/main/java/com/amplitude/core/utilities/Response.kt +++ b/core/src/main/java/com/amplitude/core/utilities/Response.kt @@ -2,30 +2,45 @@ package com.amplitude.core.utilities import com.amplitude.core.events.BaseEvent import org.json.JSONObject +import java.lang.Exception internal object HttpResponse { - fun createHttpResponse(code: Int, responseBody: JSONObject?): Response { + fun createHttpResponse(code: Int, responseBody: String?): Response { when (code) { HttpStatus.SUCCESS.code -> { return SuccessResponse() } HttpStatus.BAD_REQUEST.code -> { - return BadRequestResponse(responseBody!!) + return BadRequestResponse(JSONObject(responseBody)) } HttpStatus.PAYLOAD_TOO_LARGE.code -> { - return PayloadTooLargeResponse(responseBody!!) + return PayloadTooLargeResponse(JSONObject(responseBody)) } HttpStatus.TOO_MANY_REQUESTS.code -> { - return TooManyRequestsResponse(responseBody!!) + return TooManyRequestsResponse(JSONObject(responseBody)) } HttpStatus.TIMEOUT.code -> { return TimeoutResponse() } else -> { - return FailedResponse(responseBody!!) + return FailedResponse(parseResponseBodyOrGetDefault(responseBody)) } } } + + private fun parseResponseBodyOrGetDefault(responseBody: String?): JSONObject { + val defaultObject = JSONObject() + if (responseBody.isNullOrEmpty()) { + return defaultObject + } + + return try { + JSONObject(responseBody) + } catch (ignored: Exception) { + defaultObject.put("error", responseBody) + defaultObject + } + } } interface Response { @@ -39,7 +54,6 @@ class SuccessResponse() : Response { class BadRequestResponse(response: JSONObject) : Response { override val status: HttpStatus = HttpStatus.BAD_REQUEST val error: String = response.getStringWithDefault("error", "") - val missingField: String = response.getStringWithDefault("missing_field", "") var eventsWithInvalidFields: Set = setOf() var eventsWithMissingFields: Set = setOf() var silencedEvents: Set = setOf() @@ -87,7 +101,6 @@ class PayloadTooLargeResponse(response: JSONObject) : Response { class TooManyRequestsResponse(response: JSONObject) : Response { override val status: HttpStatus = HttpStatus.TOO_MANY_REQUESTS val error: String = response.getStringWithDefault("error", "") - val epsThreshold = response.getInt("eps_threshold") var exceededDailyQuotaUsers: Set = setOf() var exceededDailyQuotaDevices: Set = setOf() var throttledEvents: Set = setOf() diff --git a/core/src/test/kotlin/com/amplitude/core/utilities/HttpClientTest.kt b/core/src/test/kotlin/com/amplitude/core/utilities/HttpClientTest.kt index 8a4a4602..8de67ad2 100644 --- a/core/src/test/kotlin/com/amplitude/core/utilities/HttpClientTest.kt +++ b/core/src/test/kotlin/com/amplitude/core/utilities/HttpClientTest.kt @@ -131,6 +131,67 @@ class HttpClientTest { ) } + @Test + fun `test correct response when null or empty payload`() { + server.enqueue(MockResponse().setResponseCode(200)) + server.enqueue(MockResponse().setResponseCode(200).setBody("")) + + val config = + Configuration( + apiKey = apiKey, + serverUrl = server.url("/").toString(), + ) + val event = BaseEvent() + event.eventType = "test" + + val httpClient = spyk(HttpClient(config)) + val diagnostics = Diagnostics() + diagnostics.addErrorLog("error") + diagnostics.addMalformedEvent("malformed-event") + + val connection = httpClient.upload() + connection.outputStream?.let { + connection.setEvents(JSONUtil.eventsToString(listOf(event))) + connection.setDiagnostics(diagnostics) + // Upload the payloads. + connection.close() + } + + runRequest() + assertEquals(200, connection.response.status.code) + + runRequest() + assertEquals(200, connection.response.status.code) + } + + @Test + fun `test html error response is handled properly`() { + server.enqueue(MockResponse().setResponseCode(503).setBody("Error occurred")) + + val config = + Configuration( + apiKey = apiKey, + serverUrl = server.url("/").toString(), + ) + val event = BaseEvent() + event.eventType = "test" + + val httpClient = spyk(HttpClient(config)) + + val connection = httpClient.upload() + connection.outputStream?.let { + connection.setEvents(JSONUtil.eventsToString(listOf(event))) + // Upload the payloads. + connection.close() + } + + runRequest() + // Error code 503 is converted to a 500 in the http client + assertEquals(500, connection.response.status.code) + val responseBody = connection.response as FailedResponse + assertEquals("Error occurred", responseBody.error) + } + private fun runRequest(): RecordedRequest? { return try { server.takeRequest(5, TimeUnit.SECONDS)