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.
The status code is ignored in case of range being sent.
This doesn't look cool.
There was a problem hiding this comment.
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.
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.
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.
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.
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.
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. |
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-Rangein case the client sentRange.I believe this alternative is cleaner than the implemented here.
References