Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Provide support for redis:// and rediss:// #23

Closed
kjaymiller opened this issue Jun 26, 2024 · 5 comments
Closed

Provide support for redis:// and rediss:// #23

kjaymiller opened this issue Jun 26, 2024 · 5 comments

Comments

@kjaymiller
Copy link
Contributor

def parse_url(url: str) -> ConnectKwargs:
parsed: ParseResult = urlparse(url)
kwargs: ConnectKwargs = {}
for name, value_list in parse_qs(parsed.query).items():
if value_list and len(value_list) > 0:
value = unquote(value_list[0])
parser = URL_QUERY_ARGUMENT_PARSERS.get(name)
if parser:
try:
kwargs[name] = parser(value)
except (TypeError, ValueError):
raise ValueError(f"Invalid value for `{name}` in connection URL.")
else:
kwargs[name] = value
if parsed.username:
kwargs["username"] = unquote(parsed.username)
if parsed.password:
kwargs["password"] = unquote(parsed.password)
# We only support valkey://, valkeys:// and unix:// schemes.
if parsed.scheme == "unix":
if parsed.path:
kwargs["path"] = unquote(parsed.path)
kwargs["connection_class"] = UnixDomainSocketConnection
elif parsed.scheme in ("valkey", "valkeys"):
if parsed.hostname:
kwargs["host"] = unquote(parsed.hostname)
if parsed.port:
kwargs["port"] = int(parsed.port)
# If there's a path argument, use it as the db argument if a
# querystring value wasn't specified
if parsed.path and "db" not in kwargs:
try:
kwargs["db"] = int(unquote(parsed.path).replace("/", ""))
except (AttributeError, ValueError):
pass
if parsed.scheme == "valkeys":
kwargs["connection_class"] = SSLConnection
else:
valid_schemes = "valkey://, valkeys://, unix://"
raise ValueError(
f"Valkey URL must specify one of the following schemes ({valid_schemes})"
)
return kwargs

This states that only valkey://, valkeys://, and unix but some providers like aiven are still using the rediss://.

Is there a reason that we aren't allowing the redis:// and rediss:// protocols or at least providing a deprecation warning if they are used?

This error gave me initial pause on if I could use the valkey client to test with new providers of Valkey.

@kjaymiller
Copy link
Contributor Author

also happy to contribute a PR that adds redis:// and rediss://.

@aiven-sal
Copy link
Member

Thanks for reporting this issue!

You can use valkey:// and valkeys:// in place of redis:// and rediss:// regardless of what your provider says you should be using.
Valkey and Redis protocol are the same at the moment and, if they will ever change, valkey-py will only support the Valkey protocol.

But I agree that the transition will be easier if we supported redis:// and rediss:// as aliases of valkey:// and valkeys://, as long as the two protocols remain identical.

Of course PRs are always welcome.

@kjaymiller
Copy link
Contributor Author

happy to contribute. I think the biggest question to add would be should a warning be issued when redis:// or rediss:// are used?

@aiven-sal
Copy link
Member

I think that, at this point, it would be nice if people could swap redis-py with valkey-py without noticing any difference at all. So I wouldn't add a warning. At some point in the future we should definitely add a deprecation warning, but for now I'd like to make the transition as seamless as possible.

@aiven-sal
Copy link
Member

Fixed by #29

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants