Fix some commands key spec in json files (#10779)

There are some commands that has the wrong key specs.
This PR adds a key-spec related check in generate-command-code.py.
Check if the index is valid, or if there is an unused index.

The check result will look like:
```
[root]# python utils/generate-command-code.py
Processing json files...
Linking container command to subcommands...
Checking all commands...
command: RESTORE_ASKING may have unused key_spec
command: RENAME may have unused key_spec
command: PFDEBUG may have unused key_spec
command: WATCH key_specs missing flags
command: LCS arg: key2 key_spec_index error
command: RENAMENX may have unused key_spec
Error: There are errors in the commands check, please check the above logs.
```

The following commands have been fixed according to the check results:
- RESTORE ASKING: add missing arguments section (and history section)
- RENAME: newkey's key_spec_index should be 1
- PFDEBUG: add missing arguments (and change the arity from -3 to 3)
- WATCH: add missing key_specs flags: RO, like EXIST (it allow you to know the key exists, or is modified, but doesn't "leak" the data)
- LCS: key2 key_spec_index error, there is only one key-spec
- RENAMENX: newkey's key_spec_index should be 1
This commit is contained in:
Binbin 2022-05-27 17:58:00 +08:00 committed by GitHub
parent 6f7c1a8ce6
commit 2a099d49d4
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
10 changed files with 181 additions and 19 deletions

View File

@ -1595,7 +1595,7 @@ NULL
/* RENAME argument table */
struct redisCommandArg RENAME_Args[] = {
{"key",ARG_TYPE_KEY,0,NULL,NULL,NULL,CMD_ARG_NONE},
{"newkey",ARG_TYPE_KEY,0,NULL,NULL,NULL,CMD_ARG_NONE},
{"newkey",ARG_TYPE_KEY,1,NULL,NULL,NULL,CMD_ARG_NONE},
{0}
};
@ -1613,7 +1613,7 @@ commandHistory RENAMENX_History[] = {
/* RENAMENX argument table */
struct redisCommandArg RENAMENX_Args[] = {
{"key",ARG_TYPE_KEY,0,NULL,NULL,NULL,CMD_ARG_NONE},
{"newkey",ARG_TYPE_KEY,0,NULL,NULL,NULL,CMD_ARG_NONE},
{"newkey",ARG_TYPE_KEY,1,NULL,NULL,NULL,CMD_ARG_NONE},
{0}
};
@ -2602,6 +2602,13 @@ struct redisCommandArg PFCOUNT_Args[] = {
/* PFDEBUG tips */
#define PFDEBUG_tips NULL
/* PFDEBUG argument table */
struct redisCommandArg PFDEBUG_Args[] = {
{"subcommand",ARG_TYPE_STRING,-1,NULL,NULL,NULL,CMD_ARG_NONE},
{"key",ARG_TYPE_KEY,0,NULL,NULL,NULL,CMD_ARG_NONE},
{0}
};
/********** PFMERGE ********************/
/* PFMERGE history */
@ -4939,11 +4946,28 @@ struct redisCommandArg REPLICAOF_Args[] = {
/********** RESTORE_ASKING ********************/
/* RESTORE_ASKING history */
#define RESTORE_ASKING_History NULL
commandHistory RESTORE_ASKING_History[] = {
{"3.0.0","Added the `REPLACE` modifier."},
{"5.0.0","Added the `ABSTTL` modifier."},
{"5.0.0","Added the `IDLETIME` and `FREQ` options."},
{0}
};
/* RESTORE_ASKING tips */
#define RESTORE_ASKING_tips NULL
/* RESTORE_ASKING argument table */
struct redisCommandArg RESTORE_ASKING_Args[] = {
{"key",ARG_TYPE_KEY,0,NULL,NULL,NULL,CMD_ARG_NONE},
{"ttl",ARG_TYPE_INTEGER,-1,NULL,NULL,NULL,CMD_ARG_NONE},
{"serialized-value",ARG_TYPE_STRING,-1,NULL,NULL,NULL,CMD_ARG_NONE},
{"replace",ARG_TYPE_PURE_TOKEN,-1,"REPLACE",NULL,"3.0.0",CMD_ARG_OPTIONAL},
{"absttl",ARG_TYPE_PURE_TOKEN,-1,"ABSTTL",NULL,"5.0.0",CMD_ARG_OPTIONAL},
{"seconds",ARG_TYPE_INTEGER,-1,"IDLETIME",NULL,"5.0.0",CMD_ARG_OPTIONAL},
{"frequency",ARG_TYPE_INTEGER,-1,"FREQ",NULL,"5.0.0",CMD_ARG_OPTIONAL},
{0}
};
/********** ROLE ********************/
/* ROLE history */
@ -6877,7 +6901,7 @@ struct redisCommandArg INCRBYFLOAT_Args[] = {
/* LCS argument table */
struct redisCommandArg LCS_Args[] = {
{"key1",ARG_TYPE_KEY,0,NULL,NULL,NULL,CMD_ARG_NONE},
{"key2",ARG_TYPE_KEY,1,NULL,NULL,NULL,CMD_ARG_NONE},
{"key2",ARG_TYPE_KEY,0,NULL,NULL,NULL,CMD_ARG_NONE},
{"len",ARG_TYPE_PURE_TOKEN,-1,"LEN",NULL,NULL,CMD_ARG_OPTIONAL},
{"idx",ARG_TYPE_PURE_TOKEN,-1,"IDX",NULL,NULL,CMD_ARG_OPTIONAL},
{"len",ARG_TYPE_INTEGER,-1,"MINMATCHLEN",NULL,NULL,CMD_ARG_OPTIONAL},
@ -7216,7 +7240,7 @@ struct redisCommand redisCommandTable[] = {
/* hyperloglog */
{"pfadd","Adds the specified elements to the specified HyperLogLog.","O(1) to add every element.","2.8.9",CMD_DOC_NONE,NULL,NULL,COMMAND_GROUP_HYPERLOGLOG,PFADD_History,PFADD_tips,pfaddCommand,-2,CMD_WRITE|CMD_DENYOOM|CMD_FAST,ACL_CATEGORY_HYPERLOGLOG,{{NULL,CMD_KEY_RW|CMD_KEY_INSERT,KSPEC_BS_INDEX,.bs.index={1},KSPEC_FK_RANGE,.fk.range={0,1,0}}},.args=PFADD_Args},
{"pfcount","Return the approximated cardinality of the set(s) observed by the HyperLogLog at key(s).","O(1) with a very small average constant time when called with a single key. O(N) with N being the number of keys, and much bigger constant times, when called with multiple keys.","2.8.9",CMD_DOC_NONE,NULL,NULL,COMMAND_GROUP_HYPERLOGLOG,PFCOUNT_History,PFCOUNT_tips,pfcountCommand,-2,CMD_READONLY|CMD_MAY_REPLICATE,ACL_CATEGORY_HYPERLOGLOG,{{"RW because it may change the internal representation of the key, and propagate to replicas",CMD_KEY_RW|CMD_KEY_ACCESS,KSPEC_BS_INDEX,.bs.index={1},KSPEC_FK_RANGE,.fk.range={-1,1,0}}},.args=PFCOUNT_Args},
{"pfdebug","Internal commands for debugging HyperLogLog values","N/A","2.8.9",CMD_DOC_SYSCMD,NULL,NULL,COMMAND_GROUP_HYPERLOGLOG,PFDEBUG_History,PFDEBUG_tips,pfdebugCommand,-3,CMD_WRITE|CMD_DENYOOM|CMD_ADMIN,ACL_CATEGORY_HYPERLOGLOG,{{NULL,CMD_KEY_RW|CMD_KEY_ACCESS,KSPEC_BS_INDEX,.bs.index={2},KSPEC_FK_RANGE,.fk.range={0,1,0}}}},
{"pfdebug","Internal commands for debugging HyperLogLog values","N/A","2.8.9",CMD_DOC_SYSCMD,NULL,NULL,COMMAND_GROUP_HYPERLOGLOG,PFDEBUG_History,PFDEBUG_tips,pfdebugCommand,3,CMD_WRITE|CMD_DENYOOM|CMD_ADMIN,ACL_CATEGORY_HYPERLOGLOG,{{NULL,CMD_KEY_RW|CMD_KEY_ACCESS,KSPEC_BS_INDEX,.bs.index={2},KSPEC_FK_RANGE,.fk.range={0,1,0}}},.args=PFDEBUG_Args},
{"pfmerge","Merge N different HyperLogLogs into a single one.","O(N) to merge N HyperLogLogs, but with high constant times.","2.8.9",CMD_DOC_NONE,NULL,NULL,COMMAND_GROUP_HYPERLOGLOG,PFMERGE_History,PFMERGE_tips,pfmergeCommand,-2,CMD_WRITE|CMD_DENYOOM,ACL_CATEGORY_HYPERLOGLOG,{{NULL,CMD_KEY_RW|CMD_KEY_ACCESS|CMD_KEY_INSERT,KSPEC_BS_INDEX,.bs.index={1},KSPEC_FK_RANGE,.fk.range={0,1,0}},{NULL,CMD_KEY_RO|CMD_KEY_ACCESS,KSPEC_BS_INDEX,.bs.index={2},KSPEC_FK_RANGE,.fk.range={-1,1,0}}},.args=PFMERGE_Args},
{"pfselftest","An internal command for testing HyperLogLog values","N/A","2.8.9",CMD_DOC_SYSCMD,NULL,NULL,COMMAND_GROUP_HYPERLOGLOG,PFSELFTEST_History,PFSELFTEST_tips,pfselftestCommand,1,CMD_ADMIN,ACL_CATEGORY_HYPERLOGLOG},
/* list */
@ -7284,7 +7308,7 @@ struct redisCommand redisCommandTable[] = {
{"psync","Internal command used for replication",NULL,"2.8.0",CMD_DOC_NONE,NULL,NULL,COMMAND_GROUP_SERVER,PSYNC_History,PSYNC_tips,syncCommand,-3,CMD_NO_ASYNC_LOADING|CMD_ADMIN|CMD_NO_MULTI|CMD_NOSCRIPT,0,.args=PSYNC_Args},
{"replconf","An internal command for configuring the replication stream","O(1)","3.0.0",CMD_DOC_SYSCMD,NULL,NULL,COMMAND_GROUP_SERVER,REPLCONF_History,REPLCONF_tips,replconfCommand,-1,CMD_ADMIN|CMD_NOSCRIPT|CMD_LOADING|CMD_STALE|CMD_ALLOW_BUSY,0},
{"replicaof","Make the server a replica of another instance, or promote it as master.","O(1)","5.0.0",CMD_DOC_NONE,NULL,NULL,COMMAND_GROUP_SERVER,REPLICAOF_History,REPLICAOF_tips,replicaofCommand,3,CMD_NO_ASYNC_LOADING|CMD_ADMIN|CMD_NOSCRIPT|CMD_STALE,0,.args=REPLICAOF_Args},
{"restore-asking","An internal command for migrating keys in a cluster","O(1) to create the new key and additional O(N*M) to reconstruct the serialized value, where N is the number of Redis objects composing the value and M their average size. For small string values the time complexity is thus O(1)+O(1*M) where M is small, so simply O(1). However for sorted set values the complexity is O(N*M*log(N)) because inserting values into sorted sets is O(log(N)).","3.0.0",CMD_DOC_SYSCMD,NULL,NULL,COMMAND_GROUP_SERVER,RESTORE_ASKING_History,RESTORE_ASKING_tips,restoreCommand,-4,CMD_WRITE|CMD_DENYOOM|CMD_ASKING,ACL_CATEGORY_KEYSPACE|ACL_CATEGORY_DANGEROUS,{{NULL,CMD_KEY_OW|CMD_KEY_UPDATE,KSPEC_BS_INDEX,.bs.index={1},KSPEC_FK_RANGE,.fk.range={0,1,0}}}},
{"restore-asking","An internal command for migrating keys in a cluster","O(1) to create the new key and additional O(N*M) to reconstruct the serialized value, where N is the number of Redis objects composing the value and M their average size. For small string values the time complexity is thus O(1)+O(1*M) where M is small, so simply O(1). However for sorted set values the complexity is O(N*M*log(N)) because inserting values into sorted sets is O(log(N)).","3.0.0",CMD_DOC_SYSCMD,NULL,NULL,COMMAND_GROUP_SERVER,RESTORE_ASKING_History,RESTORE_ASKING_tips,restoreCommand,-4,CMD_WRITE|CMD_DENYOOM|CMD_ASKING,ACL_CATEGORY_KEYSPACE|ACL_CATEGORY_DANGEROUS,{{NULL,CMD_KEY_OW|CMD_KEY_UPDATE,KSPEC_BS_INDEX,.bs.index={1},KSPEC_FK_RANGE,.fk.range={0,1,0}}},.args=RESTORE_ASKING_Args},
{"role","Return the role of the instance in the context of replication","O(1)","2.8.12",CMD_DOC_NONE,NULL,NULL,COMMAND_GROUP_SERVER,ROLE_History,ROLE_tips,roleCommand,1,CMD_NOSCRIPT|CMD_LOADING|CMD_STALE|CMD_FAST|CMD_SENTINEL,ACL_CATEGORY_ADMIN|ACL_CATEGORY_DANGEROUS},
{"save","Synchronously save the dataset to disk","O(N) where N is the total number of keys in all databases","1.0.0",CMD_DOC_NONE,NULL,NULL,COMMAND_GROUP_SERVER,SAVE_History,SAVE_tips,saveCommand,1,CMD_NO_ASYNC_LOADING|CMD_ADMIN|CMD_NOSCRIPT|CMD_NO_MULTI,0},
{"shutdown","Synchronously save the dataset to disk and then shut down the server","O(N) when saving, where N is the total number of keys in all databases when saving data, otherwise O(1)","1.0.0",CMD_DOC_NONE,NULL,NULL,COMMAND_GROUP_SERVER,SHUTDOWN_History,SHUTDOWN_tips,shutdownCommand,-1,CMD_ADMIN|CMD_NOSCRIPT|CMD_LOADING|CMD_STALE|CMD_NO_MULTI|CMD_SENTINEL|CMD_ALLOW_BUSY,0,.args=SHUTDOWN_Args},
@ -7391,6 +7415,6 @@ struct redisCommand redisCommandTable[] = {
{"exec","Execute all commands issued after MULTI","Depends on commands in the transaction","1.2.0",CMD_DOC_NONE,NULL,NULL,COMMAND_GROUP_TRANSACTIONS,EXEC_History,EXEC_tips,execCommand,1,CMD_NOSCRIPT|CMD_LOADING|CMD_STALE|CMD_SKIP_SLOWLOG,ACL_CATEGORY_TRANSACTION},
{"multi","Mark the start of a transaction block","O(1)","1.2.0",CMD_DOC_NONE,NULL,NULL,COMMAND_GROUP_TRANSACTIONS,MULTI_History,MULTI_tips,multiCommand,1,CMD_NOSCRIPT|CMD_LOADING|CMD_STALE|CMD_FAST|CMD_ALLOW_BUSY,ACL_CATEGORY_TRANSACTION},
{"unwatch","Forget about all watched keys","O(1)","2.2.0",CMD_DOC_NONE,NULL,NULL,COMMAND_GROUP_TRANSACTIONS,UNWATCH_History,UNWATCH_tips,unwatchCommand,1,CMD_NOSCRIPT|CMD_LOADING|CMD_STALE|CMD_FAST|CMD_ALLOW_BUSY,ACL_CATEGORY_TRANSACTION},
{"watch","Watch the given keys to determine execution of the MULTI/EXEC block","O(1) for every key.","2.2.0",CMD_DOC_NONE,NULL,NULL,COMMAND_GROUP_TRANSACTIONS,WATCH_History,WATCH_tips,watchCommand,-2,CMD_NOSCRIPT|CMD_LOADING|CMD_STALE|CMD_FAST|CMD_ALLOW_BUSY,ACL_CATEGORY_TRANSACTION,{{NULL,0,KSPEC_BS_INDEX,.bs.index={1},KSPEC_FK_RANGE,.fk.range={-1,1,0}}},.args=WATCH_Args},
{"watch","Watch the given keys to determine execution of the MULTI/EXEC block","O(1) for every key.","2.2.0",CMD_DOC_NONE,NULL,NULL,COMMAND_GROUP_TRANSACTIONS,WATCH_History,WATCH_tips,watchCommand,-2,CMD_NOSCRIPT|CMD_LOADING|CMD_STALE|CMD_FAST|CMD_ALLOW_BUSY,ACL_CATEGORY_TRANSACTION,{{NULL,CMD_KEY_RO,KSPEC_BS_INDEX,.bs.index={1},KSPEC_FK_RANGE,.fk.range={-1,1,0}}},.args=WATCH_Args},
{0}
};

View File

@ -41,7 +41,7 @@
{
"name": "key2",
"type": "key",
"key_spec_index": 1
"key_spec_index": 0
},
{
"name": "len",

View File

@ -4,7 +4,7 @@
"complexity": "N/A",
"group": "hyperloglog",
"since": "2.8.9",
"arity": -3,
"arity": 3,
"function": "pfdebugCommand",
"doc_flags": [
"SYSCMD"
@ -36,6 +36,17 @@
}
}
}
],
"arguments": [
{
"name": "subcommand",
"type": "string"
},
{
"name": "key",
"type": "key",
"key_spec_index": 0
}
]
}
}

View File

@ -62,7 +62,7 @@
{
"name": "newkey",
"type": "key",
"key_spec_index": 0
"key_spec_index": 1
}
]
}

View File

@ -67,7 +67,7 @@
{
"name": "newkey",
"type": "key",
"key_spec_index": 0
"key_spec_index": 1
}
]
}

View File

@ -6,6 +6,20 @@
"since": "3.0.0",
"arity": -4,
"function": "restoreCommand",
"history": [
[
"3.0.0",
"Added the `REPLACE` modifier."
],
[
"5.0.0",
"Added the `ABSTTL` modifier."
],
[
"5.0.0",
"Added the `IDLETIME` and `FREQ` options."
]
],
"doc_flags": [
"SYSCMD"
],
@ -37,6 +51,49 @@
}
}
}
],
"arguments": [
{
"name": "key",
"type": "key",
"key_spec_index": 0
},
{
"name": "ttl",
"type": "integer"
},
{
"name": "serialized-value",
"type": "string"
},
{
"name": "replace",
"token": "REPLACE",
"type": "pure-token",
"optional": true,
"since": "3.0.0"
},
{
"name": "absttl",
"token": "ABSTTL",
"type": "pure-token",
"optional": true,
"since": "5.0.0"
},
{
"token": "IDLETIME",
"name": "seconds",
"type": "integer",
"optional": true,
"since": "5.0.0"
},
{
"token": "FREQ",
"name": "frequency",
"type": "integer",
"optional": true,
"since": "5.0.0"
}
]
}
}

