Merge remote-tracking branch 'origin/master' into fix/qdrant-doc-id-keyword-index
This commit is contained in:
Chris Coutinho
@@ -414,3 +414,99 @@ async def test_get_files_by_tag_detects_directories(mocker):
|
||||
call_args = mock_http_client.request.call_args
|
||||
assert "<d:resourcetype/>" in call_args.kwargs["content"]
|
||||
assert "<oc:systemtag>42</oc:systemtag>" in call_args.kwargs["content"]
|
||||
|
||||
|
||||
@pytest.mark.unit
|
||||
async def test_list_directory_decodes_non_ascii_names(mocker):
|
||||
"""list_directory must percent-decode <d:href> for non-ASCII filenames (issue #776).
|
||||
|
||||
RFC 3986 requires <d:href> to be percent-encoded, so a Chinese-named directory
|
||||
arrives as e.g. "%e5%ad%a6%e7%94%9f%e9%82%ae%e7%ae%b1". The MCP response should
|
||||
expose the decoded "学生邮箱", not the encoded form.
|
||||
"""
|
||||
mock_http_client = AsyncMock()
|
||||
client = WebDAVClient(mock_http_client, "testuser")
|
||||
|
||||
# PROPFIND response with one Chinese-named subdirectory and one ASCII file.
|
||||
# The first <d:response> is the parent directory and is skipped by list_directory.
|
||||
xml_content = b"""<?xml version="1.0"?>
|
||||
<d:multistatus xmlns:d="DAV:">
|
||||
<d:response>
|
||||
<d:href>/remote.php/dav/files/testuser/</d:href>
|
||||
<d:propstat>
|
||||
<d:prop>
|
||||
<d:resourcetype><d:collection/></d:resourcetype>
|
||||
</d:prop>
|
||||
</d:propstat>
|
||||
</d:response>
|
||||
<d:response>
|
||||
<d:href>/remote.php/dav/files/testuser/%e5%ad%a6%e7%94%9f%e9%82%ae%e7%ae%b1/</d:href>
|
||||
<d:propstat>
|
||||
<d:prop>
|
||||
<d:displayname>\xe5\xad\xa6\xe7\x94\x9f\xe9\x82\xae\xe7\xae\xb1</d:displayname>
|
||||
<d:resourcetype><d:collection/></d:resourcetype>
|
||||
</d:prop>
|
||||
</d:propstat>
|
||||
</d:response>
|
||||
<d:response>
|
||||
<d:href>/remote.php/dav/files/testuser/notes.txt</d:href>
|
||||
<d:propstat>
|
||||
<d:prop>
|
||||
<d:displayname>notes.txt</d:displayname>
|
||||
<d:getcontentlength>10</d:getcontentlength>
|
||||
<d:getcontenttype>text/plain</d:getcontenttype>
|
||||
<d:resourcetype/>
|
||||
</d:prop>
|
||||
</d:propstat>
|
||||
</d:response>
|
||||
</d:multistatus>"""
|
||||
|
||||
mock_response = AsyncMock()
|
||||
mock_response.content = xml_content
|
||||
mock_response.raise_for_status = mocker.Mock()
|
||||
mock_http_client.request = AsyncMock(return_value=mock_response)
|
||||
|
||||
items = await client.list_directory("")
|
||||
|
||||
by_name = {item["name"]: item for item in items}
|
||||
assert "学生邮箱" in by_name, f"expected decoded Chinese name, got: {list(by_name)}"
|
||||
assert by_name["学生邮箱"]["is_directory"] is True
|
||||
assert by_name["学生邮箱"]["path"] == "学生邮箱"
|
||||
|
||||
# ASCII entries must keep working.
|
||||
assert "notes.txt" in by_name
|
||||
assert by_name["notes.txt"]["is_directory"] is False
|
||||
|
||||
|
||||
@pytest.mark.unit
|
||||
def test_parse_search_response_decodes_non_ascii_paths(mocker):
|
||||
"""_parse_search_response must percent-decode <d:href> for non-ASCII paths (issue #776).
|
||||
|
||||
Affects find_by_name, find_by_type, list_favorites, and search_files: the `path`
|
||||
and `href` fields would otherwise leak percent-encoded URL form to callers.
|
||||
"""
|
||||
mock_http_client = AsyncMock()
|
||||
client = WebDAVClient(mock_http_client, "testuser")
|
||||
|
||||
xml_content = b"""<?xml version="1.0"?>
|
||||
<d:multistatus xmlns:d="DAV:" xmlns:oc="http://owncloud.org/ns">
|
||||
<d:response>
|
||||
<d:href>/remote.php/dav/files/testuser/%e5%ad%a6%e7%94%9f%e9%82%ae%e7%ae%b1/report.pdf</d:href>
|
||||
<d:propstat>
|
||||
<d:prop>
|
||||
<d:displayname>report.pdf</d:displayname>
|
||||
<d:getcontenttype>application/pdf</d:getcontenttype>
|
||||
<d:getcontentlength>1024</d:getcontentlength>
|
||||
<d:resourcetype/>
|
||||
</d:prop>
|
||||
</d:propstat>
|
||||
</d:response>
|
||||
</d:multistatus>"""
|
||||
|
||||
results = client._parse_search_response(xml_content, scope="")
|
||||
|
||||
assert len(results) == 1
|
||||
assert results[0]["path"] == "学生邮箱/report.pdf"
|
||||
assert results[0]["href"] == "/remote.php/dav/files/testuser/学生邮箱/report.pdf"
|
||||
# name comes from <d:displayname>, which is not URL-encoded; sanity-check it.
|
||||
assert results[0]["name"] == "report.pdf"
|
||||
|
||||
@@ -0,0 +1,215 @@
|
||||
"""Unit tests for
|
||||
`nextcloud_mcp_server.search.context.get_chunk_bbox_and_page_from_qdrant`.
|
||||
|
||||
Covers the two paths the helper handles:
|
||||
- Indexed lookup via `chunk_index` (the preferred path post
|
||||
cbcoutinho/astrolabe#75)
|
||||
- Legacy offset fallback via `(chunk_start_offset, chunk_end_offset)`, which
|
||||
may 400 in Qdrant Cloud strict mode
|
||||
|
||||
Plus the regression case from PR #767 review: when the payload has
|
||||
`chunk_bbox` but no `page_number`, the helper must surface that as
|
||||
`(bbox, None)` so callers can preserve their context-derived page_number
|
||||
fallback.
|
||||
"""
|
||||
|
||||
from unittest.mock import AsyncMock, MagicMock, patch
|
||||
|
||||
import pytest
|
||||
|
||||
# Import via the auth surface first to side-step the known
|
||||
# `nextcloud_mcp_server.search.__init__` circular-init issue (same workaround
|
||||
# used in test_chunk_context_offset_gate.py).
|
||||
import nextcloud_mcp_server.auth.viz_routes # noqa: F401
|
||||
from nextcloud_mcp_server.search import context as context_module
|
||||
from nextcloud_mcp_server.search.context import get_chunk_bbox_and_page_from_qdrant
|
||||
|
||||
pytestmark = pytest.mark.unit
|
||||
|
||||
|
||||
def _make_point(payload: dict) -> MagicMock:
|
||||
point = MagicMock()
|
||||
point.payload = payload
|
||||
return point
|
||||
|
||||
|
||||
def _patch_qdrant(scroll_return=None, scroll_side_effect=None):
|
||||
qdrant_client = MagicMock()
|
||||
if scroll_side_effect is not None:
|
||||
qdrant_client.scroll = AsyncMock(side_effect=scroll_side_effect)
|
||||
else:
|
||||
qdrant_client.scroll = AsyncMock(return_value=scroll_return)
|
||||
return patch.object(
|
||||
context_module,
|
||||
"get_qdrant_client",
|
||||
new_callable=AsyncMock,
|
||||
return_value=qdrant_client,
|
||||
), qdrant_client
|
||||
|
||||
|
||||
class TestIndexedPath:
|
||||
"""When `chunk_index` is supplied, the helper must use the indexed
|
||||
`chunk_index` filter (not the offset fallback)."""
|
||||
|
||||
async def test_returns_bbox_and_page_when_payload_complete(self):
|
||||
bbox = [[0, 0, 100, 50]]
|
||||
point = _make_point({"chunk_bbox": bbox, "page_number": 7})
|
||||
ctx, qdrant_client = _patch_qdrant(scroll_return=([point], None))
|
||||
with ctx:
|
||||
result = await get_chunk_bbox_and_page_from_qdrant(
|
||||
user_id="alice",
|
||||
doc_id=42,
|
||||
chunk_index=3,
|
||||
chunk_start=0,
|
||||
chunk_end=100,
|
||||
)
|
||||
|
||||
assert result == (bbox, 7)
|
||||
# One scroll call, and the filter must include chunk_index (not offsets)
|
||||
qdrant_client.scroll.assert_awaited_once()
|
||||
scroll_kwargs = qdrant_client.scroll.await_args.kwargs
|
||||
filter_keys = [c.key for c in scroll_kwargs["scroll_filter"].must]
|
||||
assert "chunk_index" in filter_keys
|
||||
assert "chunk_start_offset" not in filter_keys
|
||||
assert "chunk_end_offset" not in filter_keys
|
||||
|
||||
|
||||
class TestOffsetFallbackPath:
|
||||
"""When `chunk_index` is None, the helper must use the offset filter."""
|
||||
|
||||
async def test_returns_bbox_and_page_when_payload_complete(self):
|
||||
bbox = [[10, 20, 110, 70]]
|
||||
point = _make_point({"chunk_bbox": bbox, "page_number": 2})
|
||||
ctx, qdrant_client = _patch_qdrant(scroll_return=([point], None))
|
||||
with ctx:
|
||||
result = await get_chunk_bbox_and_page_from_qdrant(
|
||||
user_id="bob",
|
||||
doc_id=99,
|
||||
chunk_index=None,
|
||||
chunk_start=500,
|
||||
chunk_end=600,
|
||||
)
|
||||
|
||||
assert result == (bbox, 2)
|
||||
scroll_kwargs = qdrant_client.scroll.await_args.kwargs
|
||||
filter_keys = [c.key for c in scroll_kwargs["scroll_filter"].must]
|
||||
assert "chunk_start_offset" in filter_keys
|
||||
assert "chunk_end_offset" in filter_keys
|
||||
assert "chunk_index" not in filter_keys
|
||||
|
||||
async def test_strict_mode_400_returns_none_pair_and_warns(self, caplog):
|
||||
"""Qdrant Cloud strict mode 400s on unindexed offset filters; the
|
||||
helper must swallow the exception, log a warning, and degrade
|
||||
gracefully so the route can still return chunk text."""
|
||||
ctx, _ = _patch_qdrant(scroll_side_effect=Exception("strict mode: 400"))
|
||||
with ctx, caplog.at_level("WARNING"):
|
||||
result = await get_chunk_bbox_and_page_from_qdrant(
|
||||
user_id="bob",
|
||||
doc_id=99,
|
||||
chunk_index=None,
|
||||
chunk_start=0,
|
||||
chunk_end=100,
|
||||
)
|
||||
|
||||
assert result == (None, None)
|
||||
assert any("Failed to fetch chunk bbox" in r.message for r in caplog.records)
|
||||
|
||||
|
||||
class TestPayloadShape:
|
||||
"""Each payload field can be missing independently — callers rely on
|
||||
that to decide whether to overwrite their fallback values."""
|
||||
|
||||
async def test_empty_points_returns_none_pair(self):
|
||||
ctx, _ = _patch_qdrant(scroll_return=([], None))
|
||||
with ctx:
|
||||
result = await get_chunk_bbox_and_page_from_qdrant(
|
||||
user_id="alice",
|
||||
doc_id=1,
|
||||
chunk_index=0,
|
||||
chunk_start=0,
|
||||
chunk_end=10,
|
||||
)
|
||||
|
||||
assert result == (None, None)
|
||||
|
||||
async def test_missing_page_returns_bbox_only(self):
|
||||
"""Regression for PR #767 review issue #1: when Qdrant returns a
|
||||
point whose payload lacks `page_number`, the helper must return
|
||||
`(bbox, None)` so callers preserve their `chunk_context.page_number`
|
||||
fallback rather than clobbering it to None."""
|
||||
bbox = [[0, 0, 100, 50]]
|
||||
point = _make_point({"chunk_bbox": bbox}) # no page_number
|
||||
ctx, _ = _patch_qdrant(scroll_return=([point], None))
|
||||
with ctx:
|
||||
result = await get_chunk_bbox_and_page_from_qdrant(
|
||||
user_id="alice",
|
||||
doc_id=42,
|
||||
chunk_index=3,
|
||||
chunk_start=0,
|
||||
chunk_end=100,
|
||||
)
|
||||
|
||||
assert result == (bbox, None)
|
||||
|
||||
async def test_missing_bbox_returns_page_only(self):
|
||||
point = _make_point({"page_number": 5}) # no chunk_bbox
|
||||
ctx, _ = _patch_qdrant(scroll_return=([point], None))
|
||||
with ctx:
|
||||
result = await get_chunk_bbox_and_page_from_qdrant(
|
||||
user_id="alice",
|
||||
doc_id=42,
|
||||
chunk_index=3,
|
||||
chunk_start=0,
|
||||
chunk_end=100,
|
||||
)
|
||||
|
||||
assert result == (None, 5)
|
||||
|
||||
async def test_empty_payload_returns_none_pair(self):
|
||||
point = _make_point({})
|
||||
ctx, _ = _patch_qdrant(scroll_return=([point], None))
|
||||
with ctx:
|
||||
result = await get_chunk_bbox_and_page_from_qdrant(
|
||||
user_id="alice",
|
||||
doc_id=42,
|
||||
chunk_index=3,
|
||||
chunk_start=0,
|
||||
chunk_end=100,
|
||||
)
|
||||
|
||||
assert result == (None, None)
|
||||
|
||||
async def test_falsy_payload_treated_as_no_point(self):
|
||||
"""`if not points[0].payload` short-circuits when payload is None or
|
||||
an empty dict, mirroring the original guards in the route handlers."""
|
||||
point = MagicMock()
|
||||
point.payload = None
|
||||
ctx, _ = _patch_qdrant(scroll_return=([point], None))
|
||||
with ctx:
|
||||
result = await get_chunk_bbox_and_page_from_qdrant(
|
||||
user_id="alice",
|
||||
doc_id=42,
|
||||
chunk_index=3,
|
||||
chunk_start=0,
|
||||
chunk_end=100,
|
||||
)
|
||||
|
||||
assert result == (None, None)
|
||||
|
||||
|
||||
class TestExceptionHandling:
|
||||
"""Any error from Qdrant must produce `(None, None)` — never propagate."""
|
||||
|
||||
async def test_indexed_path_exception_returns_none_pair(self, caplog):
|
||||
ctx, _ = _patch_qdrant(scroll_side_effect=RuntimeError("qdrant unavailable"))
|
||||
with ctx, caplog.at_level("WARNING"):
|
||||
result = await get_chunk_bbox_and_page_from_qdrant(
|
||||
user_id="alice",
|
||||
doc_id=42,
|
||||
chunk_index=3,
|
||||
chunk_start=0,
|
||||
chunk_end=100,
|
||||
)
|
||||
|
||||
assert result == (None, None)
|
||||
assert any("Failed to fetch chunk bbox" in r.message for r in caplog.records)
|
||||
@@ -0,0 +1,391 @@
|
||||
"""Unit tests for `nextcloud_mcp_server.search.context.get_chunk_with_context`.
|
||||
|
||||
Focused on the chunk-lookup gate that decides whether to fall back from the
|
||||
indexed `chunk_index` path to the unindexed `(chunk_start, chunk_end)` path.
|
||||
The behaviour matters because Qdrant Cloud's strict mode rejects filters on
|
||||
unindexed fields with HTTP 400 — a fall-through there surfaces a misleading
|
||||
`logger.error` even when the caller's request would correctly resolve as a
|
||||
404 via the file fast-fail.
|
||||
"""
|
||||
|
||||
from unittest.mock import AsyncMock, MagicMock, patch
|
||||
|
||||
import pytest
|
||||
|
||||
# Import via the auth surface first to side-step a known circular-init issue
|
||||
# in `nextcloud_mcp_server.search.__init__` when `search` is imported as the
|
||||
# first entry point (also affects pre-existing tests under tests/unit/search/).
|
||||
import nextcloud_mcp_server.auth.viz_routes # noqa: F401 (init-order fixup)
|
||||
from nextcloud_mcp_server.search import context as context_module
|
||||
from nextcloud_mcp_server.search.context import get_chunk_with_context
|
||||
|
||||
pytestmark = pytest.mark.unit
|
||||
|
||||
|
||||
@pytest.fixture
|
||||
def mock_nc_client() -> MagicMock:
|
||||
return MagicMock()
|
||||
|
||||
|
||||
class TestOffsetFallbackGate:
|
||||
"""When chunk_index is provided AND doc_type=='file', the offset fallback
|
||||
must be skipped — see PR #767 review (🟡 spurious Qdrant error log).
|
||||
"""
|
||||
|
||||
async def test_file_with_chunk_index_skips_offset_fallback_on_miss(
|
||||
self, mock_nc_client
|
||||
):
|
||||
with (
|
||||
patch.object(
|
||||
context_module,
|
||||
"_get_chunk_by_index_from_qdrant",
|
||||
new_callable=AsyncMock,
|
||||
return_value=None,
|
||||
) as mock_indexed,
|
||||
patch.object(
|
||||
context_module,
|
||||
"_get_chunk_from_qdrant",
|
||||
new_callable=AsyncMock,
|
||||
return_value="should-not-be-returned",
|
||||
) as mock_offset,
|
||||
):
|
||||
result = await get_chunk_with_context(
|
||||
nc_client=mock_nc_client,
|
||||
user_id="alice",
|
||||
doc_id=12345,
|
||||
doc_type="file",
|
||||
chunk_start=0,
|
||||
chunk_end=100,
|
||||
chunk_index=3,
|
||||
total_chunks=20,
|
||||
)
|
||||
|
||||
assert result is None, "file fast-fail must return None on Qdrant miss"
|
||||
mock_indexed.assert_awaited_once()
|
||||
mock_offset.assert_not_awaited()
|
||||
|
||||
async def test_note_with_chunk_index_still_uses_offset_fallback(
|
||||
self, mock_nc_client
|
||||
):
|
||||
"""Notes/deck cards keep the offset fallback (cheap, useful for legacy
|
||||
data): the gate is file-specific.
|
||||
"""
|
||||
with (
|
||||
patch.object(
|
||||
context_module,
|
||||
"_get_chunk_by_index_from_qdrant",
|
||||
new_callable=AsyncMock,
|
||||
return_value=None,
|
||||
) as mock_indexed,
|
||||
patch.object(
|
||||
context_module,
|
||||
"_get_chunk_from_qdrant",
|
||||
new_callable=AsyncMock,
|
||||
return_value=None,
|
||||
) as mock_offset,
|
||||
patch.object(
|
||||
context_module,
|
||||
"_fetch_document_text",
|
||||
new_callable=AsyncMock,
|
||||
return_value=None,
|
||||
),
|
||||
):
|
||||
await get_chunk_with_context(
|
||||
nc_client=mock_nc_client,
|
||||
user_id="alice",
|
||||
doc_id=42,
|
||||
doc_type="note",
|
||||
chunk_start=0,
|
||||
chunk_end=10,
|
||||
chunk_index=2,
|
||||
total_chunks=5,
|
||||
)
|
||||
|
||||
mock_indexed.assert_awaited_once()
|
||||
mock_offset.assert_awaited_once()
|
||||
|
||||
async def test_file_without_chunk_index_uses_offset_fallback(self, mock_nc_client):
|
||||
"""Files with no chunk_index supplied still use the offset path —
|
||||
the gate only kicks in once the indexed lookup has been attempted.
|
||||
"""
|
||||
with (
|
||||
patch.object(
|
||||
context_module,
|
||||
"_get_chunk_by_index_from_qdrant",
|
||||
new_callable=AsyncMock,
|
||||
return_value=None,
|
||||
) as mock_indexed,
|
||||
patch.object(
|
||||
context_module,
|
||||
"_get_chunk_from_qdrant",
|
||||
new_callable=AsyncMock,
|
||||
return_value=None,
|
||||
) as mock_offset,
|
||||
):
|
||||
await get_chunk_with_context(
|
||||
nc_client=mock_nc_client,
|
||||
user_id="alice",
|
||||
doc_id=12345,
|
||||
doc_type="file",
|
||||
chunk_start=0,
|
||||
chunk_end=100,
|
||||
chunk_index=None,
|
||||
total_chunks=20,
|
||||
)
|
||||
|
||||
mock_indexed.assert_not_awaited()
|
||||
mock_offset.assert_awaited_once()
|
||||
|
||||
|
||||
class TestNullableChunkIndexPropagation:
|
||||
"""When the caller doesn't supply chunk_index, it must propagate as None
|
||||
through to ChunkContext and the position markers — distinguishing
|
||||
"unknown position" from "actually chunk 0". See PR #767 review (🟡 issue 2).
|
||||
"""
|
||||
|
||||
async def test_fast_path_without_chunk_index_returns_none_in_response(
|
||||
self, mock_nc_client
|
||||
):
|
||||
"""Note retrieved via offset fallback (chunk_index=None) → response
|
||||
chunk_index is None, markers render '?/N', and adjacent fetch is
|
||||
skipped (would otherwise produce wrong neighbours from index 0).
|
||||
"""
|
||||
with (
|
||||
patch.object(
|
||||
context_module,
|
||||
"_get_chunk_by_index_from_qdrant",
|
||||
new_callable=AsyncMock,
|
||||
return_value=None,
|
||||
) as mock_indexed,
|
||||
patch.object(
|
||||
context_module,
|
||||
"_get_chunk_from_qdrant",
|
||||
new_callable=AsyncMock,
|
||||
return_value="matched chunk text",
|
||||
),
|
||||
):
|
||||
result = await get_chunk_with_context(
|
||||
nc_client=mock_nc_client,
|
||||
user_id="alice",
|
||||
doc_id=42,
|
||||
doc_type="note",
|
||||
chunk_start=100,
|
||||
chunk_end=200,
|
||||
chunk_index=None,
|
||||
total_chunks=8,
|
||||
)
|
||||
|
||||
assert result is not None
|
||||
assert result.chunk_index is None, (
|
||||
"chunk_index must propagate as None, not default to 0"
|
||||
)
|
||||
assert "Chunk ?/8" in result.marked_text
|
||||
assert "Chunk 1 of 8" not in result.marked_text
|
||||
# Adjacent fetch must be skipped — index arithmetic from 0 would
|
||||
# query the wrong neighbours when actual position isn't 0.
|
||||
mock_indexed.assert_not_awaited()
|
||||
assert result.has_before_truncation is True
|
||||
assert result.has_after_truncation is True
|
||||
|
||||
async def test_fast_path_with_chunk_index_renders_position_correctly(
|
||||
self, mock_nc_client
|
||||
):
|
||||
"""Counter-positive: when chunk_index is supplied, response carries
|
||||
the value and markers render the explicit "Chunk N of M".
|
||||
"""
|
||||
with (
|
||||
patch.object(
|
||||
context_module,
|
||||
"_get_chunk_by_index_from_qdrant",
|
||||
new_callable=AsyncMock,
|
||||
side_effect=[
|
||||
"current chunk text", # primary lookup
|
||||
"previous chunk text", # adjacent before
|
||||
"next chunk text", # adjacent after
|
||||
],
|
||||
),
|
||||
patch.object(
|
||||
context_module,
|
||||
"_get_chunk_from_qdrant",
|
||||
new_callable=AsyncMock,
|
||||
return_value=None,
|
||||
),
|
||||
):
|
||||
result = await get_chunk_with_context(
|
||||
nc_client=mock_nc_client,
|
||||
user_id="alice",
|
||||
doc_id=42,
|
||||
doc_type="note",
|
||||
chunk_start=0,
|
||||
chunk_end=10,
|
||||
chunk_index=5,
|
||||
total_chunks=20,
|
||||
)
|
||||
|
||||
assert result is not None
|
||||
assert result.chunk_index == 5
|
||||
assert "Chunk 6 of 20" in result.marked_text
|
||||
|
||||
async def test_doc_text_fallback_without_chunk_index_returns_none(
|
||||
self, mock_nc_client
|
||||
):
|
||||
"""Doc-text fallback (Qdrant miss → re-fetch document) must also
|
||||
propagate chunk_index=None into the response so callers can tell
|
||||
the position is unknown.
|
||||
"""
|
||||
with (
|
||||
patch.object(
|
||||
context_module,
|
||||
"_get_chunk_by_index_from_qdrant",
|
||||
new_callable=AsyncMock,
|
||||
return_value=None,
|
||||
),
|
||||
patch.object(
|
||||
context_module,
|
||||
"_get_chunk_from_qdrant",
|
||||
new_callable=AsyncMock,
|
||||
return_value=None,
|
||||
),
|
||||
patch.object(
|
||||
context_module,
|
||||
"_fetch_document_text",
|
||||
new_callable=AsyncMock,
|
||||
return_value="x" * 500,
|
||||
),
|
||||
):
|
||||
result = await get_chunk_with_context(
|
||||
nc_client=mock_nc_client,
|
||||
user_id="alice",
|
||||
doc_id=42,
|
||||
doc_type="note",
|
||||
chunk_start=100,
|
||||
chunk_end=200,
|
||||
chunk_index=None,
|
||||
total_chunks=10,
|
||||
)
|
||||
|
||||
assert result is not None
|
||||
assert result.chunk_index is None
|
||||
assert "Chunk ?/10" in result.marked_text
|
||||
|
||||
|
||||
class TestAdjacentChunkBoundary:
|
||||
"""Boundary cases for the `chunk_index > 0` / `chunk_index < total_chunks - 1`
|
||||
gates that decide whether to fetch the previous / next chunk via Qdrant.
|
||||
See PR #767 review (🟡 missing boundary tests).
|
||||
"""
|
||||
|
||||
async def test_first_chunk_skips_before_fetch_only(self, mock_nc_client):
|
||||
"""At chunk_index=0 the before-fetch gate is closed (no previous
|
||||
chunk exists) but the after-fetch still runs.
|
||||
"""
|
||||
with (
|
||||
patch.object(
|
||||
context_module,
|
||||
"_get_chunk_by_index_from_qdrant",
|
||||
new_callable=AsyncMock,
|
||||
side_effect=[
|
||||
"current chunk text", # primary lookup
|
||||
"next chunk text", # adjacent after only
|
||||
],
|
||||
) as mock_indexed,
|
||||
patch.object(
|
||||
context_module,
|
||||
"_get_chunk_from_qdrant",
|
||||
new_callable=AsyncMock,
|
||||
return_value=None,
|
||||
),
|
||||
):
|
||||
result = await get_chunk_with_context(
|
||||
nc_client=mock_nc_client,
|
||||
user_id="alice",
|
||||
doc_id=42,
|
||||
doc_type="note",
|
||||
chunk_start=0,
|
||||
chunk_end=10,
|
||||
chunk_index=0,
|
||||
total_chunks=10,
|
||||
)
|
||||
|
||||
assert result is not None
|
||||
assert result.chunk_index == 0
|
||||
assert result.has_before_truncation is False
|
||||
assert result.has_after_truncation is False
|
||||
assert mock_indexed.await_count == 2, (
|
||||
"expected primary lookup + after-fetch only (no before-fetch at index 0)"
|
||||
)
|
||||
assert "Chunk 1 of 10" in result.marked_text
|
||||
|
||||
async def test_last_chunk_skips_after_fetch_only(self, mock_nc_client):
|
||||
"""At chunk_index=total_chunks-1 the after-fetch gate is closed (no
|
||||
next chunk exists) but the before-fetch still runs.
|
||||
"""
|
||||
with (
|
||||
patch.object(
|
||||
context_module,
|
||||
"_get_chunk_by_index_from_qdrant",
|
||||
new_callable=AsyncMock,
|
||||
side_effect=[
|
||||
"current chunk text", # primary lookup
|
||||
"previous chunk text", # adjacent before only
|
||||
],
|
||||
) as mock_indexed,
|
||||
patch.object(
|
||||
context_module,
|
||||
"_get_chunk_from_qdrant",
|
||||
new_callable=AsyncMock,
|
||||
return_value=None,
|
||||
),
|
||||
):
|
||||
result = await get_chunk_with_context(
|
||||
nc_client=mock_nc_client,
|
||||
user_id="alice",
|
||||
doc_id=42,
|
||||
doc_type="note",
|
||||
chunk_start=0,
|
||||
chunk_end=10,
|
||||
chunk_index=9,
|
||||
total_chunks=10,
|
||||
)
|
||||
|
||||
assert result is not None
|
||||
assert result.chunk_index == 9
|
||||
assert result.has_before_truncation is False
|
||||
assert result.has_after_truncation is False
|
||||
assert mock_indexed.await_count == 2, (
|
||||
"expected primary lookup + before-fetch only (no after-fetch at last index)"
|
||||
)
|
||||
assert "Chunk 10 of 10" in result.marked_text
|
||||
|
||||
|
||||
class TestPositionMarkers:
|
||||
"""Direct tests for `_insert_position_markers` rendering when chunk_index
|
||||
is None vs explicit.
|
||||
"""
|
||||
|
||||
def test_marker_renders_question_mark_when_chunk_index_is_none(self):
|
||||
text = context_module._insert_position_markers(
|
||||
before_context="",
|
||||
chunk_text="x",
|
||||
after_context="",
|
||||
page_number=None,
|
||||
chunk_index=None,
|
||||
total_chunks=12,
|
||||
has_before_truncation=False,
|
||||
has_after_truncation=False,
|
||||
)
|
||||
assert "Chunk ?/12" in text
|
||||
|
||||
def test_marker_renders_explicit_index_when_supplied(self):
|
||||
text = context_module._insert_position_markers(
|
||||
before_context="",
|
||||
chunk_text="x",
|
||||
after_context="",
|
||||
page_number=3,
|
||||
chunk_index=4,
|
||||
total_chunks=12,
|
||||
has_before_truncation=False,
|
||||
has_after_truncation=False,
|
||||
)
|
||||
assert "Page 3" in text
|
||||
assert "Chunk 5 of 12" in text
|
||||
@@ -258,6 +258,101 @@ class TestChunkContextCredentialPath:
|
||||
assert data["success"] is False
|
||||
assert "failed to fetch chunk context" in data["error"].lower()
|
||||
|
||||
def test_file_doc_type_qdrant_miss_yields_fast_404(self):
|
||||
"""For doc_type=file, a Qdrant miss must surface as 404 immediately
|
||||
(no slow PDF re-parse fallback). Locks the proxy-timeout fix in.
|
||||
|
||||
At the unit level we only assert the response shape; the
|
||||
no-fallback contract itself lives in `search/context.py` and is
|
||||
exercised by chunk-context tests there.
|
||||
"""
|
||||
mock_nc_client = _make_mock_nc_client()
|
||||
|
||||
with (
|
||||
patch(
|
||||
"nextcloud_mcp_server.api.visualization.validate_token_and_get_user",
|
||||
new_callable=AsyncMock,
|
||||
return_value=("testuser", True),
|
||||
),
|
||||
patch(
|
||||
"nextcloud_mcp_server.api.visualization.get_user_client_basic_auth",
|
||||
new_callable=AsyncMock,
|
||||
return_value=mock_nc_client,
|
||||
),
|
||||
patch(
|
||||
"nextcloud_mcp_server.api.visualization.get_chunk_with_context",
|
||||
new_callable=AsyncMock,
|
||||
return_value=None,
|
||||
) as mock_get_chunk,
|
||||
):
|
||||
app = create_test_app()
|
||||
client = TestClient(app)
|
||||
response = client.get(
|
||||
"/api/v1/chunk-context?doc_type=file&doc_id=12345"
|
||||
"&start=0&end=10&chunk_index=3&total_chunks=20",
|
||||
headers={"Authorization": "Bearer test-token"},
|
||||
)
|
||||
assert response.status_code == 404
|
||||
data = response.json()
|
||||
assert data["success"] is False
|
||||
# Confirm the handler called the resolver with doc_type=file
|
||||
# (not a coerced/normalized value) so the fast-fail path engages.
|
||||
kwargs = mock_get_chunk.await_args.kwargs
|
||||
assert kwargs["doc_type"] == "file"
|
||||
assert kwargs["chunk_index"] == 3
|
||||
|
||||
|
||||
class TestChunkContextParameterForwarding:
|
||||
"""Verify new chunk_index / total_chunks query params reach the lookup.
|
||||
|
||||
Regression guard for PR #767: the whole point of the fix is that callers
|
||||
pass chunk_index, and it must arrive at get_chunk_with_context as the
|
||||
primary Qdrant lookup key.
|
||||
"""
|
||||
|
||||
def test_chunk_index_and_total_chunks_forwarded(self):
|
||||
mock_nc_client = _make_mock_nc_client()
|
||||
mock_ctx = _make_mock_chunk_context()
|
||||
mock_ctx.chunk_index = 7
|
||||
mock_ctx.total_chunks = 10
|
||||
|
||||
with (
|
||||
patch(
|
||||
"nextcloud_mcp_server.api.visualization.validate_token_and_get_user",
|
||||
new_callable=AsyncMock,
|
||||
return_value=("testuser", True),
|
||||
),
|
||||
patch(
|
||||
"nextcloud_mcp_server.api.visualization.get_user_client_basic_auth",
|
||||
new_callable=AsyncMock,
|
||||
return_value=mock_nc_client,
|
||||
),
|
||||
patch(
|
||||
"nextcloud_mcp_server.api.visualization.get_chunk_with_context",
|
||||
new_callable=AsyncMock,
|
||||
return_value=mock_ctx,
|
||||
) as mock_get_chunk,
|
||||
):
|
||||
app = create_test_app()
|
||||
client = TestClient(app)
|
||||
response = client.get(
|
||||
"/api/v1/chunk-context?doc_type=note&doc_id=42"
|
||||
"&start=0&end=10&chunk_index=7&total_chunks=10",
|
||||
headers={"Authorization": "Bearer test-token"},
|
||||
)
|
||||
|
||||
assert response.status_code == 200
|
||||
kwargs = mock_get_chunk.await_args.kwargs
|
||||
assert kwargs["chunk_index"] == 7
|
||||
assert kwargs["total_chunks"] == 10
|
||||
|
||||
data = response.json()
|
||||
assert data["chunk_index"] == 7
|
||||
assert data["total_chunks"] == 10
|
||||
# page_number must be present even when None (frontend may scroll
|
||||
# by it for non-file doc types)
|
||||
assert "page_number" in data
|
||||
|
||||
|
||||
class TestChunkContextConfigErrors:
|
||||
"""Tests for configuration failure paths."""
|
||||
|
||||
@@ -0,0 +1,194 @@
|
||||
"""Unit tests for the OAuth-session chunk-context endpoint
|
||||
(`nextcloud_mcp_server.auth.viz_routes.chunk_context_endpoint`).
|
||||
|
||||
Mirrors the regression coverage of
|
||||
`tests/unit/test_management_chunk_context_endpoint.py` (which targets the
|
||||
management API route in `nextcloud_mcp_server.api.visualization`).
|
||||
|
||||
Both routes share the same purpose — fetch chunk text with surrounding
|
||||
context for the viz pane — but live behind different auth surfaces:
|
||||
|
||||
* Management API: OAuth bearer validated by `validate_token_and_get_user`
|
||||
* Viz route: Starlette session auth via `@requires("authenticated")`
|
||||
|
||||
Because of the auth-middleware difference, a separate file is cleaner than
|
||||
mixing both styles into one test module.
|
||||
"""
|
||||
|
||||
from unittest.mock import AsyncMock, MagicMock, patch
|
||||
|
||||
import pytest
|
||||
from starlette.applications import Starlette
|
||||
from starlette.authentication import (
|
||||
AuthCredentials,
|
||||
AuthenticationBackend,
|
||||
SimpleUser,
|
||||
)
|
||||
from starlette.middleware import Middleware
|
||||
from starlette.middleware.authentication import AuthenticationMiddleware
|
||||
from starlette.routing import Route
|
||||
from starlette.testclient import TestClient
|
||||
|
||||
from nextcloud_mcp_server.auth.viz_routes import chunk_context_endpoint
|
||||
|
||||
pytestmark = pytest.mark.unit
|
||||
|
||||
|
||||
class _AlwaysAuthBackend(AuthenticationBackend):
|
||||
"""Stub auth backend: every request is authenticated as `testuser`."""
|
||||
|
||||
async def authenticate(self, conn):
|
||||
return AuthCredentials(["authenticated"]), SimpleUser("testuser")
|
||||
|
||||
|
||||
def _make_app() -> Starlette:
|
||||
return Starlette(
|
||||
routes=[
|
||||
Route("/app/chunk-context", chunk_context_endpoint, methods=["GET"]),
|
||||
],
|
||||
middleware=[
|
||||
Middleware(AuthenticationMiddleware, backend=_AlwaysAuthBackend()),
|
||||
],
|
||||
)
|
||||
|
||||
|
||||
def _make_mock_chunk_context(chunk_text="chunk", before="before", after="after"):
|
||||
"""Mock a ChunkContext dataclass with enough fields for the handler."""
|
||||
ctx = MagicMock()
|
||||
ctx.chunk_text = chunk_text
|
||||
ctx.before_context = before
|
||||
ctx.after_context = after
|
||||
ctx.has_before_truncation = False
|
||||
ctx.has_after_truncation = False
|
||||
ctx.page_number = None
|
||||
ctx.chunk_index = 0
|
||||
ctx.total_chunks = 1
|
||||
return ctx
|
||||
|
||||
|
||||
def _make_mock_nc_client():
|
||||
"""Mock NextcloudClient that supports `async with`."""
|
||||
mock_client = MagicMock()
|
||||
mock_client.__aenter__ = AsyncMock(return_value=mock_client)
|
||||
mock_client.__aexit__ = AsyncMock(return_value=None)
|
||||
return mock_client
|
||||
|
||||
|
||||
def _make_mock_settings(nextcloud_host: str = "http://localhost:8080") -> MagicMock:
|
||||
"""Mock get_settings() return value with the fields the handler reads."""
|
||||
settings = MagicMock()
|
||||
settings.nextcloud_host = nextcloud_host
|
||||
settings.get_collection_name.return_value = "test-collection"
|
||||
return settings
|
||||
|
||||
|
||||
class TestVizChunkContextParameterForwarding:
|
||||
"""Regression guard mirroring TestChunkContextParameterForwarding for the
|
||||
management API: chunk_index / total_chunks must reach get_chunk_with_context.
|
||||
"""
|
||||
|
||||
def test_chunk_index_and_total_chunks_forwarded(self):
|
||||
mock_nc_client = _make_mock_nc_client()
|
||||
mock_ctx = _make_mock_chunk_context()
|
||||
mock_ctx.chunk_index = 7
|
||||
mock_ctx.total_chunks = 10
|
||||
|
||||
with (
|
||||
patch(
|
||||
"nextcloud_mcp_server.auth.viz_routes.get_settings",
|
||||
return_value=_make_mock_settings(),
|
||||
),
|
||||
patch(
|
||||
"nextcloud_mcp_server.auth.viz_routes.get_user_client_basic_auth",
|
||||
new_callable=AsyncMock,
|
||||
return_value=mock_nc_client,
|
||||
),
|
||||
patch(
|
||||
"nextcloud_mcp_server.auth.viz_routes.get_chunk_with_context",
|
||||
new_callable=AsyncMock,
|
||||
return_value=mock_ctx,
|
||||
) as mock_get_chunk,
|
||||
):
|
||||
with TestClient(_make_app()) as client:
|
||||
response = client.get(
|
||||
"/app/chunk-context?doc_type=note&doc_id=42"
|
||||
"&start=0&end=10&chunk_index=7&total_chunks=10"
|
||||
)
|
||||
|
||||
assert response.status_code == 200
|
||||
kwargs = mock_get_chunk.await_args.kwargs
|
||||
assert kwargs["chunk_index"] == 7
|
||||
assert kwargs["total_chunks"] == 10
|
||||
|
||||
data = response.json()
|
||||
assert data["chunk_index"] == 7
|
||||
assert data["total_chunks"] == 10
|
||||
# page_number must be present even when None — the response shape
|
||||
# is unconditional so the frontend can rely on the key existing.
|
||||
assert "page_number" in data
|
||||
|
||||
|
||||
class TestVizChunkContextFile404:
|
||||
"""When get_chunk_with_context returns None for doc_type=file, the route
|
||||
must surface a fast 404 — no slow PDF re-parse fallback. This guards the
|
||||
proxy-timeout fix from PR #767.
|
||||
"""
|
||||
|
||||
def test_file_doc_type_qdrant_miss_yields_fast_404(self):
|
||||
mock_nc_client = _make_mock_nc_client()
|
||||
|
||||
with (
|
||||
patch(
|
||||
"nextcloud_mcp_server.auth.viz_routes.get_settings",
|
||||
return_value=_make_mock_settings(),
|
||||
),
|
||||
patch(
|
||||
"nextcloud_mcp_server.auth.viz_routes.get_user_client_basic_auth",
|
||||
new_callable=AsyncMock,
|
||||
return_value=mock_nc_client,
|
||||
),
|
||||
patch(
|
||||
"nextcloud_mcp_server.auth.viz_routes.get_chunk_with_context",
|
||||
new_callable=AsyncMock,
|
||||
return_value=None,
|
||||
) as mock_get_chunk,
|
||||
):
|
||||
with TestClient(_make_app()) as client:
|
||||
response = client.get(
|
||||
"/app/chunk-context?doc_type=file&doc_id=12345"
|
||||
"&start=0&end=10&chunk_index=3&total_chunks=20"
|
||||
)
|
||||
|
||||
assert response.status_code == 404
|
||||
data = response.json()
|
||||
assert data["success"] is False
|
||||
assert "failed to fetch chunk context" in data["error"].lower()
|
||||
|
||||
kwargs = mock_get_chunk.await_args.kwargs
|
||||
assert kwargs["doc_type"] == "file"
|
||||
assert kwargs["chunk_index"] == 3
|
||||
assert kwargs["total_chunks"] == 20
|
||||
|
||||
|
||||
class TestVizChunkContextValueErrorLogging:
|
||||
"""Verify the route returns 400 for malformed integer params (and does
|
||||
not crash with a 500). The log-level demotion (logger.warning) is
|
||||
asserted indirectly via response shape — log-level itself is not a
|
||||
behaviour the user can observe through HTTP.
|
||||
"""
|
||||
|
||||
def test_invalid_int_param_returns_400(self):
|
||||
with patch(
|
||||
"nextcloud_mcp_server.auth.viz_routes.get_settings",
|
||||
return_value=_make_mock_settings(),
|
||||
):
|
||||
with TestClient(_make_app()) as client:
|
||||
response = client.get(
|
||||
"/app/chunk-context?doc_type=note&doc_id=1"
|
||||
"&start=not-a-number&end=10"
|
||||
)
|
||||
|
||||
assert response.status_code == 400
|
||||
data = response.json()
|
||||
assert data["success"] is False
|
||||
assert "invalid" in data["error"].lower()
|
||||
Reference in New Issue
Block a user