Skip to content

Commit 410bf8f

Browse files
Normalize Redis connection handling and CLI URL resolution (#576)
# Summary This PR makes Redis connection behavior more consistent across the SDK and CLI. It fixes cases where different surfaces created Redis clients differently, dropped connection kwargs, or treated internally-created clients as externally owned. On the SDK side, `from_existing()` paths for `SearchIndex`, `AsyncSearchIndex`, and `SemanticRouter` now consistently prefer a caller-provided client, preserve normalized connection kwargs, and correctly mark internally-created clients as owned so disconnect behavior works as expected. The shared kwargs splitting used by these entry points is now centralized in `redisvl/redis/connection.py` so internal _-prefixed kwargs do not leak into Redis client construction. This PR also routes more connection creation through `RedisConnectionFactory` instead of ad hoc client creation. That includes cache and SQL helper paths, and it fixes several extension surfaces that were passing connection kwargs incorrectly into SearchIndex. On the CLI side, Redis URL resolution is simplified and made more predictable. The helper now cleanly resolves between explicit `--url`, explicit connection flags, `REDIS_URL`, and the local default, while also avoiding malformed URL assembly and accidental empty-auth behavior. The CLI entrypoints were simplified accordingly. The net effect is a smaller and more uniform connection model: explicit clients are respected, factory-based connection behavior is reused across surfaces, reconnect/disconnect semantics are correct, and the CLI no longer has several connection-resolution edge cases that previously produced the wrong URL. <!-- CURSOR_SUMMARY --> --- > [!NOTE] > **Medium Risk** > Touches core connection creation/ownership paths across `SearchIndex`, caches, routers, and CLI URL parsing; behavior changes could affect how clients are constructed and when connections are closed. > > **Overview** > **Normalizes Redis connection behavior across the SDK and CLI.** CLI commands now resolve Redis endpoints deterministically (explicit `--url` > explicit flags > `REDIS_URL` > localhost defaults), properly URL-encode auth, and only override env URLs with SSL via scheme rewriting. > > **Unifies client creation and ownership semantics.** `SearchIndex.from_existing`/`AsyncSearchIndex.from_existing` and `SemanticRouter.from_existing` now prefer caller-provided clients, split/forward only connection kwargs to `RedisConnectionFactory`, track whether internally-created clients are owned (so `disconnect()` closes them), and ensure temporary clients are closed on load failures. Several extensions (`SemanticCache`, message histories, router init) now pass `connection_kwargs` explicitly (instead of splatting), and SQL query helper/client creation is routed through `RedisConnectionFactory`. > > Adds unit tests covering CLI URL precedence/encoding and connection normalization (client preference, kwargs forwarding, and ownership/cleanup). > > <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit 517d281. Bugbot is set up for automated code reviews on this repo. Configure [here](https://www.cursor.com/dashboard/bugbot).</sup> <!-- /CURSOR_SUMMARY --> --------- Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
1 parent 166ffd6 commit 410bf8f

14 files changed

Lines changed: 709 additions & 97 deletions

File tree

redisvl/cli/index.py

Lines changed: 2 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -97,18 +97,13 @@ def destroy(self, args: Namespace):
9797

9898
def _connect_to_index(self, args: Namespace) -> SearchIndex:
9999
# connect to redis
100-
try:
101-
redis_url = create_redis_url(args)
102-
conn = RedisConnectionFactory.get_redis_connection(redis_url=redis_url)
103-
except ValueError:
104-
print("Must set REDIS_URL environment variable or provide host and port")
105-
exit(1)
100+
redis_url = create_redis_url(args)
106101

107102
if args.index:
108103
schema = IndexSchema.from_dict({"index": {"name": args.index}})
109104
index = SearchIndex(schema=schema, redis_url=redis_url)
110105
elif args.schema:
111-
index = SearchIndex.from_yaml(args.schema, redis_client=conn)
106+
index = SearchIndex.from_yaml(args.schema, redis_url=redis_url)
112107
else:
113108
print("Index name or schema must be provided")
114109
exit(1)

redisvl/cli/stats.py

Lines changed: 1 addition & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -60,13 +60,7 @@ def stats(self, args: Namespace):
6060

6161
def _connect_to_index(self, args: Namespace) -> SearchIndex:
6262
# connect to redis
63-
try:
64-
redis_url = create_redis_url(args)
65-
except ValueError:
66-
logger.error(
67-
"Must set REDIS_ADDRESS environment variable or provide host and port"
68-
)
69-
exit(0)
63+
redis_url = create_redis_url(args)
7064

7165
if args.index:
7266
schema = IndexSchema.from_dict({"index": {"name": args.index}})

redisvl/cli/utils.py

Lines changed: 73 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -1,29 +1,77 @@
11
import os
22
from argparse import ArgumentParser, Namespace
3+
from typing import Optional
4+
from urllib.parse import quote, urlparse, urlunparse
35

6+
from redisvl.redis.constants import REDIS_URL_ENV_VAR
47
from redisvl.utils.log import get_logger
58

69
logger = get_logger("[RedisVL]")
10+
DEFAULT_REDIS_HOST = "localhost"
11+
DEFAULT_REDIS_PORT = 6379
12+
13+
14+
def _has_explicit_connection_options(args: Namespace) -> bool:
15+
return any(
16+
(
17+
getattr(args, "host", None) is not None,
18+
getattr(args, "port", None) is not None,
19+
bool(getattr(args, "user", None)),
20+
bool(getattr(args, "password", None)),
21+
)
22+
)
23+
24+
25+
def _get_auth_credentials(args: Namespace) -> tuple[Optional[str], Optional[str]]:
26+
return getattr(args, "user", None) or None, getattr(args, "password", None) or None
27+
28+
29+
def _build_redis_url(args: Namespace) -> str:
30+
scheme = "rediss" if getattr(args, "ssl", False) else "redis"
31+
host = getattr(args, "host", None)
32+
if host is None:
33+
host = DEFAULT_REDIS_HOST
34+
35+
port = getattr(args, "port", None)
36+
if port is None:
37+
port = DEFAULT_REDIS_PORT
38+
user, password = _get_auth_credentials(args)
39+
40+
auth = ""
41+
if user:
42+
auth = quote(user, safe="")
43+
if password:
44+
auth += f":{quote(password, safe='')}"
45+
auth += "@"
46+
elif password:
47+
auth = f":{quote(password, safe='')}@"
48+
49+
return f"{scheme}://{auth}{host}:{port}"
50+
51+
52+
def _apply_ssl_scheme(url: str) -> str:
53+
parsed_url = urlparse(url)
54+
if parsed_url.scheme == "rediss":
55+
return url
56+
return urlunparse(parsed_url._replace(scheme="rediss"))
757

858

959
def create_redis_url(args: Namespace) -> str:
10-
env_address = os.getenv("REDIS_URL")
60+
if args.url:
61+
return args.url
62+
if _has_explicit_connection_options(args):
63+
return _build_redis_url(args)
64+
65+
env_address = os.getenv(REDIS_URL_ENV_VAR)
1166
if env_address:
12-
logger.info(f"Using Redis address from environment variable, REDIS_URL")
67+
logger.info(
68+
f"Using Redis address from environment variable, {REDIS_URL_ENV_VAR}"
69+
)
70+
if getattr(args, "ssl", False):
71+
return _apply_ssl_scheme(env_address)
1372
return env_address
14-
elif args.url:
15-
return args.url
16-
else:
17-
url = "redis://"
18-
if args.ssl:
19-
url += "rediss://"
20-
if args.user:
21-
url += args.user
22-
if args.password:
23-
url += ":" + args.password
24-
url += "@"
25-
url += args.host + ":" + str(args.port)
26-
return url
73+
74+
return _build_redis_url(args)
2775

2876

2977
def add_index_parsing_options(parser: ArgumentParser) -> ArgumentParser:
@@ -32,9 +80,15 @@ def add_index_parsing_options(parser: ArgumentParser) -> ArgumentParser:
3280
"-s", "--schema", help="Path to schema file", type=str, required=False
3381
)
3482
parser.add_argument("-u", "--url", help="Redis URL", type=str, required=False)
35-
parser.add_argument("--host", help="Redis host", type=str, default="localhost")
36-
parser.add_argument("-p", "--port", help="Redis port", type=int, default=6379)
37-
parser.add_argument("--user", help="Redis username", type=str, default="default")
83+
parser.add_argument("--host", help="Redis host", type=str, default=None)
84+
parser.add_argument("-p", "--port", help="Redis port", type=int, default=None)
85+
parser.add_argument("--user", help="Redis username", type=str, default=None)
3886
parser.add_argument("--ssl", help="Use SSL", action="store_true")
39-
parser.add_argument("-a", "--password", help="Redis password", type=str, default="")
87+
parser.add_argument(
88+
"-a",
89+
"--password",
90+
help="Redis password",
91+
type=str,
92+
default=None,
93+
)
4094
return parser

