Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Commands' reply schema #9845

Open
guybe7 opened this issue Nov 24, 2021 · 10 comments
Open

Commands' reply schema #9845

guybe7 opened this issue Nov 24, 2021 · 10 comments
Assignees
Labels
state:major-decision Requires core team consensus

Comments

@guybe7
Copy link
Collaborator

guybe7 commented Nov 24, 2021

Background

In #9656 we add a lot of information about Redis commands, but we are missing information about the replies

Motivation

  1. Documentation. This is the primary goal.
  2. We would like, based on the output of COMMAND, to be able to generate client code in typed languages. In order to do that, we need Redis to tell us, in detail, how each reply looks like.
  3. We would like to build a fuzzer that verifies the reply structure.

Schema

The idea is to supply some sort of schema for each reply of each command.
The schema will describe the conceptual structure of the reply (for generated clients), and, in case it differs from the actual (wire) reply, it will be mentioned.
null-reply is a special case since RESP2 has two types of null-reply (null-bulk-string and null-array), so for every command that may return null, we have to mention the wire-reply.

Example for ZSCORE:

{
  "oneOf": [
    {
      "description": "Score of the member",
      "type": "number",
      "wire": {
        "resp2": "string",
        "resp3": "double"
      }
    },
    {
      "description": "Key or member does not exist",
      "type": "null",
      "wire": {
        "resp2": "null-bulk-string",
        "resp3": "null"
      }
    }
  ]
}

Example for HGETALL:

{
  "type": "object",
  "additionalProperties": {
    "type": "string"
  }
}

Note that here we don't really have to mention the wire-reply (the type is "object" which is a map, which is the return type of the command in RESP3. the only way to represent a map in RESP2 is an array, so determining the wire-reply is implicit in that case)

Example for ZRANGE:

{
  "oneOf": [
    {
      "description": "A list of member elements",
      "type": "array",
      "resp": 2,
      "items": {
        "type": "string"
      },
      "uniqueItems": true,
      "minItems": 1
    },
    {
      "type": "array",
      "description": "Members and their scores",
      "items": {
        "type": "array",
        "items": [
          {
            "description": "Member",
            "type": "string"
          },
          {
            "description": "Score",
            "type": "number"
          }
        ],
        "additionalItems": false,
        "minItems": 1
      },
      "uniqueItems": true,
      "minItems": 1
    }
  ]
}

Here, both RESP2 and RESP3 will return an array, so no need to mention the wire-reply type, but RESP3 will return exactly the array described above, while RESP2 will return a flat array of member-score. The client needs to how to convert the actual wire-reply to the reply described in the schema (see "Conversion rules" below)

Conversion rules

In addition, we will have to supply general conversion rules in this structure:
If the reply structure is THIS but instead you got THAT, here's how to convert: ...

Examples (relevant for RESP2 only):
If the reply structure is an "object" but you got an "array" the way to convert is: {arr[i]: arr[i+1]} for each even i.
If the reply structure is a "number" but you got a "string" the way to convert is: cast the string into a double

There are more complex situations that require some thinking (For example ZRANGE WITHSCORES should return an array of (member, score) tuples, but in RESP2 it's a flat array)

Thoughts

  1. the example for ZRANGE doesn't cover the case of two identical members with different scores (e.g. [["m1",6],["m1",7]]) because uniqueItems compares (member,score) tuples and not just the member name. is there a way to solve this?
  2. the reply schema is gonna be based on a JSON scheme. the type of a string is "string" but even in RESP3 we have 2 strings: simple-string and bulk-string. do we want to reflect that somehow in the reply-schema?
@oranagra oranagra added this to Backlog in 7.0 via automation Nov 30, 2021
@oranagra oranagra moved this from Backlog to In progress in 7.0 Nov 30, 2021
@oranagra oranagra added 7.0-must-have state:major-decision Requires core team consensus labels Nov 30, 2021
oranagra pushed a commit that referenced this issue Dec 15, 2021
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:
- 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 #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 #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

- 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)

Co-authored-by: Itamar Haber <itamar@garantiadata.com>
hwware pushed a commit to hwware/redis that referenced this issue Dec 20, 2021
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:
- 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 redis#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 redis#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

- 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 redis#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)

Co-authored-by: Itamar Haber <itamar@garantiadata.com>
@guybe7
Copy link
Collaborator Author

guybe7 commented Dec 22, 2021

@yossigo @yoav-steinberg i'd appreciate your opinion here

@yoav-steinberg
Copy link
Contributor

