Skip to content

Infer that interface parameters are services #31658

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 5 commits into from
Apr 13, 2021
Merged

Infer that interface parameters are services #31658

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
24a748c
Add tests!
halter73 Apr 9, 2021
44a588c
Infer that interface parameters are services
halter73 Apr 9, 2021
ea59c39
MapAction -> MinimalAction
halter73 Apr 9, 2021
6fbdbc6
More test coverage
halter73 Apr 9, 2021
38dbab5
fix build
halter73 Apr 10, 2021
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
32 changes: 20 additions & 12 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 @@ -3,7 +3,6 @@

using System;
using System.Collections.Concurrent;
using System.Diagnostics;
using System.IO;
using System.Linq;
using System.Linq.Expressions;
Expand Down Expand Up @@ -203,15 +202,7 @@ private static Expression CreateArgument(ParameterInfo parameter, FactoryContext
}
else if (parameterCustomAttributes.OfType<IFromBodyMetadata>().FirstOrDefault() is { } bodyAttribute)
{
if (factoryContext.JsonRequestBodyType is not null)
{
throw new InvalidOperationException("Action cannot have more than one FromBody attribute.");
}

factoryContext.JsonRequestBodyType = parameter.ParameterType;
factoryContext.AllowEmptyRequestBody = bodyAttribute.AllowEmpty;

return Expression.Convert(BodyValueExpr, parameter.ParameterType);
return BindParameterFromBody(parameter.ParameterType, bodyAttribute.AllowEmpty, factoryContext);
}
else if (parameter.CustomAttributes.Any(a => typeof(IFromServiceMetadata).IsAssignableFrom(a.AttributeType)))
{
Expand All @@ -229,10 +220,14 @@ private static Expression CreateArgument(ParameterInfo parameter, FactoryContext
{
return BindParameterFromRouteValueOrQueryString(parameter, parameter.Name, factoryContext);
}
else
else if (parameter.ParameterType.IsInterface)
{
return Expression.Call(GetRequiredServiceMethod.MakeGenericMethod(parameter.ParameterType), RequestServicesExpr);
}
else
{
return BindParameterFromBody(parameter.ParameterType, allowEmpty: false, factoryContext);
Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, I think 👍🏽 @davidfowl 's comment - could we rely on the presence of a default value to determine if this is optional? e.g.

MyModel Echo(MyModel model) => model; // AllowEmpty = false

MyModel Echo(MyModel model = null) => model; // AllowEmpty = true

If we were absolutely bonkers like MVC, we could also rely on nullability of the parameter to determine optionality:

MyModel Echo(MyModel? model) => model; // AllowEmpty = true

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep. I'm a huge fain of using nullability to determine AllowEmpty behavior. I think we should also use it when the [FromBody] attribute is applied without an explicit parameter.

Copy link
Contributor

Choose a reason for hiding this comment

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

Behold ye what needs to be done for nullability: https://github.com/dotnet/aspnetcore/blob/main/src/Mvc/Mvc.DataAnnotations/src/DataAnnotationsMetadataProvider.cs#L465-L568. It would be nice to put this in a shared space.

Btw, it would be a lot easier to infer nullability once we have a source generator.

}
}

private static Expression CreateMethodCall(MethodInfo methodInfo, Expression? target, Expression[] arguments) =>
Expand Down Expand Up @@ -428,7 +423,7 @@ private static Expression AddResponseWritingToMethodCall(Expression methodCall,
var invoker = Expression.Lambda<Func<object?, HttpContext, object?, Task>>(
responseWritingMethodCall, TargetExpr, HttpContextExpr, BodyValueExpr).Compile();

var bodyType = factoryContext.JsonRequestBodyType!;
var bodyType = factoryContext.JsonRequestBodyType;
object? defaultBodyValue = null;

if (factoryContext.AllowEmptyRequestBody && bodyType.IsValueType)
Expand Down Expand Up @@ -627,6 +622,19 @@ private static Expression BindParameterFromRouteValueOrQueryString(ParameterInfo
return BindParameterFromValue(parameter, Expression.Coalesce(routeValue, queryValue), factoryContext);
}

private static Expression BindParameterFromBody(Type parameterType, bool allowEmpty, FactoryContext factoryContext)
{
if (factoryContext.JsonRequestBodyType is not null)
{
throw new InvalidOperationException("Action cannot have more than one FromBody attribute.");
Copy link
Member

Choose a reason for hiding this comment

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

This error is kinda jank for the case where there's no explicit attribute.

Copy link
Contributor

Choose a reason for hiding this comment

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

Does this code path execute in the context of executing the delegate (as opposed to whenever we build the route table)? If it's the latter, we should include more details about what method produced this error. It might be good to include that regardless.

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 is build time, but I agree this error message is terrible especially in light of the new convention-based inference of parameter sources. I want to track more information about the parameters when building up the RequestDelegate arguments which will lead to better errors, but I've already started on the culture stuff so I think I'll do this as part of this PR.

}

factoryContext.JsonRequestBodyType = parameterType;
factoryContext.AllowEmptyRequestBody = allowEmpty;
Copy link
Member

Choose a reason for hiding this comment

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

Interesting that inference means you lose this feature. Can we use nullability to determine this?

Thoughts @pranavkm?


return Expression.Convert(BodyValueExpr, parameterType);
}

private static MethodInfo GetMethodInfo<T>(Expression<T> expr)
{
var mc = (MethodCallExpression)expr.Body;
Expand Down
86 changes: 51 additions & 35 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 @@ -361,14 +361,8 @@ public static bool TryParse(string? value, out MyTryParsableRecord? result)
[MemberData(nameof(TryParsableParameters))]
public async Task RequestDelegatePopulatesUnattributedTryParsableParametersFromRouteValue(Delegate action, string? routeValue, object? expectedParameterValue)
{
var invalidDataException = new InvalidDataException();
var serviceCollection = new ServiceCollection();
serviceCollection.AddSingleton(LoggerFactory);

var httpContext = new DefaultHttpContext();
httpContext.Request.RouteValues["tryParsable"] = routeValue;
httpContext.Features.Set<IHttpRequestLifetimeFeature>(new TestHttpRequestLifetimeFeature());
httpContext.RequestServices = serviceCollection.BuildServiceProvider();

var requestDelegate = RequestDelegateFactory.Create(action);

Expand Down Expand Up @@ -416,7 +410,7 @@ public async Task RequestDelegatePopulatesUnattributedTryParsableParametersFromR
Assert.Equal(42, httpContext.Items["tryParsable"]);
}

public static object[][] DelegatesWithInvalidAttributes
public static object[][] DelegatesWithAttributesOnNotTryParsableParameters
{
get
{
Expand All @@ -434,7 +428,7 @@ void InvalidFromHeader([FromHeader] object notTryParsable) { }
}

[Theory]
[MemberData(nameof(DelegatesWithInvalidAttributes))]
[MemberData(nameof(DelegatesWithAttributesOnNotTryParsableParameters))]
public void CreateThrowsInvalidOperationExceptionWhenAttributeRequiresTryParseMethodThatDoesNotExist(Delegate action)
{
var ex = Assert.Throws<InvalidOperationException>(() => RequestDelegateFactory.Create(action));
Expand All @@ -460,7 +454,6 @@ void TestAction([FromRoute] int tryParsable, [FromRoute] int tryParsable2)
invoked = true;
}

var invalidDataException = new InvalidDataException();
var serviceCollection = new ServiceCollection();
serviceCollection.AddSingleton(LoggerFactory);

Expand Down Expand Up @@ -542,47 +535,61 @@ void TestAction([FromHeader(Name = customHeaderName)] int value)
Assert.Equal(originalHeaderParam, deserializedRouteParam);
}

[Fact]
public async Task RequestDelegatePopulatesFromBodyParameter()
public static object[][] FromBodyActions
{
Todo originalTodo = new()
get
{
Name = "Write more tests!"
};
void TestExplicitFromBody(HttpContext httpContext, [FromBody] Todo todo)
{
httpContext.Items.Add("body", todo);
}

Todo? deserializedRequestBody = null;
void TestImpliedFromBody(HttpContext httpContext, Todo myService)
{
httpContext.Items.Add("body", myService);
}

void TestAction([FromBody] Todo todo)
{
deserializedRequestBody = todo;
return new[]
{
new[] { (Action<HttpContext, Todo>)TestExplicitFromBody },
new[] { (Action<HttpContext, Todo>)TestImpliedFromBody },
};
}
}

[Theory]
[MemberData(nameof(FromBodyActions))]
public async Task RequestDelegatePopulatesFromBodyParameter(Delegate action)
{
Todo originalTodo = new()
{
Name = "Write more tests!"
};

var httpContext = new DefaultHttpContext();
httpContext.Request.Headers["Content-Type"] = "application/json";

var requestBodyBytes = JsonSerializer.SerializeToUtf8Bytes(originalTodo);
httpContext.Request.Body = new MemoryStream(requestBodyBytes);

var requestDelegate = RequestDelegateFactory.Create((Action<Todo>)TestAction);
var requestDelegate = RequestDelegateFactory.Create(action);

await requestDelegate(httpContext);

var deserializedRequestBody = httpContext.Items["body"];
Assert.NotNull(deserializedRequestBody);
Assert.Equal(originalTodo.Name, deserializedRequestBody!.Name);
Assert.Equal(originalTodo.Name, ((Todo)deserializedRequestBody!).Name);
}

[Fact]
public async Task RequestDelegateRejectsEmptyBodyGivenDefaultFromBodyParameter()
[Theory]
[MemberData(nameof(FromBodyActions))]
public async Task RequestDelegateRejectsEmptyBodyGivenFromBodyParameter(Delegate action)
{
void TestAction([FromBody] Todo todo)
{
}

var httpContext = new DefaultHttpContext();
httpContext.Request.Headers["Content-Type"] = "application/json";
httpContext.Request.Headers["Content-Length"] = "0";

var requestDelegate = RequestDelegateFactory.Create((Action<Todo>)TestAction);
var requestDelegate = RequestDelegateFactory.Create(action);

await Assert.ThrowsAsync<JsonException>(() => requestDelegate(httpContext));
}
Expand Down Expand Up @@ -702,12 +709,16 @@ void TestAction([FromBody] Todo todo)
[Fact]
public void BuildRequestDelegateThrowsInvalidOperationExceptionGivenFromBodyOnMultipleParameters()
{
void TestAction([FromBody] int value1, [FromBody] int value2) { }
void TestAttributedInvalidAction([FromBody] int value1, [FromBody] int value2) { }
void TestInferredInvalidAction(Todo value1, Todo value2) { }
void TestBothInvalidAction(Todo value1, [FromBody] int value2) { }

Assert.Throws<InvalidOperationException>(() => RequestDelegateFactory.Create((Action<int, int>)TestAction));
Assert.Throws<InvalidOperationException>(() => RequestDelegateFactory.Create((Action<int, int>)TestAttributedInvalidAction));
Assert.Throws<InvalidOperationException>(() => RequestDelegateFactory.Create((Action<Todo, Todo>)TestInferredInvalidAction));
Assert.Throws<InvalidOperationException>(() => RequestDelegateFactory.Create((Action<Todo, int>)TestBothInvalidAction));
}

public static object[][] FromServiceParameter
public static object[][] FromServiceActions
{
get
{
Expand All @@ -716,7 +727,7 @@ void TestExplicitFromService(HttpContext httpContext, [FromService] MyService my
httpContext.Items.Add("service", myService);
}

void TestImpliedFromService(HttpContext httpContext, MyService myService)
void TestImpliedFromService(HttpContext httpContext, IMyService myService)
{
httpContext.Items.Add("service", myService);
}
Expand All @@ -730,13 +741,14 @@ void TestImpliedFromService(HttpContext httpContext, MyService myService)
}

[Theory]
[MemberData(nameof(FromServiceParameter))]
[MemberData(nameof(FromServiceActions))]
public async Task RequestDelegatePopulatesParametersFromServiceWithAndWithoutAttribute(Delegate action)
{
var myOriginalService = new MyService();

var serviceCollection = new ServiceCollection();
serviceCollection.AddSingleton(myOriginalService);
serviceCollection.AddSingleton<IMyService>(myOriginalService);

var httpContext = new DefaultHttpContext();
httpContext.RequestServices = serviceCollection.BuildServiceProvider();
Expand All @@ -749,11 +761,11 @@ public async Task RequestDelegatePopulatesParametersFromServiceWithAndWithoutAtt
}

[Theory]
[MemberData(nameof(FromServiceParameter))]
[MemberData(nameof(FromServiceActions))]
public async Task RequestDelegateRequiresServiceForAllFromServiceParameters(Delegate action)
{
var httpContext = new DefaultHttpContext();
httpContext.RequestServices = (new ServiceCollection()).BuildServiceProvider();
httpContext.RequestServices = new ServiceCollection().BuildServiceProvider();

var requestDelegate = RequestDelegateFactory.Create((Action<HttpContext, MyService>)action);

Expand Down Expand Up @@ -1058,7 +1070,11 @@ private class FromServiceAttribute : Attribute, IFromServiceMetadata
{
}

private class MyService
private interface IMyService
{
}

private class MyService : IMyService
{
}

Expand Down
6 changes: 3 additions & 3 deletions ...der/MapActionEndpointConventionBuilder.cs → ...MinimalActionEndpointConventionBuilder.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,16 +9,16 @@ namespace Microsoft.AspNetCore.Builder
/// <summary>
/// Builds conventions that will be used for customization of MapAction <see cref="EndpointBuilder"/> instances.
/// </summary>
public sealed class MapActionEndpointConventionBuilder : IEndpointConventionBuilder
public sealed class MinimalActionEndpointConventionBuilder : IEndpointConventionBuilder
{
private readonly List<IEndpointConventionBuilder> _endpointConventionBuilders;

internal MapActionEndpointConventionBuilder(IEndpointConventionBuilder endpointConventionBuilder)
internal MinimalActionEndpointConventionBuilder(IEndpointConventionBuilder endpointConventionBuilder)
{
_endpointConventionBuilders = new List<IEndpointConventionBuilder>() { endpointConventionBuilder };
}

internal MapActionEndpointConventionBuilder(List<IEndpointConventionBuilder> endpointConventionBuilders)
internal MinimalActionEndpointConventionBuilder(List<IEndpointConventionBuilder> endpointConventionBuilders)
{
_endpointConventionBuilders = endpointConventionBuilders;
}
Expand Down
18 changes: 9 additions & 9 deletions ...apActionEndpointRouteBuilderExtensions.cs → ...alActionEndpointRouteBuilderExtensions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ namespace Microsoft.AspNetCore.Builder
/// <summary>
/// Provides extension methods for <see cref="IEndpointRouteBuilder"/> to define HTTP API endpoints.
/// </summary>
public static class MapActionEndpointRouteBuilderExtensions
public static class MinmalActionEndpointRouteBuilderExtensions
{
// Avoid creating a new array every call
private static readonly string[] GetVerb = new[] { "GET" };
Expand All @@ -30,7 +30,7 @@ public static class MapActionEndpointRouteBuilderExtensions
/// <param name="pattern">The route pattern.</param>
/// <param name="action">The delegate executed when the endpoint is matched.</param>
/// <returns>A <see cref="IEndpointConventionBuilder"/> that can be used to further customize the endpoint.</returns>
public static MapActionEndpointConventionBuilder MapGet(
public static MinimalActionEndpointConventionBuilder MapGet(
this IEndpointRouteBuilder endpoints,
string pattern,
Delegate action)
Expand All @@ -46,7 +46,7 @@ public static MapActionEndpointConventionBuilder MapGet(
/// <param name="pattern">The route pattern.</param>
/// <param name="action">The delegate executed when the endpoint is matched.</param>
/// <returns>A <see cref="IEndpointConventionBuilder"/> that can be used to further customize the endpoint.</returns>
public static MapActionEndpointConventionBuilder MapPost(
public static MinimalActionEndpointConventionBuilder MapPost(
this IEndpointRouteBuilder endpoints,
string pattern,
Delegate action)
Expand All @@ -62,7 +62,7 @@ public static MapActionEndpointConventionBuilder MapPost(
/// <param name="pattern">The route pattern.</param>
/// <param name="action">The delegate executed when the endpoint is matched.</param>
/// <returns>A <see cref="IEndpointConventionBuilder"/> that canaction be used to further customize the endpoint.</returns>
public static MapActionEndpointConventionBuilder MapPut(
public static MinimalActionEndpointConventionBuilder MapPut(
this IEndpointRouteBuilder endpoints,
string pattern,
Delegate action)
Expand All @@ -78,7 +78,7 @@ public static MapActionEndpointConventionBuilder MapPut(
/// <param name="pattern">The route pattern.</param>
/// <param name="action">The delegate executed when the endpoint is matched.</param>
/// <returns>A <see cref="IEndpointConventionBuilder"/> that can be used to further customize the endpoint.</returns>
public static MapActionEndpointConventionBuilder MapDelete(
public static MinimalActionEndpointConventionBuilder MapDelete(
this IEndpointRouteBuilder endpoints,
string pattern,
Delegate action)
Expand All @@ -95,7 +95,7 @@ public static MapActionEndpointConventionBuilder MapDelete(
/// <param name="action">The delegate executed when the endpoint is matched.</param>
/// <param name="httpMethods">HTTP methods that the endpoint will match.</param>
/// <returns>A <see cref="IEndpointConventionBuilder"/> that can be used to further customize the endpoint.</returns>
public static MapActionEndpointConventionBuilder MapMethods(
public static MinimalActionEndpointConventionBuilder MapMethods(
this IEndpointRouteBuilder endpoints,
string pattern,
IEnumerable<string> httpMethods,
Expand All @@ -120,7 +120,7 @@ public static MapActionEndpointConventionBuilder MapMethods(
/// <param name="pattern">The route pattern.</param>
/// <param name="action">The delegate executed when the endpoint is matched.</param>
/// <returns>A <see cref="IEndpointConventionBuilder"/> that can be used to further customize the endpoint.</returns>
public static MapActionEndpointConventionBuilder Map(
public static MinimalActionEndpointConventionBuilder Map(
this IEndpointRouteBuilder endpoints,
string pattern,
Delegate action)
Expand All @@ -136,7 +136,7 @@ public static MapActionEndpointConventionBuilder Map(
/// <param name="pattern">The route pattern.</param>
/// <param name="action">The delegate executed when the endpoint is matched.</param>
/// <returns>A <see cref="IEndpointConventionBuilder"/> that can be used to further customize the endpoint.</returns>
public static MapActionEndpointConventionBuilder Map(
public static MinimalActionEndpointConventionBuilder Map(
this IEndpointRouteBuilder endpoints,
RoutePattern pattern,
Delegate action)
Expand Down Expand Up @@ -185,7 +185,7 @@ public static MapActionEndpointConventionBuilder Map(
endpoints.DataSources.Add(dataSource);
}

return new MapActionEndpointConventionBuilder(dataSource.AddEndpointBuilder(builder));
return new MinimalActionEndpointConventionBuilder(dataSource.AddEndpointBuilder(builder));
}
}
}
Loading