feat: expose S3 presigned download URL on FsNode#419
Open
Conversation
Add download_url and download_url_expiration properties to FsNodeInfo. The oc:downloadURL property was already requested via PROPFIND but never parsed — now it is. Also request nc:download-url-expiration property. When the Nextcloud storage backend is S3 with use_presigned_url enabled, these provide a direct download URL bypassing the Nextcloud proxy. Returns empty string / zero when not available (non-S3 backends). Closes #418
📝 WalkthroughWalkthroughAdds S3 presigned download URL capture and exposure: PROPFIND parsing now reads Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@nc_py_api/files/__init__.py`:
- Around line 110-111: The constructor assigns self._download_url_expiration
from kwargs without coercion, allowing a string to slip past the intended int
contract; update the code that sets self._download_url_expiration (in the class
__init__ where self._download_url and self._download_url_expiration are
initialized) to coerce the value to an int (e.g.,
int(kwargs.get("download_url_expiration", 0))) and defensively handle
non-numeric inputs (fallback to 0 or raise a clear error) so the attribute is
always an int at runtime.
In `@nc_py_api/files/_files.py`:
- Around line 312-313: The code attempts to parse
prop["nc:download-url-expiration"] into an int unguarded which can raise
ValueError on malformed/non-numeric values; update the block that sets
fs_node_args["download_url_expiration"] to validate or wrap the int(...)
conversion in a try/except ValueError (and TypeError) and on failure assign 0 so
parsing degrades safely; use the existing prop_keys/prop checks and ensure you
reference the same symbols (prop_keys, prop, fs_node_args,
"nc:download-url-expiration") when implementing the guard.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: c699e9bc-52be-4512-9abc-efbf722d7ec6
📒 Files selected for processing (2)
nc_py_api/files/__init__.pync_py_api/files/_files.py
Comment on lines
+110
to
+111
| self._download_url: str = kwargs.get("download_url", "") | ||
| self._download_url_expiration: int = kwargs.get("download_url_expiration", 0) |
There was a problem hiding this comment.
Coerce download_url_expiration to int at construction
Line 111 currently passes through raw kwargs value, so download_url_expiration can become a str at runtime despite the int API contract.
Proposed fix
- self._download_url_expiration: int = kwargs.get("download_url_expiration", 0)
+ try:
+ self._download_url_expiration = int(kwargs.get("download_url_expiration", 0))
+ except (TypeError, ValueError):
+ self._download_url_expiration = 0As per coding guidelines, "Type annotations correctness (Pydantic models are used extensively)".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@nc_py_api/files/__init__.py` around lines 110 - 111, The constructor assigns
self._download_url_expiration from kwargs without coercion, allowing a string to
slip past the intended int contract; update the code that sets
self._download_url_expiration (in the class __init__ where self._download_url
and self._download_url_expiration are initialized) to coerce the value to an int
(e.g., int(kwargs.get("download_url_expiration", 0))) and defensively handle
non-numeric inputs (fallback to 0 or raise a clear error) so the attribute is
always an int at runtime.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #419 +/- ##
==========================================
- Coverage 94.69% 94.63% -0.07%
==========================================
Files 47 47
Lines 5575 5587 +12
==========================================
+ Hits 5279 5287 +8
- Misses 296 300 +4
🚀 New features to boost your workflow:
|
Add 7 unit tests covering FsNodeInfo defaults, value pass-through, _parse_record with/without presigned URL data, false values from non-S3 backends, and malformed expiration values. Guard int() conversion of download-url-expiration with contextlib.suppress to handle malformed server responses gracefully.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@nc_py_api/files/_files.py`:
- Around line 311-312: The code currently treats any non-empty oc:downloadURL
value as truthy, causing the literal string "false" from xmltodict to be stored
in fs_node_args["download_url"]; update the check around prop_keys/prop for
"oc:downloadURL" (in _files.py) to only assign fs_node_args["download_url"] when
prop["oc:downloadURL"] is neither empty nor the literal string "false"
(case-insensitive), otherwise leave/set the default empty string to conform to
the FSNode API.
In `@tests_unit/test_download_url.py`:
- Around line 59-70: The fixture in test_parse_record_with_false_download_url
uses boolean False for "oc:downloadURL" and "nc:download-url-expiration" but
real payloads contain the string "false"; update the values in the test fixture
inside the test_parse_record_with_false_download_url function to use the string
"false" for both "oc:downloadURL" and "nc:download-url-expiration" so the test
mirrors actual XML text parsing and exercises the intended regression path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 9de4e952-0337-4db5-a0df-33a79cc19d28
📒 Files selected for processing (2)
nc_py_api/files/_files.pytests_unit/test_download_url.py
Comment on lines
+311
to
+312
| if "oc:downloadURL" in prop_keys and prop["oc:downloadURL"]: | ||
| fs_node_args["download_url"] = prop["oc:downloadURL"] |
There was a problem hiding this comment.
🧩 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 "false" string in download_url.
When WebDAV returns <oc:downloadURL>false</oc:downloadURL>, xmltodict parses it to the literal string "false". The condition at line 311 treats this non-empty string as truthy, storing "false" instead of the documented default empty string, violating the FSNode API contract.
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
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| 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) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@nc_py_api/files/_files.py` around lines 311 - 312, The code currently treats
any non-empty oc:downloadURL value as truthy, causing the literal string "false"
from xmltodict to be stored in fs_node_args["download_url"]; update the check
around prop_keys/prop for "oc:downloadURL" (in _files.py) to only assign
fs_node_args["download_url"] when prop["oc:downloadURL"] is neither empty nor
the literal string "false" (case-insensitive), otherwise leave/set the default
empty string to conform to the FSNode API.
Comment on lines
+59
to
+70
| 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, | ||
| }, |
There was a problem hiding this comment.
Use string "false" in this fixture to match real payload shape.
At Line 68-69, using False (bool) does not mirror typical XML text parsing. This test should use "false" so it can catch the exact regression path.
Proposed fix
- "oc:downloadURL": False,
- "nc:download-url-expiration": False,
+ "oc:downloadURL": "false",
+ "nc:download-url-expiration": "false",📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| 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, | |
| }, | |
| 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", | |
| }, |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests_unit/test_download_url.py` around lines 59 - 70, The fixture in
test_parse_record_with_false_download_url uses boolean False for
"oc:downloadURL" and "nc:download-url-expiration" but real payloads contain the
string "false"; update the values in the test fixture inside the
test_parse_record_with_false_download_url function to use the string "false" for
both "oc:downloadURL" and "nc:download-url-expiration" so the test mirrors
actual XML text parsing and exercises the intended regression path.
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Add download_url and download_url_expiration properties to
FsNodeInfo. Theoc:downloadURLproperty was already requested via PROPFIND but never parsed - now it is. Also requestnc:download-url-expirationproperty.When the Nextcloud storage backend is S3 with use_presigned_url enabled, these provide a direct download URL bypassing the Nextcloud proxy. Returns empty string / zero when not available (non-S3 backends).
Closes #418
Summary by CodeRabbit