jhthorsen / json-validator

:cop: Validate data against a JSON schema
https://metacpan.org/release/JSON-Validator
56 stars 57 forks source link

Add schema classes for validating OpenAPIv2 and OpenAPIv3 request/responses #227

Closed jhthorsen closed 3 years ago

jhthorsen commented 3 years ago

It would make it a lot easier to maintain https://github.com/jhthorsen/mojolicious-plugin-openapi if JSON::Validator::OpenAPI::Mojolicious was split into JSON::Validator::Schema::OpenAPIv2 and JSON::Validator::Schema::OpenAPIv3.

This could also possibly make it possible for other frameworks to easily add support for validating OpenAPI input and output.

I started implementing this in https://github.com/mojolicious/json-validator/compare/draft/openapi. I've named the branch "draft/openapi", since I think there's too much going on to make it into a PR, but I think there are interesting ideas there. I think the starting point should be just an OpenAPIv2 (or OpenAPIv3) schema class with the following methods:

...leaving out the extra M::P::OpenAPI logic that can easily be added later on:

The implementation currently takes $c which could be a controller object, but to avoid a hard coupling to any framework the $c object need to implement the following API:

See also #189 - Add OpenAPI schema classes (PR).

mohawk2 commented 3 years ago

As discussed on IRC, I'm very pleased to see a splitting into OpenAPIv2 and v3.

I feel the only good solution for an interface between JV and web frameworks is one that has a complete separation between JV and any web framework. I gather you propose to pass to the validate method an object with a defined API. In Mojolicious that would be a controller object with methods defined by helpers. That creates a developer overhead of having to ensure an object has the right API, possibly having to create a new class. There is also a runtime cost in calling methods on such an object.

I feel a better, more explicit separation is instead to pass to the validate method a list of data-structures-or-closures (DSOC):

Any of those could instead be a callback (or closure) that would return the above.

You raised the point of "coercing" the request (especially by filling in defaults). This can and should be achieved by returning modified versions of any relevant data, probably a list of:

mohawk2 commented 3 years ago

I think it's very important that the API be as uniform as possible between the two OpenAPI versions. I'd like to see an "OpenAPI" class that takes the spec, and transparently returns an object of the right class (OpenAPIv2 or v3).

mohawk2 commented 3 years ago

Part of the API across the two versions would need to be the ability to introspect the spec in a higher-level fashion than just reading the data from the actual spec. Information that one would wish to get includes:

Motivating example: https://metacpan.org/pod/GraphQL::Plugin::Convert::OpenAPI

jhthorsen commented 3 years ago

Regarding "introspect the spec": Please leave this out for now, since it can be added later. I changed the topic to make it more obvious what this issue is about.

I've pushed a new branch now, which tries to (partially) implement the API you are asking for. I'm not sure if it will be possible, but I'm willing to persu the idea once more. Yes, I've been down that road multiple times before 🙈 I'm guessing the most interesting part for now is probably https://github.com/mojolicious/json-validator/compare/jhthorsen/openapi#diff-dfd1c10d78c2524bcfe5d0ab11fa48403ddf74f075b31143d159d0aa3fe5b808

One thing that the input to the methods are not an absolute URL, but rather [$http_method, $openapi_path]. The reason for that is that I don't want to add Mojolicious::Routes kind of functionality to be able to go from "https://.../api/v4/pets/cool-cat" to "/pets/{petId}".

I might open a "draft" PR later on to make it easier to comment, but for now I need to disprove my previous facts about why this approach haven't worked before, before I do so.

abraxxa commented 3 years ago

Regarding the framwork-agnostic API: there is already Plack::Request and HTTP::Request (and many more), can we reuse those?

Besides that we shouldn't forget that you might want to validate data not coming from an API call but for example a file that you want to validate against an OpenAPI schema. When this request object can be instantiated from such data too it would be sufficient. I'm willing to help as I want to integrate an OpenAPI (ideally v3) to one of my Catalyst apps.

jhthorsen commented 3 years ago

Regarding the framwork-agnostic API: there is already Plack::Request and HTTP::Request (and many more), can we reuse those?

No, they aren't compatible, and I don't want to depend on another web framework.

Besides that we shouldn't forget that you might want to validate data not coming from an API call but for example a file...

The schema classes only care about data structures, so there should be no issue here.

I'm willing to help as I want to integrate an OpenAPI (ideally v3) to one of my Catalyst apps.

You can try to write an adapter using the API described here: https://github.com/mojolicious/json-validator/blob/jhthorsen/openapiv3/lib/JSON/Validator/Schema/OpenAPIv3.pm#L315 and https://github.com/mojolicious/json-validator/blob/jhthorsen/openapiv3/lib/JSON/Validator/Schema/OpenAPIv2.pm#L338 (Schema::OpenAPIv3 inherit all the methods from Schema::OpenAPIv2)

abraxxa commented 3 years ago

I tried using it and fixed some issues in my local repo but wasn't able to find out how to validate data against one of the api endpoints defined in my OpenAPI 3.0 schema file. Can you please provide an example how to do that? Thanks!

jhthorsen commented 3 years ago

You can look at the openapiv2-*t and openapiv3-*t tests in https://github.com/mojolicious/json-validator/tree/jhthorsen/openapiv3/t

You can also have a look at the POD at https://github.com/mojolicious/json-validator/blob/jhthorsen/openapiv3/lib/JSON/Validator/Schema/OpenAPIv2.pm#L338

OpenAPIv2.pm and OpenAPIv3.pm share the same public API, even though they do things differently internally. (OpenAPIv3.pm also inherit all the methods from OpenAPIv2.pm)

abraxxa commented 3 years ago

I did that already but the docs and tests don't match. While checking I just found out that I was on the wrong branch! draft/openapi instead of jhthorsen/openapiv3!

jhthorsen commented 3 years ago

Please do let me know what is not matching between the tests and the docs.

abraxxa commented 3 years ago

Nothing, this was because of looking at the code of branch draft/openapi but docs of jhthorsen/openapiv3. I found what looks to be a bug. When I pass the body as hashref instead of a sub to validate_request it gets incorrectly returned by the generated sub. Should this be supported? All the tests use the internal representation { exists => 0|1, value => $value }.

jhthorsen commented 3 years ago

I've now pushed the new schema classes to master. Going to release them very soon.

jhthorsen commented 3 years ago

@abraxxa:

Not sure, but you might also find https://github.com/jhthorsen/mojolicious-plugin-openapi/blob/1adbaca4fcfe9684aba3bffa816cb21ad3680bff/lib/Mojolicious/Plugin/OpenAPI/Parameters.pm in https://github.com/jhthorsen/mojolicious-plugin-openapi/pull/160 interesting. It's the module that acts as glue between J::V and Mojolicious.