Skip to content

Conversation

@cy
Copy link
Contributor

@cy cy commented Oct 22, 2020

Adds stubs for the ACL commands available in Redis 6 that were added to redis-py in v 3.4.0, except acl_log and acl_log_reset that are in redis-py master but not yet released https://github.com/andymccurdy/redis-py/blob/master/CHANGES#L11

Reference code
https://github.com/andymccurdy/redis-py/blob/master/redis/client.py#L965

Examples

acl_cat https://redis.io/commands/acl-cat

In [6]: result = Redis(host="localhost", port=6379).acl_cat()

In [7]: type(result)
Out[7]: list

acl_deluser https://redis.io/commands/acl-deluser (returns number of users that were deleted.)

In [9]: result = Redis(host="localhost", port=6379).acl_deluser("cy")

In [10]: type(result)
Out[10]: int

In [11]: result
Out[11]: 1

acl_genpass https://redis.io/commands/acl-genpass

In [12]: result = Redis(host="localhost", port=6379).acl_genpass()

In [13]: type(result)
Out[13]: str

acl_getuser https://redis.io/commands/acl-getuser

In [17]: result = Redis(host="localhost", port=6379).acl_getuser('cy')

In [18]: result
Out[18]:
{'flags': ['off'],
 'passwords': [],
 'commands': [],
 'keys': [],
 'categories': ['-@all'],
 'enabled': False}

In [19]: type(result)
Out[19]: dict

acl_list https://redis.io/commands/acl-list

In [20]: result = Redis(host="localhost", port=6379).acl_list()

In [21]: result
Out[21]: ['user cy off -@all', 'user default on nopass ~* +@all']

acl_load https://redis.io/commands/acl-load

In [22]: result = Redis(host="localhost", port=6379).acl_load()

In [23]: result
Out[23]: True

In [24]: type(result)
Out[24]: bool

acl_setuser -> https://github.com/andymccurdy/redis-py/blob/master/redis/client.py#L1042

In [28]: result = Redis(host="localhost", port=6379).acl_setuser("cy", categories=["+string"], commands=["+get"])

In [29]: result
Out[29]: True

acl_users https://redis.io/commands/acl-users

In [31]: result = Redis(host="localhost", port=6379).acl_users()

In [32]: result
Out[32]: ['cy', 'default']

acl_whoami https://redis.io/commands/acl-whoami

In [33]: result = Redis(host="localhost", port=6379).acl_whoami()

In [34]: result
Out[34]: 'default'

Copy link
Collaborator

@srittau srittau left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you, especially the link to the implementation is very useful! Remarks below.

def pubsub(self, shard_hint: Any = ..., ignore_subscribe_messages: bool = ...) -> PubSub: ...
def execute_command(self, *args, **options): ...
def parse_response(self, connection, command_name, **options): ...
def acl_cat(self, category: Optional[Text]) -> List[str]: ...
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
def acl_cat(self, category: Optional[Text]) -> List[str]: ...
def acl_cat(self, category: Optional[Text] = ...) -> List[str]: ...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed!

def acl_cat(self, category: Optional[Text]) -> List[str]: ...
def acl_deluser(self, username: Text) -> int: ...
def acl_genpass(self) -> Text: ...
def acl_getuser(self, username: Text) -> Union[None, Dict[str, Union[List[str], bool]]]: ...
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In cases like this, where the return value contains an Union (in this case Union inside the Dict, not the outer Union), we prefer to use Any instead. Otherwise the caller has to use isinstance checks each time they access the dict.

An even better alternative here would be to use a TypedDict, which can define exactly which fields and types the returned dict has. I'll leave the decision which of the three options to use (as is, Any, or a TypedDict) to you.

Also a style nit: Instead of Union[None, ...] we usually just use Optional[...].

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the detailed explanation; I chose to use Any - I would prefer to have used a TypedDict, but that requires me to name the type, which I would prefer not to do as I'm not the author of the library. (Unless there's a way to create an anonymous TypedDict that I have not been able to find!)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's fine. What we usually do in these cases is creating a name marked with an underscore. There is also the @typing.type_check_only decorator, which hasn't gained traction, but we should use it more.

Comment on lines 93 to 102
enabled: bool,
nopass: bool,
passwords: bool,
hashed_passwords: Optional[List[Text]],
categories: Optional[List[Text]],
commands: Optional[List[Text]],
keys: Optional[List[Text]],
reset: bool,
reset_keys: bool,
reset_passwords: bool,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All of these options have default values, so they should have = ....

A few more from looking at the code:

  • hashed_passwords can use the looser Sequence type and also accepts plain strings, so I suggest using Optional[Union[Text, Sequence[Text]]].
  • Similar for passwords.
  • categories, commands, and keys also can use Sequence instead of `List.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated with ... for default values and Sequence for List

- add ... for params with defaults
- use Sequence instead of List
- use Any for return value of Union
@cy cy force-pushed the redis/acl-stubs branch from 51fccae to 5a74988 Compare October 23, 2020 02:25
@srittau srittau merged commit b76d9e4 into python:master Oct 23, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants