diff --git a/redisvl/extensions/router/semantic.py b/redisvl/extensions/router/semantic.py index 16472615..2a45f811 100644 --- a/redisvl/extensions/router/semantic.py +++ b/redisvl/extensions/router/semantic.py @@ -885,6 +885,15 @@ def _make_filter_queries(ids: list[str]) -> list[FilterQuery]: queries = [] for id in ids: + if not id: + # `Tag(...) == ""` renders as the match-all `*`, so an empty id + # would match every reference in the index. Callers take the + # first row of each query's results, which turns this into + # returning -- and, from delete_route_references, deleting -- an + # arbitrary reference the caller never named. + raise ValueError( + "reference ids must be non-empty strings; received an empty id" + ) fe = Tag("reference_id") == id fq = FilterQuery( return_fields=["reference_id", "route_name", "reference"], diff --git a/tests/integration/test_semantic_router.py b/tests/integration/test_semantic_router.py index 1bc709b6..d6d9ba74 100644 --- a/tests/integration/test_semantic_router.py +++ b/tests/integration/test_semantic_router.py @@ -12,6 +12,7 @@ RoutingConfig, ) from redisvl.redis.connection import is_version_gte +from redisvl.redis.utils import convert_bytes from tests.conftest import skip_if_no_redis_search, skip_if_redis_version_below pytestmark = pytest.mark.requires_hf @@ -728,6 +729,26 @@ def test_get_route_references(semantic_router): semantic_router.get_route_references() +def test_empty_reference_id_deletes_nothing(semantic_router): + # An empty id used to render the match-all `*`, and because the caller reads + # the first row of each query's results it removed an arbitrary reference + # rather than the one asked for. + prefix = semantic_router._index.schema.index.prefix + keys_before = set( + convert_bytes(semantic_router._index._redis_client.keys(f"{prefix}*")) + ) + + with pytest.raises(ValueError): + semantic_router.delete_route_references(reference_ids=[""]) + with pytest.raises(ValueError): + semantic_router.get_route_references(reference_ids=[""]) + + keys_after = set( + convert_bytes(semantic_router._index._redis_client.keys(f"{prefix}*")) + ) + assert keys_after == keys_before + + def test_delete_route_references(semantic_router): # Get references for a specific route deleted = semantic_router.delete_route_references(route_name="greeting") diff --git a/tests/unit/test_semantic_router_queries.py b/tests/unit/test_semantic_router_queries.py new file mode 100644 index 00000000..4eb4938c --- /dev/null +++ b/tests/unit/test_semantic_router_queries.py @@ -0,0 +1,29 @@ +"""Hermetic tests for SemanticRouter's reference-id filter construction. + +`_make_filter_queries` is a static method that touches neither Redis nor a +vectorizer, so its behaviour is testable without the router's integration +fixtures -- which matters because a defect here silently deletes data. +""" + +import pytest + +from redisvl.extensions.router.semantic import SemanticRouter + + +def test_reference_id_query_is_scoped_to_the_id(): + (query,) = SemanticRouter._make_filter_queries(["abc123"]) + assert str(query._filter_expression) == "@reference_id:{abc123}" + + +@pytest.mark.parametrize("empty", ["", None], ids=["empty_string", "none"]) +def test_empty_reference_id_is_rejected_rather_than_matching_everything(empty): + # `Tag("reference_id") == ""` renders as `*`. Callers read the first row of + # each query's results, so a match-all here returns -- and, from + # delete_route_references, deletes -- a reference nobody named. + with pytest.raises(ValueError, match="non-empty"): + SemanticRouter._make_filter_queries([empty]) + + +def test_one_empty_id_rejects_the_whole_batch(): + with pytest.raises(ValueError, match="non-empty"): + SemanticRouter._make_filter_queries(["abc123", "", "def456"])