More thorough support for retrieving OpenAPI routes in a case-sensitive manner #59568
Conversation
dotnet-issue-labeler
bot
added
the
needs-area-label
dotnet-policy-service
bot
added
the
community-contribution
martincostello
added
feature-openapi
area-minimal
…ument name for OpenAPI version retrieval
There was a problem hiding this comment.
Thanks for catching this and following up with a PR!
This has got me thinking that we should audit other places where we pass in documentName in the same way. As a result, it looks like we'll also want to make a change in OpenApiDocumentProvider. The same test around the API version should work here.
sander1095
changed the title
|
@captainsafia I've implemented your requested changes, added extra comments, improved existing ones and updated the title and description of PR to better reflect the changes. Let me know if you want me to do anything else! :) |
| @@ -106,6 +105,9 @@ public static IServiceCollection AddOpenApi(this IServiceCollection services, Ac | |||
| public static IServiceCollection AddOpenApi(this IServiceCollection services) | |||
| => services.AddOpenApi(OpenApiConstants.DefaultDocumentName); | |||
|
|
|||
| /// <param name="services">The <see cref="IServiceCollection"/> to register services onto.</param> | |||
There was a problem hiding this comment.
We can remove the docstrings on private methods.
| @@ -25,10 +25,17 @@ internal sealed class OpenApiDocumentProvider(IServiceProvider serviceProvider) | |||
| /// <param name="writer">A text writer associated with the document to write to.</param> | |||
| public async Task GenerateAsync(string documentName, TextWriter writer) | |||
| { | |||
| // We need to retrieve the document name in a case-insensitive manner to support case-insensitive document name resolution. | |||
There was a problem hiding this comment.
I think we can just reference the comment in the IServiceCollection extensions file instead of duplicating the content here.
| // It's registered as "casesensitive" but we're passing in "CaseSensitive", which doesn't exist. | ||
| // Therefore, if the test doesn't throw, we know it has passed correctly. | ||
| // We still do a small check to validate the document, just in case. But the main test is that it doesn't throw. | ||
| ValidateOpenApiDocument(stringWriter, _ => { }); |
There was a problem hiding this comment.
Nit: Add an assertion here to validate the OpenAPI version as above?
|
Thanks for the review! I'll implement requested changes after my holiday (in ~2 weeks) |
More thorough support for retrieving OpenAPI routes in a case-sensitive manner
This PR fixes some oversights from #59199 around retrieving OpenAPI documents case-insensitively.
Description
This PR modifies missed files from #59199 that use the registered document name as a key for retrieving services or options. This needs to be done case-insensitively to:
OpenApiOptionsis returned, which might contain different settings than what the user had configured.This PR also adds some more comments and improves existing ones based on the previous PR around case-insensitive OpenAPI document names.
Fixes #59175