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.

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

Always rewind files on multipart uploads. #2065

Merged
merged 4 commits into from
Feb 4, 2022

Conversation

tomchristie
Copy link
Member

@tomchristie tomchristie commented Feb 4, 2022

Raised in #2064

  • Failing test case.
  • Implement rewind fix.

@tomchristie
Copy link
Member Author

tomchristie commented Feb 4, 2022

Waiting on a release of httpcore with encode/httpcore#502 in order to fix the test suite.
Once we've got that I'll re-run the suite to demo that the test currently fails, and then push the fix.

Edit Okay, done.

Test suite prior to the fix fails with this...

=================================== FAILURES ===================================
[50](https://github.com/encode/httpx/runs/5066317760?check_suite_focus=true#step:7:50)
_________________________ test_multipart_rewinds_files _________________________
[51](https://github.com/encode/httpx/runs/5066317760?check_suite_focus=true#step:7:51)

[52](https://github.com/encode/httpx/runs/5066317760?check_suite_focus=true#step:7:52)
    def test_multipart_rewinds_files():
[53](https://github.com/encode/httpx/runs/5066317760?check_suite_focus=true#step:7:53)
        with tempfile.TemporaryFile() as upload:
[54](https://github.com/encode/httpx/runs/5066317760?check_suite_focus=true#step:7:54)
            upload.write(b"Hello, world!")
[55](https://github.com/encode/httpx/runs/5066317760?check_suite_focus=true#step:7:55)
    
[56](https://github.com/encode/httpx/runs/5066317760?check_suite_focus=true#step:7:56)
            transport = httpx.MockTransport(echo_request_content)
[57](https://github.com/encode/httpx/runs/5066317760?check_suite_focus=true#step:7:57)
            client = httpx.Client(transport=transport)
[58](https://github.com/encode/httpx/runs/5066317760?check_suite_focus=true#step:7:58)
    
[59](https://github.com/encode/httpx/runs/5066317760?check_suite_focus=true#step:7:59)
            files = {"file": upload}
[60](https://github.com/encode/httpx/runs/5066317760?check_suite_focus=true#step:7:60)
            response = client.post("http://127.0.0.1:8000/", files=files)
[61](https://github.com/encode/httpx/runs/5066317760?check_suite_focus=true#step:7:61)
            assert response.status_code == 200
[62](https://github.com/encode/httpx/runs/5066317760?check_suite_focus=true#step:7:62)
>           assert b"\r\nHello, world!\r\n" in response.content
[63](https://github.com/encode/httpx/runs/5066317760?check_suite_focus=true#step:7:63)
E           assert b'\r\nHello, world!\r\n' in b'--[64](https://github.com/encode/httpx/runs/5066317760?check_suite_focus=true#step:7:64)703278012ba578d5eea2ebfafab695\r\nContent-Disposition: form-data; name="file"; filename="20"\r\nContent-Type: application/octet-stream\r\n\r\n\r\n--64703278012ba578d5eea2ebfafab695--\r\n'
64
E            +  where b'--64703278012ba578d5eea2ebfafab695\r\nContent-Disposition: form-data; name="file"; filename="20"\r\nContent-Type: application/octet-stream\r\n\r\n\r\n--64703278012ba578d5eea2ebfafab695--\r\n' = <Response [200 OK]>.content

@@ -158,9 +157,8 @@ def render_data(self) -> typing.Iterator[bytes]:
yield self._data
return

if self._consumed: # pragma: nocover
if hasattr(self.file, "seek"):
Copy link
Member

Choose a reason for hiding this comment

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

This is more of a question, so we do seek for the first file upload too, right?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yup

@ShivangKakkar
Copy link

It works like charm now! Thanks

@tomchristie tomchristie merged commit 0088253 into master Feb 4, 2022
@tomchristie tomchristie deleted the always-rewind-files-on-multipart-uploads branch February 4, 2022 14:48
@tomchristie tomchristie mentioned this pull request May 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants