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

feat: json-schema commands format #1232

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Conversation

mmkal
Copy link
Contributor

@mmkal mmkal commented Dec 29, 2019

@itamarhaber this is an idea following the discussion over the DSL in #1231

This PR consists of a small js script, which parses commands.json to produce a json file in a slightly different format. Each command has the same root-level properties, except for arguments, which is now in the format:

Array<{
  name: string;
  optional: boolean;
  schema: JSONSchema7;
}>

This allows redis to avoid re-inventing the wheel for any of the undifferentiated schema-description parts of the documentation, instead relying on json-schema, which is well established, widely used, unambiguous, flexible enough to cover edge-cases, and a good balance of human- and machine-readable.

It's worth taking a look at the generated json-schema file, since it's large, so github suppresses the diff.

Note:

Each argument is represented by JSON-schema, but conceptually the schemas are "flattened" when passed as CLI arguments. For example, DEL. This has one argument, key, which is of type array, meaning it would be valid for inputs like ['foo', 'bar'], corresponding to redis del foo bar. So to go from arguments valid per the schema to a CLI call, clients would have to do something like redis ${command} ${flatten(arguments).join(' ')}.

I also wrote a simple function that parses the .md documentation for each command to extract the return type and convert to json-schema, so that type can be kept alongside the arguments. Going forward, this json file could become the source of truth for return types, so that this repo could document not just basic return types like @array-reply, but specify the type of the items in the array too, easily and unambiguously with json-schema (e.g. { "type": "array", "items": { "type": "string" } }).

This will make client generation much easier, because existing json-schema tooling can be used rather than each client library having to write a custom parser which reverse-engineers the custom DSL currently in commands.json. (The library I maintain does exactly this reverse-engineering, and has a lot of hacks and pieces of difficult-to-maintain code. I have held off adding it to clients.json so far because of this).

My thought was that if this accepted, the generated commands.json-schema.json acts as a starting point, and would eventually take the place of commands.json. it would then be manually edited, as the source of truth, going forwards. The js file shouldn't need to hang around, it's just for generating the "starter" json-schema file. This will allow taking advantage of json-schema's API to go further with type specificity. For example, the problem highlighted in #1231 would go away, because SET could become (using yaml for conciseness):

summary: Set the string value of a key
complexity: O(1)
arguments:
- name: key
  schema:
    type: string
- name: value
  schema:
    type: string
- name: expiration
  optional: true
  schema:
    type: array
    items:
    - type: enum
      enum: [EX, PX]
    - type: integer
- name: condition
  optional: true
  schema:
    type: string
    enum: [NX, XX]
since: 1.0.0
group: string
return:
  anyOf:
  - type: string
  - type: 'null'

I'd be interested to learn what is currently using commands.json in the existing format - if you think the idea has legs, I could see what it would take for whatever's downstream (if anything) to switch over to json-schema.

@mmkal
Copy link
Contributor Author

mmkal commented Apr 7, 2020

@itamarhaber - any thoughts on this?

@mmkal mmkal mentioned this pull request Nov 23, 2020
mmkal added a commit to mmkal/handy-redis that referenced this pull request Sep 17, 2021
Fixes #324 

Supersedes #327

Fixes/patches required:

- update SET command (redis/redis-doc#1232 still getting no love. I will take that PR to my grave I guess)
- disable redis v7 commands/options tests
- disable non-deterministic command tests (hrandfield etc.)
mmkal added a commit to mmkal/handy-redis that referenced this pull request Sep 17, 2021
Fixes #324

Supersedes #327

Fixes/patches required:

- update SET command (redis/redis-doc#1232 still getting no love. I will take that PR to my grave I guess)
- disable redis v7 commands/options tests
- disable non-deterministic command tests (hrandfield etc.)
@CLAassistant
Copy link

CLAassistant commented Mar 21, 2024

CLA assistant check
All committers have signed the CLA.

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.

None yet

2 participants