python / typeshed

Collection of library stubs for Python, with static types
Other
4.2k stars 1.7k forks source link

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

Open jonathan-hill-visasq opened 1 month ago

jonathan-hill-visasq commented 1 month ago

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 https://github.com/python/typeshed/issues/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 commented 1 month ago

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 commented 1 month ago

@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 commented 1 month ago

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!

jonathan-h-grebe commented 1 month ago

I'll give the PR a go!