Skip to content
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

Merged
merged 7 commits into from Jan 7, 2022
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
6 changes: 3 additions & 3 deletions starlette/responses.py
Expand Up @@ -70,8 +70,8 @@ def init_headers(self, headers: typing.Mapping[str, str] = None) -> None:
populate_content_length = b"content-length" not in keys
populate_content_type = b"content-type" not in keys

body = getattr(self, "body", b"")
if body and populate_content_length:
if populate_content_length:
body = getattr(self, "body", b"")
content_length = str(len(body))
raw_headers.append((b"content-length", content_length.encode("latin-1")))

Expand Down Expand Up @@ -289,7 +289,7 @@ def set_stat_headers(self, stat_result: os.stat_result) -> None:
etag_base = str(stat_result.st_mtime) + "-" + str(stat_result.st_size)
etag = hashlib.md5(etag_base.encode()).hexdigest()

self.headers.setdefault("content-length", content_length)
self.headers["content-length"] = content_length
Copy link
Sponsor Member Author

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 the init_headers().

Copy link
Member

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 and FileResponse won't yet pass, but let's look at how we should resolve them once we've identified the test failures fully.

self.headers.setdefault("last-modified", last_modified)
self.headers.setdefault("etag", etag)

Expand Down
2 changes: 1 addition & 1 deletion starlette/staticfiles.py
Expand Up @@ -100,7 +100,7 @@ async def __call__(self, scope: Scope, receive: Receive, send: Send) -> None:
def get_path(self, scope: Scope) -> str:
"""
Given the ASGI scope, return the `path` string to serve up,
with OS specific path seperators, and any '..', '.' components removed.
with OS specific path separators, and any '..', '.' components removed.
"""
return os.path.normpath(os.path.join(*scope["path"].split("/")))

Expand Down
5 changes: 5 additions & 0 deletions tests/test_responses.py
Expand Up @@ -309,3 +309,8 @@ def test_head_method(test_client_factory):
client = test_client_factory(app)
response = client.head("/")
assert response.text == ""


def test_empty_response():
Copy link
Member

Choose a reason for hiding this comment

The 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?...

  • 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

Copy link
Sponsor Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When do we have a FileResponse with unknown size?

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