Skip to content

Stop assigning a version to our Validator #12

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Mar 25, 2021

Conversation

jparise
Copy link
Contributor

@jparise jparise commented Mar 24, 2021

By giving our created Validator a version, we're also registering it as
one of the global jsonschema validators. From create()'s documentation:

version (str) – an identifier for the version that this validator
class will validate. If provided, the returned validator class will
have its __name__ set to include the version, and also will have
jsonschema.validators.validates automatically called for the given
version.

Why wouldn't we want to do this?

First, this Validator effectively replaces the existing Draft4Validator
because they're both based on the "draft4" schema. Internally, that
means both will be assigned the 'http://json-schema.org/draft-04/schema'
meta-schema, and the second one to be created (ours) will replace the
first in the jsonschema.validators.meta_schemas dictionary.

I would have expected jsonschema to use the schema's 'id' property to
differentiate between them, but it doesn't appear to work that way, and
I don't know enough about JSON Schema to go further.

Second, some of the OAS30 validators assigned here assume that the
validator instance is of type OAS3Validator, and that's only true when
jsonschema.validate() is explicitly called with cls=OAS30Validator.

For example, required() expects the given validator to have .read and
.write attributes, and those are Validator extensions provide by the
OAS30Validator subclass.

This means that a generic call to jsonschema.validate() that attempts to
use one of these validators (because of the global registration caused
by the first issue above) will produce AttributeError exceptions:

/usr/local/lib/python3.7/dist-packages/jsonschema/validators.py line 932 in validate
  error = exceptions.best_match(validator.iter_errors(instance))
/usr/local/lib/python3.7/dist-packages/jsonschema/exceptions.py line 367 in best_match
  best = next(errors, None)
/usr/local/lib/python3.7/dist-packages/jsonschema/validators.py line 328 in iter_errors
  for error in errors:
/usr/local/lib/python3.7/dist-packages/openapi_schema_validator/_validators.py line 46 in required
  if validator.write and read_only or validator.read and write_only:

AttributeError: 'Oas30Validator' object has no attribute 'write'

This AttributeError behavior could be improved by adding isinstance()
checks (or similar) to those validator function implementations, but I
think the greater issue is the fact that these validators are really
only meant to be used with OAS30Validator, and the global registration
pattern doesn't support a Validator subclass.

Hence, my proposed solution is to stop registering BaseOAS30Validator as
a global jsonschema validator, at least not until some of the underlying
behaviors described above can be addressed in openapi_schema_validator
or in jsonschema itself.

By giving our created Validator a version, we're also registering it as
one of the global jsonschema validators. From create()'s documentation:

    version (str) – an identifier for the version that this validator
    class will validate. If provided, the returned validator class will
    have its __name__ set to include the version, and also will have
    jsonschema.validators.validates automatically called for the given
    version.

Why wouldn't we want to do this?

First, this Validator effectively replaces the existing Draft4Validator
because they're both based on the "draft4" schema. Internally, that
means both will be assigned the 'http://json-schema.org/draft-04/schema'
meta-schema, and the second one to be created (ours) will replace the
first in the `jsonschema.validators.meta_schemas` dictionary.

I would have expected jsonschema to use the schema's 'id' property to
differentiate between them, but it doesn't appear to work that way, and
I don't know enough about JSON Schema to go further.

Second, some of the OAS30 validators assigned here assume that the
validator instance is of type OAS3Validator, and that's only true when
jsonschema.validate() is explicitly called with `cls=OAS30Validator`.

For example, required() expects the given validator to have `.read` and
`.write` attributes, and those are Validator extensions provide by the
OAS30Validator subclass.

This means that a generic call to jsonschema.validate() that attempts to
use one of these validators (because of the global registration caused
by the first issue above) will produce AttributeError exceptions:

    /usr/local/lib/python3.7/dist-packages/jsonschema/validators.py line 932 in validate
      error = exceptions.best_match(validator.iter_errors(instance))
    /usr/local/lib/python3.7/dist-packages/jsonschema/exceptions.py line 367 in best_match
      best = next(errors, None)
    /usr/local/lib/python3.7/dist-packages/jsonschema/validators.py line 328 in iter_errors
      for error in errors:
    /usr/local/lib/python3.7/dist-packages/openapi_schema_validator/_validators.py line 46 in required
      if validator.write and read_only or validator.read and write_only:
   AttributeError: 'Oas30Validator' object has no attribute 'write'

This AttributeError behavior could be improved by adding isinstance()
checks (or similar) to those validator function implementations, but I
think the greater issue is the fact that these validators are really
only meant to be used with OAS30Validator, and the global registration
pattern doesn't support a Validator subclass.

Hence, my proposed solution is to stop registering BaseOAS30Validator as
a global jsonschema validator, at least not until some of the underlying
behaviors described above can be addressed in openapi_schema_validator
or in jsonschema itself.
@codecov
Copy link

codecov bot commented Mar 24, 2021

Codecov Report

Merging #12 (0a035d6) into master (a1e3459) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master      #12   +/-   ##
=======================================
  Coverage   59.89%   59.89%           
=======================================
  Files           6        6           
  Lines         182      182           
=======================================
  Hits          109      109           
  Misses         73       73           
Impacted Files Coverage Δ
openapi_schema_validator/validators.py 100.00% <ø> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a1e3459...0a035d6. Read the comment docs.

@p1c2u
Copy link
Collaborator

p1c2u commented Mar 25, 2021

Hi @jparise

that's interesting. I wasn't aware of that. Indeed Draft4 and Draft Wright 00 (Draft 5) share the same meta schema

https://json-schema.org/specification-links.html#table-of-all-versions-of-everything

and AFAIK there is not OAS 3.0 specific meta-schema. You solution is okay for the time being. I would just add comment with link to the issue.

@p1c2u p1c2u merged commit 39f6fa4 into python-openapi:master Mar 25, 2021
@jparise jparise deleted the validator-version branch March 25, 2021 17:39
@jparise
Copy link
Contributor Author

jparise commented Mar 25, 2021

Adding a note is a good idea. Thanks for doing that!

Would you consider making a release with this change in the next few days? It would help us resolve the root issue, which has resulted in some runtime conflicts in other places where we're using jsonschema validation.

@p1c2u
Copy link
Collaborator

p1c2u commented Mar 25, 2021

Sure, I will make a release.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants