From 6aba589a6e102beac0c7c29942fed6a68170ab22 Mon Sep 17 00:00:00 2001 From: Chris Coutinho Date: Fri, 8 May 2026 21:14:28 +0200 Subject: [PATCH] =?UTF-8?q?fix(vector):=20address=20PR=20review=20?= =?UTF-8?q?=E2=80=94=20wait=3DTrue=20backfill,=20batched=20writes,=20searc?= =?UTF-8?q?h=20helper?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses reviewer feedback on PR #773: - Backfill set_payload now uses wait=True to avoid a race where _ensure_keyword_payload_indexes builds the KEYWORD index before fire-and-forget writes have committed, leaving int payloads invisible to filters. - Batch points sharing the same int doc_id into a single set_payload call (one document → many chunks → one round-trip instead of N). - Drop _has_int_doc_id_sample short-circuit. The sample's false-negative window (clean first 256 results, ints further in) is gone; full scroll is the dominant cost on first run anyway. - Simplify _ensure_keyword_payload_indexes: the "already exists" 400 branch was dead code (Qdrant returns 200 on identical re-create); any 400 now logs a warning and continues. - search/context.py: comment the broadened file-type guard. Add explicit not doc_id.isdigit() checks at the top of note/news_item/deck_card branches in _fetch_document_text so malformed payloads surface as warnings instead of being swallowed by the broad except. Also extracts build_search_result_from_point into search/algorithms.py to deduplicate the 71-line payload-extraction loop shared by SemanticSearchAlgorithm and BM25HybridSearchAlgorithm. This fixes SonarQube's quality-gate failure (4.0% new-code duplication, max 3%). Test coverage: - 7 new unit tests for build_search_result_from_point covering missing payload, note/file/deck_card metadata, int doc_id coercion, and metadata_extras merging. - Replace _has_int_doc_id_sample tests with clean-collection no-op and per-batch grouping tests. - Update set_payload assertions from wait=False to wait=True. Co-Authored-By: Claude Opus 4.7 (1M context) --- nextcloud_mcp_server/search/algorithms.py | 66 +++++++- nextcloud_mcp_server/search/bm25_hybrid.py | 76 +++------ nextcloud_mcp_server/search/context.py | 38 ++++- nextcloud_mcp_server/search/semantic.py | 65 ++------ nextcloud_mcp_server/vector/qdrant_client.py | 76 ++++----- tests/unit/search/test_search_result.py | 158 ++++++++++++++++++- tests/unit/vector/test_qdrant_client.py | 158 ++++++++----------- 7 files changed, 385 insertions(+), 252 deletions(-) diff --git a/nextcloud_mcp_server/search/algorithms.py b/nextcloud_mcp_server/search/algorithms.py index 1d14fdf7..7b120393 100644 --- a/nextcloud_mcp_server/search/algorithms.py +++ b/nextcloud_mcp_server/search/algorithms.py @@ -5,7 +5,7 @@ from abc import ABC, abstractmethod from dataclasses import dataclass from typing import Any, Protocol, runtime_checkable -from qdrant_client.models import FieldCondition, Filter, MatchValue +from qdrant_client.models import FieldCondition, Filter, MatchValue, ScoredPoint from nextcloud_mcp_server.config import get_settings from nextcloud_mcp_server.vector.placeholder import get_placeholder_filter @@ -181,6 +181,70 @@ class SearchResult: raise ValueError(f"Score must be non-negative, got {self.score}") +def build_search_result_from_point( + point: ScoredPoint, + *, + metadata_extras: dict[str, Any] | None = None, +) -> SearchResult | None: + """Construct a SearchResult from a Qdrant ScoredPoint payload. + + Returns ``None`` when the payload is missing — callers should skip the + point. The defensive ``str()`` coercion on ``doc_id`` covers legacy int + payloads until the startup backfill has run everywhere (see + ``vector/qdrant_client.py:_backfill_doc_id_to_string``). + + Args: + point: A Qdrant ``ScoredPoint`` from a search response. + metadata_extras: Algorithm-specific metadata merged into the result's + ``metadata`` dict (e.g., ``{"search_method": "bm25_hybrid_rrf"}``). + + Returns: + A populated ``SearchResult``, or ``None`` if ``point.payload`` is + missing. + """ + if point.payload is None: + return None + + doc_id = str(point.payload["doc_id"]) + doc_type = point.payload.get("doc_type", "note") + + metadata: dict[str, Any] = { + "chunk_index": point.payload.get("chunk_index"), + "total_chunks": point.payload.get("total_chunks"), + } + if metadata_extras: + metadata.update(metadata_extras) + + # File-specific metadata for PDF viewer + if doc_type == "file" and (path := point.payload.get("file_path")): + metadata["path"] = path + + # Deck-card metadata for frontend URL construction and verify-on-read + # (ADR-019) — both board_id and stack_id are required to call + # deck.get_card without an O(boards × stacks) iteration fallback. + if doc_type == "deck_card": + if board_id := point.payload.get("board_id"): + metadata["board_id"] = board_id + if stack_id := point.payload.get("stack_id"): + metadata["stack_id"] = stack_id + + return SearchResult( + id=doc_id, + doc_type=doc_type, + title=point.payload.get("title", "Untitled"), + excerpt=point.payload.get("excerpt", ""), + score=point.score, + metadata=metadata, + chunk_start_offset=point.payload.get("chunk_start_offset"), + chunk_end_offset=point.payload.get("chunk_end_offset"), + page_number=point.payload.get("page_number"), + page_count=point.payload.get("page_count"), + chunk_index=point.payload.get("chunk_index", 0), + total_chunks=point.payload.get("total_chunks", 1), + point_id=str(point.id), + ) + + class SearchAlgorithm(ABC): """Abstract base class for search algorithms. diff --git a/nextcloud_mcp_server/search/bm25_hybrid.py b/nextcloud_mcp_server/search/bm25_hybrid.py index c6f8dfda..a400b957 100644 --- a/nextcloud_mcp_server/search/bm25_hybrid.py +++ b/nextcloud_mcp_server/search/bm25_hybrid.py @@ -10,7 +10,11 @@ from nextcloud_mcp_server.config import get_settings from nextcloud_mcp_server.embedding import get_bm25_service, get_embedding_service from nextcloud_mcp_server.observability.metrics import record_qdrant_operation from nextcloud_mcp_server.observability.tracing import trace_operation -from nextcloud_mcp_server.search.algorithms import SearchAlgorithm, SearchResult +from nextcloud_mcp_server.search.algorithms import ( + SearchAlgorithm, + SearchResult, + build_search_result_from_point, +) from nextcloud_mcp_server.vector.placeholder import get_placeholder_filter from nextcloud_mcp_server.vector.qdrant_client import get_qdrant_client @@ -202,66 +206,30 @@ class BM25HybridSearchAlgorithm(SearchAlgorithm): "search.deduplicate", attributes={"dedupe.num_points": len(search_response.points)}, ): - seen_chunks = set() - results = [] + seen_chunks: set[tuple[str, str, Any, Any]] = set() + results: list[SearchResult] = [] + metadata_extras = { + "search_method": f"bm25_hybrid_{self.fusion_name}", + } - for result in search_response.points: - if result.payload is None: + for point in search_response.points: + sr = build_search_result_from_point( + point, metadata_extras=metadata_extras + ) + if sr is None: continue - # doc_id is always str post-normalization, but defensively coerce - # legacy int payloads on read until the backfill has run everywhere. - doc_id = str(result.payload["doc_id"]) - doc_type = result.payload.get("doc_type", "note") - chunk_start = result.payload.get("chunk_start_offset") - chunk_end = result.payload.get("chunk_end_offset") - chunk_key = (doc_id, doc_type, chunk_start, chunk_end) - # Skip if we've already seen this exact chunk + chunk_key = ( + sr.id, + sr.doc_type, + sr.chunk_start_offset, + sr.chunk_end_offset, + ) if chunk_key in seen_chunks: continue - seen_chunks.add(chunk_key) - # Build metadata dict with common fields - metadata = { - "chunk_index": result.payload.get("chunk_index"), - "total_chunks": result.payload.get("total_chunks"), - "search_method": f"bm25_hybrid_{self.fusion_name}", - } - - # Add file-specific metadata for PDF viewer - if doc_type == "file" and (path := result.payload.get("file_path")): - metadata["path"] = path - - # Add deck_card-specific metadata for frontend URL construction - # and verify-on-read (ADR-019) — both board_id and stack_id are - # required to call deck.get_card without an O(boards × stacks) - # iteration fallback. - if doc_type == "deck_card": - if board_id := result.payload.get("board_id"): - metadata["board_id"] = board_id - if stack_id := result.payload.get("stack_id"): - metadata["stack_id"] = stack_id - - # Return unverified results (verification happens at output stage) - results.append( - SearchResult( - id=doc_id, - doc_type=doc_type, - title=result.payload.get("title", "Untitled"), - excerpt=result.payload.get("excerpt", ""), - score=result.score, # Fusion score (RRF or DBSF) - metadata=metadata, - chunk_start_offset=result.payload.get("chunk_start_offset"), - chunk_end_offset=result.payload.get("chunk_end_offset"), - page_number=result.payload.get("page_number"), - page_count=result.payload.get("page_count"), - chunk_index=result.payload.get("chunk_index", 0), - total_chunks=result.payload.get("total_chunks", 1), - point_id=str(result.id), # Qdrant point ID for batch retrieval - ) - ) - + results.append(sr) if len(results) >= limit: break diff --git a/nextcloud_mcp_server/search/context.py b/nextcloud_mcp_server/search/context.py index c249bb73..c3d4827a 100644 --- a/nextcloud_mcp_server/search/context.py +++ b/nextcloud_mcp_server/search/context.py @@ -415,8 +415,13 @@ async def get_chunk_with_context( f"(Qdrant cache miss, possibly legacy data)" ) - # For files, the doc_id is the numeric file ID (as a string) — resolve it - # to a WebDAV path so _fetch_document_text can retrieve the binary content. + # For files, doc_id is always the stringified numeric file ID after + # producer normalization — resolve it to a WebDAV path so + # _fetch_document_text can retrieve the binary content. The previous + # `isinstance(doc_id, int)` guard is no longer needed: file producers + # write str(file_id) and the startup backfill rewrites legacy int + # payloads. If lookup fails (e.g. truly malformed legacy data), the + # caller logs and returns None below — a re-index is the recovery path. resolved_doc_id = doc_id if doc_type == "file": file_path = await _get_file_path_from_qdrant( @@ -506,6 +511,15 @@ async def _fetch_document_text( """ try: if doc_type == "note": + # Note IDs are integers in the Nextcloud API; reject non-numeric + # doc_ids explicitly so a malformed payload surfaces in logs + # rather than getting silently swallowed by `except Exception`. + if not doc_id.isdigit(): + logger.warning( + "Expected numeric note doc_id, got %r — skipping document fetch", + doc_id, + ) + return None # Fetch note by ID note = await nc_client.notes.get_note(note_id=int(doc_id)) # Reconstruct full content as indexed: title + "\n\n" + content @@ -562,6 +576,15 @@ async def _fetch_document_text( ) return None elif doc_type == "news_item": + # News item IDs are integers in the Nextcloud News API; reject + # non-numeric doc_ids explicitly so malformed payloads surface + # rather than getting swallowed by the broad except below. + if not doc_id.isdigit(): + logger.warning( + "Expected numeric news_item doc_id, got %r — skipping document fetch", + doc_id, + ) + return None # Fetch news item by ID item = await nc_client.news.get_item(int(doc_id)) # Reconstruct full content as indexed: title + source + URL + body @@ -580,6 +603,17 @@ async def _fetch_document_text( content_parts.append(body_markdown) return "\n".join(content_parts) elif doc_type == "deck_card": + # Deck card IDs are integers in the Nextcloud Deck API; reject + # non-numeric doc_ids explicitly so malformed payloads surface + # rather than getting swallowed by the broad except below. The + # numeric check covers both the metadata-fast-path (line ~600) + # and the iteration fallback (line ~635). + if not doc_id.isdigit(): + logger.warning( + "Expected numeric deck_card doc_id, got %r — skipping document fetch", + doc_id, + ) + return None # Fetch card from Deck API # Try to get board_id/stack_id from Qdrant metadata (O(1) lookup) # Otherwise fall back to iteration (legacy data) diff --git a/nextcloud_mcp_server/search/semantic.py b/nextcloud_mcp_server/search/semantic.py index d391b653..cb15eade 100644 --- a/nextcloud_mcp_server/search/semantic.py +++ b/nextcloud_mcp_server/search/semantic.py @@ -8,7 +8,11 @@ from qdrant_client.models import FieldCondition, Filter, MatchValue from nextcloud_mcp_server.config import get_settings from nextcloud_mcp_server.embedding import get_embedding_service from nextcloud_mcp_server.observability.metrics import record_qdrant_operation -from nextcloud_mcp_server.search.algorithms import SearchAlgorithm, SearchResult +from nextcloud_mcp_server.search.algorithms import ( + SearchAlgorithm, + SearchResult, + build_search_result_from_point, +) from nextcloud_mcp_server.vector.placeholder import get_placeholder_filter from nextcloud_mcp_server.vector.qdrant_client import get_qdrant_client @@ -134,65 +138,20 @@ class SemanticSearchAlgorithm(SearchAlgorithm): # Deduplicate by (doc_id, doc_type, chunk_start, chunk_end) # This allows multiple chunks from same doc, but removes duplicate chunks - seen_chunks = set() - results = [] + seen_chunks: set[tuple[str, str, Any, Any]] = set() + results: list[SearchResult] = [] - for result in search_response.points: - if result.payload is None: + for point in search_response.points: + sr = build_search_result_from_point(point) + if sr is None: continue - # doc_id is always str post-normalization, but defensively coerce - # legacy int payloads on read until the backfill has run everywhere. - doc_id = str(result.payload["doc_id"]) - doc_type = result.payload.get("doc_type", "note") - chunk_start = result.payload.get("chunk_start_offset") - chunk_end = result.payload.get("chunk_end_offset") - chunk_key = (doc_id, doc_type, chunk_start, chunk_end) - # Skip if we've already seen this exact chunk + chunk_key = (sr.id, sr.doc_type, sr.chunk_start_offset, sr.chunk_end_offset) if chunk_key in seen_chunks: continue - seen_chunks.add(chunk_key) - # Build metadata dict with common fields - metadata = { - "chunk_index": result.payload.get("chunk_index"), - "total_chunks": result.payload.get("total_chunks"), - } - - # Add file-specific metadata for PDF viewer - if doc_type == "file" and (path := result.payload.get("file_path")): - metadata["path"] = path - - # Add deck_card-specific metadata for frontend URL construction - # and verify-on-read (ADR-019) — both board_id and stack_id are - # required to call deck.get_card without an O(boards × stacks) - # iteration fallback. - if doc_type == "deck_card": - if board_id := result.payload.get("board_id"): - metadata["board_id"] = board_id - if stack_id := result.payload.get("stack_id"): - metadata["stack_id"] = stack_id - - # Return unverified results (verification happens at output stage) - results.append( - SearchResult( - id=doc_id, - doc_type=doc_type, - title=result.payload.get("title", "Untitled"), - excerpt=result.payload.get("excerpt", ""), - score=result.score, - metadata=metadata, - chunk_start_offset=result.payload.get("chunk_start_offset"), - chunk_end_offset=result.payload.get("chunk_end_offset"), - page_number=result.payload.get("page_number"), - page_count=result.payload.get("page_count"), - chunk_index=result.payload.get("chunk_index", 0), - total_chunks=result.payload.get("total_chunks", 1), - point_id=str(result.id), # Qdrant point ID for batch retrieval - ) - ) - + results.append(sr) if len(results) >= limit: break diff --git a/nextcloud_mcp_server/vector/qdrant_client.py b/nextcloud_mcp_server/vector/qdrant_client.py index a192eeec..34b14b3a 100644 --- a/nextcloud_mcp_server/vector/qdrant_client.py +++ b/nextcloud_mcp_server/vector/qdrant_client.py @@ -1,6 +1,7 @@ """Qdrant client wrapper.""" import logging +from typing import Any from qdrant_client import AsyncQdrantClient, models from qdrant_client.http.exceptions import UnexpectedResponse @@ -28,8 +29,10 @@ async def _ensure_keyword_payload_indexes( ) -> None: """Create KEYWORD payload indexes for fields used in exact-match filters. - Idempotent: tolerates 'already exists' errors so it can run on every - startup against existing collections. + Idempotent at the Qdrant layer: re-creating an identical index returns + 200, so this can run on every startup. Schema conflicts (a pre-existing + index with a different type) surface as a 400 — log loudly so operators + can intervene, but keep going so the remaining fields still get indexed. """ for field in _KEYWORD_PAYLOAD_FIELDS: try: @@ -41,39 +44,11 @@ async def _ensure_keyword_payload_indexes( ) logger.info("Created KEYWORD payload index on '%s'", field) except UnexpectedResponse as e: - # Qdrant returns 400 if the index already exists with a different - # schema, or simply succeeds if it already matches. Treat - # already-exists as benign; surface schema conflicts loudly. body = getattr(e, "content", b"") or b"" body_text = body.decode("utf-8", errors="replace") - if "already exists" in body_text.lower(): - logger.debug("Payload index on '%s' already exists", field) - else: - logger.warning( - "Failed to create payload index on '%s': %s", field, body_text - ) - - -async def _has_int_doc_id_sample( - client: AsyncQdrantClient, collection_name: str, sample_size: int = 256 -) -> bool: - """Quick sample to decide whether the full backfill scroll is needed. - - Reading the first batch is cheap; if all sampled doc_ids are already str - (the steady-state on healthy collections), we skip the full pass. - """ - points, _ = await client.scroll( - collection_name=collection_name, - limit=sample_size, - with_payload=["doc_id"], - with_vectors=False, - ) - for point in points: - payload = point.payload or {} - value = payload.get("doc_id") - if value is not None and not isinstance(value, str): - return True - return False + logger.warning( + "Failed to create payload index on '%s': %s", field, body_text + ) async def _backfill_doc_id_to_string( @@ -84,18 +59,15 @@ async def _backfill_doc_id_to_string( Producers now uniformly write str(doc_id), but historical points may carry int values from before normalization. A KEYWORD index does not match int payloads, so any leftover int doc_ids would be silently invisible to - filters. Scroll all points and convert in-place. Idempotent. + filters. Scrolls all points once and converts in-place; idempotent (a + second pass over the same collection performs zero writes). - Skipped when the first sample batch already contains only str doc_ids. + Within each scroll batch, points sharing the same int doc_id are batched + into a single ``set_payload`` call to minimize Qdrant round-trips. """ - if not await _has_int_doc_id_sample(client, collection_name): - logger.debug( - "doc_id backfill: sample shows no legacy int payloads; skipping full scan" - ) - return - logger.info( - "Running doc_id backfill on '%s' (this may take a moment for large collections)", + "Scanning '%s' for legacy int doc_id payloads (this is a one-time " + "migration on first start after upgrade)", collection_name, ) @@ -117,6 +89,11 @@ async def _backfill_doc_id_to_string( if not points: break + # Group by stringified value so points sharing a doc_id (one document + # → many chunks) collapse into a single set_payload call. Point IDs + # can be int/str/UUID, so widen the value type to satisfy the qdrant + # client's PointsSelector signature without re-spelling the union. + by_value: dict[str, list[Any]] = {} for point in points: scanned += 1 # Qdrant client typing allows None payload even when with_payload @@ -125,13 +102,20 @@ async def _backfill_doc_id_to_string( value = payload.get("doc_id") if value is None or isinstance(value, str): continue + by_value.setdefault(str(value), []).append(point.id) + + for str_val, point_ids in by_value.items(): + # wait=True is required: _ensure_keyword_payload_indexes runs + # immediately after this function and only indexes committed + # data — fire-and-forget writes would leave int payloads + # invisible to KEYWORD filters. await client.set_payload( collection_name=collection_name, - payload={"doc_id": str(value)}, - points=[point.id], - wait=False, + payload={"doc_id": str_val}, + points=point_ids, + wait=True, ) - rewritten += 1 + rewritten += len(point_ids) if next_offset is None: break diff --git a/tests/unit/search/test_search_result.py b/tests/unit/search/test_search_result.py index c9dbf0b1..52bf15c0 100644 --- a/tests/unit/search/test_search_result.py +++ b/tests/unit/search/test_search_result.py @@ -1,8 +1,22 @@ """Unit tests for SearchResult validation.""" +from types import SimpleNamespace + import pytest -from nextcloud_mcp_server.search.algorithms import SearchResult +from nextcloud_mcp_server.search.algorithms import ( + SearchResult, + build_search_result_from_point, +) + + +def _make_point(point_id, payload, score=0.5): + """Stand-in for qdrant_client.models.ScoredPoint. + + The helper only reads ``id``, ``payload``, and ``score`` — full Pydantic + validation isn't required for unit tests. + """ + return SimpleNamespace(id=point_id, payload=payload, score=score) @pytest.mark.unit @@ -133,3 +147,145 @@ def test_search_result_with_chunk_offsets(): assert result.chunk_start_offset == 100 assert result.chunk_end_offset == 500 + + +# --------------------------------------------------------------------------- +# build_search_result_from_point +# --------------------------------------------------------------------------- + + +@pytest.mark.unit +def test_build_search_result_from_point_returns_none_when_payload_missing(): + """Helper signals the caller to skip the point by returning None.""" + point = _make_point(point_id="p1", payload=None) + + assert build_search_result_from_point(point) is None + + +@pytest.mark.unit +def test_build_search_result_from_point_note_payload(): + """Note-type payload populates the SearchResult fields without metadata extras.""" + point = _make_point( + point_id="p-1", + payload={ + "doc_id": "42", + "doc_type": "note", + "title": "Hello", + "excerpt": "world", + "chunk_start_offset": 0, + "chunk_end_offset": 100, + "chunk_index": 0, + "total_chunks": 2, + }, + score=0.91, + ) + + sr = build_search_result_from_point(point) + + assert sr is not None + assert sr.id == "42" + assert sr.doc_type == "note" + assert sr.title == "Hello" + assert sr.excerpt == "world" + assert sr.score == 0.91 + assert sr.chunk_start_offset == 0 + assert sr.chunk_end_offset == 100 + assert sr.chunk_index == 0 + assert sr.total_chunks == 2 + assert sr.point_id == "p-1" + assert sr.metadata == {"chunk_index": 0, "total_chunks": 2} + + +@pytest.mark.unit +def test_build_search_result_from_point_coerces_int_doc_id_to_str(): + """Legacy int doc_id payloads are stringified defensively.""" + point = _make_point( + point_id=1, + payload={"doc_id": 7, "doc_type": "note"}, + score=0.5, + ) + + sr = build_search_result_from_point(point) + + assert sr is not None + assert sr.id == "7" + + +@pytest.mark.unit +def test_build_search_result_from_point_file_metadata_includes_path(): + """File-type payloads with a file_path attach it under metadata['path'].""" + point = _make_point( + point_id="p-2", + payload={ + "doc_id": "100", + "doc_type": "file", + "file_path": "/Documents/report.pdf", + "page_number": 3, + "page_count": 12, + }, + ) + + sr = build_search_result_from_point(point) + + assert sr is not None + assert sr.doc_type == "file" + assert sr.metadata["path"] == "/Documents/report.pdf" + assert sr.page_number == 3 + assert sr.page_count == 12 + + +@pytest.mark.unit +def test_build_search_result_from_point_deck_card_metadata(): + """Deck-card payloads carry board_id/stack_id forward for verify-on-read.""" + point = _make_point( + point_id="p-3", + payload={ + "doc_id": "55", + "doc_type": "deck_card", + "board_id": 7, + "stack_id": 12, + "title": "Card", + }, + ) + + sr = build_search_result_from_point(point) + + assert sr is not None + assert sr.metadata["board_id"] == 7 + assert sr.metadata["stack_id"] == 12 + + +@pytest.mark.unit +def test_build_search_result_from_point_merges_metadata_extras(): + """metadata_extras override/augment the helper's computed metadata dict.""" + point = _make_point( + point_id="p-4", + payload={"doc_id": "1", "doc_type": "note"}, + ) + + sr = build_search_result_from_point( + point, metadata_extras={"search_method": "bm25_hybrid_rrf"} + ) + + assert sr is not None + assert sr.metadata["search_method"] == "bm25_hybrid_rrf" + # Common fields still present + assert "chunk_index" in sr.metadata + assert "total_chunks" in sr.metadata + + +@pytest.mark.unit +def test_build_search_result_from_point_defaults_when_optional_fields_missing(): + """Missing optional payload keys fall back to documented defaults.""" + point = _make_point(point_id="p-5", payload={"doc_id": "1"}) + + sr = build_search_result_from_point(point) + + assert sr is not None + assert sr.doc_type == "note" # default doc_type + assert sr.title == "Untitled" + assert sr.excerpt == "" + assert sr.chunk_index == 0 + assert sr.total_chunks == 1 + assert sr.chunk_start_offset is None + assert sr.chunk_end_offset is None diff --git a/tests/unit/vector/test_qdrant_client.py b/tests/unit/vector/test_qdrant_client.py index db232ddd..ab5e38c7 100644 --- a/tests/unit/vector/test_qdrant_client.py +++ b/tests/unit/vector/test_qdrant_client.py @@ -26,7 +26,6 @@ from nextcloud_mcp_server.vector.qdrant_client import ( _KEYWORD_PAYLOAD_FIELDS, _backfill_doc_id_to_string, _ensure_keyword_payload_indexes, - _has_int_doc_id_sample, ) @@ -76,32 +75,14 @@ async def test_ensure_keyword_payload_indexes_creates_each_field(mocker): @pytest.mark.unit -async def test_ensure_keyword_payload_indexes_swallows_already_exists(mocker, caplog): - """Idempotent: 'already exists' 400 is logged at debug, not raised.""" - client = mocker.AsyncMock() - # First call succeeds, second raises "already exists", third succeeds — - # exercises the per-field exception handling. - client.create_payload_index.side_effect = [ - None, - _make_unexpected( - 400, b'{"status":{"error":"Index for \\"user_id\\" already exists"}}' - ), - None, - ] +async def test_ensure_keyword_payload_indexes_logs_400_as_warning(mocker, caplog): + """Any 400 from create_payload_index is logged at WARNING and skipped. - with caplog.at_level("DEBUG", logger="nextcloud_mcp_server.vector.qdrant_client"): - await _ensure_keyword_payload_indexes(client, "test-collection") - - assert client.create_payload_index.await_count == len(_KEYWORD_PAYLOAD_FIELDS) - # The "already exists" branch logs at DEBUG; nothing reaches WARNING. - assert not any(record.levelname == "WARNING" for record in caplog.records) - - -@pytest.mark.unit -async def test_ensure_keyword_payload_indexes_logs_unrelated_400_as_warning( - mocker, caplog -): - """Schema conflicts and other 400s are surfaced as warnings, not silenced.""" + Real Qdrant returns 200 when the index already exists with a matching + schema, so 400s indicate a genuine problem (e.g., schema conflict on a + pre-existing index). The loop continues past the failure so the + remaining fields still get indexed. + """ client = mocker.AsyncMock() client.create_payload_index.side_effect = [ _make_unexpected( @@ -123,104 +104,93 @@ async def test_ensure_keyword_payload_indexes_logs_unrelated_400_as_warning( # --------------------------------------------------------------------------- -# _has_int_doc_id_sample +# _backfill_doc_id_to_string # --------------------------------------------------------------------------- @pytest.mark.unit -async def test_has_int_doc_id_sample_returns_true_when_int_present(mocker): - """Sample finds an int — caller should run the full backfill.""" - client = mocker.AsyncMock() - client.scroll.return_value = ( - [_record(1, "abc"), _record(2, 42), _record(3, "xyz")], - None, - ) +async def test_backfill_clean_collection_makes_no_writes(mocker, caplog): + """A collection with only str doc_ids triggers zero set_payload calls. - assert await _has_int_doc_id_sample(client, "c") is True - client.scroll.assert_awaited_once() - - -@pytest.mark.unit -async def test_has_int_doc_id_sample_returns_false_when_all_str(mocker): - """Sample is clean — caller should skip the full scroll.""" + Verifies idempotency: a second pass over an already-migrated collection + is a no-op modulo the read. + """ client = mocker.AsyncMock() client.scroll.return_value = ( [_record(1, "abc"), _record(2, "def")], None, ) - assert await _has_int_doc_id_sample(client, "c") is False + with caplog.at_level("INFO", logger="nextcloud_mcp_server.vector.qdrant_client"): + await _backfill_doc_id_to_string(client, "test-collection") - -@pytest.mark.unit -async def test_has_int_doc_id_sample_handles_empty_collection(mocker): - """Empty collection — nothing to backfill, return False.""" - client = mocker.AsyncMock() - client.scroll.return_value = ([], None) - - assert await _has_int_doc_id_sample(client, "c") is False - - -@pytest.mark.unit -async def test_has_int_doc_id_sample_ignores_missing_payload(mocker): - """Records with no payload don't count as int doc_ids.""" - client = mocker.AsyncMock() - client.scroll.return_value = ( - [_record(1, None), _record(2, "abc")], - None, - ) - - assert await _has_int_doc_id_sample(client, "c") is False - - -# --------------------------------------------------------------------------- -# _backfill_doc_id_to_string -# --------------------------------------------------------------------------- - - -@pytest.mark.unit -async def test_backfill_skips_when_sample_is_clean(mocker): - """Short-circuit: clean sample → no full scroll, no set_payload calls.""" - client = mocker.AsyncMock() - # Sample call returns only str payloads → backfill should not proceed. - client.scroll.return_value = ([_record(1, "abc"), _record(2, "def")], None) - - await _backfill_doc_id_to_string(client, "test-collection") - - # Exactly one scroll (the sample) and zero rewrites. - assert client.scroll.await_count == 1 client.set_payload.assert_not_awaited() + completion_logs = [ + r.getMessage() for r in caplog.records if "backfill complete" in r.getMessage() + ] + assert completion_logs, "expected an INFO log line for backfill completion" + assert "0/2" in completion_logs[0] @pytest.mark.unit async def test_backfill_rewrites_int_doc_ids_to_str(mocker): """Mixed int/str payload across two scroll pages: only ints get rewritten.""" client = mocker.AsyncMock() - # Three scroll calls: - # 1. sample → finds an int, triggers the full pass - # 2. first batch of full scroll → mixed int/str - # 3. second batch → all str, with next_offset=None to terminate + # Two scroll calls: batch 1 is mixed and reports a next_offset; batch 2 + # is mixed with next_offset=None to terminate. client.scroll.side_effect = [ - ([_record(1, 100), _record(2, "abc")], None), # sample - ([_record(1, 100), _record(2, "abc")], "next-offset-123"), # batch 1 - ([_record(3, 200), _record(4, "def")], None), # batch 2 (terminal) + ([_record(1, 100), _record(2, "abc")], "next-offset-123"), + ([_record(3, 200), _record(4, "def")], None), ] await _backfill_doc_id_to_string(client, "test-collection") - # Two rewrites: point 1 (int 100) in batch 1, point 3 (int 200) in batch 2. + # One set_payload per *unique* int value — point 1 (100) and point 3 + # (200) are in different batches with different values, so two calls. assert client.set_payload.await_count == 2 client.set_payload.assert_any_await( collection_name="test-collection", payload={"doc_id": "100"}, points=[1], - wait=False, + wait=True, ) client.set_payload.assert_any_await( collection_name="test-collection", payload={"doc_id": "200"}, points=[3], - wait=False, + wait=True, + ) + + +@pytest.mark.unit +async def test_backfill_batches_points_with_same_doc_id(mocker): + """Multiple points sharing the same int doc_id collapse to one set_payload. + + A single document indexed as multiple chunks all share its doc_id; the + backfill should issue one set_payload call covering the chunk batch. + """ + client = mocker.AsyncMock() + client.scroll.side_effect = [ + ( + [ + _record(10, 42), + _record(11, 42), + _record(12, 42), + _record(13, "already-str"), + ], + None, + ), + ] + + await _backfill_doc_id_to_string(client, "test-collection") + + # All three int-payload points share doc_id=42, so a single call covers them. + assert client.set_payload.await_count == 1 + client.set_payload.assert_awaited_with( + collection_name="test-collection", + payload={"doc_id": "42"}, + points=[10, 11, 12], + wait=True, ) @@ -229,8 +199,7 @@ async def test_backfill_emits_completion_log(mocker, caplog): """Backfill logs final rewritten/scanned counts at INFO.""" client = mocker.AsyncMock() client.scroll.side_effect = [ - ([_record(1, 7)], None), # sample triggers full pass - ([_record(1, 7), _record(2, "x")], None), # single batch, terminal + ([_record(1, 7), _record(2, "x")], None), ] with caplog.at_level("INFO", logger="nextcloud_mcp_server.vector.qdrant_client"): @@ -249,8 +218,7 @@ async def test_backfill_handles_none_payload(mocker): """A point with payload=None is skipped without crashing.""" client = mocker.AsyncMock() client.scroll.side_effect = [ - ([_record(1, 99)], None), # sample triggers full pass - ([_record(1, None), _record(2, 99)], None), # batch with one None payload + ([_record(1, None), _record(2, 99)], None), ] await _backfill_doc_id_to_string(client, "test-collection") @@ -261,5 +229,5 @@ async def test_backfill_handles_none_payload(mocker): collection_name="test-collection", payload={"doc_id": "99"}, points=[2], - wait=False, + wait=True, )