change schema validation to STRICT

[hornc]

This has the consequence that all response properties are assumed to be required (due to a quirk in json-schema) but will alert on extra undocumented properties, which we want and have identified as more important for now.

TODO: add a configuration option to allow either strict or non-strict validations, and find a way to remove the 'assume-required' assumption.

[chrisrbnelson]

This update breaks swagger validation as it assumes all properties specified in the swagger definition are required. One example of this in the error responses; we need to document all possible fields that could be returned, but most of the time won't be (e.g. model validation errors).

[hornc]

Hi @chrisrbnelson, I sent an email out discussing the proposed changes, and the feedback from others was that this was an ok thing to do for now, but that we should add in an option to relax the strictness:

Hi all,

I’ve been talking with Leon about how we should standardise our APIs’ handling of null values in responses, as it relates to the current effort to document and validate the v0.1 versions using Swagger, and also how we should deal with un-documented properties.

The following is a background to the issues, and an opportunity for you all to comment and give your opinions.

There are two possible approaches for dealing with null response properties that can be represented in valid swagger:

1) ALL properties must be returned in a response, but null is a valid value for anything where null is possible, and must be documented in the schema e.g.:

"name": {
          "type": ["string", "null"],
          "description": "Service name"
        }

Actually, the allowable values are up to the API designer, it could be an empty string or some other default, the important part is that all properties are explicitly returned.

2) Some properties of the response schema can be optional, by not listing them in the ‘required’ array. This means that if there isn’t a suitable value to return, the response does not need to show anything for that property. e.g.:

    "service": {
      "type": "object",
      "required": [ "id" ],
      "properties": {
        "id": {
          "type": "integer",
          "description": "Service id"
        },
        "name": {
          "type": ["string"],
          "description": "Service name"
        }
      } 

In the above example, only id is required, name can be left out (not a very useful response, but it shows how it is done, and it is valid documentation).

Our thoughts are that it is better to explicitly return all properties for every response (option 1) as it is clearer, and in line with the principle of least astonishment.

If there is an exceptional reason for a particular API to use optional fields (response bloat?), we can discuss, but we’d like to make the Westfield default that all properties are explicitly returned.

Currently the Apivore testing tool allows optional fields, and will successfully validate either approach, but unfortunately, as a side-effect of the json validator it uses, it will also pass if it receives un-documented properties in a response.

We also want to catch any undocumented property returned in an API response, and fail the validation since our goal is to fully document our APIs. We don’t want ‘secret’ data that can only be used if you happen to know about it.

Our proposal, if no one has strong objections, is to change Apivore to use ‘strict’ validation by default so that it will fail if any property is not returned, and also fail if it encounters a response property that is not documented. This will mean that any of our current services that work with optional properties will start to fail, and need to be modified.

Ideally Apivore, as a general testing tool, will be configurable to correctly validate either approach, but for now we need to set a Westfield standard.

If no one has any objections, I’ll make the 'strict' mode change to Apivore’s default validation behaviour next week, and add the configurable strictness options as a TODO for later.

[chrisrbnelson]

@hornc Thanks for that - it makes sense that we want to catch any undocumented properties from being returned.

From what I can tell, the JSON validator expects properties to list required properties, and additionalProperties to list optional properties. This works as we would expect; Apivore is able to validate the fields in additionalProperties if they exist (to ensure correct data type, etc...) but is also allowed to ignore them if they don't. Extra properties that aren't included in either properties or additionalProperties will still cause validation to fail.

However - additionalProperties doesn't appear to be supported in the swagger schema. At the very least, it doesn't correctly show up in swagger-ui. This could just be a bug, and if we can fix the problem it could solve the issue. We would be able to document the existence of the extra properties while also documenting that they are optional and may not be returned in the response.

[hornc]

Thanks for that research @chrisrbnelson, that's further than I got, I assumed there must be some way to get that behaviour. I'll have a play with those fields and look into the Swagger support to see if I can work something out to bring Swagger in line with that, or at least find out if there are plans to add it in a future Swagger version, or something.

Cheers!