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

Various items missing from redis client's from_url kwargs #12149

Open
jonathan-hill-visasq opened this issue Jun 17, 2024 · 4 comments
Open

Various items missing from redis client's from_url kwargs #12149

jonathan-hill-visasq opened this issue Jun 17, 2024 · 4 comments
Labels
help wanted An actionable problem of low to medium complexity where a PR would be very welcome stubs: false positive Type checkers report false errors

Comments

@jonathan-hill-visasq
Copy link

jonathan-hill-visasq commented Jun 17, 2024

To get an instance of the redis client (redis.Redis), you can either call the constructor (docs) directly, or use the from_url method.

In both cases you can specify options using kwargs, but a few of the kwargs are missing from the stub for from_url. This results in mypy (v1.10.0) giving an error when you use one of those kwargs - for example ssl_ca_data like so:

import redis


redis.Redis.from_url(
        url="redis://abc",
        health_check_interval=3,
        ssl_ca_data="aaa",
)
s.py:5: error: No overload variant of "from_url" of "Redis" matches argument types "str", "int", "str"  [call-overload]

(Using any of the kwargs defined in the stub for from_url is ok)

At runtime, specifying ssl_ca_data to from_url works fine, so its a false positive in mypy caused by the inaccurate stub as far as I can tell. Also from examining the insides of from_url, ssl_ca_data does seem to be a valid kwarg to give it.

From the discussion at #10592 I'm aware that the typing for redis is incomplete and a bit up in the air in general, but this seems like a relatively easy thing to fix.

environment information

python version: 3.11.0
redis version: 4.4.0
types-redis version: 4.5.4.1

edited
corrected url arg in code excerpt to reflect ssl schema (ie, start with 'redis://').
(This results in the connection_pool.connection_class in the Redis object being set as SSLConnection - a class which has ssl_ca_data defined as constructor kwarg - so in this case passing ssl_ca_data to Redis.from_url is meaningful.)

@srittau
Copy link
Collaborator

srittau commented Jun 17, 2024

Please note that your example won't work at runtime (using redis 4.6.0):

import redis

r = redis.Redis.from_url(
        url="redis://",
        health_check_interval=3,
        ssl_ca_data="aaa",
)
r.get("aa")
Traceback (most recent call last):
  File "/home/srittau/foo/lib/python3.12/site-packages/redis/connection.py", line 1454, in get_connection
    connection = self._available_connections.pop()
                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
IndexError: pop from empty list

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "/home/srittau/foo.py", line 8, in <module>
    r.get("aa")
  File "/home/srittau/foo/lib/python3.12/site-packages/redis/commands/core.py", line 1816, in get
    return self.execute_command("GET", name)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/srittau/foo/lib/python3.12/site-packages/redis/client.py", line 1266, in execute_command
    conn = self.connection or pool.get_connection(command_name, **options)
                              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/srittau/foo/lib/python3.12/site-packages/redis/connection.py", line 1456, in get_connection
    connection = self.make_connection()
                 ^^^^^^^^^^^^^^^^^^^^^^
  File "/home/srittau/foo/lib/python3.12/site-packages/redis/connection.py", line 1496, in make_connection
    return self.connection_class(**self.connection_kwargs)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/srittau/foo/lib/python3.12/site-packages/redis/connection.py", line 960, in __init__
    super().__init__(**kwargs)
TypeError: AbstractConnection.__init__() got an unexpected keyword argument 'ssl_ca_data'

The additional kwargs are passed to the Connection the moment a connection is established. The standard Connection class has no keyword argument ssl_ca_data.

@jonathan-hill-visasq
Copy link
Author

jonathan-hill-visasq commented Jun 18, 2024

@srittau my apologies, my excerpt didn't reflect the connection url's schema (now changed).

In the case that the from_url function infers ssl, the connection class becomes SSLConnection which has ssl_ca_data defined as a kwarg.

So admittedly ssl_ca_data isn't meaningful for every instance of Redis, but the same could surely be said for the regular constructor surely.

@srittau
Copy link
Collaborator

srittau commented Jun 18, 2024

Ah, that makes sense. So we should probably add the arguments to SSLConnection as well, or just add **kwargs: Any. (With an appropriate comment in either case.) PR welcome!

@srittau srittau added stubs: false positive Type checkers report false errors help wanted An actionable problem of low to medium complexity where a PR would be very welcome labels Jun 18, 2024
@jonathan-h-grebe
Copy link
Contributor

I'll give the PR a go!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted An actionable problem of low to medium complexity where a PR would be very welcome stubs: false positive Type checkers report false errors
Projects
None yet
Development

No branches or pull requests

3 participants