-
Notifications
You must be signed in to change notification settings - Fork 19
fix: preserve URL query parameters in storage flash for signed URLs #435
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
base: main
Are you sure you want to change the base?
Changes from all commits
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 |
|---|---|---|
|
|
@@ -44,6 +44,21 @@ async def aclose(self): | |
| pass | ||
|
|
||
|
|
||
| def clean_filename(path: PathBuf) -> str: | ||
| """Extract a clean filename from a path or URL, stripping query parameters. | ||
|
|
||
| This handles paths returned by operator_for_path() which may contain | ||
| query parameters for signed URLs (e.g. /path/to/image.raw.xz?Expires=...&Signature=...). | ||
| """ | ||
| path_str = str(path) | ||
| if path_str.startswith(("http://", "https://")): | ||
| return urlparse(path_str).path.split("/")[-1] | ||
| name = Path(path_str).name | ||
| if "?" in name: | ||
| name = name.split("?", 1)[0] | ||
| return name | ||
|
|
||
|
|
||
| def operator_for_path(path: PathBuf) -> tuple[PathBuf, Operator, str]: | ||
| """Create an operator for the given path | ||
| Return a tuple of: | ||
|
|
@@ -54,7 +69,13 @@ def operator_for_path(path: PathBuf) -> tuple[PathBuf, Operator, str]: | |
| if type(path) is str and path.startswith(("http://", "https://")): | ||
| parsed_url = urlparse(path) | ||
| operator = Operator("http", root="/", endpoint=f"{parsed_url.scheme}://{parsed_url.netloc}") | ||
| return Path(parsed_url.path), operator, "http" | ||
| # Preserve query parameters in the path so that signed URLs | ||
| # (e.g. CloudFront URLs with ?Expires=...&Signature=...&Key-Pair-Id=...) | ||
| # are fetched correctly by the OpenDAL HTTP operator. | ||
| op_path = parsed_url.path | ||
| if parsed_url.query: | ||
| op_path = f"{op_path}?{parsed_url.query}" | ||
| return op_path, operator, "http" | ||
|
Member
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. Now that The flashers package handles this correctly after your changes, but the ridesx client at filename = Path(path_buf).namewhich would produce Would it make sense to either update the ridesx caller as well, or provide a shared helper for extracting a clean filename from the return value of AI-generated, human reviewed
Contributor
Author
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. Good catch -- this is indeed a real bug. The ridesx client's Fixed in 4aaa40a by:
|
||
| else: | ||
| return Path(path).resolve(), Operator("fs", root="/"), "fs" | ||
|
|
||
|
|
||
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.
Nice test! One thing to note: this covers the code path where the signed URL is passed through directly (no download to the exporter httpd). The more involved path where query params are reconstructed from
parsed.path+parsed.query(the bearer token branch inflash()) is only covered indirectly via the unit tests of the individual helpers. Consider whether an integration-level test for that branch would be valuable -- not a blocker, just something to keep in mind.AI-generated, human reviewed
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.
Thanks for the note. Agreed that an integration-level test for the bearer token branch would add value. I'll keep that in mind as a follow-up -- the current unit tests for the individual helpers provide reasonable coverage for now and adding a full integration test for that path would require more complex mocking of the HTTP download + bearer auth flow which feels out of scope for this fix PR.