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

More robust check for upload files in binary mode #2630

Merged
merged 7 commits into from
Apr 20, 2023

Conversation

lkuchenb
Copy link
Contributor

This is a minor patch proposal fixing fixing an issue with a recently introduced check for binary file objects in multipart uploads. The currently used check

httpx/httpx/_multipart.py

Lines 125 to 128 in f1157db

if "b" not in getattr(fileobj, "mode", "b"):
raise TypeError(
"Multipart file uploads must be opened in binary mode, not text mode."
)

misses out on file objects that read and write byte-like objects, but have the mode attribute set to r such as ZipFile.open() which is binary-only.

@lkuchenb
Copy link
Contributor Author

The StringIO check is now somewhat obsolete, I've changed the order so that the more specific test comes first, but they could probably also be merged into one with a meaningful combined message.

@tomchristie
Copy link
Member

do we think it's worth adding an extra test for this case or we happy to accept the pr as-is?

@tomchristie tomchristie changed the title Fix check for binary mode More robust check for upload files in binary mode Apr 20, 2023
@tomchristie tomchristie merged commit 472597f into encode:master Apr 20, 2023
@tomchristie
Copy link
Member

Thanks!

@lkuchenb
Copy link
Contributor Author

Thanks for the review @tomchristie - I didn't think covering this with a test would make much sense since the test would involve intentionally making false assumptions about standard library behavior, not something one would typically want to test IMHO.

@lkuchenb lkuchenb deleted the fix/multipart_filemode_check branch May 10, 2023 14:42
@epenet epenet mentioned this pull request May 17, 2023
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.

2 participants