Conversation
📝 WalkthroughWalkthroughMultipart construction is deferred: Changes
Sequence DiagramsequenceDiagram
participant Py as Python Code
participant MW as Multipart (wrapper)
participant PS as Part Storage
participant FB as Form Builder
participant HTTP as HTTP Client
rect rgba(100,150,255,0.5)
Note over Py,FB: Old flow (immediate construction)
Py->>MW: Multipart::new(parts)
MW->>FB: build multipart::Form immediately
FB-->>MW: multipart::Form
MW->>HTTP: send request with form
end
rect rgba(100,255,150,0.5)
Note over Py,FB: New flow (deferred construction)
Py->>MW: Multipart::new(parts)
MW->>PS: store Part metadata & Value
PS-->>MW: parts cached
Py->>MW: reuse Multipart
MW->>FB: FromPyObject triggers on-demand form build
FB->>PS: iterate parts (clone or take values)
FB-->>MW: assembled multipart::Form
MW->>HTTP: send request with form
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the multipart form implementation to support form reuse by storing parts and their values (Text, Bytes, File, or Stream) within the Multipart and Part structs. This allows clonable parts to be reused across multiple requests while maintaining one-shot behavior for streams. Feedback was provided regarding the use of block_on when building file parts, as this can cause panics if called from within an existing Tokio runtime; it is recommended to make the form building process asynchronous.
| Value::File(path) => pyo3_async_runtimes::tokio::get_runtime() | ||
| .block_on(multipart::Part::file(path)) | ||
| .map_err(Error::from)?, |
There was a problem hiding this comment.
Using block_on inside build_inner is risky and can lead to panics if this code is executed on a thread that is already part of a Tokio runtime (e.g., the event loop thread). Since Multipart::extract is called during argument parsing for async functions like execute_request, it is highly likely to be called from such a context.
Consider making build_inner, into_inner, and build_form asynchronous. You can then defer the actual construction of the multipart::Form to the execute_request function in src/client/req.rs, where it can be properly awaited without blocking the thread. This would also involve changing Multipart::extract to only clone the parts and leave the form field as None initially.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/multipart_test.py (1)
80-90: Consider making discarded Multipart explicit.Line 87 creates a
Multipartwhose result is discarded. While this correctly tests the side effect of consuming the stream value, assigning to_would make the intent clearer.Suggested change for clarity
part = Part(name="stream", value=bytes_stream()) - Multipart(part) + _ = Multipart(part) # Consumes the stream value from part with pytest.raises(RuntimeError): Multipart(part)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/multipart_test.py` around lines 80 - 90, The test creates a Multipart from part and discards the result implicitly; make that intent explicit by assigning the created Multipart to a throwaway variable (e.g., _) so it’s clear the value is intentionally discarded — update the invocation Multipart(part) in test_reuse_same_part_without_copy_fails_for_stream_value to use an explicit discard variable while keeping the rest of the test (use the same Part, bytes_stream, and pytest.raises block).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/multipart_test.py`:
- Around line 80-90: The test creates a Multipart from part and discards the
result implicitly; make that intent explicit by assigning the created Multipart
to a throwaway variable (e.g., _) so it’s clear the value is intentionally
discarded — update the invocation Multipart(part) in
test_reuse_same_part_without_copy_fails_for_stream_value to use an explicit
discard variable while keeping the rest of the test (use the same Part,
bytes_stream, and pytest.raises block).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d8287f23-eaff-48d4-bb7a-857fac92a4c4
📒 Files selected for processing (3)
src/client/body/multipart.rssrc/client/req.rstests/multipart_test.py
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/client/body/multipart.rs (1)
141-147: Consider a more descriptive error for exhausted stream values.
Error::Memorymay be confusing when the actual cause is that a stream-based part was already consumed. A dedicated error variant likeError::StreamConsumedorError::ValueExhaustedwould provide clearer diagnostics to users.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client/body/multipart.rs` around lines 141 - 147, The current clone_value_or_take method in multipart.rs returns Error::Memory when a stream-based part is already consumed; add a dedicated error variant (e.g., Error::StreamConsumed or Error::ValueExhausted) to the Error enum, implement its conversion to the PyErr/into() path used elsewhere, and replace the Error::Memory usage in clone_value_or_take (and any other places that semantically represent an exhausted stream) to return the new variant so callers get a clear, descriptive error from functions like clone_value_or_take.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/client/body/multipart.rs`:
- Around line 141-147: The current clone_value_or_take method in multipart.rs
returns Error::Memory when a stream-based part is already consumed; add a
dedicated error variant (e.g., Error::StreamConsumed or Error::ValueExhausted)
to the Error enum, implement its conversion to the PyErr/into() path used
elsewhere, and replace the Error::Memory usage in clone_value_or_take (and any
other places that semantically represent an exhausted stream) to return the new
variant so callers get a clear, descriptive error from functions like
clone_value_or_take.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0aa12ccf-112a-4aa2-b61a-46944c26e987
📒 Files selected for processing (3)
src/client/body/multipart.rssrc/client/req.rstests/multipart_test.py
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/multipart_test.py
1 participant
Summary by CodeRabbit
New Features
Tests