-
-
Notifications
You must be signed in to change notification settings - Fork 903
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
Add support for range headers to FileResponse
#1999
Conversation
This PR aims to solve #950. The implementation differs from [baize's FileResponse](https://github.com/abersheeran/baize/blob/23791841f30ca92775e50a544a8606d1d4deac93/baize/asgi/responses.py#L184), since that one takes in consideration the "range" request header. The desgin decision is justified as the Response classes in Starlette are naive in regards to the request.
) -> None: | ||
self.path = path | ||
self.status_code = status_code | ||
self.status_code = status_code if range is None else 206 |
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.
The status code is ignored in case of range being sent.
This doesn't look cool.
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.
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.
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.
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.")
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.
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 :)
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)) |
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.
This is what a developer would need to do by its own with this solution.
It doesn't look good.
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.
What about something like... FileResponse.with_range_from_headers(path=path, filename=..., headers=Headers(scope=scope))
? I agree this is awkward as-is.
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.
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... 🤔
Let's focus on 1.0 for now. |
If this is revisited someday, two embellishments to consider:
|
The implementation differs from baize's FileResponse, since that one takes in consideration the "range" request header. This is because the Response classes in Starlette are naive in regard to the request.
Alternative Solution
Don't be naive in regard to the request, and send a
Content-Range
in case the client sentRange
.I believe this alternative is cleaner than the implemented here.
References