JsonFeature: Fixed header behavior

ktor-client/ktor-client-features/ktor-client-json/common/src/io/ktor/client/features/json/JsonFeature.kt

@@ -24,6 +24,25 @@ import io.ktor.utils.io.*
  */
 expect fun defaultSerializer(): JsonSerializer
 
+/**
+ * Configuration values for [JsonFeature.Config.addAcceptHeader], defining when accept header is
+ * added to requests.
+ */
+enum class AddAcceptHeader {
+    /** Always adds accept header(s) even if the request already has such a header. */
+    Always,
+
+    /**
+     * Only adds accept header(s) if none exists in the request.
+     *
+     * This can be used to make sure that only one accept header is ever sent to the server.
+     */
+    IfAcceptHeaderMissing,
+
+    /** Never adds accept header(s) to requests. */
+    Never
+}
+
 /**
  * [HttpClient] feature that serializes/de-serializes as JSON custom objects
  * to request and from response bodies using a [serializer].
@@ -37,15 +56,21 @@ expect fun defaultSerializer(): JsonSerializer
  *
  * @property serializer that is used to serialize and deserialize request/response bodies
  * @property acceptContentTypes that are allowed when receiving content
+ * @property addAcceptHeader defines when accept headers are added to requests
  */
 class JsonFeature internal constructor(
     val serializer: JsonSerializer,
-    @KtorExperimentalAPI val acceptContentTypes: List<ContentType>
+    @KtorExperimentalAPI val acceptContentTypes: List<ContentType>,
+    val addAcceptHeader: AddAcceptHeader
 ) {
-    @Deprecated("Install feature properly instead of direct instantiation.", level = DeprecationLevel.ERROR)
+    @Deprecated(
+        "Install feature properly instead of direct instantiation.",
+        level = DeprecationLevel.ERROR
+    )
     constructor(serializer: JsonSerializer) : this(
         serializer,
-        listOf(ContentType.Application.Json)
+        listOf(ContentType.Application.Json),
+        AddAcceptHeader.Always
     )
 
     /**
@@ -62,7 +87,8 @@ class JsonFeature internal constructor(
         /**
          * Backing field with mutable list of content types that are handled by this feature.
          */
-        private val _acceptContentTypes: MutableList<ContentType> = mutableListOf(ContentType.Application.Json)
+        private val _acceptContentTypes: MutableList<ContentType> =
+            mutableListOf(ContentType.Application.Json)
 
         /**
          * List of content types that are handled by this feature.
@@ -86,6 +112,13 @@ class JsonFeature internal constructor(
         fun accept(vararg contentTypes: ContentType) {
             _acceptContentTypes += contentTypes
         }
+
+        /**
+         * Defines under which conditions accept headers are added to requests.
+         *
+         * Default value is [AddAcceptHeader.Always].
+         */
+        var addAcceptHeader: AddAcceptHeader = AddAcceptHeader.Always
     }
 
     /**
@@ -98,13 +131,26 @@ class JsonFeature internal constructor(
             val config = Config().apply(block)
             val serializer = config.serializer ?: defaultSerializer()
             val allowedContentTypes = config.acceptContentTypes.toList()
+            val addAcceptHeader = config.addAcceptHeader
 
-            return JsonFeature(serializer, allowedContentTypes)
+            return JsonFeature(serializer, allowedContentTypes, addAcceptHeader)
         }
 
         override fun install(feature: JsonFeature, scope: HttpClient) {
             scope.requestPipeline.intercept(HttpRequestPipeline.Transform) { payload ->
-                feature.acceptContentTypes.forEach { context.accept(it) }
+                if (feature.addAcceptHeader == AddAcceptHeader.Always
+                    || (feature.addAcceptHeader == AddAcceptHeader.IfAcceptHeaderMissing
+                        && !context.headers.contains(HttpHeaders.Accept))
+                ) {
+                    feature.acceptContentTypes.forEach { acceptType ->
+                        // Don't add duplicate headers. Otherwise, some servers can return 400
+                        // and we might not always be in the position to fix the server.
+                        val accepted = context.headers.getAll(HttpHeaders.Accept)
+                        if (accepted?.any { acceptType.match(it) } != true) {
+                            context.accept(acceptType)
+                        }
+                    }
+                }
 
                 val contentType = context.contentType() ?: return@intercept
                 if (feature.acceptContentTypes.none { contentType.match(it) })
@@ -123,7 +169,9 @@ class JsonFeature internal constructor(
             scope.responsePipeline.intercept(HttpResponsePipeline.Transform) { (info, body) ->
                 if (body !is ByteReadChannel) return@intercept
 
-                if (feature.acceptContentTypes.none { context.response.contentType()?.match(it) == true }) {
+                if (feature.acceptContentTypes.none {
+                        context.response.contentType()?.match(it) == true
+                    }) {
                     return@intercept
                 }
 

ktor-client/ktor-client-features/ktor-client-json/common/test/JsonFeatureTest.kt

@@ -4,6 +4,10 @@
 
 package io.ktor.client.features.json
 
+import io.ktor.client.engine.mock.*
+import io.ktor.client.request.*
+import io.ktor.client.statement.*
+import io.ktor.client.tests.utils.*
 import io.ktor.http.*
 import kotlin.test.*
 
@@ -36,4 +40,64 @@ class JsonFeatureTest {
         assertTrue { config.acceptContentTypes.contains(ContentType.Application.Xml) }
         assertTrue { config.acceptContentTypes.contains(ContentType.Application.Pdf) }
     }
+
+    @Test
+    fun testAddAcceptHeaderAlways() = testWithEngine(MockEngine) {
+        acceptConfig(AddAcceptHeader.Always)
+
+        test { client ->
+            assertEquals("application/json", client.get())
+            assertEquals("application/json", client.get {
+                accept(ContentType.Application.Json)
+            })
+            assertEquals("application/xml,application/json", client.get {
+                accept(ContentType.Application.Xml)
+            })
+        }
+    }
+
+    @Test
+    fun testAddAcceptHeaderIfMissing() = testWithEngine(MockEngine) {
+        acceptConfig(AddAcceptHeader.IfAcceptHeaderMissing)
+
+        test { client ->
+            assertEquals("application/json", client.get())
+            assertEquals("application/json", client.get {
+                accept(ContentType.Application.Json)
+            })
+            assertEquals("application/xml", client.get {
+                accept(ContentType.Application.Xml)
+            })
+        }
+    }
+
+    @Test
+    fun testAddAcceptHeaderNever() = testWithEngine(MockEngine) {
+        acceptConfig(AddAcceptHeader.Never)
+
+        test { client ->
+            assertEquals("*/*", client.get())
+            assertEquals("application/json", client.get {
+                accept(ContentType.Application.Json)
+            })
+            assertEquals("application/xml", client.get {
+                accept(ContentType.Application.Xml)
+            })
+        }
+    }
+
+    fun TestClientBuilder<MockEngineConfig>.acceptConfig(value: AddAcceptHeader) {
+        config {
+            engine {
+                addHandler { request ->
+                    respondOk(
+                        request.headers.getAll(HttpHeaders.Accept)?.joinToString(",") ?: ""
+                    )
+                }
+            }
+            install(JsonFeature) {
+                addAcceptHeader = value
+            }
+        }
+    }
 }