-
Notifications
You must be signed in to change notification settings - Fork 264
Adds OpenAPITreeNode for translating OpenAPI paths into a directory structure #547
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
Conversation
Merges origin into fork
Co-authored-by: DeVere Dyett <ddyett@microsoft.com>
Co-authored-by: DeVere Dyett <ddyett@microsoft.com>
Co-authored-by: DeVere Dyett <ddyett@microsoft.com>
Utils.CheckArgumentNullOrEmpty(label, nameof(label)); | ||
|
||
OpenApiUrlTreeNode root = new OpenApiUrlTreeNode("/"); | ||
|
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.
in our copy we init with empty string. I'm not suggesting any change here, I'm just curious about the reason behind the difference?
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 figured that the root path segment should ideally be a /
instead of just an empty string. In the Graph OpenAPI docs, at least, we represent it as such.
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.
In the powershell open api descriptions the servers already contain a trailing slash. I'm not sure whether this is just us or a standard practice in the community. What I'm wondering is whether starting with a slash here will lead to a double slash and/or to consumers of the open api lib to have to control for that?
The paths also always start with a slash.
If we look at pet store which is v2, they use host + base path with no trailing slash, but the paths also all start with a slash.
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.
In OpenAPI specs for both vers. 2.x and 3.x a trailing slash or lack of it thereof in the basePath
or servers
respectively has no effect. You can experiment with the pet store OpenAPI spec. in https://editor.swagger.io/
For path definitions, paths should always start with a /
. See OpenAPI spec for v2 and v3
path = path.Substring(1); | ||
} | ||
|
||
var segments = path.Split('/'); |
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.
remove empty entries?
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.
Well, we can. But in either case, it would still work okay, since we're checking for null or empty strings either way here: https://github.com/irvinesunday/OpenAPI.NET/blob/7d36d697524aeb43369477d3b297996381256045/src/Microsoft.OpenApi/Services/OpenApiUrlTreeNode.cs#L168
/// <summary> | ||
/// Dictionary of labels and Path Item objects that describe the operations available on a node. | ||
/// </summary> | ||
public IDictionary<string, OpenApiPathItem> PathItems { get; } = new Dictionary<string, OpenApiPathItem>(); |
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.
in our version path item is singular (not a dictionary). I'm not asking to change anything, I'm just curious about the motivation for the change.
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.
Having it as a dictionary of Labels and PathItem objects will enable us to easily represent the different OpenAPI operations exposed by a single path in e.g. both V1.0 and Beta versions of the Graph metadata. In this case, the versions (labels) will be dictionary keys.
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.
Thanks for providing more details. I'm not sure I follow entirely here. As beta and v1.0 are base paths for the API shouldn't we end up in a situation where we either have:
- a gigantic open api description with all the paths, everything starting with beta or v1
- two open api descriptions, one for beta, one for v1, with a basepath
In which case, why do we need that labeling capability?
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.
Both scenarios are feasible with this new structure.
Scenario 1: If for example you start with translating v1.0
OpenAPI doc. into an OpenApiUrlTreeNode
object, you can later attach the beta
document onto it. If a path in the beta
version has already been defined in the v1.0
version, no new path will be created, but rather its operations (OpenApiPathItem
) will be appended to the existing v1.0
path in the node, and this will be labelled as beta
(for example) within the PathItems
dictionary. Agora makes use of this scenario.
Scenario 2: If you want to have separate OpenApiUrlTreeNode
objects, one representing v1.0
and the other beta
, that's perfectly doable. In each case, all the segments for a given OpenApiUrlTreeNode
object will have their PathItems
dictionary key as the specified version passed in the Create(OpenApiDocument doc, string label)
method.
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.
perfect! thanks for going through all the details with me!
Co-authored-by: Vincent Biret <vincentbiret@hotmail.com>
Co-authored-by: Vincent Biret <vincentbiret@hotmail.com>
Proposes: