From 0bbc251fbde597de571defbb21ef9b012faeee77 Mon Sep 17 00:00:00 2001 From: Vishal Bala Date: Thu, 3 Sep 2026 10:51:58 +0200 Subject: [PATCH 1/4] refactor(index)!: remove the deprecated connect and set_client methods SearchIndex.connect, SearchIndex.set_client and their async counterparts have warned since v0.4.0, and AsyncSearchIndex.connect emitted three DeprecationWarnings per call because it delegated to set_client. None had working ownership semantics: set_client attached a finalizer based on whatever ownership the index already had, so it closed a caller's client on an index built from redis_url and never took ownership on one built with a client. Pass connection parameters to the constructor instead, with owns_client when the index should close a client you supplied. AsyncSearchIndex._validate_client goes with them, since set_client was its only caller. It carried the sync-to-async client coercion, so handing an async index a sync client is now rejected rather than silently converted. Both constructors gained a guard for the wrong client flavour, which also restores the rejection set_client used to provide for the sync index. connection_args and redis_kwargs are removed too. They had a second, undecorated entry point through from_existing that warned about nothing, so the deprecated spelling would otherwise have outlived the removal. Both now raise a TypeError naming connection_kwargs, because **kwargs would otherwise swallow them in silence; the message names keywords only, never values, since connection kwargs carry passwords. With those gone every caller of _split_from_existing_kwargs passes the same tuple, so nested_connection_keys is inlined. One side effect worth recording: @deprecated_argument wraps the function it decorates in an untyped wrapper, so mypy had never checked a single call site of either constructor. Removing it surfaced four pre-existing type errors in the semantic cache, fixed here with the cast idiom the cache base module already uses. --- redisvl/extensions/cache/llm/semantic.py | 16 ++- redisvl/extensions/router/semantic.py | 5 +- redisvl/index/index.py | 135 ++++--------------- redisvl/redis/connection.py | 35 ++++- tests/integration/test_async_search_index.py | 41 ++---- tests/integration/test_search_index.py | 28 +--- tests/unit/test_connection_normalization.py | 40 ++++++ 7 files changed, 119 insertions(+), 181 deletions(-) diff --git a/redisvl/extensions/cache/llm/semantic.py b/redisvl/extensions/cache/llm/semantic.py index 87383057..cd99e3f6 100644 --- a/redisvl/extensions/cache/llm/semantic.py +++ b/redisvl/extensions/cache/llm/semantic.py @@ -1,5 +1,5 @@ import asyncio -from typing import Any +from typing import Any, cast from pydantic import ValidationError from redis import Redis @@ -187,8 +187,11 @@ def __init__( self._index = SearchIndex( schema=schema, redis_client=self._redis_client, - redis_url=self.redis_kwargs["redis_url"], - connection_kwargs=self.redis_kwargs["connection_kwargs"] or None, + redis_url=cast(str | None, self.redis_kwargs["redis_url"]), + connection_kwargs=cast( + "dict[str, Any] | None", self.redis_kwargs["connection_kwargs"] + ) + or None, ) self._aindex = None @@ -260,8 +263,11 @@ async def _get_async_index(self) -> AsyncSearchIndex: self._aindex = AsyncSearchIndex( schema=self._index.schema, redis_client=async_client, - redis_url=self.redis_kwargs["redis_url"], - connection_kwargs=self.redis_kwargs["connection_kwargs"] or None, + redis_url=cast(str | None, self.redis_kwargs["redis_url"]), + connection_kwargs=cast( + "dict[str, Any] | None", self.redis_kwargs["connection_kwargs"] + ) + or None, ) return self._aindex 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..5b68089e 100644 --- a/redisvl/index/index.py +++ b/redisvl/index/index.py @@ -39,7 +39,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 +75,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, @@ -813,7 +814,6 @@ class SearchIndex(BaseSearchIndex): """ - @deprecated_argument("connection_args", "Use connection_kwargs instead.") def __init__( self, schema: IndexSchema, @@ -844,12 +844,21 @@ def __init__( you created, or False to keep one the index would otherwise close, in which case closing it becomes your responsibility. """ - 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") + # Reject the wrong client flavour up front. isinstance, not + # issubclass(type(...)): Mock(spec=AsyncRedis) sets __class__, so this + # catches a mis-flavoured spec'd mock while leaving bare mocks alone. + # Rests on the sync and async hierarchies staying disjoint. + if isinstance(redis_client, (AsyncRedis, AsyncRedisCluster)): + raise TypeError( + "SearchIndex requires a sync Redis client; pass an async " + "client to AsyncSearchIndex instead." + ) + self.schema = schema self._validate_on_load = validate_on_load self._lib_name: str | None = kwargs.pop("lib_name", None) @@ -922,10 +931,7 @@ def from_existing( 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 @@ -996,52 +1002,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. @@ -2152,7 +2112,6 @@ class AsyncSearchIndex(BaseSearchIndex): """ - @deprecated_argument("redis_kwargs", "Use connection_kwargs instead.") def __init__( self, schema: IndexSchema, @@ -2183,13 +2142,19 @@ def __init__( you created, or False to keep one the index would otherwise close, in which case closing it becomes your responsibility. """ - 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") + # See the note in SearchIndex.__init__ on why this is isinstance. + if isinstance(redis_client, (Redis, RedisCluster)): + raise TypeError( + "AsyncSearchIndex requires an async Redis client; pass a sync " + "client to SearchIndex instead." + ) + self.schema = schema self._validate_on_load = validate_on_load self._lib_name: str | None = kwargs.pop("lib_name", None) @@ -2252,10 +2217,7 @@ async def from_existing( "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"), - ) + init_kwargs, connection_kwargs = _split_from_existing_kwargs(dict(kwargs)) lib_name = cast(str | None, init_kwargs.get("lib_name")) created_redis_client = False @@ -2301,33 +2263,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 +2288,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: diff --git a/redisvl/redis/connection.py b/redisvl/redis/connection.py index d4e44ea8..1bf80646 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,32 @@ logger = get_logger(__name__) +# Connection keywords that were removed rather than renamed. Kept as a table so +# a caller who passes one gets told what to use instead, rather than having the +# name silently ignored or forwarded to redis-py as an unexpected argument. +REMOVED_CONNECTION_KWARGS = { + "connection_args": "connection_kwargs", + "redis_kwargs": "connection_kwargs", +} + + +def _reject_removed_kwargs(kwargs: Mapping[str, Any]) -> None: + """Raise if a caller passed a keyword that has been removed. + + 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. + """ + for name, replacement in REMOVED_CONNECTION_KWARGS.items(): + if name in kwargs: + raise TypeError(f"{name} is no longer supported; use {replacement} instead") + + 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]]: + _reject_removed_kwargs(kwargs) + init_kwargs: dict[str, Any] = {} connection_kwargs: dict[str, Any] = {} @@ -43,10 +67,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..863a15ef 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,27 +206,15 @@ 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) +def test_async_search_index_rejects_sync_client(client, index_schema): + """An async index must not accept a sync client. - if async_index.client: - await async_index.disconnect() - assert async_index.client is None + The removed set_client() silently converted one via + sync_to_async_redis; the constructor now rejects it outright, so the + mismatch surfaces at construction rather than at the first await. + """ + with pytest.raises(TypeError): + AsyncSearchIndex(schema=index_schema, redis_client=client) @pytest.mark.asyncio @@ -779,17 +765,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..55c43b59 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,27 +275,14 @@ 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 +def test_search_index_rejects_async_client(async_client, index_schema): + """A sync index must not accept an 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 + Previously only reachable through the removed set_client(); the + constructor now rejects it before any connection is attempted. + """ + with pytest.raises(TypeError): + SearchIndex(schema=index_schema, redis_client=async_client) def test_search_index_create(index): diff --git a/tests/unit/test_connection_normalization.py b/tests/unit/test_connection_normalization.py index 28560f79..e58cfb1a 100644 --- a/tests/unit/test_connection_normalization.py +++ b/tests/unit/test_connection_normalization.py @@ -8,6 +8,7 @@ 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 +437,42 @@ 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) From 29b4900c882313ab64250643467035fee1d662c9 Mon Sep 17 00:00:00 2001 From: Vishal Bala Date: Thu, 3 Sep 2026 11:16:56 +0200 Subject: [PATCH 2/4] refactor(index): address review of the deprecated-client removal MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Six review perspectives ran against the removal. Their substantive findings, in one commit because they overlap: The removed-keyword table was unreachable on one path that mattered. AsyncSearchIndex.from_existing checked for a client before splitting kwargs, so a caller passing redis_kwargs — the async alias, and so the likeliest caller — got a generic "must provide redis_url or redis_client" instead of being told the current spelling. Split first. The wrong-flavour guard was duplicated in both constructors and unreachable from from_existing, where validate_sync_redis fired first with a message that says what is wrong but not what to do. It now lives on BaseSearchIndex, driven by two class attributes, and runs ahead of the broader validation on both paths. Its message leads with what the index needs, names the class it got — qualified, since redis.Redis and redis.asyncio.Redis share a bare __name__ — and ends with the advice, which for the async index now includes sync_to_async_redis for callers who relied on the coercion that went away with _validate_client. The guard is deliberately one-sided: it rejects the wrong flavour but does not assert the object is a client at all, because a positive check would reject the unspecced mocks much of the suite injects. That is now recorded in the docstring rather than left for a reader to infer, and the isinstance choice is pinned by a test instead of only asserted in a comment. _owns_redis_client folds into the same table, replacing two inline blocks and leaving one mechanism for "that keyword is gone" rather than two. The table drops its claim that these keywords were "removed rather than renamed", which its own values contradicted, and no longer says "no longer supported" — the callers sharing it never all accepted every name, so the router was telling users a keyword it never took had been withdrawn. Both guard tests move to the unit suite: neither touches Redis, and the sync one requested the async fixture, which pytest-asyncio 0.24 stopped allowing. Also fixes a pre-existing duplicate in the async ownership tests, where the disconnect_sync case awaited disconnect() instead and so left disconnect_sync on an owned client untested. --- redisvl/index/index.py | 90 +++++++++++++------- redisvl/redis/connection.py | 19 +++-- tests/integration/test_async_search_index.py | 13 +-- tests/integration/test_search_index.py | 10 --- tests/unit/test_connection_normalization.py | 38 +++++++++ tests/unit/test_index_gc_finalizer.py | 6 +- 6 files changed, 113 insertions(+), 63 deletions(-) diff --git a/redisvl/index/index.py b/redisvl/index/index.py index 5b68089e..a5112a65 100644 --- a/redisvl/index/index.py +++ b/redisvl/index/index.py @@ -11,6 +11,7 @@ Any, AsyncGenerator, Callable, + ClassVar, Generator, Iterable, Iterator, @@ -571,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. @@ -849,15 +885,7 @@ def __init__( if not isinstance(schema, IndexSchema): raise ValueError("Must provide a valid IndexSchema object") - # Reject the wrong client flavour up front. isinstance, not - # issubclass(type(...)): Mock(spec=AsyncRedis) sets __class__, so this - # catches a mis-flavoured spec'd mock while leaving bare mocks alone. - # Rests on the sync and async hierarchies staying disjoint. - if isinstance(redis_client, (AsyncRedis, AsyncRedisCluster)): - raise TypeError( - "SearchIndex requires a sync Redis client; pass an async " - "client to AsyncSearchIndex instead." - ) + self._reject_wrong_client_flavour(redis_client) self.schema = schema self._validate_on_load = validate_on_load @@ -871,13 +899,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 = ( @@ -890,6 +911,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. @@ -936,6 +962,8 @@ def from_existing( 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 @@ -2148,12 +2176,7 @@ def __init__( if not isinstance(schema, IndexSchema): raise ValueError("Must provide a valid IndexSchema object") - # See the note in SearchIndex.__init__ on why this is isinstance. - if isinstance(redis_client, (Redis, RedisCluster)): - raise TypeError( - "AsyncSearchIndex requires an async Redis client; pass a sync " - "client to SearchIndex instead." - ) + self._reject_wrong_client_flavour(redis_client) self.schema = schema self._validate_on_load = validate_on_load @@ -2171,13 +2194,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 = ( @@ -2190,6 +2206,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( @@ -2212,16 +2235,21 @@ async def from_existing( the client. Defaults to True when this method created the client from `redis_url`, and False when you supplied one. """ + # 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)) 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 diff --git a/redisvl/redis/connection.py b/redisvl/redis/connection.py index 1bf80646..e1763174 100644 --- a/redisvl/redis/connection.py +++ b/redisvl/redis/connection.py @@ -30,25 +30,30 @@ logger = get_logger(__name__) -# Connection keywords that were removed rather than renamed. Kept as a table so -# a caller who passes one gets told what to use instead, rather than having the -# name silently ignored or forwarded to redis-py as an unexpected argument. -REMOVED_CONNECTION_KWARGS = { +# 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 has been removed. + """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(): + for name, replacement in _REMOVED_CONNECTION_KWARGS.items(): if name in kwargs: - raise TypeError(f"{name} is no longer supported; use {replacement} instead") + raise TypeError(f"{name} is not a supported keyword; use {replacement}") def _split_from_existing_kwargs( diff --git a/tests/integration/test_async_search_index.py b/tests/integration/test_async_search_index.py index 863a15ef..6b0185d1 100644 --- a/tests/integration/test_async_search_index.py +++ b/tests/integration/test_async_search_index.py @@ -206,17 +206,6 @@ async def test_search_index_client(async_client, index_schema): assert async_index.client == async_client -def test_async_search_index_rejects_sync_client(client, index_schema): - """An async index must not accept a sync client. - - The removed set_client() silently converted one via - sync_to_async_redis; the constructor now rejects it outright, so the - mismatch surfaces at construction rather than at the first await. - """ - with pytest.raises(TypeError): - AsyncSearchIndex(schema=index_schema, redis_client=client) - - @pytest.mark.asyncio async def test_search_index_create(async_index): await async_index.create(overwrite=True, drop=True) @@ -477,7 +466,7 @@ async def test_search_index_that_owns_client_disconnect(index_schema, redis_url) async def test_search_index_that_owns_client_disconnect_sync(index_schema, redis_url): async_index = AsyncSearchIndex(schema=index_schema, redis_url=redis_url) await async_index.create(overwrite=True, drop=True) - await async_index.disconnect() + async_index.disconnect_sync() assert async_index._redis_client is None diff --git a/tests/integration/test_search_index.py b/tests/integration/test_search_index.py index 55c43b59..783d328d 100644 --- a/tests/integration/test_search_index.py +++ b/tests/integration/test_search_index.py @@ -275,16 +275,6 @@ def test_search_index_client(client, index_schema): assert index.client == client -def test_search_index_rejects_async_client(async_client, index_schema): - """A sync index must not accept an async client. - - Previously only reachable through the removed set_client(); the - constructor now rejects it before any connection is attempted. - """ - with pytest.raises(TypeError): - SearchIndex(schema=index_schema, redis_client=async_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 e58cfb1a..14ded504 100644 --- a/tests/unit/test_connection_normalization.py +++ b/tests/unit/test_connection_normalization.py @@ -3,6 +3,8 @@ 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 @@ -476,3 +478,39 @@ def test_rejection_message_never_echoes_the_value(): ) 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. """ From 950afe807ac40e3afbe0526c6b815979c52b8973 Mon Sep 17 00:00:00 2001 From: Vishal Bala Date: Thu, 3 Sep 2026 11:20:15 +0200 Subject: [PATCH 3/4] docs(index): restore the client-injection guidance the removal dropped set_client()'s docstring was the only place in the rendered API reference that explained using a client you configured yourself, and docs/api/searchindex.rst renders both classes with a bare autoclass, so deleting the method took the explanation with it. redis_client= now appears in no example anywhere. Both class docstrings gain one, along with when the index will and will not close such a client. Neither constructor documented the three TypeErrors it can now raise, so the rename of connection_args and redis_kwargs existed only inside an exception string and a private table. Both from_existing docstrings also omitted connection_kwargs, the keyword those exceptions name as the replacement, even though they accept it through **kwargs. _split_from_existing_kwargs gains a docstring: its name promises splitting, it now also rejects, and it consumes the dict it is handed. Replaces the four casts the previous commit added, plus four that predated it, with a TypedDict on BaseCache.redis_kwargs. The casts were all working around one missing annotation on a heterogeneous dict literal, and cast() asserted a type the checker could not verify, so nothing stopped a later edit from making the assertion false. A TypedDict is a plain dict at runtime, so this is annotation-only. --- redisvl/extensions/cache/base.py | 26 +++++++++----- redisvl/extensions/cache/llm/semantic.py | 16 +++------ redisvl/index/index.py | 44 ++++++++++++++++++++++++ redisvl/redis/connection.py | 7 ++++ 4 files changed, 74 insertions(+), 19 deletions(-) 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/cache/llm/semantic.py b/redisvl/extensions/cache/llm/semantic.py index cd99e3f6..87383057 100644 --- a/redisvl/extensions/cache/llm/semantic.py +++ b/redisvl/extensions/cache/llm/semantic.py @@ -1,5 +1,5 @@ import asyncio -from typing import Any, cast +from typing import Any from pydantic import ValidationError from redis import Redis @@ -187,11 +187,8 @@ def __init__( self._index = SearchIndex( schema=schema, redis_client=self._redis_client, - redis_url=cast(str | None, self.redis_kwargs["redis_url"]), - connection_kwargs=cast( - "dict[str, Any] | None", self.redis_kwargs["connection_kwargs"] - ) - or None, + redis_url=self.redis_kwargs["redis_url"], + connection_kwargs=self.redis_kwargs["connection_kwargs"] or None, ) self._aindex = None @@ -263,11 +260,8 @@ async def _get_async_index(self) -> AsyncSearchIndex: self._aindex = AsyncSearchIndex( schema=self._index.schema, redis_client=async_client, - redis_url=cast(str | None, self.redis_kwargs["redis_url"]), - connection_kwargs=cast( - "dict[str, Any] | None", self.redis_kwargs["connection_kwargs"] - ) - or None, + redis_url=self.redis_kwargs["redis_url"], + connection_kwargs=self.redis_kwargs["connection_kwargs"] or None, ) return self._aindex diff --git a/redisvl/index/index.py b/redisvl/index/index.py index a5112a65..748474cb 100644 --- a/redisvl/index/index.py +++ b/redisvl/index/index.py @@ -848,6 +848,18 @@ 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) + """ def __init__( @@ -879,6 +891,12 @@ 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``. """ _reject_removed_kwargs(kwargs) @@ -953,6 +971,10 @@ 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. @@ -2138,6 +2160,18 @@ 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) + """ def __init__( @@ -2169,6 +2203,12 @@ 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``. """ _reject_removed_kwargs(kwargs) @@ -2234,6 +2274,10 @@ 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 diff --git a/redisvl/redis/connection.py b/redisvl/redis/connection.py index e1763174..1acacb8c 100644 --- a/redisvl/redis/connection.py +++ b/redisvl/redis/connection.py @@ -59,6 +59,13 @@ def _reject_removed_kwargs(kwargs: Mapping[str, Any]) -> None: def _split_from_existing_kwargs( 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] = {} From d9ad737ff370cfc24ec4a5e94d12f735688f533d Mon Sep 17 00:00:00 2001 From: Vishal Bala Date: Thu, 3 Sep 2026 11:24:16 +0200 Subject: [PATCH 4/4] test(index): pin what disconnect_sync actually does in a running loop MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The async ownership tests had two copies of the same assertion: the disconnect_sync case awaited disconnect() instead of calling disconnect_sync(), so the method it was named for was never exercised. Calling it revealed why that mattered — inside a running loop it is a silent no-op, because sync_wrapper reaches loop.run_until_complete on an already-running loop and swallows the RuntimeError. That is the intended design: the method exists for callers outside a loop, such as __del__ and shutdown hooks, and the closing path is covered there by the finalizer unit tests. But an undocumented silent no-op reads as a bug to whoever finds it next, so the test now asserts it and the docstring says it. --- redisvl/index/index.py | 3 +++ tests/integration/test_async_search_index.py | 17 ++++++++++++++++- 2 files changed, 19 insertions(+), 1 deletion(-) diff --git a/redisvl/index/index.py b/redisvl/index/index.py index 748474cb..6effd34f 100644 --- a/redisvl/index/index.py +++ b/redisvl/index/index.py @@ -3373,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/tests/integration/test_async_search_index.py b/tests/integration/test_async_search_index.py index 6b0185d1..ed2109f2 100644 --- a/tests/integration/test_async_search_index.py +++ b/tests/integration/test_async_search_index.py @@ -463,10 +463,25 @@ 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