Multiple Content-Type Headers about reboot HOT 5 CLOSED

As you say, as per the HTTP spec, dispatch should not send two Content-Type headers in any case (I don't think anyone disagrees on that one). I also agree that the order in which to set body and header should not matter, i.e.
url("http://google.com").POST << "some content" <:< headers and
url("http://google.com").POST <:< headers << "some content"
should be equivalent. A fallback Content-Type header should only be used if the user did not specify anything manually, I guess.

from reboot.

Digging a bit deeper here, the defaultContentType method calls setContentType which itself calls setHeader, eventually on the underlying RequestBuilder.

The implementation of setHeader does the right thing, it replaces any headers of the same name. I added a test for this using the setContentType method: 267e0a3

The demo project that @efuquen linked to uses the <:< verb, which calls addHeader rather than setHeader. At that point defaultContentType has already done its thing and addHeader will do what it is supposed to do, which is to not care whether the given header has already been added to the request.

In hindsight, yes this feature shouldn't have been included in a point release. And the default change produces unexpected results when used in conjunction with <:<. I'd like to make an improvement here, but I don't see a way for the default setting to be more sensitive than it already is. We're not going to special-case <:< for particular header key strings.

We could make some headway on what I see an another underlying problem here, the fact that <:< adds rather than sets headers even when used with a Map where keys must be unique. If we overload <:< to use setHeader when the provided value is known to be a Map[String,String], everything would work as @efuquen expected. Providing a more general Traversable[(String,String)] would continue to use addHeader, as it should.

Of course, there's still a risk that we'd break somebody else's code

request  <:< Map("repeating-header" -> "value1") <:< Map("repeating-header" -> "value2")

Hopefully that is not what anybody is doing, when they can instead provide a sequence of tuples.

Thoughts on this option?

from reboot.

Late to the party, and sorry about this! I sent in #72 without thinking about the <:< interactions.

We're not going to special-case <:< for particular header key strings.

Why not? I think it makes sense to treat Content-Type differently, since we went down that path already. In my case I have two sets of source code. foo.scala that's dispatch-agnostic code, which sets the HTTP headers and dispatch wrappers like httpclients_dispatch0111.scala for different versions of dispatch.

// foo.scala: dispatch non-specific source
val headers = Map("Content-Type" -> "application/soap+xml; charset=UTF-8; action=something", ...)

// httpclients_dispatch0111.scala
val req = url(address.toString).setContentType("text/xml", "utf-8") << in <:< headers

If <:< could specially treat Content-Type and turn it into setContentType(...) call, that would probably break less of the existing code when people upgrade to 0.11.2.

from reboot.

Although I am somewhat hesitant with the Content-Type specific solution, I agree it's less likely to break anyone's code vs the more generic one outlined by @n8han. But, one problem I see with @n8han solution is it's not clear from a verb like <:< that using a Map as input will do something different then using a Traversable without it being documented somewhere, vs the more clear calls to setHeader and addHeader. Regardless, I think a change like that shouldn't happen with the next point release.

I would say the ideal thing to do would to have any header that has been set by default by the library be overwriteable, but behavior outside of that should remain the same from the previous version. I.e. if I set only one Content-Type header then it should override the default text/plain value, but if I were to set two Content-Type's then the first one should override the default and the second Content-Type (and third, fourth, etc.) should be added as new headers. Even though this is still not correct in terms of the standard this would preserve the same behavior we had with 0.11.0 but will still set a default Content-Type header if none has been provided for string input, preserving the improvement in #72 . This also once again leaves any potential bad behavior at the foot of the library user, he will get two Content-Type headers because he mistakenly set two, not because the library has added one without his knowledge.

Then, any above changes that would potentially break some previous code could be made in the 0.12.x series, if it's agreed upon as a better way to handle this case. Also, here is a link to the section on headers of the new RFC that supersedes the old single HTTP 1.1 RFC. After a quick read doesn't seem like much has changed with regard to multiple headers, but figured it would be good to link the new authoritative source.

http://tools.ietf.org/html/rfc7230#section-3.2

from reboot.

I've submitted a PR with my proposal to resolve this issue, slightly different from what I outlined above but a cleaner solution.

from reboot.