fix(webdav): drop anyio.Lock and add integration tests for tag exclusion
Addresses two points from the latest PR #764 review: 1. The anyio.Lock in get_excluded_file_paths bought nothing under anyio's cooperative multitasking model (single-threaded between awaits, raw set mutations are already safe). _resolve_one_tag now builds a local set of paths and appends it to a shared list — list append between awaits is safe without a lock — and the caller merges via set().union(*results) after the task group completes. This removes the cognitive overhead the reviewer flagged without changing the public API. 2. Adds tests/integration/test_tag_exclusion.py exercising the resolution pipeline end-to-end against a real Nextcloud instance: creates a system tag, tags a real file and a real directory, verifies get_excluded_file_paths resolves both via real PROPFIND + REPORT calls, and verifies is_path_excluded correctly classifies exact matches, descendants of tagged directories, and unrelated paths. Includes the disabled-feature short-circuit case. Cleanup runs in reverse order (untag, delete files); per-run uuid suffix avoids cross-run interference. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
Chris Coutinho
co-authored by
Claude Opus 4.7
Claude Opus 4.7
parent
d179ca8c8b
commit
35abfb2e3a
@@ -42,13 +42,15 @@ def get_excluded_tag_names() -> list[str]:
|
||||
async def _resolve_one_tag(
|
||||
tag_name: str,
|
||||
webdav: WebDAVClient,
|
||||
excluded: set[str],
|
||||
lock: anyio.Lock,
|
||||
results: list[set[str]],
|
||||
) -> None:
|
||||
"""Resolve a single tag's paths and merge into *excluded* under *lock*.
|
||||
"""Resolve a single tag's paths and append them as a set to *results*.
|
||||
|
||||
Swallows its own exceptions so a failure for one tag does not abort
|
||||
the surrounding task group (preserves fail-open per-tag semantics).
|
||||
Each task writes to a distinct slot in the shared list — append from
|
||||
cooperative tasks is safe under anyio (single-threaded between
|
||||
awaits) without an explicit lock. Swallows its own exceptions so a
|
||||
failure for one tag does not abort the surrounding task group
|
||||
(preserves fail-open per-tag semantics).
|
||||
"""
|
||||
try:
|
||||
tag = await webdav.get_tag_by_name(tag_name)
|
||||
@@ -76,16 +78,17 @@ async def _resolve_one_tag(
|
||||
)
|
||||
return
|
||||
|
||||
async with lock:
|
||||
paths: set[str] = set()
|
||||
for f in files:
|
||||
path = _normalise_path(f["path"])
|
||||
excluded.add(path)
|
||||
paths.add(path)
|
||||
if f.get("is_directory"):
|
||||
logger.debug(
|
||||
"Excluding directory %r (tag %r) — descendants will be hidden",
|
||||
path,
|
||||
tag_name,
|
||||
)
|
||||
results.append(paths)
|
||||
|
||||
|
||||
async def get_excluded_file_paths(webdav: WebDAVClient) -> set[str]:
|
||||
@@ -96,7 +99,9 @@ async def get_excluded_file_paths(webdav: WebDAVClient) -> set[str]:
|
||||
|
||||
Per-tag resolution is fanned out via ``anyio.create_task_group`` so
|
||||
that the 2N network calls (1 PROPFIND + 1 REPORT per tag) run
|
||||
concurrently rather than serially.
|
||||
concurrently rather than serially. No lock is needed: each task
|
||||
appends its own ``set`` to a shared list, and append is atomic
|
||||
between awaits under anyio's cooperative single-threaded model.
|
||||
|
||||
**Failure mode is fail-open per tag**: if the systemtags endpoint is
|
||||
unreachable or returns an error for a given tag, that tag is skipped
|
||||
@@ -110,12 +115,12 @@ async def get_excluded_file_paths(webdav: WebDAVClient) -> set[str]:
|
||||
if not tag_names:
|
||||
return set()
|
||||
|
||||
excluded: set[str] = set()
|
||||
lock = anyio.Lock()
|
||||
results: list[set[str]] = []
|
||||
async with anyio.create_task_group() as tg:
|
||||
for tag_name in tag_names:
|
||||
tg.start_soon(_resolve_one_tag, tag_name, webdav, excluded, lock)
|
||||
tg.start_soon(_resolve_one_tag, tag_name, webdav, results)
|
||||
|
||||
excluded: set[str] = set().union(*results)
|
||||
if excluded:
|
||||
# `len(excluded)` counts directly-tagged entries — descendants of
|
||||
# tagged directories are hidden too but resolved at check time.
|
||||
|
||||
@@ -0,0 +1,191 @@
|
||||
"""End-to-end integration tests for tag-based file exclusion (issue #710).
|
||||
|
||||
These exercise the full resolution pipeline against a real Nextcloud
|
||||
instance:
|
||||
|
||||
1. Create a system tag via the WebDAV ``/systemtags`` API.
|
||||
2. Create real files / a real directory and tag them.
|
||||
3. Resolve the configured ``EXCLUDED_TAGS`` to paths via
|
||||
``get_excluded_file_paths`` — this issues a real PROPFIND against
|
||||
``/systemtags/`` and a real REPORT against the user's WebDAV root.
|
||||
4. Verify ``is_path_excluded`` correctly classifies tagged files,
|
||||
descendants of tagged directories, and unrelated paths.
|
||||
|
||||
Unlike the unit tests in ``tests/unit/test_tag_exclusion.py``, which
|
||||
mock the WebDAV layer, this test catches integration-level issues:
|
||||
malformed XML responses, namespace mismatches, missing fields, and
|
||||
PROPFIND/REPORT semantics that diverge from what the unit-test mocks
|
||||
assume.
|
||||
|
||||
The MCP server layer is exercised by the unit tests (where
|
||||
``EXCLUDED_TAGS`` is patched at the config layer); spinning up a fresh
|
||||
MCP container with a custom env var per integration test would not
|
||||
add coverage proportional to the cost.
|
||||
"""
|
||||
|
||||
import logging
|
||||
import uuid
|
||||
|
||||
import pytest
|
||||
|
||||
from nextcloud_mcp_server.client import NextcloudClient
|
||||
from nextcloud_mcp_server.server.tag_exclusion import (
|
||||
get_excluded_file_paths,
|
||||
is_path_excluded,
|
||||
)
|
||||
|
||||
logger = logging.getLogger(__name__)
|
||||
pytestmark = pytest.mark.integration
|
||||
|
||||
|
||||
@pytest.fixture
|
||||
async def excluded_tag_environment(nc_client: NextcloudClient):
|
||||
"""Provision a tag, a tagged file, a tagged directory, and an
|
||||
untagged sibling — all in a unique per-run namespace.
|
||||
|
||||
Yields a dict with the layout. Cleanup runs in reverse order:
|
||||
untag, delete files, leave the tag (no public delete API on the
|
||||
client today; tags are cheap and unique-per-run).
|
||||
"""
|
||||
suffix = uuid.uuid4().hex[:8]
|
||||
tag_name = f"mcp-no-ai-{suffix}"
|
||||
test_dir = f"mcp_tag_excl_{suffix}"
|
||||
tagged_file = f"{test_dir}/SECRET.txt"
|
||||
tagged_dir = f"{test_dir}/private"
|
||||
tagged_dir_child = f"{tagged_dir}/inside.txt"
|
||||
untagged_file = f"{test_dir}/visible.txt"
|
||||
|
||||
# Layout
|
||||
await nc_client.webdav.create_directory(test_dir)
|
||||
await nc_client.webdav.create_directory(tagged_dir)
|
||||
await nc_client.webdav.write_file(tagged_file, b"top secret", "text/plain")
|
||||
await nc_client.webdav.write_file(tagged_dir_child, b"inside private", "text/plain")
|
||||
await nc_client.webdav.write_file(untagged_file, b"public", "text/plain")
|
||||
|
||||
# Tag definition
|
||||
tag = await nc_client.webdav.get_or_create_tag(
|
||||
name=tag_name,
|
||||
user_visible=True,
|
||||
# In production we recommend user_assignable=False; for tests we
|
||||
# keep it True so cleanup via remove_tag_from_file works under
|
||||
# the same credentials.
|
||||
user_assignable=True,
|
||||
)
|
||||
assert tag["id"] is not None, "tag creation did not return an id"
|
||||
|
||||
# Tag assignments — needs file IDs
|
||||
secret_info = await nc_client.webdav.get_file_info(tagged_file)
|
||||
assert secret_info is not None
|
||||
private_info = await nc_client.webdav.get_file_info(tagged_dir)
|
||||
assert private_info is not None
|
||||
|
||||
await nc_client.webdav.assign_tag_to_file(secret_info["id"], tag["id"])
|
||||
await nc_client.webdav.assign_tag_to_file(private_info["id"], tag["id"])
|
||||
|
||||
yield {
|
||||
"tag_name": tag_name,
|
||||
"tag_id": tag["id"],
|
||||
"test_dir": test_dir,
|
||||
"tagged_file": tagged_file,
|
||||
"tagged_dir": tagged_dir,
|
||||
"tagged_dir_child": tagged_dir_child,
|
||||
"untagged_file": untagged_file,
|
||||
"tagged_file_id": secret_info["id"],
|
||||
"tagged_dir_id": private_info["id"],
|
||||
}
|
||||
|
||||
# Cleanup
|
||||
for file_id, tag_id in (
|
||||
(secret_info["id"], tag["id"]),
|
||||
(private_info["id"], tag["id"]),
|
||||
):
|
||||
try:
|
||||
await nc_client.webdav.remove_tag_from_file(file_id, tag_id)
|
||||
except Exception as e:
|
||||
logger.warning("failed to untag file %s: %s", file_id, e)
|
||||
try:
|
||||
await nc_client.webdav.delete_resource(test_dir)
|
||||
except Exception as e:
|
||||
logger.warning("failed to delete %s: %s", test_dir, e)
|
||||
|
||||
|
||||
async def test_get_excluded_file_paths_resolves_real_systemtags(
|
||||
excluded_tag_environment, nc_client: NextcloudClient, mocker
|
||||
):
|
||||
"""``get_excluded_file_paths`` resolves a real Nextcloud system tag
|
||||
to real WebDAV paths via PROPFIND + REPORT.
|
||||
|
||||
Patches ``get_excluded_tag_names`` at the module level so we can
|
||||
target our per-run tag without restarting the MCP server with a
|
||||
custom ``EXCLUDED_TAGS`` env var.
|
||||
"""
|
||||
env = excluded_tag_environment
|
||||
mocker.patch(
|
||||
"nextcloud_mcp_server.server.tag_exclusion.get_excluded_tag_names",
|
||||
return_value=[env["tag_name"]],
|
||||
)
|
||||
|
||||
excluded = await get_excluded_file_paths(nc_client.webdav)
|
||||
|
||||
# Both directly-tagged entries appear (paths are normalised — no
|
||||
# leading slashes).
|
||||
assert env["tagged_file"].lstrip("/") in excluded
|
||||
assert env["tagged_dir"].lstrip("/") in excluded
|
||||
|
||||
# Descendants of the tagged directory are NOT in the resolved set
|
||||
# by themselves — they are blocked at check time via prefix match.
|
||||
assert env["tagged_dir_child"].lstrip("/") not in excluded
|
||||
|
||||
# Untagged sibling is not in the set.
|
||||
assert env["untagged_file"].lstrip("/") not in excluded
|
||||
|
||||
|
||||
async def test_is_path_excluded_against_real_resolved_set(
|
||||
excluded_tag_environment, nc_client: NextcloudClient, mocker
|
||||
):
|
||||
"""End-to-end: real tag → real PROPFIND/REPORT → ``is_path_excluded``
|
||||
classifies real paths correctly. Covers exact match, descendant of
|
||||
tagged directory, and unrelated path against an untagged sibling.
|
||||
"""
|
||||
env = excluded_tag_environment
|
||||
mocker.patch(
|
||||
"nextcloud_mcp_server.server.tag_exclusion.get_excluded_tag_names",
|
||||
return_value=[env["tag_name"]],
|
||||
)
|
||||
|
||||
excluded = await get_excluded_file_paths(nc_client.webdav)
|
||||
|
||||
# Exact match on the tagged file.
|
||||
assert is_path_excluded(env["tagged_file"], excluded) is True
|
||||
|
||||
# Exact match on the tagged directory.
|
||||
assert is_path_excluded(env["tagged_dir"], excluded) is True
|
||||
|
||||
# A child of the tagged directory is excluded by prefix match (this
|
||||
# is the descendant-of-tagged-dir case that ``get_excluded_file_paths``
|
||||
# alone would NOT cover; the prefix matching in ``is_path_excluded``
|
||||
# is what makes recursive exclusion work).
|
||||
assert is_path_excluded(env["tagged_dir_child"], excluded) is True
|
||||
|
||||
# The untagged sibling file is NOT excluded.
|
||||
assert is_path_excluded(env["untagged_file"], excluded) is False
|
||||
|
||||
# A path outside the test directory is NOT excluded.
|
||||
assert is_path_excluded("/this-path-was-never-created", excluded) is False
|
||||
|
||||
|
||||
async def test_feature_disabled_returns_empty_set_against_real_server(
|
||||
excluded_tag_environment, nc_client: NextcloudClient, mocker
|
||||
):
|
||||
"""With ``EXCLUDED_TAGS`` empty, ``get_excluded_file_paths`` is a
|
||||
no-op even when tags exist on the server. Verifies the early-exit
|
||||
short-circuit still holds against a real instance.
|
||||
"""
|
||||
mocker.patch(
|
||||
"nextcloud_mcp_server.server.tag_exclusion.get_excluded_tag_names",
|
||||
return_value=[],
|
||||
)
|
||||
|
||||
excluded = await get_excluded_file_paths(nc_client.webdav)
|
||||
|
||||
assert excluded == set()
|
||||
Reference in New Issue
Block a user