diff --git a/redisvl/extensions/cache/base.py b/redisvl/extensions/cache/base.py index 212faec2..39b5566c 100644 --- a/redisvl/extensions/cache/base.py +++ b/redisvl/extensions/cache/base.py @@ -6,7 +6,7 @@ import asyncio from collections.abc import Mapping -from typing import Any, cast +from typing import Any, TypedDict from redis import Redis # For backwards compatibility in type checking from redis.cluster import RedisCluster @@ -15,6 +15,18 @@ from redisvl.types import AsyncRedisClient, SyncRedisClient +class _CacheConnectionKwargs(TypedDict): + """The connection parameters a cache keeps for building its clients. + + Annotated so each value keeps its own type. Without it the dict literal + widens to the union of all three, and every read needs a cast. + """ + + redis_client: SyncRedisClient | None + redis_url: str + connection_kwargs: dict[str, Any] + + class BaseCache: """Base abstract cache interface for all RedisVL caches. @@ -51,7 +63,7 @@ def __init__( self._ttl: int | None = None self.set_ttl(ttl) - self.redis_kwargs = { + self.redis_kwargs: _CacheConnectionKwargs = { "redis_client": redis_client, "redis_url": redis_url, "connection_kwargs": connection_kwargs, @@ -121,8 +133,8 @@ def _get_redis_client(self) -> SyncRedisClient: """ if self._redis_client is None: # Create new Redis client - url = cast(str | None, self.redis_kwargs["redis_url"]) - kwargs = cast(dict[str, Any], self.redis_kwargs["connection_kwargs"]) + url = self.redis_kwargs["redis_url"] + kwargs = self.redis_kwargs["connection_kwargs"] self._redis_client = RedisConnectionFactory.get_redis_connection( redis_url=url, **kwargs, @@ -146,10 +158,8 @@ async def _get_async_redis_client(self) -> AsyncRedisClient: if provided and isinstance(provided, (Redis, RedisCluster)): client = RedisConnectionFactory.sync_to_async_redis(provided) else: - url = cast(str | None, self.redis_kwargs["redis_url"]) - kwargs = cast( - dict[str, Any], self.redis_kwargs["connection_kwargs"] - ) + url = self.redis_kwargs["redis_url"] + kwargs = self.redis_kwargs["connection_kwargs"] client = await RedisConnectionFactory._get_aredis_connection( redis_url=url, **kwargs ) diff --git a/redisvl/extensions/router/semantic.py b/redisvl/extensions/router/semantic.py index f3490d7b..a6ffa9e1 100644 --- a/redisvl/extensions/router/semantic.py +++ b/redisvl/extensions/router/semantic.py @@ -171,10 +171,7 @@ def from_existing( overwrite = kwargs.pop("overwrite", False) if not create_index and overwrite: raise ValueError(CREATE_INDEX_OVERWRITE_CONFLICT) - init_kwargs, connection_kwargs = _split_from_existing_kwargs( - dict(kwargs), - nested_connection_keys=("connection_kwargs",), - ) + init_kwargs, connection_kwargs = _split_from_existing_kwargs(dict(kwargs)) lib_name = init_kwargs.get("lib_name") index_kwargs: dict[str, Any] = {} created_redis_client = False diff --git a/redisvl/index/index.py b/redisvl/index/index.py index 8eddcfff..6effd34f 100644 --- a/redisvl/index/index.py +++ b/redisvl/index/index.py @@ -11,6 +11,7 @@ Any, AsyncGenerator, Callable, + ClassVar, Generator, Iterable, Iterator, @@ -39,7 +40,7 @@ make_dict, ) from redisvl.types import AsyncRedisClient, SyncRedisClient, SyncRedisCluster -from redisvl.utils.utils import deprecated_argument, deprecated_function, sync_wrapper +from redisvl.utils.utils import deprecated_argument, sync_wrapper if TYPE_CHECKING: from redis.commands.search.aggregation import AggregateResult @@ -75,6 +76,7 @@ from redisvl.query.filter import FilterExpression from redisvl.redis.connection import ( RedisConnectionFactory, + _reject_removed_kwargs, _split_from_existing_kwargs, convert_index_info_to_schema, supports_svs, @@ -570,9 +572,44 @@ class BaseSearchIndex: _client_finalizer: "weakref.finalize | None" = None + # Set per subclass: the client classes this index cannot use, what it + # needs instead, and where to send the caller. + _wrong_flavour_clients: ClassVar[tuple[type, ...]] + _wrong_flavour_needs: ClassVar[str] + _wrong_flavour_advice: ClassVar[str] + def __init__(*args, **kwargs): pass + @classmethod + def _reject_wrong_client_flavour(cls, redis_client: Any) -> None: + """Reject a sync client on an async index, or the reverse. + + Uses ``isinstance`` rather than ``issubclass(type(...))`` so a + ``Mock(spec=...)`` of the wrong flavour is caught, since it sets + ``__class__``, while an unspecced mock still passes. Rests on the sync + and async client hierarchies staying disjoint, which holds across the + supported redis-py range. + + Deliberately one-sided: it rejects the wrong flavour but does not + assert the object is a Redis client at all, so a connection pool or + an unrelated object still gets through and fails later. A positive + check would reject the unspecced mocks the test suite relies on. + + Called from ``__init__`` and, ahead of the broader + ``validate_sync_redis``/``validate_async_redis``, from + ``from_existing``, so this message wins on both paths. + """ + if isinstance(redis_client, cls._wrong_flavour_clients): + wrong = type(redis_client) + # Qualified name because redis.Redis and redis.asyncio.Redis share + # a bare __name__, which would make the message useless here. + raise TypeError( + f"{cls.__name__} requires {cls._wrong_flavour_needs}, got " + f"{wrong.__module__}.{wrong.__qualname__}. " + f"{cls._wrong_flavour_advice}" + ) + def _register_client_finalizer(self, client: Any) -> None: """Register a finalizer that closes ``client`` once this index is garbage collected. @@ -811,9 +848,20 @@ class SearchIndex(BaseSearchIndex): # delete index and data index.delete(drop=True) + Pass ``redis_client`` to use a client you configured yourself. The index + leaves such a client open when it is disconnected or garbage collected; + pass ``owns_client=True`` to hand that responsibility over. + + .. code-block:: python + + from redis import Redis + from redisvl.index import SearchIndex + + client = Redis.from_url("redis://localhost:6379", socket_timeout=5) + index = SearchIndex.from_yaml("schemas/schema.yaml", redis_client=client) + """ - @deprecated_argument("connection_args", "Use connection_kwargs instead.") def __init__( self, schema: IndexSchema, @@ -843,13 +891,20 @@ def __init__( only if it created one itself. Pass True to hand over a client you created, or False to keep one the index would otherwise close, in which case closing it becomes your responsibility. + + Raises: + ValueError: If ``schema`` is not an IndexSchema. + TypeError: If ``redis_client`` is the wrong flavour, or if a + keyword that no longer exists is passed. ``connection_args`` + and ``redis_kwargs`` are both now ``connection_kwargs``. """ - if "connection_args" in kwargs: - connection_kwargs = kwargs.pop("connection_args") + _reject_removed_kwargs(kwargs) if not isinstance(schema, IndexSchema): raise ValueError("Must provide a valid IndexSchema object") + self._reject_wrong_client_flavour(redis_client) + self.schema = schema self._validate_on_load = validate_on_load self._lib_name: str | None = kwargs.pop("lib_name", None) @@ -862,13 +917,6 @@ def __init__( self._sql_executors: dict[str, Any] = {} self._validated_client = kwargs.pop("_client_validated", False) - if "_owns_redis_client" in kwargs: - # Underscore-prefixed kwargs are forwarded verbatim by - # _split_from_existing_kwargs, so this would otherwise be dropped - # in silence and leak the connection it used to control. - raise TypeError( - "_owns_redis_client is no longer accepted; use owns_client instead" - ) # Must be assigned before _register_client_finalizer, which gates on # this flag. self._owns_redis_client = ( @@ -881,6 +929,11 @@ def __init__( self._register_client_finalizer(redis_client) _finalizer_close_client = staticmethod(_close_owned_sync_client) + _wrong_flavour_clients = (AsyncRedis, AsyncRedisCluster) + _wrong_flavour_needs = "a sync Redis client" + _wrong_flavour_advice = ( + "Pass a sync client here, or use AsyncSearchIndex for async clients." + ) def disconnect(self): """Close the Redis client if this index owns it. @@ -918,18 +971,21 @@ def from_existing( owns_client (Optional[bool], optional): Whether the index closes the client. Defaults to True when this method created the client from `redis_url`, and False when you supplied one. + connection_kwargs (Optional[Dict[str, Any]]): Redis client + connection args, used only when this method creates the + client from ``redis_url``. Other keyword arguments are + treated the same way. Raises: ValueError: If redis_url or redis_client is not provided. """ - init_kwargs, connection_kwargs = _split_from_existing_kwargs( - dict(kwargs), - nested_connection_keys=("connection_kwargs", "connection_args"), - ) + init_kwargs, connection_kwargs = _split_from_existing_kwargs(dict(kwargs)) lib_name = cast(str | None, init_kwargs.get("lib_name")) created_redis_client = False if redis_client: + # Ahead of validate_*_redis, whose message is generic. + cls._reject_wrong_client_flavour(redis_client) # Validate client type and set lib name RedisConnectionFactory.validate_sync_redis(redis_client, lib_name) # Mark that client was already validated to avoid duplicate calls @@ -996,52 +1052,6 @@ def _redis_client(self) -> SyncRedisClient: self._validated_client = True return self.__redis_client - @deprecated_function("connect", "Pass connection parameters in __init__.") - def connect(self, redis_url: str | None = None, **kwargs): - """Connect to a Redis instance using the provided `redis_url`, falling - back to the `REDIS_URL` environment variable (if available). - - Note: Additional keyword arguments (`**kwargs`) can be used to provide - extra options specific to the Redis connection. - - Args: - redis_url (Optional[str], optional): The URL of the Redis server to - connect to. - - Raises: - redis.exceptions.ConnectionError: If the connection to the Redis - server fails. - ValueError: If the Redis URL is not provided nor accessible - through the `REDIS_URL` environment variable. - ModuleNotFoundError: If required Redis modules are not installed. - """ - self.invalidate_sql_schema_cache() - self.__redis_client = RedisConnectionFactory.get_redis_connection( - redis_url=redis_url, **kwargs - ) - self._register_client_finalizer(self.__redis_client) - - @deprecated_function("set_client", "Pass connection parameters in __init__.") - def set_client(self, redis_client: SyncRedisClient, **kwargs): - """Manually set the Redis client to use with the search index. - - This method configures the search index to use a specific Redis or - Async Redis client. It is useful for cases where an external, - custom-configured client is preferred instead of creating a new one. - - Args: - redis_client (Redis): A Redis or Async Redis - client instance to be used for the connection. - - Raises: - TypeError: If the provided client is not valid. - """ - RedisConnectionFactory.validate_sync_redis(redis_client) - self.invalidate_sql_schema_cache() - self.__redis_client = redis_client - self._register_client_finalizer(redis_client) - return self - def _check_svs_support(self) -> None: """Validate SVS-VAMANA support. @@ -2150,9 +2160,20 @@ class AsyncSearchIndex(BaseSearchIndex): # delete index and data await index.delete(drop=True) + Pass ``redis_client`` to use a client you configured yourself. The index + leaves such a client open when it is disconnected or garbage collected; + pass ``owns_client=True`` to hand that responsibility over. + + .. code-block:: python + + from redis.asyncio import Redis + from redisvl.index import AsyncSearchIndex + + client = Redis.from_url("redis://localhost:6379", socket_timeout=5) + index = AsyncSearchIndex.from_yaml("schemas/schema.yaml", redis_client=client) + """ - @deprecated_argument("redis_kwargs", "Use connection_kwargs instead.") def __init__( self, schema: IndexSchema, @@ -2182,14 +2203,21 @@ def __init__( only if it created one itself. Pass True to hand over a client you created, or False to keep one the index would otherwise close, in which case closing it becomes your responsibility. + + Raises: + ValueError: If ``schema`` is not an IndexSchema. + TypeError: If ``redis_client`` is the wrong flavour, or if a + keyword that no longer exists is passed. ``connection_args`` + and ``redis_kwargs`` are both now ``connection_kwargs``. """ - if "redis_kwargs" in kwargs: - connection_kwargs = kwargs.pop("redis_kwargs") + _reject_removed_kwargs(kwargs) # final validation on schema object if not isinstance(schema, IndexSchema): raise ValueError("Must provide a valid IndexSchema object") + self._reject_wrong_client_flavour(redis_client) + self.schema = schema self._validate_on_load = validate_on_load self._lib_name: str | None = kwargs.pop("lib_name", None) @@ -2206,13 +2234,6 @@ def __init__( self._sql_executors: dict[str, Any] = {} self._validated_client = kwargs.pop("_client_validated", False) - if "_owns_redis_client" in kwargs: - # Underscore-prefixed kwargs are forwarded verbatim by - # _split_from_existing_kwargs, so this would otherwise be dropped - # in silence and leak the connection it used to control. - raise TypeError( - "_owns_redis_client is no longer accepted; use owns_client instead" - ) # Must be assigned before _register_client_finalizer, which gates on # this flag. self._owns_redis_client = ( @@ -2225,6 +2246,13 @@ def __init__( self._register_client_finalizer(redis_client) _finalizer_close_client = staticmethod(_close_owned_async_client) + _wrong_flavour_clients = (Redis, RedisCluster) + _wrong_flavour_needs = "an async Redis client" + _wrong_flavour_advice = ( + "Pass an async client here, use SearchIndex for sync clients, or " + "convert a non-cluster client with " + "RedisConnectionFactory.sync_to_async_redis()." + ) @classmethod async def from_existing( @@ -2246,20 +2274,26 @@ async def from_existing( owns_client (Optional[bool], optional): Whether the index closes the client. Defaults to True when this method created the client from `redis_url`, and False when you supplied one. + connection_kwargs (Optional[Dict[str, Any]]): Redis client + connection args, used only when this method creates the + client from ``redis_url``. Other keyword arguments are + treated the same way. """ + # Split before the presence check below: redis_kwargs was the async + # alias, so a caller passing it with no client would otherwise get the + # generic ValueError rather than being told the current spelling. + init_kwargs, connection_kwargs = _split_from_existing_kwargs(dict(kwargs)) + if not redis_url and not redis_client: raise ValueError( "Must provide either a redis_url or redis_client to fetch Redis index info." ) - - init_kwargs, connection_kwargs = _split_from_existing_kwargs( - dict(kwargs), - nested_connection_keys=("connection_kwargs", "redis_kwargs"), - ) lib_name = cast(str | None, init_kwargs.get("lib_name")) created_redis_client = False if redis_client: + # Ahead of validate_*_redis, whose message is generic. + cls._reject_wrong_client_flavour(redis_client) # Validate client type and set lib name await RedisConnectionFactory.validate_async_redis(redis_client, lib_name) # Mark that client was already validated to avoid duplicate calls @@ -2301,33 +2335,6 @@ def client(self) -> AsyncRedisClient | None: """The underlying redis-py client object.""" return self._redis_client - @deprecated_function("connect", "Pass connection parameters in __init__.") - async def connect(self, redis_url: str | None = None, **kwargs): - """[DEPRECATED] Connect to a Redis instance. Use connection parameters in __init__.""" - warnings.warn( - "connect() is deprecated; pass connection parameters in __init__", - DeprecationWarning, - ) - self.invalidate_sql_schema_cache() - client = await RedisConnectionFactory._get_aredis_connection( - redis_url=redis_url, **kwargs - ) - await self.set_client(client) - - @deprecated_function("set_client", "Pass connection parameters in __init__.") - async def set_client(self, redis_client: AsyncRedisClient | SyncRedisClient): - """ - [DEPRECATED] Manually set the Redis client to use with the search index. - This method is deprecated; please provide connection parameters in __init__. - """ - redis_client = await self._validate_client(redis_client) - self.invalidate_sql_schema_cache() - await self.disconnect() - async with self._lock: - self._redis_client = redis_client - self._register_client_finalizer(redis_client) - return self - async def _get_client(self) -> AsyncRedisClient: """Lazily instantiate and return the async Redis client.""" if self._redis_client is None: @@ -2353,30 +2360,6 @@ async def _get_client(self) -> AsyncRedisClient: self._validated_client = True return self._redis_client - async def _validate_client( - self, redis_client: AsyncRedisClient | SyncRedisClient - ) -> AsyncRedisClient: - # Handle deprecated sync client conversion - if isinstance(redis_client, (Redis, RedisCluster)): - warnings.warn( - "Passing a sync Redis client to AsyncSearchIndex is deprecated " - "and will be removed in the next major version. Please use an " - "async Redis client instead.", - DeprecationWarning, - ) - # Use a new variable name - async_redis_client: AsyncRedisClient = ( - RedisConnectionFactory.sync_to_async_redis(redis_client) - ) - return async_redis_client # Return the converted client - # Check if it's a valid async client (standard or cluster) - elif not isinstance(redis_client, (AsyncRedis, AsyncRedisCluster)): - raise ValueError( - "Invalid async client type: must be AsyncRedis or AsyncRedisCluster" - ) - # If it passed the elif, it's already an AsyncRedisClient - return redis_client - @staticmethod async def _info(name: str, redis_client: AsyncRedisClient) -> dict[str, Any]: try: @@ -3390,6 +3373,9 @@ def disconnect_sync(self): For callers outside an event loop, such as ``__del__`` or a shutdown hook. Honours ``owns_client`` exactly as :meth:`disconnect` does. + + Does nothing if a loop is already running, since it cannot await the + close from inside one. Use :meth:`disconnect` there. """ if self._redis_client is None or not self._owns_redis_client: return diff --git a/redisvl/redis/connection.py b/redisvl/redis/connection.py index d4e44ea8..1acacb8c 100644 --- a/redisvl/redis/connection.py +++ b/redisvl/redis/connection.py @@ -1,5 +1,6 @@ import os -from typing import Any, Sequence, TypeVar, overload +from collections.abc import Mapping +from typing import Any, TypeVar, overload from urllib.parse import parse_qs, urlencode, urlparse, urlunparse from warnings import warn @@ -29,9 +30,44 @@ logger = get_logger(__name__) +# Old spellings and their current equivalents. Kept as a table so **kwargs +# cannot swallow one in silence: a caller who passes an old name is told the +# current one, rather than having it ignored or forwarded to redis-py as an +# unexpected argument. +_REMOVED_CONNECTION_KWARGS = { + "connection_args": "connection_kwargs", + "redis_kwargs": "connection_kwargs", + "_owns_redis_client": "owns_client", +} + + +def _reject_removed_kwargs(kwargs: Mapping[str, Any]) -> None: + """Raise if a caller passed a keyword that no longer exists. + + The message names the keyword and its replacement only. Connection + keywords carry passwords, so echoing a rejected value would put a live + credential into an exception string and from there into logs. + + It says "not a supported keyword" rather than "no longer supported" + because the callers sharing this table never all accepted every name. + """ + for name, replacement in _REMOVED_CONNECTION_KWARGS.items(): + if name in kwargs: + raise TypeError(f"{name} is not a supported keyword; use {replacement}") + + def _split_from_existing_kwargs( - kwargs: dict[str, Any], *, nested_connection_keys: Sequence[str] + kwargs: dict[str, Any], ) -> tuple[dict[str, Any], dict[str, Any]]: + """Split ``from_existing`` kwargs into constructor and connection halves. + + Consumes ``kwargs`` in place. ``connection_kwargs`` is merged flat into + the connection half, so a caller may either nest or spread it. Removed + keywords are rejected first, so a stale spelling fails here rather than + reaching redis-py as an unexpected argument. + """ + _reject_removed_kwargs(kwargs) + init_kwargs: dict[str, Any] = {} connection_kwargs: dict[str, Any] = {} @@ -43,10 +79,9 @@ def _split_from_existing_kwargs( if key.startswith("_"): init_kwargs[key] = kwargs.pop(key) - for key in nested_connection_keys: - nested_kwargs = kwargs.pop(key, None) - if nested_kwargs is not None: - connection_kwargs.update(nested_kwargs) + nested_kwargs = kwargs.pop("connection_kwargs", None) + if nested_kwargs is not None: + connection_kwargs.update(nested_kwargs) connection_kwargs.update(kwargs) return init_kwargs, connection_kwargs diff --git a/tests/integration/test_async_search_index.py b/tests/integration/test_async_search_index.py index c52387e6..ed2109f2 100644 --- a/tests/integration/test_async_search_index.py +++ b/tests/integration/test_async_search_index.py @@ -1,10 +1,8 @@ -import warnings from random import choice from unittest import mock import pytest import redis -from redis import Redis as SyncRedis from redis.asyncio import Redis as AsyncRedis from redisvl.exceptions import QueryValidationError, RedisSearchError, RedisVLError @@ -208,29 +206,6 @@ async def test_search_index_client(async_client, index_schema): assert async_index.client == async_client -@pytest.mark.asyncio -async def test_search_index_set_client(client, redis_url, index_schema): - # Use async with for the index that owns its initial client via redis_url - async with AsyncSearchIndex( - schema=index_schema, redis_url=redis_url - ) as async_index: - # Ignore deprecation warnings for set_client - with warnings.catch_warnings(): - warnings.filterwarnings("ignore", category=DeprecationWarning) - await async_index.create(overwrite=True, drop=True) - assert isinstance(async_index.client, AsyncRedis) - - # Tests deprecated sync -> async conversion behavior - assert isinstance(client, SyncRedis) - - await async_index.set_client(client) - assert isinstance(async_index.client, AsyncRedis) - - if async_index.client: - await async_index.disconnect() - assert async_index.client is None - - @pytest.mark.asyncio async def test_search_index_create(async_index): await async_index.create(overwrite=True, drop=True) @@ -488,9 +463,24 @@ async def test_search_index_that_owns_client_disconnect(index_schema, redis_url) @pytest.mark.asyncio -async def test_search_index_that_owns_client_disconnect_sync(index_schema, redis_url): +async def test_disconnect_sync_is_a_no_op_inside_a_running_loop( + index_schema, redis_url +): + """disconnect_sync() does nothing when a loop is already running. + + It exists for callers outside an event loop, such as __del__ and + shutdown hooks: sync_wrapper calls loop.run_until_complete, which raises + on an already-running loop and is swallowed. Asserting the no-op here + documents behaviour that would otherwise look like a silent bug. The + path that does close the client is covered outside a loop by + tests/unit/test_index_gc_finalizer.py. + """ async_index = AsyncSearchIndex(schema=index_schema, redis_url=redis_url) await async_index.create(overwrite=True, drop=True) + + async_index.disconnect_sync() + + assert async_index._redis_client is not None await async_index.disconnect() assert async_index._redis_client is None @@ -779,17 +769,6 @@ async def test_search_index_validates_query_with_hnsw_algorithm( await async_hnsw_index.query(query) -@pytest.mark.asyncio -async def test_async_search_index_connect(index_schema, redis_url): - """Test that AsyncSearchIndex.connect() works with redis_url parameter.""" - async_index = AsyncSearchIndex(schema=index_schema) - with warnings.catch_warnings(): - warnings.filterwarnings("ignore", category=DeprecationWarning) - await async_index.connect(redis_url=redis_url) - assert async_index.client is not None - await async_index.disconnect() - - @pytest.mark.asyncio @pytest.mark.parametrize("ttl", [None, 30]) async def test_search_index_load_with_ttl(async_index, ttl): diff --git a/tests/integration/test_search_index.py b/tests/integration/test_search_index.py index f1c6c829..783d328d 100644 --- a/tests/integration/test_search_index.py +++ b/tests/integration/test_search_index.py @@ -1,4 +1,3 @@ -import warnings from random import choice from unittest import mock @@ -276,29 +275,6 @@ def test_search_index_client(client, index_schema): assert index.client == client -def test_search_index_set_client(async_client, redis_url, index_schema): - index = SearchIndex(schema=index_schema, redis_url=redis_url) - - with warnings.catch_warnings(): - warnings.filterwarnings("ignore", category=DeprecationWarning) - index.create(overwrite=True, drop=True) - assert index.client - # should not be able to set an async client here - with pytest.raises(TypeError): - index.set_client(async_client) - assert index.client is not async_client - - index.disconnect() - assert index.client is None - - -def test_search_index_connect(index, redis_url): - with warnings.catch_warnings(): - warnings.filterwarnings("ignore", category=DeprecationWarning) - index.connect(redis_url=redis_url) - assert index.client - - def test_search_index_create(index): index.create(overwrite=True, drop=True) assert index.exists() diff --git a/tests/unit/test_connection_normalization.py b/tests/unit/test_connection_normalization.py index 28560f79..14ded504 100644 --- a/tests/unit/test_connection_normalization.py +++ b/tests/unit/test_connection_normalization.py @@ -3,11 +3,14 @@ from unittest.mock import AsyncMock, MagicMock, patch import pytest +from redis import Redis +from redis.asyncio import Redis as AsyncRedis from redisvl.extensions.cache.embeddings import EmbeddingsCache from redisvl.extensions.router.semantic import SemanticRouter from redisvl.index import AsyncSearchIndex, SearchIndex from redisvl.query.sql import SQLQuery +from redisvl.schema import IndexSchema from redisvl.utils.utils import assert_no_warnings @@ -436,3 +439,78 @@ def test_sql_query_does_not_create_new_connection_when_client_provided(): schema_cache_strategy="lazy", ) assert command == "FT.SEARCH idx *" + + +@pytest.mark.parametrize( + "factory", + [ + lambda **kw: SearchIndex(IndexSchema.from_dict(_schema_dict()), **kw), + lambda **kw: AsyncSearchIndex(IndexSchema.from_dict(_schema_dict()), **kw), + ], + ids=["sync", "async"], +) +@pytest.mark.parametrize("removed", ["connection_args", "redis_kwargs"]) +def test_constructors_reject_removed_connection_kwargs(factory, removed): + """Removed keywords must fail loudly, not be swallowed by **kwargs.""" + with pytest.raises(TypeError) as excinfo: + factory(**{removed: {"decode_responses": True}}) + + assert removed in str(excinfo.value) + assert "connection_kwargs" in str(excinfo.value) + + +def test_from_existing_rejects_removed_connection_kwargs(): + """The from_existing path routed these into connection_kwargs before.""" + with pytest.raises(TypeError) as excinfo: + SearchIndex.from_existing( + "idx", redis_url="redis://localhost:6379", connection_args={"db": 1} + ) + + assert "connection_kwargs" in str(excinfo.value) + + +def test_rejection_message_never_echoes_the_value(): + """Connection kwargs carry passwords; the message names keys only.""" + with pytest.raises(TypeError) as excinfo: + SearchIndex( + IndexSchema.from_dict(_schema_dict()), + connection_args={"password": "s3cr3t-do-not-log"}, + ) + + assert "s3cr3t-do-not-log" not in str(excinfo.value) + + +@pytest.mark.parametrize( + "index_cls, wrong_client", + [ + (SearchIndex, AsyncRedis.from_url("redis://localhost:6379")), + (AsyncSearchIndex, Redis.from_url("redis://localhost:6379")), + ], + ids=["sync-index-async-client", "async-index-sync-client"], +) +def test_constructors_reject_the_wrong_client_flavour(index_cls, wrong_client): + """A mismatched client fails at construction, not at first use. + + For the sync index this rejection was previously reachable only through + the removed set_client(); for the async index the removed + _validate_client() silently converted the client instead. + """ + with pytest.raises(TypeError) as excinfo: + index_cls(IndexSchema.from_dict(_schema_dict()), redis_client=wrong_client) + + assert "Redis client" in str(excinfo.value) + + +def test_wrong_flavour_guard_catches_specced_mocks(): + """Pins the isinstance choice the guard documents. + + Mock(spec=...) sets __class__, so isinstance catches a mis-flavoured + spec'd mock where issubclass(type(...)) would not. An unspecced mock must + still pass, because much of the suite injects one. + """ + schema = IndexSchema.from_dict(_schema_dict()) + + with pytest.raises(TypeError): + SearchIndex(schema, redis_client=MagicMock(spec=AsyncRedis)) + + assert SearchIndex(schema, redis_client=MagicMock()) is not None diff --git a/tests/unit/test_index_gc_finalizer.py b/tests/unit/test_index_gc_finalizer.py index edc0aed0..5d2a4f43 100644 --- a/tests/unit/test_index_gc_finalizer.py +++ b/tests/unit/test_index_gc_finalizer.py @@ -229,9 +229,9 @@ class TestOwnsClientHandover: """``owns_client`` overrides who closes the client. By default an index closes only a client it created itself. These tests - cover the two explicit overrides at construction, which is where - ownership should be stated: the deprecated ``set_client()`` inherits - whatever ownership the index already had, so it can hand the index a + cover the two explicit overrides at construction, which is now the only + place ownership can be stated: the removed ``set_client()`` inherited + whatever ownership the index already had, and so could be handed a caller's client and then close it. """