Demo pr #11
Closed
Demo pr #11
Conversation
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
WalkthroughThe pull request introduces a new feature for handling multipart file uploads by allowing additional headers for each file part, excluding "Content-Type". The `MultipartField` class and its constructor are updated to support these headers, and a `ValueError` is raised if "Content-Type" is included. The `FileTypes` type alias is updated to a 4-tuple format. New tests are added to verify the correct handling of headers and ensure constraints are enforced.Changes
Sequence DiagramsequenceDiagram
participant Client
participant MultipartField
participant Tests
Client->>MultipartField: Create with file and headers
MultipartField->>MultipartField: Validate headers
alt Headers include "Content-Type"
MultipartField-->>Client: Raise ValueError
else
MultipartField-->>Client: Accept headers
end
Tests->>MultipartField: Run tests for header handling
MultipartField-->>Tests: Return results
Tests-->>Client: Verify correct behavior
|
![deeployed-peer-dev[bot]](https://avatars.githubusercontent.com/in/1074077?s=60&v=4){kind=small}
There was a problem hiding this comment.
Commits
Files that changed from the base of the PR and between Base and 807cc83 commits: 3Files Selected For Review: 3
- httpx/_multipart.py with diff length 2384.
- httpx/_types.py with diff length 378.
- tests/test_multipart.py with diff length 2099.
Review comments generated: 5
- Review: 1
- LGTM: 4
- Identified issue already discussed: 0
Tips
Chat with Deeployed Bot (@reviewed-deeployed)
- Reply on review comments left by this bot to ask follow-up questions. A review comment is a comment on a diff or a file.
- Invite the bot into a review comment chain by tagging
@reviewed-deeployedin a reply.
Code suggestions
- The bot may make code suggestions, but please review them carefully before committing since the line number ranges may be misaligned.
- You can edit the comment made by the bot and manually tweak the suggestion if it is slightly off.
Pausing incremental reviews
- Add
reviewed-deeployed: ignoreanywhere in the PR description to pause further reviews from the bot.
Comment on lines
+123
to
+141
| @pytest.mark.parametrize("content_type", [None, "text/plain"]) | ||
| @pytest.mark.parametrize( | ||
| "headers", | ||
| [ | ||
| {"content-type": "text/plain"}, | ||
| {"Content-Type": "text/plain"}, | ||
| {"CONTENT-TYPE": "text/plain"}, | ||
| ], | ||
| ) | ||
| def test_multipart_headers_include_content_type( | ||
| content_type: typing.Optional[str], headers: typing.Dict[str, str] | ||
| ) -> None: | ||
| """Including contet-type in the multipart headers should not be allowed""" | ||
| client = httpx.Client(transport=httpx.MockTransport(echo_request_content)) | ||
|
|
||
| files = {"file": ("test.txt", b"content", content_type, headers)} | ||
| pat = "Content-Type cannot be included in multipart headers" | ||
| with pytest.raises(ValueError, match=pat): | ||
| client.post("http://127.0.0.1:8000/", files=files) |
There was a problem hiding this comment.
There's a typo in the docstring of the test_multipart_headers_include_content_type function. The word "contet-type" should be corrected to "content-type".
"""Including contet-type in the multipart headers should not be allowed"""
+ """Including content-type in the multipart headers should not be allowed"""
Collaborator
atakanserifoglu
left a comment
atakanserifoglu
left a comment
There was a problem hiding this comment.
✅ Conditional approval has been given. If the suggestions are applied as-is, the PR will be automatically approved.
httpx/_multipart.py
Outdated
| 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() |
Collaborator
There was a problem hiding this comment.
check should be case insensitive to header name
Suggested change
| key, val = f"\r\n{header_name}: ".encode(), header_value.encode() | |
| key, val = f"\r\n{header_name.lower()}: ".encode(), header_value.encode() |
[cond-approve] Suggestions from @atakanserifoglu
atakanserifoglu
approved these changes
Jul 30, 2025
Collaborator
atakanserifoglu
left a comment
atakanserifoglu
left a comment
There was a problem hiding this comment.
Auto-approved by GitHub App after suggestions from @atakanserifoglu were merged.
There was a problem hiding this comment.
Files Selected For Review: 6
- httpx/_multipart.py with diff length 2393.
- httpx/_types.py with diff length 378.
- tests/test_multipart.py with diff length 2099.
- httpx/_multipart.py with diff length 2393.
- httpx/_types.py with diff length 378.
- tests/test_multipart.py with diff length 2099.
Review comments generated: 12
- Review: 4
- LGTM: 8
- Identified issue already discussed: 0
Tips
Chat with Deeployed Bot (@reviewed-deeployed)
- Reply on review comments left by this bot to ask follow-up questions. A review comment is a comment on a diff or a file.
- Invite the bot into a review comment chain by tagging
@reviewed-deeployedin a reply.
Code suggestions
- The bot may make code suggestions, but please review them carefully before committing since the line number ranges may be misaligned.
- You can edit the comment made by the bot and manually tweak the suggestion if it is slightly off.
Pausing incremental reviews
- Add
reviewed-deeployed: ignoreanywhere in the PR description to pause further reviews from the bot.
Comment on lines
+123
to
+141
| @pytest.mark.parametrize("content_type", [None, "text/plain"]) | ||
| @pytest.mark.parametrize( | ||
| "headers", | ||
| [ | ||
| {"content-type": "text/plain"}, | ||
| {"Content-Type": "text/plain"}, | ||
| {"CONTENT-TYPE": "text/plain"}, | ||
| ], | ||
| ) | ||
| def test_multipart_headers_include_content_type( | ||
| content_type: typing.Optional[str], headers: typing.Dict[str, str] | ||
| ) -> None: | ||
| """Including contet-type in the multipart headers should not be allowed""" | ||
| client = httpx.Client(transport=httpx.MockTransport(echo_request_content)) | ||
|
|
||
| files = {"file": ("test.txt", b"content", content_type, headers)} | ||
| pat = "Content-Type cannot be included in multipart headers" | ||
| with pytest.raises(ValueError, match=pat): | ||
| client.post("http://127.0.0.1:8000/", files=files) |
There was a problem hiding this comment.
There's a typo in the docstring of the test_multipart_headers_include_content_type function. The word "contet-type" should be corrected to "content-type".
"""Including contet-type in the multipart headers should not be allowed"""
+ """Including content-type in the multipart headers should not be allowed"""
Comment on lines
82
to
+95
| 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.
The logic for handling the tuple unpacking and the subsequent checks for headers is correct. However, the indentation for the else block seems off. It should align with the try block to ensure proper execution flow.
filename, fileobj, content_type, headers = value # type: ignore
except ValueError:
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"
)
Comment on lines
136
to
144
| 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.lower()}: ".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.
The conversion of header names to lowercase before encoding is correct, but it might be more efficient to perform this operation once when setting the headers, rather than during each call to this method. Consider adjusting the logic to handle this earlier in the process.
for header_name, header_value in self.headers.items():
key, val = f"\r\n{header_name.lower()}: ".encode(), header_value.encode()
parts.extend([key, val])
Comment on lines
+123
to
+141
| @pytest.mark.parametrize("content_type", [None, "text/plain"]) | ||
| @pytest.mark.parametrize( | ||
| "headers", | ||
| [ | ||
| {"content-type": "text/plain"}, | ||
| {"Content-Type": "text/plain"}, | ||
| {"CONTENT-TYPE": "text/plain"}, | ||
| ], | ||
| ) | ||
| def test_multipart_headers_include_content_type( | ||
| content_type: typing.Optional[str], headers: typing.Dict[str, str] | ||
| ) -> None: | ||
| """Including contet-type in the multipart headers should not be allowed""" | ||
| client = httpx.Client(transport=httpx.MockTransport(echo_request_content)) | ||
|
|
||
| files = {"file": ("test.txt", b"content", content_type, headers)} | ||
| pat = "Content-Type cannot be included in multipart headers" | ||
| with pytest.raises(ValueError, match=pat): | ||
| client.post("http://127.0.0.1:8000/", files=files) |
There was a problem hiding this comment.
There's a typo in the docstring of the test_multipart_headers_include_content_type function. The word "contet-type" should be corrected to "content-type".
"""Including contet-type in the multipart headers should not be allowed"""
+ """Including content-type in the multipart headers should not be allowed"""
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 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.
👍 Conditional Approval Checklist
Summary by Reviewed
New Feature
Test
ValueError.These changes improve the flexibility and robustness of multipart file uploads by allowing more granular control over headers, while ensuring compliance with header constraints.