Skip to content
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

Factor out OpenAPI validation core for reusability #12

Closed
sisp opened this issue Apr 24, 2019 · 5 comments · Fixed by #14

Comments

@sisp
Copy link
Contributor

commented Apr 24, 2019

falcon-oas is for the most part a general OpenAPI validation library with a Falcon integration. Would you be open to factoring out the general OpenAPI validation core into an independent library? This way, other web frameworks like Flask can leverage this common core library, too. In my opinion, the community should agree on a common OpenAPI core library and add thin integrations with the different web frameworks leveraging the common core library instead of writing an extension specific to a particular web framework that includes the core logic all over again.

Some context:

I've been trying out several OpenAPI libraries including falcon-oas over the past year or so and noticed that efforts are scattered across the different libraries (with varying quality) although they should all be sharing a common core. I think you've done a terrific job in creating a solid validation core library leveraging jsonschema which is a proven JSON schema library. Not all OpenAPI validation libraries use jsonschema, e.g. openapi-core. Some do but don't support OpenAPI 3, e.g. bravado-core, or tightly couple the validation core with a particular web framework, e.g. connexion. falguard is quite similar to falcon-oas but builds upon bravado-core which doesn't seem to get OpenAPI 3 support anytime soon. Also, most OpenAPI libraries only report the first validation error whereas falcon-oas reports all, which I personally prefer. And then there are libraries that generate OpenAPI specs from inline annotations like flask-restplus, quart-openapi, flasgger etc., all of which implement the OpenAPI core logic all over again. This repeated core logic implementation also leads to slow adoption of OpenAPI 3 because each library needs to make the necessary changes.

@grktsh

This comment has been minimized.

Copy link
Owner

commented Apr 25, 2019

Common OpenAPI core library is great but I'm not sure if leveraging jsonschem is great. It made OpenAPI request validation easy but it made oneOf and anyOf unmarshaling inefficient:

for sub_schema in schema.get('oneOf') or schema.get('anyOf') or []:
    try:
        # TODO: Remove duplicate validation
        return self.unmarshal(instance, sub_schema)
    except ValidationError:
        pass

I think own validator is needed for that. Any thoughts?

Would you like to start from falcon_oas.oas codebase and replace jsonschem with own validator if necessary, or contribute to openapi-core from the beginning?

@sisp

This comment has been minimized.

Copy link
Contributor Author

commented Apr 25, 2019

I see your point about unmarshaling oneOf/anyOf being inefficient. I wonder how big a performance hit this re-validation really is in practice. It's more important that it works correctly than squeezing out a bit more performance, in my opinion.

I think own validator is needed for that.

Are you thinking along the lines of this?

I'm wondering whether it makes sense in this particular library to let a custom jsonschema validator for oneOf/anyOf mutate the schema so that unmarshaling uses a schema whose oneOf/anyOf fields have been resolved based on the instance. In the jsonschema FAQ, they talk about setting a default value in the instance. For this to work, I think $refs need to be dereferenced/inlined because oneOf/anyOf in a referenced schema may be resolved differently in the different places where that schema is referenced. Just a thought how unmarshaling oneOf/anyOf could become more efficient. But as I said, I'm not sure whether the performance hit is that big and it's much more important that the behavior is correct.

openapi-core does not use jsonschema but implements JSON Schema validation from scratch. I'm not convinced that this is a good/scalable approach because a lot of effort has been put into jsonschema to create a correct validator that supports the complete spec (and even different revisions). Looking at openapi-core's currently open issues, I'm seeing several that are purely related to JSON Schema validation. In addition, openapi-core only returns the first error it encounters rather than all errors which I'm not happy with. The openapi-core owner says "I work on new jsonschema validation process that will fix the issue.", but (1) does he literally mean jsonschema and (2) when will this change become available? If he indeed intends to use jsonschema in the future, then that will be at best what falcon_oas.oas is already offering now with 100% test coverage.

Based on my impression of falcon-oas so far, I think it's the most solid OpenAPI implementation for Python out there. It's well tested, relies on jsonschema (which I consider a good decision), and falcon_oas.oas is already separate from the Falcon integration. I'd pull out falcon_oas.oas into an independent library (oas happens to be still available on PyPI ;-)) and make falcon-oas depend on oas. Down the road, I'd provide at least a Flask integration in addition to falcon-oas to reach a larger community that may be interested in helping maintain oas.

@grktsh

This comment has been minimized.

Copy link
Owner

commented Apr 26, 2019

Thank you for sharing your thoughts. I will factor out falcon_oas.oas.

@grktsh

This comment has been minimized.

Copy link
Owner

commented Apr 26, 2019

I extracted OpenAPI 3 validation from falcon_oas.oas to https://github.com/grktsh/python-oas and released v0.1.0. Could you check it, please?
And #14 is a PR for this issue.

@sisp

This comment has been minimized.

Copy link
Contributor Author

commented Apr 27, 2019

Awesome! #14 looks good to me, I think you can merge it.

@grktsh grktsh closed this in #14 Apr 27, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.