alphagov / tech-docs-gem

Gem to distribute the tech docs project
https://tdt-documentation.london.cloudapps.digital/
MIT License
15 stars 38 forks source link

Fix bug in API docs that shows required attributes as optional #113

Closed tijmenb closed 4 years ago

tijmenb commented 5 years ago

:wave: from DfE!

This fixes a bug in the OpenAPI reference renderer that affects which attributes of a schema are considered to be required.

For the uninitiated: in OpenAPI, schemas allow you to define what objects/resources/models your API will return. It uses JSON Schema for this.

For example:

schemas:
  Pet:
    required:
      - id
      - name
    properties:
      id:
        type: integer
        format: int64
      name:
        type: string
      tag:
        type: string

This means that any Pet objects will have at least an id and a name, and an optional tag.

Currently the logic to display which attributes are required is wrong, because the code expects something like:

id:
  type: integer
  format: int64
  required: true

This isn't valid JSON Schema as far as I can see:

https://json-schema.org/understanding-json-schema/reference/object.html#required-properties

This commit fixes the bug by making the check look at the required array in the schema, rather than the property.

How to review

Read the main commit, the other one is a small refactoring to make things clearer.

Before

Screenshot 2019-09-17 at 22 08 34

After

Screenshot 2019-09-17 at 22 07 48
tijmenb commented 4 years ago

Thanks @36degrees!

I'm actually going to close this, because it was based on a misunderstanding of OpenAPI and JSON Schema. It turns out that OpenAPI "forked" JSON Schema and made some subtle changes. This is just wonderful.

Thus, my assertion that "required: true" isn't JSON Schema is true, but it's not relevant to OpenAPI. And this fix would actually break support for OpenAPI.

Because we found a number of additional issues with the API reference feature, we've mostly rewritten it for our tech docs. Once we've tested this version with users we'll try to push it back into the tech-docs-gem.

I haven't been able to find another user of the API reference feature in the tech docs by the way, if you are using it let me know, and we can talk about improving it.