Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 17 additions & 2 deletions python/semantic_kernel/connectors/redis.py
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
from redis.commands.search.field import Field as RedisField
from redis.commands.search.field import NumericField, TagField, TextField, VectorField
from redis.commands.search.index_definition import IndexDefinition, IndexType
from redis.exceptions import ResponseError
from redisvl.index import AsyncSearchIndex
from redisvl.index.index import process_results
from redisvl.query.filter import FilterExpression, Num, Tag, Text
Expand Down Expand Up @@ -59,6 +60,11 @@

logger = logging.getLogger(__name__)

# Redis Search has no structured "index not found" reply and has worded it differently across
# versions ("Unknown index name", "<name>: no such index", and since 8.8 an error code), so the
# missing-index case is recognised by its text.
_MISSING_INDEX_ERROR_FRAGMENTS = ("unknown index name", "no such index", "search_index_not_found", "index not found")

TKey = TypeVar("TKey", bound=str)
TQuery = TypeVar("TQuery", bound=BaseQuery)

Expand Down Expand Up @@ -315,8 +321,17 @@ async def collection_exists(self, **kwargs) -> bool:
try:
await self.redis_database.ft(self.collection_name).info()
return True
except Exception:
return False
except ResponseError as exc:
# Redis answers FT.INFO for an unknown index with an error reply, which redis-py
# surfaces as a plain ResponseError. Other failures also arrive as ResponseError or a
# subclass of it (NoPermissionError, ClusterDownError), so only a plain ResponseError
# with the missing-index wording means "not there"; everything else propagates, as
# ConnectionError and TimeoutError do. ensure_collection_deleted() gates the drop on
# this answer, so a swallowed failure made it return successfully having deleted nothing.
message = str(exc).lower()
if type(exc) is ResponseError and any(fragment in message for fragment in _MISSING_INDEX_ERROR_FRAGMENTS):
return False
raise

@override
async def ensure_collection_deleted(self, **kwargs) -> None:
Expand Down
65 changes: 63 additions & 2 deletions python/tests/unit/connectors/memory/test_redis_store.py
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@
import numpy as np
from pytest import fixture, mark, raises
from redis.asyncio.client import Redis
from redis.exceptions import ClusterDownError, NoPermissionError, ResponseError
from redis.exceptions import ConnectionError as RedisConnectionError

import semantic_kernel.connectors.redis as redis_module
from semantic_kernel.connectors.redis import (
Expand Down Expand Up @@ -281,12 +283,71 @@ async def test_collection_exists(collection_hash, mock_collection_exists):
await collection_hash.collection_exists()


async def test_collection_exists_false(collection_hash, mock_collection_exists):
mock_collection_exists.side_effect = Exception
@mark.parametrize(
"message",
[
"Unknown Index name",
"Unknown index name (or name is an alias itself)",
"test: no such index",
"SEARCH_INDEX_NOT_FOUND",
"Index not found",
],
)
async def test_collection_exists_false(collection_hash, mock_collection_exists, message):
# ResponseError is what redis-py raises when FT.INFO reports an unknown index, worded
# differently across Redis Search versions. The previous bare Exception did not represent
# that, and matched a timeout just as readily.
mock_collection_exists.side_effect = ResponseError(message)
exists = await collection_hash.collection_exists()
assert not exists


async def test_collection_exists_propagates_connection_errors(collection_hash, mock_collection_exists):
"""A cluster that cannot be reached is not an absent index.

ensure_collection_deleted() drops the index only when collection_exists() says it is there, so
reporting a connection failure as "not there" made the delete a silent no-op.
"""
mock_collection_exists.side_effect = RedisConnectionError("connection refused")
with raises(RedisConnectionError):
await collection_hash.collection_exists()


@mark.parametrize(
"error",
[
NoPermissionError("User default has no permissions to run the 'ft.info' command"),
ClusterDownError("The cluster is down"),
ResponseError("ERR unexpected reply"),
# The wording match must not apply to a subclass, even when the message happens to
# contain a missing-index phrase (here through the user name).
NoPermissionError("User search_index_not_found has no permissions to run the 'ft.info' command"),
],
ids=["no_permission", "cluster_down", "other_response_error", "no_permission_with_matching_text"],
)
async def test_collection_exists_propagates_other_response_errors(collection_hash, mock_collection_exists, error):
"""Only the missing-index reply means "not there".

NoPermissionError and ClusterDownError subclass ResponseError, so catching ResponseError alone
still reported a denied or failed FT.INFO as an absent index.
"""
mock_collection_exists.side_effect = error
with raises(type(error)):
await collection_hash.collection_exists()


async def test_ensure_collection_deleted_propagates_permission_errors(
collection_hash, mock_collection_exists, mock_ensure_collection_deleted
):
"""A denied FT.INFO must fail the delete instead of skipping the drop and returning normally."""
mock_collection_exists.side_effect = NoPermissionError(
"User default has no permissions to run the 'ft.info' command"
)
with raises(NoPermissionError):
await collection_hash.ensure_collection_deleted()
mock_ensure_collection_deleted.assert_not_called()


async def test_ensure_collection_deleted(collection_hash, mock_ensure_collection_deleted):
await collection_hash.ensure_collection_deleted()
await collection_hash.ensure_collection_deleted()
Expand Down
Loading