From 65ee2f42019b7dd496d8cd6b9ac4f955e582525c Mon Sep 17 00:00:00 2001 From: Alex Date: Wed, 16 Sep 2026 23:22:07 +0100 Subject: [PATCH] fix: keep the whole Redis URL when handing out its databases MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The three URLs were built by string surgery, so anything after the database number was mangled rather than kept: rediss://host:6380/0?ssl_cert_reqs=required came out as .../0?ssl_cert_reqs=required/0, and a URL carrying a query but no database had /0 appended after the query. TLS and managed Redis endpoints usually carry exactly those parameters. The URL is split properly now, the three databases go in the path, and scheme, credentials, host, query and fragment are preserved. A URL that cannot be numbered this way — one that is not redis:// or rediss://, or that has something other than a number where the database goes — is refused with a message instead of being turned into something that merely looks like a URL. --- docsgpt/deploy/commands.py | 27 +++++++++++++++++++-------- tests/deploy/test_native.py | 29 +++++++++++++++++++++++++++++ 2 files changed, 48 insertions(+), 8 deletions(-) diff --git a/docsgpt/deploy/commands.py b/docsgpt/deploy/commands.py index 1495bb5b..8d00f7bb 100644 --- a/docsgpt/deploy/commands.py +++ b/docsgpt/deploy/commands.py @@ -17,6 +17,7 @@ from dataclasses import dataclass from datetime import datetime, timezone from pathlib import Path from typing import Any, Optional +from urllib.parse import urlsplit, urlunsplit from docsgpt.deploy import backup as backup_format from docsgpt.deploy import envfile, native, stack @@ -205,16 +206,26 @@ def _redis_urls(base: str) -> dict[str, str]: They start at database 0, or at the one the URL names: ``redis://host:6379/5`` puts them on 5, 6 and 7, which is how one Redis is shared with something that already uses the first databases. + Everything else in the URL is kept — TLS, credentials, and query parameters such as the + ``ssl_cert_reqs`` that a rediss:// endpoint usually needs. """ - trimmed = base.rstrip("/") - first = 0 - head, _, tail = trimmed.rpartition("/") - if tail.isdigit(): - trimmed, first = head, int(tail) + parts = urlsplit(base) + if parts.scheme not in ("redis", "rediss"): + raise DeployError( + f"the Redis URL {base!r} should start with redis:// or rediss://, with any options as " + "query parameters, so the broker, the result backend and the cache can be given a " + "database each." + ) + path = parts.path.rstrip("/").lstrip("/") + if path and not path.isdigit(): + raise DeployError( + f"the Redis URL {base!r} has {path!r} where a database number would go. Pass a URL like " + "redis://host:6379 or redis://host:6379/5." + ) + first = int(path) if path else 0 return { - "CELERY_BROKER_URL": f"{trimmed}/{first}", - "CELERY_RESULT_BACKEND": f"{trimmed}/{first + 1}", - "CACHE_REDIS_URL": f"{trimmed}/{first + 2}", + key: urlunsplit((parts.scheme, parts.netloc, f"/{first + offset}", parts.query, parts.fragment)) + for key, offset in (("CELERY_BROKER_URL", 0), ("CELERY_RESULT_BACKEND", 1), ("CACHE_REDIS_URL", 2)) } diff --git a/tests/deploy/test_native.py b/tests/deploy/test_native.py index 31dc6729..cd7092ca 100644 --- a/tests/deploy/test_native.py +++ b/tests/deploy/test_native.py @@ -180,6 +180,35 @@ class TestNativeUp: "--expose", "local", "--no-docling"] assert _run(argv, _native_context()) == 0 + def test_a_redis_url_keeps_its_credentials_tls_and_query(self, tmp_path): + """A rediss:// endpoint usually needs ssl_cert_reqs, and losing it breaks every connection.""" + url = "rediss://user:pw@redis.example.com:6380/3?ssl_cert_reqs=required" + argv = ["up", "--native", "--dir", str(tmp_path), "--yes", + "--postgres-uri", "postgresql://localhost/d", "--redis-url", url] + assert _run(argv, _native_context()) == 0 + env = envfile.read(tmp_path / ".env") + host = "rediss://user:pw@redis.example.com:6380" + assert env["CELERY_BROKER_URL"] == f"{host}/3?ssl_cert_reqs=required" + assert env["CELERY_RESULT_BACKEND"] == f"{host}/4?ssl_cert_reqs=required" + assert env["CACHE_REDIS_URL"] == f"{host}/5?ssl_cert_reqs=required" + + def test_a_redis_url_with_a_query_and_no_database(self, tmp_path): + argv = ["up", "--native", "--dir", str(tmp_path), "--yes", "--postgres-uri", "postgresql://localhost/d", + "--redis-url", "redis://localhost:6379?health_check_interval=30"] + assert _run(argv, _native_context()) == 0 + env = envfile.read(tmp_path / ".env") + assert env["CELERY_BROKER_URL"] == "redis://localhost:6379/0?health_check_interval=30" + assert env["CACHE_REDIS_URL"] == "redis://localhost:6379/2?health_check_interval=30" + + @pytest.mark.parametrize("url", ["localhost:6379", "redis+socket:///var/run/redis.sock", + "redis://localhost:6379/queue"]) + def test_a_redis_url_that_cannot_be_numbered_is_refused(self, tmp_path, url): + """Silently turning it into something that looks like a URL is the worse failure.""" + argv = ["up", "--native", "--dir", str(tmp_path), "--yes", + "--postgres-uri", "postgresql://localhost/d", "--redis-url", url] + with pytest.raises(DeployError, match="Redis URL"): + _run(argv, _native_context()) + def test_a_database_url_is_required(self, tmp_path): with pytest.raises(DeployError, match="--postgres-uri"): _run(["up", "--native", "--dir", str(tmp_path), "--yes"], _native_context())