[release/7.0] Infer response metadata in RequestDelegateFactory

[halter73]

RequestDelegateFactory should infer metadata about the response bodies it produces similar to the way it infers metadata about the request bodies it consumes.

Description

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.

With this PR, RequestDelegateFactory now infers and adds "application/json" and "text/plain" ProducesResponseTypeMetadata with the appropriate return Type (for POCOs, not for string).

Fixes #43675

Customer Impact

@DamianEdwards mentioned:

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.

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. This PR makes it so RDF now infers the 200 "text\plain" response as well.

Regression?

• Yes
• No

This is a brand-new scenario though.

Risk

• High
• Medium
• Low

This mostly impacts the new WithOpenApi() and MapGroup() features.

Verification

• Manual (required)
• Automated

Packaging changes reviewed?

• Yes
• No
• N/A

@DamianEdwards @captainsafia @Pilchie

[ghost]

Hi @halter73. Please make sure you've updated the PR description to use the Shiproom Template. Also, make sure this PR is not marked as a draft and is ready-to-merge.

To learn more about how to prepare a servicing PR click here.

[DamianEdwards]

@halter73 do we not need to update the ApiExplorer provider now too, as what was inferred from MethodInfo before is now already in metadata?

[halter73]

@halter73 do we not need to update the ApiExplorer provider now too, as what was inferred from MethodInfo before is now already in metadata?

No. The following logic now prevents CreateDefaultApiResponseType(Type responseType) from ever being called since there is now IProducesResponseTypeMetadata associated with the endpoint.

aspnetcore/src/Mvc/Mvc.ApiExplorer/src/EndpointMetadataApiDescriptionProvider.cs

Lines 340 to 354 in 5f91f1b

	var responseProviderMetadata = endpointMetadata.GetOrderedMetadata<IApiResponseMetadataProvider>();
	var producesResponseMetadata = endpointMetadata.GetOrderedMetadata<IProducesResponseTypeMetadata>();
	var errorMetadata = endpointMetadata.GetMetadata<ProducesErrorResponseTypeAttribute>();
	var defaultErrorType = errorMetadata?.Type ?? typeof(void);
	var contentTypes = new MediaTypeCollection();
	var responseProviderMetadataTypes = ApiResponseTypeProvider.ReadResponseMetadata(
	responseProviderMetadata, responseType, defaultErrorType, contentTypes);
	var producesResponseMetadataTypes = ApiResponseTypeProvider.ReadResponseMetadata(producesResponseMetadata, responseType);
	// We favor types added via the extension methods (which implements IProducesResponseTypeMetadata)
	// over those that are added via attributes.
	var responseMetadataTypes = producesResponseMetadataTypes.Values.Concat(responseProviderMetadataTypes.Values);
	if (responseMetadataTypes.Any())

This does mean that just like WithOpenApi(), ApiExplorer will still generate an "empty" 200 response in void cases where RDF still does not infer IProducesResponseTypeMetadata.

RDF reasonably makes no claim to be able to infer anything about the response in void-returning cases. WithOpenApi() and ApiExplorer do though, and this avoids a breaking behavioral change with both these APIs.

Compatibility is also why I was unable to delete the internal response type inference based on the return type in WithOpenApi() and ApiExplorer. There's a chance that these are still used in cases where RDF hasn't inferred IProducesResponseTypeMetadata and I don't think we want to break those scenarios.

[halter73]

@Pilchie should I send a tactics email for this? Or is still unnecessary since we haven't branched rc2 yet?

[captainsafia]

Super nit: we don't need to have this in code since it's tracked in the issue locker and that's a lot more resilient than in-code TODOs.

[halter73]

I'm glad there's an issue. I agree that's more important, but I think the comment is also fine. I don't want to rekick the build just for this at least.

[captainsafia]

Suggested change

	builder.Metadata.Add(ProducesResponseTypeMetadata.CreateUnvalidated(type: null, statusCode: 200, PlaintextContentType));
	builder.Metadata.Add(ProducesResponseTypeMetadata.CreateUnvalidated(type: typeof(string), statusCode: 200, PlaintextContentType));

Why null over typeof(string) here?

[halter73]

I left a brief comment here but I immediately resolved it. Basically, the caller shouldn't care if internally we used a Utf8ContentHttpResult and ReadOnlySpan<byte> or string. It has no observable impact on the API, so string doesn't have any impact on the schema. "text\plain" should be sufficient.

[captainsafia]

Not sure making this nullable is the right call here. Can you clarify your reasoning as to why this is?

[halter73]

This type is internal. The interface is public, and Type is already nullable there. Everything reading Type is using the interface is dealing with the possible null anyway for custom interface implementations which is why I didn't need to react.

[captainsafia]

What do you mean by this comment?

[halter73]

I was just surprised when updating this test that .Produces<string>(201); creates "application/json" metadata. I would have expected "text\plain", but this is existing behavior. You can still explicitly set the content-type if you want.

[DamianEdwards]

Wait it does? That seems like a bug.

[captainsafia]

Yep, this has been behavior for a while. The Produces extension method sets the type to application/json by default unless explicitly defined.

We had some extensive conversations about this when we were building out the defaults and decided that setting the default to application/json would be sensible for most minimal cases.

[halter73]

#43979. If we agree it's important, I'll try to get a PR out by tomorrow before branching. I'm also going to open an API proposal about making Produces and Accepts work with groups. I think we should probably just retarget these from RouteHandlerBuilder to IEndopintConventionBuilder, but we can consider RouteGroupBuilder-specific overloads although that's less extensible.

[Pilchie]

should I send a tactics email for this? Or is still unnecessary since we haven't branched rc2 yet?

if it’s ready to go by end of day tomorrow I can approve. We are branch on Thursday.

[Pilchie]

Approved for .NET 7 RC2.