diff --git a/docs/configuration.md b/docs/configuration.md index 0214bf50..bd7dd87f 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -337,14 +337,24 @@ 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. +2. **`doc_id` backfill** — scans the collection once and rewrites any + legacy integer `doc_id` payloads to strings so they match the keyword + index. Idempotent: on a clean collection (all `doc_id` values already + `str`), the scroll runs but emits zero writes. On the first start after + the upgrade, expect a delay proportional to point count while writes + are issued. Both steps emit INFO-level log lines so operators can track progress. +> **Operator note:** if the server logs `TypeError: SemanticSearchResult.id +> must be int-convertible` after upgrading, this indicates a `doc_type` +> with non-numeric ids has been indexed but the public response model +> (`SemanticSearchResult.id: int`) has not been widened to accept strings. +> Semantic search itself is not broken — the boundary cast in +> `server/semantic.py` is failing loudly on purpose so the discrepancy is +> caught early. Either widen the public model's `id` field or convert the +> id at the verifier layer. + #### Explicit Override Set `QDRANT_COLLECTION` to use a specific collection name: @@ -426,9 +436,16 @@ DOCUMENT_CHUNK_OVERLAP=50 # Overlapping words between chunks (defaul ### Embedding Service Configuration -The server uses an embedding service to generate vector representations. Two options are available: +The server picks an embedding provider via auto-detection. Priority order +(see `nextcloud_mcp_server/providers/registry.py`): -#### Ollama (Recommended) +1. **Bedrock** — if `AWS_REGION` or `BEDROCK_EMBEDDING_MODEL` is set +2. **OpenAI** — if `OPENAI_API_KEY` is set +3. **Mistral** — if `MISTRAL_API_KEY` is set +4. **Ollama** — if `OLLAMA_BASE_URL` is set +5. **Simple** — fallback when nothing else is configured + +#### Ollama (Recommended for self-hosted) Use a local Ollama instance for embeddings: @@ -438,9 +455,52 @@ OLLAMA_EMBEDDING_MODEL=nomic-embed-text # Default model OLLAMA_VERIFY_SSL=true # Verify SSL certificates ``` +#### OpenAI + +Hosted OpenAI embeddings (or any OpenAI-compatible API via `OPENAI_BASE_URL`): + +```dotenv +OPENAI_API_KEY=sk-... +OPENAI_EMBEDDING_MODEL=text-embedding-3-small # default +# OPENAI_BASE_URL=https://models.github.ai/inference # optional +``` + +#### Mistral + +Hosted Mistral embeddings. Requires a Mistral API key from +[console.mistral.ai](https://console.mistral.ai). Currently embeddings only +(no text generation). + +```dotenv +MISTRAL_API_KEY=... +MISTRAL_EMBEDDING_MODEL=mistral-embed # default; produces 1024-dim vectors +# MISTRAL_BASE_URL=https://api.mistral.ai # optional override (proxies, on-prem) +``` + +Switching to or from Mistral forces a new Qdrant collection because the +collection name encodes the model (see "Qdrant Collection Naming" above). + +#### Amazon Bedrock + +Bedrock provides hosted embedding models (Titan, Cohere) and uses the AWS +credential chain (env vars, profiles, or IAM role): + +```dotenv +AWS_REGION=us-east-1 +BEDROCK_EMBEDDING_MODEL=amazon.titan-embed-text-v2:0 +# AWS_ACCESS_KEY_ID / AWS_SECRET_ACCESS_KEY are optional — boto3 will use +# the standard credential chain if not set. +``` + #### Simple Embedding Provider (Fallback) -If `OLLAMA_BASE_URL` is not set, the server uses a simple random embedding provider for testing. This is **not suitable for production** as it generates random embeddings with no semantic meaning. +If no provider env var is set, the server falls back to a simple deterministic +embedding provider for testing. This is **not suitable for production** as +its embeddings have no semantic meaning. + +```dotenv +SIMPLE_EMBEDDING_DIMENSION=384 # optional; default 384 +``` ### Document Chunking Configuration @@ -549,7 +609,21 @@ equivalent.** Operators who need a runtime toggle should open an issue. | `VECTOR_SYNC_QUEUE_MAX_SIZE` | ⚠️ Optional | `10000` | Max queued documents | | `OLLAMA_BASE_URL` | ⚠️ Optional | - | Ollama API endpoint for embeddings | | `OLLAMA_EMBEDDING_MODEL` | ⚠️ Optional | `nomic-embed-text` | Embedding model to use | +| `OLLAMA_GENERATION_MODEL` | ⚠️ Optional | - | Ollama model for text generation | | `OLLAMA_VERIFY_SSL` | ⚠️ Optional | `true` | Verify SSL certificates | +| `OPENAI_API_KEY` | ⚠️ Optional | - | OpenAI API key (selects OpenAI provider) | +| `OPENAI_BASE_URL` | ⚠️ Optional | - | OpenAI base URL override (for compatible APIs) | +| `OPENAI_EMBEDDING_MODEL` | ⚠️ Optional | `text-embedding-3-small` | OpenAI embedding model | +| `OPENAI_GENERATION_MODEL` | ⚠️ Optional | - | OpenAI model for text generation | +| `MISTRAL_API_KEY` | ⚠️ Optional | - | Mistral API key (selects Mistral provider) | +| `MISTRAL_EMBEDDING_MODEL` | ⚠️ Optional | `mistral-embed` | Mistral embedding model (1024-dim) | +| `MISTRAL_BASE_URL` | ⚠️ Optional | - | Mistral base URL override (proxies, on-prem) | +| `AWS_REGION` | ⚠️ Optional | - | AWS region (selects Bedrock provider) | +| `AWS_ACCESS_KEY_ID` | ⚠️ Optional | - | AWS access key (boto3 credential chain fallback) | +| `AWS_SECRET_ACCESS_KEY` | ⚠️ Optional | - | AWS secret key (boto3 credential chain fallback) | +| `BEDROCK_EMBEDDING_MODEL` | ⚠️ Optional | - | Bedrock embedding model ID | +| `BEDROCK_GENERATION_MODEL` | ⚠️ Optional | - | Bedrock generation model ID | +| `SIMPLE_EMBEDDING_DIMENSION` | ⚠️ Optional | `384` | Dimension for the fallback Simple provider | | `DOCUMENT_CHUNK_SIZE` | ⚠️ Optional | `512` | Words per chunk for document embedding | | `DOCUMENT_CHUNK_OVERLAP` | ⚠️ Optional | `50` | Overlapping words between chunks (must be < chunk size) | diff --git a/nextcloud_mcp_server/search/algorithms.py b/nextcloud_mcp_server/search/algorithms.py index 7b120393..f89a8933 100644 --- a/nextcloud_mcp_server/search/algorithms.py +++ b/nextcloud_mcp_server/search/algorithms.py @@ -11,6 +11,8 @@ from nextcloud_mcp_server.config import get_settings from nextcloud_mcp_server.vector.placeholder import get_placeholder_filter from nextcloud_mcp_server.vector.qdrant_client import get_qdrant_client +logger = logging.getLogger(__name__) + @runtime_checkable class NextcloudClientProtocol(Protocol): @@ -91,7 +93,6 @@ async def get_indexed_doc_types(user_id: str) -> set[str]: ... # Search notes """ - logger = logging.getLogger(__name__) settings = get_settings() qdrant_client = await get_qdrant_client() @@ -205,15 +206,19 @@ def build_search_result_from_point( if point.payload is None: return None - doc_id = str(point.payload["doc_id"]) + raw_doc_id = point.payload.get("doc_id") + if raw_doc_id is None: + logger.warning("Skipping point %s: missing doc_id in payload", point.id) + return None + doc_id = str(raw_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) + # Caller-supplied metadata is merged first; payload-derived common fields + # (chunk_index, total_chunks) win in case of key collisions so they always + # reflect the actual point. + metadata: dict[str, Any] = dict(metadata_extras) if metadata_extras else {} + metadata["chunk_index"] = point.payload.get("chunk_index") + metadata["total_chunks"] = point.payload.get("total_chunks") # File-specific metadata for PDF viewer if doc_type == "file" and (path := point.payload.get("file_path")): diff --git a/nextcloud_mcp_server/vector/qdrant_client.py b/nextcloud_mcp_server/vector/qdrant_client.py index 34b14b3a..9c3808f7 100644 --- a/nextcloud_mcp_server/vector/qdrant_client.py +++ b/nextcloud_mcp_server/vector/qdrant_client.py @@ -46,9 +46,21 @@ async def _ensure_keyword_payload_indexes( except UnexpectedResponse as e: body = getattr(e, "content", b"") or b"" body_text = body.decode("utf-8", errors="replace") - logger.warning( - "Failed to create payload index on '%s': %s", field, body_text - ) + # 400 is the expected schema-conflict path (index already exists + # with a different type). 5xx / network-shaped errors should not + # be silently downgraded — keep the loop going so the remaining + # fields still get attempted, but log at error so operators see it. + if e.status_code == 400: + logger.warning( + "Schema conflict on payload index '%s': %s", field, body_text + ) + else: + logger.error( + "Unexpected error creating payload index on '%s' (status %s): %s", + field, + e.status_code, + body_text, + ) async def _backfill_doc_id_to_string( diff --git a/tests/unit/search/test_search_result.py b/tests/unit/search/test_search_result.py index 52bf15c0..e29ed496 100644 --- a/tests/unit/search/test_search_result.py +++ b/tests/unit/search/test_search_result.py @@ -162,6 +162,14 @@ def test_build_search_result_from_point_returns_none_when_payload_missing(): assert build_search_result_from_point(point) is None +@pytest.mark.unit +def test_build_search_result_from_point_returns_none_when_doc_id_missing(): + """A payload without a doc_id key is skipped instead of raising KeyError.""" + point = _make_point(point_id="p-bad", payload={"doc_type": "note"}) + + 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.""" @@ -187,7 +195,7 @@ def test_build_search_result_from_point_note_payload(): assert sr.doc_type == "note" assert sr.title == "Hello" assert sr.excerpt == "world" - assert sr.score == 0.91 + assert sr.score == pytest.approx(0.91) assert sr.chunk_start_offset == 0 assert sr.chunk_end_offset == 100 assert sr.chunk_index == 0 @@ -257,21 +265,34 @@ def test_build_search_result_from_point_deck_card_metadata(): @pytest.mark.unit def test_build_search_result_from_point_merges_metadata_extras(): - """metadata_extras override/augment the helper's computed metadata dict.""" + """metadata_extras augment the helper's computed metadata dict. + + Common fields (chunk_index, total_chunks) win over caller-supplied + extras to keep them tied to the actual point. + """ point = _make_point( point_id="p-4", - payload={"doc_id": "1", "doc_type": "note"}, + payload={ + "doc_id": "1", + "doc_type": "note", + "chunk_index": 3, + "total_chunks": 9, + }, ) sr = build_search_result_from_point( - point, metadata_extras={"search_method": "bm25_hybrid_rrf"} + point, + metadata_extras={ + "search_method": "bm25_hybrid_rrf", + # Caller tries to override a common field — should be ignored. + "chunk_index": "should-be-overwritten", + }, ) 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 + assert sr.metadata["chunk_index"] == 3 + assert sr.metadata["total_chunks"] == 9 @pytest.mark.unit diff --git a/tests/unit/vector/test_qdrant_client.py b/tests/unit/vector/test_qdrant_client.py index ab5e38c7..2ec93ec9 100644 --- a/tests/unit/vector/test_qdrant_client.py +++ b/tests/unit/vector/test_qdrant_client.py @@ -103,6 +103,32 @@ async def test_ensure_keyword_payload_indexes_logs_400_as_warning(mocker, caplog assert "different schema" in warnings[0].getMessage() +@pytest.mark.unit +async def test_ensure_keyword_payload_indexes_logs_non_400_as_error(mocker, caplog): + """A non-400 status from create_payload_index escalates to ERROR. + + A 5xx response (e.g., Qdrant temporarily unavailable) should not be + silently downgraded to a warning the way a 400 schema-conflict is. + The loop still continues so the remaining fields get attempted. + """ + client = mocker.AsyncMock() + client.create_payload_index.side_effect = [ + _make_unexpected(500, b'{"status":{"error":"internal server error"}}'), + None, + None, + ] + + with caplog.at_level("ERROR", 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) + errors = [r for r in caplog.records if r.levelname == "ERROR"] + assert len(errors) == 1 + msg = errors[0].getMessage() + assert "500" in msg + assert "internal server error" in msg + + # --------------------------------------------------------------------------- # _backfill_doc_id_to_string # --------------------------------------------------------------------------- @@ -231,3 +257,27 @@ async def test_backfill_handles_none_payload(mocker): points=[2], wait=True, ) + + +@pytest.mark.unit +async def test_backfill_handles_payload_with_explicit_none_doc_id(mocker): + """A payload of {doc_id: None, ...} is skipped just like payload=None.""" + client = mocker.AsyncMock() + # Build the record manually to distinguish payload=None from payload={"doc_id": None}. + point_with_explicit_none = SimpleNamespace( + id=1, payload={"doc_id": None, "doc_type": "file"} + ) + client.scroll.side_effect = [ + ([point_with_explicit_none, _record(2, 99)], None), + ] + + await _backfill_doc_id_to_string(client, "test-collection") + + # Only the int doc_id at point 2 was rewritten; the explicit-None payload 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=True, + )