From 6d9165751e52b916ff91cb060154a8a8b525440e Mon Sep 17 00:00:00 2001 From: Vishal Bala Date: Wed, 12 Aug 2026 16:11:49 +0200 Subject: [PATCH] fix: let a connection open when CLIENT SETINFO is denied RedisVL announces itself on connect with `CLIENT SETINFO LIB-NAME` and, if that is refused, sends the same string through `ECHO`. The `ECHO` call was unguarded, so `NoPermissionError` escaped while the connection was being created, before any index operation could be attempted: NoPermissionError: User has no permissions to run the 'echo' command An application role assembled from `@read`/`@write` hits this. `CLIENT SETINFO` is tagged `@connection` and `@slow`, `ECHO` is `@connection` and `@fast`, and neither is in `@read` or `@write` -- so a rule built up from those categories never grants either. Subtracting `@dangerous` is not what denies them: measured on Redis 8.4.5, `+@all -@dangerous` permits both, which is why the shape that fails is a hand-written `+@read +@write` rule rather than a broad rule with exclusions. Note also that `+@read +@write +@slow` permits `CLIENT SETINFO` while still denying `ECHO`, so the fallback was never a reliable second chance. The `ECHO` fallback is deleted rather than guarded. It was added in 934d269 (#155) as a telemetry breadcrumb for servers older than Redis 7.2, where `CLIENT SETINFO` does not exist, so the library name would at least appear in `MONITOR` or the slowlog. It earns nothing today: it fires only when `CLIENT SETINFO` errors, its argument reaches nothing that reads `lib-name` (`CLIENT LIST` and `CLIENT INFO` take that field from `SETINFO` alone, and it would only enter the slowlog with `slowlog-log-slower-than` near zero), and redis-py already covers the old-server case one layer down, sending its own `CLIENT SETINFO` during the connection handshake under `try/except ResponseError: pass`. The `hasattr(client, "echo")` guard goes with it, along with the comment claiming `RedisCluster` has no `echo`; both `redis.cluster.RedisCluster` and `redis.asyncio.cluster.RedisCluster` expose it. The four duplicated blocks are now one sync and one async helper. The explicit `client_setinfo` call is kept deliberately -- redis-py's handshake reports its own name, and this overwrites it with the composed `redis-py(redisvl_v...;)` string that adoption metrics read -- and the helper docstring says so, so it does not read as redundant with the handshake. `except ResponseError` is also deliberate. In the connection-factory path this is the first command on a freshly created connection and therefore the de-facto connectivity check, so broadening to `RedisError` would swallow `ConnectionError` and defer a real failure to some later command. `AuthenticationError` subclasses `ConnectionError`, not `ResponseError`, so `WRONGPASS` and `NOAUTH` keep propagating either way. ## Tests - `tests/unit/test_client_identification.py` -- 16 cases over both twins. Mutation-checked: restoring the `ECHO` fallback fails six unit cases plus the integration test, and broadening the `except` to `Exception` fails two. Both refusals are covered, since only one is a permission problem: a plain `ResponseError` stands for a pre-7.2 server, which is the case that lets the fallback go. Identification is asserted on all three URL shapes -- sentinel, cluster, standalone -- because it sits after that fan-out and moving it into one branch otherwise goes unnoticed; live cluster tests need `--run-cluster-tests` and never run in CI. - An integration test opening a connection under `+@read +@write`, with the premise pinned: `CLIENT SETINFO` must raise `NoPermissionError` for that user, so the test cannot go vacuous if Redis ever grants it to that role. It lives in `test_connection.py` beside the other identification tests. - ACL user setup moves into an `acl_user` fixture, reused by the existing `-@admin` test in `test_search_index.py`. Rules are applied after `reset` because `ACL SETUSER` is additive and usernames are derived from the test's node id; the user is dropped before the connections it authenticated; and the fixture skips on deployments that reject `ACL SETUSER`. ## Docs `docs/user_guide/installation.md` said a credential permitted to run neither command fails at connection time. That is no longer true. The replacement names `+client|setinfo` for anyone who wants RedisVL attributed in `CLIENT LIST`, and is explicit that this labels only the connection RedisVL opens -- redis-py labels the rest of the pool as plain `redis-py`. It also gains a cluster caveat found while verifying this fix: `CLUSTER SLOTS` is tagged `@slow` only, so `RedisCluster.from_url` cannot discover the topology under a `+@read +@write` rule and the connection fails before identification is even attempted, reported as `Redis Cluster cannot be connected`. Such a deployment needs `+cluster|slots` as well. ## Not in scope The rest of the ACL documentation pass. `installation.md:189`, `:208`, `:210` and `:223` are stale for a different reason -- they predate the `create_index=False` opt-out -- and are corrected alongside it, not here. Identification still reaches only one connection. The explicit call labels whichever pooled connection it borrows, not the rest of the pool or any reconnect, and on a cluster redis-py routes it to the default node alone. Passing the composed name in as redis-py's `lib_name`/`driver_info` would fix that, but `redis>=5.0,<8.0` straddles the deprecation of `lib_name` in favour of `driver_info` and needs version-conditional handling. --- docs/user_guide/installation.md | 4 +- redisvl/redis/connection.py | 107 +++++++----- tests/conftest.py | 80 +++++++++ tests/integration/test_connection.py | 27 ++- tests/integration/test_search_index.py | 36 ++-- tests/unit/test_client_identification.py | 210 +++++++++++++++++++++++ tests/unit/test_url_deprecation.py | 3 - 7 files changed, 393 insertions(+), 74 deletions(-) create mode 100644 tests/unit/test_client_identification.py diff --git a/docs/user_guide/installation.md b/docs/user_guide/installation.md index 70840b1da..8283afad6 100644 --- a/docs/user_guide/installation.md +++ b/docs/user_guide/installation.md @@ -211,7 +211,9 @@ Index enumeration is the only thing an `-@admin` rule breaks. It is reached by ` Other categories gate different operations. `FT.DROPINDEX` is tagged `@dangerous` and `@write` as well as `@search`, so a policy that subtracts `@dangerous` denies `index.delete()`, `rvl index delete`, and `rvl index destroy`. -Outside of Redis Search, RedisVL identifies itself on connect with `CLIENT SETINFO` and falls back to `ECHO`. A credential permitted to run neither currently fails when the connection is created, before any index operation. +Outside of Redis Search, RedisVL identifies itself on connect with `CLIENT SETINFO`. That command is tagged `@connection` and `@slow`, and belongs to neither `@read` nor `@write`, so a rule built up from those categories never grants it. A credential that cannot run it still connects: identification only populates the `lib-name` field that `CLIENT LIST` and `CLIENT INFO` display, so a refusal is ignored (and logged, if you have configured logging at debug level). Grant `+client|setinfo` if you want RedisVL to appear as the connecting library there — note that this labels the connection RedisVL opens, while redis-py labels the rest of the pool as plain `redis-py`. + +Cluster deployments need one more grant. `RedisCluster` discovers the topology with `CLUSTER SLOTS`, which is tagged `@slow` only, so a credential assembled from `+@read +@write` cannot open a clustered connection at all — redis-py reports this as `Redis Cluster cannot be connected`, with the underlying permission error chained beneath it. Grant `+cluster|slots` alongside the rules above. ### Redis Cloud and Redis Software diff --git a/redisvl/redis/connection.py b/redisvl/redis/connection.py index 44247d1f6..20f22832b 100644 --- a/redisvl/redis/connection.py +++ b/redisvl/redis/connection.py @@ -212,6 +212,43 @@ def make_lib_name(*args) -> str: return f"redis-py({custom_libs})" +def _identify_client(client: SyncRedisClient, lib_name: str | None = None) -> None: + """Report RedisVL as the connecting library, tolerating a refusal. + + redis-py sends its own ``CLIENT SETINFO`` during the connection handshake, + so this call is here to overwrite that with the composed + ``redis-py(redisvl_v...;)`` name that adoption metrics read. Keep + it: without it the library and any wrapper above it go unattributed. + + A refusal is ignored, because ``CLIENT SETINFO`` only populates the + ``lib-name`` field that ``CLIENT LIST`` and ``CLIENT INFO`` display -- its + own documentation tells client libraries to ignore failures. Two credentials + hit this: one granting neither ``@connection`` nor the command itself, and + one on a server predating Redis 7.2, where the command does not exist. + + Only ``ResponseError`` is caught. In the connection-factory path this is the + first command issued on a freshly created connection, which makes it the + de-facto connectivity check, so swallowing ``ConnectionError`` would defer a + genuine failure to some later and more confusing command. Note that on a + cluster client redis-py routes the command to the default node only, so the + label reaches one node rather than the whole cluster. + """ + try: + client.client_setinfo("LIB-NAME", make_lib_name(lib_name)) + except ResponseError as e: + logger.debug(f"CLIENT SETINFO was not applied, continuing without it: {e}") + + +async def _aidentify_client( + client: AsyncRedisClient, lib_name: str | None = None +) -> None: + """Async version of :func:`_identify_client`.""" + try: + await client.client_setinfo("LIB-NAME", make_lib_name(lib_name)) + except ResponseError as e: + logger.debug(f"CLIENT SETINFO was not applied, continuing without it: {e}") + + def convert_index_info_to_schema(index_info: dict[str, Any]) -> dict[str, Any]: """Convert the output of FT.INFO into a schema-ready dictionary. @@ -540,15 +577,7 @@ def get_redis_connection( client = RedisCluster.from_url(url, **kwargs) else: client = Redis.from_url(url, **kwargs) - # Module validation removed - operations will fail naturally if modules are missing - # Set client library name only - _lib_name = make_lib_name(kwargs.get("lib_name")) - try: - client.client_setinfo("LIB-NAME", _lib_name) - except ResponseError: - # Fall back to a simple log echo - if hasattr(client, "echo"): - client.echo(_lib_name) + _identify_client(client, kwargs.get("lib_name")) return client @staticmethod @@ -599,15 +628,7 @@ async def _get_aredis_connection( ) client = AsyncRedis.from_url(cleaned_url, **cleaned_kwargs) - # Module validation removed - operations will fail naturally if modules are missing - # Set client library name only - _lib_name = make_lib_name(kwargs.get("lib_name")) - try: - await client.client_setinfo("LIB-NAME", _lib_name) - except ResponseError: - # Fall back to a simple log echo - if hasattr(client, "echo"): - await client.echo(_lib_name) + await _aidentify_client(client, kwargs.get("lib_name")) return client @staticmethod @@ -718,52 +739,50 @@ def validate_sync_redis( redis_client: SyncRedisClient, lib_name: str | None = None, ) -> None: - """Validates the sync Redis client. + """Check the client type and report the library name. - Note: Module validation has been removed. This method now only validates - the client type and sets the library name. + Identification is best effort: a server that refuses ``CLIENT SETINFO`` + is tolerated, so the only failure raised here is a wrong client type. + (Module validation was removed; a missing module now surfaces when an + operation needs it.) + + Args: + redis_client (SyncRedisClient): The client to check. + lib_name (Optional[str]): Name of a library wrapping RedisVL, to + report alongside it. Defaults to None. + + Raises: + TypeError: If the client is not a Redis or RedisCluster instance. """ if not issubclass(type(redis_client), (Redis, RedisCluster)): raise TypeError( "Invalid Redis client instance. Must be Redis or RedisCluster." ) - # Set client library name - _lib_name = make_lib_name(lib_name) - try: - redis_client.client_setinfo("LIB-NAME", _lib_name) - except ResponseError: - # Fall back to a simple log echo - # For RedisCluster, echo is not available - if hasattr(redis_client, "echo"): - redis_client.echo(_lib_name) - - # Module validation removed - operations will fail naturally if modules are missing + _identify_client(redis_client, lib_name) @staticmethod async def validate_async_redis( redis_client: AsyncRedisClient, lib_name: str | None = None, ) -> None: - """Validates the async Redis client. + """Async version of :meth:`validate_sync_redis`. - Note: Module validation has been removed. This method now only validates - the client type and sets the library name. + Args: + redis_client (AsyncRedisClient): The client to check. + lib_name (Optional[str]): Name of a library wrapping RedisVL, to + report alongside it. Defaults to None. + + Raises: + TypeError: If the client is not an async Redis or RedisCluster + instance. """ if not issubclass(type(redis_client), (AsyncRedis, AsyncRedisCluster)): raise TypeError( "Invalid async Redis client instance. Must be async Redis or async RedisCluster." ) - # Set client library name - _lib_name = make_lib_name(lib_name) - try: - await redis_client.client_setinfo("LIB-NAME", _lib_name) - except ResponseError: - # Fall back to a simple log echo - if hasattr(redis_client, "echo"): - await redis_client.echo(_lib_name) - # Module validation removed - operations will fail naturally if modules are missing + await _aidentify_client(redis_client, lib_name) @staticmethod @overload diff --git a/tests/conftest.py b/tests/conftest.py index cd09ef5d4..084d83447 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -4,14 +4,17 @@ import re import subprocess import sys +from contextlib import contextmanager, suppress from datetime import datetime, timezone import pytest +from redis.exceptions import ResponseError from testcontainers.compose import DockerCompose from redisvl.index.index import AsyncSearchIndex, SearchIndex from redisvl.redis.connection import RedisConnectionFactory, is_version_gte from redisvl.redis.utils import array_to_buffer +from redisvl.types import SyncRedisClient # Check if we're on Python 3.14+ where sentence-transformers may not work SKIP_HF = sys.version_info >= (3, 14) @@ -212,6 +215,83 @@ def client(redis_url): yield conn +class _AclUser: + """A temporary ACL user, plus the RedisVL connections opened as them. + + `username` and `password` are public so a test can hand them to anything + that takes connection kwargs, such as an extension constructor, instead of + going through `connect()`. + """ + + def __init__(self, username: str, password: str, redis_url: str): + self.username = username + self.password = password + self._redis_url = redis_url + self._clients: list[SyncRedisClient] = [] + + def connect(self, **kwargs): + """Open a RedisVL connection authenticated as this user.""" + conn = RedisConnectionFactory.get_redis_connection( + redis_url=self._redis_url, + username=self.username, + password=self.password, + **kwargs, + ) + self._clients.append(conn) + return conn + + def close_all(self) -> None: + for conn in self._clients: + with suppress(Exception): + conn.close() + + +@pytest.fixture +def acl_user(client, redis_url, redis_test_name): + """Create a temporary ACL user, yielding a handle that connects as them. + + The returned factory takes a required `name` and the ACL rules to apply, in + the order Redis applies them: rules are evaluated left to right, so + `+@all -@admin` and `-@admin +@all` do not mean the same thing. `on` and a + password are supplied here, so callers pass only key patterns, channel + patterns and command rules:: + + with acl_user("~*", "&*", "+@read", "+@write", name="read_write") as user: + restricted = user.connect() + + Rules are applied after `reset`, because `ACL SETUSER` is additive and + usernames are derived from the test's node id -- without it, a run whose + teardown was skipped by a crash would layer new rules onto a stale user. + + ACL users are server-global, so a leaked one outlives the test and its + worker; the user is dropped before the connections it authenticated. Redis + Software manages ACLs through its own control plane and does not support the + `>password` syntax used here, so tests built on this fixture only run + against Redis Open Source -- they skip elsewhere. + """ + + @contextmanager + def _make_acl_user(*rules: str, name: str): + username = redis_test_name(name) + password = "test-acl-password" + try: + client.execute_command( + "ACL", "SETUSER", username, "reset", "on", f">{password}", *rules + ) + except ResponseError as e: + pytest.skip(f"Deployment does not support ACL SETUSER: {e}") + user = _AclUser(username, password, redis_url) + try: + yield user + finally: + try: + client.execute_command("ACL", "DELUSER", username) + finally: + user.close_all() + + return _make_acl_user + + @pytest.fixture def cluster_client(redis_cluster_url): """ diff --git a/tests/integration/test_connection.py b/tests/integration/test_connection.py index f3a8ea727..b62d79bef 100644 --- a/tests/integration/test_connection.py +++ b/tests/integration/test_connection.py @@ -3,7 +3,7 @@ import pytest from redis import Redis from redis.asyncio import Redis as AsyncRedis -from redis.exceptions import ConnectionError +from redis.exceptions import ConnectionError, NoPermissionError from redisvl.redis.connection import ( RedisConnectionFactory, @@ -136,6 +136,31 @@ def test_unknown_redis(self): bad_client.ping() +def test_connection_opens_without_identification_permission(acl_user): + """A credential that cannot identify the client must still connect. + + RedisVL announces itself with CLIENT SETINFO when a connection is made, and + used to fall back to ECHO if that was refused. Both commands are + `@connection` and are in neither `@read` nor `@write`, so a role built up + from those categories never grants either -- and the unguarded fallback made + RedisVL fail while the connection was being created, before any index + operation could be attempted. + + Only a live server can show that this role really denies the command, which + is why this is an integration test. + """ + with acl_user("~*", "&*", "+@read", "+@write", name="acl_no_connection") as user: + restricted = user.connect() + + # Pin the premise. If a future Redis lets this role run CLIENT SETINFO, + # the test is no longer exercising the tolerance and should say so. + with pytest.raises(NoPermissionError): + restricted.client_setinfo("LIB-NAME", "probe") + + # And the connection is genuinely usable for what the role does permit. + assert restricted.exists("no-such-key") == 0 + + def test_validate_redis(client): skip_if_redis_version_below(client, "7.2.0") RedisConnectionFactory.validate_sync_redis(client) diff --git a/tests/integration/test_search_index.py b/tests/integration/test_search_index.py index bac05612e..2c3740040 100644 --- a/tests/integration/test_search_index.py +++ b/tests/integration/test_search_index.py @@ -313,9 +313,7 @@ def test_search_index_delete(index): assert index.name not in index.listall() -def test_exists_under_acl_without_admin_commands( - index, client, redis_url, redis_test_name -): +def test_exists_under_acl_without_admin_commands(index, client, acl_user): """exists() must work for a credential that grants search but drops @admin. Redis tags FT._LIST @admin as well as @search, so this ACL shape used to @@ -327,30 +325,18 @@ def test_exists_under_acl_without_admin_commands( skip_if_no_redis_search(client) index.create(overwrite=True, drop=True) - username = redis_test_name("acl_no_admin") - password = "test-acl-password" - client.execute_command( - "ACL", "SETUSER", username, "on", f">{password}", "~*", "&*", "+@all", "-@admin" - ) - restricted = None try: - restricted = RedisConnectionFactory.get_redis_connection( - redis_url=redis_url, username=username, password=password - ) - # Pin the premise so this test cannot quietly become vacuous: if a - # future Redis stops gating FT._LIST behind @admin, the reason for - # preferring FT.INFO is gone and we want to hear about it here. - with pytest.raises(NoPermissionError): - restricted.execute_command("FT._LIST") - - restricted_index = SearchIndex(schema=index.schema, redis_client=restricted) - assert restricted_index.exists() is True + with acl_user("~*", "&*", "+@all", "-@admin", name="acl_no_admin") as user: + restricted = user.connect() + # Pin the premise so this test cannot quietly become vacuous: if a + # future Redis stops gating FT._LIST behind @admin, the reason for + # preferring FT.INFO is gone and we want to hear about it here. + with pytest.raises(NoPermissionError): + restricted.execute_command("FT._LIST") + + restricted_index = SearchIndex(schema=index.schema, redis_client=restricted) + assert restricted_index.exists() is True finally: - # Drop the ACL user before anything else that could raise: users are - # server-global, so a leaked one outlives this test and its worker. - client.execute_command("ACL", "DELUSER", username) - if restricted is not None: - restricted.close() index.delete(drop=True) diff --git a/tests/unit/test_client_identification.py b/tests/unit/test_client_identification.py new file mode 100644 index 000000000..9c741bd1d --- /dev/null +++ b/tests/unit/test_client_identification.py @@ -0,0 +1,210 @@ +""" +Unit tests for client identification tolerance. + +RedisVL announces itself with ``CLIENT SETINFO LIB-NAME`` when a connection is +made, so that adoption metrics can attribute traffic to the library and to any +wrapper above it. The command is cosmetic -- it only populates the ``lib-name`` +field of ``CLIENT LIST`` and ``CLIENT INFO`` -- so a refusal must never stop a +connection from opening. Two refusals matter: a credential that grants neither +`@connection` nor the command itself, and a server predating Redis 7.2, where +the command does not exist. +""" + +import logging +from unittest.mock import AsyncMock, Mock, patch + +import pytest +from redis import Redis +from redis.asyncio import Redis as AsyncRedis +from redis.exceptions import ConnectionError, NoPermissionError, ResponseError + +from redisvl.redis import connection as connection_module +from redisvl.redis.connection import RedisConnectionFactory, make_lib_name + +PLAIN_URL = "redis://localhost:6379" +CLUSTER_URL = "redis://localhost:6379?cluster=true" +SENTINEL_URL = "redis+sentinel://localhost:26379/mymaster" + +# Both must be tolerated, and only the first is a permission problem. The second +# is the case that lets the ECHO fallback go: redis-py's own handshake already +# absorbs it, so RedisVL does not need a workaround for old servers. +REFUSALS = [ + ( + NoPermissionError, + "User acl_user has no permissions to run the 'client|setinfo' command", + ), + ( + ResponseError, + "ERR Unknown subcommand or wrong number of arguments for 'setinfo'", + ), +] +REFUSAL_IDS = ["denied-by-acl", "unsupported-by-server"] + +UNREACHABLE = "Error 111 connecting to localhost:6379. Connection refused." + + +def _sync_client(setinfo_error=None): + """A real Redis instance -- no socket is opened -- with stubbed commands. + + The type has to survive ``issubclass`` in ``validate_sync_redis``, so this + cannot be a bare ``MagicMock``. Returns the stubs so assertions do not have + to reach back through the client. + """ + client = Redis.from_url(PLAIN_URL) + setinfo, echo = Mock(side_effect=setinfo_error), Mock() + client.client_setinfo, client.echo = setinfo, echo + return client, setinfo, echo + + +def _async_client(setinfo_error=None): + client = AsyncRedis.from_url(PLAIN_URL) + setinfo, echo = AsyncMock(side_effect=setinfo_error), AsyncMock() + client.client_setinfo, client.echo = setinfo, echo + return client, setinfo, echo + + +class TestConnectionFactoryIdentification: + """get_redis_connection() must survive a refused identification.""" + + @pytest.mark.parametrize("exc_type,message", REFUSALS, ids=REFUSAL_IDS) + def test_refused_identification_still_returns_a_client(self, exc_type, message): + client, _, echo = _sync_client(setinfo_error=exc_type(message)) + with patch.object(connection_module.Redis, "from_url", return_value=client): + returned = RedisConnectionFactory.get_redis_connection(redis_url=PLAIN_URL) + assert returned is client + # ECHO used to carry the library name when SETINFO was refused. It is + # denied by the same ACL rule and reaches nothing that reads lib-name, + # so the fallback is gone and must not come back. + echo.assert_not_called() + + def test_connection_failure_is_not_swallowed(self): + # SETINFO is the first command on a freshly created connection, which + # makes it the de-facto connectivity check. Widening the except clause + # would defer a real failure to some later, more confusing command. + client, _, _ = _sync_client(setinfo_error=ConnectionError(UNREACHABLE)) + with patch.object(connection_module.Redis, "from_url", return_value=client): + with pytest.raises(ConnectionError): + RedisConnectionFactory.get_redis_connection(redis_url=PLAIN_URL) + + @pytest.mark.parametrize( + "url", + [PLAIN_URL, CLUSTER_URL, SENTINEL_URL], + ids=["standalone", "cluster", "sentinel"], + ) + def test_identification_reaches_every_url_shape(self, url): + # Identification sits after the sentinel/cluster/standalone fan-out. + # Only the standalone branch has live coverage elsewhere -- cluster + # integration tests need --run-cluster-tests and never run in CI -- so + # this is the only guard against the call sliding into one branch. + client, setinfo, _ = _sync_client() + if url == SENTINEL_URL: + target = patch.object( + RedisConnectionFactory, "_redis_sentinel_client", return_value=client + ) + elif url == CLUSTER_URL: + target = patch.object( + connection_module.RedisCluster, "from_url", return_value=client + ) + else: + target = patch.object( + connection_module.Redis, "from_url", return_value=client + ) + with target: + RedisConnectionFactory.get_redis_connection(redis_url=url) + setinfo.assert_called_once_with("LIB-NAME", make_lib_name(None)) + + def test_wrapper_lib_name_is_reported(self): + # The composed string is the reason the explicit call is kept at all: + # redis-py's handshake reports its own name, not this one. + client, setinfo, _ = _sync_client() + with patch.object(connection_module.Redis, "from_url", return_value=client): + RedisConnectionFactory.get_redis_connection( + redis_url=PLAIN_URL, lib_name="langchain-redis_v1.0.0" + ) + reported = setinfo.call_args.args[1] + assert "redisvl_v" in reported and "langchain-redis_v1.0.0" in reported + + def test_refusal_is_logged(self, caplog): + # A refusal is otherwise invisible to the caller, so the log line is the + # only affordance for "why is lib-name empty?". + client, _, _ = _sync_client(setinfo_error=NoPermissionError(REFUSALS[0][1])) + with caplog.at_level(logging.DEBUG, logger="redisvl.redis.connection"): + with patch.object(connection_module.Redis, "from_url", return_value=client): + RedisConnectionFactory.get_redis_connection(redis_url=PLAIN_URL) + assert "CLIENT SETINFO" in caplog.text + + +class TestAsyncConnectionFactoryIdentification: + """The async twin is a hand-maintained copy, so each branch needs a witness.""" + + @pytest.mark.parametrize("exc_type,message", REFUSALS, ids=REFUSAL_IDS) + @pytest.mark.asyncio + async def test_refused_identification_still_returns_a_client( + self, exc_type, message + ): + client, _, echo = _async_client(setinfo_error=exc_type(message)) + with patch.object( + connection_module.AsyncRedis, "from_url", return_value=client + ): + returned = await RedisConnectionFactory._get_aredis_connection( + redis_url=PLAIN_URL + ) + assert returned is client + # assert_not_called, not assert_not_awaited: the latter passes for a + # reintroduced fallback that forgot its await, which is exactly the slip + # a hand-maintained twin invites. + echo.assert_not_called() + + @pytest.mark.asyncio + async def test_connection_failure_is_not_swallowed(self): + client, _, _ = _async_client(setinfo_error=ConnectionError(UNREACHABLE)) + with patch.object( + connection_module.AsyncRedis, "from_url", return_value=client + ): + with pytest.raises(ConnectionError): + await RedisConnectionFactory._get_aredis_connection(redis_url=PLAIN_URL) + + @pytest.mark.asyncio + async def test_wrapper_lib_name_is_reported(self): + client, setinfo, _ = _async_client() + with patch.object( + connection_module.AsyncRedis, "from_url", return_value=client + ): + await RedisConnectionFactory._get_aredis_connection( + redis_url=PLAIN_URL, lib_name="langchain-redis_v1.0.0" + ) + reported = setinfo.call_args.args[1] + assert "redisvl_v" in reported and "langchain-redis_v1.0.0" in reported + + +class TestValidateSyncRedis: + """The user-supplied-client path shares the same tolerance.""" + + def test_refused_identification_is_tolerated(self): + client, setinfo, echo = _sync_client( + setinfo_error=NoPermissionError(REFUSALS[0][1]) + ) + RedisConnectionFactory.validate_sync_redis(client) + setinfo.assert_called_once_with("LIB-NAME", make_lib_name(None)) + echo.assert_not_called() + + def test_client_type_is_still_validated(self): + with pytest.raises(TypeError): + RedisConnectionFactory.validate_sync_redis("not a client") + + +class TestValidateAsyncRedis: + @pytest.mark.asyncio + async def test_client_type_is_still_validated(self): + # A sync client is the mistake this guard exists to catch. + with pytest.raises(TypeError): + await RedisConnectionFactory.validate_async_redis(Redis.from_url(PLAIN_URL)) + + @pytest.mark.asyncio + async def test_refused_identification_is_tolerated(self): + client, setinfo, echo = _async_client( + setinfo_error=NoPermissionError(REFUSALS[0][1]) + ) + await RedisConnectionFactory.validate_async_redis(client) + setinfo.assert_called_once_with("LIB-NAME", make_lib_name(None)) + echo.assert_not_called() diff --git a/tests/unit/test_url_deprecation.py b/tests/unit/test_url_deprecation.py index b70385fca..d70ff4cb8 100644 --- a/tests/unit/test_url_deprecation.py +++ b/tests/unit/test_url_deprecation.py @@ -11,9 +11,6 @@ class DummyAsyncClient: async def client_setinfo(self, *args, **kwargs): return None - async def echo(self, *args, **kwargs): - return None - @pytest.mark.asyncio async def test__get_aredis_connection_deprecates_url_kwarg_only():