search

Yelp / swagger_spec_validator

Other
104 stars 71 forks source link

fix: additionalProperties and array items should be validated #134

Closed brycedrennan closed 4 years ago

brycedrennan commented 4 years ago

object definitions under object additionalProperties or under array items were not being validated.

This adds that validation and test cases.

coveralls commented 4 years ago

Coverage Status

Coverage increased (+0.01%) to 98.554% when pulling c961b877679ac6d5ec983b095df13ddf77772a93 on brycedrennan:additional-properties-bugfix into dc03f20cd4b2140dba967226821afc56defdf915 on Yelp:master.

brycedrennan commented 4 years ago

@macisamuele would appreciate your consideration of these crucial bugfixes.

brycedrennan commented 4 years ago

@sjaensch any hope of getting this merged in? I totally understand if it's not a priority. I'll just self-host a fork if needed.

brycedrennan commented 4 years ago

@sjaensch all good feedback although perhaps not worth opening another PR for?

brycedrennan commented 4 years ago

@macisamuele no worries - thanks for looking at this!

kpfleming commented 4 years ago

I haven't fully debugged this yet, but an application I have which uses Bravado is now failing its tests, reporting "AttributeError: 'list' object has no attribute 'get'\n" on line 474 of validator20.py. I'm going to test rolling back to 2.6.0 now.

kpfleming commented 4 years ago

I've at least confirmed the problem was introduced in 2.7.0; tests on Python 3.5-3.8 all fail with 2.7.0, but pass with 2.6.0.

kpfleming commented 4 years ago

The spec being used in these tests is here.

brycedrennan commented 4 years ago

Looks like this is invalid in your spec? https://github.com/kpfleming/ansible-pdns-auth-api/blob/master/api-swagger.json#L1078

brycedrennan commented 4 years ago

Which comes from here: https://github.com/PowerDNS/pdns/blob/master/docs/http-api/swagger/authoritative-api-swagger.yaml#L403

kpfleming commented 4 years ago

Ah-ha! Thanks for catching that. We'll get that fixed.

kpfleming commented 4 years ago

Indeed, this sort of response is not even supported in Swagger/OAS 2.0 (it could be in OAS 3.0 using anyOf).

kpfleming commented 4 years ago

Thanks again for your help tracking this down. I'll add a test in the PowerDNS build system to use this package to validate the schema directly, so that things like this won't creep in again.

Should I open an issue about the 'ungraceful' error in this situation? It seems that I should have received a SwaggerValidationError exception instead of a raw Python AttributeError.