fix: Only parse response body when a body is expected (#185)

* fix: Only parse response body when a body is expected
amplitude · Mar 16, 2024 · cfd3bb8 · cfd3bb8
1 parent bb3003c
commit cfd3bb8
Show file tree

Hide file tree

Showing 3 changed files with 82 additions and 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
@@ -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
@@ -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<Int> = setOf()
     var eventsWithMissingFields: Set<Int> = setOf()
     var silencedEvents: Set<Int> = 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<String> = setOf()
     var exceededDailyQuotaDevices: Set<String> = setOf()
     var throttledEvents: Set<Int> = 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
@@ -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("<html>Error occurred</html>"))
+
+        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("<html>Error occurred</html>", responseBody.error)
+    }
+
     private fun runRequest(): RecordedRequest? {
         return try {
             server.takeRequest(5, TimeUnit.SECONDS)