fix(vector): address PR review round 12 — bool guard + strict doc_id validation

- _group_int_doc_ids: use type(value) is not int instead of isinstance,
  since bool is an int subclass and would otherwise stringify to
  "True"/"False" and corrupt legacy payloads on backfill.
- Replace doc_id.isdigit() guards in 5 boundary sites
  (api/visualization, auth/viz_routes, search/context note/news_item/
  deck_card branches) with a shared is_valid_nextcloud_doc_id helper
  that rejects "0", leading zeros, and Unicode digit classes
  (superscripts, Arabic-Indic, Devanagari) which pass isdigit() but
  cannot be valid MySQL AUTO_INCREMENT IDs.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
Chris Coutinho
2026-05-09 20:28:47 +02:00
co-authored by Claude Opus 4.7
parent f3ce46da0f
commit f9ad7dc52e
8 changed files with 133 additions and 24 deletions
View File
+54
View File
@@ -0,0 +1,54 @@
"""Unit tests for shared boundary validators."""
import pytest
from nextcloud_mcp_server.utils.validation import is_valid_nextcloud_doc_id
@pytest.mark.unit
@pytest.mark.parametrize(
"value",
[
"1",
"42",
"1234567890",
"9999999999999999999",
],
)
def test_accepts_positive_ascii_integers(value):
"""Any positive ASCII integer (no leading zero) is a valid doc_id."""
assert is_valid_nextcloud_doc_id(value) is True
@pytest.mark.unit
@pytest.mark.parametrize(
"value,reason",
[
("", "empty string"),
("0", "MySQL AUTO_INCREMENT starts at 1"),
("01", "leading zero"),
("00", "leading zeros"),
("-1", "negative"),
("+1", "explicit sign"),
("1.0", "float-like"),
(" 1", "leading whitespace"),
("1 ", "trailing whitespace"),
("1\n", "trailing newline"),
("abc", "alphabetic"),
("1a", "trailing letter"),
("a1", "leading letter"),
# Unicode digit classes that pass str.isdigit() but are not ASCII.
# `²` (U+00B2) is a superscript and would slip past the old guard.
("²", "Unicode superscript-2"),
# `٢` (U+0662) Arabic-Indic digit two — passes both isdigit() and
# isdecimal(), so only an explicit ASCII regex catches it.
("٢", "Arabic-Indic digit two"),
# `१` (U+0967) Devanagari digit one — same story.
("", "Devanagari digit one"),
# Mixed ASCII + Unicode digits.
("", "mixed ASCII + Arabic-Indic"),
],
)
def test_rejects_invalid_doc_ids(value, reason):
"""Reject empty/zero/leading-zero/non-ASCII/non-digit inputs."""
assert is_valid_nextcloud_doc_id(value) is False, f"should reject: {reason}"
+30
View File
@@ -678,6 +678,36 @@ def test_group_int_doc_ids_skips_float_and_warns(caplog):
assert "99" in msg
@pytest.mark.unit
def test_group_int_doc_ids_skips_bool_and_warns(caplog):
"""A bool doc_id is not stringified to "True"/"False"; it logs and skips.
``isinstance(True, int)`` is ``True`` because ``bool`` is a subclass of
``int`` in Python, so a naive ``isinstance(value, int)`` guard would let
a boolean payload through and write ``str(True)`` → ``"True"`` into
Qdrant. Producers never write bools, but the strict ``type(value) is
int`` guard ensures any future producer bug surfaces as a WARNING and is
not silently stringified.
"""
bool_point = SimpleNamespace(id=33, payload={"doc_id": True})
int_point = SimpleNamespace(id=42, payload={"doc_id": 7})
with caplog.at_level("WARNING", logger="nextcloud_mcp_server.vector.qdrant_client"):
by_value, scanned = _group_int_doc_ids([bool_point, int_point])
# Only the int point made it into by_value — "True" is *not* a key.
assert by_value == {"7": [42]}
assert "True" not in by_value
assert "False" not in by_value
assert scanned == 2
warnings = [r for r in caplog.records if r.levelname == "WARNING"]
assert len(warnings) == 1
msg = warnings[0].getMessage()
assert "bool" in msg
assert "33" in msg
@pytest.mark.unit
def test_group_int_doc_ids_handles_str_and_missing_silently(caplog):
"""str / missing doc_id payloads are skipped without warning.