Avoid mutating FileResponse headers on range requests#3144
Merged
Conversation
FileResponse mutates self.headers when handling range requests, making the instance unusable for subsequent calls. Copy raw_headers into a local MutableHeaders before setting content-range, content-type, and content-length.
Replace confusing parametrized test that calls other test functions with a straightforward range-then-full-request assertion.
FileResponse headers on range requests
Contributor
|
I added the parameterized test, because some combinations already worked and some didn't. It exercises all 9 combinations of all three possible code paths (no range, simple range, multiple ranges). I agree that the simplified test is easier to follow, but it doesn't provide the coverage of the parameterized test. |
Owner
Author
|
Hmm... Which ones didn't work? Can we add only the ones that were failing in main? |
Contributor
|
Checking out my original commit, then discarding my changes to
(The order is backwards in the name of the test, because I should have switched the order of the decorators.) $ git checkout 6bf294ba80174145ba459f6a336641516a279838
$ git checkout HEAD^ -- starlette/responses.py
$ uv run pytest -k test_file_response_multiple_calls -rf
...
=========================== short test summary info ============================
FAILED tests/test_responses.py::test_file_response_multiple_calls[asyncio-test_file_response_without_range-test_file_response_range] - assert 'content-range' not in Headers({'content-type': 'text/plain; charset...
FAILED tests/test_responses.py::test_file_response_multiple_calls[asyncio-test_file_response_without_range-test_file_response_range_multi] - AssertionError: assert '448' == '526'
FAILED tests/test_responses.py::test_file_response_multiple_calls[asyncio-test_file_response_range-test_file_response_range_multi] - AssertionError: assert 'multipart/by...ce7954c8c33f9' == 'text/plain; chars...
FAILED tests/test_responses.py::test_file_response_multiple_calls[asyncio-test_file_response_range_multi-test_file_response_range] - assert 'content-range' not in Headers({'content-type': 'multipart/byterange...
FAILED tests/test_responses.py::test_file_response_multiple_calls[asyncio-test_file_response_range_multi-test_file_response_range_multi] - AssertionError: assert '512' == '448'
FAILED tests/test_responses.py::test_file_response_multiple_calls[trio-test_file_response_without_range-test_file_response_range] - assert 'content-range' not in Headers({'content-type': 'text/plain; charset...
FAILED tests/test_responses.py::test_file_response_multiple_calls[trio-test_file_response_without_range-test_file_response_range_multi] - AssertionError: assert '448' == '526'
FAILED tests/test_responses.py::test_file_response_multiple_calls[trio-test_file_response_range-test_file_response_range_multi] - AssertionError: assert 'multipart/by...fc75ee3b0533e' == 'text/plain; chars...
FAILED tests/test_responses.py::test_file_response_multiple_calls[trio-test_file_response_range_multi-test_file_response_range] - assert 'content-range' not in Headers({'content-type': 'multipart/byterange...
FAILED tests/test_responses.py::test_file_response_multiple_calls[trio-test_file_response_range_multi-test_file_response_range_multi] - AssertionError: assert '512' == '448'
================= 10 failed, 8 passed, 945 deselected in 0.49s =================It's possible, of course, to just add those 5 cases. Alternatively, I've attached a patch on top of main that would restore the parametrized test, but with clearer |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
FileResponsemutatesself.headerswhen handling range requests, making the instance unusable for subsequent calls. Copyraw_headersinto a localMutableHeadersbefore settingcontent-range,content-type, andcontent-length.Credit to @viccie30.