diff --git a/redisvl/index/index.py b/redisvl/index/index.py index 6effd34f..99c6ef9b 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") @@ -923,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). @@ -1032,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 @@ -2210,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) + _reject_removed_kwargs(kwargs, _REMOVED_INDEX_KWARGS) # final validation on schema object if not isinstance(schema, IndexSchema): @@ -2240,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). @@ -2338,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 1acacb8c..d822fe39 100644 --- a/redisvl/redis/connection.py +++ b/redisvl/redis/connection.py @@ -23,25 +23,36 @@ 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 = { +# +# 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", } +_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 +60,16 @@ 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 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_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 +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) + _reject_removed_kwargs(kwargs, _REMOVED_INDEX_KWARGS) init_kwargs: dict[str, Any] = {} connection_kwargs: dict[str, Any] = {} @@ -618,37 +636,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, @@ -657,18 +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) 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 +675,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,23 +685,23 @@ 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. 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``. """ - _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,35 +727,51 @@ async def _get_aredis_connection( return client @staticmethod - @deprecated_argument("url", "redis_url") def get_async_redis_connection( redis_url: str | None = None, **kwargs, ) -> 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 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. 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, ) - _deprecated_url = kwargs.pop("url", None) - url = _deprecated_url or redis_url or get_address_from_env() + url = redis_url or get_address_from_env() kwargs.setdefault("protocol", 2) if url.startswith("redis+sentinel"): @@ -791,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) @@ -801,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 b62d79be..e1573f0e 100644 --- a/tests/integration/test_connection.py +++ b/tests/integration/test_connection.py @@ -1,8 +1,4 @@ -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 +15,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 +95,27 @@ 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() - - @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() - - 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() - os.environ["REDIS_URL"] = redis_url +class TestGetRedisConnection: + """URL-resolution error paths for the sync factory. + + Nothing else in the suite covers them; every other test and fixture + passes a working 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): - 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..d8d788f2 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,76 @@ 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( + "factory", + [ + RedisConnectionFactory.get_redis_connection, + RedisConnectionFactory.get_async_redis_connection, + RedisConnectionFactory.get_redis_cluster_connection, + RedisConnectionFactory.get_async_redis_cluster_connection, + ], + ids=["sync", "async", "sync-cluster", "async-cluster"], +) +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 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: + 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" + ) 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" - )