Comments (5)
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.
Related Issues (20)
- Provide plain-english alternatives to every method in the symbolic DSL
- Bump to AHC to 2.0.33
- Possible domain / group id change HOT 4
- Implement implicit vs explicit verb setting
- Cross-build / abstract against AHC HOT 1
- Investigate different thread allocation semantics under the hood
- Please publish a version for Scala 2.13.0-RC1 HOT 15
- sbt eviction notice HOT 1
- Incorrect UriEncoding of emoji characters HOT 1
- setting request body with << changes method from PUT to POST HOT 6
- Requesting URLs with custom parameters is not possible HOT 6
- Emoji character in domains not recognized correctly (punycode) HOT 1
- Emoji character in domains not recognized correctly (punycode) HOT 8
- Rename primary branch HOT 1
- Add support for Scala 2.13 HOT 1
- Add support for Dotty HOT 5
- Update scala-xml dependency to version 2
- Set up dependency update for this repository
- Snapshots are not deployed to sonatype HOT 1
- Upgrade to AHC 3.0.0 HOT 3
Recommend Projects
-
React
A declarative, efficient, and flexible JavaScript library for building user interfaces.
-
Vue.js
🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
-
Typescript
TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
-
TensorFlow
An Open Source Machine Learning Framework for Everyone
-
Django
The Web framework for perfectionists with deadlines.
-
Laravel
A PHP framework for web artisans
-
D3
Bring data to life with SVG, Canvas and HTML. 📊📈🎉
-
Recommend Topics
-
javascript
JavaScript (JS) is a lightweight interpreted programming language with first-class functions.
-
web
Some thing interesting about web. New door for the world.
-
server
A server is a program made to process requests and deliver data to clients.
-
Machine learning
Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.
-
Visualization
Some thing interesting about visualization, use data art
-
Game
Some thing interesting about game, make everyone happy.
Recommend Org
-
Facebook
We are working to build community through open source technology. NB: members must have two-factor auth.
-
Microsoft
Open source projects and samples from Microsoft.
-
Google
Google ❤️ Open Source for everyone.
-
Alibaba
Alibaba Open Source for everyone
-
D3
Data-Driven Documents codes.
-
Tencent
China tencent open source team.
from reboot.