octokit / routes

machine-readable, always up-to-date GitHub REST API route specifications
MIT License
84 stars 42 forks source link

Response schemas: set all keys as required, add `additionalProperties = false` flag #725

Open gr2m opened 4 years ago

gr2m commented 4 years ago

We assume that the response schemas are complete. There will be no additional keys, and all keys are guaranteed to be set.

The current definitions for response schemas don't set the additionalProperties or required flags.

A simple patch method to add them looks like this

function patchSchema(schema) {
  if (schema.items) {
    patchSchema(schema.items);
  }

  if (!schema.properties) return;

  // make all keys required, set additionalProperties to false
  schema.required = Object.keys(schema.properties);
  schema.additionalProperties = false;

  Object.values(schema.properties).forEach(patchSchema);
}
gr2m commented 4 years ago

Make sure to add the keys in response schema overrides in workarounds.js

RomanHotsiy commented 4 years ago

@gr2m what about some keys that were generated from null?

e.g. here https://developer.github.com/v3/repos/#response template_repository is null in examples so I don't think it should be marked as required.

Btw, template_repository in this particular case is turned into type: string property which I suppose is another issue.

gr2m commented 4 years ago

right now, none of the keys are marked as required, which is not ideal either.

e.g. here https://developer.github.com/v3/repos/#response template_repository is null in examples so I don't think it should be marked as required.

I don't see the template_repository in the response?

RomanHotsiy commented 4 years ago

image

gr2m commented 4 years ago

I see it now. Sorry, odd that I missed it before.

Anyway, there is no ideal solution to this right now. We can add more workarounds to /lib/endpoint/overrides/workarounds.js on a case-to-case basis. But I think setting everything to required is still the better default.

Btw, template_repository in this particular case is turned into type: string property which I suppose is another issue

yeah, that seems unrelated