fix: refuse bool and float item IDs everywhere instead of coercing them

libs/envector/langchain_envector/vectorstore.py

@@ -1,5 +1,6 @@
 from __future__ import annotations
 
+import numbers
 import warnings
 from typing import Any, Dict, Iterable, List, Optional, Sequence, Tuple
 
@@ -17,26 +18,71 @@
 SDK_HAS_GET_BY_IDS = hasattr(_SdkIndex, "get_by_ids")
 
 
+# Item IDs travel as proto int64; a larger value cannot name a row and the
+# SDK would fail to encode it.
+_MAX_ITEM_ID = 2**63 - 1
+
+
+def _readable_item_id(value: Any) -> Optional[int]:
+    """The item ID ``value`` names exactly, or ``None`` when it names none.
+
+    The one ID check every method shares: only a positive integer within int64
+    — any ``numbers.Integral``, so NumPy integers count; the server issues item
+    IDs as ``int64`` — or an ASCII decimal string of one counts. ``bool`` and
+    ``float`` are not item IDs — ``int(True)`` is 1 and ``int(3.9)`` is 3, so
+    coercing them would address a different document — and neither are
+    non-ASCII digits such as ``"٣"`` or ``"３"``, which ``str.isdecimal``
+    accepts.
+    """
+    if isinstance(value, bool):
+        return None
+    if isinstance(value, numbers.Integral):
+        return int(value) if 0 < value <= _MAX_ITEM_ID else None
+    if isinstance(value, str):
+        text = value.strip()
+        if text.isascii() and text.isdigit():
+            item_id = int(text)
+            return item_id if 0 < item_id <= _MAX_ITEM_ID else None
+    return None
+
+
+def _is_non_positive_integer(value: Any) -> bool:
+    """True for an integer (or its ASCII decimal string, sign allowed) <= 0."""
+    if isinstance(value, bool):
+        return False
+    if isinstance(value, numbers.Integral):
+        return value <= 0
+    if isinstance(value, str):
+        text = value.strip()
+        digits = text[1:] if text[:1] in "+-" else text
+        return bool(digits) and digits.isascii() and digits.isdigit() and int(text) <= 0
+    return False
+
+
 def _mutation_items(
     item_ids: List[Any], label: str, *, dedupe: bool = False
 ) -> List[int]:
-    """Coerce caller-supplied IDs to the ``int`` item_ids the SDK addresses.
+    """Turn caller-supplied IDs into the ``int`` item_ids the SDK addresses.
 
-    The SDK rejects non-positive and repeated ids with its own message; both
-    are caught here and named after the calling method. ``dedupe=True``
-    (delete) drops repeats instead, since deleting a row twice is deleting it.
+    Used by the methods that change rows (delete, update, upsert), so a value
+    that names no item raises instead of being coerced onto another item — see
+    `_readable_item_id`. Non-positive and repeated ids are named after the
+    calling method. ``dedupe=True`` (delete) drops repeats instead, since
+    deleting a row twice is deleting it.
     """
-    try:
-        ints = [int(x) for x in item_ids]
-    except (TypeError, ValueError) as e:
-        raise ValueError(
-            f"Envector.{label} expects integer item IDs (or numeric strings) "
-            "as returned by add_texts/add_documents."
-        ) from e
-    if any(i <= 0 for i in ints):
-        raise ValueError(
-            f"Envector.{label}: item IDs are positive integers (got {min(ints)})."
-        )
+    ints: List[int] = []
+    for x in item_ids:
+        item_id = _readable_item_id(x)
+        if item_id is None:
+            if _is_non_positive_integer(x):
+                raise ValueError(
+                    f"Envector.{label}: item IDs are positive integers (got {x!r})."
+                )
+            raise ValueError(
+                f"Envector.{label} expects integer item IDs (a positive int or its "
+                f"decimal string) as returned by add_texts/add_documents; got {x!r}."
+            )
+        ints.append(item_id)
     if dedupe:
         return list(dict.fromkeys(ints))
     if len(set(ints)) != len(ints):
@@ -48,56 +94,23 @@ def _split_caller_ids(ids: List[Any]) -> Tuple[List[Optional[int]], List[Any]]:
     """Sort caller-supplied IDs into enVector item IDs and everything else.
 
     Returns ``(item_ids, foreign)``: ``item_ids`` is positional against ``ids``
