From 656cbddbeb556be960dbdd301444ec81f007935e Mon Sep 17 00:00:00 2001 From: Aryan Pardeshi Date: Sun, 9 Aug 2026 02:58:38 +0530 Subject: [PATCH 1/2] fix: do not fail connection setup when ACL denies both CLIENT SETINFO and ECHO --- redisvl/redis/connection.py | 20 ++++++++-- tests/unit/test_connection_acl.py | 62 +++++++++++++++++++++++++++++++ 2 files changed, 78 insertions(+), 4 deletions(-) create mode 100644 tests/unit/test_connection_acl.py diff --git a/redisvl/redis/connection.py b/redisvl/redis/connection.py index 44247d1f6..42b3786aa 100644 --- a/redisvl/redis/connection.py +++ b/redisvl/redis/connection.py @@ -548,7 +548,10 @@ def get_redis_connection( except ResponseError: # Fall back to a simple log echo if hasattr(client, "echo"): - client.echo(_lib_name) + try: + client.echo(_lib_name) + except ResponseError as e: + logger.debug(f"Failed to echo lib_name due to ACL restrictions: {e}") return client @staticmethod @@ -607,7 +610,10 @@ async def _get_aredis_connection( except ResponseError: # Fall back to a simple log echo if hasattr(client, "echo"): - await client.echo(_lib_name) + try: + await client.echo(_lib_name) + except ResponseError as e: + logger.debug(f"Failed to echo lib_name due to ACL restrictions: {e}") return client @staticmethod @@ -736,7 +742,10 @@ def validate_sync_redis( # Fall back to a simple log echo # For RedisCluster, echo is not available if hasattr(redis_client, "echo"): - redis_client.echo(_lib_name) + try: + redis_client.echo(_lib_name) + except ResponseError as e: + logger.debug(f"Failed to echo lib_name due to ACL restrictions: {e}") # Module validation removed - operations will fail naturally if modules are missing @@ -761,7 +770,10 @@ async def validate_async_redis( except ResponseError: # Fall back to a simple log echo if hasattr(redis_client, "echo"): - await redis_client.echo(_lib_name) + try: + await redis_client.echo(_lib_name) + except ResponseError as e: + logger.debug(f"Failed to echo lib_name due to ACL restrictions: {e}") # Module validation removed - operations will fail naturally if modules are missing diff --git a/tests/unit/test_connection_acl.py b/tests/unit/test_connection_acl.py new file mode 100644 index 000000000..e15227930 --- /dev/null +++ b/tests/unit/test_connection_acl.py @@ -0,0 +1,62 @@ +import pytest +from unittest.mock import MagicMock, AsyncMock, patch +from redis.exceptions import ResponseError + +from redisvl.redis.connection import RedisConnectionFactory + + +def test_get_redis_connection_acl_fallback_denied(): + mock_client = MagicMock() + mock_client.client_setinfo.side_effect = ResponseError("NOPERM client setinfo denied") + mock_client.echo.side_effect = ResponseError("NOPERM echo denied") + + with patch("redisvl.redis.connection.Redis.from_url", return_value=mock_client): + # Should not raise exception even when both client_setinfo and echo fail with ACL ResponseError + client = RedisConnectionFactory.get_redis_connection("redis://localhost:6379") + assert client == mock_client + mock_client.client_setinfo.assert_called_once() + mock_client.echo.assert_called_once() + + +@pytest.mark.asyncio +async def test_get_aredis_connection_acl_fallback_denied(): + mock_client = MagicMock() + mock_client.client_setinfo = AsyncMock(side_effect=ResponseError("NOPERM client setinfo denied")) + mock_client.echo = AsyncMock(side_effect=ResponseError("NOPERM echo denied")) + + with patch("redisvl.redis.connection.AsyncRedis.from_url", return_value=mock_client): + client = await RedisConnectionFactory._get_aredis_connection("redis://localhost:6379") + assert client == mock_client + mock_client.client_setinfo.assert_called_once() + mock_client.echo.assert_called_once() + + +def test_validate_sync_redis_acl_fallback_denied(): + mock_client = MagicMock() + # Ensure issubclass check passes by mocking type or using Redis subclass + from redis import Redis + class MockRedis(Redis): + pass + + mock_client = MagicMock(spec=MockRedis) + mock_client.client_setinfo.side_effect = ResponseError("NOPERM client setinfo denied") + mock_client.echo.side_effect = ResponseError("NOPERM echo denied") + + RedisConnectionFactory.validate_sync_redis(mock_client) + mock_client.client_setinfo.assert_called_once() + mock_client.echo.assert_called_once() + + +@pytest.mark.asyncio +async def test_validate_async_redis_acl_fallback_denied(): + from redis.asyncio import Redis as AsyncRedis + class MockAsyncRedis(AsyncRedis): + pass + + mock_client = MagicMock(spec=MockAsyncRedis) + mock_client.client_setinfo = AsyncMock(side_effect=ResponseError("NOPERM client setinfo denied")) + mock_client.echo = AsyncMock(side_effect=ResponseError("NOPERM echo denied")) + + await RedisConnectionFactory.validate_async_redis(mock_client) + mock_client.client_setinfo.assert_called_once() + mock_client.echo.assert_called_once() From 02d54097a1cdf3be8187d6bd52aec0a68cf36191 Mon Sep 17 00:00:00 2001 From: Aryan Pardeshi Date: Sun, 9 Aug 2026 11:35:32 +0530 Subject: [PATCH 2/2] test: use real Redis instances so validate_* ACL tests reach the echo fallback --- redisvl/redis/connection.py | 16 ++++-- tests/unit/test_connection_acl.py | 92 ++++++++++++++++++++----------- 2 files changed, 72 insertions(+), 36 deletions(-) diff --git a/redisvl/redis/connection.py b/redisvl/redis/connection.py index 42b3786aa..5de9600d3 100644 --- a/redisvl/redis/connection.py +++ b/redisvl/redis/connection.py @@ -551,7 +551,9 @@ def get_redis_connection( try: client.echo(_lib_name) except ResponseError as e: - logger.debug(f"Failed to echo lib_name due to ACL restrictions: {e}") + logger.debug( + f"Failed to echo lib_name due to ACL restrictions: {e}" + ) return client @staticmethod @@ -613,7 +615,9 @@ async def _get_aredis_connection( try: await client.echo(_lib_name) except ResponseError as e: - logger.debug(f"Failed to echo lib_name due to ACL restrictions: {e}") + logger.debug( + f"Failed to echo lib_name due to ACL restrictions: {e}" + ) return client @staticmethod @@ -745,7 +749,9 @@ def validate_sync_redis( try: redis_client.echo(_lib_name) except ResponseError as e: - logger.debug(f"Failed to echo lib_name due to ACL restrictions: {e}") + logger.debug( + f"Failed to echo lib_name due to ACL restrictions: {e}" + ) # Module validation removed - operations will fail naturally if modules are missing @@ -773,7 +779,9 @@ async def validate_async_redis( try: await redis_client.echo(_lib_name) except ResponseError as e: - logger.debug(f"Failed to echo lib_name due to ACL restrictions: {e}") + logger.debug( + f"Failed to echo lib_name due to ACL restrictions: {e}" + ) # Module validation removed - operations will fail naturally if modules are missing diff --git a/tests/unit/test_connection_acl.py b/tests/unit/test_connection_acl.py index e15227930..eae6b38db 100644 --- a/tests/unit/test_connection_acl.py +++ b/tests/unit/test_connection_acl.py @@ -1,62 +1,90 @@ +from unittest.mock import AsyncMock, MagicMock, patch + import pytest -from unittest.mock import MagicMock, AsyncMock, patch +from redis import Redis +from redis.asyncio import Redis as AsyncRedis from redis.exceptions import ResponseError from redisvl.redis.connection import RedisConnectionFactory def test_get_redis_connection_acl_fallback_denied(): + """Connection setup must survive an ACL that denies CLIENT SETINFO and ECHO.""" mock_client = MagicMock() - mock_client.client_setinfo.side_effect = ResponseError("NOPERM client setinfo denied") + mock_client.client_setinfo.side_effect = ResponseError( + "NOPERM client setinfo denied" + ) mock_client.echo.side_effect = ResponseError("NOPERM echo denied") with patch("redisvl.redis.connection.Redis.from_url", return_value=mock_client): - # Should not raise exception even when both client_setinfo and echo fail with ACL ResponseError client = RedisConnectionFactory.get_redis_connection("redis://localhost:6379") - assert client == mock_client - mock_client.client_setinfo.assert_called_once() - mock_client.echo.assert_called_once() + + assert client == mock_client + mock_client.client_setinfo.assert_called_once() + mock_client.echo.assert_called_once() @pytest.mark.asyncio async def test_get_aredis_connection_acl_fallback_denied(): + """The async factory must behave identically to the sync one.""" mock_client = MagicMock() - mock_client.client_setinfo = AsyncMock(side_effect=ResponseError("NOPERM client setinfo denied")) + mock_client.client_setinfo = AsyncMock( + side_effect=ResponseError("NOPERM client setinfo denied") + ) mock_client.echo = AsyncMock(side_effect=ResponseError("NOPERM echo denied")) - with patch("redisvl.redis.connection.AsyncRedis.from_url", return_value=mock_client): - client = await RedisConnectionFactory._get_aredis_connection("redis://localhost:6379") - assert client == mock_client - mock_client.client_setinfo.assert_called_once() - mock_client.echo.assert_called_once() + with patch( + "redisvl.redis.connection.AsyncRedis.from_url", return_value=mock_client + ): + client = await RedisConnectionFactory._get_aredis_connection( + "redis://localhost:6379" + ) + + assert client == mock_client + mock_client.client_setinfo.assert_called_once() + mock_client.echo.assert_called_once() def test_validate_sync_redis_acl_fallback_denied(): - mock_client = MagicMock() - # Ensure issubclass check passes by mocking type or using Redis subclass - from redis import Redis - class MockRedis(Redis): - pass + """validate_sync_redis must not propagate a denied ECHO fallback. - mock_client = MagicMock(spec=MockRedis) - mock_client.client_setinfo.side_effect = ResponseError("NOPERM client setinfo denied") - mock_client.echo.side_effect = ResponseError("NOPERM echo denied") + A real ``Redis`` instance is used because ``validate_sync_redis`` gates on + ``issubclass(type(redis_client), ...)``, which a ``MagicMock(spec=Redis)`` + does not satisfy. Construction does not open a socket, and both commands are + patched, so no server is contacted. + """ + client = Redis(host="localhost", port=6379) - RedisConnectionFactory.validate_sync_redis(mock_client) - mock_client.client_setinfo.assert_called_once() - mock_client.echo.assert_called_once() + with ( + patch.object( + client, "client_setinfo", side_effect=ResponseError("NOPERM setinfo denied") + ) as setinfo, + patch.object( + client, "echo", side_effect=ResponseError("NOPERM echo denied") + ) as echo, + ): + RedisConnectionFactory.validate_sync_redis(client) + + setinfo.assert_called_once() + echo.assert_called_once() @pytest.mark.asyncio async def test_validate_async_redis_acl_fallback_denied(): - from redis.asyncio import Redis as AsyncRedis - class MockAsyncRedis(AsyncRedis): - pass + """validate_async_redis must not propagate a denied ECHO fallback.""" + client = AsyncRedis(host="localhost", port=6379) - mock_client = MagicMock(spec=MockAsyncRedis) - mock_client.client_setinfo = AsyncMock(side_effect=ResponseError("NOPERM client setinfo denied")) - mock_client.echo = AsyncMock(side_effect=ResponseError("NOPERM echo denied")) + with ( + patch.object( + client, + "client_setinfo", + AsyncMock(side_effect=ResponseError("NOPERM setinfo denied")), + ) as setinfo, + patch.object( + client, "echo", AsyncMock(side_effect=ResponseError("NOPERM echo denied")) + ) as echo, + ): + await RedisConnectionFactory.validate_async_redis(client) - await RedisConnectionFactory.validate_async_redis(mock_client) - mock_client.client_setinfo.assert_called_once() - mock_client.echo.assert_called_once() + setinfo.assert_called_once() + echo.assert_called_once()