diff --git a/docs/configuration.md b/docs/configuration.md index f6f5d8e0..0214bf50 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -329,6 +329,22 @@ OLLAMA_EMBEDDING_MODEL=all-minilm - **Switching models requires re-embedding** all documents (may take time for large note collections) - **Old collection remains** in Qdrant and can be deleted manually if no longer needed +#### Startup migrations on existing collections + +On the first call to `get_qdrant_client()` against an existing collection, the +server runs two idempotent migrations: + +1. **Payload-index creation** — adds `KEYWORD` payload indexes for `doc_id`, + `user_id`, and `doc_type`. Required by Qdrant for any `FieldCondition` + filter. Cheap; runs even on healthy collections. +2. **`doc_id` backfill** — rewrites any legacy integer `doc_id` payloads to + strings so they match the keyword index. Skipped after a quick sample + shows the collection is already clean. On a dirty collection, the full + scroll runs once and writes are emitted point-by-point — expect a delay + proportional to point count on the first startup after the upgrade. + +Both steps emit INFO-level log lines so operators can track progress. + #### Explicit Override Set `QDRANT_COLLECTION` to use a specific collection name: diff --git a/nextcloud_mcp_server/api/visualization.py b/nextcloud_mcp_server/api/visualization.py index 242c7c13..18bcbd4f 100644 --- a/nextcloud_mcp_server/api/visualization.py +++ b/nextcloud_mcp_server/api/visualization.py @@ -511,8 +511,8 @@ async def get_chunk_context(request: Request) -> JSONResponse: raise ValueError("end must be greater than start") except ValueError as e: return JSONResponse({"success": False, "error": str(e)}, status_code=400) - # Convert doc_id to int if possible (most IDs are int) - doc_id_val: str | int = int(doc_id) if doc_id.isdigit() else doc_id + # doc_id is keyword-indexed in Qdrant as str — pass through verbatim + # (no int coercion; producers always stringify on write). # Get Nextcloud host from OAuth context oauth_ctx = request.app.state.oauth_context @@ -537,7 +537,7 @@ async def get_chunk_context(request: Request) -> JSONResponse: chunk_context = await get_chunk_with_context( nc_client=nc_client, user_id=user_id, - doc_id=doc_id_val, + doc_id=doc_id, doc_type=doc_type, chunk_start=start, chunk_end=end, @@ -570,7 +570,7 @@ async def get_chunk_context(request: Request) -> JSONResponse: must=[ get_placeholder_filter(), FieldCondition( - key="doc_id", match=MatchValue(value=doc_id_val) + key="doc_id", match=MatchValue(value=doc_id) ), FieldCondition( key="user_id", match=MatchValue(value=user_id) diff --git a/nextcloud_mcp_server/auth/viz_routes.py b/nextcloud_mcp_server/auth/viz_routes.py index d072c373..01b8a917 100644 --- a/nextcloud_mcp_server/auth/viz_routes.py +++ b/nextcloud_mcp_server/auth/viz_routes.py @@ -285,7 +285,11 @@ async def vector_visualization_search(request: Request) -> JSONResponse: vector = point.vector if vector is not None and point.payload: - doc_id = point.payload.get("doc_id") + # SearchResult.id is str; coerce payload doc_id to match + # so the tuple lookup below succeeds even on legacy + # int-typed payloads written before normalization. + raw_doc_id = point.payload.get("doc_id") + doc_id = None if raw_doc_id is None else str(raw_doc_id) chunk_start = point.payload.get("chunk_start_offset") chunk_end = point.payload.get("chunk_end_offset") chunk_key = (doc_id, chunk_start, chunk_end) @@ -555,8 +559,7 @@ async def chunk_context_endpoint(request: Request) -> JSONResponse: start = int(start_str) end = int(end_str) - # Convert doc_id to int (all document types use int IDs) - doc_id_int = int(doc_id) + # doc_id is keyword-indexed in Qdrant as str — pass through verbatim. user_id = request.user.display_name settings = get_settings() @@ -580,7 +583,7 @@ async def chunk_context_endpoint(request: Request) -> JSONResponse: chunk_context = await get_chunk_with_context( nc_client=nc_client, user_id=user_id, - doc_id=doc_id_int, + doc_id=doc_id, doc_type=doc_type, chunk_start=start, chunk_end=end, @@ -620,7 +623,7 @@ async def chunk_context_endpoint(request: Request) -> JSONResponse: must=[ get_placeholder_filter(), FieldCondition( - key="doc_id", match=MatchValue(value=doc_id_int) + key="doc_id", match=MatchValue(value=doc_id) ), FieldCondition( key="user_id", match=MatchValue(value=username) diff --git a/nextcloud_mcp_server/models/semantic.py b/nextcloud_mcp_server/models/semantic.py index 4612d26d..bb0091dc 100644 --- a/nextcloud_mcp_server/models/semantic.py +++ b/nextcloud_mcp_server/models/semantic.py @@ -11,10 +11,12 @@ class SemanticSearchResult(BaseModel): id: int = Field( description=( "Document ID. Numeric for all currently indexed types (notes, files, " - "deck cards, news items). The internal SearchResult.id is typed as " - "int|str to leave room for future doc types with string identifiers; " - "the MCP response narrows to int and a future widening here would be " - "a deliberate, breaking-by-design API change." + "deck cards, news items). The internal SearchResult.id is stringified " + "for Qdrant's keyword-indexed doc_id payload; the MCP response narrows " + "back to int via int(r.id). A future doc_type with non-numeric ids " + "would surface here as a TypeError at the narrowing boundary, " + "forcing a deliberate widening of this field rather than a silent " + "API change." ) ) doc_type: str = Field( diff --git a/nextcloud_mcp_server/search/algorithms.py b/nextcloud_mcp_server/search/algorithms.py index 2657bb45..1d14fdf7 100644 --- a/nextcloud_mcp_server/search/algorithms.py +++ b/nextcloud_mcp_server/search/algorithms.py @@ -132,9 +132,12 @@ class SearchResult: """A single search result with metadata and score. Attributes: - id: Document ID. Numeric for indexed types today (notes, files, - deck cards, news items), but typed as ``int | str`` to allow - future doc types that use string identifiers (e.g., file paths). + id: Document ID — always a string. Producers stringify their native + ID before writing to Qdrant so the keyword payload index on + ``doc_id`` matches every point regardless of source doc_type. + Public response models (e.g. ``SemanticSearchResult``) re-narrow + this back to ``int`` at the MCP boundary via ``int(r.id)`` — + see ``server/semantic.py`` for the narrowing site. doc_type: Document type (note, file, calendar, contact, etc.) title: Document title excerpt: Content excerpt showing match context @@ -151,7 +154,7 @@ class SearchResult: point_id: Qdrant point ID for batch vector retrieval (None if not from Qdrant) """ - id: int | str + id: str doc_type: str title: str excerpt: str diff --git a/nextcloud_mcp_server/search/bm25_hybrid.py b/nextcloud_mcp_server/search/bm25_hybrid.py index c5848450..c6f8dfda 100644 --- a/nextcloud_mcp_server/search/bm25_hybrid.py +++ b/nextcloud_mcp_server/search/bm25_hybrid.py @@ -208,8 +208,9 @@ class BM25HybridSearchAlgorithm(SearchAlgorithm): for result in search_response.points: if result.payload is None: continue - # doc_id can be int (files) or str (notes/news_items/deck_cards) — see scanner.py - doc_id = result.payload["doc_id"] + # 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") diff --git a/nextcloud_mcp_server/search/context.py b/nextcloud_mcp_server/search/context.py index cff7d1cc..c249bb73 100644 --- a/nextcloud_mcp_server/search/context.py +++ b/nextcloud_mcp_server/search/context.py @@ -20,7 +20,7 @@ logger = logging.getLogger(__name__) async def _get_chunk_from_qdrant( - user_id: str, doc_id: int, doc_type: str, chunk_start: int, chunk_end: int + user_id: str, doc_id: str, doc_type: str, chunk_start: int, chunk_end: int ) -> str | None: """Retrieve full chunk text from Qdrant payload. @@ -87,7 +87,7 @@ async def _get_chunk_from_qdrant( async def _get_chunk_by_index_from_qdrant( - user_id: str, doc_id: int, doc_type: str, chunk_index: int + user_id: str, doc_id: str, doc_type: str, chunk_index: int ) -> str | None: """Retrieve chunk text by chunk_index from Qdrant payload. @@ -145,7 +145,7 @@ async def _get_chunk_by_index_from_qdrant( async def _get_file_path_from_qdrant( - user_id: str, file_id: int, chunk_start: int, chunk_end: int + user_id: str, file_id: str, chunk_start: int, chunk_end: int ) -> str | None: """Resolve file_id to file_path by querying Qdrant payload. @@ -202,7 +202,7 @@ async def _get_file_path_from_qdrant( async def _get_deck_metadata_from_qdrant( - user_id: str, card_id: int + user_id: str, card_id: str ) -> dict[str, int] | None: """Retrieve board_id and stack_id for a deck card from Qdrant payload. @@ -288,7 +288,7 @@ class ChunkContext: async def get_chunk_with_context( nc_client: NextcloudClient, user_id: str, - doc_id: str | int, + doc_id: str, doc_type: str, chunk_start: int, chunk_end: int, @@ -306,7 +306,7 @@ async def get_chunk_with_context( Args: nc_client: Authenticated Nextcloud client user_id: User ID who owns the document - doc_id: Document ID (int for notes/files) + doc_id: Document ID (str — keyword-indexed in Qdrant payload) doc_type: Type of document ("note", "file", etc.) chunk_start: Character offset where chunk starts chunk_end: Character offset where chunk ends @@ -319,17 +319,10 @@ async def get_chunk_with_context( ChunkContext with expanded context and markers, or None if document cannot be retrieved """ - # Convert doc_id to int for Qdrant query - doc_id_int = ( - int(doc_id) - if isinstance(doc_id, str) and doc_id.isdigit() - else (doc_id if isinstance(doc_id, int) else None) - ) - # Try to get chunk from Qdrant first (fast path) - if doc_id_int is not None: + if doc_id: chunk_text = await _get_chunk_from_qdrant( - user_id, doc_id_int, doc_type, chunk_start, chunk_end + user_id, doc_id, doc_type, chunk_start, chunk_end ) if chunk_text: logger.info( @@ -350,7 +343,7 @@ async def get_chunk_with_context( # Fetch previous chunk if not first chunk if chunk_index > 0: before_chunk = await _get_chunk_by_index_from_qdrant( - user_id, doc_id_int, doc_type, chunk_index - 1 + user_id, doc_id, doc_type, chunk_index - 1 ) if before_chunk: # Remove overlap: the last chunk_overlap chars of previous chunk @@ -371,7 +364,7 @@ async def get_chunk_with_context( # Fetch next chunk if not last chunk if chunk_index < total_chunks - 1: after_chunk = await _get_chunk_by_index_from_qdrant( - user_id, doc_id_int, doc_type, chunk_index + 1 + user_id, doc_id, doc_type, chunk_index + 1 ) if after_chunk: # Remove overlap: the first chunk_overlap chars of next chunk @@ -422,9 +415,10 @@ async def get_chunk_with_context( f"(Qdrant cache miss, possibly legacy data)" ) - # For files, retrieve file_path from Qdrant payload + # 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. resolved_doc_id = doc_id - if doc_type == "file" and isinstance(doc_id, int): + if doc_type == "file": file_path = await _get_file_path_from_qdrant( user_id, doc_id, chunk_start, chunk_end ) @@ -498,7 +492,7 @@ async def get_chunk_with_context( async def _fetch_document_text( - nc_client: NextcloudClient, doc_id: str | int, doc_type: str, user_id: str + nc_client: NextcloudClient, doc_id: str, doc_type: str, user_id: str ) -> str | None: """Fetch full text content of a document. @@ -590,7 +584,7 @@ async def _fetch_document_text( # Try to get board_id/stack_id from Qdrant metadata (O(1) lookup) # Otherwise fall back to iteration (legacy data) card = None - deck_metadata = await _get_deck_metadata_from_qdrant(user_id, int(doc_id)) + deck_metadata = await _get_deck_metadata_from_qdrant(user_id, doc_id) if deck_metadata: # Fast path: Direct lookup with known board_id/stack_id diff --git a/nextcloud_mcp_server/search/semantic.py b/nextcloud_mcp_server/search/semantic.py index c01b0a37..d391b653 100644 --- a/nextcloud_mcp_server/search/semantic.py +++ b/nextcloud_mcp_server/search/semantic.py @@ -140,8 +140,9 @@ class SemanticSearchAlgorithm(SearchAlgorithm): for result in search_response.points: if result.payload is None: continue - # doc_id can be int (notes) or str (files - file paths) - doc_id = result.payload["doc_id"] + # 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") diff --git a/nextcloud_mcp_server/search/verification.py b/nextcloud_mcp_server/search/verification.py index bb3fb678..e5665f3e 100644 --- a/nextcloud_mcp_server/search/verification.py +++ b/nextcloud_mcp_server/search/verification.py @@ -565,7 +565,7 @@ async def verify_search_results( # complete by the time `verify_search_results` returns. if evict_on_missing and inaccessible: - async def evict(doc_id: int | str, doc_type: str) -> None: + async def evict(doc_id: str, doc_type: str) -> None: try: await delete_document_points(doc_id, doc_type, user_id) except Exception as e: diff --git a/nextcloud_mcp_server/server/semantic.py b/nextcloud_mcp_server/server/semantic.py index c1beb208..e8d1a396 100644 --- a/nextcloud_mcp_server/server/semantic.py +++ b/nextcloud_mcp_server/server/semantic.py @@ -224,12 +224,11 @@ def configure_semantic_tools(mcp: FastMCP): search_results = verified_results[:limit] # Convert SearchResult objects to SemanticSearchResult for response. - # SearchResult.id is typed `int | str` for forward-compat with future - # doc_types, but every currently indexed type uses numeric ids and - # the MCP response model narrows to `int`. Casting here makes the - # narrowing explicit and surfaces any future string-id type as a - # loud failure at the boundary instead of silently widening the - # public API. + # SearchResult.id is `str` (Qdrant keyword-indexed payload), but + # every currently indexed type uses numeric ids and the MCP response + # model narrows to `int`. Casting here makes the narrowing explicit + # and surfaces any future non-numeric-id type as a loud failure at + # the boundary instead of silently widening the public API. results = [] for r in search_results: try: @@ -304,7 +303,10 @@ def configure_semantic_tools(mcp: FastMCP): chunk_context = await get_chunk_with_context( nc_client=client, user_id=username, - doc_id=result.id, + # SemanticSearchResult.id is the int-narrowed + # public form; get_chunk_with_context queries + # Qdrant where doc_id is keyword-indexed as str. + doc_id=str(result.id), doc_type=result.doc_type, chunk_start=result.chunk_start_offset, chunk_end=result.chunk_end_offset, diff --git a/nextcloud_mcp_server/vector/eviction.py b/nextcloud_mcp_server/vector/eviction.py index 44874e08..8c7555a7 100644 --- a/nextcloud_mcp_server/vector/eviction.py +++ b/nextcloud_mcp_server/vector/eviction.py @@ -18,7 +18,7 @@ logger = logging.getLogger(__name__) async def delete_document_points( - doc_id: str | int, + doc_id: str, doc_type: str, user_id: str, ) -> None: @@ -29,7 +29,7 @@ async def delete_document_points( not present — Qdrant returns successfully with zero points affected. Args: - doc_id: Document ID (int for notes/files/cards/news, str otherwise) + doc_id: Document ID (str — keyword-indexed in Qdrant payload) doc_type: Document type (note, file, deck_card, news_item) user_id: Owner of the points being evicted diff --git a/nextcloud_mcp_server/vector/placeholder.py b/nextcloud_mcp_server/vector/placeholder.py index fcd12bca..4950c2fc 100644 --- a/nextcloud_mcp_server/vector/placeholder.py +++ b/nextcloud_mcp_server/vector/placeholder.py @@ -31,7 +31,7 @@ from nextcloud_mcp_server.vector.qdrant_client import get_qdrant_client logger = logging.getLogger(__name__) -def _generate_placeholder_id(doc_type: str, doc_id: str | int) -> str: +def _generate_placeholder_id(doc_type: str, doc_id: str) -> str: """Generate deterministic UUID for placeholder point. Args: @@ -46,7 +46,7 @@ def _generate_placeholder_id(doc_type: str, doc_id: str | int) -> str: async def write_placeholder_point( - doc_id: str | int, + doc_id: str, doc_type: str, user_id: str, modified_at: int, @@ -60,7 +60,7 @@ async def write_placeholder_point( processing completes. Args: - doc_id: Document ID (int for notes/files) + doc_id: Document ID (always str — see DocumentTask) doc_type: Document type (note, file, etc.) user_id: User ID who owns the document modified_at: Document modification timestamp @@ -135,7 +135,7 @@ async def write_placeholder_point( async def query_document_metadata( - doc_id: str | int, + doc_id: str, doc_type: str, user_id: str, ) -> dict | None: @@ -185,7 +185,7 @@ async def query_document_metadata( async def delete_placeholder_point( - doc_id: str | int, + doc_id: str, doc_type: str, user_id: str, ) -> None: @@ -230,7 +230,7 @@ async def delete_placeholder_point( async def update_placeholder_status( - doc_id: str | int, + doc_id: str, doc_type: str, user_id: str, status: str, diff --git a/nextcloud_mcp_server/vector/qdrant_client.py b/nextcloud_mcp_server/vector/qdrant_client.py index b2aa0e65..a192eeec 100644 --- a/nextcloud_mcp_server/vector/qdrant_client.py +++ b/nextcloud_mcp_server/vector/qdrant_client.py @@ -3,7 +3,8 @@ import logging from qdrant_client import AsyncQdrantClient, models -from qdrant_client.models import Distance, VectorParams +from qdrant_client.http.exceptions import UnexpectedResponse +from qdrant_client.models import Distance, PayloadSchemaType, VectorParams from nextcloud_mcp_server.config import get_settings from nextcloud_mcp_server.embedding import get_embedding_service @@ -11,10 +12,137 @@ from nextcloud_mcp_server.embedding import get_embedding_service logger = logging.getLogger(__name__) +# Payload fields filtered by exact-match in scanner/processor/placeholder/eviction. +# Qdrant requires a payload index for any field used in a FieldCondition; without +# one, queries fail with HTTP 400 ("Index required but not found"). All three +# carry string values after producer normalization, so a KEYWORD index is the +# correct schema (see ADR notes in commit message). +_KEYWORD_PAYLOAD_FIELDS: tuple[str, ...] = ("doc_id", "user_id", "doc_type") + # Singleton instance _qdrant_client: AsyncQdrantClient | None = None +async def _ensure_keyword_payload_indexes( + client: AsyncQdrantClient, collection_name: str +) -> 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. + """ + for field in _KEYWORD_PAYLOAD_FIELDS: + try: + await client.create_payload_index( + collection_name=collection_name, + field_name=field, + field_schema=PayloadSchemaType.KEYWORD, + wait=True, + ) + 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 + + +async def _backfill_doc_id_to_string( + client: AsyncQdrantClient, collection_name: str +) -> None: + """Rewrite legacy integer doc_id payloads to strings. + + 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. + + Skipped when the first sample batch already contains only str doc_ids. + """ + 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)", + collection_name, + ) + + rewritten = 0 + scanned = 0 + # Qdrant scroll returns next_offset as PointId | None — keep it untyped here + # so the qdrant client's full union (UUID/int/str/PointId) flows through. + next_offset = None + batch_size = 256 + + while True: + points, next_offset = await client.scroll( + collection_name=collection_name, + limit=batch_size, + offset=next_offset, + with_payload=["doc_id"], + with_vectors=False, + ) + if not points: + break + + for point in points: + scanned += 1 + # Qdrant client typing allows None payload even when with_payload + # was requested; defensive default so the type checker is happy. + payload = point.payload or {} + value = payload.get("doc_id") + if value is None or isinstance(value, str): + continue + await client.set_payload( + collection_name=collection_name, + payload={"doc_id": str(value)}, + points=[point.id], + wait=False, + ) + rewritten += 1 + + if next_offset is None: + break + + logger.info( + "doc_id backfill complete: rewrote %d/%d payloads from int to str", + rewritten, + scanned, + ) + + async def get_qdrant_client() -> AsyncQdrantClient: """ Get singleton Qdrant client instance. @@ -110,6 +238,12 @@ async def get_qdrant_client() -> AsyncQdrantClient: f"(dimension={actual_dimension}, model={settings.get_embedding_model_name()})" ) + # Existing collections may pre-date the doc_id normalization / + # payload-index work. Backfill before creating the index so the + # index covers every point. + await _backfill_doc_id_to_string(_qdrant_client, collection_name) + await _ensure_keyword_payload_indexes(_qdrant_client, collection_name) + else: # Collection doesn't exist - create it embedding_model = settings.get_embedding_model_name() @@ -141,5 +275,6 @@ async def get_qdrant_client() -> AsyncQdrantClient: f" Distance: COSINE\n" f"Background sync will index all documents with dense + sparse vectors." ) + await _ensure_keyword_payload_indexes(_qdrant_client, collection_name) return _qdrant_client diff --git a/nextcloud_mcp_server/vector/scanner.py b/nextcloud_mcp_server/vector/scanner.py index 4f4edf3f..6a5a3758 100644 --- a/nextcloud_mcp_server/vector/scanner.py +++ b/nextcloud_mcp_server/vector/scanner.py @@ -48,7 +48,7 @@ class DocumentTask: """Document task for processing queue.""" user_id: str - doc_id: int | str # int for files/notes, str for legacy + doc_id: str # Always str — see vector/qdrant_client.py keyword index doc_type: str # "note", "file", "calendar" operation: str # "index" or "delete" modified_at: int @@ -228,7 +228,7 @@ async def scan_user_documents( ) indexed_doc_ids = { - point.payload["doc_id"] + str(point.payload["doc_id"]) for point in (scroll_result[0] or []) if point.payload is not None } @@ -401,7 +401,7 @@ async def scan_user_documents( ) indexed_file_ids = { - point.payload["doc_id"] + str(point.payload["doc_id"]) for point in (file_scroll_result[0] or []) if point.payload is not None } @@ -456,7 +456,9 @@ async def scan_user_documents( for file_info in tagged_files: # Files are already filtered by MIME type in find_files_by_tag() file_count += 1 - file_id = file_info["id"] # Use numeric file ID, not path + # Normalize file ID to str — Qdrant doc_id payload is keyword-indexed + # and producers across doc_types must agree on a single type. + file_id = str(file_info["id"]) file_path = file_info["path"] # Keep path for logging nextcloud_file_ids.add(file_id) @@ -679,7 +681,7 @@ async def scan_news_items( limit=10000, ) indexed_item_ids = { - point.payload["doc_id"] + str(point.payload["doc_id"]) for point in (scroll_result[0] or []) if point.payload is not None } @@ -858,7 +860,7 @@ async def scan_deck_cards( limit=10000, ) indexed_card_ids = { - point.payload["doc_id"] + str(point.payload["doc_id"]) for point in (scroll_result[0] or []) if point.payload is not None } diff --git a/nextcloud_mcp_server/vector/visualization.py b/nextcloud_mcp_server/vector/visualization.py index 5ffb4bd1..3da8d2e6 100644 --- a/nextcloud_mcp_server/vector/visualization.py +++ b/nextcloud_mcp_server/vector/visualization.py @@ -70,7 +70,10 @@ async def compute_pca_coordinates( vector = point.vector if vector is not None and point.payload: - doc_id = point.payload.get("doc_id") + # SearchResult.id is str; coerce payload doc_id to match so the + # tuple lookup below succeeds even on legacy int-typed payloads. + raw_doc_id = point.payload.get("doc_id") + doc_id = None if raw_doc_id is None else str(raw_doc_id) chunk_start = point.payload.get("chunk_start_offset") chunk_end = point.payload.get("chunk_end_offset") chunk_key = (doc_id, chunk_start, chunk_end) diff --git a/tests/unit/vector/__init__.py b/tests/unit/vector/__init__.py new file mode 100644 index 00000000..e69de29b diff --git a/tests/unit/vector/test_qdrant_client.py b/tests/unit/vector/test_qdrant_client.py new file mode 100644 index 00000000..db232ddd --- /dev/null +++ b/tests/unit/vector/test_qdrant_client.py @@ -0,0 +1,265 @@ +"""Unit tests for Qdrant payload-index helpers and doc_id backfill. + +These cover the startup-time migrations added to ``vector/qdrant_client.py`` +after production HTTP 400 errors revealed that: + +1. The collection had no payload index for ``doc_id``, so any + ``FieldCondition(key="doc_id", ...)`` filter failed at the Qdrant layer. +2. Producers wrote a mix of ``int`` and ``str`` values for ``doc_id``, so a + single keyword index could not have covered both kinds even if it had + existed. + +The fix has three coordinated parts; this module covers the two helpers that +run at startup. Producer-side normalization is exercised by the existing +scanner tests. +""" + +from types import SimpleNamespace +from unittest.mock import call + +import httpx +import pytest +from qdrant_client.http.exceptions import UnexpectedResponse +from qdrant_client.models import PayloadSchemaType + +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, +) + + +def _make_unexpected(status_code: int, body: bytes) -> UnexpectedResponse: + """Build a real UnexpectedResponse for raise_for_status-style branches.""" + return UnexpectedResponse( + status_code=status_code, + reason_phrase="Bad Request", + content=body, + headers=httpx.Headers(), + ) + + +def _record(point_id: int | str, doc_id: int | str | None) -> SimpleNamespace: + """Stand-in for qdrant_client.http.models.Record. + + Tests don't need full Pydantic validation — only ``id`` and ``payload`` + are read by the helpers under test. + """ + payload: dict | None = {"doc_id": doc_id} if doc_id is not None else None + return SimpleNamespace(id=point_id, payload=payload) + + +# --------------------------------------------------------------------------- +# _ensure_keyword_payload_indexes +# --------------------------------------------------------------------------- + + +@pytest.mark.unit +async def test_ensure_keyword_payload_indexes_creates_each_field(mocker): + """Happy path: every field in _KEYWORD_PAYLOAD_FIELDS gets a KEYWORD index.""" + client = mocker.AsyncMock() + + await _ensure_keyword_payload_indexes(client, "test-collection") + + assert client.create_payload_index.await_count == len(_KEYWORD_PAYLOAD_FIELDS) + expected_calls = [ + call( + collection_name="test-collection", + field_name=field, + field_schema=PayloadSchemaType.KEYWORD, + wait=True, + ) + for field in _KEYWORD_PAYLOAD_FIELDS + ] + client.create_payload_index.assert_has_awaits(expected_calls, any_order=False) + + +@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, + ] + + 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.""" + client = mocker.AsyncMock() + client.create_payload_index.side_effect = [ + _make_unexpected( + 400, + b'{"status":{"error":"field \\"doc_id\\" indexed with different schema"}}', + ), + None, + None, + ] + + with caplog.at_level("WARNING", logger="nextcloud_mcp_server.vector.qdrant_client"): + await _ensure_keyword_payload_indexes(client, "test-collection") + + # Loop continued past the failing field; all three were attempted. + assert client.create_payload_index.await_count == len(_KEYWORD_PAYLOAD_FIELDS) + warnings = [r for r in caplog.records if r.levelname == "WARNING"] + assert len(warnings) == 1 + assert "different schema" in warnings[0].getMessage() + + +# --------------------------------------------------------------------------- +# _has_int_doc_id_sample +# --------------------------------------------------------------------------- + + +@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, + ) + + 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.""" + 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 + + +@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() + + +@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 + 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) + ] + + 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. + 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, + ) + client.set_payload.assert_any_await( + collection_name="test-collection", + payload={"doc_id": "200"}, + points=[3], + wait=False, + ) + + +@pytest.mark.unit +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 + ] + + with caplog.at_level("INFO", logger="nextcloud_mcp_server.vector.qdrant_client"): + await _backfill_doc_id_to_string(client, "test-collection") + + 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" + msg = completion_logs[0] + assert "1/2" in msg, f"expected '1/2' rewritten/scanned in {msg!r}" + + +@pytest.mark.unit +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 + ] + + await _backfill_doc_id_to_string(client, "test-collection") + + # Only the int doc_id at point 2 was rewritten; the None-payload point was skipped. + assert client.set_payload.await_count == 1 + client.set_payload.assert_awaited_with( + collection_name="test-collection", + payload={"doc_id": "99"}, + points=[2], + wait=False, + )