Skip to content

Added support for type based parameter binding (#35496) #35535

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 3 commits into from
Aug 20, 2021
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
201 changes: 159 additions & 42 deletions src/Http/Http.Extensions/src/RequestDelegateFactory.cs

Large diffs are not rendered by default.

317 changes: 315 additions & 2 deletions src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -499,6 +499,53 @@ public static bool TryParse(string? value, out MyTryParsableRecord? result)
}
}

private class MyBindAsyncTypeThatThrows
{
public static ValueTask<object?> BindAsync(HttpContext context)
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the behavior like if the signature does not match? e.g. ValueTask BindAsync(HttpContext context)?

Copy link
Member Author

@davidfowl davidfowl Aug 20, 2021

Choose a reason for hiding this comment

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

It won't find it sorta like TryParse today and will fall through to the other things.

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 could fail if it found the method but had the wrong return type?

Copy link
Contributor

Choose a reason for hiding this comment

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

Toss it in the Analyzer pile?

Copy link
Member

@halter73 halter73 Aug 20, 2021

Choose a reason for hiding this comment

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

Can we just look for ValueTask<{type}> in FindBindAsyncMethod? What's the benefit of allowing any object to be returned? I get that we're going to box anyway for structs, but might as well lean on the type-safety checks of the compiler.

We could even throw at startup if a parameter type has a static BindAsync(HttpContext) method with the wrong return type.

{
throw new InvalidOperationException("BindAsync failed");
}
}

private record MyBindAsyncRecord(Uri Uri)
{
public static ValueTask<object?> BindAsync(HttpContext context)
{
if (!Uri.TryCreate(context.Request.Headers.Referer, UriKind.Absolute, out var uri))
{
return ValueTask.FromResult<object?>(null);
}

return ValueTask.FromResult<object?>(new MyBindAsyncRecord(uri));
}

// TryParse(HttpContext, ...) should be preferred over TryParse(string, ...) if there's
// no [FromRoute] or [FromQuery] attributes.
public static bool TryParse(string? value, out MyBindAsyncRecord? result)
{
throw new NotImplementedException();
}
}

private record struct MyBindAsyncStruct(Uri Uri)
{
public static ValueTask<object?> BindAsync(HttpContext context)
{
if (!Uri.TryCreate(context.Request.Headers.Referer, UriKind.Absolute, out var uri))
{
return ValueTask.FromResult<object?>(null);
}

return ValueTask.FromResult<object?>(new MyBindAsyncStruct(uri));
}

// TryParse(HttpContext, ...) should be preferred over TryParse(string, ...) if there's
// no [FromRoute] or [FromQuery] attributes.
public static bool TryParse(string? value, out MyBindAsyncStruct result) =>
throw new NotImplementedException();
}


[Theory]
[MemberData(nameof(TryParsableParameters))]
public async Task RequestDelegatePopulatesUnattributedTryParsableParametersFromRouteValue(Delegate action, string? routeValue, object? expectedParameterValue)
Expand Down Expand Up @@ -560,6 +607,84 @@ public async Task RequestDelegatePopulatesUnattributedTryParsableParametersFromR
Assert.Equal(42, httpContext.Items["tryParsable"]);
}

[Fact]
public async Task RequestDelegatePrefersBindAsyncOverTryParseString()
{
var httpContext = new DefaultHttpContext();

httpContext.Request.Headers.Referer = "https://example.org";

var requestDelegate = RequestDelegateFactory.Create((HttpContext httpContext, MyBindAsyncRecord tryParsable) =>
{
httpContext.Items["tryParsable"] = tryParsable;
});

await requestDelegate(httpContext);

Assert.Equal(new MyBindAsyncRecord(new Uri("https://example.org")), httpContext.Items["tryParsable"]);
}

[Fact]
public async Task RequestDelegatePrefersBindAsyncOverTryParseStringForNonNullableStruct()
{
var httpContext = new DefaultHttpContext();

httpContext.Request.Headers.Referer = "https://example.org";

var requestDelegate = RequestDelegateFactory.Create((HttpContext httpContext, MyBindAsyncStruct tryParsable) =>
{
httpContext.Items["tryParsable"] = tryParsable;
});

await requestDelegate(httpContext);

Assert.Equal(new MyBindAsyncStruct(new Uri("https://example.org")), httpContext.Items["tryParsable"]);
}

