Don't omit Content-Length header for Content-Length: 0 cases

[Kludex]

This PR adds the content-length header by default, if not already included. For more information, please check on #1099 and the referred RFC.

Next step would be to add a conditional to check if status_code < 200 and status_code != (204, 304)`

Closes #1099

[Kludex]

We are sure that content-length exists at this point, considering the change of conditional in the init_headers().

[lovelydinosaur]

I'd be a bit careful about that assumption. Personally I'd start by reverting this bit, making sure we've got the different test cases I've described. The StreamingResponse and FileResponse won't yet pass, but let's look at how we should resolve them once we've identified the test failures fully.

[lovelydinosaur]

Okay, so forgetting for a moment about 1xx, 204, 304, here's a sensible set of test cases that I can think of wrt. setting Content-Length...

• Response() - Regular empty response. Content-Length: 0 (RedirectResponse is a good real-world example of this case)
• Response(content='...') - Regular non-empty response. Content-Length: x
• FileResponse(...) - File response, unknown length. Transfer-Encoding: chunked.
• FileResponse(..., stat_result=...) - File response, known length. Content-Length: x
• StreamingResponse(...) - Streaming response, unknown length. Transfer-Encoding: chunked
• StreamingResponse(..., headers={"Content-Length": "123"}) - Streaming response, known length. Content-Length: x

For Transfer-Encoding: chunked cases it's also potentially okay to just omit the header, since I think the ASGI server is responsible for adding that, but I've not fully verified that.

I'd suggest the next step here is expanding the tests in this PR so that we're covering each case, and can see which ones need fixing up.

[lovelydinosaur]

This is the test case that we should probably expand to cover the 6 different cases described. I guess we want these test cases?...

• test_empty_response
• test_non_empty_response
• test_file_response_unknown_size
• test_file_response_known_size
• test_streaming_response_unknown_size
• test_streaming_response_known_size

[Kludex]

When do we have a FileResponse with unknown size?

[lovelydinosaur]

I've retitled this PR to be more representative of the change.

[Kludex]

Expressing the thoughts I've shared on Gitter here as well:

I need to be sure that the response class is not StreamingResponse or FileResponse before adding the Content-Length: 0, because those four cases are already being handled now (file response unknown size, file response known size, streaming response known size, and streaming response unknown size).

I don't have (or I can't see) any attribute I can check to be sure it's not a "Transfer-Encoding: chunked" on the at the init_headers() point.

I've added a conditional on the Response.init_headers() to check if the response class was one of those two before adding the content-length: 0 headers... Can we do better?

[Kludex]

Updated the solution to check if body is present on the response. If it's not, then it doesn't add the Content-Lenght: 0 headers.

[lovelydinosaur]

Great! This is a really nicely targeted fix now. 👍

Possibly improvements still for the 1xx, 204, 304 cases, but we can consider those as a separate conversation.

[Kludex]

For future reference, some discussions about this PR can be retrieved on Gitter: https://gitter.im/encode/community?at=61d6e204526fb77b3166a445

[Kludex]

I've removed the period at the end to squash the commit.

Thanks @tomchristie :)

[lovelydinosaur]

I didn't realise at the time, but we could just name this one test_file_response.

[Kludex]

There's one above already (that also checks the content-length being present), maybe it's just redundant.