fix: paginate tagged-folder SEARCH so the scanner discovers all files
The vector-sync scanner expanded a tagged folder into its PDF descendants via `WebdavClient.find_by_type(scope=dir)` with no result limit. A WebDAV SEARCH with no `<d:nresults>` returns only Nextcloud's default page (~100 on the affected instance), so large tagged folders were silently truncated and most documents were never queued for indexing (e.g. a 220-file folder yielded 100). Add `search_files_all`, which pages the SEARCH to completion. It uses `<d:firstresult>` offset paging where supported and, because Nextcloud 31 ignores offset (verified against a live instance), detects the repeated page and falls back to a single bounded fetch with an explicit large `<d:nresults>`. `find_all_by_type` wraps this and is now used for tagged-folder expansion; `find_by_type` is unchanged for the interactive MCP tools. Crossing `WEBDAV_SEARCH_MAX_RESULTS` logs a warning and increments the new `astrolabe_document_scan_truncated_total` metric, so a coverage cap can never again hide files silently. Scope: this fixes discovery only. Cross-user double-processing of identical shared files (point-ID collisions) is tracked separately. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
Chris Coutinho
co-authored by
Claude Opus 4.8
Claude Opus 4.8
parent
0919513f21
commit
01cb7cf08c
@@ -2,10 +2,11 @@
|
||||
|
||||
Currently covers ``find_files_by_tag``: the wrapper that combines
|
||||
``WebDAVClient.get_tag_by_name``, ``WebDAVClient.get_files_by_tag``, and
|
||||
``WebDAVClient.find_by_type`` to resolve a system tag (and any tagged
|
||||
``WebDAVClient.find_all_by_type`` to resolve a system tag (and any tagged
|
||||
folders) into a flat list of files.
|
||||
"""
|
||||
|
||||
from typing import Any
|
||||
from unittest.mock import AsyncMock
|
||||
|
||||
import pytest
|
||||
@@ -13,13 +14,15 @@ import pytest
|
||||
from nextcloud_mcp_server.client import NextcloudClient, _normalise_search_result
|
||||
|
||||
|
||||
def _make_client() -> NextcloudClient:
|
||||
def _make_client() -> Any:
|
||||
"""Build a NextcloudClient with mocked sub-clients.
|
||||
|
||||
The client constructor opens an httpx session; we don't need it, just
|
||||
a stub instance whose ``webdav`` attribute we can replace.
|
||||
a stub instance whose ``webdav`` attribute we can replace. Returned as
|
||||
``Any`` so tests can freely reassign mocked methods on the sub-clients
|
||||
without fighting the real ``WebDAVClient`` signatures.
|
||||
"""
|
||||
client = NextcloudClient.__new__(NextcloudClient)
|
||||
client: Any = NextcloudClient.__new__(NextcloudClient)
|
||||
client.username = "alice"
|
||||
client.webdav = AsyncMock()
|
||||
return client
|
||||
@@ -99,7 +102,7 @@ class TestFindFilesByTag:
|
||||
result = await client.find_files_by_tag("vector-index")
|
||||
|
||||
assert result == []
|
||||
client.webdav.find_by_type.assert_not_called()
|
||||
client.webdav.find_all_by_type.assert_not_called()
|
||||
|
||||
async def test_directly_tagged_files_pass_through_with_mime_filter(self):
|
||||
client = _make_client()
|
||||
@@ -127,7 +130,7 @@ class TestFindFilesByTag:
|
||||
|
||||
assert {f["id"] for f in result} == {1}
|
||||
# No tagged dirs → no SEARCH walk.
|
||||
client.webdav.find_by_type.assert_not_called()
|
||||
client.webdav.find_all_by_type.assert_not_called()
|
||||
|
||||
async def test_expands_tagged_directory_into_pdf_descendants(self):
|
||||
client = _make_client()
|
||||
@@ -144,7 +147,7 @@ class TestFindFilesByTag:
|
||||
]
|
||||
)
|
||||
# Search inside the folder returns two PDFs.
|
||||
client.webdav.find_by_type = AsyncMock(
|
||||
client.webdav.find_all_by_type = AsyncMock(
|
||||
return_value=[
|
||||
{
|
||||
"file_id": 11,
|
||||
@@ -174,8 +177,8 @@ class TestFindFilesByTag:
|
||||
assert f["last_modified_timestamp"] is not None
|
||||
# SEARCH was scoped to the tagged folder (no leading slash) and
|
||||
# forwarded the requested MIME type as the positional first arg.
|
||||
client.webdav.find_by_type.assert_awaited_once()
|
||||
call_args = client.webdav.find_by_type.await_args
|
||||
client.webdav.find_all_by_type.assert_awaited_once()
|
||||
call_args = client.webdav.find_all_by_type.await_args
|
||||
assert call_args.args[0] == "application/pdf"
|
||||
assert call_args.kwargs["scope"] == "corpus"
|
||||
|
||||
@@ -199,7 +202,7 @@ class TestFindFilesByTag:
|
||||
},
|
||||
]
|
||||
)
|
||||
client.webdav.find_by_type = AsyncMock(
|
||||
client.webdav.find_all_by_type = AsyncMock(
|
||||
return_value=[
|
||||
{
|
||||
"file_id": 11,
|
||||
@@ -244,7 +247,9 @@ class TestFindFilesByTag:
|
||||
},
|
||||
]
|
||||
)
|
||||
client.webdav.find_by_type = AsyncMock(side_effect=RuntimeError("REPORT 500"))
|
||||
client.webdav.find_all_by_type = AsyncMock(
|
||||
side_effect=RuntimeError("REPORT 500")
|
||||
)
|
||||
|
||||
import logging
|
||||
|
||||
@@ -282,10 +287,10 @@ class TestFindFilesByTag:
|
||||
# Without a MIME filter, directory expansion would fan out
|
||||
# uncontrollably — the helper deliberately skips it.
|
||||
assert {f["id"] for f in result} == {7}
|
||||
client.webdav.find_by_type.assert_not_called()
|
||||
client.webdav.find_all_by_type.assert_not_called()
|
||||
|
||||
async def test_skips_descendant_directories_in_search_results(self):
|
||||
"""find_by_type can return collections too (e.g. when the SEARCH
|
||||
"""find_all_by_type can return collections too (e.g. when the SEARCH
|
||||
backend treats a folder's mime type as matching). Those must not
|
||||
slip through and clobber file IDs."""
|
||||
client = _make_client()
|
||||
@@ -300,7 +305,7 @@ class TestFindFilesByTag:
|
||||
}
|
||||
]
|
||||
)
|
||||
client.webdav.find_by_type = AsyncMock(
|
||||
client.webdav.find_all_by_type = AsyncMock(
|
||||
return_value=[
|
||||
{
|
||||
"file_id": 50,
|
||||
|
||||
@@ -0,0 +1,158 @@
|
||||
"""Unit tests for paged WebDAV SEARCH (complete folder discovery).
|
||||
|
||||
These cover ``WebDAVClient.search_files_all`` -- the helper the vector-sync
|
||||
scanner uses to expand a tagged folder into *all* its descendants, rather than
|
||||
just Nextcloud's default ~100-result SEARCH page (which silently truncated large
|
||||
folders and left documents unindexed).
|
||||
|
||||
The behaviour we pin:
|
||||
* offset paging when the server honours ``<d:firstresult>``;
|
||||
* automatic fallback to a single bounded fetch when the server *ignores*
|
||||
offset (the real Nextcloud 31 behaviour -- a page repeats already-seen rows);
|
||||
* a single short page terminates immediately;
|
||||
* crossing ``max_results`` warns + increments the truncation metric;
|
||||
* ``_build_search_xml`` emits the offset element only when asked.
|
||||
"""
|
||||
|
||||
from typing import Any
|
||||
from unittest.mock import AsyncMock
|
||||
|
||||
import httpx
|
||||
import pytest
|
||||
|
||||
from nextcloud_mcp_server.client.webdav import WebDAVClient
|
||||
|
||||
pytestmark = pytest.mark.unit
|
||||
|
||||
|
||||
def _make_client(mocker) -> Any:
|
||||
# Returned as Any so tests can reassign mocked search methods without
|
||||
# tripping ty's invalid-assignment on the real WebDAVClient signatures.
|
||||
client: Any = WebDAVClient(mocker.AsyncMock(spec=httpx.AsyncClient), "alice")
|
||||
return client
|
||||
|
||||
|
||||
def _corpus(n: int) -> list[dict]:
|
||||
return [{"file_id": i, "path": f"/dir/f{i}.pdf"} for i in range(n)]
|
||||
|
||||
|
||||
async def test_single_short_page_returns_all_in_one_call(mocker):
|
||||
"""A folder smaller than the page size resolves in a single SEARCH."""
|
||||
client = _make_client(mocker)
|
||||
client.search_files = AsyncMock(return_value=_corpus(10))
|
||||
|
||||
results = await client.search_files_all(scope="dir", page_size=500)
|
||||
|
||||
assert [r["file_id"] for r in results] == list(range(10))
|
||||
client.search_files.assert_awaited_once()
|
||||
|
||||
|
||||
async def test_offset_honored_pages_through_entire_corpus(mocker):
|
||||
"""When the server honours offset, every page is fetched until exhausted."""
|
||||
client = _make_client(mocker)
|
||||
corpus = _corpus(250)
|
||||
|
||||
async def fake(*, limit, offset=0, **_):
|
||||
return corpus[offset : offset + limit]
|
||||
|
||||
client.search_files = AsyncMock(side_effect=fake)
|
||||
|
||||
results = await client.search_files_all(scope="dir", page_size=100)
|
||||
|
||||
assert [r["file_id"] for r in results] == list(range(250))
|
||||
# 100, 100, 50 -> three pages, no fallback needed
|
||||
assert client.search_files.await_count == 3
|
||||
|
||||
|
||||
async def test_offset_ignored_falls_back_to_single_fetch(mocker):
|
||||
"""Real Nextcloud ignores offset; we must still return the full corpus."""
|
||||
client = _make_client(mocker)
|
||||
corpus = _corpus(250)
|
||||
|
||||
async def fake(*, limit, offset=0, **_):
|
||||
# offset IGNORED: always return the first ``limit`` rows.
|
||||
return corpus[:limit]
|
||||
|
||||
client.search_files = AsyncMock(side_effect=fake)
|
||||
|
||||
results = await client.search_files_all(scope="dir", page_size=100)
|
||||
|
||||
# page0 (0..99) -> page1(offset=100) repeats 0..99 -> detected ->
|
||||
# single fetch with the large ceiling returns everything.
|
||||
assert [r["file_id"] for r in results] == list(range(250))
|
||||
|
||||
|
||||
async def test_truncation_warns_and_increments_metric(mocker):
|
||||
"""Hitting max_results must surface (warn + metric), never silently drop."""
|
||||
client = _make_client(mocker)
|
||||
corpus = _corpus(20)
|
||||
|
||||
async def fake(*, limit, offset=0, **_):
|
||||
return corpus[offset : offset + limit]
|
||||
|
||||
client.search_files = AsyncMock(side_effect=fake)
|
||||
metric = mocker.patch(
|
||||
"nextcloud_mcp_server.client.webdav.document_scan_truncated_total"
|
||||
)
|
||||
|
||||
results = await client.search_files_all(scope="dir", page_size=5, max_results=5)
|
||||
|
||||
assert len(results) == 5
|
||||
metric.inc.assert_called_once()
|
||||
|
||||
|
||||
async def test_offset_ignored_fallback_truncation_metric(mocker):
|
||||
"""The fallback path also reports truncation when it hits the ceiling."""
|
||||
client = _make_client(mocker)
|
||||
corpus = _corpus(40)
|
||||
|
||||
async def fake(*, limit, offset=0, **_):
|
||||
return corpus[:limit] # offset ignored
|
||||
|
||||
client.search_files = AsyncMock(side_effect=fake)
|
||||
metric = mocker.patch(
|
||||
"nextcloud_mcp_server.client.webdav.document_scan_truncated_total"
|
||||
)
|
||||
|
||||
results = await client.search_files_all(scope="dir", page_size=10, max_results=10)
|
||||
|
||||
assert len(results) == 10
|
||||
metric.inc.assert_called_once()
|
||||
|
||||
|
||||
async def test_find_all_by_type_delegates_to_search_files_all(mocker):
|
||||
client = _make_client(mocker)
|
||||
client.search_files_all = AsyncMock(return_value=_corpus(3))
|
||||
|
||||
results = await client.find_all_by_type("application/pdf", scope="dir")
|
||||
|
||||
assert len(results) == 3
|
||||
kwargs = client.search_files_all.await_args.kwargs
|
||||
assert kwargs["scope"] == "dir"
|
||||
assert "fileid" in kwargs["properties"]
|
||||
assert "application/pdf" in kwargs["where_conditions"]
|
||||
|
||||
|
||||
def test_build_search_xml_emits_offset_only_when_set(mocker):
|
||||
client = _make_client(mocker)
|
||||
|
||||
paged = client._build_search_xml(
|
||||
scope="dir",
|
||||
where_conditions="",
|
||||
properties=["fileid"],
|
||||
order_by=None,
|
||||
limit=100,
|
||||
offset=200,
|
||||
)
|
||||
assert "<d:nresults>100</d:nresults>" in paged
|
||||
assert "<d:firstresult>200</d:firstresult>" in paged
|
||||
|
||||
unlimited = client._build_search_xml(
|
||||
scope="dir",
|
||||
where_conditions="",
|
||||
properties=["fileid"],
|
||||
order_by=None,
|
||||
limit=None,
|
||||
offset=None,
|
||||
)
|
||||
assert "<d:limit>" not in unlimited
|
||||
Reference in New Issue
Block a user