[Backport 2.5] convert empty httpEntity to {} to avoid DeliveryStatus…

… initialization exception (#660) * convert empty httpEntity to {} to avoid DeliveryStatus initialization exception (#648) * convert empty httpEntity to {} to avoid DeliveryStatus initialization exception Signed-off-by: Hailong Cui <[email protected]> * add integTest case for webhook empty response Signed-off-by: Hailong Cui <[email protected]> --------- Signed-off-by: Hailong Cui <[email protected]> (cherry picked from commit 94c225d) * Add missing import Signed-off-by: Hailong Cui <[email protected]> --------- Signed-off-by: Hailong Cui <[email protected]> Co-authored-by: Hailong Cui <[email protected]>
opensearch-project · Jul 28, 2023 · 387cb9b · 387cb9b
1 parent ecdb9a2
commit 387cb9b
Show file tree

Hide file tree

Showing 6 changed files with 107 additions and 4 deletions.
diff --git a/...ns/core/src/main/kotlin/org/opensearch/notifications/core/client/DestinationHttpClient.kt b/...ns/core/src/main/kotlin/org/opensearch/notifications/core/client/DestinationHttpClient.kt
@@ -141,7 +141,9 @@ class DestinationHttpClient {
     @Throws(IOException::class)
     fun getResponseString(response: CloseableHttpResponse): String {
         val entity: HttpEntity = response.entity ?: return "{}"
-        return EntityUtils.toString(entity)
+        val responseString = EntityUtils.toString(entity)
+        // DeliveryStatus need statusText must not be empty, convert empty response to {}
+        return if (responseString.isNullOrEmpty()) "{}" else responseString
     }
 
     @Throws(IOException::class)

diff --git a/...e/src/test/kotlin/org/opensearch/notifications/core/destinations/ChimeDestinationTests.kt b/...e/src/test/kotlin/org/opensearch/notifications/core/destinations/ChimeDestinationTests.kt
@@ -91,7 +91,7 @@ internal class ChimeDestinationTests {
     @Test
     fun `test chime message empty entity response`() {
         val mockHttpClient: CloseableHttpClient = EasyMock.createMock(CloseableHttpClient::class.java)
-        val expectedWebhookResponse = DestinationMessageResponse(RestStatus.OK.status, "")
+        val expectedWebhookResponse = DestinationMessageResponse(RestStatus.OK.status, "{}")
 
         val httpResponse: CloseableHttpResponse = EasyMock.createMock(CloseableHttpResponse::class.java)
         EasyMock.expect(mockHttpClient.execute(EasyMock.anyObject(HttpPost::class.java))).andReturn(httpResponse)

diff --git a/...st/kotlin/org/opensearch/notifications/core/destinations/CustomWebhookDestinationTests.kt b/...st/kotlin/org/opensearch/notifications/core/destinations/CustomWebhookDestinationTests.kt
@@ -6,6 +6,7 @@
 package org.opensearch.notifications.core.destinations
 
 import io.mockk.every
+import io.mockk.mockk
 import io.mockk.mockkStatic
 import org.apache.http.client.methods.CloseableHttpResponse
 import org.apache.http.client.methods.HttpPatch
@@ -108,7 +109,7 @@ internal class CustomWebhookDestinationTests {
     @MethodSource("methodToHttpRequestType")
     fun `test custom webhook message empty entity response`(method: String, expectedHttpClass: Class<HttpUriRequest>) {
         val mockHttpClient: CloseableHttpClient = EasyMock.createMock(CloseableHttpClient::class.java)
-        val expectedWebhookResponse = DestinationMessageResponse(RestStatus.OK.status, "")
+        val expectedWebhookResponse = DestinationMessageResponse(RestStatus.OK.status, "{}")
 
         val httpResponse: CloseableHttpResponse = EasyMock.createMock(CloseableHttpResponse::class.java)
         EasyMock.expect(mockHttpClient.execute(EasyMock.anyObject(HttpPost::class.java))).andReturn(httpResponse)
@@ -230,4 +231,17 @@ internal class CustomWebhookDestinationTests {
         val actualRequestBody = httpClient.buildRequestBody(destination, message)
         assertEquals(messageText, actualRequestBody)
     }
+
+    @Test
+    fun `test get response string`() {
+        val httpClient = DestinationHttpClient()
+        val response = mockk<CloseableHttpResponse>()
+        every { response.entity } returns null
+        var responseString = httpClient.getResponseString(response)
+        assertEquals(responseString, "{}")
+
+        every { response.entity } returns StringEntity("")
+        responseString = httpClient.getResponseString(response)
+        assertEquals(responseString, "{}")
+    }
 }
diff --git a/...e/src/test/kotlin/org/opensearch/notifications/core/destinations/SlackDestinationTests.kt b/...e/src/test/kotlin/org/opensearch/notifications/core/destinations/SlackDestinationTests.kt
@@ -94,7 +94,7 @@ internal class SlackDestinationTests {
     @Test
     fun `test Slack message empty entity response`() {
         val mockHttpClient: CloseableHttpClient = EasyMock.createMock(CloseableHttpClient::class.java)
-        val expectedWebhookResponse = DestinationMessageResponse(RestStatus.OK.status, "")
+        val expectedWebhookResponse = DestinationMessageResponse(RestStatus.OK.status, "{}")
 
         val httpResponse: CloseableHttpResponse = EasyMock.createMock(CloseableHttpResponse::class.java)
         EasyMock.expect(mockHttpClient.execute(EasyMock.anyObject(HttpPost::class.java))).andReturn(httpResponse)

diff --git a/notifications/notifications/build.gradle b/notifications/notifications/build.gradle
@@ -231,6 +231,12 @@ integTest {
             excludeTestsMatching "org.opensearch.integtest.bwc.*IT"
         }
     }
+
+    if (usingRemoteCluster) {
+        filter {
+            excludeTestsMatching "org.opensearch.integtest.send.SendTestMessageWithMockServerIT"
+        }
+    }
 }
 
 project.getTasks().getByName("bundlePlugin").dependsOn(findProject(":${rootProject.name}-core").tasks.getByPath(":${rootProject.name}-core:bundlePlugin"))

diff --git a/...ications/src/test/kotlin/org/opensearch/integtest/send/SendTestMessageWithMockServerIT.kt b/...ications/src/test/kotlin/org/opensearch/integtest/send/SendTestMessageWithMockServerIT.kt
@@ -0,0 +1,81 @@
+/*
+ * Copyright OpenSearch Contributors
+ * SPDX-License-Identifier: Apache-2.0
+ */
+
+package org.opensearch.integtest.send
+
+import com.google.gson.JsonArray
+import com.google.gson.JsonObject
+import com.sun.net.httpserver.HttpServer
+import org.junit.AfterClass
+import org.junit.Assert
+import org.junit.BeforeClass
+import org.opensearch.integtest.PluginRestTestCase
+import org.opensearch.notifications.NotificationPlugin.Companion.PLUGIN_BASE_URI
+import org.opensearch.rest.RestRequest
+import org.opensearch.rest.RestStatus
+import java.net.InetAddress
+import java.net.InetSocketAddress
+
+internal class SendTestMessageWithMockServerIT : PluginRestTestCase() {
+
+    fun `test webhook return with empty entity`() {
+        val url = "http://${server.address.hostString}:${server.address.port}/webhook"
+        logger.info("webhook url = {}", url)
+        // Create webhook notification config
+        val createRequestJsonString = """
+        {
+            "config":{
+                "name":"this is a sample config name",
+                "description":"this is a sample config description",
+                "config_type":"webhook",
+                "is_enabled":true,
+                "webhook":{
+                    "url":"$url",
+                    "header_params": {
+                       "Content-type": "text/plain"
+                    }
+                }
+            }
+        }
+        """.trimIndent()
+        val configId = createConfigWithRequestJsonString(createRequestJsonString)
+        Assert.assertNotNull(configId)
+        Thread.sleep(1000)
+
+        // send test message
+        val sendResponse = executeRequest(
+            RestRequest.Method.POST.name, "$PLUGIN_BASE_URI/feature/test/$configId", "", RestStatus.OK.status
+        )
+
+        logger.info(sendResponse)
+
+        // verify failure response is with message
+        val deliveryStatus = (sendResponse.get("status_list") as JsonArray).get(0).asJsonObject
+            .get("delivery_status") as JsonObject
+        Assert.assertEquals(deliveryStatus.get("status_code").asString, "200")
+        Assert.assertEquals(deliveryStatus.get("status_text").asString, "{}")
+    }
+
+    companion object {
+        private lateinit var server: HttpServer
+
+        @JvmStatic
+        @BeforeClass
+        fun setupWebhook() {
+            server = HttpServer.create(InetSocketAddress(InetAddress.getLoopbackAddress(), 0), 0)
+
+            server.createContext("/webhook") {
+                it.sendResponseHeaders(200, -1)
+            }
+            server.start()
+        }
+
+        @JvmStatic
+        @AfterClass
+        fun stopMockServer() {
+            server.stop(1)
+        }
+    }
+}