Use comma to divide headers (Fix #1765).

ktorio · Apr 17, 2020 · a9c1c7b · a9c1c7b
1 parent 3c54c68
commit a9c1c7b
Show file tree

Hide file tree

Showing 6 changed files with 92 additions and 3 deletions.
diff --git a/ktor-client/ktor-client-core/common/src/io/ktor/client/engine/Utils.kt b/ktor-client/ktor-client-core/common/src/io/ktor/client/engine/Utils.kt
@@ -33,7 +33,8 @@ fun mergeHeaders(
         if (HttpHeaders.ContentLength == key) return@forEach // set later
         if (HttpHeaders.ContentType == key) return@forEach // set later
 
-        block(key, values.joinToString(";"))
+        // https://www.w3.org/Protocols/rfc2616/rfc2616-sec4.html#sec4.2
+        block(key, values.joinToString(","))
     }
 
     val missingAgent = requestHeaders[HttpHeaders.UserAgent] == null && content.headers[HttpHeaders.UserAgent] == null

diff --git a/ktor-client/ktor-client-tests/common/test/io/ktor/client/tests/HeadersTest.kt b/ktor-client/ktor-client-tests/common/test/io/ktor/client/tests/HeadersTest.kt
@@ -13,7 +13,7 @@ import kotlin.test.*
 class HeadersTest : ClientLoader() {
 
     @Test
-    fun headersReturnNullWhenMissing(): Unit = clientTests {
+    fun headersReturnNullWhenMissing() = clientTests {
         config {}
         test { client ->
             client.get<HttpResponse>("$TEST_SERVER/headers/").let {
@@ -25,4 +25,41 @@ class HeadersTest : ClientLoader() {
             }
         }
     }
+
+    @Test
+    fun headersMergeTest() = clientTests(listOf("Js")) {
+        config {}
+        test { client ->
+            client.get<HttpResponse>("$TEST_SERVER/headers-merge/") {
+                accept(ContentType.Text.Html)
+                accept(ContentType.Application.Json)
+            }.let {
+                assertEquals(HttpStatusCode.OK, it.status)
+                assertEquals("JSON", it.readText())
+                assertEquals("application/json; charset=UTF-8", it.headers[HttpHeaders.ContentType])
+            }
+
+            client.get<HttpResponse>("$TEST_SERVER/headers-merge/") {
+                accept(ContentType.Text.Html)
+                accept(ContentType.Application.Xml)
+            }.let {
+                assertEquals("XML", it.readText())
+                assertEquals("application/xml; charset=UTF-8", it.headers[HttpHeaders.ContentType])
+            }
+        }
+    }
+
+    @Test
+    fun testTest() = clientTests(listOf("Js")) {
+        config {}
+        test { client ->
+            val lines = client.get<String>("$HTTP_PROXY_SERVER/headers-merge") {
+                accept(ContentType.Application.Xml)
+                accept(ContentType.Application.Json)
+            }.split("\n")
+
+            val acceptHeaderLine = lines.first { it.startsWith("Accept:") }
+            assertEquals("Accept: application/xml,application/json", acceptHeaderLine)
+        }
+    }
 }
diff --git a/ktor-client/ktor-client-tests/common/test/io/ktor/client/tests/engine/UtilsTest.kt b/ktor-client/ktor-client-tests/common/test/io/ktor/client/tests/engine/UtilsTest.kt
@@ -0,0 +1,27 @@
+/*
+ * Copyright 2014-2020 JetBrains s.r.o and contributors. Use of this source code is governed by the Apache 2.0 license.
+ */
+
+package io.ktor.client.tests.engine
+
+import io.ktor.client.engine.*
+import io.ktor.client.utils.*
+import io.ktor.http.*
+import kotlin.test.*
+
+class UtilsTest {
+    @Test
+    fun testMergeHeaders() {
+        val headers = HeadersBuilder().apply {
+            append("Accept", "application/xml")
+            append("Accept", "application/json")
+        }
+
+        val result = mutableMapOf<String, String>()
+        mergeHeaders(headers.build(), EmptyContent) {
+                key, value -> result[key] = value
+        }
+
+        assertEquals("application/xml,application/json", result["Accept"])
+    }
+}
diff --git a/ktor-client/ktor-client-tests/jvm/src/io/ktor/client/tests/utils/tests/Headers.kt b/ktor-client/ktor-client-tests/jvm/src/io/ktor/client/tests/utils/tests/Headers.kt
@@ -19,5 +19,18 @@ internal fun Application.headersTestServer() {
                 call.respond(HttpStatusCode.OK, "OK")
             }
         }
+
+        route("/headers-merge") {
+            accept(ContentType.Application.Json) {
+                get("/") {
+                    call.respondText("JSON", ContentType.Application.Json, HttpStatusCode.OK)
+                }
+            }
+            accept(ContentType.Application.Xml) {
+                get("/") {
+                    call.respondText("XML", ContentType.Application.Xml, HttpStatusCode.OK)
+                }
+            }
+        }
     }
 }
diff --git a/ktor-client/ktor-client-tests/jvm/src/io/ktor/client/tests/utils/tests/Proxy.kt b/ktor-client/ktor-client-tests/jvm/src/io/ktor/client/tests/utils/tests/Proxy.kt
@@ -8,14 +8,18 @@ import io.ktor.client.utils.*
 import io.ktor.http.*
 import io.ktor.network.sockets.*
 import io.ktor.utils.io.*
+import kotlin.text.*
 
 suspend fun proxyHandler(socket: Socket) {
     val input = socket.openReadChannel()
     val output = socket.openWriteChannel()
 
     val statusLine = input.readUTF8Line()
+    val requestData = StringBuilder()
+    requestData.append(statusLine).append("\n")
     while (true) {
         val line = input.readUTF8Line() ?: ""
+        requestData.append(line).append("\n")
         if (line.isEmpty()) {
             break
         }
@@ -28,6 +32,13 @@ suspend fun proxyHandler(socket: Socket) {
                 append(HttpHeaders.ContentType, ContentType.Application.Json)
             }, "{\"status\": \"ok\"}"
         )
+        "GET /headers-merge HTTP/1.1" -> buildResponse(
+            HttpStatusCode.OK,
+            buildHeaders {
+                append(HttpHeaders.ContentType, ContentType.Text.Plain)
+            },
+            requestData.toString()
+        )
         else -> buildResponse(HttpStatusCode.BadRequest)
     }
 

diff --git a/ktor-server/ktor-server-test-host/jvm/src/io/ktor/server/testing/EngineTestSuite.kt b/ktor-server/ktor-server-test-host/jvm/src/io/ktor/server/testing/EngineTestSuite.kt
@@ -1894,7 +1894,7 @@ abstract class EngineTestSuite<TEngine : ApplicationEngine, TConfiguration : App
 
             get("/") {
                 assertEquals("foo", call.request.headers["X-Single-Value"])
-                assertEquals("foo;bar", call.request.headers["X-Double-Value"])
+                assertEquals("foo,bar", call.request.headers["X-Double-Value"])
 
                 assertNull(call.request.headers["X-Nonexistent-Header"])
                 assertNull(call.request.headers.getAll("X-Nonexistent-Header"))