fix(vector): address PR review round 2 — status branching, doc_id guard, doc restore
- _ensure_keyword_payload_indexes: distinguish 400 (schema conflict, warning)
from other status codes (5xx/network, error) so a transient outage doesn't
silently leave the collection unindexed.
- build_search_result_from_point: use .get("doc_id") + return None on missing
instead of KeyError-crashing the search; reverse metadata merge order so
payload-derived chunk_index/total_chunks win over caller-supplied extras.
- docs/configuration.md: restore the OpenAI/Mistral/Bedrock/Simple provider
sections + reference-table rows that were dropped in the rebase. Reword
the "Startup migrations" bullet to describe what the code actually does
(no sampling — full scroll, zero writes when clean). Add operator note
about the SemanticSearchResult.id TypeError path.
- tests: pytest.approx for float equality (Sonar python:S1244); coverage
for non-400 → ERROR, payload={doc_id: None}, and missing doc_id key.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
Chris Coutinho
co-authored by
Claude Opus 4.7
Claude Opus 4.7
parent
6aba589a6e
commit
b5b4025bb4
+82
-8
@@ -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) |
|
||||
|
||||
|
||||
@@ -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")):
|
||||
|
||||
@@ -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(
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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,
|
||||
)
|
||||
|
||||
Reference in New Issue
Block a user