Skip to content
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

More thorough support for retrieving OpenAPI routes in a case-sensitive manner #59568

Open
wants to merge 10 commits into
base: main
Choose a base branch
from
Original file line number Diff line number Diff line change
Expand Up @@ -30,10 +30,12 @@ public static IEndpointConventionBuilder MapOpenApi(this IEndpointRouteBuilder e
var options = endpoints.ServiceProvider.GetRequiredService<IOptionsMonitor<OpenApiOptions>>();
return endpoints.MapGet(pattern, async (HttpContext context, string documentName = OpenApiConstants.DefaultDocumentName) =>
{
// We need to retrieve the document name in a case-insensitive manner
// to support case-insensitive document name resolution.
// Keyed Services are case-sensitive by default, which doesn't work well for document names in ASP.NET Core
// as routing in ASP.NET Core is case-insensitive by default.
// We need to retrieve the document name in a case-insensitive manner to support case-insensitive document name resolution.
// The document service is registered with a key equal to the document name, but in lowercase.
// The GetRequiredKeyedService() method is case-sensitive, which doesn't work well for OpenAPI document names here,
// as the document name is also used as the route to retrieve the document, so we need to ensure this is lowercased to achieve consistency with ASP.NET Core routing.
// The same goes for the document options below, which is also case-sensitive, and thus we need to pass in a case-insensitive document name.
// See OpenApiServiceCollectionExtensions.cs for more info.
var lowercasedDocumentName = documentName.ToLowerInvariant();

// It would be ideal to use the `HttpResponseStreamWriter` to
Expand All @@ -50,7 +52,7 @@ public static IEndpointConventionBuilder MapOpenApi(this IEndpointRouteBuilder e
else
{
var document = await documentService.GetOpenApiDocumentAsync(context.RequestServices, context.RequestAborted);
var documentOptions = options.Get(documentName);
var documentOptions = options.Get(lowercasedDocumentName);
sander1095 marked this conversation as resolved.
Show resolved Hide resolved
using var output = MemoryBufferWriter.Get();
using var writer = Utf8BufferTextWriter.Get(output);
try
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -57,10 +57,9 @@ public static IServiceCollection AddOpenApi(this IServiceCollection services, st
ArgumentNullException.ThrowIfNull(services);
ArgumentNullException.ThrowIfNull(configureOptions);

// We need to store the document name in a case-insensitive manner
// to support case-insensitive document name resolution.
// Keyed Services are case-sensitive by default, which doesn't work well for document names in ASP.NET Core
// as routing in ASP.NET Core is case-insensitive by default.
// We need to register the document name in a case-insensitive manner to support case-insensitive document name resolution.
// The document name is used to store and retrieve keyed services and configuration options, which are all case-sensitive.
// To achieve parity with ASP.NET Core routing, which is case-insensitive, we need to ensure the document name is lowercased.
var lowercasedDocumentName = documentName.ToLowerInvariant();

services.AddOpenApiCore(lowercasedDocumentName);
Expand Down Expand Up @@ -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>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can remove the docstrings on private methods.

/// <param name="documentName">Please ensure this is lowercased to prevent case-sensitive routing issues</param>
/// <remarks>See https://github.com/dotnet/aspnetcore/issues/59175 for more information around the routing issue mentioned above</remarks>
private static IServiceCollection AddOpenApiCore(this IServiceCollection services, string documentName)
{
services.AddEndpointsApiExplorer();
Expand Down
20 changes: 17 additions & 3 deletions src/OpenApi/src/Services/OpenApiDocumentProvider.cs
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can just reference the comment in the IServiceCollection extensions file instead of duplicating the content here.

// The options are registered with a key equal to the document name, but in lowercase.
// The options monitor's Get() method is case-sensitive, which doesn't work well for OpenAPI document names here,
// as the document name is also used as the route to retrieve the document, so we need to ensure this is lowercased to achieve consistency with ASP.NET Core routing.
// See OpenApiServiceCollectionExtensions.cs for more info.
var lowercasedDocumentName = documentName.ToLowerInvariant();

var options = serviceProvider.GetRequiredService<IOptionsMonitor<OpenApiOptions>>();
var namedOption = options.Get(documentName);
var namedOption = options.Get(lowercasedDocumentName);
var resolvedOpenApiVersion = namedOption.OpenApiVersion;
await GenerateAsync(documentName, writer, resolvedOpenApiVersion);
await GenerateAsync(lowercasedDocumentName, writer, resolvedOpenApiVersion);
}

/// <summary>
Expand All @@ -40,10 +47,17 @@ public async Task GenerateAsync(string documentName, TextWriter writer)
/// <param name="openApiSpecVersion">The OpenAPI specification version to use when serializing the document.</param>
public async Task GenerateAsync(string documentName, TextWriter writer, OpenApiSpecVersion openApiSpecVersion)
{
// We need to retrieve the document name in a case-insensitive manner to support case-insensitive document name resolution.
// The document service is registered with a key equal to the document name, but in lowercase.
// The GetRequiredKeyedService() method is case-sensitive, which doesn't work well for OpenAPI document names here,
// as the document name is also used as the route to retrieve the document, so we need to ensure this is lowercased to achieve consistency with ASP.NET Core routing.
// See OpenApiServiceCollectionExtensions.cs for more info.
var lowercasedDocumentName = documentName.ToLowerInvariant();

// Microsoft.OpenAPI does not provide async APIs for writing the JSON
// document to a file. See https://github.com/microsoft/OpenAPI.NET/issues/421 for
// more info.
var targetDocumentService = serviceProvider.GetRequiredKeyedService<OpenApiDocumentService>(documentName);
var targetDocumentService = serviceProvider.GetRequiredKeyedService<OpenApiDocumentService>(lowercasedDocumentName);
using var scopedService = serviceProvider.CreateScope();
var document = await targetDocumentService.GetOpenApiDocumentAsync(scopedService.ServiceProvider);
var jsonWriter = new OpenApiJsonWriter(writer);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
using Microsoft.OpenApi.Models;
using Microsoft.OpenApi.Readers;
using System.Text;
using Microsoft.OpenApi;

public class OpenApiEndpointRouteBuilderExtensionsTests : OpenApiDocumentServiceTestBase
{
Expand Down Expand Up @@ -156,6 +157,42 @@ public async Task MapOpenApi_ReturnsDocumentWhenPathIsCaseSensitive(string regis
Assert.Equal(StatusCodes.Status200OK, context.Response.StatusCode);
}

[Fact]
public async Task MapOpenApi_ShouldRetrieveOptionsInACaseInsensitiveManner()
{
// Arrange
var hostEnvironment = new HostEnvironment() { ApplicationName = nameof(OpenApiEndpointRouteBuilderExtensionsTests) };
var serviceProviderIsService = new ServiceProviderIsService();
var serviceProvider = CreateServiceProvider("casesensitive", OpenApiSpecVersion.OpenApi2_0);
var builder = new DefaultEndpointRouteBuilder(new ApplicationBuilder(serviceProvider));
builder.MapOpenApi("/openapi/{documentName}.json");
var context = new DefaultHttpContext();
var responseBodyStream = new MemoryStream();
context.Response.Body = responseBodyStream;
context.RequestServices = serviceProvider;
context.Request.RouteValues.Add("documentName", "CaseSensitive");
var endpoint = builder.DataSources.First().Endpoints[0];

// Act
var requestDelegate = endpoint.RequestDelegate;
await requestDelegate(context);

// Assert
Assert.Equal(StatusCodes.Status200OK, context.Response.StatusCode);
var responseString = Encoding.UTF8.GetString(responseBodyStream.ToArray());

// When we receive an OpenAPI document, we use an OptionsMonitor to retrieve OpenAPI options which are stored with a key equal the requested document name.
// This key is case-sensitive. If the document doesn't exist, the options monitor return a default instance, in which the OpenAPI version is set to v3.
// This could cause bugs! You'd get your document, but depending on the casing you used in the document name you passed to the function, you'll receive different OpenAPI document versions.
// We want to prevent this from happening. Therefore:
// By setting up a v2 document on the "casesensitive" route and requesting it on "CaseSensitive",
// we can test that the we've configured the options monitor to retrieve the options in a case-insensitive manner.
// If it is case-sensitive, it would return a default instance with OpenAPI version v3, which would cause this test to fail!
// However, if it would return the v2 instance, which was configured on the lowercase - case-insensitive - documentname, the test would pass!
// For more info, see OpenApiEndpointRouteBuilderExtensions.cs
Assert.StartsWith("{\n \"swagger\": \"2.0\"", responseString);
}

[Theory]
[InlineData("/openapi.json", "application/json;charset=utf-8", false)]
[InlineData("/openapi.yaml", "text/plain+yaml;charset=utf-8", true)]
Expand Down Expand Up @@ -201,7 +238,7 @@ private static void ValidateOpenApiDocument(MemoryStream documentStream, Action<
action(document);
}

private static IServiceProvider CreateServiceProvider(string documentName = Microsoft.AspNetCore.OpenApi.OpenApiConstants.DefaultDocumentName)
private static IServiceProvider CreateServiceProvider(string documentName = Microsoft.AspNetCore.OpenApi.OpenApiConstants.DefaultDocumentName, OpenApiSpecVersion openApiSpecVersion = OpenApiSpecVersion.OpenApi3_0)
{
var hostEnvironment = new HostEnvironment() { ApplicationName = nameof(OpenApiEndpointRouteBuilderExtensionsTests) };
var serviceProviderIsService = new ServiceProviderIsService();
Expand All @@ -210,7 +247,7 @@ private static IServiceProvider CreateServiceProvider(string documentName = Micr
.AddSingleton<IHostEnvironment>(hostEnvironment)
.AddSingleton(CreateApiDescriptionGroupCollectionProvider())
.AddSingleton<ILoggerFactory>(NullLoggerFactory.Instance)
.AddOpenApi(documentName)
.AddOpenApi(documentName, x => x.OpenApiVersion = openApiSpecVersion)
.BuildServiceProvider();
return serviceProvider;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
using Microsoft.Extensions.ApiDescriptions;
using Microsoft.Extensions.DependencyInjection;
using Microsoft.Extensions.Hosting;
using Microsoft.OpenApi;
using Microsoft.OpenApi.Models;
using Microsoft.OpenApi.Readers;
using static Microsoft.AspNetCore.OpenApi.Tests.OpenApiOperationGeneratorTests;
Expand All @@ -30,6 +31,56 @@ public async Task GenerateAsync_ReturnsDocument()
});
}

[Fact]
public async Task GenerateAsync_ShouldRetrieveOptionsInACaseInsensitiveManner()
{
// Arrange
var documentName = "CaseSensitive";
var serviceProvider = CreateServiceProvider(["casesensitive"], OpenApiSpecVersion.OpenApi2_0);
var documentProvider = new OpenApiDocumentProvider(serviceProvider);
var stringWriter = new StringWriter();

// Act
await documentProvider.GenerateAsync(documentName, stringWriter);

// Assert
var document = stringWriter.ToString();

// When we generate an OpenAPI document, we use an OptionsMonitor to retrieve OpenAPI options which are stored with a key equal the requested document name.
// This key is case-sensitive. If the document doesn't exist, the options monitor return a default instance, in which the OpenAPI version is set to v3.
// This could cause bugs! You'd get your document, but depending on the casing you used in the document name you passed to the function, you'll receive different OpenAPI document versions.
// We want to prevent this from happening. Therefore:
// By setting up a v2 document on the "casesensitive" route and requesting it on "CaseSensitive",
// we can test that the we've configured the options monitor to retrieve the options in a case-insensitive manner.
// If it is case-sensitive, it would return a default instance with OpenAPI version v3, which would cause this test to fail!
// However, if it would return the v2 instance, which was configured on the lowercase - case-insensitive - documentname, the test would pass!
Assert.StartsWith("{\n \"swagger\": \"2.0\"", document);
}

[Fact]
public async Task GenerateAsync_ShouldRetrieveOpenApiDocumentServiceWithACaseInsensitiveKey()
{
// Arrange
var documentName = "CaseSensitive";
var serviceProvider = CreateServiceProvider(["casesensitive"]);
var documentProvider = new OpenApiDocumentProvider(serviceProvider);
var stringWriter = new StringWriter();

// Act
await documentProvider.GenerateAsync(documentName, stringWriter, OpenApiSpecVersion.OpenApi3_0);

// Assert

// If the Document Service is retrieved with a non-existent (case-sensitive) key, it would throw:
// https://learn.microsoft.com/en-us/dotnet/api/microsoft.extensions.dependencyinjection.serviceproviderkeyedserviceextensions.getrequiredkeyedservice?view=net-9.0-pp

// In this test's case, we're testing that the document service is retrieved with a case-insensitive key.
// 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, _ => { });
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: Add an assertion here to validate the OpenAPI version as above?

}

[Fact]
public void GetDocumentNames_ReturnsAllRegisteredDocumentName()
{
Expand All @@ -56,7 +107,7 @@ private static void ValidateOpenApiDocument(StringWriter stringWriter, Action<Op
action(document);
}

private static IServiceProvider CreateServiceProvider(string[] documentNames)
private static IServiceProvider CreateServiceProvider(string[] documentNames, OpenApiSpecVersion openApiSpecVersion = OpenApiSpecVersion.OpenApi3_0)
{
var hostEnvironment = new HostEnvironment() { ApplicationName = nameof(OpenApiDocumentProviderTests) };
var serviceProviderIsService = new ServiceProviderIsService();
Expand All @@ -66,7 +117,7 @@ private static IServiceProvider CreateServiceProvider(string[] documentNames)
.AddSingleton(CreateApiDescriptionGroupCollectionProvider());
foreach (var documentName in documentNames)
{
serviceCollection.AddOpenApi(documentName);
serviceCollection.AddOpenApi(documentName, x => x.OpenApiVersion = openApiSpecVersion);
}
var serviceProvider = serviceCollection.BuildServiceProvider(validateScopes: true);
return serviceProvider;
Expand Down
Loading