Support resolving OpenApiPaths entries from document #54847
Conversation
captainsafia
requested review from
a team,
halter73 and
brunolins16
as code owners
ghost
added
the
old-area-web-frameworks-do-not-use
captainsafia
added
area-mvc
| /// </summary> | ||
| /// <param name="apiDescription">The ApiDescription to resolve an operation type from.</param> | ||
| /// <returns>The <see cref="OperationType"/> associated with the given <paramref name="apiDescription"/>.</returns> | ||
| public static OperationType ToOperationType(this ApiDescription apiDescription) => |
There was a problem hiding this comment.
Nit: since it's apiDescription.HttpMethod and not apiDescription that's being converted, I might call this "GetOperationType".
| } | ||
| } | ||
| // "" -> "/" | ||
| if (routePattern.PathSegments.Count == 0) |
There was a problem hiding this comment.
It doesn't matter either way, but I actually had in mind strippedRoute.Length == 0.
There was a problem hiding this comment.
Maybe we should check this before the loop (and string builder allocation) and just return "/";?
There was a problem hiding this comment.
Maybe we should check this before the loop (and string builder allocation) and just return "/";?
Yep -- this is a good call.
| { | ||
| strippedRoute.Append('/'); | ||
| var segment = routePattern.PathSegments[i]; | ||
| foreach (var part in segment.Parts) |
There was a problem hiding this comment.
Is it possible for the segment to contain neither literal parts nor parameter parts? If so, you might end up with "//".
There was a problem hiding this comment.
From looking at the routing logic, the other possible option this could be is a RoutePatternSeparatorPart. According to the the doc string it looks like it's only used as the separator for dot-based separators that appear at the end of a route. In that case, we'll want to to use the content of the separator directly.
| /// the object to support filtering each | ||
| /// description instance into its appropriate document. | ||
| /// </remarks> | ||
| internal OpenApiPaths GetOpenApiPaths() |
There was a problem hiding this comment.
Why is this internal? I couldn't find a call outside this type.
There was a problem hiding this comment.
It was originally internal for testing but I ended up not invoking the GetOpenApiPaths method directly.
|
|
||
| // Act & Assert | ||
| var exception = Assert.Throws<InvalidOperationException>(() => apiDescription.ToOperationType()); | ||
| Assert.Equal("Unsupported HTTP method: UNKNOWN", exception.Message); |
There was a problem hiding this comment.
Nit: A name that isn't all uppercase would validate that the message gives the method string verbatim.
| ApplicationName = "TestApplication" | ||
| }; | ||
| var docService = new OpenApiDocumentService( | ||
| "v1", |
There was a problem hiding this comment.
Would it be worthwhile to test with a document name other than the default?
There was a problem hiding this comment.
Yep -- doesn't hurt to add another test here.
* Add support for generating OpenAPI info and paths * Address feedback * Fix sample and options injection * Address more feedback
Co-authored-by: Martin Costello <[email protected]> Co-authored-by: Rick Anderson <[email protected]> This PR adds support for OpenAPI document generation, sans schema generation to Microsoft.AspNetCore.OpenApi. Relevant changes are available in individual PRs: - Add entry-point APIs for OpenAPI support (#54789) - Support resolving OpenApiPaths entries from document (#54847) - Support generating OpenAPI operation and associated fields (#54903) - Add APIs for OpenAPI document transformers (#54935) - Add support for generating OpenAPI parameters (#55041) - Add support for generating OpenAPI responses (#55020) - Add support for generating OpenAPI request bodies (#55040)
This PR adds support for generating the
OpenApiPathsfrom a given set of ApiDescriptions.