Implement EmptyResponse class

[declaresub]

Closes #1178, #1099

[JayH5]

Suggested change

	Signature: `Response(status_code:int, headers: typing.Optional[typing.Dict[str, str]] = None, background: typing.Optional[BackgroundTask] = None)`
	Signature: `EmptyResponse(status_code:int, headers: typing.Optional[typing.Dict[str, str]] = None, background: typing.Optional[BackgroundTask] = None)`

Alternatively, just take this line out. We don't have proper, generated API docs so trying to be consistent is difficult.

[declaresub]

I fixed the class name. Following the superclass documentation seemed like the right thing to do.

[tomchristie]

I'm not convinced that we'd need a separate response class to deal with this - although maybe I'v not looked into it enough.
Don't recall Django, Flask, and others taking this approach. Can't we instead make sure that Response either includes or doesn't include the appropriate headers depending on the status code?

[declaresub]

You certainly could take the approach of adding more special-case code to Response. Since the status_code attribute can be set after initialization, I believe this approach would best be done in Response.call. There you would check the value of self.status_code, modify headers as appropriate just before sending, and send an empty body for the status codes that require it.

I would be willing to do this.

[Kludex]

I'm not convinced that we'd need a separate response class to deal with this - although maybe I'v not looked into it enough. Don't recall Django, Flask, and others taking this approach. Can't we instead make sure that Response either includes or doesn't include the appropriate headers depending on the status code?

Flask: They do the same as what this PR proposes.

Django:

• 304: /~https://github.com/django/django/blob/bf7afe9c4e21f5fe5090c47b2b6ffc5a03a85815/django/http/response.py#L534-L545
• I can't find a special treatment for the other status code on Django, and also, see this comment.

[declaresub]

I have revised the pull request to move handling of responses with status code 1xx, 204, 205, 304 into the Response class.

This is probably the better approach, as users can just set the status code, and the right thing should happen. Also it fits better with fastapi, and its use of JSONResponse by default.

[Kludex]

Why? Is it obvious?

Shouldn't we focus on what is defined?

[Kludex]

I'm extremely happy about the changes here. :)

Thank you @declaresub ! 🎉

This is an important PR, so I'll feel more comfortable having @tomchristie 's approval.

[tomchristie]

This is not at all clear to me.
For instance, while it's related to #1099, it doesn't appear to close it.

Implementation-wise I'd expect any "do we generate a Content-Length header or not" to be handled in the init_headers() method, but let's not start from that point.

Can we take this back to basics first...

Issue #1099 describes things at the API level that I'd like to see:

---

Expected behavior

>>> from starlette.responses import RedirectResponse
>>> RedirectResponse(url="/", status_code=302).headers
MutableHeaders({'content-length': '0', 'location': '/'})

I expect there to be a content-length field specifying zero length

Actual behavior

>>> from starlette.responses import RedirectResponse
>>> RedirectResponse(url="/", status_code=302).headers
MutableHeaders({'location': '/'})

Header has no content-length field.

---

The issue here isn't that we ought to be stripping the Content-Length header in several cases, but rather than we're not adding a correct Content-Length: 0 header in cases where there is no body.

Which is what #1099 describes...

The Content-Length header field is not set for cases where body is empty. In some cases this seems to not be according to the HTTP/1.1 spec and creates troubles when using for example traefik as a proxy in front of a starlette-based backend.

The sensible tack here would be to deal with that first.

I'd expect a pull request there to have a unit test at the API level of eg...

def test_empty_response():
    response = Response()
    assert response.headers["Content-Length"] == "0"

And that the implementation to resolve this would be in init_headers().

Once we have that PR we'd then want to take a look at refining it to deal with the 1xx, 204, and 304 cases.

[declaresub]

I agree that #1099 is not closed here, but I don't think that fixing it in init_headers is the best approach. Indeed, were I to do this, I would start by getting rid of that method. Then I would move the invocation of render from init into call, and set the content-* headers at that time.

[declaresub]

And, at this point, I wonder whether the right thing to do would be to create a new issue that describes the need to set body and content-length appropriately for all combinations of request method and status code, then close the existing issues #1270, #1099, and maybe corresponding pull requests, with a reference to this new issue that describes the goal.

[tomchristie]

@declaresub - I'd like to close this PR and #1178 - an EmptyResponse class isn't the API we're looking for here.

#1099 is a valid issue, and properly described, so let's leave it open.

#1395 is the start of a PR addressing that issue.

[tomchristie]

Thanks for having spent some time looking into this, and for prompting the issue to get addressed.
An EmptyResponse class isn't the API I'd want to see here (also eg. not what we see in flask, django).

Closing in favour of #1397.