Add support for HTTP Range to FileResponse

[Kludex]

• Closes Support for HTTP range requests (for FileResponse) #950

Inspired by Baize and copied from my own implementation: https://github.com/Kludex/file-response .

[abersheeran]

hahaha, I like this.

[trim21]

We should pre-compile regex pattern. Althrough re has a built-in cache, but it has size limit and for project using many regex it still may be invalidated and pattern get re-compiled.

[Kludex]

We should pre-compile regex pattern. Althrough re has a built-in cache, but it has size limit and for project using many regex it still may be invalidated and pattern get re-compiled.

PR welcome. 👍

[graingert]

I'm a bit concerned that this uses predictable randomness

I'd be tempted to go for 19 characters using SystemRandom.choices to get 96 bits of entropy

[trim21]

why join(random_choices) anyway?

It's also slower than secrets.token_hex(6/7/13) unless 13 has some special meaning here.

[Kludex]

Do other frameworks do something different from this?

Would you like to open a PR for it?

[abersheeran]

13 has some special meaning here

Nothing special. It's just a random number I picked, and the spec doesn't make any specific requirements on this length.

[abersheeran]

I'm a bit concerned that this uses predictable randomness

I'd be tempted to go for 19 characters using SystemRandom.choices to get 96 bits of entropy

This is predicted to be secure...it does not serve any cryptographic purpose.

[graingert]

Urllib3 does this https://github.com/urllib3/urllib3/blob/2458bfcd3dacdf6c196e98d077fc6bb02a5fc1df/src/urllib3/filepost.py#L26 (it's for form data but same sort of concept)

nginx is just totally weird https://github.com/nginx/nginx/blob/b1e07409b1071564e8dd8c70436cba5da19a1575/src/core/ngx_file.c#L17 it uses a random number generator based on successive additions of 123456

Apache is bizarre also, it's using predictable randomness, but it's a constant defined each time config is loaded https://github.com/apache/httpd/blob/64deb516d6a3b8d872dab9d27a1f0cba17548190/modules/http/http_core.c#L350

[frostming]

Yes, urllib3 implementation is just equivalent to secrets.token_hex()

[graingert]

Yeah I'd probably go for secrets.token_hex()

[frostming]

Does it send all content within the range?

[Kludex]

I'm not sure I understand the question... Could you rephrase it?

[frostming]

IIUC, this statement may send chunk with self.chunk_size, not the full content between end - start, and I didn't find similar logic to that in handle_single_range(pasted below) to loop until no more body to send：

while more_body:
    chunk = await file.read(min(self.chunk_size, end - start))
    start += len(chunk)
    more_body = len(chunk) == self.chunk_size and start < end
    await send({"type": "http.response.body", "body": chunk, "more_body": more_body})

[Kludex]

I think we may benefit from some comments/docstrings around here. But I don't see anything wrong?

Could you try to come up with a test scenario that you think it would be problematic here?

This is a basic test for multiple ranges (to help):

def test_file_response_merge_ranges(file_response_client: TestClient) -> None:
    response = file_response_client.get("/", headers={"Range": "bytes=0-100, 50-200"})
    assert response.status_code == 206
    assert response.headers["content-length"] == "201"
    assert response.headers["content-range"] == f"bytes 0-200/{len(README.encode('utf8'))}"

[frostming]

Yes, the above test works but needs to adjust the chunk_size to less than 100.

I thought you would immediately see my points. This statement

chunk = await file.read(min(self.chunk_size, end - start))

may not send full content if the chunk_size is smaller, and you didn't send the rest anywhere in this branch.

[Kludex]

Oh... I see your point.

[Kludex]

PR is welcome here, otherwise I'll work on it later.

[Kludex]

Thanks @frostming 🙏