-    with ``None`` wherever the entry was ``None`` or not an integer, and
-    ``foreign`` lists the non-integer values so the caller can be told they
-    were not honoured.
+    with ``None`` wherever the entry was ``None`` or names no item (see
+    `_readable_item_id`), and ``foreign`` lists those other values so the
+    caller can be told they were not honoured.
     """
     item_ids: List[Optional[int]] = []
     foreign: List[Any] = []
     for x in ids:
         if x is None:
             item_ids.append(None)
             continue
-        try:
-            value = int(x)
-        except (TypeError, ValueError):
-            value = 0
-        if value <= 0:  # the server issues positive ints only
-            item_ids.append(None)
+        value = _readable_item_id(x)
+        item_ids.append(value)
+        if value is None:
             foreign.append(x)
-        else:
-            item_ids.append(value)
     return item_ids, foreign
 
 
-# Item IDs travel as proto int64; a larger value cannot name a row and the
-# SDK would fail to encode it.
-_MAX_ITEM_ID = 2**63 - 1
-
-
-def _readable_item_id(value: Any) -> Optional[int]:
-    """The item ID ``value`` names exactly, or ``None`` when it names none.
-
-    For `get_by_ids`, which must never read an item the caller did not name:
-    only a positive ``int`` within int64 (the server issues item IDs as
-    ``int64``), or an ASCII decimal string of one, counts. ``bool`` and
-    ``float`` are not item IDs — ``int(True)`` is 1 and ``int(3.9)`` is 3, so
-    coercing them would return a different document — and neither are
-    non-ASCII digits such as ``"٣"`` or ``"３"``, which ``str.isdecimal``
-    accepts.
-    """
-    if isinstance(value, bool):
-        return None
-    if isinstance(value, int):
-        return value if 0 < value <= _MAX_ITEM_ID else None
-    if isinstance(value, str):
-        text = value.strip()
-        if text.isascii() and text.isdigit():
-            item_id = int(text)
-            return item_id if 0 < item_id <= _MAX_ITEM_ID else None
-    return None
-
-
 def _one_embedding_arg(embedding: Any, embeddings: Any) -> Any:
     """Resolve the standard positional ``embedding`` and our older
     ``embeddings=`` keyword into one value, rejecting conflicting pairs."""
@@ -279,8 +292,9 @@ def add_texts(
         not affected.
 
         ``ids`` follows LangChain's add-or-update contract as far as enVector
-        allows: an entry that is an item ID (int or numeric str, such as the
-        ``Document.id`` search results carry) updates that item in place; an ID
+        allows: an entry that is an item ID (a positive int or its decimal str,
+        such as the ``Document.id`` search results carry) updates that item in
+        place; an ID
         with no live row, or a non-integer ID, cannot be created, so that row is
         inserted with a server-issued ID and a ``UserWarning``. ``None`` entries
         insert. The returned list holds the IDs actually in the index, as
@@ -431,8 +445,9 @@ def delete(
         """Delete items from the encrypted index by item ID.
 
         Accepts the ``item_id`` values returned from ``add_texts`` /
-        ``add_documents``. Both ``int`` and ``str`` (numeric) IDs are accepted
-        and coerced to ``int`` before being passed to the SDK.
+        ``add_documents``, as ``str`` or ``int``. Any other value — ``bool`` and
+        ``float`` included — raises ``ValueError`` rather than being coerced onto
+        another item (``3.9`` would otherwise delete item 3).
 
         Deletion is asynchronous server-side; by default this waits until the
         affected shards are rebuilt and the remaining data is searchable again

tests/test_vectorstore.py

@@ -1490,3 +1490,101 @@ def test_search_skips_a_hit_with_empty_text_and_metadata():
     ]
     store = Envector(config=_cfg(), embeddings=FakeEmbeddings(dim=4), client=client)
     assert [d.id for d in store.similarity_search("q", k=2)] == ["2"]
