admin/mcp-nextcloud
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

fix(vector): address PR review round 3 — sentinel guard, skip indexed fields, narrow types

Browse Source 
- Add a fixed-UUID sentinel point written after a successful doc_id
  backfill so subsequent restarts retrieve it and short-circuit the
  O(N) scroll. Sentinel has no user_id/doc_id/doc_type payload so
  production search filters never see it.
- Pre-fetch payload_schema in _ensure_keyword_payload_indexes and
  silently skip fields that are already indexed; the "Created KEYWORD
  payload index" INFO log fires only on actual creation.
- Narrow stale `int | str` doc_id annotations to `str` across
  search/verification.py (BatchVerifier return type, per-verifier
  accessible sets, by_type / accessible_by_type / inaccessible
  collections); drop the now-redundant `type(d).__name__` prefix in
  the dropped-docs log.
- Align the backfill log message with the PR description's
  "Running doc_id backfill" promise; add a caller cross-reference to
  the wait=True comment.
- Fix _get_file_path_from_qdrant docstring (file_id is str, not numeric).
- Convert legacy `id=1` to `id="1"` in test_search_result.py to match
  the SearchResult.id: str annotation.

Three new unit tests cover sentinel-found, sentinel-written, and
skip-existing-index branches; existing backfill tests pass dimension
and explicit retrieve.return_value=[] for the no-sentinel path.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
Chris Coutinho
2026-05-08 22:59:46 +02:00
co-authored by Claude Opus 4.7
parent b5b4025bb4
commit 92b2d50cd7
5 changed files with 223 additions and 47 deletions
Download Patch File Download Diff File Expand all files Collapse all files
tests/unit/search/test_search_result.py
+8 -8
View File
@@ -23,7 +23,7 @@ def _make_point(point_id, payload, score=0.5):
def test_search_result_rrf_score_in_range():
"""Test SearchResult accepts RRF scores in [0.0, 1.0] range."""
result = SearchResult(
id=1,
id="1",
doc_type="note",
title="Test Note",
excerpt="Test excerpt",
@@ -37,7 +37,7 @@ def test_search_result_rrf_score_in_range():
def test_search_result_rrf_score_at_lower_bound():
"""Test SearchResult accepts RRF score at lower bound (0.0)."""
result = SearchResult(
id=1,
id="1",
doc_type="note",
title="Test Note",
excerpt="Test excerpt",
@@ -51,7 +51,7 @@ def test_search_result_rrf_score_at_lower_bound():
def test_search_result_rrf_score_at_upper_bound():
"""Test SearchResult accepts RRF score at upper bound (1.0)."""
result = SearchResult(
id=1,
id="1",
doc_type="note",
title="Test Note",
excerpt="Test excerpt",
@@ -71,7 +71,7 @@ def test_search_result_dbsf_score_above_one():
"""
# Typical DBSF score when both systems agree
result = SearchResult(
id=1,
id="1",
doc_type="note",
title="Highly Relevant Note",
excerpt="Contains keywords and is semantically similar",
@@ -88,7 +88,7 @@ def test_search_result_dbsf_score_edge_case():
Maximum DBSF score with 2 systems: 1.0 (dense) + 1.0 (sparse) = 2.0
"""
result = SearchResult(
id=1,
id="1",
doc_type="note",
title="Perfect Match",
excerpt="Perfect semantic and keyword match",
@@ -103,7 +103,7 @@ def test_search_result_negative_score_raises_error():
"""Test SearchResult rejects negative scores."""
with pytest.raises(ValueError) as exc_info:
SearchResult(
id=1,
id="1",
doc_type="note",
title="Test Note",
excerpt="Test excerpt",
@@ -118,7 +118,7 @@ def test_search_result_negative_score_raises_error():
def test_search_result_with_metadata():
"""Test SearchResult with optional metadata field."""
result = SearchResult(
id=1,
id="1",
doc_type="note",
title="Test Note",
excerpt="Test excerpt",
@@ -136,7 +136,7 @@ def test_search_result_with_metadata():
def test_search_result_with_chunk_offsets():
"""Test SearchResult with chunk offset information."""
result = SearchResult(
id=1,
id="1",
doc_type="note",
title="Test Note",
excerpt="matching chunk text",
tests/unit/vector/test_qdrant_client.py
+114 -8
View File
@@ -23,12 +23,32 @@ from qdrant_client.http.exceptions import UnexpectedResponse
from qdrant_client.models import PayloadSchemaType
from nextcloud_mcp_server.vector.qdrant_client import (
_DOC_ID_BACKFILL_SENTINEL_ID,
_KEYWORD_PAYLOAD_FIELDS,
_backfill_doc_id_to_string,
_ensure_keyword_payload_indexes,
)
def _empty_collection_info() -> SimpleNamespace:
"""Stand-in for a CollectionInfo with no payload indexes yet.
Tests for _ensure_keyword_payload_indexes only read ``payload_schema``
off the result. None / empty dict both signal "no indexes" — use {}
here to match the production-code default.
"""
return SimpleNamespace(payload_schema={})
def _backfill_dimension() -> int:
"""Vector dimension for sentinel writes in backfill tests.
Any positive int is fine — the sentinel point is never read by the
test bodies, only the upsert call site is asserted.
"""
return 4
def _make_unexpected(status_code: int, body: bytes) -> UnexpectedResponse:
"""Build a real UnexpectedResponse for raise_for_status-style branches."""
return UnexpectedResponse(
@@ -58,6 +78,7 @@ def _record(point_id: int | str, doc_id: int | str | None) -> SimpleNamespace:
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()
client.get_collection.return_value = _empty_collection_info()
await _ensure_keyword_payload_indexes(client, "test-collection")
@@ -74,6 +95,37 @@ async def test_ensure_keyword_payload_indexes_creates_each_field(mocker):
client.create_payload_index.assert_has_awaits(expected_calls, any_order=False)
@pytest.mark.unit
async def test_ensure_keyword_payload_indexes_skips_fields_already_indexed(
mocker, caplog
):
"""Routine restart path: existing payload indexes are silently skipped.
Without the pre-fetch, every restart logs `Created KEYWORD payload
index on '<field>'` for every field — noise that hides genuinely
interesting first-time-creation lines. With the pre-fetch, no log
fires and no Qdrant write round-trip happens for already-indexed
fields.
"""
client = mocker.AsyncMock()
client.get_collection.return_value = SimpleNamespace(
payload_schema={"doc_id": object()}
)
with caplog.at_level("INFO", logger="nextcloud_mcp_server.vector.qdrant_client"):
await _ensure_keyword_payload_indexes(client, "test-collection")
# Only the two missing fields are created.
assert client.create_payload_index.await_count == 2
created_fields = {
c.kwargs["field_name"] for c in client.create_payload_index.await_args_list
}
assert created_fields == {"user_id", "doc_type"}
# No INFO log fires for the already-indexed field.
info_messages = [r.getMessage() for r in caplog.records if r.levelname == "INFO"]
assert not any("doc_id" in m for m in info_messages), info_messages
@pytest.mark.unit
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.
@@ -84,6 +136,7 @@ async def test_ensure_keyword_payload_indexes_logs_400_as_warning(mocker, caplog
remaining fields still get indexed.
"""
client = mocker.AsyncMock()
client.get_collection.return_value = _empty_collection_info()
client.create_payload_index.side_effect = [
_make_unexpected(
400,
@@ -112,6 +165,7 @@ async def test_ensure_keyword_payload_indexes_logs_non_400_as_error(mocker, capl
The loop still continues so the remaining fields get attempted.
"""
client = mocker.AsyncMock()
client.get_collection.return_value = _empty_collection_info()
client.create_payload_index.side_effect = [
_make_unexpected(500, b'{"status":{"error":"internal server error"}}'),
None,
@@ -138,17 +192,20 @@ async def test_ensure_keyword_payload_indexes_logs_non_400_as_error(mocker, capl
async def test_backfill_clean_collection_makes_no_writes(mocker, caplog):
"""A collection with only str doc_ids triggers zero set_payload calls.
Verifies idempotency: a second pass over an already-migrated collection
is a no-op modulo the read.
Verifies the no-write path: scroll runs, no payloads need rewriting,
and a sentinel is written so subsequent restarts can short-circuit.
"""
client = mocker.AsyncMock()
client.retrieve.return_value = [] # No sentinel — backfill must run
client.scroll.return_value = (
[_record(1, "abc"), _record(2, "def")],
None,
)
with caplog.at_level("INFO", logger="nextcloud_mcp_server.vector.qdrant_client"):
await _backfill_doc_id_to_string(client, "test-collection")
await _backfill_doc_id_to_string(
client, "test-collection", _backfill_dimension()
)
client.set_payload.assert_not_awaited()
completion_logs = [
@@ -158,10 +215,53 @@ async def test_backfill_clean_collection_makes_no_writes(mocker, caplog):
assert "0/2" in completion_logs[0]
@pytest.mark.unit
async def test_backfill_skips_when_sentinel_present(mocker, caplog):
"""If the sentinel exists, retrieve() returns it and the scroll is skipped.
This is the routine-restart fast path: the migration already ran on a
previous start, so we avoid the O(N) scroll entirely.
"""
client = mocker.AsyncMock()
client.retrieve.return_value = [SimpleNamespace(id=_DOC_ID_BACKFILL_SENTINEL_ID)]
with caplog.at_level("DEBUG", logger="nextcloud_mcp_server.vector.qdrant_client"):
await _backfill_doc_id_to_string(
client, "test-collection", _backfill_dimension()
)
client.scroll.assert_not_awaited()
client.set_payload.assert_not_awaited()
client.upsert.assert_not_awaited()
debug_msgs = [r.getMessage() for r in caplog.records if r.levelname == "DEBUG"]
assert any("sentinel" in m and "skipping" in m for m in debug_msgs), debug_msgs
@pytest.mark.unit
async def test_backfill_writes_sentinel_after_successful_scroll(mocker):
"""Successful backfill writes a sentinel point so future restarts skip."""
client = mocker.AsyncMock()
client.retrieve.return_value = [] # No sentinel — backfill must run
client.scroll.return_value = ([_record(1, "abc")], None)
await _backfill_doc_id_to_string(client, "test-collection", _backfill_dimension())
# Single upsert with the sentinel UUID + migration marker payload.
assert client.upsert.await_count == 1
upsert_kwargs = client.upsert.await_args.kwargs
assert upsert_kwargs["collection_name"] == "test-collection"
assert upsert_kwargs["wait"] is True
points = upsert_kwargs["points"]
assert len(points) == 1
assert points[0].id == _DOC_ID_BACKFILL_SENTINEL_ID
assert points[0].payload == {"_migration_marker": "doc_id_v1"}
@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()
client.retrieve.return_value = []
# 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 = [
@@ -169,7 +269,7 @@ async def test_backfill_rewrites_int_doc_ids_to_str(mocker):
([_record(3, 200), _record(4, "def")], None),
]
await _backfill_doc_id_to_string(client, "test-collection")
await _backfill_doc_id_to_string(client, "test-collection", _backfill_dimension())
# One set_payload per *unique* int value — point 1 (100) and point 3
# (200) are in different batches with different values, so two calls.
@@ -196,6 +296,7 @@ async def test_backfill_batches_points_with_same_doc_id(mocker):
backfill should issue one set_payload call covering the chunk batch.
"""
client = mocker.AsyncMock()
client.retrieve.return_value = []
client.scroll.side_effect = [
(
[
@@ -208,7 +309,7 @@ async def test_backfill_batches_points_with_same_doc_id(mocker):
),
]
await _backfill_doc_id_to_string(client, "test-collection")
await _backfill_doc_id_to_string(client, "test-collection", _backfill_dimension())
# All three int-payload points share doc_id=42, so a single call covers them.
assert client.set_payload.await_count == 1
@@ -224,12 +325,15 @@ async def test_backfill_batches_points_with_same_doc_id(mocker):
async def test_backfill_emits_completion_log(mocker, caplog):
"""Backfill logs final rewritten/scanned counts at INFO."""
client = mocker.AsyncMock()
client.retrieve.return_value = []
client.scroll.side_effect = [
([_record(1, 7), _record(2, "x")], None),
]
with caplog.at_level("INFO", logger="nextcloud_mcp_server.vector.qdrant_client"):
await _backfill_doc_id_to_string(client, "test-collection")
await _backfill_doc_id_to_string(
client, "test-collection", _backfill_dimension()
)
completion_logs = [
r.getMessage() for r in caplog.records if "backfill complete" in r.getMessage()
@@ -243,11 +347,12 @@ async def test_backfill_emits_completion_log(mocker, caplog):
async def test_backfill_handles_none_payload(mocker):
"""A point with payload=None is skipped without crashing."""
client = mocker.AsyncMock()
client.retrieve.return_value = []
client.scroll.side_effect = [
([_record(1, None), _record(2, 99)], None),
]
await _backfill_doc_id_to_string(client, "test-collection")
await _backfill_doc_id_to_string(client, "test-collection", _backfill_dimension())
# Only the int doc_id at point 2 was rewritten; the None-payload point was skipped.
assert client.set_payload.await_count == 1
@@ -263,6 +368,7 @@ async def test_backfill_handles_none_payload(mocker):
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()
client.retrieve.return_value = []
# 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"}
@@ -271,7 +377,7 @@ async def test_backfill_handles_payload_with_explicit_none_doc_id(mocker):
([point_with_explicit_none, _record(2, 99)], None),
]
await _backfill_doc_id_to_string(client, "test-collection")
await _backfill_doc_id_to_string(client, "test-collection", _backfill_dimension())
# Only the int doc_id at point 2 was rewritten; the explicit-None payload was skipped.
assert client.set_payload.await_count == 1