Yelp / swagger_spec_validator

Other
104 stars 71 forks source link

Fix recursive ref resolution for some specs split across multiple files #140

Closed sjaensch closed 4 years ago

sjaensch commented 4 years ago

We're still encountering validation errors internally due to the fact that since version 2.7.0 array item definitions are properly validated (see #134).

Example traceback:

[...]
  File "/nail/live/yelp/virtualenv_run/local/lib/python2.7/site-packages/swagger_spec_validator/validator20.py", line 170, in validate_spec
    validate_apis(apis, bound_deref)
  File "/nail/live/yelp/virtualenv_run/local/lib/python2.7/site-packages/swagger_spec_validator/validator20.py", line 387, in validate_apis
    validate_responses(api_name, oper_name, oper_body['responses'], deref)
  File "/nail/live/yelp/virtualenv_run/local/lib/python2.7/site-packages/swagger_spec_validator/validator20.py", line 285, in validate_responses
    visited_definitions=set(),
  File "/nail/live/yelp/virtualenv_run/local/lib/python2.7/site-packages/swagger_spec_validator/validator20.py", line 514, in validate_definition
    visited_definitions=visited_definitions,
  File "/nail/live/yelp/virtualenv_run/local/lib/python2.7/site-packages/swagger_spec_validator/validator20.py", line 506, in validate_definition
    visited_definitions=visited_definitions
  File "/nail/live/yelp/virtualenv_run/local/lib/python2.7/site-packages/swagger_spec_validator/validator20.py", line 458, in validate_arrays_in_definition
    visited_definitions=visited_definitions,
  File "/nail/live/yelp/virtualenv_run/local/lib/python2.7/site-packages/swagger_spec_validator/validator20.py", line 514, in validate_definition
    visited_definitions=visited_definitions,
  File "/nail/live/yelp/virtualenv_run/local/lib/python2.7/site-packages/swagger_spec_validator/validator20.py", line 506, in validate_definition
    visited_definitions=visited_definitions
  File "/nail/live/yelp/virtualenv_run/local/lib/python2.7/site-packages/swagger_spec_validator/validator20.py", line 458, in validate_arrays_in_definition
    visited_definitions=visited_definitions,
  File "/nail/live/yelp/virtualenv_run/local/lib/python2.7/site-packages/swagger_spec_validator/validator20.py", line 501, in validate_definition
    validate_defaults_in_definition(definition, deref)
  File "/nail/live/yelp/virtualenv_run/local/lib/python2.7/site-packages/swagger_spec_validator/validator20.py", line 443, in validate_defaults_in_definition
    validate_property_default(property_spec, deref)
  File "/nail/live/yelp/virtualenv_run/local/lib/python2.7/site-packages/swagger_spec_validator/validator20.py", line 432, in validate_property_default
    deref_property_spec = deref(property_spec)
  File "/nail/live/yelp/virtualenv_run/local/lib/python2.7/site-packages/swagger_spec_validator/validator20.py", line 122, in deref
    with resolver.resolving(ref) as target:
  File "/usr/lib/python2.7/contextlib.py", line 17, in __enter__
    return self.gen.next()
  File "/nail/live/yelp/virtualenv_run/local/lib/python2.7/site-packages/jsonschema/validators.py", line 327, in resolving
    url, resolved = self.resolve(ref)
  File "/nail/live/yelp/virtualenv_run/local/lib/python2.7/site-packages/jsonschema/validators.py", line 336, in resolve
    return url, self._remote_cache(url)
  File "/nail/live/yelp/virtualenv_run/local/lib/python2.7/site-packages/functools32/functools32.py", line 400, in wrapper
    result = user_function(*args, **kwds)
  File "/nail/live/yelp/virtualenv_run/local/lib/python2.7/site-packages/jsonschema/validators.py", line 348, in resolve_from_url
    return self.resolve_fragment(document, fragment)
  File "/nail/live/yelp/virtualenv_run/local/lib/python2.7/site-packages/jsonschema/validators.py", line 375, in resolve_fragment
    "Unresolvable JSON pointer: %r" % fragment
RefResolutionError: Unresolvable JSON pointer: u'definitions/SurveyQuestion'

The reason why the ref is unresolvable is that it has no scope attached - we do this manually in deref_and_validate. This is important in the case of specs split across multiple files. The reason it has no scope is that we exit too early when we detect a cycle, not attaching the scope first. This is fine if the schema in question can be reached in other ways, but there's cases like the one I added to the test, where the answer property of the question definition will only ever be reached once, while following the response spec → answerquestion. If we don't attach a scope to the answer property jsonschema won't be able to deref it, as without the attached scope it will only have the root schema document, which does not contain the definition for answer.

This is not a problem for the rest of the spec in tests/data/v2.0/test_complicated_refs, as it puts paths and definitions into well-known root elements of the swagger spec, which we don't always do internally.

The test case I added causes tests/validator20/validate_spec_test.py::test_complicated_refs to fail on master, while it passes on my branch.

coveralls commented 4 years ago

Coverage Status

Coverage remained the same at 98.563% when pulling b6b5de6ddd8920409e34627537fae52bd7f9afc7 on sjaensch-fix-recursive-ref-resolution into 815666623440ea6bed20bbb599608d7416c2b315 on master.

macisamuele commented 4 years ago

I'll be checking this later this evening. Attaching the scope seems a good idea, but I'm curious on how we didn't noticed this before. Is that possible that we were essentially never validating this?

By the way I would check if you can reproduce the issue by using #/definitions/... instead of #definitions/... (if my memory is not wrong that can be triggering the issue)

sjaensch commented 4 years ago

Yes, due to #134 we were never validating array item schemas. I'll check later or tomorrow morning whether our specs use #/definitions/ or #definitions/, but I single-stepped through this issue and could observe that within the call to deref it only had the root swagger spec, and tried to find it within the definition section there. That's why we do the whole x-scope annotation work for jsonschema.