First of all thank you for the commands.json format. I am currently working on an API code generator for the redis-rs rust crate and the format is a pleasure to work with. But for some scenarios there could be made some improvements. I list them all here for a initial discussion about them. If you consider them valid I can create follow up issues with more details and probably a full list of affected commands afterwards.
Return Types
Currently the return types are not documented in the commands.json I am not sure about the stability guarantees of the return types vs. arguments, but I guess it would be nice for type safety if the commands.json could also specify the return types if they consist of a fixed set of fields (e.g. SLOWLOG GET) We currently just pass the task the response in a fallible way to the user of the API. This might be a hard task as some commands have different return types based on the passed arguments e.g CLIENT KILL.
Cursors
Some commands (e.g. SCAN) return a cursor based iterator. Currently this is not visible in the commands.json. There is only a hint for nondeterministic_output which has a different meaning. Would it possible to add a hint to all commands that return a cursor? This would allow us in the code generation to change the return type to a Iterator based one.
Mutual exclusive arguments
There are some commands like CLIENT KILL that have a sole list of optional arguments. The docs for CLIENT KILL seem to explain that some of the arguments are mutually exclusive.
There is the old way using just ip:port as the first argument then there is also the new way of using the ADDR token. The docs mention that you can use multiple filters, but I am not sure how that will interact with CLIENT KILL ip:port ADDR ip:port. Are you allowed to specify the, basically same, arguments?
Also it seems odd to me that LADDR and ADDR use the same argument name. In principle there is difference between these arguments and that should be reflected in the argument name to be able to generate function signatures with meaningful argument names. Can we improve that in some way? I guess it will be hard to rename the argument in a minor release, thus I see this as a long term goal. For the meantime we could maintain a overwrite list ourselves.
Comment From: itamarhaber
Hi @valkum
Thank you for working on improving the ecosystem and Redis itself! I'll move this ticket to the main repository because as of v7 the commands.json file is generated by the server's runtime.
WRT return types: we have this effort in progress, it would be amazing to get your feedback on it - https://github.com/redis/redis/issues/9845
WRT cursor hint: this can certainly be added, but how would the client benefit from that? I think an explanation of the use case would help here.
WRT mutually exclusive: generally speaking, mutual exclusiveness is expressed as oneof, so I would like to identify the places where we missed that and fix.
WRT CLIENT KILL: this feels like a separate issue. In any case
Are you allowed to specify the, basically same, arguments?
IIRC (i.e. w/o looking at the implementation), yes, args can be repeated.
same argument name
The arguments' names are for display purposes only
Can we improve that in some way?
PR to https://github.com/redis/redis/blob/unstable/src/commands/client-kill.json :)
Comment From: guybe7
hi @valkum
about CLIENT KILL ip:port ADDR ip:port - you are right, the json file is incorrect. it should be oneOf(old_ip:addr_format, new_format_with_filter_value_pairs)
do you want to PR?
Comment From: valkum
Thanks for your answers. I can prepare a PR for CLIENT KILL and have a look at your work for return types early next week.
Regarding the cursors: with cursors you need to implement a different behavior than for other commands if you want to return a full collection or an iterator to the user of the API. Redis-rs currently has handwritten implementations that return iterators for commands that return a cursor but it would be really handy if we could generate these as well to ease additions of new commands in the future.
Comment From: oranagra
I'd like to join this party and mention that seeing this issue really cheers me up. we've invested a ton of time designing that new json system, starting with Redis as the SSOT of its commands and key-specs even before it, followed by Treat subcommands as commands. and as mentioned the next goal is the command reply schema.
we had a clear vision of what we want to enable, but we were not confident anyone will really ever use it (existing client libraries seem happy with what they have), so the fact you find it useful and provide feedback is superb, and the fact you can evaluate the decisions we made for the reply schema before it goes live, would really be priceless.
one note though, from the origin of this issue, it looks like you were using the json file from the redis-doc repo, i'd argue that we didn't intend for that.
ideally clients should interrogate the live redis they speak to (using COMMAND).
since your case is involved with code generation, you can do a similar thing to what we do to generate the redis-doc json file (which is basically doing reids-cli --json COMMAND see the utils/generate-commands-json.py file that does that for redis-doc.
p.s. we certainly didn't mean for people to directly use the individual json files in the src/commands folder.
Comment From: valkum
I prepared a PR for the CLIENT KILL docs. Thanks for your feedback @oranagra. We definitely can change the way we get the commands.json in our build step. But for the sake of reproducibility we probably store a copy in the client lib repo itself.
I got sick last weekend and am not 100% fit again so I hope I can have a look at https://github.com/redis/redis/issues/9845 and https://github.com/redis/redis/pull/10273 on the upcoming weekend or early next week.
Comment From: itamarhaber
Take care of yourself and get well soon @valkum 💊
Comment From: valkum
I left you some feedback on #10273. I assume this will need some time before it lands.
In the meantime I would like to hear your opinions on adding a hint for cursor based iterator responses. This would only touch a hand full of commands and could be a simple new hint named cursor. This also allows to improve code generation for libraries that want to stick with RESP2 as #10273 only supports RESP3.
Comment From: oranagra
my initial gut feeling about it is that we don't want a cursor hint. the reality is that all that introspection and metadata can only bring you some distance, but not the whole way to the target. some cases will always have to have some hard coded logic set to them. that said, i'm still open to let the discussion proceed and change my mind.