fix(search): cap path_prefixes server-side; unify Iterable typing
Round 3 review follow-ups: - Enforce the folder cap (MAX_PATH_PREFIXES=20) inside normalize_path_prefixes so the REST/viz endpoints are bounded too, not just the MCP tool's Field and the PHP client. Single server-side enforcement point; the MCP tool's Field(max_length=...) now references the same constant. - Widen the SearchAlgorithm ABC and both concrete implementations' path_prefixes param to Iterable[str] | None, matching the widening of build_base_filter_conditions from the prior round. - Add a normalize_path_prefixes cap test. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
co-authored by
Claude Opus 4.8
parent
ea108140ab
commit
9c0c6a0c50
@@ -44,6 +44,12 @@ from nextcloud_mcp_server.vector.placeholder import get_placeholder_filter
|
|||||||
|
|
||||||
logger = logging.getLogger(__name__)
|
logger = logging.getLogger(__name__)
|
||||||
|
|
||||||
|
# Upper bound on the number of folder filters a single search may apply. Caps
|
||||||
|
# the width of the ``Filter(should=[...])`` OR-clause built from path_prefixes
|
||||||
|
# so no caller can degrade query latency with a huge folder list. Mirrored by
|
||||||
|
# the ``Field(max_length=...)`` on the MCP tool and the Astrolabe PHP cap.
|
||||||
|
MAX_PATH_PREFIXES = 20
|
||||||
|
|
||||||
# Short-lived per-user cache for the OCS shares lookup, which otherwise runs on
|
# Short-lived per-user cache for the OCS shares lookup, which otherwise runs on
|
||||||
# every search/viz request. Trades up to this many seconds of share-visibility
|
# every search/viz request. Trades up to this many seconds of share-visibility
|
||||||
# staleness (a freshly-granted share is searchable a little late) for avoiding
|
# staleness (a freshly-granted share is searchable a little late) for avoiding
|
||||||
@@ -195,7 +201,7 @@ def normalize_path_prefixes(
|
|||||||
path_prefixes: Iterable[str] | None = None,
|
path_prefixes: Iterable[str] | None = None,
|
||||||
) -> list[str]:
|
) -> list[str]:
|
||||||
"""Merge the legacy single ``path_prefix`` and list ``path_prefixes`` into
|
"""Merge the legacy single ``path_prefix`` and list ``path_prefixes`` into
|
||||||
one clean, de-duplicated list of folder filters.
|
one clean, de-duplicated, bounded list of folder filters.
|
||||||
|
|
||||||
Blank/whitespace entries are dropped (an empty UI field must mean "no
|
Blank/whitespace entries are dropped (an empty UI field must mean "no
|
||||||
filter", not "match everything"), surrounding whitespace is stripped, and
|
filter", not "match everything"), surrounding whitespace is stripped, and
|
||||||
@@ -203,13 +209,21 @@ def normalize_path_prefixes(
|
|||||||
the pre-ADR-027-Phase-2 single-value contract working while callers migrate
|
the pre-ADR-027-Phase-2 single-value contract working while callers migrate
|
||||||
to the multi-folder list.
|
to the multi-folder list.
|
||||||
|
|
||||||
|
The result is capped at ``MAX_PATH_PREFIXES`` so no caller — the MCP tool,
|
||||||
|
the REST/viz endpoints, or a misbehaving client — can build an unbounded
|
||||||
|
``Filter(should=[...])`` OR-clause that would widen every Qdrant query.
|
||||||
|
This is the single server-side enforcement point (the MCP tool also
|
||||||
|
declares ``Field(max_length=...)`` for an earlier, clearer validation
|
||||||
|
error, and the Astrolabe PHP controller caps before forwarding).
|
||||||
|
|
||||||
Args:
|
Args:
|
||||||
path_prefix: Legacy single folder filter (deprecated; folded into the
|
path_prefix: Legacy single folder filter (deprecated; folded into the
|
||||||
returned list).
|
returned list).
|
||||||
path_prefixes: Zero or more folder filters.
|
path_prefixes: Zero or more folder filters.
|
||||||
|
|
||||||
Returns:
|
Returns:
|
||||||
Ordered, de-duplicated list of non-empty folder filters (possibly empty).
|
Ordered, de-duplicated list of non-empty folder filters, capped at
|
||||||
|
``MAX_PATH_PREFIXES`` (possibly empty).
|
||||||
"""
|
"""
|
||||||
raw: list[str] = []
|
raw: list[str] = []
|
||||||
if path_prefix:
|
if path_prefix:
|
||||||
@@ -227,7 +241,7 @@ def normalize_path_prefixes(
|
|||||||
if stripped and stripped not in seen:
|
if stripped and stripped not in seen:
|
||||||
seen.add(stripped)
|
seen.add(stripped)
|
||||||
cleaned.append(stripped)
|
cleaned.append(stripped)
|
||||||
return cleaned
|
return cleaned[:MAX_PATH_PREFIXES]
|
||||||
|
|
||||||
|
|
||||||
def build_base_filter_conditions(
|
def build_base_filter_conditions(
|
||||||
|
|||||||
@@ -2,6 +2,7 @@
|
|||||||
|
|
||||||
import logging
|
import logging
|
||||||
from abc import ABC, abstractmethod
|
from abc import ABC, abstractmethod
|
||||||
|
from collections.abc import Iterable
|
||||||
from dataclasses import dataclass
|
from dataclasses import dataclass
|
||||||
from typing import Any, Protocol, runtime_checkable
|
from typing import Any, Protocol, runtime_checkable
|
||||||
|
|
||||||
@@ -300,7 +301,7 @@ class SearchAlgorithm(ABC):
|
|||||||
modified_after: int | None = None,
|
modified_after: int | None = None,
|
||||||
modified_before: int | None = None,
|
modified_before: int | None = None,
|
||||||
path_prefix: str | None = None,
|
path_prefix: str | None = None,
|
||||||
path_prefixes: list[str] | None = None,
|
path_prefixes: Iterable[str] | None = None,
|
||||||
**kwargs: Any,
|
**kwargs: Any,
|
||||||
) -> list[SearchResult]:
|
) -> list[SearchResult]:
|
||||||
"""Execute search with the given parameters.
|
"""Execute search with the given parameters.
|
||||||
|
|||||||
@@ -1,6 +1,7 @@
|
|||||||
"""BM25 hybrid search algorithm using Qdrant native RRF fusion."""
|
"""BM25 hybrid search algorithm using Qdrant native RRF fusion."""
|
||||||
|
|
||||||
import logging
|
import logging
|
||||||
|
from collections.abc import Iterable
|
||||||
from typing import Any
|
from typing import Any
|
||||||
|
|
||||||
from qdrant_client import models
|
from qdrant_client import models
|
||||||
@@ -75,7 +76,7 @@ class BM25HybridSearchAlgorithm(SearchAlgorithm):
|
|||||||
modified_after: int | None = None,
|
modified_after: int | None = None,
|
||||||
modified_before: int | None = None,
|
modified_before: int | None = None,
|
||||||
path_prefix: str | None = None,
|
path_prefix: str | None = None,
|
||||||
path_prefixes: list[str] | None = None,
|
path_prefixes: Iterable[str] | None = None,
|
||||||
**kwargs: Any,
|
**kwargs: Any,
|
||||||
) -> list[SearchResult]:
|
) -> list[SearchResult]:
|
||||||
"""
|
"""
|
||||||
|
|||||||
@@ -1,6 +1,7 @@
|
|||||||
"""Semantic search algorithm using vector similarity (Qdrant)."""
|
"""Semantic search algorithm using vector similarity (Qdrant)."""
|
||||||
|
|
||||||
import logging
|
import logging
|
||||||
|
from collections.abc import Iterable
|
||||||
from typing import Any
|
from typing import Any
|
||||||
|
|
||||||
from qdrant_client.models import FieldCondition, Filter, MatchAny
|
from qdrant_client.models import FieldCondition, Filter, MatchAny
|
||||||
@@ -55,7 +56,7 @@ class SemanticSearchAlgorithm(SearchAlgorithm):
|
|||||||
modified_after: int | None = None,
|
modified_after: int | None = None,
|
||||||
modified_before: int | None = None,
|
modified_before: int | None = None,
|
||||||
path_prefix: str | None = None,
|
path_prefix: str | None = None,
|
||||||
path_prefixes: list[str] | None = None,
|
path_prefixes: Iterable[str] | None = None,
|
||||||
**kwargs: Any,
|
**kwargs: Any,
|
||||||
) -> list[SearchResult]:
|
) -> list[SearchResult]:
|
||||||
"""Execute semantic search using vector similarity.
|
"""Execute semantic search using vector similarity.
|
||||||
|
|||||||
@@ -33,6 +33,7 @@ from nextcloud_mcp_server.observability.metrics import (
|
|||||||
instrument_tool,
|
instrument_tool,
|
||||||
)
|
)
|
||||||
from nextcloud_mcp_server.search.access_filter import (
|
from nextcloud_mcp_server.search.access_filter import (
|
||||||
|
MAX_PATH_PREFIXES,
|
||||||
list_accessible_owners,
|
list_accessible_owners,
|
||||||
normalize_path_prefixes,
|
normalize_path_prefixes,
|
||||||
)
|
)
|
||||||
@@ -102,14 +103,14 @@ def configure_semantic_tools(mcp: FastMCP):
|
|||||||
path_prefixes: Annotated[
|
path_prefixes: Annotated[
|
||||||
list[str] | None,
|
list[str] | None,
|
||||||
Field(
|
Field(
|
||||||
max_length=20,
|
max_length=MAX_PATH_PREFIXES,
|
||||||
description=(
|
description=(
|
||||||
"Restrict to files under any of these folders/paths "
|
"Restrict to files under any of these folders/paths "
|
||||||
"(e.g. ['/Projects/Reports', '/Shared/Specs']). Folders are "
|
"(e.g. ['/Projects/Reports', '/Shared/Specs']). Folders are "
|
||||||
"OR-ed together. Matches the file_path of indexed files "
|
"OR-ed together. Matches the file_path of indexed files "
|
||||||
"only, so setting it implicitly limits results to files. "
|
"only, so setting it implicitly limits results to files. "
|
||||||
"Capped at 20 folders to bound the OR-filter width. "
|
f"Capped at {MAX_PATH_PREFIXES} folders to bound the "
|
||||||
"None or empty = no path filter."
|
"OR-filter width. None or empty = no path filter."
|
||||||
),
|
),
|
||||||
),
|
),
|
||||||
] = None,
|
] = None,
|
||||||
|
|||||||
@@ -9,6 +9,7 @@ from qdrant_client.models import FieldCondition, Filter, MatchText, Range
|
|||||||
|
|
||||||
from nextcloud_mcp_server.search import access_filter
|
from nextcloud_mcp_server.search import access_filter
|
||||||
from nextcloud_mcp_server.search.access_filter import (
|
from nextcloud_mcp_server.search.access_filter import (
|
||||||
|
MAX_PATH_PREFIXES,
|
||||||
build_base_filter_conditions,
|
build_base_filter_conditions,
|
||||||
build_ownership_filter,
|
build_ownership_filter,
|
||||||
clear_accessible_owners_cache,
|
clear_accessible_owners_cache,
|
||||||
@@ -377,3 +378,12 @@ class TestNormalizePathPrefixes:
|
|||||||
" /Projects ", ["/Archive", "/Projects", " ", "/Specs"]
|
" /Projects ", ["/Archive", "/Projects", " ", "/Specs"]
|
||||||
)
|
)
|
||||||
assert result == ["/Projects", "/Archive", "/Specs"]
|
assert result == ["/Projects", "/Archive", "/Specs"]
|
||||||
|
|
||||||
|
@pytest.mark.unit
|
||||||
|
def test_caps_at_max_path_prefixes(self) -> None:
|
||||||
|
# A huge list is truncated to MAX_PATH_PREFIXES so no caller can build
|
||||||
|
# an unbounded OR-clause; the first N (order-preserving) survive.
|
||||||
|
folders = [f"/dir{i}" for i in range(MAX_PATH_PREFIXES + 30)]
|
||||||
|
result = normalize_path_prefixes(None, folders)
|
||||||
|
assert len(result) == MAX_PATH_PREFIXES
|
||||||
|
assert result == folders[:MAX_PATH_PREFIXES]
|
||||||
|
|||||||
Reference in New Issue
Block a user