Closed dxuhuang closed 7 years ago
Merging #569 into develop will increase coverage by
0.22%
. The diff coverage is83.33%
.
@@ Coverage Diff @@
## develop #569 +/- ##
===========================================
+ Coverage 84.69% 84.91% +0.22%
===========================================
Files 98 99 +1
Lines 4422 4427 +5
Branches 360 348 -12
===========================================
+ Hits 3745 3759 +14
+ Misses 677 668 -9
Impacted Files | Coverage Δ | |
---|---|---|
...rc/main/scala/colossus/service/ServiceClient.scala | 91.6% <ø> (-2.8%) |
:arrow_down: |
...n/scala/colossus/controller/OutputController.scala | 88.09% <0%> (ø) |
:arrow_up: |
...rc/main/scala/colossus/protocols/http/Header.scala | 84.02% <0%> (+0.69%) |
:arrow_up: |
...a/colossus/protocols/http/HttpResponseParser.scala | 68.18% <100%> (+0.32%) |
:arrow_up: |
.../main/scala/colossus/protocols/http/HttpBody.scala | 85.71% <100%> (+6.54%) |
:arrow_up: |
...la/colossus/protocols/http/HttpRequestParser.scala | 95.23% <100%> (+3.23%) |
:arrow_up: |
...n/scala/colossus/protocols/http/HttpResponse.scala | 89.13% <100%> (+0.24%) |
:arrow_up: |
...s/src/main/scala/colossus/core/server/Server.scala | 95.07% <0%> (-0.71%) |
:arrow_down: |
...src/main/scala/colossus/metrics/StatReporter.scala | 73.91% <0%> (ø) |
:arrow_up: |
... and 17 more |
Continue to review full report at Codecov.
Legend - Click here to learn more
Δ = absolute <relative> (impact)
,ø = not affected
,? = missing data
Powered by Codecov. Last update 7eddc1a...c4a523b. Read the comment docs.
You are correct that you can set the content-type via withHeader
, but that isn't very useful since you'll end up with two content-type headers when you actually want one (the one set withHeader
). I know the problem has bitten me twice and at least one other person. I'm not sure what functionality we would lose/break if content type was removed from the body.
Yeah that's definitely a good point. I'm going to go ahead and approve this PR. I'm still a little concerned that there's a better reason for keeping the header with the body that I'm just not thinking of right now, but since the PR still allows the HttpBodyEncoder
to work properly, which was my main concern, I think it should be ok.
👍
bump @benblack86 updated docs
@DanSimon @benblack86 @aliyakamercan @amotamed @jlbelmonte
Second stab at https://github.com/tumblr/colossus/issues/545
So what we'd like to be able to do is set the content-type on the
HttpResponse
without having to bother the internalHttpBody
. I don't see an inherent reason why the content-type has to be part ofHttpBody
anyway; after all, the only substantial place it's used is in theencode()
method that writes to theDynamicOutBuffer
, and the body itself is just a byte array. If we add the content-typeHeader
properly,HttpResponseHead.encode()
will take care of it just the same.The only issue I see is potential duplicate headers since
HttpHeaders
is just a wrapper around a linked list, but I think that should still be handled inHttpHeaders
alone. In fact I'm thinking about going a bit further and makingHttpHeaders
wrap aHashMap[String, HttpHeader]
instead.Discuss.