fix: detect original_url before operator guard to fix encoding with explicit operators#678
Conversation
…xplicit operators When callers pass an explicit operator (e.g. after resolving via operator_for_path()), the original_url detection was skipped because it was nested inside `if operator is None`. This caused HTTP URLs to go through OpenDAL presign_read, mangling percent-encoded paths. Move the URL check before the guard in all three methods. Assisted-by: claude-opus-4.6 Signed-off-by: Benny Zlotnik <bzlotnik@redhat.com>
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 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 |
bennyz
added
the
build-pr-images/jumpstarter
Container ImagesThe following container images have been built for this PR:
Images expire after 7 days. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
python/packages/jumpstarter-driver-opendal/jumpstarter_driver_opendal/driver_test.py (1)
386-393: ⚡ Quick winConsider adding test coverage for StorageMuxFlasherClient.flash.
The fix in
client.py(lines 775-776) applies the same pattern toStorageMuxFlasherClient.flash, but there's no explicit test validating that this method also preserves encoding when given an explicit operator. While the logic is identical toFlasherClient._flash_single, adding a test would increase confidence and prevent regressions.📝 Suggested test to add
def test_storage_mux_flasher_http_with_explicit_operator(): """StorageMuxFlasherClient.flash must use original_url bypass even when operator is passed explicitly.""" with serve(MockStorageMuxFlasher()) as flasher: with _http_path_recording_server() as (port, received_paths): url = f"http://127.0.0.1:{port}/path%40encoded/file.bin" explicit_operator = Operator("http", endpoint=f"http://127.0.0.1:{port}") flasher.flash(url, operator=explicit_operator) _assert_encoding_preserved(received_paths)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@python/packages/jumpstarter-driver-opendal/jumpstarter_driver_opendal/driver_test.py` around lines 386 - 393, Add a new test mirroring test_flash_http_with_explicit_operator but exercising StorageMuxFlasherClient.flash: create a test named test_storage_mux_flasher_http_with_explicit_operator that uses serve(MockStorageMuxFlasher()) and _http_path_recording_server(), constructs url = f"http://127.0.0.1:{port}/path%40encoded/file.bin", creates explicit_operator = Operator("http", endpoint=f"http://127.0.0.1:{port}"), calls flasher.flash(url, operator=explicit_operator) and then calls _assert_encoding_preserved(received_paths) to verify encoding is preserved for StorageMuxFlasherClient.flash.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In
`@python/packages/jumpstarter-driver-opendal/jumpstarter_driver_opendal/driver_test.py`:
- Around line 386-393: Add a new test mirroring
test_flash_http_with_explicit_operator but exercising
StorageMuxFlasherClient.flash: create a test named
test_storage_mux_flasher_http_with_explicit_operator that uses
serve(MockStorageMuxFlasher()) and _http_path_recording_server(), constructs url
= f"http://127.0.0.1:{port}/path%40encoded/file.bin", creates explicit_operator
= Operator("http", endpoint=f"http://127.0.0.1:{port}"), calls
flasher.flash(url, operator=explicit_operator) and then calls
_assert_encoding_preserved(received_paths) to verify encoding is preserved for
StorageMuxFlasherClient.flash.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b26d6462-7c3e-49d6-b191-d3f738b819c6
📒 Files selected for processing (2)
python/packages/jumpstarter-driver-opendal/jumpstarter_driver_opendal/client.pypython/packages/jumpstarter-driver-opendal/jumpstarter_driver_opendal/driver_test.py
2 participants
When callers pass an explicit operator (e.g. after resolving via operator_for_path()), the original_url detection was skipped because it was nested inside
if operator is None. This caused HTTP URLs to go through OpenDAL presign_read, mangling percent-encoded paths. Move the URL check before the guard in all three methods.