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

RequestDelegateFactory should infer IProducesResponseTypeMetadata #43675

Closed
halter73 opened this issue Sep 1, 2022 · 6 comments · Fixed by #43961
Closed

RequestDelegateFactory should infer IProducesResponseTypeMetadata #43675

halter73 opened this issue Sep 1, 2022 · 6 comments · Fixed by #43961
Assignees
Labels
area-minimal Includes minimal APIs, endpoint filters, parameter binding, request delegate generator etc area-web-frameworks *DEPRECATED* This label is deprecated in favor of the area-mvc and area-minimal labels enhancement This issue represents an ask for new feature or an enhancement to an existing one feature-minimal-actions Controller-like actions for endpoint routing
Milestone

Comments

@halter73
Copy link
Member

halter73 commented Sep 1, 2022

Is your feature request related to a problem? Please describe the problem.

RequestDelegateFactory already infers IAcceptsMetadata when it's reading JSON request bodies or multipart form data, so it's peculiar that it does not infer IProducesResponseTypeMetadata when our TypedResults do.

I don't think many people will expect us to infer response type metadata when returning a TypedResult like Ok<MyType> or Task<Ok<MyType>>, but not infer any response metadata when returning MyType or Task<MyType>. I think this is a weird gap.

Describe the solution you'd like

RequestDelegateFactory should infer and add "application/json" ProducesResponseTypeMetadata with the appropriate return Type if it can be inferred while creating the RequestDelegate in AddResponseWritingToMethodCall.

@halter73 halter73 added area-web-frameworks *DEPRECATED* This label is deprecated in favor of the area-mvc and area-minimal labels feature-minimal-actions Controller-like actions for endpoint routing labels Sep 1, 2022
@DamianEdwards
Copy link
Member

I'm a little confused. I thought RDF already inferred this in .NET 6.0? If I create a new .NET 6 web API project using Minimal APIs (weather forecast) and run it, the Swagger info correctly shows the API as returning WeatherForecast. Or is that because we populate those details in ApiExplorer even though the endpoint metadata is not added?

@DamianEdwards
Copy link
Member

I seem to recall we talked about this in .NET 6 and the desire was always to have RDF add the appropriate metadata and then have the ApiExplorer provider simply use the metadata to populate the API details but that we never quite got there. Is there anything more than IProducesResponseTypeMetadata that we should be adding?

@halter73
Copy link
Member Author

halter73 commented Sep 1, 2022

Or is that because we populate those details in ApiExplorer even though the endpoint metadata is not added?

I think that's what's happening.

@DamianEdwards
Copy link
Member

DamianEdwards commented Sep 12, 2022

So I just hit a scenario where this might help. I was attempting to get JWT bearer auth and the new problem details service support to work in an app, such that that Swagger UI enables logging in and making requests from the browser.

For an API defined like:

var builder = WebApplication.Create();

builder.Services.AddAuthentication().AddJwtBearer();
builder.Services.AddAuthorizationBuilder();
builder.Services.AddProblemDetails();
builder.Services.AddSwaggerGen();

var app = builder.Build();

app.UseStatusCodePages();
// Have to explicitly order AuthN/AuthZ middleware to get StatusCodePages to handle the 401/403
app.UseAuthentication();
app.UseAuthorization();

var examples = app.MapGroup("/").WithTags("Examples");

// Describe that all APIs can return errors as JSON or plain text
examples.WithMetadata(new ProducesResponseTypeAttribute(typeof(ProblemDetails), 401, "application/problem+json", "text/plain"));
examples.WithMetadata(new ProducesResponseTypeAttribute(typeof(ProblemDetails), 403, "application/problem+json", "text/plain"));

examples.MapGet("/secret", (ClaimsPrincipal user) => "shhh").RequireAuthorization();

app.Run();

This results in the API being described as only returning 401 or 403, as the inferred response (200: text/plain) is omitted due to the explicit metadata in the group:

{
    "/secret": {
        "get": {
            "tags": [
                "Examples"
            ],
            "responses": {
                "401": {
                    "description": "Unauthorized",
                    "content": {
                        "application/problem+json": {
                            "schema": {
                                "$ref": "#/components/schemas/ProblemDetails"
                            }
                        },
                        "text/plain": {
                            "schema": {
                                "$ref": "#/components/schemas/ProblemDetails"
                            }
                        }
                    }
                },
                "403": {
                    "description": "Forbidden",
                    "content": {
                        "application/problem+json": {
                            "schema": {
                                "$ref": "#/components/schemas/ProblemDetails"
                            }
                        },
                        "text/plain": {
                            "schema": {
                                "$ref": "#/components/schemas/ProblemDetails"
                            }
                        }
                    }
                }
            }
        }
    }
}

To workaround this, one has to explicitly add back the inferred produces metadata directly to the endpoint definition:

examples.MapGet("/secret", (ClaimsPrincipal user) => "shhh")
    .RequireAuthorization()
    .Produces(200, typeof(string), "text/plain");

@halter73
Copy link
Member Author

@Pilchie I was thinking that it might be okay to let this slide from 7.0, but given this recent feedback, I'm reprioritizing this and starting on it now. I originally wanted this in rc2 anyway. This will be coming in hot though. I should have a PR ready tomorrow.

@Pilchie
Copy link
Member

Pilchie commented Sep 13, 2022

Sounds good - thanks for the heads up.

@halter73 halter73 linked a pull request Sep 13, 2022 that will close this issue
10 tasks
@danroth27 danroth27 added the enhancement This issue represents an ask for new feature or an enhancement to an existing one label Oct 7, 2022
@ghost ghost locked as resolved and limited conversation to collaborators Nov 7, 2022
@amcasey amcasey added the area-minimal Includes minimal APIs, endpoint filters, parameter binding, request delegate generator etc label Jun 2, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-minimal Includes minimal APIs, endpoint filters, parameter binding, request delegate generator etc area-web-frameworks *DEPRECATED* This label is deprecated in favor of the area-mvc and area-minimal labels enhancement This issue represents an ask for new feature or an enhancement to an existing one feature-minimal-actions Controller-like actions for endpoint routing
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants