Yelp / bravado-core

Other
109 stars 98 forks source link

Be resilient in case of None in references #315

Closed macisamuele closed 5 years ago

macisamuele commented 5 years ago

The goal of this PR is to make sure that reference detection takes care of the actual value of the json-pointer (value of $ref attribute).

This issue was noticed while debugging a recursion error that was happening while flattening specs that have None value in references. While this condition is odd while writing with JSON specs it is not uncommon while working with YAML specs (we're humans and is not so weird to forget to quote a reference if it starts with #).

definitions:
  mod1:
    type: object
  mod2:
    $ref: #/definitions/mod1   <- this will be rendered as {'$ref': None} as `#` in YAML represents a comment

Unbounded recursion during flattening is happening because deref will try to dereference it and it will actually succeed by getting the original object (the whole swagger specs), then descending it we will be back to the case of having None in $ref and so until we get an ugly RecursionError.

To makes flattening process safer for this case I did first added checks for isinstance(obj['$ref'], six.string_types) in flattening and model discovery.

After doing that I wanted to benchmark the performance hit that we could have if we introduce this additional check in bravado_core.schema.is_ref directly. The benchmark results shows that adding the isinstance check is not making things slower so I decided to make sure that is_ref check is more accurate.

Benchmarks are available on: https://gist.github.com/macisamuele/42434160b17edaf22fcbf2415902c926

NOTE: the alternative solution is available on https://github.com/macisamuele/bravado-core/tree/maci-be-resilient-in-case-of-None-ref-no-edit-to-is_ref

coveralls commented 5 years ago

Coverage Status

Coverage increased (+0.1%) to 98.458% when pulling 66e1ac6f48bc0c68541a2f60738136f454f22319 on macisamuele:maci-be-resilient-in-case-of-None-ref into cde813a6b89e2cf8322e0ad737f294955a512bad on Yelp:master.