Encoding of response with empty body #6444
Conversation
| @@ -63,8 +64,12 @@ private[ember] object Encoder { | |||
| () | |||
| } | |||
| } | |||
| if (!chunked && !appliedContentLength && resp.status.isEntityAllowed) { | |||
| stringBuilder.append(chunkedTansferEncodingHeaderRaw).append(CRLF) | |||
| if (!appliedContentLength && resp.body == EmptyBody && resp.status.isEntityAllowed) { | |||
There was a problem hiding this comment.
The logic here looks correct, but it's observable behavior that's dependent on reference equality of a stream. They're both "correct" HTTP, and this is a better outcome for a common case. I think I'm okay with this, but calling it out.
In main, with the new entity model, we can target the Empty entity and not have this concern.
There was a problem hiding this comment.
I looked at it, and I could not see any nice way to check whether a fs2 stream is empty. Maybe somebody can enlighten me.
There was a problem hiding this comment.
I am still confused why it was broken with an empty body and the chunked encoding.
There was a problem hiding this comment.
There really isn't a good way. We can "peek" a stream, but that semantically blocks until the first chunk of the body (if any) is produced, and would delay rendering the headers. In scalaz-stream, we used to be able to introspect the algebra and find a wider class of empty streams, but still not all off them. Effects are, well, effectful.
There was a problem hiding this comment.
Ah ok. Makes sense 👍
There was a problem hiding this comment.
I think I'm okay with this, but calling it out.
Seems fine to me.
| test("respToBytes should encode a no body response correctly with stream") { | ||
| val resp = Response[IO](Status.Ok, body = Stream.chunk(Chunk.empty)) | ||
|
|
||
| val expected = | ||
| """HTTP/1.1 200 OK | ||
| |Transfer-Encoding: chunked | ||
| | | ||
| |0 | ||
| | | ||
| |""".stripMargin | ||
|
|
||
| Helpers.encodeResponseRig(resp).assertEquals(expected) | ||
| } |
There was a problem hiding this comment.
So is this intended behavior? I guess it's the only implementation that makes sense.
There was a problem hiding this comment.
Yeah, I don't think we can do better than this. And even though it's arguably overspecified, I like the test to alert us to behavior changes.
|
This will need an import sort. I think |
|
Only question is whether to actually have |
|
Do those tests pass on this branch without your change? I think only one of them would? We will want to tweak this on the merge to main, where we can detect empty entities (it's a sum type, so no FS2 introspection is involved). |
|
Yes. Only one will pass. |
|
I'd say this is correctly targeted for 0.23 then. |
| @@ -63,8 +64,12 @@ private[ember] object Encoder { | |||
| () | |||
| } | |||
| } | |||
| if (!chunked && !appliedContentLength && resp.status.isEntityAllowed) { | |||
| stringBuilder.append(chunkedTansferEncodingHeaderRaw).append(CRLF) | |||
| if (!appliedContentLength && resp.body == EmptyBody && resp.status.isEntityAllowed) { | |||
There was a problem hiding this comment.
I think I'm okay with this, but calling it out.
Seems fine to me.
Fixes #6427
Around the repository there are different ways that empty responses are encoded. In some places the content length is set to 0, and in other places like the auth middleware, an empty response is returned with no headers set.
Here is a potential fix where an empty body with
content-length: 0is always set when the status code allows a non-empty response and the header is not already set.Something similar could probably also be done in the request encoding as well.