View File

@ -18,6 +18,9 @@
],
"key_specs": [
{
"flags": [
"RO"
],
"begin_search": {
"index": {
"pos": 1

View File

@ -1495,8 +1495,13 @@ cleanup:
if (o) decrRefCount(o);
}
/* PFDEBUG <subcommand> <key> ... args ...
* Different debugging related operations about the HLL implementation. */
/* Different debugging related operations about the HLL implementation.
*
* PFDEBUG GETREG <key>
* PFDEBUG DECODE <key>
* PFDEBUG ENCODING <key>
* PFDEBUG TODENSE <key>
*/
void pfdebugCommand(client *c) {
char *cmd = c->argv[1]->ptr;
struct hllhdr *hdr;

View File

@ -1,8 +1,7 @@
#!/usr/bin/env python3
import os
import glob
import json
import os
ARG_TYPES = {
"string": "ARG_TYPE_STRING",
@ -68,6 +67,55 @@ def get_optional_desc_string(desc, field, force_uppercase=False):
return ret.replace("\n", "\\n")
def check_command_args_key_specs(args, command_key_specs_index_set, command_arg_key_specs_index_set):
if not args:
return True
for arg in args:
if arg.key_spec_index is not None:
assert isinstance(arg.key_spec_index, int)
if arg.key_spec_index not in command_key_specs_index_set:
print("command: %s arg: %s key_spec_index error" % (command.fullname(), arg.name))
return False
command_arg_key_specs_index_set.add(arg.key_spec_index)
if not check_command_args_key_specs(arg.subargs, command_key_specs_index_set, command_arg_key_specs_index_set):
return False
return True
def check_command_key_specs(command):
if not command.key_specs:
return True
assert isinstance(command.key_specs, list)
for cmd_key_spec in command.key_specs:
if "flags" not in cmd_key_spec:
print("command: %s key_specs missing flags" % command.fullname())
return False
if "NOT_KEY" in cmd_key_spec["flags"]:
# Like SUNSUBSCRIBE / SPUBLISH / SSUBSCRIBE
return True
command_key_specs_index_set = set(range(len(command.key_specs)))
command_arg_key_specs_index_set = set()
# Collect key_spec used for each arg, including arg.subarg
if not check_command_args_key_specs(command.args, command_key_specs_index_set, command_arg_key_specs_index_set):
return False
# Check if we have key_specs not used
if command_key_specs_index_set != command_arg_key_specs_index_set:
print("command: %s may have unused key_spec" % command.fullname())
return False
return True
# Globals
subcommands = {} # container_name -> dict(subcommand_name -> Subcommand) - Only subcommands
commands = {} # command_name -> Command - Only commands
@ -132,6 +180,7 @@ class Argument(object):
self.desc = desc
self.name = self.desc["name"].lower()
self.type = self.desc["type"]
self.key_spec_index = self.desc.get("key_spec_index", None)
self.parent_name = parent_name
self.subargs = []
self.subargs_name = None
@ -200,6 +249,7 @@ class Command(object):
self.name = name.upper()
self.desc = desc
self.group = self.desc["group"]
self.key_specs = self.desc.get("key_specs", [])
self.subcommands = []
self.args = []
for arg_desc in self.desc.get("arguments", []):
@ -271,7 +321,7 @@ class Command(object):
def _key_specs_code():
s = ""
for spec in self.desc.get("key_specs", []):
for spec in self.key_specs:
s += "{%s}," % KeySpec(spec).struct_code()
return s[:-1]
@ -398,6 +448,17 @@ for command in commands.values():
subcommand.group = command.group
command.subcommands.append(subcommand)
check_command_error_counter = 0 # An error counter is used to count errors in command checking.
print("Checking all commands...")
for command in commands.values():
if not check_command_key_specs(command):
check_command_error_counter += 1
if check_command_error_counter != 0:
print("Error: There are errors in the commands check, please check the above logs.")
exit(1)
print("Generating commands.c...")
with open("%s/commands.c" % srcdir, "w") as f:
f.write("/* Automatically generated by %s, do not edit. */\n\n" % os.path.basename(__file__))

View File

@ -1,10 +1,10 @@
#!/usr/bin/env python3
import argparse
import json
import os
import subprocess
from collections import OrderedDict
from sys import argv, stdin
import os
from sys import argv
def convert_flags_to_boolean_dict(flags):
@ -118,7 +118,8 @@ if __name__ == '__main__':
stdout, stderr = p.communicate()
commands = json.loads(stdout)
p = subprocess.Popen([args.cli, '-h', args.host, '-p', str(args.port), '--json', 'command', 'docs'], stdout=subprocess.PIPE)
p = subprocess.Popen([args.cli, '-h', args.host, '-p', str(args.port), '--json', 'command', 'docs'],
stdout=subprocess.PIPE)
stdout, stderr = p.communicate()
docs = json.loads(stdout)