Skip to content

Commit 0f9ccda

Browse files
bbakoumabbakouma
authored andcommitted
test: add unit tests for Redis Cluster support and stricter URL validation
- _strip_db_from_url now raises ValueError for non-zero database numbers (e.g. /1, /2) instead of silently stripping them, so misconfigurations fail fast - Add TestStripDbFromUrl: tests for /0 stripping, no-path, slash-only, and non-zero DB rejection - Add TestClusterMode: tests for cluster_mode=true (RedisCluster), cluster_mode=false (standalone), and missing attr fallback - Update .env.example with REDIS_CLUSTER_MODE documentation
1 parent 5547cd3 commit 0f9ccda

File tree

3 files changed

+169
-3
lines changed

3 files changed

+169
-3
lines changed

.env.example

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -653,6 +653,17 @@ OTEL_EXPORTER_OTLP_ENDPOINT=http://localhost:4317
653653
# Return strings instead of bytes (recommended: true)
654654
# REDIS_DECODE_RESPONSES=true
655655

656+
# =============================================================================
657+
# Redis Cluster Mode
658+
# =============================================================================
659+
660+
# Enable Redis Cluster support for deployments using Redis Cluster (e.g.
661+
# Bitnami redis-cluster Helm chart). When enabled, the client uses
662+
# redis.asyncio.RedisCluster which handles MOVED/ASK redirects automatically.
663+
# The REDIS_URL must NOT include a database number (e.g. /0) in cluster mode.
664+
# Default: false
665+
# REDIS_CLUSTER_MODE=false
666+
656667
# =============================================================================
657668
# Redis Parser Configuration (Performance - ADR-026)
658669
# =============================================================================

mcpgateway/utils/redis_client.py

Lines changed: 23 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -94,18 +94,38 @@ def _get_async_parser_class(parser_setting: str) -> tuple[Any, str]:
9494
def _strip_db_from_url(url: str) -> str:
9595
"""Remove the database number from a Redis URL for cluster mode.
9696
97-
Redis Cluster only supports database 0, so ``/0`` (or any ``/N``) must be
98-
stripped from the URL before passing it to ``RedisCluster.from_url()``.
97+
Redis Cluster only supports database 0. If the URL contains ``/0`` it is
98+
silently stripped. If it contains a non-zero database (``/1``, ``/2``, …)
99+
a ``ValueError`` is raised so that misconfigurations fail fast instead of
100+
being silently ignored.
99101
100102
Args:
101103
url: Redis connection URL, e.g. ``redis://:pass@host:6379/0``
102104
103105
Returns:
104106
URL without the trailing database path, e.g. ``redis://:pass@host:6379``
107+
108+
Raises:
109+
ValueError: If the URL specifies a non-zero database number.
110+
111+
Examples:
112+
>>> _strip_db_from_url("redis://:pass@host:6379/0")
113+
'redis://:pass@host:6379'
114+
>>> _strip_db_from_url("redis://:pass@host:6379")
115+
'redis://:pass@host:6379'
116+
>>> _strip_db_from_url("redis://:pass@host:6379/1")
117+
Traceback (most recent call last):
118+
...
119+
ValueError: Redis Cluster only supports database 0, but REDIS_URL specifies /1. Remove the database selector or use /0.
105120
"""
106121
parsed = urlparse(url)
107122
if parsed.path and parsed.path not in ("", "/"):
108-
# Remove the database path (e.g. /0, /1, etc.)
123+
db_str = parsed.path.lstrip("/")
124+
if db_str and db_str != "0":
125+
raise ValueError(
126+
f"Redis Cluster only supports database 0, but REDIS_URL "
127+
f"specifies /{db_str}. Remove the database selector or use /0."
128+
)
109129
cleaned = parsed._replace(path="")
110130
return cleaned.geturl()
111131
return url

tests/unit/mcpgateway/utils/test_redis_client.py

Lines changed: 135 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -510,3 +510,138 @@ async def test_get_redis_client_parser_configuration_error_returns_none():
510510