[Fact]
public async Task RequestDelegateUsesTryParseStringoOverBindAsyncGivenExplicitAttribute()
{
var fromRouteRequestDelegate = RequestDelegateFactory.Create((HttpContext httpContext, [FromRoute] MyBindAsyncRecord tryParsable) => { });
var fromQueryRequestDelegate = RequestDelegateFactory.Create((HttpContext httpContext, [FromQuery] MyBindAsyncRecord tryParsable) => { });

var httpContext = new DefaultHttpContext
{
Request =
{
RouteValues =
{
["tryParsable"] = "foo"
},
Query = new QueryCollection(new Dictionary<string, StringValues>
{
["tryParsable"] = "foo"
}),
},
};

await Assert.ThrowsAsync<NotImplementedException>(() => fromRouteRequestDelegate(httpContext));
await Assert.ThrowsAsync<NotImplementedException>(() => fromQueryRequestDelegate(httpContext));
}

[Fact]
public async Task RequestDelegateUsesTryParseStringOverBindAsyncGivenNullableStruct()
{
var fromRouteRequestDelegate = RequestDelegateFactory.Create((HttpContext httpContext, MyBindAsyncStruct? tryParsable) => { });

var httpContext = new DefaultHttpContext
{
Request =
{
RouteValues =
{
["tryParsable"] = "foo"
},
},
};

await Assert.ThrowsAsync<NotImplementedException>(() => fromRouteRequestDelegate(httpContext));
}

public static object[][] DelegatesWithAttributesOnNotTryParsableParameters
{
get
Expand Down Expand Up @@ -629,11 +754,169 @@ void TestAction([FromRoute] int tryParsable, [FromRoute] int tryParsable2)
Assert.Equal(LogLevel.Debug, logs[0].LogLevel);
Assert.Equal(@"Failed to bind parameter ""Int32 tryParsable"" from ""invalid!"".", logs[0].Message);

Assert.Equal(new EventId(3, "ParamaterBindingFailed"), logs[0].EventId);
Assert.Equal(LogLevel.Debug, logs[0].LogLevel);
Assert.Equal(new EventId(3, "ParamaterBindingFailed"), logs[1].EventId);
Assert.Equal(LogLevel.Debug, logs[1].LogLevel);
Assert.Equal(@"Failed to bind parameter ""Int32 tryParsable2"" from ""invalid again!"".", logs[1].Message);
}

[Fact]
public async Task RequestDelegateLogsBindAsyncFailuresAndSets400Response()
{
// Not supplying any headers will cause the HttpContext TryParse overload to fail.
var httpContext = new DefaultHttpContext()
{
RequestServices = new ServiceCollection().AddSingleton(LoggerFactory).BuildServiceProvider(),
};

var invoked = false;

var requestDelegate = RequestDelegateFactory.Create((MyBindAsyncRecord arg1, MyBindAsyncRecord arg2) =>
{
invoked = true;
});

await requestDelegate(httpContext);

Assert.False(invoked);
Assert.False(httpContext.RequestAborted.IsCancellationRequested);
Assert.Equal(400, httpContext.Response.StatusCode);

var logs = TestSink.Writes.ToArray();

Assert.Equal(2, logs.Length);

Assert.Equal(new EventId(4, "RequiredParameterNotProvided"), logs[0].EventId);
Assert.Equal(LogLevel.Debug, logs[0].LogLevel);
Assert.Equal(@"Required parameter ""MyBindAsyncRecord arg1"" was not provided.", logs[0].Message);

Assert.Equal(new EventId(4, "RequiredParameterNotProvided"), logs[1].EventId);
Assert.Equal(LogLevel.Debug, logs[1].LogLevel);
Assert.Equal(@"Required parameter ""MyBindAsyncRecord arg2"" was not provided.", logs[1].Message);
}

[Fact]
public async Task BindAsyncExceptionsThrowException()
{
// Not supplying any headers will cause the HttpContext TryParse overload to fail.
var httpContext = new DefaultHttpContext()
{
RequestServices = new ServiceCollection().AddSingleton(LoggerFactory).BuildServiceProvider(),
};

var requestDelegate = RequestDelegateFactory.Create((MyBindAsyncTypeThatThrows arg1) => { });

var ex = await Assert.ThrowsAsync<InvalidOperationException>(() => requestDelegate(httpContext));
Assert.Equal("BindAsync failed", ex.Message);
}

