-
Notifications
You must be signed in to change notification settings - Fork 240
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
fix/reference proxy single copy #2031
base: dev
Are you sure you want to change the base?
Conversation
Signed-off-by: Vincent Biret <[email protected]>
Signed-off-by: Vincent Biret <[email protected]>
Signed-off-by: Vincent Biret <[email protected]>
Signed-off-by: Vincent Biret <[email protected]>
Signed-off-by: Vincent Biret <[email protected]>
Signed-off-by: Vincent Biret <[email protected]>
Signed-off-by: Vincent Biret <[email protected]>
Signed-off-by: Vincent Biret <[email protected]>
Signed-off-by: Vincent Biret <[email protected]>
Signed-off-by: Vincent Biret <[email protected]>
Signed-off-by: Vincent Biret <[email protected]>
Signed-off-by: Vincent Biret <[email protected]>
/// <param name="openApiSchema">The schema to add</param> | ||
/// <param name="id">The id for the component</param> | ||
/// <returns>Whether the schema was added to the components.</returns> | ||
public bool AddComponentSchema(string id, OpenApiSchema openApiSchema) |
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.
Is this not relevant for all types of components? Should we just have a AddComponent method with overloads for the different types of components?
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'm iterating here, so far the only one that was problematic in the context of yoko was the schemas.
We also need to think about making collections read only maybe, and adding a method to remove entries, so everything is in sync.
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.
We already have an extension method for registering components into a document's workspace(components registry). Should we just reuse these?
OpenAPI.NET/src/Microsoft.OpenApi/Services/OpenApiWorkspace.cs
Lines 62 to 139 in cce2495
public void RegisterComponents(OpenApiDocument document) | |
{ | |
if (document?.Components == null) return; | |
string baseUri = document.BaseUri + OpenApiConstants.ComponentsSegment; | |
string location; | |
// Register Schema | |
foreach (var item in document.Components.Schemas) | |
{ | |
location = item.Value.Id ?? baseUri + ReferenceType.Schema.GetDisplayName() + "/" + item.Key; | |
RegisterComponent(location, item.Value); | |
} | |
// Register Parameters | |
foreach (var item in document.Components.Parameters) | |
{ | |
location = baseUri + ReferenceType.Parameter.GetDisplayName() + "/" + item.Key; | |
RegisterComponent(location, item.Value); | |
} | |
// Register Responses | |
foreach (var item in document.Components.Responses) | |
{ | |
location = baseUri + ReferenceType.Response.GetDisplayName() + "/" + item.Key; | |
RegisterComponent(location, item.Value); | |
} | |
// Register RequestBodies | |
foreach (var item in document.Components.RequestBodies) | |
{ | |
location = baseUri + ReferenceType.RequestBody.GetDisplayName() + "/" + item.Key; | |
RegisterComponent(location, item.Value); | |
} | |
// Register Links | |
foreach (var item in document.Components.Links) | |
{ | |
location = baseUri + ReferenceType.Link.GetDisplayName() + "/" + item.Key; | |
RegisterComponent(location, item.Value); | |
} | |
// Register Callbacks | |
foreach (var item in document.Components.Callbacks) | |
{ | |
location = baseUri + ReferenceType.Callback.GetDisplayName() + "/" + item.Key; | |
RegisterComponent(location, item.Value); | |
} | |
// Register PathItems | |
foreach (var item in document.Components.PathItems) | |
{ | |
location = baseUri + ReferenceType.PathItem.GetDisplayName() + "/" + item.Key; | |
RegisterComponent(location, item.Value); | |
} | |
// Register Examples | |
foreach (var item in document.Components.Examples) | |
{ | |
location = baseUri + ReferenceType.Example.GetDisplayName() + "/" + item.Key; | |
RegisterComponent(location, item.Value); | |
} | |
// Register Headers | |
foreach (var item in document.Components.Headers) | |
{ | |
location = baseUri + ReferenceType.Header.GetDisplayName() + "/" + item.Key; | |
RegisterComponent(location, item.Value); | |
} | |
// Register SecuritySchemes | |
foreach (var item in document.Components.SecuritySchemes) | |
{ | |
location = baseUri + ReferenceType.SecurityScheme.GetDisplayName() + "/" + item.Key; | |
RegisterComponent(location, item.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.
@darrelmiller I've just updated the methods to take into account all the component types.
@MaggieKimani1 no, this method registers everything at once. This is problematic because ordering might matter in some contexts (typically OData), and it results in a perf hit when wanting to register only a few components. Also, it's only accessible on the workspace, my goal was to make it accessible on the document for better discovery.
Signed-off-by: Vincent Biret <[email protected]>
Signed-off-by: Vincent Biret <[email protected]>
Quality Gate failedFailed conditions |
partial #1998