redisvl/extensions/cache/base.py

Lines changed: 12 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,7 @@
55
"""
66

77
from collections.abc import Mapping
8-
from typing import Any, Dict, Optional
8+
from typing import Any, Dict, Optional, cast
99

1010
from redis import Redis # For backwards compatibility in type checking
1111
from redis.cluster import RedisCluster
@@ -113,9 +113,12 @@ def _get_redis_client(self) -> SyncRedisClient:
113113
"""
114114
if self._redis_client is None:
115115
# Create new Redis client
116-
url = self.redis_kwargs["redis_url"]
117-
kwargs = self.redis_kwargs["connection_kwargs"]
118-
self._redis_client = Redis.from_url(url, **kwargs) # type: ignore
116+
url = cast(Optional[str], self.redis_kwargs["redis_url"])
117+
kwargs = cast(Dict[str, Any], self.redis_kwargs["connection_kwargs"])
118+
self._redis_client = RedisConnectionFactory.get_redis_connection(
119+
redis_url=url,
120+
**kwargs,
121+
)
119122
return self._redis_client
120123

121124
async def _get_async_redis_client(self) -> AsyncRedisClient:
@@ -132,15 +135,12 @@ async def _get_async_redis_client(self) -> AsyncRedisClient:
132135
client
133136
)
134137
else:
135-
url = str(self.redis_kwargs["redis_url"])
136-
kwargs = self.redis_kwargs.get("connection_kwargs", {})
137-
if not isinstance(kwargs, Mapping):
138-
raise TypeError(
139-
"Expected `connection_kwargs` to be a dictionary (e.g. {'decode_responses': True}), "
140-
f"but got type: {type(kwargs).__name__}"
141-
)
138+
url = cast(Optional[str], self.redis_kwargs["redis_url"])
139+
kwargs = cast(Dict[str, Any], self.redis_kwargs["connection_kwargs"])
142140
self._async_redis_client = (
143-
RedisConnectionFactory.get_async_redis_connection(url, **kwargs)
141+
RedisConnectionFactory.get_async_redis_connection(
142+
redis_url=url, **kwargs
143+
)
144144
)
145145
return self._async_redis_client
146146