[Fact]
public async Task BindAsyncWithBodyArgument()
{
Todo originalTodo = new()
{
Name = "Write more tests!"
};

var httpContext = new DefaultHttpContext();

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

httpContext.Request.Headers["Content-Type"] = "application/json";
httpContext.Request.Headers["Content-Length"] = stream.Length.ToString();
httpContext.Features.Set<IHttpRequestBodyDetectionFeature>(new RequestBodyDetectionFeature(true));

var jsonOptions = new JsonOptions();
jsonOptions.SerializerOptions.Converters.Add(new TodoJsonConverter());

var mock = new Mock<IServiceProvider>();
mock.Setup(m => m.GetService(It.IsAny<Type>())).Returns<Type>(t =>
{
if (t == typeof(IOptions<JsonOptions>))
{
return Options.Create(jsonOptions);
}
return null;
});

httpContext.RequestServices = mock.Object;
httpContext.Request.Headers.Referer = "https://example.org";

var invoked = false;

var requestDelegate = RequestDelegateFactory.Create((HttpContext context, MyBindAsyncRecord arg1, Todo todo) =>
{
invoked = true;
context.Items[nameof(arg1)] = arg1;
context.Items[nameof(todo)] = todo;
});

await requestDelegate(httpContext);

Assert.True(invoked);
var arg = httpContext.Items["arg1"] as MyBindAsyncRecord;
Assert.NotNull(arg);
Assert.Equal("https://example.org/", arg!.Uri.ToString());
var todo = httpContext.Items["todo"] as Todo;
Assert.NotNull(todo);
Assert.Equal("Write more tests!", todo!.Name);
}

[Fact]
public async Task BindAsyncRunsBeforeBodyBinding()
{
Todo originalTodo = new()
{
Name = "Write more tests!"
};

var httpContext = new DefaultHttpContext();

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

httpContext.Request.Headers["Content-Type"] = "application/json";
httpContext.Request.Headers["Content-Length"] = stream.Length.ToString();
httpContext.Features.Set<IHttpRequestBodyDetectionFeature>(new RequestBodyDetectionFeature(true));

var jsonOptions = new JsonOptions();
jsonOptions.SerializerOptions.Converters.Add(new TodoJsonConverter());

var mock = new Mock<IServiceProvider>();
mock.Setup(m => m.GetService(It.IsAny<Type>())).Returns<Type>(t =>
{
if (t == typeof(IOptions<JsonOptions>))
{
return Options.Create(jsonOptions);
}
return null;
});

httpContext.RequestServices = mock.Object;
httpContext.Request.Headers.Referer = "https://example.org";

var invoked = false;

var requestDelegate = RequestDelegateFactory.Create((HttpContext context, CustomTodo customTodo, Todo todo) =>
{
invoked = true;
context.Items[nameof(customTodo)] = customTodo;
context.Items[nameof(todo)] = todo;
});

await requestDelegate(httpContext);

Assert.True(invoked);
var todo0 = httpContext.Items["customTodo"] as Todo;
Assert.NotNull(todo0);
Assert.Equal("Write more tests!", todo0!.Name);
var todo1 = httpContext.Items["todo"] as Todo;
Assert.NotNull(todo1);
Assert.Equal("Write more tests!", todo1!.Name);
}

[Fact]
public async Task RequestDelegatePopulatesFromQueryParameterBasedOnParameterName()
{
Expand Down Expand Up @@ -1669,6 +1952,26 @@ public async Task RequestDelegateHandlesBodyParamOptionality(Delegate @delegate,
}
}

[Fact]
public async Task RequestDelegateDoesSupportBindAsyncOptionality()
{
var httpContext = new DefaultHttpContext()
{
RequestServices = new ServiceCollection().AddSingleton(LoggerFactory).BuildServiceProvider(),
};

var invoked = false;

var requestDelegate = RequestDelegateFactory.Create((MyBindAsyncRecord? arg1) =>
{
invoked = true;
});

await requestDelegate(httpContext);

Assert.True(invoked);
}

public static IEnumerable<object?[]> ServiceParamOptionalityData
{
get
Expand Down Expand Up @@ -1843,6 +2146,16 @@ private class Todo : ITodo
public bool IsComplete { get; set; }
}

private class CustomTodo : Todo
{
public static async ValueTask<object?> BindAsync(HttpContext context)
{
var body = await context.Request.ReadFromJsonAsync<CustomTodo>();
context.Request.Body.Position = 0;
return body;
}
}

private record struct TodoStruct(int Id, string? Name, bool IsComplete) : ITodo;

private interface ITodo
Expand Down
Loading