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

Improve schema files #13

Closed
chadrik opened this issue May 24, 2016 · 3 comments
Closed

Improve schema files #13

chadrik opened this issue May 24, 2016 · 3 comments
Assignees

Comments

@chadrik
Copy link
Contributor

chadrik commented May 24, 2016

Looking through the schema files I realized that these are quite a few problems and shortcomings with the current schema files:

  • The schema files actually contain many schemas: one for each message type under "output" and "input". While it might make it easier to edit and read, there seems to be some expectation in the json-schema spec and community that there is only one schema per document. For example, each schema doc is expected to have an "id" attribute specifying where the file can be downloaded on the web. more here. One-file-per-schema also makes it easier when working with command-line tools which seem to have this expectation.

  • we can improve the schemas by using the $ref keyword to avoid duplication. Note that this can be used to refer to other objects in the same doc { "$ref": "#/definitions/address" } or in another doc { "$ref": "definitions.json#/address" }.

  • no $schema keyword: it's good form to indicate what version of the draft is being used

  • many required properties are not indicated as such (there are none in component.json). easy enough to cross reference properties with "optional" in their description.

  • the "required" keyword must be a list of properties and can only exist for compound schemas. e.g. this is bad:

    "changenode": {
      "description": "Change the metadata associated to a node in the graph",
      "properties": {
        "id": {
          "type": "string",
          "description": "identifier for the node",
          "required": true
        },
        "metadata": {
          "type": "object",
          "description": "structure of key-value pairs for node metadata",
          "required": true
        },
        "graph": {
          "type": "string",
          "description": "graph the action targets",
          "required": true
        }
      }
    }

    this is good:

    "changenode": {
      "description": "Change the metadata associated to a node in the graph",
      "properties": {
        "id": {
          "type": "string",
          "description": "identifier for the node",
        },
        "metadata": {
          "type": "object",
          "description": "structure of key-value pairs for node metadata",
        },
        "graph": {
          "type": "string",
          "description": "graph the action targets",
        }
      },
      "required": ["id", "metadata", "graph"]
    }
  • nested schemas don't indicate "type": "object". it's implicit, but should probably be there.

  • "outPorts" and "inPorts" should indicate that they are an array of objects

  • by default, extra keywords in the document are allowed. I think we should set "additionalProperties": false for all schemas to prevent typos from slipping through without error.

I tried using one of our schemas for validation (after extracting it from its parent doc) and got an error ("schema/json/component.json#/properties/name: true is not a valid "required", must be a array."), so I think technically our schemas are not valid as they are. we should definitely be running validation of our schema files in the tests for this repo

This ticket should be done in conjunction with converting fbp-test to use the schemas.

Some resources for learning more about json-schema:

@chadrik
Copy link
Contributor Author

chadrik commented May 24, 2016

Looks like many-schemas-per-file may be ok, you just have to assign an "id" to each document: http://stackoverflow.com/questions/8179137/how-to-manage-multiple-json-schema-files

@ifitzpatrick ifitzpatrick mentioned this issue Jun 1, 2016
@jonnor
Copy link
Contributor

jonnor commented Jun 22, 2016

Looks like the schemas are valid now? So guess if we want to keep this issue open, it should be renamed to "Split schemas into one per request/response"? Or we just close it after adding ids?

@chadrik
Copy link
Contributor Author

chadrik commented Jun 22, 2016

Closed with #17

@chadrik chadrik closed this as completed Jun 22, 2016
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

No branches or pull requests

3 participants