redisvl/extensions/cache/llm/semantic.py

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -141,7 +141,7 @@ def __init__(
141141
schema=schema,
142142
redis_client=self._redis_client,
143143
redis_url=self.redis_kwargs["redis_url"],
144-
**self.redis_kwargs["connection_kwargs"],
144+
connection_kwargs=self.redis_kwargs["connection_kwargs"] or None,
145145
)
146146
self._aindex = None
147147

@@ -211,7 +211,7 @@ async def _get_async_index(self) -> AsyncSearchIndex:
211211
schema=self._index.schema,
212212
redis_client=async_client,
213213
redis_url=self.redis_kwargs["redis_url"],
214-
**self.redis_kwargs["connection_kwargs"],
214+
connection_kwargs=self.redis_kwargs["connection_kwargs"] or None,
215215
)
216216
return self._aindex
217217

redisvl/extensions/message_history/message_history.py

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -61,7 +61,7 @@ def __init__(
6161
schema=schema,
6262
redis_client=redis_client,
6363
redis_url=redis_url,
64-
**connection_kwargs,
64+
connection_kwargs=connection_kwargs or None,
6565
)
6666

6767
self._index.create(overwrite=False)

redisvl/extensions/message_history/semantic_history.py

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -103,7 +103,7 @@ def __init__(
103103
schema=schema,
104104
redis_client=redis_client,
105105
redis_url=redis_url,
106-
**connection_kwargs,
106+
connection_kwargs=connection_kwargs or None,
107107
)
108108

109109
# Check for existing message history index

redisvl/extensions/router/semantic.py

