Skip to content

[release/7.0] Infer response metadata in RequestDelegateFactory #43961

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

Merged
merged 4 commits into from
Sep 14, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
<Project Sdk="Microsoft.NET.Sdk">
<Project Sdk="Microsoft.NET.Sdk">

<PropertyGroup>
<Description>ASP.NET Core common extension methods for HTTP abstractions, HTTP headers, HTTP request/response, and session state.</Description>
Expand All @@ -17,6 +17,7 @@
<Compile Include="$(SharedSourceRoot)EndpointMetadataPopulator.cs" LinkBase="Shared"/>
<Compile Include="$(SharedSourceRoot)PropertyAsParameterInfo.cs" LinkBase="Shared"/>
<Compile Include="..\..\Shared\StreamCopyOperationInternal.cs" LinkBase="Shared" />
<Compile Include="$(SharedSourceRoot)ApiExplorerTypes\*.cs" LinkBase="Shared" />
<Compile Include="$(SharedSourceRoot)RoutingMetadata\AcceptsMetadata.cs" LinkBase="Shared" />
<Compile Include="$(SharedSourceRoot)TypeNameHelper\TypeNameHelper.cs" LinkBase="Shared"/>
<Compile Include="$(SharedSourceRoot)ProblemDetails\ProblemDetailsDefaults.cs" LinkBase="Shared" />
Expand Down
63 changes: 55 additions & 8 deletions src/Http/Http.Extensions/src/RequestDelegateFactory.cs
Original file line number Diff line number Diff line change
Expand Up @@ -110,8 +110,9 @@ public static partial class RequestDelegateFactory
private static readonly MemberExpression FilterContextHttpContextStatusCodeExpr = Expression.Property(FilterContextHttpContextResponseExpr, typeof(HttpResponse).GetProperty(nameof(HttpResponse.StatusCode))!);
private static readonly ParameterExpression InvokedFilterContextExpr = Expression.Parameter(typeof(EndpointFilterInvocationContext), "filterContext");

private static readonly string[] DefaultAcceptsContentType = new[] { "application/json" };
private static readonly string[] DefaultAcceptsAndProducesContentType = new[] { JsonConstants.JsonContentType };
private static readonly string[] FormFileContentType = new[] { "multipart/form-data" };
private static readonly string[] PlaintextContentType = new[] { "text/plain" };

