fix(python): validate collection document ids

[HosniBelfeki]

Summary

This adds Python-side normalization and validation for document IDs passed to Collection.fetch() and Collection.delete().

Previously, invalid values such as None, bytes, non-string values, or lists containing empty/non-string IDs could reach the native layer and fail with less direct errors. This now rejects invalid IDs early with a clear ValueError, while still preserving single-ID return behavior and accepting string sequences such as tuples by normalizing them to lists.

Tests

• python -m ruff check python/zvec/model/collection.py python/tests/test_collection.py
• python -m ruff format --check python/zvec/model/collection.py python/tests/test_collection.py
• python -m py_compile python/zvec/model/collection.py python/tests/test_collection.py

Note: focused pytest in this local workspace resolves the installed zvec package instead of the edited source package, so the new tests are intended to run in CI against the PR source tree.

[Copilot]

Pull request overview

This PR adds Python-side normalization and validation for document IDs passed to Collection.fetch() and Collection.delete(), rejecting invalid inputs early with clearer ValueErrors before they reach the native layer.

Changes:

• Introduces _normalize_doc_ids() to validate and normalize ids inputs (single string vs string sequences, excluding bytes-like values).
• Updates Collection.delete() and Collection.fetch() to use the new normalization/validation logic and to accept Sequence[str].
• Adds pytest coverage for rejecting invalid ids and for accepting tuple inputs (normalized to lists) in both fetch and delete.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

File	Description
python/zvec/model/collection.py	Adds _normalize_doc_ids() and routes fetch/delete through it; updates type hints to accept Sequence[str].
python/tests/test_collection.py	Adds tests asserting invalid IDs are rejected and tuple IDs are accepted/normalized for fetch and delete.

Suppressed comments (1)

python/tests/test_collection.py:830

• The new validation path rejects an empty string when ids is a single str (via _normalize_doc_ids), but this case is not covered by the new parametrized invalid-id tests for delete (only [""] is tested). Adding "" here would prevent regressions in the single-id branch.

    @pytest.mark.parametrize("ids", [None, 1, b"doc_1", ["doc_1", None], [""]])

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

Suppressed comments (3)

python/tests/test_collection.py:838

• These new delete() validation tests don't use the real collection fixture, but because they live under @pytest.mark.usefixtures("test_collection"), they still create/destroy a native collection for each test. Consider moving them to a separate non-fixture class/module-level tests to avoid extra setup/teardown.

class TestCollectionDelete:
    @pytest.mark.parametrize("ids", [None, 1, b"doc_1", "", ["doc_1", None], [""]])
    def test_collection_delete_rejects_invalid_ids(self, ids):
        collection = Collection.__new__(Collection)
        collection._obj = MagicMock()

        with pytest.raises(ValueError, match="ids must"):
            collection.delete(ids)

        collection._obj.Delete.assert_not_called()

python/zvec/model/collection.py:358

• The delete() docstring still says the list-return behavior applies only when "a list was given", but the method now accepts tuples (and normalizes any sequence). This makes the documented behavior slightly inaccurate.

        Returns:
            Union[Status, list[Status]]: If a single id was given, returns its Status;
            if a list was given, returns a list of Status objects.
        """

python/tests/test_collection.py:437

• These new fetch() validation tests don't use the real collection fixture, but because they live under @pytest.mark.usefixtures("test_collection"), they still create/destroy a native collection for each test. This adds unnecessary setup/teardown cost and potential flakiness for tests that only need a MagicMock.

This issue also appears on line 829 of the same file.

class TestCollectionFetch:
    @pytest.mark.parametrize("ids", [None, 1, b"doc_1", "", ["doc_1", None], [""]])
    def test_collection_fetch_rejects_invalid_ids(self, ids):
        collection = Collection.__new__(Collection)
        collection._obj = MagicMock()

        with pytest.raises(ValueError, match="ids must"):
            collection.fetch(ids)

        collection._obj.Fetch.assert_not_called()

[HosniBelfeki]

Addressed Copilot review feedback: single empty-string IDs are now covered in fetch/delete validation tests, the non-string overload no longer overlaps with the single-string overload, and existing list inputs are reused without an unnecessary copy. I also updated this branch with the latest upstream main.

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

Suppressed comments (1)

python/zvec/model/collection.py:357

• The delete() docstring return description still says the list-return behavior only applies when a "list" is given, but the method now also accepts tuples (and normalizes them to a list). This is misleading for API consumers reading the docstring.

        Returns:
            Union[Status, list[Status]]: If a single id was given, returns its Status;
            if a list was given, returns a list of Status objects.

[Cuiyus]

This PR is useful for improving Python-side error messages, but it does not appear necessary for native safety: pybind11 already validates conversion to std::vectorstd::string, while the C++ implementation safely handles missing keys.