-
Notifications
You must be signed in to change notification settings - Fork 12
feat: expose S3 presigned download URL on FsNode #419
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 | ||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -1,5 +1,6 @@ | ||||||||||||||||||
| """Helper functions for **FilesAPI** and **AsyncFilesAPI** classes.""" | ||||||||||||||||||
|
|
||||||||||||||||||
| import contextlib | ||||||||||||||||||
| import enum | ||||||||||||||||||
| from datetime import datetime, timezone | ||||||||||||||||||
| from io import BytesIO | ||||||||||||||||||
|
|
@@ -26,6 +27,7 @@ | |||||||||||||||||
| "oc:id", | ||||||||||||||||||
| "oc:fileid", | ||||||||||||||||||
| "oc:downloadURL", | ||||||||||||||||||
| "nc:download-url-expiration", | ||||||||||||||||||
| "oc:dDC", | ||||||||||||||||||
| "oc:permissions", | ||||||||||||||||||
| "oc:checksums", | ||||||||||||||||||
|
|
@@ -306,6 +308,11 @@ def _parse_record(full_path: str, prop_stats: list[dict]) -> FsNode: # noqa pyl | |||||||||||||||||
| fs_node_args["mimetype"] = prop["d:getcontenttype"] | ||||||||||||||||||
| if "oc:permissions" in prop_keys: | ||||||||||||||||||
| fs_node_args["permissions"] = prop["oc:permissions"] | ||||||||||||||||||
| if "oc:downloadURL" in prop_keys and prop["oc:downloadURL"]: | ||||||||||||||||||
| fs_node_args["download_url"] = prop["oc:downloadURL"] | ||||||||||||||||||
|
Comment on lines
+311
to
+312
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. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
python - <<'PY'
import xmltodict
xml = '<d:prop xmlns:d="DAV:" xmlns:oc="http://owncloud.org/ns"><oc:downloadURL>false</oc:downloadURL></d:prop>'
value = xmltodict.parse(xml)["d:prop"]["oc:downloadURL"]
print(type(value).__name__, repr(value))
PYRepository: cloud-py-api/nc_py_api Length of output: 77 🏁 Script executed: # Check the actual code at lines 311-312 and surrounding context
sed -n '300,320p' nc_py_api/files/_files.py | cat -nRepository: cloud-py-api/nc_py_api Length of output: 1496 🏁 Script executed: # Find the Pydantic model definition for FSNode to understand the download_url field
rg -A 10 "class FSNode" nc_py_api/ | head -50Repository: cloud-py-api/nc_py_api Length of output: 48 🏁 Script executed: # Search for download_url field definition and type annotation
rg "download_url" nc_py_api/files/ -B 2 -A 2Repository: cloud-py-api/nc_py_api Length of output: 2249 🏁 Script executed: # Check for existing tests related to download_url
rg -l "download_url" nc_py_api/tests/ --type pyRepository: cloud-py-api/nc_py_api Length of output: 122 🏁 Script executed: # Look for similar patterns in the file that might have the same issue
rg "if \".*\" in prop_keys and prop\[" nc_py_api/files/_files.py | head -20Repository: cloud-py-api/nc_py_api Length of output: 227 Fix truthiness check for literal When WebDAV returns Proposed fix- if "oc:downloadURL" in prop_keys and prop["oc:downloadURL"]:
- fs_node_args["download_url"] = prop["oc:downloadURL"]
+ if "oc:downloadURL" in prop_keys:
+ download_url = prop["oc:downloadURL"]
+ if isinstance(download_url, str):
+ download_url = download_url.strip()
+ if download_url not in (None, "", False) and str(download_url).lower() != "false":
+ fs_node_args["download_url"] = str(download_url)📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||
| if "nc:download-url-expiration" in prop_keys and prop["nc:download-url-expiration"]: | ||||||||||||||||||
| with contextlib.suppress(TypeError, ValueError): | ||||||||||||||||||
| fs_node_args["download_url_expiration"] = int(prop["nc:download-url-expiration"]) | ||||||||||||||||||
| if "oc:favorite" in prop_keys: | ||||||||||||||||||
| fs_node_args["favorite"] = bool(int(prop["oc:favorite"])) | ||||||||||||||||||
| if "nc:trashbin-filename" in prop_keys: | ||||||||||||||||||
|
|
@@ -367,7 +374,7 @@ def _webdav_response_to_records(webdav_res: Response, info: str) -> list[dict]: | |||||||||||||||||
| if "d:error" in response_data: | ||||||||||||||||||
| err = response_data["d:error"] | ||||||||||||||||||
| raise NextcloudException( | ||||||||||||||||||
| reason=f'{err["s:exception"]}: {err["s:message"]}'.replace("\n", ""), info=info, response=webdav_res | ||||||||||||||||||
| reason=f"{err['s:exception']}: {err['s:message']}".replace("\n", ""), info=info, response=webdav_res | ||||||||||||||||||
| ) | ||||||||||||||||||
| response = response_data["d:multistatus"].get("d:response", []) | ||||||||||||||||||
| return [response] if isinstance(response, dict) else response | ||||||||||||||||||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,92 @@ | ||||||||||||||||||||||||||||||||||||||||||||||||||
| """Tests for S3 presigned download URL properties on FsNode/FsNodeInfo.""" | ||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||
| from nc_py_api.files import FsNode, FsNodeInfo | ||||||||||||||||||||||||||||||||||||||||||||||||||
| from nc_py_api.files._files import _parse_record | ||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||
| def test_fsnode_info_download_url_defaults(): | ||||||||||||||||||||||||||||||||||||||||||||||||||
| info = FsNodeInfo() | ||||||||||||||||||||||||||||||||||||||||||||||||||
| assert info.download_url == "" | ||||||||||||||||||||||||||||||||||||||||||||||||||
| assert info.download_url_expiration == 0 | ||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||
| def test_fsnode_info_download_url_with_values(): | ||||||||||||||||||||||||||||||||||||||||||||||||||
| info = FsNodeInfo(download_url="https://s3.example.com/bucket/obj?sig=abc", download_url_expiration=1700000000) | ||||||||||||||||||||||||||||||||||||||||||||||||||
| assert info.download_url == "https://s3.example.com/bucket/obj?sig=abc" | ||||||||||||||||||||||||||||||||||||||||||||||||||
| assert info.download_url_expiration == 1700000000 | ||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||
| def test_fsnode_passes_download_url_to_info(): | ||||||||||||||||||||||||||||||||||||||||||||||||||
| node = FsNode( | ||||||||||||||||||||||||||||||||||||||||||||||||||
| "files/admin/test.txt", file_id="00000123", download_url="https://s3.test/f", download_url_expiration=9999 | ||||||||||||||||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||||||||||||||||
| assert node.info.download_url == "https://s3.test/f" | ||||||||||||||||||||||||||||||||||||||||||||||||||
| assert node.info.download_url_expiration == 9999 | ||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||
| def test_parse_record_with_download_url(): | ||||||||||||||||||||||||||||||||||||||||||||||||||
| prop_stat = { | ||||||||||||||||||||||||||||||||||||||||||||||||||
| "d:status": "HTTP/1.1 200 OK", | ||||||||||||||||||||||||||||||||||||||||||||||||||
| "d:prop": { | ||||||||||||||||||||||||||||||||||||||||||||||||||
| "oc:id": "00000123", | ||||||||||||||||||||||||||||||||||||||||||||||||||
| "oc:fileid": "123", | ||||||||||||||||||||||||||||||||||||||||||||||||||
| "oc:permissions": "RGDNVW", | ||||||||||||||||||||||||||||||||||||||||||||||||||
| "d:getetag": '"abc123"', | ||||||||||||||||||||||||||||||||||||||||||||||||||
| "oc:downloadURL": "https://s3.example.com/bucket/urn:oid:123?X-Amz-Signature=abc", | ||||||||||||||||||||||||||||||||||||||||||||||||||
| "nc:download-url-expiration": "1700000000", | ||||||||||||||||||||||||||||||||||||||||||||||||||
| }, | ||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||
| node = _parse_record("files/admin/test.txt", [prop_stat]) | ||||||||||||||||||||||||||||||||||||||||||||||||||
| assert node.info.download_url == "https://s3.example.com/bucket/urn:oid:123?X-Amz-Signature=abc" | ||||||||||||||||||||||||||||||||||||||||||||||||||
| assert node.info.download_url_expiration == 1700000000 | ||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||
| def test_parse_record_without_download_url(): | ||||||||||||||||||||||||||||||||||||||||||||||||||
| prop_stat = { | ||||||||||||||||||||||||||||||||||||||||||||||||||
| "d:status": "HTTP/1.1 200 OK", | ||||||||||||||||||||||||||||||||||||||||||||||||||
| "d:prop": { | ||||||||||||||||||||||||||||||||||||||||||||||||||
| "oc:id": "00000123", | ||||||||||||||||||||||||||||||||||||||||||||||||||
| "oc:fileid": "123", | ||||||||||||||||||||||||||||||||||||||||||||||||||
| "oc:permissions": "RGDNVW", | ||||||||||||||||||||||||||||||||||||||||||||||||||
| "d:getetag": '"abc123"', | ||||||||||||||||||||||||||||||||||||||||||||||||||
| }, | ||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||
| node = _parse_record("files/admin/test.txt", [prop_stat]) | ||||||||||||||||||||||||||||||||||||||||||||||||||
| assert node.info.download_url == "" | ||||||||||||||||||||||||||||||||||||||||||||||||||
| assert node.info.download_url_expiration == 0 | ||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||
| def test_parse_record_with_false_download_url(): | ||||||||||||||||||||||||||||||||||||||||||||||||||
| """When storage doesn't support presigned URLs, server returns 'false'.""" | ||||||||||||||||||||||||||||||||||||||||||||||||||
| prop_stat = { | ||||||||||||||||||||||||||||||||||||||||||||||||||
| "d:status": "HTTP/1.1 200 OK", | ||||||||||||||||||||||||||||||||||||||||||||||||||
| "d:prop": { | ||||||||||||||||||||||||||||||||||||||||||||||||||
| "oc:id": "00000123", | ||||||||||||||||||||||||||||||||||||||||||||||||||
| "oc:fileid": "123", | ||||||||||||||||||||||||||||||||||||||||||||||||||
| "oc:permissions": "RGDNVW", | ||||||||||||||||||||||||||||||||||||||||||||||||||
| "d:getetag": '"abc123"', | ||||||||||||||||||||||||||||||||||||||||||||||||||
| "oc:downloadURL": False, | ||||||||||||||||||||||||||||||||||||||||||||||||||
| "nc:download-url-expiration": False, | ||||||||||||||||||||||||||||||||||||||||||||||||||
| }, | ||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+59
to
+70
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. Use string At Line 68-69, using Proposed fix- "oc:downloadURL": False,
- "nc:download-url-expiration": False,
+ "oc:downloadURL": "false",
+ "nc:download-url-expiration": "false",📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||
| node = _parse_record("files/admin/test.txt", [prop_stat]) | ||||||||||||||||||||||||||||||||||||||||||||||||||
| assert node.info.download_url == "" | ||||||||||||||||||||||||||||||||||||||||||||||||||
| assert node.info.download_url_expiration == 0 | ||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||
| def test_parse_record_with_malformed_expiration(): | ||||||||||||||||||||||||||||||||||||||||||||||||||
| """Malformed expiration should not crash parsing.""" | ||||||||||||||||||||||||||||||||||||||||||||||||||
| prop_stat = { | ||||||||||||||||||||||||||||||||||||||||||||||||||
| "d:status": "HTTP/1.1 200 OK", | ||||||||||||||||||||||||||||||||||||||||||||||||||
| "d:prop": { | ||||||||||||||||||||||||||||||||||||||||||||||||||
| "oc:id": "00000123", | ||||||||||||||||||||||||||||||||||||||||||||||||||
| "oc:fileid": "123", | ||||||||||||||||||||||||||||||||||||||||||||||||||
| "oc:permissions": "RGDNVW", | ||||||||||||||||||||||||||||||||||||||||||||||||||
| "d:getetag": '"abc123"', | ||||||||||||||||||||||||||||||||||||||||||||||||||
| "oc:downloadURL": "https://s3.test/f", | ||||||||||||||||||||||||||||||||||||||||||||||||||
| "nc:download-url-expiration": "not-a-number", | ||||||||||||||||||||||||||||||||||||||||||||||||||
| }, | ||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||
| node = _parse_record("files/admin/test.txt", [prop_stat]) | ||||||||||||||||||||||||||||||||||||||||||||||||||
| assert node.info.download_url == "https://s3.test/f" | ||||||||||||||||||||||||||||||||||||||||||||||||||
| assert node.info.download_url_expiration == 0 | ||||||||||||||||||||||||||||||||||||||||||||||||||
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.
Coerce
download_url_expirationtointat constructionLine 111 currently passes through raw
kwargsvalue, sodownload_url_expirationcan become astrat runtime despite theintAPI contract.Proposed fix
As per coding guidelines, "Type annotations correctness (Pydantic models are used extensively)".
🤖 Prompt for AI Agents