/// <summary>
/// Returns metadata inferred automatically for the <see cref="RequestDelegate"/> created by <see cref="Create(Delegate, RequestDelegateFactoryOptions?, RequestDelegateMetadataResult?)"/>.
Expand Down Expand Up @@ -378,10 +379,10 @@ private static Expression[] CreateArgumentsAndInferMetadata(MethodInfo methodInf

if (!factoryContext.MetadataAlreadyInferred)
{
PopulateBuiltInResponseTypeMetadata(methodInfo.ReturnType, factoryContext.EndpointBuilder);

// Add metadata provided by the delegate return type and parameter types next, this will be more specific than inferred metadata from above
EndpointMetadataPopulator.PopulateMetadata(methodInfo,
factoryContext.EndpointBuilder,
factoryContext.Parameters);
EndpointMetadataPopulator.PopulateMetadata(methodInfo, factoryContext.EndpointBuilder, factoryContext.Parameters);
}

return args;
Expand Down Expand Up @@ -927,6 +928,47 @@ private static Expression CreateParamCheckingResponseWritingMethodCall(Type retu
return Expression.Block(localVariables, checkParamAndCallMethod);
}

private static void PopulateBuiltInResponseTypeMetadata(Type returnType, EndpointBuilder builder)
{
if (returnType.IsByRefLike)
{
throw GetUnsupportedReturnTypeException(returnType);
}

if (returnType == typeof(Task) || returnType == typeof(ValueTask))
{
returnType = typeof(void);
}
else if (AwaitableInfo.IsTypeAwaitable(returnType, out _))
{
var genericTypeDefinition = returnType.IsGenericType ? returnType.GetGenericTypeDefinition() : null;

if (genericTypeDefinition == typeof(Task<>) || genericTypeDefinition == typeof(ValueTask<>))
{
returnType = returnType.GetGenericArguments()[0];
}
else
{
throw GetUnsupportedReturnTypeException(returnType);
}
}

// Skip void returns and IResults. IResults might implement IEndpointMetadataProvider but otherwise we don't know what it might do.
if (returnType == typeof(void) || typeof(IResult).IsAssignableFrom(returnType))
{
return;
}

if (returnType == typeof(string))
{
builder.Metadata.Add(ProducesResponseTypeMetadata.CreateUnvalidated(type: null, statusCode: 200, PlaintextContentType));
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Member Author

@halter73 halter73 Sep 14, 2022

Choose a reason for hiding this comment

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

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.

}
else
{
builder.Metadata.Add(ProducesResponseTypeMetadata.CreateUnvalidated(returnType, statusCode: 200, DefaultAcceptsAndProducesContentType));
}
}

private static Expression AddResponseWritingToMethodCall(Expression methodCall, Type returnType)
{
// Exact request delegate match
Expand Down Expand Up @@ -1021,7 +1063,7 @@ private static Expression AddResponseWritingToMethodCall(Expression methodCall,
else
{
// TODO: Handle custom awaitables
throw new NotSupportedException($"Unsupported return type: {TypeNameHelper.GetTypeDisplayName(returnType)}");
throw GetUnsupportedReturnTypeException(returnType);
}
}
else if (typeof(IResult).IsAssignableFrom(returnType))
Expand All @@ -1039,8 +1081,7 @@ private static Expression AddResponseWritingToMethodCall(Expression methodCall,
}
else if (returnType.IsByRefLike)
{
// Unsupported
throw new NotSupportedException($"Unsupported return type: {TypeNameHelper.GetTypeDisplayName(returnType)}");
throw GetUnsupportedReturnTypeException(returnType);
}
else if (returnType.IsValueType)
{
Expand Down Expand Up @@ -1849,7 +1890,7 @@ private static Expression BindParameterFromBody(ParameterInfo parameter, bool al

factoryContext.JsonRequestBodyParameter = parameter;
factoryContext.AllowEmptyRequestBody = allowEmpty || isOptional;
AddInferredAcceptsMetadata(factoryContext, parameter.ParameterType, DefaultAcceptsContentType);
AddInferredAcceptsMetadata(factoryContext, parameter.ParameterType, DefaultAcceptsAndProducesContentType);

if (!factoryContext.AllowEmptyRequestBody)
{
Expand Down Expand Up @@ -2152,6 +2193,12 @@ private static async Task ExecuteResultWriteResponse(IResult? result, HttpContex
{
await EnsureRequestResultNotNull(result).ExecuteAsync(httpContext);
}

private static NotSupportedException GetUnsupportedReturnTypeException(Type returnType)
{
return new NotSupportedException($"Unsupported return type: {TypeNameHelper.GetTypeDisplayName(returnType)}");
}

private static class RequestDelegateFactoryConstants
{
public const string RouteAttribute = "Route (Attribute)";
Expand Down
48 changes: 42 additions & 6 deletions src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@
#nullable enable

using System.Buffers;
using System.Diagnostics.CodeAnalysis;
using System.Globalization;
using System.IO.Pipelines;
using System.Linq.Expressions;
Expand Down Expand Up @@ -6033,7 +6032,7 @@ string HelloName()
public void Create_DoesNotAddDelegateMethodInfo_AsMetadata()
{
// Arrange
var @delegate = () => "Hello";
var @delegate = () => { };

// Act
var result = RequestDelegateFactory.Create(@delegate);
Expand All @@ -6043,6 +6042,30 @@ public void Create_DoesNotAddDelegateMethodInfo_AsMetadata()
Assert.Empty(result.EndpointMetadata);
}

[Fact]
public void Create_AddJsonResponseType_AsMetadata()
{
var @delegate = () => new object();
var result = RequestDelegateFactory.Create(@delegate);

var responseMetadata = Assert.IsAssignableFrom<IProducesResponseTypeMetadata>(Assert.Single(result.EndpointMetadata));

Assert.Equal("application/json", Assert.Single(responseMetadata.ContentTypes));
Assert.Equal(typeof(object), responseMetadata.Type);
}

[Fact]
public void Create_AddPlaintextResponseType_AsMetadata()
{
var @delegate = () => "Hello";
var result = RequestDelegateFactory.Create(@delegate);

var responseMetadata = Assert.IsAssignableFrom<IProducesResponseTypeMetadata>(Assert.Single(result.EndpointMetadata));

Assert.Equal("text/plain", Assert.Single(responseMetadata.ContentTypes));
Assert.Null(responseMetadata.Type);
}

[Fact]
public void Create_DoesNotAddAnythingBefore_ThePassedInEndpointMetadata()
{
Expand Down Expand Up @@ -6278,7 +6301,7 @@ public void Create_CombinesPropertiesAsParameterMetadata_AndTopLevelParameter()
public void Create_CombinesAllMetadata_InCorrectOrder()
{
// Arrange
var @delegate = [Attribute1, Attribute2] (AddsCustomParameterMetadata param1) => new CountsDefaultEndpointMetadataResult();
var @delegate = [Attribute1, Attribute2] (AddsCustomParameterMetadata param1) => new CountsDefaultEndpointMetadataPoco();
var options = new RequestDelegateFactoryOptions
{
EndpointBuilder = CreateEndpointBuilder(new List<object>
Expand All @@ -6298,12 +6321,14 @@ public void Create_CombinesAllMetadata_InCorrectOrder()
m => Assert.True(m is CustomEndpointMetadata { Source: MetadataSource.Caller }),
// Inferred AcceptsMetadata from RDF for complex type
m => Assert.True(m is AcceptsMetadata am && am.RequestType == typeof(AddsCustomParameterMetadata)),
// Inferred ProducesResopnseTypeMetadata from RDF for complex type
m => Assert.Equal(typeof(CountsDefaultEndpointMetadataPoco), ((IProducesResponseTypeMetadata)m).Type),
// Metadata provided by parameters implementing IEndpointParameterMetadataProvider
m => Assert.True(m is ParameterNameMetadata { Name: "param1" }),
// Metadata provided by parameters implementing IEndpointMetadataProvider
m => Assert.True(m is CustomEndpointMetadata { Source: MetadataSource.Parameter }),
// Metadata provided by return type implementing IEndpointMetadataProvider
m => Assert.True(m is MetadataCountMetadata { Count: 4 }));
m => Assert.True(m is MetadataCountMetadata { Count: 5 }));
}

[Fact]
Expand Down Expand Up @@ -6369,7 +6394,7 @@ public void Create_DoesNotInferMetadata_GivenManuallyConstructedMetadataResult()
public void InferMetadata_ThenCreate_CombinesAllMetadata_InCorrectOrder()
{
// Arrange
var @delegate = [Attribute1, Attribute2] (AddsCustomParameterMetadata param1) => new CountsDefaultEndpointMetadataResult();
var @delegate = [Attribute1, Attribute2] (AddsCustomParameterMetadata param1) => new CountsDefaultEndpointMetadataPoco();
var options = new RequestDelegateFactoryOptions
{
EndpointBuilder = CreateEndpointBuilder(),
Expand All @@ -6384,12 +6409,14 @@ public void InferMetadata_ThenCreate_CombinesAllMetadata_InCorrectOrder()
Assert.Collection(result.EndpointMetadata,
// Inferred AcceptsMetadata from RDF for complex type
m => Assert.True(m is AcceptsMetadata am && am.RequestType == typeof(AddsCustomParameterMetadata)),
// Inferred ProducesResopnseTypeMetadata from RDF for complex type
m => Assert.Equal(typeof(CountsDefaultEndpointMetadataPoco), ((IProducesResponseTypeMetadata)m).Type),
// Metadata provided by parameters implementing IEndpointParameterMetadataProvider
m => Assert.True(m is ParameterNameMetadata { Name: "param1" }),
// Metadata provided by parameters implementing IEndpointMetadataProvider
m => Assert.True(m is CustomEndpointMetadata { Source: MetadataSource.Parameter }),
// Metadata provided by return type implementing IEndpointMetadataProvider
m => Assert.True(m is MetadataCountMetadata { Count: 3 }),
m => Assert.True(m is MetadataCountMetadata { Count: 4 }),
// Entry-specific metadata added after a call to InferMetadata
m => Assert.True(m is CustomEndpointMetadata { Source: MetadataSource.Caller }));
}
Expand Down Expand Up @@ -6635,6 +6662,15 @@ public static void PopulateMetadata(MethodInfo method, EndpointBuilder builder)
public Task ExecuteAsync(HttpContext httpContext) => Task.CompletedTask;
}

private class CountsDefaultEndpointMetadataPoco : IEndpointMetadataProvider
{
public static void PopulateMetadata(MethodInfo method, EndpointBuilder builder)
{
var currentMetadataCount = builder.Metadata.Count;
builder.Metadata.Add(new MetadataCountMetadata { Count = currentMetadataCount });
}
}

private class RemovesAcceptsParameterMetadata : IEndpointParameterMetadataProvider
{
public static void PopulateMetadata(ParameterInfo parameter, EndpointBuilder builder)
Expand Down
2 changes: 2 additions & 0 deletions src/OpenApi/src/OpenApiGenerator.cs
Original file line number Diff line number Diff line change
Expand Up @@ -193,6 +193,8 @@ private static OpenApiResponses GetOpenApiResponses(MethodInfo method, EndpointM
foreach (var annotation in eligibileAnnotations)
{
var statusCode = annotation.Key.ToString(CultureInfo.InvariantCulture);

// TODO: Use the discarded response Type for schema generation
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

var (_, contentTypes) = annotation.Value;
var responseContent = new Dictionary<string, OpenApiMediaType>();

Expand Down
10 changes: 9 additions & 1 deletion src/OpenApi/test/OpenApiRouteHandlerBuilderExtensionTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -136,8 +136,16 @@ public void WithOpenApi_WorksWithMapGroupAndEndpointAnnotations()
var groupDataSource = Assert.Single(builder.DataSources);
var endpoint = Assert.Single(groupDataSource.Endpoints);
var operation = endpoint.Metadata.GetMetadata<OpenApiOperation>();

Assert.NotNull(operation);
Assert.Equal("201", operation.Responses.Keys.SingleOrDefault());
Assert.Equal(2, operation.Responses.Count);

var defaultOperation = operation.Responses["200"];
Assert.True(defaultOperation.Content.ContainsKey("text/plain"));

var annotatedOperation = operation.Responses["201"];
// Produces doesn't special case string??
Copy link
Member

Choose a reason for hiding this comment

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

What do you mean by this comment?

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

Wait it does? That seems like a bug.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

#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.

Assert.True(annotatedOperation.Content.ContainsKey("application/json"));
}

[Fact]
Expand Down
32 changes: 14 additions & 18 deletions src/Shared/ApiExplorerTypes/ProducesResponseTypeMetadata.cs
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

using System;
using System.Linq;
using Microsoft.AspNetCore.Http.Metadata;
using Microsoft.Net.Http.Headers;
Expand All @@ -20,11 +19,11 @@ internal sealed class ProducesResponseTypeMetadata : IProducesResponseTypeMetada
/// </summary>
/// <param name="statusCode">The HTTP response status code.</param>
public ProducesResponseTypeMetadata(int statusCode)
: this(typeof(void), statusCode)
: this(type: null, statusCode, Enumerable.Empty<string>())
{
IsResponseTypeSetByDefault = true;
}

// Only for internal use where validation is unnecessary.
/// <summary>
/// Initializes an instance of <see cref="ProducesResponseTypeMetadata"/>.
/// </summary>
Expand All @@ -34,7 +33,6 @@ public ProducesResponseTypeMetadata(Type type, int statusCode)
{
Type = type ?? throw new ArgumentNullException(nameof(type));
StatusCode = statusCode;
IsResponseTypeSetByDefault = false;
_contentTypes = Enumerable.Empty<string>();
}

Expand All @@ -54,7 +52,6 @@ public ProducesResponseTypeMetadata(Type type, int statusCode, string contentTyp

Type = type ?? throw new ArgumentNullException(nameof(type));
StatusCode = statusCode;
IsResponseTypeSetByDefault = false;

MediaTypeHeaderValue.Parse(contentType);
for (var i = 0; i < additionalContentTypes.Length; i++)
Expand All @@ -65,30 +62,29 @@ public ProducesResponseTypeMetadata(Type type, int statusCode, string contentTyp
_contentTypes = GetContentTypes(contentType, additionalContentTypes);
}

// Only for internal use where validation is unnecessary.
private ProducesResponseTypeMetadata(Type? type, int statusCode, IEnumerable<string> contentTypes)
{

Type = type;
StatusCode = statusCode;
_contentTypes = contentTypes;
}

/// <summary>
/// Gets or sets the type of the value returned by an action.
/// </summary>
public Type Type { get; set; }
public Type? Type { get; set; }
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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.


/// <summary>
/// Gets or sets the HTTP status code of the response.
/// </summary>
public int StatusCode { get; set; }

/// <summary>
/// Used to distinguish a `Type` set by default in the constructor versus
/// one provided by the user.
///
/// When <see langword="false"/>, then <see cref="Type"/> is set by user.
///
/// When <see langword="true"/>, then <see cref="Type"/> is set by by
/// default in the constructor
/// </summary>
/// <value></value>
internal bool IsResponseTypeSetByDefault { get; }

public IEnumerable<string> ContentTypes => _contentTypes;

internal static ProducesResponseTypeMetadata CreateUnvalidated(Type? type, int statusCode, IEnumerable<string> contentTypes) => new(type, statusCode, contentTypes);

private static List<string> GetContentTypes(string contentType, string[] additionalContentTypes)
{
var contentTypes = new List<string>(additionalContentTypes.Length + 1);
Expand Down