diff --git a/CHANGELOG.md b/CHANGELOG.md index 0ff94929..6b101ff4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,12 @@ All notable changes to this project will be documented in this file. +## [0.30.3 - 2026-08-03] + +### Fixed + +- PROPFIND property lists are no longer mutated in place. `get_propfind_properties()` and both `trashbin_list()` implementations extended the shared `PROPFIND_PROPERTIES` constant with `+=`, so it grew on every call (7 entries per call against servers advertising `files.locking`, 3 per `trashbin_list()`). Long-running clients ended up sending multi-megabyte PROPFIND bodies that could exhaust the server's workers. Both property constants are now immutable tuples, so this class of bug cannot come back. #453 Thanks to @ciberkids + ## [0.30.2 - 2026-06-02] ### Changed diff --git a/nc_py_api/files/_files.py b/nc_py_api/files/_files.py index 7d584e46..e6496460 100644 --- a/nc_py_api/files/_files.py +++ b/nc_py_api/files/_files.py @@ -2,6 +2,8 @@ import contextlib import enum +import typing +from collections.abc import Sequence from datetime import datetime, timezone from io import BytesIO from json import dumps, loads @@ -16,7 +18,7 @@ from .._misc import check_capabilities, clear_from_params_empty from . import FsNode, SystemTag -PROPFIND_PROPERTIES = [ +PROPFIND_PROPERTIES: typing.Final[tuple[str, ...]] = ( "d:resourcetype", "d:getlastmodified", "d:creationdate", @@ -34,9 +36,9 @@ "oc:share-types", "oc:favorite", "nc:is-encrypted", -] +) -PROPFIND_LOCKING_PROPERTIES = [ +PROPFIND_LOCKING_PROPERTIES: typing.Final[tuple[str, ...]] = ( "nc:lock", "nc:lock-owner-displayname", "nc:lock-owner", @@ -44,7 +46,7 @@ "nc:lock-owner-editor", # App id of an app owned lock "nc:lock-time", # Timestamp of the log creation time "nc:lock-timeout", # TTL of the lock in seconds staring from the creation time -] +) SEARCH_PROPERTIES_MAP = { "name": "d:displayname", # like, eq @@ -66,8 +68,8 @@ class PropFindType(enum.IntEnum): VERSIONS_FILE_ID = 3 -def get_propfind_properties(capabilities: dict) -> list: - r = PROPFIND_PROPERTIES +def get_propfind_properties(capabilities: dict) -> list[str]: + r = list(PROPFIND_PROPERTIES) if not check_capabilities("files.locking", capabilities): r += PROPFIND_LOCKING_PROPERTIES return r @@ -222,7 +224,7 @@ def build_update_tag_req( def build_listdir_req( - user: str, path: str, properties: list[str], prop_type: PropFindType + user: str, path: str, properties: Sequence[str], prop_type: PropFindType ) -> tuple[ElementTree.Element, str]: root = ElementTree.Element( "d:propfind", @@ -245,7 +247,7 @@ def build_listdir_response( webdav_response: Response, user: str, path: str, - properties: list[str], + properties: Sequence[str], exclude_self: bool, prop_type: PropFindType, ) -> list[FsNode]: diff --git a/nc_py_api/files/files.py b/nc_py_api/files/files.py index 32b14d65..eaf027d9 100644 --- a/nc_py_api/files/files.py +++ b/nc_py_api/files/files.py @@ -2,6 +2,7 @@ import builtins import os +from collections.abc import Sequence from pathlib import Path from urllib.parse import quote @@ -278,8 +279,12 @@ def setfav(self, path: str | FsNode, value: int | bool) -> None: def trashbin_list(self) -> list[FsNode]: """Returns a list of all entries in the TrashBin.""" - properties = PROPFIND_PROPERTIES - properties += ["nc:trashbin-filename", "nc:trashbin-original-location", "nc:trashbin-deletion-time"] + properties = [ + *PROPFIND_PROPERTIES, + "nc:trashbin-filename", + "nc:trashbin-original-location", + "nc:trashbin-deletion-time", + ] return self._listdir( self._session.user, "", properties=properties, depth=1, exclude_self=False, prop_type=PropFindType.TRASHBIN ) @@ -457,7 +462,7 @@ def _listdir( self, user: str, path: str, - properties: list[str], + properties: Sequence[str], depth: int, exclude_self: bool, prop_type: PropFindType = PropFindType.DEFAULT, diff --git a/nc_py_api/files/files_async.py b/nc_py_api/files/files_async.py index 1d1e2648..05e09fae 100644 --- a/nc_py_api/files/files_async.py +++ b/nc_py_api/files/files_async.py @@ -2,6 +2,7 @@ import builtins import os +from collections.abc import Sequence from pathlib import Path from urllib.parse import quote @@ -282,8 +283,12 @@ async def setfav(self, path: str | FsNode, value: int | bool) -> None: async def trashbin_list(self) -> list[FsNode]: """Returns a list of all entries in the TrashBin.""" - properties = PROPFIND_PROPERTIES - properties += ["nc:trashbin-filename", "nc:trashbin-original-location", "nc:trashbin-deletion-time"] + properties = [ + *PROPFIND_PROPERTIES, + "nc:trashbin-filename", + "nc:trashbin-original-location", + "nc:trashbin-deletion-time", + ] return await self._listdir( await self._session.user, "", @@ -466,7 +471,7 @@ async def _listdir( self, user: str, path: str, - properties: list[str], + properties: Sequence[str], depth: int, exclude_self: bool, prop_type: PropFindType = PropFindType.DEFAULT, diff --git a/tests_unit/test_propfind_properties.py b/tests_unit/test_propfind_properties.py new file mode 100644 index 00000000..550ef734 --- /dev/null +++ b/tests_unit/test_propfind_properties.py @@ -0,0 +1,105 @@ +"""Tests that the shared PROPFIND property lists are never mutated in place.""" + +import types + +import pytest + +from nc_py_api.files._files import ( + PROPFIND_LOCKING_PROPERTIES, + PROPFIND_PROPERTIES, + PropFindType, + get_propfind_properties, +) +from nc_py_api.files.files import FilesAPI +from nc_py_api.files.files_async import AsyncFilesAPI + +CAPS_WITH_LOCKING = {"files": {"locking": "1.0"}} +CAPS_WITHOUT_LOCKING = {"files": {}} +TRASHBIN_PROPERTIES = ["nc:trashbin-filename", "nc:trashbin-original-location", "nc:trashbin-deletion-time"] +# real copies taken at import: comparing against the live constants would be vacuous +# if they are ever turned back into lists, since the name would alias the same object +EXPECTED_PROPERTIES = tuple(PROPFIND_PROPERTIES) +EXPECTED_LOCKING_PROPERTIES = tuple(PROPFIND_LOCKING_PROPERTIES) + + +def test_propfind_constants_are_immutable(): + assert isinstance(PROPFIND_PROPERTIES, tuple) + assert isinstance(PROPFIND_LOCKING_PROPERTIES, tuple) + with pytest.raises(AttributeError): + PROPFIND_PROPERTIES.append("nc:not-allowed") + + +def test_get_propfind_properties_does_not_mutate_constants(): + for _ in range(10): + get_propfind_properties(CAPS_WITH_LOCKING) + get_propfind_properties(CAPS_WITHOUT_LOCKING) + assert tuple(PROPFIND_PROPERTIES) == EXPECTED_PROPERTIES + assert tuple(PROPFIND_LOCKING_PROPERTIES) == EXPECTED_LOCKING_PROPERTIES + + +def test_get_propfind_properties_returns_fresh_list(): + first = get_propfind_properties(CAPS_WITH_LOCKING) + second = get_propfind_properties(CAPS_WITH_LOCKING) + assert isinstance(first, list) + assert first == second + assert first is not second + first.append("nc:added-by-caller") + assert "nc:added-by-caller" not in second + assert "nc:added-by-caller" not in PROPFIND_PROPERTIES + assert tuple(PROPFIND_PROPERTIES) == EXPECTED_PROPERTIES + + +def test_get_propfind_properties_locking_capability(): + with_locking = get_propfind_properties(CAPS_WITH_LOCKING) + without_locking = get_propfind_properties(CAPS_WITHOUT_LOCKING) + assert with_locking == [*EXPECTED_PROPERTIES, *EXPECTED_LOCKING_PROPERTIES] + assert without_locking == list(EXPECTED_PROPERTIES) + + +def test_trashbin_list_does_not_mutate_constants(monkeypatch): + requested = [] + + def _fake_listdir(_self, _user, _path, **kwargs): + requested.append(list(kwargs["properties"])) + return [] + + monkeypatch.setattr(FilesAPI, "_listdir", _fake_listdir) + files_api = FilesAPI(types.SimpleNamespace(user="admin")) + for _ in range(3): + files_api.trashbin_list() + assert tuple(PROPFIND_PROPERTIES) == EXPECTED_PROPERTIES + for properties in requested: + assert properties == [*EXPECTED_PROPERTIES, *TRASHBIN_PROPERTIES] + + +async def test_trashbin_list_async_does_not_mutate_constants(monkeypatch): + requested = [] + + async def _fake_listdir(_self, _user, _path, **kwargs): + requested.append(list(kwargs["properties"])) + return [] + + class _StubSession: + @property + async def user(self) -> str: + return "admin" + + monkeypatch.setattr(AsyncFilesAPI, "_listdir", _fake_listdir) + files_api = AsyncFilesAPI(_StubSession()) + for _ in range(3): + await files_api.trashbin_list() + assert tuple(PROPFIND_PROPERTIES) == EXPECTED_PROPERTIES + for properties in requested: + assert properties == [*EXPECTED_PROPERTIES, *TRASHBIN_PROPERTIES] + + +def test_trashbin_list_requests_trashbin_prop_type(monkeypatch): + calls = [] + + def _fake_listdir(_self, _user, _path, **kwargs): + calls.append(kwargs["prop_type"]) + return [] + + monkeypatch.setattr(FilesAPI, "_listdir", _fake_listdir) + FilesAPI(types.SimpleNamespace(user="admin")).trashbin_list() + assert calls == [PropFindType.TRASHBIN]