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.

Sign up for GitHub

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

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

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
8c8ccd2
Infer response metadata in RequestDelegateFactory
halter73 Sep 13, 2022
9477b44
Update OpenApiRouteHandlerBuilderExtensionTests
halter73 Sep 13, 2022
6bae793
Infer 200 status despite HttpContext and HttpResponse parameters
halter73 Sep 14, 2022
df90b67
Remove dead code
halter73 Sep 14, 2022
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
3 changes: 2 additions & 1 deletion src/Http/Http.Extensions/src/Microsoft.AspNetCore.Http.Extensions.csproj
View file
Open in desktop
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
83 changes: 75 additions & 8 deletions src/Http/Http.Extensions/src/RequestDelegateFactory.cs
View file
Open in desktop
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.Parameters, 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,56 @@ private static Expression CreateParamCheckingResponseWritingMethodCall(Type retu
return Expression.Block(localVariables, checkParamAndCallMethod);
}

private static void PopulateBuiltInResponseTypeMetadata(Type returnType, IEnumerable<ParameterInfo> parameters, 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;
}

// Skip methods that have HttpContext or HttpResponse parameters that might change the status code from 200.
foreach (var parameter in parameters)
{
if (parameter.ParameterType == typeof(HttpContext) || parameter.ParameterType == typeof(HttpResponse))
{
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 •
edited
Loading

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 +1072,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 +1090,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 @@ -1771,6 +1821,17 @@ private static Expression BindParameterFromBindAsync(ParameterInfo parameter, Re
return Expression.Convert(boundValueExpr, parameter.ParameterType);
}

private static void AddInferredProducesResponseTypeMetadata(RequestDelegateFactoryContext factoryContext, Type type, string[] contentTypes)
{
if (factoryContext.MetadataAlreadyInferred)
{
return;
}

// Type cannot be null, and contentTypes is either [ "application/json" ] or [ "text/plain" ] both of which are valid.
factoryContext.EndpointBuilder.Metadata.Add(ProducesResponseTypeMetadata.CreateUnvalidated(type, statusCode: 200, contentTypes));
}

private static void AddInferredAcceptsMetadata(RequestDelegateFactoryContext factoryContext, Type type, string[] contentTypes)
{
if (factoryContext.MetadataAlreadyInferred)
Expand Down Expand Up @@ -1849,7 +1910,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 +2213,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
67 changes: 57 additions & 10 deletions src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs
View file
Open in desktop
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,52 @@ 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);
}

public static object[][] HttpContextAndHttpResponseParameterMethods => new[]
{
new object[] { (int foo, HttpContext context) => new object() },
new object[] { (HttpResponse response, int bar) => new object() },
new object[] { (HttpContext context, int baz) => "Hello" },
new object[] { (HttpResponse response) => "Hello" },
new object[] { (HttpContext context) => new object() },
new object[] { (HttpResponse response) => new object() },
new object[] { (HttpContext context) => "Hello" },
new object[] { (HttpResponse response) => "Hello" },
};

[Theory]
[MemberData(nameof(HttpContextAndHttpResponseParameterMethods))]
public void Create_DoesNotAddResponseType_AsMetadata_IfHasHttpContextOrHttpResponseParameter(Delegate @delegate)
{
var result = RequestDelegateFactory.Create(@delegate);

// Methods that have HttpContext or HttpResponse parameters are skipped because they might change the status code from 200.
Assert.Empty(result.EndpointMetadata);
}

[Fact]
public void Create_DoesNotAddAnythingBefore_ThePassedInEndpointMetadata()
{
Expand Down Expand Up @@ -6147,7 +6192,7 @@ public void Create_DiscoversEndpointMetadata_FromValueTaskWrappedReturnTypeImple
public void Create_CombinesDefaultMetadata_AndMetadataFromReturnTypesImplementingIEndpointMetadataProvider()
{
// Arrange
var @delegate = () => new CountsDefaultEndpointMetadataResult();
var @delegate = (HttpContext context) => new CountsDefaultEndpointMetadataResult();
var options = new RequestDelegateFactoryOptions
{
EndpointBuilder = CreateEndpointBuilder(new List<object>
Expand All @@ -6169,7 +6214,7 @@ public void Create_CombinesDefaultMetadata_AndMetadataFromReturnTypesImplementin
public void Create_CombinesDefaultMetadata_AndMetadataFromTaskWrappedReturnTypesImplementingIEndpointMetadataProvider()
{
// Arrange
var @delegate = () => Task.FromResult(new CountsDefaultEndpointMetadataResult());
var @delegate = (HttpContext context) => Task.FromResult(new CountsDefaultEndpointMetadataResult());
var options = new RequestDelegateFactoryOptions
{
EndpointBuilder = CreateEndpointBuilder(new List<object>
Expand All @@ -6191,7 +6236,7 @@ public void Create_CombinesDefaultMetadata_AndMetadataFromTaskWrappedReturnTypes
public void Create_CombinesDefaultMetadata_AndMetadataFromValueTaskWrappedReturnTypesImplementingIEndpointMetadataProvider()
{
// Arrange
var @delegate = () => ValueTask.FromResult(new CountsDefaultEndpointMetadataResult());
var @delegate = (HttpContext context) => ValueTask.FromResult(new CountsDefaultEndpointMetadataResult());
var options = new RequestDelegateFactoryOptions
{
EndpointBuilder = CreateEndpointBuilder(new List<object>
Expand Down Expand Up @@ -6298,12 +6343,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(CountsDefaultEndpointMetadataResult), ((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 @@ -6384,12 +6431,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(CountsDefaultEndpointMetadataResult), ((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 @@ -6624,15 +6673,13 @@ public static void PopulateMetadata(MethodInfo method, EndpointBuilder builder)
public Task ExecuteAsync(HttpContext httpContext) => throw new NotImplementedException();
}

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

public Task ExecuteAsync(HttpContext httpContext) => Task.CompletedTask;
}

private class RemovesAcceptsParameterMetadata : IEndpointParameterMetadataProvider
Expand Down
2 changes: 2 additions & 0 deletions src/OpenApi/src/OpenApiGenerator.cs
View file
Open in desktop
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
View file
Open in desktop
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
Loading