Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 16 additions & 0 deletions nc_py_api/files/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -107,6 +107,8 @@ def __init__(self, **kwargs):
self.creation_date = kwargs.get("creation_date", datetime.datetime(1970, 1, 1))
except (ValueError, TypeError):
self.creation_date = datetime.datetime(1970, 1, 1)
self._download_url: str = kwargs.get("download_url", "")
self._download_url_expiration: int = kwargs.get("download_url_expiration", 0)
Comment on lines +110 to +111
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

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 = 0

As 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.

self._trashbin: dict[str, str | int] = {}
for i in ("trashbin_filename", "trashbin_original_location", "trashbin_deletion_time"):
if i in kwargs:
Expand All @@ -132,6 +134,20 @@ def permissions(self) -> str:
"""Permissions for the object."""
return self._raw_data["permissions"]

@property
def download_url(self) -> str:
"""S3 presigned URL for direct download, bypassing Nextcloud.

Only available when the storage backend is S3 with ``use_presigned_url`` enabled.
Empty string when not available.
"""
return self._download_url

@property
def download_url_expiration(self) -> int:
"""Expiration timestamp for :py:attr:`download_url`. Zero when not available."""
return self._download_url_expiration

@property
def last_modified(self) -> datetime.datetime:
"""Time when the object was last modified.
Expand Down
9 changes: 8 additions & 1 deletion nc_py_api/files/_files.py
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
Expand All @@ -26,6 +27,7 @@
"oc:id",
"oc:fileid",
"oc:downloadURL",
"nc:download-url-expiration",
"oc:dDC",
"oc:permissions",
"oc:checksums",
Expand Down Expand Up @@ -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
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 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))
PY

Repository: 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 -n

Repository: 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 -50

Repository: 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 2

Repository: 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 py

Repository: 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 -20

Repository: 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.

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:
Expand Down Expand Up @@ -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
92 changes: 92 additions & 0 deletions tests_unit/test_download_url.py
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
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

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.

}
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
Loading