dotnet / aspnetcore

ASP.NET Core is a cross-platform .NET framework for building modern cloud-based web applications on Windows, Mac, or Linux.
https://asp.net
MIT License
35.11k stars 9.91k forks source link

`RequestDelegate`-based handlers do not surface in OpenAPI descriptions #44970

Open YataoFeng opened 1 year ago

YataoFeng commented 1 year ago

Is there an existing issue for this?

Describe the bug

hi I found that the minimal Api TypedResults method does not automatically inject HttpContext. The httpcontext principle is here in the reference https://learn.microsoft.com/en-us/aspnet/core/fundamentals/minimal-apis?view=aspnetcore-7.0#binding-precedence

Expected Behavior

The TypedResults method can automatically inject HttpContext.

Steps To Reproduce

File:Program.cs

var builder = WebApplication.CreateBuilder(args);

builder.Services.AddEndpointsApiExplorer();
builder.Services.AddSwaggerGen();

var app = builder.Build();

if (app.Environment.IsDevelopment())
{
    app.UseSwagger();
    app.UseSwaggerUI();
}

app.MapGroup("/connect").MapConnect();

app.Run();

File:Connect.cs

public static class Connect
{
    public static RouteGroupBuilder MapConnect(this RouteGroupBuilder group)
    {
        group.MapGet("/authorize", AuthorizeAsync);
        group.MapGet("/GoodMinimalApi", GoodMinimalApi);
        return group;
    }
    public static async Task<IResult> GoodMinimalApi() // not HttpContext
    {
        await Task.CompletedTask;
        return TypedResults.Ok();
    }
    public static async Task<IResult> AuthorizeAsync(HttpContext context)
    {
        await Task.CompletedTask;
        return TypedResults.Ok();
    }
}

End result

image

Exceptions (if any)

No response

.NET Version

dotnet 7.0

Anything else?

Microsoft Visual Studio Enterprise 2022 (64 位) - Current版本 17.4.0 ASP.NET Core 7 Minimal Api

quan0zhou commented 1 year ago

This is a problem with Swashbuckle.AspNetCore

YataoFeng commented 1 year ago

This is a problem with Swashbuckle.AspNetCore

3q

halter73 commented 1 year ago

Does updating

group.MapGet("/authorize", AuthorizeAsync);

to

group.MapGet("/authorize", (Delegate)AuthorizeAsync);

fix your issue?

It looks like you're getting the RequestDelegate overload of MapGet when you need the Delegate overload in order to process the Task<T> (specifically Task<IResult>) overload. @JamesNK added an analyzer to warn about this and recommend this workaround described https://github.com/dotnet/aspnetcore/issues/44316.

This didn't get added in time for the .NET 7 release though. Is this something we could backport to the .NET 7 SDK in a servicing release? I think there could be false positives, so that might be risky.

We wanted to just make this work (see see https://github.com/dotnet/aspnetcore/pull/40235), but this caused issues for trimming and AOT. Using source generators, we might be able to just make this work once a gain but in a way that's trim friendly.

halter73 commented 1 year ago

Also, if you want to get the best swagger output for TypedResults, you want to include the specific TypedResult in the return type. Task<IResult> loses information. Task<Ok> indicates that the route handler will return a 200 response. This might be the default assumption, but this becomes more important with different status codes (e.g. Task<NotFound> or Task<Results<Ok, NotFound>>) or with a specific schema (e.g. Task<Ok<MyDto>>).

So you should prefer

public static async Task<Ok> GoodMinimalApi()

over

public static async Task<IResult> GoodMinimalApi()

You can read more about TypedResults and Results<TResult1, ..., TResultN> at https://learn.microsoft.com/en-us/aspnet/core/fundamentals/minimal-apis/responses?view=aspnetcore-7.0#resultstresult1-tresultn.

captainsafia commented 1 year ago

This is a problem with Swashbuckle.AspNetCore

This issue actually starts in ASP.NET Core. Our OpenAPI infrastructure relies on MethodInfo being available in EndpointMetadata. However, as mentioned above, since your code is resolving to the RequestDelegate overload of MapGet, we explicitly don't add method info to the metadata in this case.

In .NET 7 RC1, we actually enabled adding MethodInfo to metadata for all types of endpoints (ones using RequestDelegate or Delegate) but we got feedback that this was resulting in undesired behavior (see https://github.com/dotnet/aspnetcore/issues/44005) so we undid it.

On solution we discussed was making it so that calling WithOpenApi on an endpoint would add it to OpenAPI definition regardless of the delegate type. This would mean your problem would be solved by calling group.MapGet("/authorize", AuthorizeAsync).WithOpenApi(); or app.MapGroup("/connect").MapConnect().WithOpenApi();.

Does that seem like a sensible solution?

cc: @martincostello for thoughts as well

martincostello commented 1 year ago

I like the idea of making it opt-in via something like WithOpenApi().

My main concern (and how I found #44005) was that third party code might start exposing unintended endpoints that are difficult to suppress from the OpenAPI document (if you notice them even appear) if you can't update the source of the endpoint to mark it has hidden.

captainsafia commented 1 year ago

I like the idea of making it opt-in via something like WithOpenApi().

My main concern (and how I found #44005) was that third party code might start exposing unintended endpoints that are difficult to suppress from the OpenAPI document (if you notice them even appear) if you can't update the source of the endpoint to mark it has hidden.

Thanks for the feedback! Yeah, I'm thinking that by leveraging WithOpenApi as an opt-in and introducing support for https://github.com/dotnet/aspnetcore/issues/44192, there will be flexibility to include and omit these RequestDelegate-based endpoints as needed.

ghost commented 1 year ago

Thanks for contacting us.

We're moving this issue to the .NET 8 Planning milestone for future evaluation / consideration. We would like to keep this around to collect more feedback, which can help us with prioritizing this work. We will re-evaluate this issue, during our next planning meeting(s). If we later determine, that the issue has no community involvement, or it's very rare and low-impact issue, we will close it - so that the team can focus on more important and high impact issues. To learn more about what to expect next and how this issue will be handled you can read more about our triage process here.

captainsafia commented 1 year ago

I started working on a PR for this (https://github.com/dotnet/aspnetcore/pull/47346) and ran into a little snafu when defining the OpenAPI operation, specifically around how the HttpContext parameter is handled.

Ultimately, I decided that by default, an HttpContext parameter would not be emitted to the OpenAPI spec since HttpContext isn't a deserializable or parsable parameter type and doesn't represent a public API for the endpoint. Users can always modify this by configuring their own parameters in the WithOpenApi extension method.

This means that when testing with the Swagger UI, you'll only be able to send parameter-less and body-less requests to the endpoint for testing. This restriction makes sense to me. Thoughts?

halter73 commented 1 year ago

Ignoring HttpContext parameters when building the OpenAPI operation makes sense to me too. You can't infer anything about what the route handler is doing just from knowing it takes in an HttpContext. Is there really any alternative?

syedsuhaib commented 8 months ago

I am using the Delegate overload of MapGet but my issue is slightly different:

I have a response dto being returned by the delegate whose display title I would like to customize in Swagger. While using controllers, decorating the response model with a SwaggerSchemaAttribute would do the trick like so:

[SwaggerSchema(Title = "ResponseNameToShowInSwagger", Description = "I expect to see this description is swagger.")]
public class MyResponse
{
}

However, this no longer seems to be respected by minimal APIs. I just want to know whether the issue being tracked here will solve my problem as well or could there a different cause of my issue? Will appreciate any heads up on this.

captainsafia commented 8 months ago

@syedsuhaib Can you file a separate issue for this? I can provide guidance there...