+
+
+# Values that look like item IDs to int(...) but name none: True -> 1,
+# 3.9 / 3.0 -> 3, "٣" (Arabic-Indic three) -> 3. None may address an item.
+_COERCIBLE_NON_IDS = [True, 3.9, 3.0, "٣"]
+
+
+@pytest.mark.parametrize("not_an_id", _COERCIBLE_NON_IDS)
+def test_delete_refuses_values_that_name_no_item(not_an_id):
+    client = FakeClient()
+    store = Envector(config=_cfg(), embeddings=FakeEmbeddings(dim=4), client=client)
+    store.add_texts(["a", "b", "c"])
+
+    with pytest.raises(ValueError, match="expects integer item IDs"):
+        store.delete([not_an_id])
+    assert client.index.deleted == []
+
+
+@pytest.mark.parametrize("not_an_id", _COERCIBLE_NON_IDS)
+def test_update_and_upsert_refuse_values_that_name_no_item(not_an_id):
+    client = FakeClient()
+    store = Envector(config=_cfg(), embeddings=FakeEmbeddings(dim=4), client=client)
+    store.add_texts(["a", "b", "c"])
+    doc = LC_Document(page_content="new")
+
+    with pytest.raises(ValueError, match="expects integer item IDs"):
+        store.update_metadata([not_an_id], ["new"])
+    with pytest.raises(ValueError, match="expects integer item IDs"):
+        store.update_documents([not_an_id], [doc])
+    with pytest.raises(ValueError, match="expects integer item IDs"):
+        store.upsert_documents([doc], ids=[not_an_id])
+    assert client.index.updates == []
+    assert client.index.upserts == []
+
+
+@pytest.mark.parametrize("not_an_id", _COERCIBLE_NON_IDS)
+def test_add_texts_inserts_instead_of_overwriting_on_a_non_id(not_an_id):
+    # add_texts keeps its existing rule for ids it cannot honour: insert as a
+    # new row and warn. It must not update item 3 in place.
+    client = FakeClient()
+    store = Envector(config=_cfg(), embeddings=FakeEmbeddings(dim=4), client=client)
+    store.add_texts(["a", "b", "c"])
+
+    with pytest.warns(UserWarning, match="not enVector item IDs"):
+        new_ids = store.add_texts(["x"], ids=[not_an_id])
+    assert new_ids == ["4"]
+    assert client.index.upserts == []
+    assert client.index.stored[(None, 3)] == '{"text": "c", "metadata": {}}'
+
+
+def test_mutations_still_take_ints_and_decimal_strings():
+    client = FakeClient()
+    store = Envector(config=_cfg(), embeddings=FakeEmbeddings(dim=4), client=client)
+    store.add_texts(["a", "b", "c", "d"])
+
+    store.delete(["1", " 2 ", 3])
+    assert client.index.deleted[-1]["item_ids"] == [1, 2, 3]
+    store.update_metadata([4], ["d2"])
+    assert [it.item_id for it in client.index.updates[-1]["items"]] == [4]
+
+
+def test_non_positive_ids_keep_their_own_message():
+    client = FakeClient()
+    store = Envector(config=_cfg(), embeddings=FakeEmbeddings(dim=4), client=client)
+    for bad in (0, -3, "-3", "0"):
+        with pytest.raises(ValueError, match="positive integers"):
+            store.delete([bad])
+
+
+def test_get_by_ids_ignores_non_ascii_digits():
+    client = FakeClient()
+    store = Envector(config=_cfg(), embeddings=FakeEmbeddings(dim=4), client=client)
+    store.add_texts(["a", "b", "c"])
+    assert store.get_by_ids(["٣"]) == []
+    assert client.index.fetched == []
+
+
+def test_numpy_integers_are_item_ids_and_numpy_bool_float_are_not():
+    # IDs often come out of NumPy arrays or pandas columns. A NumPy integer is
+    # an integer; np.bool_ and np.float64 are not item IDs any more than their
+    # Python counterparts.
+    np = pytest.importorskip("numpy")
+    client = FakeClient()
+    store = Envector(config=_cfg(), embeddings=FakeEmbeddings(dim=4), client=client)
+    store.add_texts(["a", "b", "c", "d"])
+
+    for good in (np.int64(3), np.int32(3), np.uint64(3)):
+        assert [d.page_content for d in store.get_by_ids([good])] == ["c"]
+    store.delete([np.int64(2)])
+    assert client.index.deleted[-1]["item_ids"] == [2]
+    assert type(client.index.deleted[-1]["item_ids"][0]) is int
+
+    for bad in (np.bool_(True), np.float64(3.9), np.float64(3.0)):
+        assert store.get_by_ids([bad]) == []
+        with pytest.raises(ValueError, match="expects integer item IDs"):
+            store.delete([bad])
+    with pytest.raises(ValueError, match="positive integers"):
+        store.delete([np.int64(0)])