Lines changed: 56 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,7 @@
1818
from redisvl.index import SearchIndex
1919
from redisvl.query import FilterQuery, VectorRangeQuery
2020
from redisvl.query.filter import Tag
21-
from redisvl.redis.connection import RedisConnectionFactory
21+
from redisvl.redis.connection import RedisConnectionFactory, _split_from_existing_kwargs
2222
from redisvl.redis.utils import convert_bytes, hashify, make_dict
2323
from redisvl.types import SyncRedisClient
2424
from redisvl.utils.log import get_logger
@@ -72,6 +72,7 @@ def __init__(
7272
for the redis client. Defaults to empty {}.
7373
"""
7474
dtype = kwargs.pop("dtype", None)
75+
index_kwargs = kwargs.pop("_index_kwargs", None)
7576

7677
# Validate a provided vectorizer or set the default
7778
if vectorizer:
@@ -104,7 +105,13 @@ def __init__(
104105
redis_client=redis_client,
105106
)
106107

107-
self._initialize_index(redis_client, redis_url, overwrite, **connection_kwargs)
108+
self._initialize_index(
109+
redis_client,
110+
redis_url,
111+
overwrite,
112+
connection_kwargs=connection_kwargs or None,
113+
index_kwargs=index_kwargs,
114+
)
108115

109116
self._index.client.json().set(f"{self.name}:route_config", f".", self.to_dict()) # type: ignore
110117

@@ -117,27 +124,57 @@ def from_existing(
117124
**kwargs,
118125
) -> "SemanticRouter":
119126
"""Return SemanticRouter instance from existing index."""
120-
if redis_url:
127+
init_kwargs, connection_kwargs = _split_from_existing_kwargs(
128+
dict(kwargs),
129+
nested_connection_keys=("connection_kwargs",),
130+
)
131+
lib_name = init_kwargs.get("lib_name")
132+
index_kwargs: Dict[str, Any] = {}
133+
created_redis_client = False
134+
135+
if redis_client:
136+
# Just validate client type and set lib name
137+
RedisConnectionFactory.validate_sync_redis(redis_client, lib_name)
138+
index_kwargs["_client_validated"] = True
139+
else:
140+
factory_kwargs = {**connection_kwargs}
141+
if lib_name is not None:
142+
factory_kwargs["lib_name"] = lib_name
121143
redis_client = RedisConnectionFactory.get_redis_connection(
122144
redis_url=redis_url,
123-
**kwargs,
124-
)
125-
elif redis_client:
126-
# Just validate client type and set lib name
127-
RedisConnectionFactory.validate_sync_redis(redis_client)
128-
if redis_client is None:
129-
raise ValueError(
130-
"Creating Redis client failed. Please check the redis_url and connection_kwargs."
145+
**factory_kwargs,
131146
)
147+
index_kwargs["_client_validated"] = True
148+
index_kwargs["_owns_redis_client"] = True
149+
if lib_name is not None:
150+
index_kwargs["lib_name"] = lib_name
151+
created_redis_client = True
132152

133-
router_dict = redis_client.json().get(f"{name}:route_config")
153+
try:
154+
router_dict = redis_client.json().get(f"{name}:route_config")
155+
except Exception:
156+
if created_redis_client and redis_client is not None:
157+
redis_client.close()
158+
raise
134159
if not isinstance(router_dict, dict):
160+
if created_redis_client and redis_client is not None:
161+
redis_client.close()
135162
raise ValueError(
136163
f"No valid router config found for {name}. Received: {router_dict!r}"
137164
)
138-
return cls.from_dict(
139-
router_dict, redis_url=redis_url, redis_client=redis_client
140-
)
165+
resolved_redis_url = redis_url if created_redis_client else None
166+
try:
167+
return cls.from_dict(
168+
router_dict,
169+
redis_url=resolved_redis_url,
170+
redis_client=redis_client,
171+
connection_kwargs=connection_kwargs or None,
172+
_index_kwargs={**init_kwargs, **index_kwargs} or None,
173+
)
174+
except Exception:
175+
if created_redis_client and redis_client is not None:
176+
redis_client.close()
177+
raise
141178

142179
@deprecated_argument("dtype")
143180
def _initialize_index(
@@ -146,7 +183,8 @@ def _initialize_index(
146183
redis_url: str = "redis://localhost:6379",
147184
overwrite: bool = False,
148185
dtype: str = "float32",
149-
**connection_kwargs,
186+
connection_kwargs: Optional[Dict[str, Any]] = None,
187+
index_kwargs: Optional[Dict[str, Any]] = None,
150188
):
151189
"""Initialize the search index and handle Redis connection."""
152190

@@ -158,7 +196,8 @@ def _initialize_index(
158196
schema=schema,
159197
redis_client=redis_client,
160198
redis_url=redis_url,
161-
**connection_kwargs,
199+
connection_kwargs=connection_kwargs,
200+
**(index_kwargs or {}),
162201
)
163202

164203
# Check for existing router index

0 commit comments

Comments
 (0)