511511
assert client is None
512512
mock_from_url.assert_not_called()
513+
514+
515+
# ---------------------------------------------------------------------------
516+
# Tests for Redis Cluster support
517+
# ---------------------------------------------------------------------------
518+
519+
520+
class TestStripDbFromUrl:
521+
"""Tests for _strip_db_from_url helper."""
522+
523+
def test_strips_db_zero(self):
524+
"""Strips /0 from URL."""
525+
from mcpgateway.utils.redis_client import _strip_db_from_url
526+
527+
result = _strip_db_from_url("redis://:pass@host:6379/0")
528+
assert result == "redis://:pass@host:6379"
529+
530+
def test_no_db_path_unchanged(self):
531+
"""URL without database path is returned unchanged."""
532+
from mcpgateway.utils.redis_client import _strip_db_from_url
533+
534+
result = _strip_db_from_url("redis://:pass@host:6379")
535+
assert result == "redis://:pass@host:6379"
536+
537+
def test_slash_only_unchanged(self):
538+
"""URL with trailing slash only is returned unchanged."""
539+
from mcpgateway.utils.redis_client import _strip_db_from_url
540+
541+
result = _strip_db_from_url("redis://:pass@host:6379/")
542+
assert result == "redis://:pass@host:6379/"
543+
544+
def test_non_zero_db_raises_error(self):
545+
"""Non-zero database number raises ValueError."""
546+
from mcpgateway.utils.redis_client import _strip_db_from_url
547+
548+
with pytest.raises(ValueError, match="Redis Cluster only supports database 0"):
549+
_strip_db_from_url("redis://:pass@host:6379/1")
550+
551+
def test_non_zero_db_2_raises_error(self):
552+
"""Database /2 raises ValueError."""
553+
from mcpgateway.utils.redis_client import _strip_db_from_url
554+
555+
with pytest.raises(ValueError, match="Redis Cluster only supports database 0"):
556+
_strip_db_from_url("redis://:pass@host:6379/2")
557+
558+
559+
class TestClusterMode:
560+
"""Tests for REDIS_CLUSTER_MODE setting."""
561+
562+
@pytest.mark.asyncio
563+
async def test_cluster_mode_false_uses_standalone(self):
564+
"""When redis_cluster_mode=False, uses standalone from_url."""
565+
mock_redis = AsyncMock()
566+
mock_redis.ping = AsyncMock(return_value=True)
567+
568+
with patch("mcpgateway.config.settings") as mock_settings:
569+
mock_settings.cache_type = "redis"
570+
mock_settings.redis_url = "redis://localhost:6379/0"
571+
mock_settings.redis_decode_responses = True
572+
mock_settings.redis_max_connections = 10
573+
mock_settings.redis_socket_timeout = 5.0
574+
mock_settings.redis_socket_connect_timeout = 5.0
575+
mock_settings.redis_retry_on_timeout = True
576+
mock_settings.redis_health_check_interval = 30
577+
mock_settings.redis_parser = "auto"
578+
mock_settings.redis_cluster_mode = False
579+
580+
with patch("redis.asyncio.from_url", return_value=mock_redis) as mock_from_url:
581+
client = await get_redis_client()
582+
583+
assert client is mock_redis
584+
mock_from_url.assert_called_once()
585+
586+
@pytest.mark.asyncio
587+
async def test_cluster_mode_true_uses_redis_cluster(self):
588+
"""When redis_cluster_mode=True, uses RedisCluster.from_url."""
589+
mock_cluster = AsyncMock()
590+
mock_cluster.ping = AsyncMock(return_value=True)
591+
592+
mock_redis_cluster_cls = MagicMock()
593+
mock_redis_cluster_cls.from_url = MagicMock(return_value=mock_cluster)
594+
595+
with patch("mcpgateway.config.settings") as mock_settings:
596+
mock_settings.cache_type = "redis"
597+
mock_settings.redis_url = "redis://:pass@redis-cluster:6379/0"
598+
mock_settings.redis_decode_responses = True
599+
mock_settings.redis_max_connections = 10
600+
mock_settings.redis_socket_timeout = 5.0
601+
mock_settings.redis_socket_connect_timeout = 5.0
602+
mock_settings.redis_retry_on_timeout = True
603+
mock_settings.redis_health_check_interval = 30
604+
mock_settings.redis_parser = "auto"
605+
mock_settings.redis_cluster_mode = True
606+
607+
mock_aioredis = MagicMock()
608+
mock_aioredis.RedisCluster = mock_redis_cluster_cls
609+
610+
with patch.dict("sys.modules", {"redis.asyncio": mock_aioredis}):
611+
with patch("redis.asyncio", mock_aioredis):
612+
_reset_client()
613+
# Directly test the helper
614+
from mcpgateway.utils.redis_client import _create_cluster_client
615+
616+
client = await _create_cluster_client(mock_settings, mock_aioredis, None)
617+
618+
assert client is mock_cluster
619+
mock_redis_cluster_cls.from_url.assert_called_once()
620+
# Verify /0 was stripped from URL
621+
call_args = mock_redis_cluster_cls.from_url.call_args
622+
assert "/0" not in call_args[0][0]
623+
624+
@pytest.mark.asyncio
625+
async def test_cluster_mode_missing_attr_defaults_false(self):
626+
"""When redis_cluster_mode attr is missing, defaults to False (standalone)."""
627+
mock_redis = AsyncMock()
628+
mock_redis.ping = AsyncMock(return_value=True)
629+
630+
with patch("mcpgateway.config.settings") as mock_settings:
631+
mock_settings.cache_type = "redis"
632+
mock_settings.redis_url = "redis://localhost:6379/0"
633+
mock_settings.redis_decode_responses = True
634+
mock_settings.redis_max_connections = 10
635+
mock_settings.redis_socket_timeout = 5.0
636+
mock_settings.redis_socket_connect_timeout = 5.0
637+
mock_settings.redis_retry_on_timeout = True
638+
mock_settings.redis_health_check_interval = 30
639+
mock_settings.redis_parser = "auto"
640+
# Simulate missing attribute
641+
del mock_settings.redis_cluster_mode
642+
643+
with patch("redis.asyncio.from_url", return_value=mock_redis) as mock_from_url:
644+
client = await get_redis_client()
645+
646+
assert client is mock_redis
647+
mock_from_url.assert_called_once()

0 commit comments

Comments
 (0)