refactor(connection): remove RedisConnectionFactory.connect and the url= keyword

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:

redisvl/redis/connection.py

@@ -23,35 +23,53 @@
     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
     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.
+    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