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
Allow custom headers in multipart/form-data requests #1936
Changes from 9 commits
b6a4c5b
01fc2fc
79fcd72
08f96bf
9421685
a43ffe7
cbb9f05
0a913b9
c55188d
8e04a40
6fe8c06
ec474fd
79d1521
195c661
1c9dc26
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -78,23 +78,39 @@ def __init__(self, name: str, value: FileTypes) -> None: | |
|
||
fileobj: FileContent | ||
|
||
headers: typing.Dict[str, str] = {} | ||
content_type: typing.Optional[str] = None | ||
|
||
if isinstance(value, tuple): | ||
try: | ||
filename, fileobj, content_type = value # type: ignore | ||
filename, fileobj, content_type, headers = value # type: ignore | ||
except ValueError: | ||
filename, fileobj = value # type: ignore | ||
content_type = guess_content_type(filename) | ||
try: | ||
filename, fileobj, content_type = value # type: ignore | ||
except ValueError: | ||
filename, fileobj = value # type: ignore | ||
else: | ||
headers = {k.title(): v for k, v in headers.items()} | ||
if "Content-Type" in headers: | ||
raise ValueError( | ||
"Content-Type cannot be included in multipart headers" | ||
) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Perhaps we don't need to do this check. It's odd behaviour for the developer to set the My preference would be that we don't do the explicit check here. In the case of conflicts I'd probably have I'm not absolute on this one, but slight preference. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Makes sense. I thought it'd be a good idea to check what requests does here. It looks like it silently ignores the header in the requests.post("http://example.com", files=[("test", ("test_filename", b"data", "text/plain", {"Content-Type": "text/csv"}))]) Gets sent as Digging into why this is the case, it seems like it's just an implementation detail in urllib3. It happens here. I'm not sure what the right thing to do here is, but if you feel like it's best to go with no error and making Another alternative would be to have the 3rd parameter be either a string representing the content type or a headers dict. We can't really make the 3rd parameter always be a headers dict because that would be a breaking change for httpx. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I reckon let's do that, yeah.
I actually quite like that yes, neat idea. The big-tuples API is... not helpful really. But let's probably just go with the path of least resistance here. Perhaps one day we'll want an There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
👍 donzo
Agreed! I added a comment in the code explaining the reasoning behind the big tuple API (inherited from requests) and how we might want to change it in the future. |
||
else: | ||
filename = Path(str(getattr(value, "name", "upload"))).name | ||
fileobj = value | ||
|
||
if content_type is None: | ||
content_type = guess_content_type(filename) | ||
|
||
if content_type is not None: | ||
headers["Content-Type"] = content_type | ||
|
||
if isinstance(fileobj, (str, io.StringIO)): | ||
raise TypeError(f"Expected bytes or bytes-like object got: {type(fileobj)}") | ||
|
||
self.filename = filename | ||
self.file = fileobj | ||
self.content_type = content_type | ||
self.headers = headers | ||
self._consumed = False | ||
|
||
def get_length(self) -> int: | ||
|
@@ -122,9 +138,9 @@ def render_headers(self) -> bytes: | |
if self.filename: | ||
filename = format_form_param("filename", self.filename) | ||
parts.extend([b"; ", filename]) | ||
if self.content_type is not None: | ||
content_type = self.content_type.encode() | ||
parts.extend([b"\r\nContent-Type: ", content_type]) | ||
for header_name, header_value in self.headers.items(): | ||
key, val = f"\r\n{header_name}: ".encode(), header_value.encode() | ||
parts.extend([key, val]) | ||
parts.append(b"\r\n\r\n") | ||
self._headers = b"".join(parts) | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we should
.title()
case here.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah... I see the comparison case. Huh. Fiddly.