Change behavior of adding headers to default to replacing header valu…

…es (#2149)

core/src/main/scala/sttp/client4/requestBuilder.scala

@@ -68,73 +68,83 @@ trait PartialRequestBuilder[+PR <: PartialRequestBuilder[PR, R], +R]
   def options(uri: Uri): R = method(Method.OPTIONS, uri)
   def patch(uri: Uri): R = method(Method.PATCH, uri)
 
-  def contentType(ct: String): PR =
-    header(HeaderNames.ContentType, ct, replaceExisting = true)
-  def contentType(mt: MediaType): PR =
-    header(HeaderNames.ContentType, mt.toString, replaceExisting = true)
+  def contentType(ct: String): PR = header(HeaderNames.ContentType, ct)
+  def contentType(mt: MediaType): PR = header(HeaderNames.ContentType, mt.toString)
   def contentType(ct: String, encoding: String): PR =
-    header(HeaderNames.ContentType, contentTypeWithCharset(ct, encoding), replaceExisting = true)
-  def contentLength(l: Long): PR =
-    header(HeaderNames.ContentLength, l.toString, replaceExisting = true)
+    header(HeaderNames.ContentType, contentTypeWithCharset(ct, encoding))
+  def contentLength(l: Long): PR = header(HeaderNames.ContentLength, l.toString)
 
-  /** Adds the given header to the end of the headers sequence.
-    * @param replaceExisting
-    *   If there's already a header with the same name, should it be replaced?
+  /** Adds the given header to the headers of this request. If a header with the same name already exists, the default
+    * is to replace it with the given one.
+    *
+    * @param onDuplicate
+    *   What should happen if there's already a header with the same name. The default is to replace.
     */
-  def header(h: Header, replaceExisting: Boolean = false): PR = {
-    val current = if (replaceExisting) headers.filterNot(_.is(h.name)) else headers
-    withHeaders(headers = current :+ h)
-  }
+  def header(h: Header, onDuplicate: DuplicateHeaderBehavior = DuplicateHeaderBehavior.Replace): PR =
+    onDuplicate match {
+      case DuplicateHeaderBehavior.Replace =>
+        val filtered = headers.filterNot(_.is(h.name))
+        withHeaders(headers = filtered :+ h)
+      case DuplicateHeaderBehavior.Combine =>
+        val (existing, other) = headers.partition(_.is(h.name))
+        val separator = if (h.is(HeaderNames.Cookie)) "; " else ", "
+        val combined = Header(h.name, (existing.map(_.value) :+ h.value).mkString(separator))
+        withHeaders(headers = other :+ combined)
+      case DuplicateHeaderBehavior.Add =>
+        withHeaders(headers = headers :+ h)
+    }
 
-  /** Adds the given header to the end of the headers sequence.
-    * @param replaceExisting
-    *   If there's already a header with the same name, should it be replaced?
+  /** Adds the given header to the headers of this request.
+    * @param onDuplicate
+    *   What should happen if there's already a header with the same name. See [[header(Header)]].
     */
-  def header(k: String, v: String, replaceExisting: Boolean): PR =
-    header(Header(k, v), replaceExisting)
+  def header(k: String, v: String, onDuplicate: DuplicateHeaderBehavior): PR =
+    header(Header(k, v), onDuplicate)
 
-  /** Adds the given header to the end of the headers sequence. */
+  /** Adds the given header to the headers of this request. If a header with the same name already exists, it's
+    * replaced.
+    */
   def header(k: String, v: String): PR = header(Header(k, v))
 
-  /** Adds the given header to the end of the headers sequence, if the value is defined. Otherwise has no effect. */
+  /** Adds the given header to the headers of this request, if the value is defined. Otherwise has no effect. If a
+    * header with the same name already exists, it's replaced.
+    */
   def header(k: String, ov: Option[String]): PR = ov.fold(this)(header(k, _))
 
-  /** Adds the given headers to the end of the headers sequence. */
+  /** Adds the given headers to the headers of this request. If a header with the same name already exists, it's
+    * replaced.
+    */
   def headers(hs: Map[String, String]): PR = headers(hs.map(t => Header(t._1, t._2)).toSeq: _*)
 
-  /** Adds the given headers to the end of the headers sequence.
-    * @param replaceExisting
-    *   If there's already a header with the same name, should it be replaced?
+  /** Adds the given headers to the headers of this request. If a header with the same name already exists, it's
+    * replaced.
     */
-  def headers(hs: Map[String, String], replaceExisting: Boolean): PR =
-    if (replaceExisting) hs.foldLeft(this)((s, h) => s.header(h._1, h._2, replaceExisting))
-    else headers(hs)
-
-  /** Adds the given headers to the end of the headers sequence. */
-  def headers(hs: Header*): PR = withHeaders(headers = headers ++ hs)
-
-  /** Adds the given headers to the end of the headers sequence. */
-  def headers(hs: Seq[Header], replaceExisting: Boolean): PR =
-    if (replaceExisting) hs.foldLeft(this)((s, h) => s.header(h, replaceExisting))
-    else headers(hs: _*)
+  def headers(hs: Header*): PR = hs.foldLeft(this)(_.header(_))
 
   def auth: SpecifyAuthScheme[PR] =
     new SpecifyAuthScheme[PR](HeaderNames.Authorization, this, DigestAuthenticationBackend.DigestAuthTag)
   def proxyAuth: SpecifyAuthScheme[PR] =
     new SpecifyAuthScheme[PR](HeaderNames.ProxyAuthorization, this, DigestAuthenticationBackend.ProxyDigestAuthTag)
-  def acceptEncoding(encoding: String): PR =
-    header(HeaderNames.AcceptEncoding, encoding, replaceExisting = true)
+  def acceptEncoding(encoding: String): PR = header(HeaderNames.AcceptEncoding, encoding)
 
+  /** Adds the given cookie. Any previously defined cookies are left intact. */
   def cookie(nv: (String, String)): PR = cookies(nv)
+
+  /** Adds the given cookie. Any previously defined cookies are left intact. */
   def cookie(n: String, v: String): PR = cookies((n, v))
+
+  /** Adds the cookies from the given response. Any previously defined cookies are left intact. */
   def cookies(r: Response[_]): PR = cookies(r.cookies.collect { case Right(c) => c }.map(c => (c.name, c.value)): _*)
+
+  /** Adds the given cookies. Any previously defined cookies are left intact. */
   def cookies(cs: Iterable[CookieWithMeta]): PR = cookies(cs.map(c => (c.name, c.value)).toSeq: _*)
-  def cookies(nvs: (String, String)*): PR =
-    header(
-      HeaderNames.Cookie,
-      (headers.find(_.name == HeaderNames.Cookie).map(_.value).toSeq ++ nvs.map(p => p._1 + "=" + p._2)).mkString("; "),
-      replaceExisting = true
-    )
+
+  /** Adds the given cookies. Any previously defined cookies are left intact. */
+  def cookies(nvs: (String, String)*): PR = header(
+    HeaderNames.Cookie,
+    nvs.map(p => p._1 + "=" + p._2).mkString("; "),
+    onDuplicate = DuplicateHeaderBehavior.Combine
+  )
 
   private[client4] def hasContentType: Boolean = headers.exists(_.is(HeaderNames.ContentType))
   private[client4] def setContentTypeIfMissing(mt: MediaType): PR =
@@ -364,3 +374,19 @@ final case class PartialRequest[T](
   *   The type of request
   */
 trait RequestBuilder[+R <: RequestBuilder[R]] extends PartialRequestBuilder[R, R] { self: R => }
+
+/** Specifies what should happen when adding a header to a request description, and a header with that name already
+  * exists. See [[PartialRequestBuilder.header(Header)]].
+  */
+sealed trait DuplicateHeaderBehavior
+object DuplicateHeaderBehavior {
+
+  /** Replaces any headers with the same name. */
+  case object Replace extends DuplicateHeaderBehavior
+
+  /** Combines the header values using `,`, except for `Cookie`, where values are combined using `;`. */
+  case object Combine extends DuplicateHeaderBehavior
+
+  /** Adds the header, leaving any other headers with the same name intact. */
+  case object Add extends DuplicateHeaderBehavior
+}

core/src/test/scala/sttp/client4/RequestTests.scala

@@ -66,35 +66,46 @@ class RequestTests extends AnyFlatSpec with Matchers {
   }
 
   it should "properly replace headers" in {
-    emptyRequest.header("H1", "V1").header("H1", "V2").headers shouldBe List(Header("H1", "V1"), Header("H1", "V2"))
-    emptyRequest.header("H1", "V1").header("H1", "V2", replaceExisting = true).headers shouldBe List(Header("H1", "V2"))
-
-    emptyRequest.header(Header("H1", "V1")).header(Header("H1", "V2")).headers shouldBe List(
+    emptyRequest.header("H1", "V1").header("H1", "V2").headers shouldBe List(Header("H1", "V2"))
+    emptyRequest.header("H1", "V1").header("H1", "V2", onDuplicate = DuplicateHeaderBehavior.Add).headers shouldBe List(
       Header("H1", "V1"),
       Header("H1", "V2")
     )
-    emptyRequest.header(Header("H1", "V1")).header(Header("H1", "V2"), replaceExisting = true).headers shouldBe List(
+    emptyRequest
+      .header("H1", "V1")
+      .header("H1", "V2", onDuplicate = DuplicateHeaderBehavior.Combine)
+      .headers shouldBe List(Header("H1", "V1, V2"))
+
+    emptyRequest.header(Header("H1", "V1")).header(Header("H1", "V2")).headers shouldBe List(Header("H1", "V2"))
+    emptyRequest
+      .header(Header("H1", "V1"))
+      .header(Header("H1", "V2"), onDuplicate = DuplicateHeaderBehavior.Add)
+      .headers shouldBe List(
+      Header("H1", "V1"),
       Header("H1", "V2")
     )
 
     emptyRequest
       .headers(Map("H1" -> "V1", "H2" -> "V2"))
       .headers(Map("H1" -> "V11", "H3" -> "V3"))
       .headers
-      .toSet shouldBe Set(Header("H1", "V1"), Header("H2", "V2"), Header("H1", "V11"), Header("H3", "V3"))
-    emptyRequest
-      .headers(Map("H1" -> "V1", "H2" -> "V2"))
-      .headers(Map("H1" -> "V11", "H3" -> "V3"), replaceExisting = true)
-      .headers
       .toSet shouldBe Set(Header("H2", "V2"), Header("H1", "V11"), Header("H3", "V3"))
 
     emptyRequest
       .headers(Header("H1", "V1"), Header("H2", "V2"))
       .headers(Header("H1", "V11"), Header("H3", "V3"))
-      .headers shouldBe List(Header("H1", "V1"), Header("H2", "V2"), Header("H1", "V11"), Header("H3", "V3"))
-    emptyRequest
-      .headers(Header("H1", "V1"), Header("H2", "V2"))
-      .headers(List(Header("H1", "V11"), Header("H3", "V3")), replaceExisting = true)
       .headers shouldBe List(Header("H2", "V2"), Header("H1", "V11"), Header("H3", "V3"))
   }
+
+  it should "use same headers regardless of order in which body() and headers() are called" in {
+    emptyRequest.headers(Map("Content-Type" -> "application/json")).body("1234").headers.toSet shouldBe Set(
+      Header("Content-Type", "application/json"),
+      Header("Content-Length", "4")
+    )
+
+    emptyRequest.body("1234").headers(Map("Content-Type" -> "application/json")).headers.toSet shouldBe Set(
+      Header("Content-Type", "application/json"),
+      Header("Content-Length", "4")
+    )
+  }
 }

core/src/test/scalanative/sttp/client4/testing/SyncHttpTest.scala

@@ -288,19 +288,19 @@ trait SyncHttpTest
     }
 
     "decompress using gzip" in {
-      val req = compress.header("Accept-Encoding", "gzip", replaceExisting = true)
+      val req = compress.header("Accept-Encoding", "gzip")
       val resp = req.send(backend)
       resp.body should be(Right(decompressedBody))
     }
 
     "decompress using deflate" in {
-      val req = compress.header("Accept-Encoding", "deflate", replaceExisting = true)
+      val req = compress.header("Accept-Encoding", "deflate")
       val resp = req.send(backend)
       resp.body should be(Right(decompressedBody))
     }
 
     "work despite providing an unsupported encoding" in {
-      val req = compress.header("Accept-Encoding", "br", replaceExisting = true)
+      val req = compress.header("Accept-Encoding", "br")
       val resp = req.send(backend)
       resp.body should be(Right(decompressedBody))
     }

docs/requests/headers.md

@@ -10,17 +10,20 @@ basicRequest.header("User-Agent", "myapp")
 
 As with any other request definition modifier, this method will yield a new request, which has the given header set. The headers can be set at any point when defining the request, arbitrarily interleaved with other modifiers.
 
-While most headers should be set only once on a request, HTTP allows setting a header multiple times. That's why the `header` method has an additional optional boolean parameter, `replaceExisting`, which defaults to `true`. This way, if the same header is specified twice, only the last value will be included in the request. If previous values should be preserved, set this parameter to `false`.
+While most headers should be set only once on a request, HTTP allows setting a header multiple times, or with multiple values. That's why the `header` method has an additional parameter, `onDuplicate`, which by default is set to `DuplicateHeaderBehavior.Replace`. This way, if the same header is specified twice, only the last value will be included in the request. Alternatively:
+
+* if previous values should be preserved, set this parameter to `DuplicateHeaderBehavior.Add`
+* if the values of the headers should be combined using `,`, or in case of cookies with `;`, use `DuplicateHeaderBehavior.Combine`
 
 There are also variants of this method accepting a number of headers:
 
 ```scala mdoc:compile-only
 import sttp.client4._
 import sttp.model._
 
-basicRequest.header(Header("k1", "v1"), replaceExisting = false)
+basicRequest.header(Header("k1", "v1"), onDuplicate = DuplicateHeaderBehavior.Add)
 basicRequest.header("k2", "v2")
-basicRequest.header("k3", "v3", replaceExisting = true)
+basicRequest.header("k3", "v3", DuplicateHeaderBehavior.Combine)
 basicRequest.headers(Map("k4" -> "v4", "k5" -> "v5"))
 basicRequest.headers(Header("k9", "v9"), Header("k10", "v10"), Header("k11", "v11"))
 ```