openapiv3 icon indicating copy to clipboard operation
openapiv3 copied to clipboard

Support for OpenAPI v3.1

Open JamesHinshelwood opened this issue 3 years ago • 10 comments

Resolves #50 .

I'm leaving this as a draft for now so people can take a look at the approach and ideally try using this branch to see if it meets their needs. I'll be doing the same for a couple of weeks as I'm not yet convinced that this is the right way of adding 3.1 support.

Also note that lots of the lines changed in this PR were due to moving the 3.0 support to a separate sub-module. It would probably make sense to review the commits separately.

JamesHinshelwood avatar Feb 12 '22 10:02 JamesHinshelwood

I was skeptical of this approach, but seeing it in action, I'm warming to it. I haven't been through it in fine details, but I had a couple of high level points and questions.

  • I think a desirable outcome would be if this were compatible with past versions by default. I could see this as excessively limiting, but it would be nice if there were a way to introduce this compatibly into version 1.x of this crate, get some use, and then make an incompatible change with 2.0. For example, in lib.rs do a pub use v3_0::* and rename the multi-version enum to something like OpenAPIVersion or OpenAPIGeneric (neither of those names is great). Alternatively, I could imagine just telling people to s/openapiv3::/openapiv3::v3_0::/g and that would suffice...

  • What would you think of a "upgrade" function that would convert 3.0.x to 3.1? It seems pretty straightforward (https://www.openapis.org/blog/2021/02/16/migrating-from-openapi-3-0-to-3-1-0). For consumers of OpenAPI documents, putting into a canonical form seems very helpful.

  • One of the key benefits of OpenAPI v3.1 is convergence with JSON Schema. I really like that you're using schemars::schema::Schema here! I'm not sure, however, about flattening the untagged enum. I also think while we don't need to add extensions which are part of SchemaObject we will need to add example and discriminator (https://github.com/OAI/OpenAPI-Specification/blob/main/versions/3.1.0.md#fixed-fields-20).

  • With regard to this review, I appreciate the wrapping of comments, but perhaps we could pull that out to minimize the diffs?

  • There's a lot of code/comment in comment between v3_0 and v3_1. It might be worth considering how we share that code where it's literally identical e.g. via include! (since use self::* could replace use crate::v3_X::*).

In summary: this is great stuff; let's move it forward.

ahl avatar Mar 19 '22 17:03 ahl

Also: you might want to run this through the repo I've used for testing: https://github.com/ahl/openapiv3-test

ahl avatar Mar 19 '22 17:03 ahl

@JamesHinshelwood - Are you still available to carry this to victory? If not, I'd be happy to clone your branch and make the necessary changes.

rrichardson avatar May 21 '22 04:05 rrichardson

@rrichardson I think at this point it would be reasonable to build on this work if you have cycles. Please see my comments above if you're going to take a swing https://github.com/glademiller/openapiv3/pull/55#issuecomment-1073045225

ahl avatar May 21 '22 15:05 ahl

@ahl - I will work on this today.

One question regarding Security Schemes: In The SecuritySchemes Spec they casually mention

This object MAY be extended with Specification Extensions.

How do you propose we deal with this? (I think it's true for 3.0 as well)
This is of special interest to me, because my company uses openAPI to generate Protobuf, among other things, so we take advantage of the fact that we're not specifically tied to the limited set of Auth mechanisms that are "supported" by OpenAPI

rrichardson avatar May 23 '22 17:05 rrichardson

One more thing: Regarding upgrades, it seems the most idiomatic approach would be just to impl TryFrom<v3_0::Foo> for v3_1::Foo (From, I'm not sure if they're perfectly compatible) for every type, so that anyone upgrading could just

let my_old_schema: v3_0::OpenAPI = serde_json::frrom_str(&x).unwrap(); 
let my_new_schema: v3_1::OpenAPI = my_old_schema.try_into().unwrap(); 
let bytes = serde_json::to_vec(my_new_schema).unwrap();

I guess the question is: Where should we put such an impl? Part of me says that v3_1 and v3_0 shouldn't know about each other. The other part says that it should be acceptable if newer versions know about older versions.

Thoughts?

rrichardson avatar May 23 '22 18:05 rrichardson

I'm ambivalent about including conversion logic in this library (which in my mind is just serde (de)serialization implementations!), but if you do:

The other part says that it should be acceptable if newer versions know about older versions.

^ I agree with this :)

dlowe avatar May 23 '22 18:05 dlowe

How do you propose we deal with this?

I think we need to add extensions to each of the SecurityScheme variants; alternatives?

Agreed on the conversion approach.

One more wrinkle for 3.1: the schemars crate seems to be only sporadically maintained. I think it's fine to prototype against it, but before we publish a version that depends upon it, I think we should get clarity on its future... or fork the repo if it's a dead end.

ahl avatar May 23 '22 20:05 ahl

One more wrinkle for 3.1: the schemars crate seems to be only sporadically maintained.

It seems to get a new version every couple of months, it was updated as recently as 6 days ago. The PRs and Issues, while numerous, do seem to make some progress.

It does seem like it could use a couple more reviewers/committers, but it seems actively maintained.

I guess we'll see in a bit how it performs in the context of OpenAPI.

rrichardson avatar May 23 '22 23:05 rrichardson

Ok. I't not quite ready, but it is close. I need to do something about the From<> for Schema.
https://github.com/glademiller/openapiv3/pull/58

rrichardson avatar May 24 '22 05:05 rrichardson

Sorry I dropped this, thanks for picking it up @rrichardson - Your PR looks good to me FWIW. Since this PR is a subset of yours, I will close it.

JamesHinshelwood avatar Nov 25 '22 10:11 JamesHinshelwood