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.

Sign up for GitHub

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

Jump to bottom

Add support for range headers to FileResponse #1999

Closed
wants to merge 1 commit into from
Closed

Add support for range headers to FileResponse #1999

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
28 changes: 24 additions & 4 deletions starlette/responses.py
View file
Open in desktop
Expand Up @@ -290,9 +290,11 @@ def __init__(
stat_result: typing.Optional[os.stat_result] = None,
method: typing.Optional[str] = None,
content_disposition_type: str = "attachment",
range: typing.Optional[typing.Tuple[int, int]] = None,
) -> None:
self.path = path
self.status_code = status_code
self.status_code = status_code if range is None else 206
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.

The status code is ignored in case of range being sent.

This doesn't look cool.

Copy link

Choose a reason for hiding this comment

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

Can you change status_code: int = 200 to status_code: Optional[int] = None then here set the default to 200 if there is no range and 206 if there is a range? If it's explicitly set to something other than 206 and there is a range then throw an error.

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.

This would make the FileResponse status_code type differ from the other Response classes.

But indeed, I can do the following instead:

        self.status_code = status_code
        self.range = range
        if self.range is not None and self.status_code != 206:
            raise RuntimeError("Range requests must have a 206 status code.")

Copy link

Choose a reason for hiding this comment

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

Well the Response.status_code type would always be an integer still, the default value is just inferred and None is used in to signal that it was not passed explicitly.

Throwing an exception seems much better than ignoring the code though :)

self.range = range
self.filename = filename
self.send_header_only = method is not None and method.upper() == "HEAD"
if media_type is None:
Expand All @@ -316,9 +318,17 @@ def __init__(
self.set_stat_headers(stat_result)

def set_stat_headers(self, stat_result: os.stat_result) -> None:
content_length = str(stat_result.st_size)
size = str(stat_result.st_size)
last_modified = formatdate(stat_result.st_mtime, usegmt=True)
etag_base = str(stat_result.st_mtime) + "-" + str(stat_result.st_size)
if self.range is not None:
start, end = self.range
etag_base += f"-{start}/{end}"
content_length = str(end - start + 1)
self.headers.setdefault("accept-ranges", "bytes")
self.headers.setdefault("content-range", f"bytes {start}-{end}/{size}")
else:
content_length = size
etag = md5_hexdigest(etag_base.encode(), usedforsecurity=False)

self.headers.setdefault("content-length", content_length)
Expand All @@ -336,6 +346,8 @@ async def __call__(self, scope: Scope, receive: Receive, send: Send) -> None:
mode = stat_result.st_mode
if not stat.S_ISREG(mode):
raise RuntimeError(f"File at path {self.path} is not a file.")
else:
stat_result = self.stat_result
await send(
{
"type": "http.response.start",
Expand All @@ -347,10 +359,18 @@ async def __call__(self, scope: Scope, receive: Receive, send: Send) -> None:
await send({"type": "http.response.body", "body": b"", "more_body": False})
else:
async with await anyio.open_file(self.path, mode="rb") as file:
if self.range is not None:
start, end = self.range
await file.seek(start)
else:
start, end = 0, stat_result.st_size - 1
remaining_bytes = end - start + 1
more_body = True
while more_body:
chunk = await file.read(self.chunk_size)
more_body = len(chunk) == self.chunk_size
chunk_size = min(remaining_bytes, self.chunk_size)
chunk = await file.read(chunk_size)
remaining_bytes -= len(chunk)
more_body = remaining_bytes > 0 and len(chunk) == chunk_size
await send(
{
"type": "http.response.body",
Expand Down
26 changes: 26 additions & 0 deletions tests/test_responses.py
View file
Open in desktop
Expand Up @@ -5,6 +5,7 @@

from starlette import status
from starlette.background import BackgroundTask
from starlette.datastructures import Headers
from starlette.requests import Request
from starlette.responses import (
FileResponse,
Expand Down Expand Up @@ -241,6 +242,31 @@ async def app(scope, receive, send):
assert filled_by_bg_task == "6, 7, 8, 9"


def test_file_response_with_range(tmpdir, test_client_factory):
path = os.path.join(tmpdir, "xyz")
content = b"<file content>"
with open(path, "wb") as file:
file.write(content)

async def app(scope, receive, send):
range_header = Headers(scope=scope)["range"]
start, end = (int(v) for v in range_header[len("bytes=") :].split("-"))
response = FileResponse(path=path, filename="example.png", range=(start, end))
Comment on lines +252 to +254
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.

This is what a developer would need to do by its own with this solution.

It doesn't look good.

Copy link

@zanieb zanieb Jan 7, 2023 •
edited

Choose a reason for hiding this comment

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

What about something like... FileResponse.with_range_from_headers(path=path, filename=..., headers=Headers(scope=scope))? I agree this is awkward as-is.

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.

Hmm... No... But you actually gave me an idea:

range_header = Headers(scope=scope)["range"]
FileResponse(path=path, filename=..., range_header=range_header)

Still... I'm not comfortable yet... 🤔

await response(scope, receive, send)

client = test_client_factory(app)
response = client.get("/", headers={"range": "bytes=1-12"})
expected_disposition = 'attachment; filename="example.png"'
assert response.status_code == status.HTTP_206_PARTIAL_CONTENT
assert response.content == content[1:13]
assert response.headers["content-type"] == "image/png"
assert response.headers["content-disposition"] == expected_disposition
assert response.headers["content-range"] == "bytes 1-12/14"
assert "content-length" in response.headers
assert "last-modified" in response.headers
assert "etag" in response.headers


def test_file_response_with_directory_raises_error(tmpdir, test_client_factory):
app = FileResponse(path=tmpdir, filename="example.png")
client = test_client_factory(app)
Expand Down