I'm leaning towards not providing any structured reply description for RESP2. RESP2 makes the description much more complicated and also makes the generated client parsing much more complex. I think there are also cases where supporting RESP2 in the reply actually prevents us from taking advantage of RESP3 features. For example if a command can return an integer or boolean, and we want to support RESP2 we just can't do so. So there needs to be some long-term process of discarding RESP2 otherwise the advantages of RESP3 become minimal.

Having said that, assuming all we care about is RESP3, why do we even need this schema? RESP3 is structured and clients can generate a typed response just by parsing the RESP3 protocol. Another concern is that in Redis we have different reply structures based on the command arguments (like WITHSOCRES) and not just the command itself. I can't think any way a client can query the server about what to expect for a given command without also passing all the arguments. And that would be very inefficient.

@guybe7
Copy link
Collaborator Author

guybe7 commented Dec 23, 2021

@yoav-steinberg thank you

I like the idea of implementing the reply-schema just for RESP3 (it'll provide motivation to slowly deprecate RESP2).

I think we would still need this feature, even if we only provide data relevant for RESP3. IIUC the idea was that, given the reply of COMMAND, I can generate the code of a Redis client in a strongly-typed language.
Imagine we would like to use C: we would need to know the reply-schema the commands in order to generate the structures needed to hold the replies.

another aspect of this feature is the have the ability to build a testing fuzzer that validates the reply type (what we report in COMMAND vs. what we actually get)

@yoav-steinberg
Copy link
Contributor

For a typed-dynamic language (java) it'll be possible to generate the APIs without this. For a static-typed language (c,rust) then it won't, and you're right. It's just that I don't see how to provide the reply schema for a command in Redis given that the schema is dependent on the arguments.

@guybe7
Copy link
Collaborator Author

guybe7 commented Dec 23, 2021

yes, that's true.

for example, ZRANGE can return a list of strings or a list of (string, double) tuples.
in C, that would make the return type a union. maybe it's up to the caller to know which "side" of the union to use?

@zuiderkwast
Copy link
Contributor

@yoav-steinberg

assuming all we care about is RESP3, why do we even need this schema? RESP3 is structured and clients can generate a typed response just by parsing the RESP3 protocol.

If RESP3 is like JSON, then the schema is like JSON-Schema. I think we need it to generate a C struct for each object.

It's just that I don't see how to provide the reply schema for a command in Redis given that the schema is dependent on the arguments.

@guybe7

maybe it's up to the caller to know which "side" of the union to use?

This seems dangerous, but I think we need to accept "oneof", i.e. a C union in some cases and force the users to know what they're doing. In C, it's easy to mess up anyway (but in Rust it shouldn't be possible). These are corner cases anyway. AFAIK most applications only use GET, SET, DEL, HGET, HSET, DEL and a few more...

ZRANGE is really bad for this feature. We get much nicer reply schemas if we deprecate the WITHSCORES option and add a separate command ZRANGEWITHSCORES, then both commands will have simple schemas. I'm not saying we must do it now, but we can keep it in mind for later.

@yoav-steinberg
Copy link
Contributor

yoav-steinberg commented Jan 4, 2022

@zuiderkwast

but in Rust it shouldn't be possible

A note about rust: Unions in rust are unsafe, simply because there's no way to guarantee how the data will be accessed. Rust's solution for this is using an enum (with encapsulated data fields) but, again, there's no way for us to know how to match these enums to the response from the command alone (without understanding the args).

If RESP3 is like JSON, then the schema is like JSON-Schema.

I understand this, but in reality we can't match the response to a given struct. We can use unions in C/rust and fixed objects in other languages but it'll be up to the user to match the response to a give object type. Something like (will vary by language):

x = myredis.zrange(arags....)
print(type(x)) // UndefinedZrangeResponse
x = x.asWithScores() // User explicitly defines which ZrangeResponse they expect (may raise an exception)
print(type(x)) // ZrangeResponseWithScores
// Do something with x

The alternative would be just to return a dynamic JSON style structured response.

@zuiderkwast
Copy link
Contributor

@yoav-steinberg I agree the value in return schemas is very limited. Most commands just return a string, an array of strings, a map of strings to strings (HGETALL) or simply "OK". A special struct provide no benefit in these cases. It just increases complexity.

Thanks for reminding me of Rust enums (AKA algebraic datatypes AKA tagged unions). It would be theoretically possible to provide some form of rules mapping commands to different return schemas depending on arguments so that clients could use some logic to pick the right schema, but it would be too complex. (Idea: A list of conditional schema-rules like [{"condition": ["has_arg", "WITHSCORES"], "schema": ...}, {"condition": ["not_has_arg", "WITHSCORES"], "schema": ...}].) I don't think it's worth it.

Parsing RESP3 directly to structs can potentially use less memory for responses to commands like COMMAND and CLUSTER SLOTS but these are not preformance critical anyway. I think the complexity outweights the benefit.

@guybe7
Copy link
Collaborator Author

guybe7 commented Jan 11, 2022

@oranagra @yossigo and I had a meeting on the subject.
summary:

  1. we feel ok with the fact that some commands' reply structure depends on the arguments and it's the caller's responsibility to know which is the relevant one. this comes after looking at other request-reply systems like OpenAPI, where the reply schema can also be oneOf and the caller is responsible to know which schema is the relevant one.
  2. the reply schemas will describe RESP3 replies only. even though RESP3 is structured, we want to use reply schema for documentation (and possibly to create a fuzzer that validates the replies)
  3. for documentation, the description field will include an explanation about the scenario in which the reply is sent, including any relation to arguments. for example, for ZRANGE's two schemas we will need to state that one is with WITHSCORES and the other is without.
  4. for documentation, there will be another optional field "notes" in which we will add a short description about the representation in RESP2, in case it's not trivial (RESP3's ZRANGE's nested array vs. RESP2's flat array, for example)

given the above:

  1. we can generate the "return" section of all commands in redis-doc (given that "description" and "notes" are comprehensive enough)
  2. we can generate a client in a strongly typed language (but the return type could be a conceptual union and the caller needs to know which schema is relevant). see the section below for RESP2 support.
  3. we can create a fuzzer for RESP3.

notes about RESP2:

  1. we will not be able to use the fuzzer to verify RESP2 replies (we are ok with that, it's time to accept RESP3 as the future RESP)
  2. to create generated clients we will need to know how to convert the actual reply to the one expected.
    • number and boolean are always strings in RESP2 so the conversion is easy
    • objects (maps) are always a flat array in RESP2
    • others (nested array in RESP3's ZRANGE and others) will need some special handling (so the client will not be totally auto-generated)

@guybe7
Copy link
Collaborator Author

guybe7 commented Jan 11, 2022

if we only describe RESP3:

{
  "oneOf": [
    {
      "description": "Score of the member",
      "type": "number"
    },
    {
      "description": "Key or member does not exist",
      "type": "null"
    }
  ]
}

Example for HGETALL:

{
  "type": "object",
  "additionalProperties": {
    "type": "string"
  }
}

Example for ZRANGE:

{
  "oneOf": [
    {
      "description": "A list of member elements",
      "type": "array",
      "items": {
        "type": "string"
      },
      "uniqueItems": true,
      "minItems": 1
    },
    {
      "type": "array",
      "description": "Members and their scores. Returned in case WITHSCORES is given",
      "notes": "In RESP2 this is returned as a flat array",
      "items": {
        "type": "array",
        "items": [
          {
            "description": "Member",
            "type": "string"
          },
          {
            "description": "Score",
            "type": "number"
          }
        ],
        "additionalItems": false,
        "minItems": 1
      },
      "uniqueItems": true,
      "minItems": 1
    }
  ]
}

panjf2000 pushed a commit to panjf2000/redis that referenced this issue Feb 3, 2022
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:
- 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 redis#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 redis#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

- 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 redis#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)

Co-authored-by: Itamar Haber <itamar@garantiadata.com>
@oranagra oranagra moved this from In progress to To Do in 7.0 Mar 21, 2022
@oranagra oranagra moved this from To Do to Backlog in 7.0 Mar 21, 2022
@oranagra oranagra moved this from Backlog to To Do in 7.0 Mar 21, 2022
@oranagra oranagra removed this from To Do in 7.0 Apr 17, 2022
oranagra added a commit that referenced this issue Mar 11, 2023
Work in progress towards implementing a reply schema as part of COMMAND DOCS, see #9845
Since ironing the details of the reply schema of each and every command can take a long time, we
would like to merge this PR when the infrastructure is ready, and let this mature in the unstable branch.
Meanwhile the changes of this PR are internal, they are part of the repo, but do not affect the produced build.

### Background
In #9656 we add a lot of information about Redis commands, but we are missing information about the replies

### Motivation
1. Documentation. This is the primary goal.
2. It should be possible, based on the output of COMMAND, to be able to generate client code in typed
  languages. In order to do that, we need Redis to tell us, in detail, what each reply looks like.
3. We would like to build a fuzzer that verifies the reply structure (for now we use the existing
  testsuite, see the "Testing" section)

### Schema
The idea is to supply some sort of schema for the various replies of each command.
The schema will describe the conceptual structure of the reply (for generated clients), as defined in RESP3.
Note that the reply structure itself may change, depending on the arguments (e.g. `XINFO STREAM`, with
and without the `FULL` modifier)
We decided to use the standard json-schema (see https://json-schema.org/) as the reply-schema.

Example for `BZPOPMIN`:
```
"reply_schema": {
    "oneOf": [
        {
            "description": "Timeout reached and no elements were popped.",
            "type": "null"
        },
        {
            "description": "The keyname, popped member, and its score.",
            "type": "array",
            "minItems": 3,
            "maxItems": 3,
            "items": [
                {
                    "description": "Keyname",
                    "type": "string"
                },
                {
                    "description": "Member",
                    "type": "string"
                },
                {
                    "description": "Score",
                    "type": "number"
                }
            ]
        }
    ]
}
```

#### Notes
1.  It is ok that some commands' reply structure depends on the arguments and it's the caller's responsibility
  to know which is the relevant one. this comes after looking at other request-reply systems like OpenAPI,
  where the reply schema can also be oneOf and the caller is responsible to know which schema is the relevant one.
2. The reply schemas will describe RESP3 replies only. even though RESP3 is structured, we want to use reply
  schema for documentation (and possibly to create a fuzzer that validates the replies)
3. For documentation, the description field will include an explanation of the scenario in which the reply is sent,
  including any relation to arguments. for example, for `ZRANGE`'s two schemas we will need to state that one
  is with `WITHSCORES` and the other is without.
4. For documentation, there will be another optional field "notes" in which we will add a short description of
  the representation in RESP2, in case it's not trivial (RESP3's `ZRANGE`'s nested array vs. RESP2's flat
  array, for example)

Given the above:
1. We can generate the "return" section of all commands in [redis-doc](https://redis.io/commands/)
  (given that "description" and "notes" are comprehensive enough)
2. We can generate a client in a strongly typed language (but the return type could be a conceptual
  `union` and the caller needs to know which schema is relevant). see the section below for RESP2 support.
3. We can create a fuzzer for RESP3.

### Limitations (because we are using the standard json-schema)
The problem is that Redis' replies are more diverse than what the json format allows. This means that,
when we convert the reply to a json (in order to validate the schema against it), we lose information (see
the "Testing" section below).
The other option would have been to extend the standard json-schema (and json format) to include stuff
like sets, bulk-strings, error-string, etc. but that would mean also extending the schema-validator - and that
seemed like too much work, so we decided to compromise.

Examples:
1. We cannot tell the difference between an "array" and a "set"
2. We cannot tell the difference between simple-string and bulk-string
3. we cannot verify true uniqueness of items in commands like ZRANGE: json-schema doesn't cover the
  case of two identical members with different scores (e.g. `[["m1",6],["m1",7]]`) because `uniqueItems`
  compares (member,score) tuples and not just the member name. 

### Testing
This commit includes some changes inside Redis in order to verify the schemas (existing and future ones)
are indeed correct (i.e. describe the actual response of Redis).
To do that, we added a debugging feature to Redis that causes it to produce a log of all the commands
it executed and their replies.
For that, Redis needs to be compiled with `-DLOG_REQ_RES` and run with
`--reg-res-logfile <file> --client-default-resp 3` (the testsuite already does that if you run it with
`--log-req-res --force-resp3`)
You should run the testsuite with the above args (and `--dont-clean`) in order to make Redis generate
`.reqres` files (same dir as the `stdout` files) which contain request-response pairs.
These files are later on processed by `./utils/req-res-log-validator.py` which does:
1. Goes over req-res files, generated by redis-servers, spawned by the testsuite (see logreqres.c)
2. For each request-response pair, it validates the response against the request's reply_schema
  (obtained from the extended COMMAND DOCS)
5. In order to get good coverage of the Redis commands, and all their different replies, we chose to use
  the existing redis test suite, rather than attempt to write a fuzzer.

#### Notes about RESP2
1. We will not be able to use the testing tool to verify RESP2 replies (we are ok with that, it's time to
  accept RESP3 as the future RESP)
2. Since the majority of the test suite is using RESP2, and we want the server to reply with RESP3
  so that we can validate it, we will need to know how to convert the actual reply to the one expected.
   - number and boolean are always strings in RESP2 so the conversion is easy
   - objects (maps) are always a flat array in RESP2
   - others (nested array in RESP3's `ZRANGE` and others) will need some special per-command
     handling (so the client will not be totally auto-generated)

Example for ZRANGE:
```
"reply_schema": {
    "anyOf": [
        {
            "description": "A list of member elements",
            "type": "array",
            "uniqueItems": true,
            "items": {
                "type": "string"
            }
        },
        {
            "description": "Members and their scores. Returned in case `WITHSCORES` was used.",
            "notes": "In RESP2 this is returned as a flat array",
            "type": "array",
            "uniqueItems": true,
            "items": {
                "type": "array",
                "minItems": 2,
                "maxItems": 2,
                "items": [
                    {
                        "description": "Member",
                        "type": "string"
                    },
                    {
                        "description": "Score",
                        "type": "number"
                    }
                ]
            }
        }
    ]
}
```

### Other changes
1. Some tests that behave differently depending on the RESP are now being tested for both RESP,
  regardless of the special log-req-res mode ("Pub/Sub PING" for example)
2. Update the history field of CLIENT LIST
3. Added basic tests for commands that were not covered at all by the testsuite

### TODO

- [x] (maybe a different PR) add a "condition" field to anyOf/oneOf schemas that refers to args. e.g.
  when `SET` return NULL, the condition is `arguments.get||arguments.condition`, for `OK` the condition
  is `!arguments.get`, and for `string` the condition is `arguments.get` - #11896
- [x] (maybe a different PR) also run `runtest-cluster` in the req-res logging mode
- [x] add the new tests to GH actions (i.e. compile with `-DLOG_REQ_RES`, run the tests, and run the validator)
- [x] (maybe a different PR) figure out a way to warn about (sub)schemas that are uncovered by the output
  of the tests - #11897
- [x] (probably a separate PR) add all missing schemas
- [x] check why "SDOWN is triggered by misconfigured instance replying with errors" fails with --log-req-res
- [x] move the response transformers to their own file (run both regular, cluster, and sentinel tests - need to
  fight with the tcl including mechanism a bit)
- [x] issue: module API - #11898
- [x] (probably a separate PR): improve schemas: add `required` to `object`s - #11899

Co-authored-by: Ozan Tezcan <ozantezcan@gmail.com>
Co-authored-by: Hanna Fadida <hanna.fadida@redislabs.com>
Co-authored-by: Oran Agra <oran@redislabs.com>
Co-authored-by: Shaya Potter <shaya@redislabs.com>
Mixficsol pushed a commit to Mixficsol/redis that referenced this issue Apr 12, 2023
Work in progress towards implementing a reply schema as part of COMMAND DOCS, see redis#9845
Since ironing the details of the reply schema of each and every command can take a long time, we
would like to merge this PR when the infrastructure is ready, and let this mature in the unstable branch.
Meanwhile the changes of this PR are internal, they are part of the repo, but do not affect the produced build.

### Background
In redis#9656 we add a lot of information about Redis commands, but we are missing information about the replies

### Motivation
1. Documentation. This is the primary goal.
2. It should be possible, based on the output of COMMAND, to be able to generate client code in typed
  languages. In order to do that, we need Redis to tell us, in detail, what each reply looks like.
3. We would like to build a fuzzer that verifies the reply structure (for now we use the existing
  testsuite, see the "Testing" section)

### Schema
The idea is to supply some sort of schema for the various replies of each command.
The schema will describe the conceptual structure of the reply (for generated clients), as defined in RESP3.
Note that the reply structure itself may change, depending on the arguments (e.g. `XINFO STREAM`, with
and without the `FULL` modifier)
We decided to use the standard json-schema (see https://json-schema.org/) as the reply-schema.

Example for `BZPOPMIN`:
```
"reply_schema": {
    "oneOf": [
        {
            "description": "Timeout reached and no elements were popped.",
            "type": "null"
        },
        {
            "description": "The keyname, popped member, and its score.",
            "type": "array",
            "minItems": 3,
            "maxItems": 3,
            "items": [
                {
                    "description": "Keyname",
                    "type": "string"
                },
                {
                    "description": "Member",
                    "type": "string"
                },
                {
                    "description": "Score",
                    "type": "number"
                }
            ]
        }
    ]
}
```

#### Notes
1.  It is ok that some commands' reply structure depends on the arguments and it's the caller's responsibility
  to know which is the relevant one. this comes after looking at other request-reply systems like OpenAPI,
  where the reply schema can also be oneOf and the caller is responsible to know which schema is the relevant one.
2. The reply schemas will describe RESP3 replies only. even though RESP3 is structured, we want to use reply
  schema for documentation (and possibly to create a fuzzer that validates the replies)
3. For documentation, the description field will include an explanation of the scenario in which the reply is sent,
  including any relation to arguments. for example, for `ZRANGE`'s two schemas we will need to state that one
  is with `WITHSCORES` and the other is without.
4. For documentation, there will be another optional field "notes" in which we will add a short description of
  the representation in RESP2, in case it's not trivial (RESP3's `ZRANGE`'s nested array vs. RESP2's flat
  array, for example)

Given the above:
1. We can generate the "return" section of all commands in [redis-doc](https://redis.io/commands/)
  (given that "description" and "notes" are comprehensive enough)
2. We can generate a client in a strongly typed language (but the return type could be a conceptual
  `union` and the caller needs to know which schema is relevant). see the section below for RESP2 support.
3. We can create a fuzzer for RESP3.

### Limitations (because we are using the standard json-schema)
The problem is that Redis' replies are more diverse than what the json format allows. This means that,
when we convert the reply to a json (in order to validate the schema against it), we lose information (see
the "Testing" section below).
The other option would have been to extend the standard json-schema (and json format) to include stuff
like sets, bulk-strings, error-string, etc. but that would mean also extending the schema-validator - and that
seemed like too much work, so we decided to compromise.

Examples:
1. We cannot tell the difference between an "array" and a "set"
2. We cannot tell the difference between simple-string and bulk-string
3. we cannot verify true uniqueness of items in commands like ZRANGE: json-schema doesn't cover the
  case of two identical members with different scores (e.g. `[["m1",6],["m1",7]]`) because `uniqueItems`
  compares (member,score) tuples and not just the member name. 

### Testing
This commit includes some changes inside Redis in order to verify the schemas (existing and future ones)
are indeed correct (i.e. describe the actual response of Redis).
To do that, we added a debugging feature to Redis that causes it to produce a log of all the commands
it executed and their replies.
For that, Redis needs to be compiled with `-DLOG_REQ_RES` and run with
`--reg-res-logfile <file> --client-default-resp 3` (the testsuite already does that if you run it with
`--log-req-res --force-resp3`)
You should run the testsuite with the above args (and `--dont-clean`) in order to make Redis generate
`.reqres` files (same dir as the `stdout` files) which contain request-response pairs.
These files are later on processed by `./utils/req-res-log-validator.py` which does:
1. Goes over req-res files, generated by redis-servers, spawned by the testsuite (see logreqres.c)
2. For each request-response pair, it validates the response against the request's reply_schema
  (obtained from the extended COMMAND DOCS)
5. In order to get good coverage of the Redis commands, and all their different replies, we chose to use
  the existing redis test suite, rather than attempt to write a fuzzer.

#### Notes about RESP2
1. We will not be able to use the testing tool to verify RESP2 replies (we are ok with that, it's time to
  accept RESP3 as the future RESP)
2. Since the majority of the test suite is using RESP2, and we want the server to reply with RESP3
  so that we can validate it, we will need to know how to convert the actual reply to the one expected.
   - number and boolean are always strings in RESP2 so the conversion is easy
   - objects (maps) are always a flat array in RESP2
   - others (nested array in RESP3's `ZRANGE` and others) will need some special per-command
     handling (so the client will not be totally auto-generated)

Example for ZRANGE:
```
"reply_schema": {
    "anyOf": [
        {
            "description": "A list of member elements",
            "type": "array",
            "uniqueItems": true,
            "items": {
                "type": "string"
            }
        },
        {
            "description": "Members and their scores. Returned in case `WITHSCORES` was used.",
            "notes": "In RESP2 this is returned as a flat array",
            "type": "array",
            "uniqueItems": true,
            "items": {
                "type": "array",
                "minItems": 2,
                "maxItems": 2,
                "items": [
                    {
                        "description": "Member",
                        "type": "string"
                    },
                    {
                        "description": "Score",
                        "type": "number"
                    }
                ]
            }
        }
    ]
}
```

### Other changes
1. Some tests that behave differently depending on the RESP are now being tested for both RESP,
  regardless of the special log-req-res mode ("Pub/Sub PING" for example)
2. Update the history field of CLIENT LIST
3. Added basic tests for commands that were not covered at all by the testsuite

### TODO

- [x] (maybe a different PR) add a "condition" field to anyOf/oneOf schemas that refers to args. e.g.
  when `SET` return NULL, the condition is `arguments.get||arguments.condition`, for `OK` the condition
  is `!arguments.get`, and for `string` the condition is `arguments.get` - redis#11896
- [x] (maybe a different PR) also run `runtest-cluster` in the req-res logging mode
- [x] add the new tests to GH actions (i.e. compile with `-DLOG_REQ_RES`, run the tests, and run the validator)
- [x] (maybe a different PR) figure out a way to warn about (sub)schemas that are uncovered by the output
  of the tests - redis#11897
- [x] (probably a separate PR) add all missing schemas
- [x] check why "SDOWN is triggered by misconfigured instance replying with errors" fails with --log-req-res
- [x] move the response transformers to their own file (run both regular, cluster, and sentinel tests - need to
  fight with the tcl including mechanism a bit)
- [x] issue: module API - redis#11898
- [x] (probably a separate PR): improve schemas: add `required` to `object`s - redis#11899

Co-authored-by: Ozan Tezcan <ozantezcan@gmail.com>
Co-authored-by: Hanna Fadida <hanna.fadida@redislabs.com>
Co-authored-by: Oran Agra <oran@redislabs.com>
Co-authored-by: Shaya Potter <shaya@redislabs.com>
enjoy-binbin pushed a commit to enjoy-binbin/redis that referenced this issue Jul 31, 2023
Work in progress towards implementing a reply schema as part of COMMAND DOCS, see redis#9845
Since ironing the details of the reply schema of each and every command can take a long time, we
would like to merge this PR when the infrastructure is ready, and let this mature in the unstable branch.
Meanwhile the changes of this PR are internal, they are part of the repo, but do not affect the produced build.

### Background
In redis#9656 we add a lot of information about Redis commands, but we are missing information about the replies

### Motivation
1. Documentation. This is the primary goal.
2. It should be possible, based on the output of COMMAND, to be able to generate client code in typed
  languages. In order to do that, we need Redis to tell us, in detail, what each reply looks like.
3. We would like to build a fuzzer that verifies the reply structure (for now we use the existing
  testsuite, see the "Testing" section)

### Schema
The idea is to supply some sort of schema for the various replies of each command.
The schema will describe the conceptual structure of the reply (for generated clients), as defined in RESP3.
Note that the reply structure itself may change, depending on the arguments (e.g. `XINFO STREAM`, with
and without the `FULL` modifier)
We decided to use the standard json-schema (see https://json-schema.org/) as the reply-schema.

Example for `BZPOPMIN`:
```
"reply_schema": {
    "oneOf": [
        {
            "description": "Timeout reached and no elements were popped.",
            "type": "null"
        },
        {
            "description": "The keyname, popped member, and its score.",
            "type": "array",
            "minItems": 3,
            "maxItems": 3,
            "items": [
                {
                    "description": "Keyname",
                    "type": "string"
                },
                {
                    "description": "Member",
                    "type": "string"
                },
                {
                    "description": "Score",
                    "type": "number"
                }
            ]
        }
    ]
}
```

#### Notes
1.  It is ok that some commands' reply structure depends on the arguments and it's the caller's responsibility
  to know which is the relevant one. this comes after looking at other request-reply systems like OpenAPI,
  where the reply schema can also be oneOf and the caller is responsible to know which schema is the relevant one.
2. The reply schemas will describe RESP3 replies only. even though RESP3 is structured, we want to use reply
  schema for documentation (and possibly to create a fuzzer that validates the replies)
3. For documentation, the description field will include an explanation of the scenario in which the reply is sent,
  including any relation to arguments. for example, for `ZRANGE`'s two schemas we will need to state that one
  is with `WITHSCORES` and the other is without.
4. For documentation, there will be another optional field "notes" in which we will add a short description of
  the representation in RESP2, in case it's not trivial (RESP3's `ZRANGE`'s nested array vs. RESP2's flat
  array, for example)

Given the above:
1. We can generate the "return" section of all commands in [redis-doc](https://redis.io/commands/)
  (given that "description" and "notes" are comprehensive enough)
2. We can generate a client in a strongly typed language (but the return type could be a conceptual
  `union` and the caller needs to know which schema is relevant). see the section below for RESP2 support.
3. We can create a fuzzer for RESP3.

### Limitations (because we are using the standard json-schema)
The problem is that Redis' replies are more diverse than what the json format allows. This means that,
when we convert the reply to a json (in order to validate the schema against it), we lose information (see
the "Testing" section below).
The other option would have been to extend the standard json-schema (and json format) to include stuff
like sets, bulk-strings, error-string, etc. but that would mean also extending the schema-validator - and that
seemed like too much work, so we decided to compromise.

Examples:
1. We cannot tell the difference between an "array" and a "set"
2. We cannot tell the difference between simple-string and bulk-string
3. we cannot verify true uniqueness of items in commands like ZRANGE: json-schema doesn't cover the
  case of two identical members with different scores (e.g. `[["m1",6],["m1",7]]`) because `uniqueItems`
  compares (member,score) tuples and not just the member name. 

### Testing
This commit includes some changes inside Redis in order to verify the schemas (existing and future ones)
are indeed correct (i.e. describe the actual response of Redis).
To do that, we added a debugging feature to Redis that causes it to produce a log of all the commands
it executed and their replies.
For that, Redis needs to be compiled with `-DLOG_REQ_RES` and run with
`--reg-res-logfile <file> --client-default-resp 3` (the testsuite already does that if you run it with
`--log-req-res --force-resp3`)
You should run the testsuite with the above args (and `--dont-clean`) in order to make Redis generate
`.reqres` files (same dir as the `stdout` files) which contain request-response pairs.
These files are later on processed by `./utils/req-res-log-validator.py` which does:
1. Goes over req-res files, generated by redis-servers, spawned by the testsuite (see logreqres.c)
2. For each request-response pair, it validates the response against the request's reply_schema
  (obtained from the extended COMMAND DOCS)
5. In order to get good coverage of the Redis commands, and all their different replies, we chose to use
  the existing redis test suite, rather than attempt to write a fuzzer.

#### Notes about RESP2
1. We will not be able to use the testing tool to verify RESP2 replies (we are ok with that, it's time to
  accept RESP3 as the future RESP)
2. Since the majority of the test suite is using RESP2, and we want the server to reply with RESP3
  so that we can validate it, we will need to know how to convert the actual reply to the one expected.
   - number and boolean are always strings in RESP2 so the conversion is easy
   - objects (maps) are always a flat array in RESP2
   - others (nested array in RESP3's `ZRANGE` and others) will need some special per-command
     handling (so the client will not be totally auto-generated)

Example for ZRANGE:
```
"reply_schema": {
    "anyOf": [
        {
            "description": "A list of member elements",
            "type": "array",
            "uniqueItems": true,
            "items": {
                "type": "string"
            }
        },
        {
            "description": "Members and their scores. Returned in case `WITHSCORES` was used.",
            "notes": "In RESP2 this is returned as a flat array",
            "type": "array",
            "uniqueItems": true,
            "items": {
                "type": "array",
                "minItems": 2,
                "maxItems": 2,
                "items": [
                    {
                        "description": "Member",
                        "type": "string"
                    },
                    {
                        "description": "Score",
                        "type": "number"
                    }
                ]
            }
        }
    ]
}
```

### Other changes
1. Some tests that behave differently depending on the RESP are now being tested for both RESP,
  regardless of the special log-req-res mode ("Pub/Sub PING" for example)
2. Update the history field of CLIENT LIST
3. Added basic tests for commands that were not covered at all by the testsuite

### TODO

- [x] (maybe a different PR) add a "condition" field to anyOf/oneOf schemas that refers to args. e.g.
  when `SET` return NULL, the condition is `arguments.get||arguments.condition`, for `OK` the condition
  is `!arguments.get`, and for `string` the condition is `arguments.get` - redis#11896
- [x] (maybe a different PR) also run `runtest-cluster` in the req-res logging mode
- [x] add the new tests to GH actions (i.e. compile with `-DLOG_REQ_RES`, run the tests, and run the validator)
- [x] (maybe a different PR) figure out a way to warn about (sub)schemas that are uncovered by the output
  of the tests - redis#11897
- [x] (probably a separate PR) add all missing schemas
- [x] check why "SDOWN is triggered by misconfigured instance replying with errors" fails with --log-req-res
- [x] move the response transformers to their own file (run both regular, cluster, and sentinel tests - need to
  fight with the tcl including mechanism a bit)
- [x] issue: module API - redis#11898
- [x] (probably a separate PR): improve schemas: add `required` to `object`s - redis#11899

Co-authored-by: Ozan Tezcan <ozantezcan@gmail.com>
Co-authored-by: Hanna Fadida <hanna.fadida@redislabs.com>
Co-authored-by: Oran Agra <oran@redislabs.com>
Co-authored-by: Shaya Potter <shaya@redislabs.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
state:major-decision Requires core team consensus
Projects
None yet
Development

No branches or pull requests

4 participants