Auto-generate the command table from JSON files

[guybe7]

This is the final fix of #9359, except #9845, and #9876, and #9944 which will be handled separately.

Delete the hardcoded command table and replace it with an auto-generated table, based
on a JSON file that describes the commands (each command must have a JSON file).

These JSON files are the SSOT of everything there is to know about Redis commands,
and it is reflected fully in COMMAND INFO.

These JSON files are used to generate commands.c (using a python script), which is then committed to the repo and compiled.

The purpose is:

• Clients and proxies will be able to get much more info from redis, instead of relying on hard coded logic.
• drop the dependency between Redis-user and the commands.json in redis-doc.
• delete help.h and have redis-cli learn everything it needs to know just by issuing COMMAND (will be done in a separate PR)
• redis.io should stop using commands.json and learn everything from Redis (ultimately one of the release artifacts should be a large JSON, containing all the information about all of the commands, which will be generated from COMMAND's reply)
• the byproduct of this is:
  • module commands will be able to provide that info and possibly be more of a first-class citizens
  • in theory, one may be able to generate a redis client library for a strictly typed language, by using this info.

Interface changes

COMMAND INFO's reply change (and arg-less COMMAND)

Before this commit the reply at index 7 contained the key-specs list
and reply at index 8 contained the sub-commands list (Both unreleased).
Now, reply at index 7 is a map of:
NOTE, many of these where later moved to COMMAND DOCS, see: #10056)

• summary - short command description
• since - debut version
• group - command group
• complexity - complexity string
• doc-flags - flags used for documentation (e.g. "deprecated")
• deprecated-since - if deprecated, from which version?
• replaced-by - if deprecated, which command replaced it?
• history - a list of (version, what-changed) tuples
• hints - a list of strings, meant to provide hints for clients/proxies. see Define and document command tips #9876
• arguments - an array of arguments. each element is a map, with the possibility of nesting (sub-arguments)
• key-specs - an array of keys specs (already in unstable, just changed location)
• subcommands - a list of sub-commands (already in unstable, just changed location)
• reply-schema - will be added in the future (see Commands' reply schema #9845)

more details on these can be found in redis/redis-doc#1697

only the first three fields are mandatory

API changes (unreleased API obviously) - now they take RedisModuleCommand opaque pointer instead of looking up the command by name.

Note: these where later moved, see: #10108)

• RM_CreateSubcommand
• RM_AddCommandKeySpec
• RM_SetCommandKeySpecBeginSearchIndex
• RM_SetCommandKeySpecBeginSearchKeyword
• RM_SetCommandKeySpecFindKeysRange
• RM_SetCommandKeySpecFindKeysKeynum

Currently, we did not add module API to provide additional information about their commands because we couldn't agree on how the API should look like, see #9944.

Somehow related changes

1. Literals should be in uppercase while placeholder in lowercase. Now all the GEO* command will be documented with M|KM|FT|MI and can take both lowercase and uppercase

Unrelated changes

1. Bugfix: no_madaory_keys was absent in COMMAND's reply
2. expose CMD_MODULE as "module" via COMMAND
3. have a dedicated uint64 for ACL categories (instead of having them in the same uint64 as command flags)

TODO

• Some information is missing from the JSON files (mostly summary and complexity info) - will go over them manually
• Open a GH issue: define and document general hints for RM_SetCommandHints - Define and document command tips #9876
• Create a PR for redis-doc, describing all the changes in this PR (namely the changes in COMMAND) - Refresh COMMAND, add topics: command-args, key-specs redis-doc#1697
• Add test: per command, all key-specs are referred to, and every key arg reference appears at least once in the key-specs array - tested manually, I don't see a value in writing an automatic test for this.
• Write the argument array where missing:
  'fcall',
  'fcall_ro',
  'georadius_ro',
  'georadiusbymember_ro',
  'substr',
  'xsetid'
  internal command that take arguments (but we will not bother to add them to the jsons)
  'pfdebug',
  'replconf',
  'restore-asking',
• Generate the arg array of COMMAND LIST
• Come up with a way to prevent people from using the jsons directly, and force them to use COMMAND: all flag and acl cats will be uppercase in the jsons, the script to generate commands.c will lower them
• Create jsons for new function command(s) + the new form of CONFIG SET
• fix moduleAPI for arguments ("value" is gone from c file but not from h. fix test module)
• Yossi's comments
• add CMD_MODULE to COMMAND INFO

Open issues:

Argument's "value" and "name"

how should we call what is now called “value” (of an argument)?
it is one of the following:

• a string to display when rendering the syntax
• an array of sub-arguments

also, do we want to get rid of “name” ?it is indeed redundant (args are ordered so we can identify one by its nested index)

update: this is what we decided:

1. “name” is mandatory. if the arg doesn’t have subargs this is the string we use for rendering the syntax
2. get rid of “value”. if the command has subargs they will appear under “arguments”

basically, the players in syntax rendering are “name” (if arg doesn’t have subargs) , “token” (if exists), and “arguments” (if exists)

Usage of versions in module API

When a module sets "since", should it be the debut Redis version or the module version?

update: for now, the code suggests it's the module version

Optional args interchangeability

the rule is that all optional args, at the same nesting level, may interchange as long as there isn’t a non-optional arg between them
e.g.

SORT key [BY pattern] [LIMIT offset count] [GET pattern [GET pattern ...]] [ASC|DESC] [ALPHA] [STORE destination]

all optionals can interchange

XPENDING key group [[IDLE min-idle-time] start end count [consumer]]

IDLE min-idle-time and consumer can’t interchange because there’s a non-optional between them

the only exception is MIGRATE, which actually has two forms:
either

MIGRATE host port key destination-db timeout [COPY] [REPLACE] [AUTH password] [AUTH2 username password]

or

MIGRATE host port "" destination-db timeout [COPY] [REPLACE] [AUTH password] [AUTH2 username password] KEYS key [key ...]

we have two options:

1. say f*ck it, it’s only one exception, the cli hints will be wrong
2. rearrange the command json file so that it’ll look like MIGRATE host port (key destination-db timeout [COPY] [REPLACE] [AUTH password] [AUTH2 username password] | "" destination-db timeout [COPY] [REPLACE] [AUTH password] [AUTH2 username password] KEYS key [key ...])
3. add an ad-hoc feature in the JSON that says "this arg is optional, but only if (another arg) is not (value)"

update: for now we've decided to go with (1)

[guybe7]

comments from Oran:

1. remove both help.h and commands.c from makefile. document somewhere how to re-gen commands.c
2. in the command json files, i suggest to put the arity, acl, key_specs and all other short metadata before the arguments (put the short ones first and the long ones last) (@itamarhaber FYI - it's better than alphabetical ordering - basically have "arguments" as the last field, and sort alphabetically the N-1 first fields)
3. i'd rather use "resp3" over just "3" in the json files.
4. re-arrange return_types: it should be a list oj objects. each object will have "summary", "type" (which can be either a string or an object of RESP2 and RESP3) and "contant_value" (optional, only for simple string)
5. delete return_summary from jsons
6. split the doc in commands.c - the first half should be next to struct redisCommand.
7. try to avoid using the preprocessor in commands.c
8. turn all non-freetext fields into enum (command.group, arg.type and return_type.type) we will no longer need redisCommandValueArgType (see Auto-generate the command table from JSON files #9656 (comment))

[guybe7]

(note to self: pushed fixes to all above, except (7))

[yossigo]

@guybe7 I'm addressing the open issues at the top comment:

another issue is whether we want to provide a full-blown description of the return-type or just the immediate one? e.g. ZPOP returns an array of (member, score) tuples in RESP3 and a flat-map array for RESP2. in the current design we will just state that it returns an "array" for both, is that enough?

Because the differences between RESP2 and RESP3 are only semantic, I tend to say it probably makes sense to have a single returns element (and description) that refers to both versions, and branch only the type part as you listed in the example. If this proves to be not good enough, I'd just consider the entire returns element version specific and include an optional versions field that lists applicable protocol versions (with default being versions: [2,3]). I think we need to choose one of these two approaches but not both.

we have a few open questions about pure tokens:
should they have a "type"? usually, type is used to validate some free-test user input
is the token itself be under "token" or under "value"? if it's under "value" we can declare it as a mandatory attribute

I think there can be a type: flag that indicates there are no user supplied values, and the specified value indicates that flag was set.

[jhelbaum]

@guybe7

I've started to work on integrating these changes into redis-cli.c. So far I see at least two issues:

1. static char *commandGroups[] is missing - this is used for outputting command help.
2. struct redisCommand is defined in server.h. This requires redis-cli.c to include server.h for the struct definition. Aside from the massive header coupling this introduces (server.h desperately needs to be broken up regardless), it's incompatible with redis-cli.c, which uses the hiredis implementation of sds.h instead of the one included from server.h. I'm not familiar with the differences between these implementations so I don't know how important this is. But it seems to me a poor design choice to require a client library to include server.h. The command-related definitions should be broken out into a separate header file.

[enjoy-binbin]

                    {
                        "name": "unix-time",
                        "type": "integer",
                        "token": "EXAT"
                    },
                    {
                        "name": "unix-time",
                        "type": "integer",
                        "token": "PXAT"
                    },

should we change it to?

                    {
                        "name": "timestamp",
                        "type": "unix-time",
                        "token": "EXAT"
                    },
                    {
                        "name": "milliseconds-timestamp",
                        "type": "unix-time",
                        "token": "PXAT"
                    },

i see in set.json

                    {
                        "name": "unix-time-seconds",
                        "type": "unix-time",
                        "token": "EXAT",
                        "since": "6.2.0"
                    },
                    {
                        "name": "unix-time-milliseconds",
                        "type": "unix-time",
                        "token": "PXAT",
                        "since": "6.2.0"
                    },

in pexpireat.json or expireat.json

            {
                "name": "milliseconds-timestamp",
                "type": "unix-time"
            },

            {
                "name": "timestamp",
                "type": "unix-time"
            },

make a demo commit: 6d1b509

[oranagra]

@enjoy-binbin if you find a problem or an inconsistency on an already merged PR, please just post a PR (rather than a comment in the closed one).

p.s. i'm not sure if milliseconds timestamp could be technically considered a "unix-time" but i see it already is in some fields, so let's at least be consistent.

[yangbodong22011]

Suggested change

	"key_spec_index": 1
	"key_spec_index": 0

@guybe7 Please confirm it should be 0 here?

[oranagra]

@yangbodong22011 you're right. there's only one key-spec, so it can't be at index 1.
please make a PR.

p.s. how did you find that? maybe we can benefit from some mechanism that validates there.

[yangbodong22011]

@oranagra Just found out while looking at the code, I haven't checked the rest of the commands.

[yangbodong22011]

please make a PR.

We might have to think of other ways to make sure other commands don't go wrong, rather than fixing one by one.

[guybe7]

thanks, @yangbodong22011 will you create a PR, or should I?

and yes, we need to figure out a better way to spot these issues... got any ideas?

[yangbodong22011]

Is it possible to check whether the index is valid in generate-command-code.py and whether the key_spec contains an unused index.

rename and renamenx also has problem. (have 2 index, but just use key_spec_index: 0)

[enjoy-binbin]

Is it possible to check whether the index is valid in generate-command-code.py and whether the key_spec contains an unused index.

i write a small ugly test script, and find out these, you guys can check on it, in case you guys forget this one

• LCS: key2 key_spec_index error

• RENAME: have unused key_spec

• RENAMENX: have unused key_spec

• WATCH: key_specs missing flags?

• SUNSUBSCRIBE / SPUBLISH / SSUBSCRIBE are NOT_KEY, i suppose they are ok

• RESTORE-ASKING: missing arguments, should it be like RESTORE?

• PFDEBUG: missing arguments, this is a debug command, so i suppose it is ok

[zuiderkwast]

@enjoy-binbin How "ugly" is your script? I think we need it in a test case or a separate CI job.

[oranagra]

@enjoy-binbin thank you for reminding us.. looks like we forgot it.
so first thing's first, let's fix them, please make a PR.

if your test script is tcl based, let's put it in the test suite.
alternatively, we could set some assertions in the C code.
or put another test in the utils folder and somehow run it in some CI or manually from time to time (not preferred)

[enjoy-binbin]

i just write some logic in generate-command-code and the check spec_key logic base on my understanding
i just wanted to see if we have any other commands that have the same problem first (we can fix it first and then try to find a way to check it)
ok, i can summery it and then let's discuss it in other thread.

can see it in #10779