-
Notifications
You must be signed in to change notification settings - Fork 47
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
Support for OpenAPI v3.1 #55
Conversation
Co-authored-by: James Hinshelwood <[email protected]>
Co-authored-by: James Hinshelwood <[email protected]>
Co-authored-by: James Hinshelwood <[email protected]>
Co-authored-by: James Hinshelwood <[email protected]>
Co-authored-by: James Hinshelwood <[email protected]>
Co-authored-by: James Hinshelwood <[email protected]>
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.
In summary: this is great stuff; let's move it forward. |
Also: you might want to run this through the repo I've used for testing: https://github.com/ahl/openapiv3-test |
/// object, the behavior is undefined. See the rules for resolving Relative | ||
/// References. | ||
#[serde(rename = "$ref", skip_serializing_if = "Option::is_none")] | ||
reference: Option<String>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please make this field public, so we continue to be able to use struct literal syntax for these!
/// object, the behavior is undefined. See the rules for resolving Relative | ||
/// References. | ||
#[serde(rename = "$ref", skip_serializing_if = "Option::is_none")] | ||
reference: Option<String>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please make this field public, so we continue to be able to use struct literal syntax for these!
/// Inline extensions to this object. Additional properties MAY omit the | ||
/// `x-` prefix within this object. | ||
#[serde(flatten)] | ||
pub extensions: IndexMap<String, serde_json::Value>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe this should be removed; schemars::schema::Schema
already includes extensions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dlowe - Can you clarify that? I don't see any mention of extensions in schemars::schema::Schema.
Meanwhile, it appears that the x-*
extension convetion is still part of the 3.1 schema.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SchemaObject
has a serde-flattened extensions
field here:
https://github.com/GREsau/schemars/blob/master/schemars/src/schema.rs#L165
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah. I've been unable to find this in the spec: Does jsonschema or schemars use the x-*
prefix?
If not, we should probably still support such functionality.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nevermind. I see that the functionality of the two is pretty much identical. So my last comment didn't make any sense. I agree it should probably go away.
@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 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 #55 (comment) |
@ahl - I will work on this today. One question regarding Security Schemes: In The SecuritySchemes Spec they casually mention
How do you propose we deal with this? (I think it's true for 3.0 as well) |
One more thing: Regarding upgrades, it seems the most idiomatic approach would be just to impl
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? |
I'm ambivalent about including conversion logic in this library (which in my mind is just serde (de)serialization implementations!), but if you do:
^ I agree with this :) |
I think we need to add extensions to each of the Agreed on the conversion approach. One more wrinkle for 3.1: the |
It seems to get a new version every couple of months, it was updated as recently as 6 days ago. 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. |
Ok. I't not quite ready, but it is close. I need to do something about the From<> for Schema. |
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. |
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.