search

Yelp / swagger_spec_validator

Other
104 stars 71 forks source link

Disallow multiple types in schema definitions #106

Closed sjaensch closed 6 years ago

sjaensch commented 6 years ago

Swagger 2 does not support a schema object having multiple types. This is not explicitly called out in the spec, and there's confusion around it since JSONSchema draft 4 does allow multiple types. The spec maintainers have publicly stated that multiple types are not supported (see OAI/OpenAPI-Specification#458) and have clarified the spec for OpenAPI 3 to mention that fact.

Additionally, our tooling around Swagger generally does not support multiple types either. So let's make sure swagger_spec_validator reports this problem.

coveralls commented 6 years ago

Coverage Status

Coverage increased (+0.01%) to 98.427% when pulling 7775152fc548d89b4aa24b8aed631385797e76b8 on sjaensch-COREBACK-6759-disallow-multiple-types into 7e27cadba4e10a1c8c442198f6811a08b7f58f24 on master.

ptte commented 6 years ago

Hi! Thanks for all the work on this project!

Just a note that this change breaks anything currently running with the assumption that you can define multiple types, ie it's a significant breaking change, at least it was for us. Not saying you shouldn't do it but in our case it would have been very helpful if it was deprecated with a warning or similar :)

sjaensch commented 6 years ago

Hey @ptte, thanks for the feedback! How would you prefer us to proceed? We could release 2.4.1 that converts the error to a warning, would that help? We don't have any other big development currently going on for swagger_spec_validator, the other option would be for you to stay on 2.3.1 until you're ready to upgrade. We've identified usage of multiple types internally as well, and we've fixed those. Interestingly, Swagger Editor doesn't complain about using multiple types. Probably, just like swagger_spec_validator in the past, it just used JSONSchema validation for that field.

ptte commented 6 years ago

How would you prefer us to proceed?

we have just locked to 2.3.1 for now in order to build, and we'll probably try to migrate over to 2.4 at some point soon so for us it's already solved. So from my point of view this is fine :)

chribsen commented 6 years ago

+1 for deprecating this. Up until recently we were using this as a feature to allow nullable fields (i.e. "types":["null", number]). After this change has been introduced (commit daecbe08) our tests no longer pass validation.