Skip to content

Commit b014f93

Browse files
authored
fix(files): never mutate the shared PROPFIND property lists (#455)
`get_propfind_properties()` and both `trashbin_list()` implementations extended the shared `PROPFIND_PROPERTIES` constant with `+=` instead of copying it, so it grew on every call: +7 entries when the server advertises `files.locking`, +3 per `trashbin_list()`. Long-running clients ended up sending multi-megabyte PROPFIND bodies (measured: ~138 bytes of growth per `listdir()` call), and the server cost is `O(properties x resources)`. - build a fresh list in all three places - both constants are now `Final[tuple[str, ...]]`, so in-place mutation cannot come back - `Sequence[str]` where those lists are only iterated - 7 regression tests in `tests_unit/`, verified to fail on `main` Closes #453 <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **Bug Fixes** - Fixed repeated file and directory listing requests expanding over time. - Prevented shared property settings from being modified during trash-bin and locking-property operations. - Improved request consistency for synchronous and asynchronous listings. - **Tests** - Added coverage confirming property settings remain unchanged, fresh property lists are returned, and trash-bin requests use the correct properties. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Oleksandr Piskun <oleksandr2088@icloud.com>
1 parent 126a31e commit b014f93

5 files changed

Lines changed: 137 additions & 14 deletions

File tree

CHANGELOG.md

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,12 @@
22

33
All notable changes to this project will be documented in this file.
44

5+
## [0.30.3 - 2026-08-03]
6+
7+
### Fixed
8+
9+
- 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
10+
511
## [0.30.2 - 2026-06-02]
612

713
### Changed

nc_py_api/files/_files.py

Lines changed: 10 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,8 @@
22

33
import contextlib
44
import enum
5+
import typing
6+
from collections.abc import Sequence
57
from datetime import datetime, timezone
68
from io import BytesIO
79
from json import dumps, loads
@@ -16,7 +18,7 @@
1618
from .._misc import check_capabilities, clear_from_params_empty
1719
from . import FsNode, SystemTag
1820

19-
PROPFIND_PROPERTIES = [
21+
PROPFIND_PROPERTIES: typing.Final[tuple[str, ...]] = (
2022
"d:resourcetype",
2123
"d:getlastmodified",
2224
"d:creationdate",
@@ -34,17 +36,17 @@
3436
"oc:share-types",
3537
"oc:favorite",
3638
"nc:is-encrypted",
37-
]
39+
)
3840

39-
PROPFIND_LOCKING_PROPERTIES = [
41+
PROPFIND_LOCKING_PROPERTIES: typing.Final[tuple[str, ...]] = (
4042
"nc:lock",
4143
"nc:lock-owner-displayname",
4244
"nc:lock-owner",
4345
"nc:lock-owner-type",
4446
"nc:lock-owner-editor", # App id of an app owned lock
4547
"nc:lock-time", # Timestamp of the log creation time
4648
"nc:lock-timeout", # TTL of the lock in seconds staring from the creation time
47-
]
49+
)
4850

4951
SEARCH_PROPERTIES_MAP = {
5052
"name": "d:displayname", # like, eq
@@ -66,8 +68,8 @@ class PropFindType(enum.IntEnum):
6668
VERSIONS_FILE_ID = 3
6769

6870

69-
def get_propfind_properties(capabilities: dict) -> list:
70-
r = PROPFIND_PROPERTIES
71+
def get_propfind_properties(capabilities: dict) -> list[str]:
72+
r = list(PROPFIND_PROPERTIES)
7173
if not check_capabilities("files.locking", capabilities):
7274
r += PROPFIND_LOCKING_PROPERTIES
7375
return r
@@ -222,7 +224,7 @@ def build_update_tag_req(
222224

223225

224226
def build_listdir_req(
225-
user: str, path: str, properties: list[str], prop_type: PropFindType
227+
user: str, path: str, properties: Sequence[str], prop_type: PropFindType
226228
) -> tuple[ElementTree.Element, str]:
227229
root = ElementTree.Element(
228230
"d:propfind",
@@ -245,7 +247,7 @@ def build_listdir_response(
245247
webdav_response: Response,
246248
user: str,
247249
path: str,
248-
properties: list[str],
250+
properties: Sequence[str],
249251
exclude_self: bool,
250252
prop_type: PropFindType,
251253
) -> list[FsNode]:

nc_py_api/files/files.py

Lines changed: 8 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@
22

33
import builtins
44
import os
5+
from collections.abc import Sequence
56
from pathlib import Path
67
from urllib.parse import quote
78

@@ -278,8 +279,12 @@ def setfav(self, path: str | FsNode, value: int | bool) -> None:
278279

279280
def trashbin_list(self) -> list[FsNode]:
280281
"""Returns a list of all entries in the TrashBin."""
281-
properties = PROPFIND_PROPERTIES
282-
properties += ["nc:trashbin-filename", "nc:trashbin-original-location", "nc:trashbin-deletion-time"]
282+
properties = [
283+
*PROPFIND_PROPERTIES,
284+
"nc:trashbin-filename",
285+
"nc:trashbin-original-location",
286+
"nc:trashbin-deletion-time",
287+
]
283288
return self._listdir(
284289
self._session.user, "", properties=properties, depth=1, exclude_self=False, prop_type=PropFindType.TRASHBIN
285290
)
@@ -457,7 +462,7 @@ def _listdir(
457462
self,
458463
user: str,
459464
path: str,
460-
properties: list[str],
465+
properties: Sequence[str],
461466
depth: int,
462467
exclude_self: bool,
463468
prop_type: PropFindType = PropFindType.DEFAULT,

nc_py_api/files/files_async.py

Lines changed: 8 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@
22

33
import builtins
44
import os
5+
from collections.abc import Sequence
56
from pathlib import Path
67
from urllib.parse import quote
78

@@ -282,8 +283,12 @@ async def setfav(self, path: str | FsNode, value: int | bool) -> None:
282283

283284
async def trashbin_list(self) -> list[FsNode]:
284285
"""Returns a list of all entries in the TrashBin."""
285-
properties = PROPFIND_PROPERTIES
286-
properties += ["nc:trashbin-filename", "nc:trashbin-original-location", "nc:trashbin-deletion-time"]
286+
properties = [
287+
*PROPFIND_PROPERTIES,
288+
"nc:trashbin-filename",
289+
"nc:trashbin-original-location",
290+
"nc:trashbin-deletion-time",
291+
]
287292
return await self._listdir(
288293
await self._session.user,
289294
"",
@@ -466,7 +471,7 @@ async def _listdir(
466471
self,
467472
user: str,
468473
path: str,
469-
properties: list[str],
474+
properties: Sequence[str],
470475
depth: int,
471476
exclude_self: bool,
472477
prop_type: PropFindType = PropFindType.DEFAULT,
Lines changed: 105 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,105 @@
1+
"""Tests that the shared PROPFIND property lists are never mutated in place."""
2+
3+
import types
4+
5+
import pytest
6+
7+
from nc_py_api.files._files import (
8+
PROPFIND_LOCKING_PROPERTIES,
9+
PROPFIND_PROPERTIES,
10+
PropFindType,
11+
get_propfind_properties,
12+
)
13+
from nc_py_api.files.files import FilesAPI
14+
from nc_py_api.files.files_async import AsyncFilesAPI
15+
16+
CAPS_WITH_LOCKING = {"files": {"locking": "1.0"}}
17+
CAPS_WITHOUT_LOCKING = {"files": {}}
18+
TRASHBIN_PROPERTIES = ["nc:trashbin-filename", "nc:trashbin-original-location", "nc:trashbin-deletion-time"]
19+
# real copies taken at import: comparing against the live constants would be vacuous
20+
# if they are ever turned back into lists, since the name would alias the same object
21+
EXPECTED_PROPERTIES = tuple(PROPFIND_PROPERTIES)
22+
EXPECTED_LOCKING_PROPERTIES = tuple(PROPFIND_LOCKING_PROPERTIES)
23+
24+
25+
def test_propfind_constants_are_immutable():
26+
assert isinstance(PROPFIND_PROPERTIES, tuple)
27+
assert isinstance(PROPFIND_LOCKING_PROPERTIES, tuple)
28+
with pytest.raises(AttributeError):
29+
PROPFIND_PROPERTIES.append("nc:not-allowed")
30+
31+
32+
def test_get_propfind_properties_does_not_mutate_constants():
33+
for _ in range(10):
34+
get_propfind_properties(CAPS_WITH_LOCKING)
35+
get_propfind_properties(CAPS_WITHOUT_LOCKING)
36+
assert tuple(PROPFIND_PROPERTIES) == EXPECTED_PROPERTIES
37+
assert tuple(PROPFIND_LOCKING_PROPERTIES) == EXPECTED_LOCKING_PROPERTIES
38+
39+
40+
def test_get_propfind_properties_returns_fresh_list():
41+
first = get_propfind_properties(CAPS_WITH_LOCKING)
42+
second = get_propfind_properties(CAPS_WITH_LOCKING)
43+
assert isinstance(first, list)
44+
assert first == second
45+
assert first is not second
46+
first.append("nc:added-by-caller")
47+
assert "nc:added-by-caller" not in second
48+
assert "nc:added-by-caller" not in PROPFIND_PROPERTIES
49+
assert tuple(PROPFIND_PROPERTIES) == EXPECTED_PROPERTIES
50+
51+
52+
def test_get_propfind_properties_locking_capability():
53+
with_locking = get_propfind_properties(CAPS_WITH_LOCKING)
54+
without_locking = get_propfind_properties(CAPS_WITHOUT_LOCKING)
55+
assert with_locking == [*EXPECTED_PROPERTIES, *EXPECTED_LOCKING_PROPERTIES]
56+
assert without_locking == list(EXPECTED_PROPERTIES)
57+
58+
59+
def test_trashbin_list_does_not_mutate_constants(monkeypatch):
60+
requested = []
61+
62+
def _fake_listdir(_self, _user, _path, **kwargs):
63+
requested.append(list(kwargs["properties"]))
64+
return []
65+
66+
monkeypatch.setattr(FilesAPI, "_listdir", _fake_listdir)
67+
files_api = FilesAPI(types.SimpleNamespace(user="admin"))
68+
for _ in range(3):
69+
files_api.trashbin_list()
70+
assert tuple(PROPFIND_PROPERTIES) == EXPECTED_PROPERTIES
71+
for properties in requested:
72+
assert properties == [*EXPECTED_PROPERTIES, *TRASHBIN_PROPERTIES]
73+
74+
75+
async def test_trashbin_list_async_does_not_mutate_constants(monkeypatch):
76+
requested = []
77+
78+
async def _fake_listdir(_self, _user, _path, **kwargs):
79+
requested.append(list(kwargs["properties"]))
80+
return []
81+
82+
class _StubSession:
83+
@property
84+
async def user(self) -> str:
85+
return "admin"
86+
87+
monkeypatch.setattr(AsyncFilesAPI, "_listdir", _fake_listdir)
88+
files_api = AsyncFilesAPI(_StubSession())
89+
for _ in range(3):
90+
await files_api.trashbin_list()
91+
assert tuple(PROPFIND_PROPERTIES) == EXPECTED_PROPERTIES
92+
for properties in requested:
93+
assert properties == [*EXPECTED_PROPERTIES, *TRASHBIN_PROPERTIES]
94+
95+
96+
def test_trashbin_list_requests_trashbin_prop_type(monkeypatch):
97+
calls = []
98+
99+
def _fake_listdir(_self, _user, _path, **kwargs):
100+
calls.append(kwargs["prop_type"])
101+
return []
102+
103+
monkeypatch.setattr(FilesAPI, "_listdir", _fake_listdir)
104+
FilesAPI(types.SimpleNamespace(user="admin")).trashbin_list()
105+
assert calls == [PropFindType.TRASHBIN]

0 commit comments

Comments
 (0)