-
-
Notifications
You must be signed in to change notification settings - Fork 978
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Don't omit Content-Length
header for Content-Length: 0
cases
#1395
Changes from 4 commits
d8fcbfe
cb9369f
9e5239d
6f9631c
799b016
b04a7b4
d0f48a7
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -13,6 +13,7 @@ | |
Response, | ||
StreamingResponse, | ||
) | ||
from starlette.testclient import TestClient | ||
|
||
|
||
def test_text_response(test_client_factory): | ||
|
@@ -73,6 +74,20 @@ async def app(scope, receive, send): | |
assert response.url == "http://testserver/I%20%E2%99%A5%20Starlette/" | ||
|
||
|
||
def test_redirect_response_content_length_header(test_client_factory): | ||
async def app(scope, receive, send): | ||
if scope["path"] == "/": | ||
response = Response("hello", media_type="text/plain") # pragma: nocover | ||
else: | ||
response = RedirectResponse("/") | ||
await response(scope, receive, send) | ||
|
||
client: TestClient = test_client_factory(app) | ||
response = client.request("GET", "/redirect", allow_redirects=False) | ||
assert response.url == "http://testserver/redirect" | ||
assert response.headers["content-length"] == "0" | ||
|
||
|
||
def test_streaming_response(test_client_factory): | ||
filled_by_bg_task = "" | ||
|
||
|
@@ -309,3 +324,8 @@ def test_head_method(test_client_factory): | |
client = test_client_factory(app) | ||
response = client.head("/") | ||
assert response.text == "" | ||
|
||
|
||
def test_empty_response(): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is the test case that we should probably expand to cover the 6 different cases described. I guess we want these test cases?...
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. When do we have a |
||
response = Response() | ||
assert response.headers["Content-Length"] == "0" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are sure that
content-length
exists at this point, considering the change of conditional in theinit_headers()
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
andFileResponse
won't yet pass, but let's look at how we should resolve them once we've identified the test failures fully.