allow setting an explicit multipart boundary via headers #2278
Conversation
tests/test_multipart.py
Outdated
| @pytest.mark.parametrize( | ||
| "header", | ||
| [ | ||
| "multipart/form-data; charset=utf-8", | ||
| "multipart/form-data; charset=utf-8; ", | ||
| ], | ||
| ) | ||
| def test_multipart_header_without_boundary(header: str) -> None: | ||
| client = httpx.Client(transport=httpx.MockTransport(echo_request_content)) | ||
|
|
||
| files = {"file": io.BytesIO(b"<file content>")} | ||
| headers = {"content-type": header} | ||
| response = client.post("http://127.0.0.1:8000/", files=files, headers=headers) | ||
| assert response.status_code == 200 | ||
|
|
||
| # We're using the cgi module to verify the behavior here, which is a | ||
| # bit grungy, but sufficient just for our testing purposes. | ||
| boundary = response.request.headers["Content-Type"].split("boundary=")[-1] | ||
| content_length = response.request.headers["Content-Length"] | ||
| pdict: dict = { | ||
| "boundary": boundary.encode("ascii"), | ||
| "CONTENT-LENGTH": content_length, | ||
| } | ||
| multipart = cgi.parse_multipart(io.BytesIO(response.content), pdict) | ||
|
|
||
| assert multipart["file"] == [b"<file content>"] |
There was a problem hiding this comment.
This test is fails on main/master as well, but I think it would be important to fix in this PR. What needs to happen is we need to create the random boundary and then insert that into the existing header. The issue is that there is currently no machinery / precedent for encode_request() modifying headers, so it would take a bit more refactoring to get working. Alternatively, we can raise an error if a user gives us a content-type: multipart/form-data without an explicit header?
httpx/_content.py
Outdated
| @@ -187,6 +198,7 @@ def encode_request( | |||
| files: Optional[RequestFiles] = None, | |||
| json: Optional[Any] = None, | |||
| boundary: Optional[bytes] = None, | |||
| headers: Optional[Mapping[str, str]] = None, | |||
There was a problem hiding this comment.
How about we keep things a little tighter here by passing content_type: str = None instead? That restricts the information we're passing around to the one thing we're actually interested in.
httpx/_multipart.py
Outdated
| for section in content_type.split(";"): | ||
| if section.strip().startswith("boundary="): | ||
| return section.strip().split("boundary=")[-1].encode("latin-1") | ||
| raise ValueError("Missing boundary in multipart/form-data content-type header") |
There was a problem hiding this comment.
Instead of raising ValueError we could just return None if the content type doesn't start with "multipart/form-data", or doesn't include a valid boundary.
(Users can currently submit requests with Content-Type headers that don't properly match up to the data they include, so just being lax here seems okay to me.)
There was a problem hiding this comment.
Note that since content-type gets set via setdefault. If it already exists, it won't be overwritten. So in the case where the user provided the content-type header but did not provide the multipart boundary we'd be sending out a request with no multipart boundary, which completely violates the spec and no server will be able to parse.
There was a problem hiding this comment.
So in the case where the user provided the content-type header but did not provide the multipart boundary we'd be sending out a request with no multipart boundary, which completely violates the spec and no server will be able to parse.
True. We could either hard-error in that case, or just ignore it and allow users to do that.
There is also a whole class of cases here where a user can set a Content-Type that doesn't match the encoding type they're using with data=/files=/json=. (Eg. they can set Content-Type: application/json on either form or multipart requests.) But that's probably okay.
|
@tomchristie this is ready for another review whenever you get a chance |
Co-authored-by: Jean Hominal <jhominal@gmail.com>
Thank you for your thorough and timely reviews! |
Closes #2235