From d7a234e8ced79030c3112793c8ce7165a1286361 Mon Sep 17 00:00:00 2001 From: Vishal Bala Date: Thu, 3 Sep 2026 14:26:59 +0200 Subject: [PATCH 1/2] refactor(connection)!: remove RedisConnectionFactory.connect and the url keyword MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit connect() was a thin dispatcher over get_redis_connection and get_async_redis_connection, deprecated since v0.4.0, with no callers left anywhere in the library. The url keyword on the two async factories was deprecated in v0.11.0 in favour of redis_url. url is not simply dropped. Left unguarded it reaches is_cluster_url, which takes url positionally, so a caller would get "got multiple values for argument 'url'" — an error naming neither RedisVL nor the rename — or a REDIS_URL ValueError if that variable happened to be unset. All three factories now name the replacement instead. The removed-keyword table splits in two along entry points rather than gaining a fourth shared entry. url belongs to the connection factories and connection_args to the index constructors, and a single table would have told a factory caller that connection_args had been withdrawn when that door never accepted it. TestConnect loses the two tests that only asserted the dispatcher returned a sync or async client, which every fixture in the suite already does. Its three URL-resolution error paths survive on get_redis_connection and are covered nowhere else, so they move rather than go. The module-level filterwarnings that masked connect()'s own warning goes too; its comment already said to remove it with the method. test_url_deprecation.py goes with the keyword. Two of its four tests were degenerate — both asserted pytest.warns and passed on the unrelated "will become async" warning rather than on anything about url. The one property worth keeping, that the main async factory is warning-free on the modern spelling, moves next to the other tests for those factories. It is load-bearing: the cache builds its async client through that factory specifically to avoid warning users about an API they never called. --- redisvl/index/index.py | 5 +- redisvl/redis/connection.py | 79 ++++++++------------------ tests/integration/test_connection.py | 33 ++++------- tests/unit/test_connection_protocol.py | 39 +++++++++++++ tests/unit/test_url_deprecation.py | 67 ---------------------- 5 files changed, 76 insertions(+), 147 deletions(-) delete mode 100644 tests/unit/test_url_deprecation.py diff --git a/redisvl/index/index.py b/redisvl/index/index.py index 6effd34f..7780bb41 100644 --- a/redisvl/index/index.py +++ b/redisvl/index/index.py @@ -75,6 +75,7 @@ from redisvl.query.aggregate import AggregateHybridQuery from redisvl.query.filter import FilterExpression from redisvl.redis.connection import ( + REMOVED_INDEX_KWARGS, RedisConnectionFactory, _reject_removed_kwargs, _split_from_existing_kwargs, @@ -898,7 +899,7 @@ def __init__( keyword that no longer exists is passed. ``connection_args`` and ``redis_kwargs`` are both now ``connection_kwargs``. """ - _reject_removed_kwargs(kwargs) + _reject_removed_kwargs(kwargs, REMOVED_INDEX_KWARGS) if not isinstance(schema, IndexSchema): raise ValueError("Must provide a valid IndexSchema object") @@ -2210,7 +2211,7 @@ def __init__( keyword that no longer exists is passed. ``connection_args`` and ``redis_kwargs`` are both now ``connection_kwargs``. """ - _reject_removed_kwargs(kwargs) + _reject_removed_kwargs(kwargs, REMOVED_INDEX_KWARGS) # final validation on schema object if not isinstance(schema, IndexSchema): diff --git a/redisvl/redis/connection.py b/redisvl/redis/connection.py index 1acacb8c..92a17a56 100644 --- a/redisvl/redis/connection.py +++ b/redisvl/redis/connection.py @@ -23,25 +23,30 @@ SVS_MIN_SEARCH_VERSION, ) from redisvl.redis.utils import convert_bytes, is_cluster_url, make_dict -from redisvl.types import AsyncRedisClient, RedisClient, SyncRedisClient +from redisvl.types import AsyncRedisClient, SyncRedisClient from redisvl.utils.log import get_logger -from redisvl.utils.utils import deprecated_argument, deprecated_function logger = get_logger(__name__) -# Old spellings and their current equivalents. Kept as a table so **kwargs +# Old spellings and their current equivalents. Kept as tables 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 = { +# unexpected argument. Split by entry point so a caller is only told about +# keywords its own door ever accepted. +REMOVED_INDEX_KWARGS = { "connection_args": "connection_kwargs", "redis_kwargs": "connection_kwargs", "_owns_redis_client": "owns_client", } +# Connection factory entry points. +REMOVED_FACTORY_KWARGS = {"url": "redis_url"} -def _reject_removed_kwargs(kwargs: Mapping[str, Any]) -> None: + +def _reject_removed_kwargs( + kwargs: Mapping[str, Any], removed: Mapping[str, str] +) -> None: """Raise if a caller passed a keyword that no longer exists. The message names the keyword and its replacement only. Connection @@ -49,9 +54,9 @@ def _reject_removed_kwargs(kwargs: Mapping[str, Any]) -> None: 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. + because a caller may never have accepted the name in the first place. """ - for name, replacement in _REMOVED_CONNECTION_KWARGS.items(): + for name, replacement in removed.items(): if name in kwargs: raise TypeError(f"{name} is not a supported keyword; use {replacement}") @@ -66,7 +71,7 @@ def _split_from_existing_kwargs( keywords are rejected first, so a stale spelling fails here rather than reaching redis-py as an unexpected argument. """ - _reject_removed_kwargs(kwargs) + _reject_removed_kwargs(kwargs, REMOVED_INDEX_KWARGS) init_kwargs: dict[str, Any] = {} connection_kwargs: dict[str, Any] = {} @@ -618,37 +623,6 @@ class RedisConnectionFactory: configuration. """ - @classmethod - @deprecated_function( - "connect", "Please use `get_redis_connection` or `get_async_redis_connection`." - ) - def connect( - cls, redis_url: str | None = None, use_async: bool = False, **kwargs - ) -> RedisClient: - """Create a connection to the Redis database based on a URL and some - connection kwargs. - - This method sets up either a synchronous or asynchronous Redis client - based on the provided parameters. - - Args: - redis_url (Optional[str]): The URL of the Redis server to connect - to. If not provided, the environment variable REDIS_URL is used. - use_async (bool): If True, an asynchronous client is created. - Defaults to False. - **kwargs: Additional keyword arguments to be passed to the Redis - client constructor. - - Raises: - ValueError: If redis_url is not provided and REDIS_URL environment - variable is not set. - """ - redis_url = redis_url or get_address_from_env() - connection_func = ( - cls.get_async_redis_connection if use_async else cls.get_redis_connection - ) - return connection_func(redis_url, **kwargs) # type: ignore - @staticmethod def get_redis_connection( redis_url: str | None = None, @@ -669,6 +643,7 @@ def get_redis_connection( ValueError: If url is not provided and REDIS_URL environment variable is not set. """ + _reject_removed_kwargs(kwargs, REMOVED_FACTORY_KWARGS) url = redis_url or get_address_from_env() # redis-py 8 defaults to RESP3, which changes raw Search command reply # shapes. Keep RedisVL's existing RESP2 behavior unless requested. @@ -684,7 +659,6 @@ def get_redis_connection( return client @staticmethod - @deprecated_argument("url", "redis_url") async def _get_aredis_connection( redis_url: str | None = None, **kwargs, @@ -695,11 +669,8 @@ async def _get_aredis_connection( only used internally by the library now. Args: - redis_url (Optional[str]): The URL of the Redis server. If neither - `redis_url` nor `url` are provided, the environment variable - REDIS_URL is used. - url (Optional[str]): Former parameter for the URL of the Redis - server. Use `redis_url` instead. (Deprecated) + redis_url (Optional[str]): The URL of the Redis server. If not + provided, the environment variable REDIS_URL is used. **kwargs: Additional keyword arguments to be passed to the async Redis client constructor. @@ -710,8 +681,8 @@ async def _get_aredis_connection( ValueError: If url is not provided and REDIS_URL environment variable is not set. """ - _deprecated_url = kwargs.pop("url", None) - url = _deprecated_url or redis_url or get_address_from_env() + _reject_removed_kwargs(kwargs, REMOVED_FACTORY_KWARGS) + url = redis_url or get_address_from_env() # Keep sync and async clients on the same backward-compatible default. kwargs.setdefault("protocol", 2) @@ -737,7 +708,6 @@ async def _get_aredis_connection( return client @staticmethod - @deprecated_argument("url", "redis_url") def get_async_redis_connection( redis_url: str | None = None, **kwargs, @@ -745,11 +715,8 @@ def get_async_redis_connection( """Creates and returns an asynchronous Redis client. Args: - redis_url (Optional[str]): The URL of the Redis server. If neither - `redis_url` nor `url` are provided, the environment variable - REDIS_URL is used. - url (Optional[str]): Former parameter for the URL of the Redis - server. Use `redis_url` instead. (Deprecated) + redis_url (Optional[str]): The URL of the Redis server. If not + provided, the environment variable REDIS_URL is used. **kwargs: Additional keyword arguments to be passed to the async Redis client constructor. @@ -764,8 +731,8 @@ def get_async_redis_connection( "get_async_redis_connection will become async in a future release.", DeprecationWarning, ) - _deprecated_url = kwargs.pop("url", None) - url = _deprecated_url or redis_url or get_address_from_env() + _reject_removed_kwargs(kwargs, REMOVED_FACTORY_KWARGS) + url = redis_url or get_address_from_env() kwargs.setdefault("protocol", 2) if url.startswith("redis+sentinel"): diff --git a/tests/integration/test_connection.py b/tests/integration/test_connection.py index b62d79be..cb0a6de8 100644 --- a/tests/integration/test_connection.py +++ b/tests/integration/test_connection.py @@ -1,8 +1,6 @@ import os import pytest -from redis import Redis -from redis.asyncio import Redis as AsyncRedis from redis.exceptions import ConnectionError, NoPermissionError from redisvl.redis.connection import ( @@ -19,9 +17,6 @@ EXPECTED_LIB_NAME = f"redis-py(redisvl_v{__version__})" -# Remove after we remove connect() method from RedisConnectionFactory -pytestmark = pytest.mark.filterwarnings("ignore::DeprecationWarning") - def test_unpack_redis_modules(): module_list = [ @@ -102,37 +97,31 @@ def test_convert_index_info_to_schema(): assert schema.index.name == index_info["index_name"] -class TestConnect: - def test_sync_redis_connect(self, redis_url): - client = RedisConnectionFactory.connect(redis_url) - assert client is not None - assert isinstance(client, Redis) - # Perform a simple operation - assert client.ping() +class TestGetRedisConnection: + """URL-resolution error paths, covered nowhere else in the suite. - @pytest.mark.asyncio - async def test_async_redis_connect(self, redis_url): - client = RedisConnectionFactory.connect(redis_url, use_async=True) - assert client is not None - assert isinstance(client, AsyncRedis) - # Perform a simple operation - assert await client.ping() + These moved off the removed RedisConnectionFactory.connect(); the two + tests that only asserted the dispatcher returned a sync or async client + went with it, since every fixture in the suite builds clients this way. + """ def test_missing_env_var(self): redis_url = os.getenv("REDIS_URL") if redis_url: del os.environ["REDIS_URL"] with pytest.raises(ValueError): - RedisConnectionFactory.connect() + RedisConnectionFactory.get_redis_connection() os.environ["REDIS_URL"] = redis_url def test_invalid_url_format(self): with pytest.raises(ValueError): - RedisConnectionFactory.connect(redis_url="invalid_url_format") + RedisConnectionFactory.get_redis_connection(redis_url="invalid_url_format") def test_unknown_redis(self): with pytest.raises(ConnectionError): - bad_client = RedisConnectionFactory.connect(redis_url="redis://fake:1234") + bad_client = RedisConnectionFactory.get_redis_connection( + redis_url="redis://fake:1234" + ) bad_client.ping() diff --git a/tests/unit/test_connection_protocol.py b/tests/unit/test_connection_protocol.py index 55d00050..dbdf8773 100644 --- a/tests/unit/test_connection_protocol.py +++ b/tests/unit/test_connection_protocol.py @@ -3,6 +3,7 @@ import pytest from redisvl.redis.connection import RedisConnectionFactory +from redisvl.utils.utils import assert_no_warnings def test_sync_connection_defaults_to_resp2(): @@ -109,3 +110,41 @@ def test_async_cluster_connection_preserves_explicit_protocol(): ) from_url.assert_called_once_with("redis://localhost:6379", protocol=3) + + +@pytest.mark.asyncio +async def test_aredis_connection_emits_no_warning_on_the_modern_spelling(): + """The main async factory must stay warning-free. + + Carried over from the deleted url-deprecation tests, and load-bearing: + BaseCache builds its async client through this factory specifically to + avoid warning users about an API they never called. + """ + with patch( + "redisvl.redis.connection.AsyncRedis.from_url", return_value=AsyncMock() + ): + with assert_no_warnings(): + await RedisConnectionFactory._get_aredis_connection( + redis_url="redis://localhost:6379" + ) + + +@pytest.mark.parametrize( + "call", + [ + lambda: RedisConnectionFactory.get_redis_connection(url="redis://x:6379"), + lambda: RedisConnectionFactory.get_async_redis_connection(url="redis://x:6379"), + ], + ids=["sync", "async"], +) +def test_factories_reject_the_removed_url_keyword(call): + """url= now names its replacement instead of failing obscurely. + + Left unguarded it reaches is_cluster_url, which takes url positionally, + and raises "got multiple values for argument 'url'" — an error naming + neither RedisVL nor the rename. + """ + with pytest.raises(TypeError) as excinfo: + call() + + assert "redis_url" in str(excinfo.value) diff --git a/tests/unit/test_url_deprecation.py b/tests/unit/test_url_deprecation.py deleted file mode 100644 index 7b01cefa..00000000 --- a/tests/unit/test_url_deprecation.py +++ /dev/null @@ -1,67 +0,0 @@ -import logging -from unittest.mock import AsyncMock, MagicMock, patch - -import pytest - -from redisvl.redis.connection import RedisConnectionFactory -from redisvl.utils.utils import assert_no_warnings - - -class DummyAsyncClient: - async def client_setinfo(self, *args, **kwargs): - return None - - -@pytest.mark.asyncio -async def test__get_aredis_connection_deprecates_url_kwarg_only(): - # Patch AsyncRedis.from_url to avoid real network calls - with patch( - "redisvl.redis.connection.AsyncRedis.from_url", return_value=DummyAsyncClient() - ): - with pytest.warns(DeprecationWarning) as record: - await RedisConnectionFactory._get_aredis_connection( - url="redis://localhost:6379" - ) - - assert any( - str(w.message) - == ( - "Argument url is deprecated and will be removed in a future release. " - "Use redis_url instead." - ) - for w in record - ) - - -@pytest.mark.asyncio -async def test__get_aredis_connection_no_deprecation_with_redis_url(): - # Patch AsyncRedis.from_url to avoid real network calls - with patch( - "redisvl.redis.connection.AsyncRedis.from_url", return_value=DummyAsyncClient() - ): - with assert_no_warnings(): - await RedisConnectionFactory._get_aredis_connection( - redis_url="redis://localhost:6379" - ) - - -def test_get_async_redis_connection_deprecates_url_kwarg_only(): - # Patch AsyncRedis.from_url to avoid real network calls - with patch( - "redisvl.redis.connection.AsyncRedis.from_url", return_value=MagicMock() - ): - with pytest.warns(DeprecationWarning): - RedisConnectionFactory.get_async_redis_connection( - url="redis://localhost:6379" - ) - - -def test_get_async_redis_connection_no_deprecation_with_redis_url(): - # Patch AsyncRedis.from_url to avoid real network calls - with patch( - "redisvl.redis.connection.AsyncRedis.from_url", return_value=MagicMock() - ): - with pytest.warns(DeprecationWarning): - RedisConnectionFactory.get_async_redis_connection( - redis_url="redis://localhost:6379" - ) From 5aad4fac59951d5262615b0f0fc97e4413e2a5d6 Mon Sep 17 00:00:00 2001 From: Vishal Bala Date: Thu, 3 Sep 2026 14:49:18 +0200 Subject: [PATCH 2/2] refactor(connection): address review of the factory removal MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Six review perspectives ran against the removal. The substantive findings: The keyword guard covered three of five factory entry points. get_redis_cluster_connection and get_async_redis_cluster_connection never got it, so url= there still produced "got multiple values for argument 'url'" — the exact error the guard exists to replace, reaching the users most likely to be calling a factory by hand. All five are guarded now. get_async_redis_connection warned before validating, so a call about to be rejected still emitted advisory noise, and the warning would have become the surfaced error under -W error. It now validates first, and carries stacklevel so the warning points at the caller rather than at this module. Its text said the function "will become async", which describes the wrong thing: the substantive difference from _get_aredis_connection is that this is the one factory that never sends CLIENT SETINFO, so its client reports no library name and defers its first connection. The docstring says that, and says the function is not being removed and has no replacement to migrate to, rather than pointing users at a private sibling. owns_client made a latent path reachable. disconnect() nulls an owned client and the lazy accessor re-creates one, but a handed-over client leaves no redis_url to rebuild from, so it fell through to REDIS_URL and could silently connect to a different server than the caller supplied. Both accessors now refuse, naming the reason. The no-argument constructor still resolves from the environment, which is a documented path. get_redis_connection documented a url parameter its signature does not have and that this branch made fatal, and all three factories still told readers in Raises to pass it. The two tables also lost their leading underscore in the split, publishing module-level names that feed a private helper; both are private again, and the comment now says which door each table serves so a future entry lands in the right one. Tests: the "will become async" notice was asserted nowhere after the url-deprecation file went, so the warn() could have been deleted with the suite still green. The rejection test now covers all five guards rather than two, and pins that a url containing a password is not echoed into the message. test_missing_env_var restored REDIS_URL outside a finally, so a failed assertion would have left it unset and cascaded into every later test that resolves its URL from the environment; monkeypatch handles it. --- redisvl/index/index.py | 25 +++++++- redisvl/redis/connection.py | 85 +++++++++++++++++++------- tests/integration/test_connection.py | 20 +++--- tests/unit/test_connection_protocol.py | 53 +++++++++++++--- 4 files changed, 136 insertions(+), 47 deletions(-) diff --git a/redisvl/index/index.py b/redisvl/index/index.py index 7780bb41..99c6ef9b 100644 --- a/redisvl/index/index.py +++ b/redisvl/index/index.py @@ -75,7 +75,7 @@ from redisvl.query.aggregate import AggregateHybridQuery from redisvl.query.filter import FilterExpression from redisvl.redis.connection import ( - REMOVED_INDEX_KWARGS, + _REMOVED_INDEX_KWARGS, RedisConnectionFactory, _reject_removed_kwargs, _split_from_existing_kwargs, @@ -899,7 +899,7 @@ def __init__( keyword that no longer exists is passed. ``connection_args`` and ``redis_kwargs`` are both now ``connection_kwargs``. """ - _reject_removed_kwargs(kwargs, REMOVED_INDEX_KWARGS) + _reject_removed_kwargs(kwargs, _REMOVED_INDEX_KWARGS) if not isinstance(schema, IndexSchema): raise ValueError("Must provide a valid IndexSchema object") @@ -924,6 +924,9 @@ def __init__( redis_client is None if owns_client is None else bool(owns_client) ) self._client_finalizer = None + # Whether the caller handed us a client. If they did and gave no URL, + # there is nothing to rebuild from once that client is closed. + self._client_was_injected = redis_client is not None # Close the owned client when this index is garbage collected. When # the client is created lazily, registration happens at creation time # instead (see the _redis_client property). @@ -1033,6 +1036,13 @@ def _redis_client(self) -> SyncRedisClient: Lazily creates a Redis client instance if it doesn't exist. """ if self.__redis_client is None: + if self._client_was_injected and self._redis_url is None: + raise RedisVLError( + "This index was given a Redis client and that client has " + "been closed, so there is nothing left to connect with. " + "Build a new index, or pass redis_url so the index can " + "manage its own connection." + ) with self._lock: if self.__redis_client is None: # Pass lib_name to connection factory @@ -2211,7 +2221,7 @@ def __init__( keyword that no longer exists is passed. ``connection_args`` and ``redis_kwargs`` are both now ``connection_kwargs``. """ - _reject_removed_kwargs(kwargs, REMOVED_INDEX_KWARGS) + _reject_removed_kwargs(kwargs, _REMOVED_INDEX_KWARGS) # final validation on schema object if not isinstance(schema, IndexSchema): @@ -2241,6 +2251,8 @@ def __init__( redis_client is None if owns_client is None else bool(owns_client) ) self._client_finalizer = None + # See the note in SearchIndex.__init__. + self._client_was_injected = redis_client is not None # Close the owned client when this index is garbage collected. When # the client is created lazily, registration happens at creation time # instead (see _get_client). @@ -2339,6 +2351,13 @@ def client(self) -> AsyncRedisClient | None: async def _get_client(self) -> AsyncRedisClient: """Lazily instantiate and return the async Redis client.""" if self._redis_client is None: + if self._client_was_injected and self._redis_url is None: + raise RedisVLError( + "This index was given a Redis client and that client has " + "been closed, so there is nothing left to connect with. " + "Build a new index, or pass redis_url so the index can " + "manage its own connection." + ) async with self._lock: # Double-check to protect against concurrent access if self._redis_client is None: diff --git a/redisvl/redis/connection.py b/redisvl/redis/connection.py index 92a17a56..d822fe39 100644 --- a/redisvl/redis/connection.py +++ b/redisvl/redis/connection.py @@ -32,16 +32,22 @@ # Old spellings and their current equivalents. Kept as tables 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. Split by entry point so a caller is only told about -# keywords its own door ever accepted. -REMOVED_INDEX_KWARGS = { +# unexpected argument. +# +# One table per entry point, so a caller is only told about keywords its own +# door ever accepted. Add a new entry to the table for the door that used to +# accept it: _REMOVED_INDEX_KWARGS for the SearchIndex and AsyncSearchIndex +# constructors and their from_existing, _REMOVED_FACTORY_KWARGS for the +# RedisConnectionFactory.get_*_connection functions. A keyword both doors +# accepted belongs in both. Entries are safe to drop once no supported +# caller still passes them; until then they cost nothing. +_REMOVED_INDEX_KWARGS = { "connection_args": "connection_kwargs", "redis_kwargs": "connection_kwargs", "_owns_redis_client": "owns_client", } -# Connection factory entry points. -REMOVED_FACTORY_KWARGS = {"url": "redis_url"} +_REMOVED_FACTORY_KWARGS = {"url": "redis_url"} def _reject_removed_kwargs( @@ -54,7 +60,14 @@ def _reject_removed_kwargs( credential into an exception string and from there into logs. It says "not a supported keyword" rather than "no longer supported" - because a caller may never have accepted the name in the first place. + because even within one table a name need not have been accepted + everywhere: ``url`` reached the two async factories but never + ``get_redis_connection``. + + Args: + kwargs: The caller's leftover keyword arguments. + removed: The table for this entry point, either + ``_REMOVED_INDEX_KWARGS`` or ``_REMOVED_FACTORY_KWARGS``. """ for name, replacement in removed.items(): if name in kwargs: @@ -71,7 +84,7 @@ def _split_from_existing_kwargs( keywords are rejected first, so a stale spelling fails here rather than reaching redis-py as an unexpected argument. """ - _reject_removed_kwargs(kwargs, REMOVED_INDEX_KWARGS) + _reject_removed_kwargs(kwargs, _REMOVED_INDEX_KWARGS) init_kwargs: dict[str, Any] = {} connection_kwargs: dict[str, Any] = {} @@ -631,19 +644,22 @@ def get_redis_connection( """Creates and returns a synchronous Redis client. Args: - url (Optional[str]): The URL of the Redis server. If not provided, - the environment variable REDIS_URL is used. + redis_url (Optional[str]): The URL of the Redis server. If not + provided, the environment variable REDIS_URL is used. **kwargs: Additional keyword arguments to be passed to the Redis client constructor. Returns: - Redis: A synchronous Redis client instance. + SyncRedisClient: A Redis or RedisCluster client, depending on the + URL. Raises: - ValueError: If url is not provided and REDIS_URL environment - variable is not set. + ValueError: If ``redis_url`` is not provided and the REDIS_URL + environment variable is not set. + TypeError: If a keyword that no longer exists is passed. ``url`` + is now ``redis_url``. """ - _reject_removed_kwargs(kwargs, REMOVED_FACTORY_KWARGS) + _reject_removed_kwargs(kwargs, _REMOVED_FACTORY_KWARGS) url = redis_url or get_address_from_env() # redis-py 8 defaults to RESP3, which changes raw Search command reply # shapes. Keep RedisVL's existing RESP2 behavior unless requested. @@ -675,13 +691,16 @@ async def _get_aredis_connection( Redis client constructor. Returns: - AsyncRedisClient: An asynchronous Redis client instance (either AsyncRedis or AsyncRedisCluster). + AsyncRedisClient: An AsyncRedis or AsyncRedisCluster client, + depending on the URL. Raises: - ValueError: If url is not provided and REDIS_URL environment - variable is not set. + ValueError: If ``redis_url`` is not provided and the REDIS_URL + environment variable is not set. + TypeError: If a keyword that no longer exists is passed. ``url`` + is now ``redis_url``. """ - _reject_removed_kwargs(kwargs, REMOVED_FACTORY_KWARGS) + _reject_removed_kwargs(kwargs, _REMOVED_FACTORY_KWARGS) url = redis_url or get_address_from_env() # Keep sync and async clients on the same backward-compatible default. kwargs.setdefault("protocol", 2) @@ -714,6 +733,17 @@ def get_async_redis_connection( ) -> AsyncRedisClient: """Creates and returns an asynchronous Redis client. + Synchronous today; it will become a coroutine in a future major + release, at which point callers will have to await it. It warns on + every call as notice of that change. It is not being removed, and + there is no replacement to migrate to. + + Unlike the client the index builds for itself, the client returned + here has not been identified to the server with CLIENT SETINFO, so + it reports no library name and defers its first connection to the + first command. To avoid managing a client at all, pass ``redis_url`` + to :class:`~redisvl.index.AsyncSearchIndex` and let it build one. + Args: redis_url (Optional[str]): The URL of the Redis server. If not provided, the environment variable REDIS_URL is used. @@ -721,17 +751,26 @@ def get_async_redis_connection( Redis client constructor. Returns: - AsyncRedis: An asynchronous Redis client instance. + AsyncRedisClient: An AsyncRedis or AsyncRedisCluster client, + depending on the URL. Raises: - ValueError: If url is not provided and REDIS_URL environment - variable is not set. + ValueError: If ``redis_url`` is not provided and the REDIS_URL + environment variable is not set. + TypeError: If a keyword that no longer exists is passed. ``url`` + is now ``redis_url``. + + Warns: + DeprecationWarning: Always, as notice of the coming signature + change. This function is not being removed. """ + _reject_removed_kwargs(kwargs, _REMOVED_FACTORY_KWARGS) warn( - "get_async_redis_connection will become async in a future release.", + "get_async_redis_connection will become a coroutine in a future " + "major release and will then need to be awaited.", DeprecationWarning, + stacklevel=2, ) - _reject_removed_kwargs(kwargs, REMOVED_FACTORY_KWARGS) url = redis_url or get_address_from_env() kwargs.setdefault("protocol", 2) @@ -758,6 +797,7 @@ def get_redis_cluster_connection( **kwargs, ) -> RedisCluster: """Creates and returns a synchronous Redis client for a Redis cluster.""" + _reject_removed_kwargs(kwargs, _REMOVED_FACTORY_KWARGS) url = redis_url or get_address_from_env() kwargs.setdefault("protocol", 2) return RedisCluster.from_url(url, **kwargs) @@ -768,6 +808,7 @@ def get_async_redis_cluster_connection( **kwargs, ) -> AsyncRedisCluster: """Creates and returns an asynchronous Redis client for a Redis cluster.""" + _reject_removed_kwargs(kwargs, _REMOVED_FACTORY_KWARGS) url = redis_url or get_address_from_env() kwargs.setdefault("protocol", 2) # Strip 'cluster' parameter as AsyncRedisCluster doesn't accept it diff --git a/tests/integration/test_connection.py b/tests/integration/test_connection.py index cb0a6de8..e1573f0e 100644 --- a/tests/integration/test_connection.py +++ b/tests/integration/test_connection.py @@ -1,5 +1,3 @@ -import os - import pytest from redis.exceptions import ConnectionError, NoPermissionError @@ -98,20 +96,16 @@ def test_convert_index_info_to_schema(): class TestGetRedisConnection: - """URL-resolution error paths, covered nowhere else in the suite. + """URL-resolution error paths for the sync factory. - These moved off the removed RedisConnectionFactory.connect(); the two - tests that only asserted the dispatcher returned a sync or async client - went with it, since every fixture in the suite builds clients this way. + Nothing else in the suite covers them; every other test and fixture + passes a working redis_url. """ - def test_missing_env_var(self): - redis_url = os.getenv("REDIS_URL") - if redis_url: - del os.environ["REDIS_URL"] - with pytest.raises(ValueError): - RedisConnectionFactory.get_redis_connection() - os.environ["REDIS_URL"] = redis_url + def test_missing_env_var(self, monkeypatch): + monkeypatch.delenv("REDIS_URL", raising=False) + with pytest.raises(ValueError): + RedisConnectionFactory.get_redis_connection() def test_invalid_url_format(self): with pytest.raises(ValueError): diff --git a/tests/unit/test_connection_protocol.py b/tests/unit/test_connection_protocol.py index dbdf8773..d8d788f2 100644 --- a/tests/unit/test_connection_protocol.py +++ b/tests/unit/test_connection_protocol.py @@ -130,21 +130,56 @@ async def test_aredis_connection_emits_no_warning_on_the_modern_spelling(): @pytest.mark.parametrize( - "call", + "factory", [ - lambda: RedisConnectionFactory.get_redis_connection(url="redis://x:6379"), - lambda: RedisConnectionFactory.get_async_redis_connection(url="redis://x:6379"), + RedisConnectionFactory.get_redis_connection, + RedisConnectionFactory.get_async_redis_connection, + RedisConnectionFactory.get_redis_cluster_connection, + RedisConnectionFactory.get_async_redis_cluster_connection, ], - ids=["sync", "async"], + ids=["sync", "async", "sync-cluster", "async-cluster"], ) -def test_factories_reject_the_removed_url_keyword(call): +def test_factories_reject_the_removed_url_keyword(factory): """url= now names its replacement instead of failing obscurely. - Left unguarded it reaches is_cluster_url, which takes url positionally, - and raises "got multiple values for argument 'url'" — an error naming - neither RedisVL nor the rename. + Left unguarded it reaches is_cluster_url or from_url, both of which take + url positionally, and raises "got multiple values for argument 'url'" -- + an error naming neither RedisVL nor the rename. + + A Redis URL routinely embeds a password, so this also pins that the + message names the keyword without echoing the value. + """ + with pytest.raises(TypeError) as excinfo: + factory(url="redis://user:s3cr3t-do-not-log@x:6379") + + message = str(excinfo.value) + assert "url" in message and "redis_url" in message + assert "s3cr3t-do-not-log" not in message + + +@pytest.mark.asyncio +async def test_private_async_factory_rejects_the_removed_url_keyword(): + """The guard on the factory every internal async path actually uses. + + AsyncSearchIndex and BaseCache both build their client through this one, + so a regression here is the one that would reach users. """ with pytest.raises(TypeError) as excinfo: - call() + await RedisConnectionFactory._get_aredis_connection(url="redis://x:6379") assert "redis_url" in str(excinfo.value) + + +def test_get_async_redis_connection_still_warns_about_becoming_async(): + """The signature-change notice is this function's only user-facing signal. + + Its previous assertion went out with the url-deprecation tests, so the + warn() could have been deleted or reworded with the suite still green. + """ + with patch( + "redisvl.redis.connection.AsyncRedis.from_url", return_value=MagicMock() + ): + with pytest.warns(DeprecationWarning, match="will become a coroutine"): + RedisConnectionFactory.get_async_redis_connection( + redis_url="redis://localhost:6379" + )