Skip to content
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

Added support for binding the raw request body #39388

Merged
merged 24 commits into from
Jan 20, 2022
Merged

Added support for binding the raw request body #39388

Show file tree
Hide file tree
Changes from 12 commits
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
daf6a86
Added support for binding the raw request body
davidfowl Jan 9, 2022
154abd1
Fix some issues add more features
davidfowl Jan 9, 2022
d08f132
Random clean up
davidfowl Jan 9, 2022
3e2fff8
Remove support for byte[] and ReadOnlyMemory<byte>
davidfowl Jan 9, 2022
547ce9b
Fix method names and remove extra fluff
davidfowl Jan 9, 2022
c7d1168
Removed extra space
davidfowl Jan 9, 2022
8a17a3b
Removed JSON from the error message
davidfowl Jan 9, 2022
34244b1
Formatting
davidfowl Jan 9, 2022
7c9b45e
Remove the boxing for raw body binding
davidfowl Jan 10, 2022
d2085fd
Added more tests
davidfowl Jan 10, 2022
f45db2b
Small nit
davidfowl Jan 10, 2022
a0711dc
Added more tests
davidfowl Jan 10, 2022
caf7d66
Remove tuple and use ValueTask instead of Task
davidfowl Jan 10, 2022
df767a7
Set the body to a null stream after consuming it
davidfowl Jan 11, 2022
191a9cf
PR feedback
davidfowl Jan 12, 2022
a6637e0
Make sure we can read from the original pipereader when we unwind fro…
davidfowl Jan 12, 2022
39152b3
Support scenarios where nulling out the stream doesn't change the pip…
davidfowl Jan 12, 2022
0210b28
Remove support for the raw body as a ROS<byte>
davidfowl Jan 20, 2022
f5c305a
Small reverts
davidfowl Jan 20, 2022
5b03d54
Fixed more things
davidfowl Jan 20, 2022
c1f4063
Moar
davidfowl Jan 20, 2022
1cd9735
Reverted more stuff
davidfowl Jan 20, 2022
27c3f48
One more I hope
davidfowl Jan 20, 2022
9a7f593
Fixed the test
davidfowl Jan 20, 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
149 changes: 133 additions & 16 deletions src/Http/Http.Extensions/src/RequestDelegateFactory.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,10 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

using System.Buffers;
using System.Diagnostics;
using System.Globalization;
using System.IO.Pipelines;
using System.Linq;
using System.Linq.Expressions;
using System.Reflection;
Expand Down Expand Up @@ -48,6 +50,8 @@ public static partial class RequestDelegateFactory

private static readonly ParameterExpression TargetExpr = Expression.Parameter(typeof(object), "target");
private static readonly ParameterExpression BodyValueExpr = Expression.Parameter(typeof(object), "bodyValue");
private static readonly ParameterExpression RawBodyValueExpr = Expression.Parameter(typeof(ReadOnlySequence<byte>), "bodyValue");
private static readonly MemberExpression RawBodyIsEmptyExpr = Expression.Property(RawBodyValueExpr, typeof(ReadOnlySequence<byte>).GetProperty(nameof(ReadOnlySequence<byte>.IsEmpty))!);
private static readonly ParameterExpression WasParamCheckFailureExpr = Expression.Variable(typeof(bool), "wasParamCheckFailure");
private static readonly ParameterExpression BoundValuesArrayExpr = Expression.Parameter(typeof(object[]), "boundValues");

Expand All @@ -61,6 +65,8 @@ public static partial class RequestDelegateFactory
private static readonly MemberExpression QueryExpr = Expression.Property(HttpRequestExpr, typeof(HttpRequest).GetProperty(nameof(HttpRequest.Query))!);
private static readonly MemberExpression HeadersExpr = Expression.Property(HttpRequestExpr, typeof(HttpRequest).GetProperty(nameof(HttpRequest.Headers))!);
private static readonly MemberExpression FormExpr = Expression.Property(HttpRequestExpr, typeof(HttpRequest).GetProperty(nameof(HttpRequest.Form))!);
private static readonly MemberExpression RequestStreamExpr = Expression.Property(HttpRequestExpr, typeof(HttpRequest).GetProperty(nameof(HttpRequest.Body))!);
private static readonly MemberExpression RequestPipeReaderExpr = Expression.Property(HttpRequestExpr, typeof(HttpRequest).GetProperty(nameof(HttpRequest.BodyReader))!);
private static readonly MemberExpression FormFilesExpr = Expression.Property(FormExpr, typeof(IFormCollection).GetProperty(nameof(IFormCollection.Files))!);
private static readonly MemberExpression StatusCodeExpr = Expression.Property(HttpResponseExpr, typeof(HttpResponse).GetProperty(nameof(HttpResponse.StatusCode))!);
private static readonly MemberExpression CompletedTaskExpr = Expression.Property(null, (PropertyInfo)GetMemberInfo<Func<Task>>(() => Task.CompletedTask));
Expand Down Expand Up @@ -199,10 +205,10 @@ private static Expression[] CreateArguments(ParameterInfo[]? parameters, Factory
var errorMessage = BuildErrorMessageForInferredBodyParameter(factoryContext);
throw new InvalidOperationException(errorMessage);
}
if (factoryContext.JsonRequestBodyParameter is not null &&
if (factoryContext.RequestBodyParameter is not null &&
factoryContext.FirstFormRequestBodyParameter is not null)
{
var errorMessage = BuildErrorMessageForFormAndJsonBodyParameters(factoryContext);
var errorMessage = BuildErrorMessageForFormAndBodyParameters(factoryContext);
throw new InvalidOperationException(errorMessage);
}
if (factoryContext.HasMultipleBodyParameters)
Expand Down Expand Up @@ -302,6 +308,14 @@ private static Expression CreateArgument(ParameterInfo parameter, FactoryContext
{
return BindParameterFromFormFile(parameter, parameter.Name, factoryContext, RequestDelegateFactoryConstants.FormFileParameter);
}
else if (parameter.ParameterType == typeof(Stream))
{
return RequestStreamExpr;
}
else if (parameter.ParameterType == typeof(PipeReader))
{
return RequestPipeReaderExpr;
}
else if (ParameterBindingMethodCache.HasBindAsyncMethod(parameter))
{
return BindParameterFromBindAsync(parameter, factoryContext);
Expand Down Expand Up @@ -549,7 +563,7 @@ private static Expression AddResponseWritingToMethodCall(Expression methodCall,

private static Func<object?, HttpContext, Task> HandleRequestBodyAndCompileRequestDelegate(Expression responseWritingMethodCall, FactoryContext factoryContext)
{
if (factoryContext.JsonRequestBodyParameter is null && !factoryContext.ReadForm)
if (factoryContext.RequestBodyParameter is null && !factoryContext.ReadForm)
{
if (factoryContext.ParameterBinders.Count > 0)
{
Expand Down Expand Up @@ -582,19 +596,102 @@ private static Expression AddResponseWritingToMethodCall(Expression methodCall,
{
return HandleRequestBodyAndCompileRequestDelegateForForm(responseWritingMethodCall, factoryContext);
}
else if (factoryContext.IsRawRequestBody)
{
return HandleRequestBodyAndCompileRequestDelegateForRawBody(responseWritingMethodCall, factoryContext);
}

return HandleRequestBodyAndCompileRequestDelegateForJsonBody(responseWritingMethodCall, factoryContext);
}

private static Func<object?, HttpContext, Task> HandleRequestBodyAndCompileRequestDelegateForRawBody(Expression responseWritingMethodCall, FactoryContext factoryContext)
{
Debug.Assert(factoryContext.RequestBodyParameter is not null, "factoryContext.RequestBodyParameter is null for a body parameter.");
Copy link
Contributor

Choose a reason for hiding this comment

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

Did you mean

Suggested change
Debug.Assert(factoryContext.RequestBodyParameter is not null, "factoryContext.RequestBodyParameter is null for a body parameter.");
Debug.Assert(factoryContext.RequestBodyParameter is null, "factoryContext.RequestBodyParameter is null for a body parameter.");

Copy link
Member Author

@davidfowl davidfowl Jan 12, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Don't think so. We're asserting that it isn't null

Debug.Assert(factoryContext.RequestBodyParameter.Name is not null, "CreateArgument() should throw if parameter.Name is null.");

if (factoryContext.ParameterBinders.Count > 0)
{
// We need to generate the code for reading from the body before calling into the delegate
var continuation = Expression.Lambda<Func<object?, HttpContext, ReadOnlySequence<byte>, object?[], Task>>(
Copy link
Contributor

Choose a reason for hiding this comment

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

Noob question - why is this task returning (vs ValueTask returning)?

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 could be either but using Task vs ValueTask but there’s no benefit to using ValueTask here. There’s no pooling to be had and synchronous completion won’t allocate regardless

responseWritingMethodCall, TargetExpr, HttpContextExpr, RawBodyValueExpr, BoundValuesArrayExpr).Compile();

// Looping over arrays is faster
var binders = factoryContext.ParameterBinders.ToArray();
var count = binders.Length;
Copy link
Contributor

Choose a reason for hiding this comment

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

You could capture this inside the lamda, no?

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 originally didn’t do it would only capture these locals only (in case we nulled out the factory context later).


return async (target, httpContext) =>
{
// Run these first so that they can potentially read and rewind the body
var boundValues = new object?[count];

for (var i = 0; i < count; i++)
{
boundValues[i] = await binders[i](httpContext);
}

var (bodyValue, successful) = await TryReadBodyAsync(httpContext);

if (!successful)
{
return;
}

await continuation(target, httpContext, bodyValue, boundValues);
};
}
else
{
return HandleRequestBodyAndCompileRequestDelegateForJson(responseWritingMethodCall, factoryContext);
// We need to generate the code for reading from the body before calling into the delegate
var continuation = Expression.Lambda<Func<object?, HttpContext, ReadOnlySequence<byte>, Task>>(
responseWritingMethodCall, TargetExpr, HttpContextExpr, RawBodyValueExpr).Compile();

return async (target, httpContext) =>
{
var (bodyValue, successful) = await TryReadBodyAsync(httpContext);

if (!successful)
{
return;
}

await continuation(target, httpContext, bodyValue);
};
}

static async Task<(ReadOnlySequence<byte>, bool)> TryReadBodyAsync(HttpContext httpContext)
{
var feature = httpContext.Features.Get<IHttpRequestBodyDetectionFeature>();

if (feature?.CanHaveBody == true)
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 with JSON if there isn't a body to read from? We should try and emulate that behavior.

Copy link
Member Author

Choose a reason for hiding this comment

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

If allow empty body is true it’ll bind otherwise 400

{
var bodyReader = httpContext.Request.BodyReader;

while (true)
{
var result = await bodyReader.ReadAsync();
var buffer = result.Buffer;

if (result.IsCompleted)
{
return (buffer, true);
}

// Buffer the body
bodyReader.AdvanceTo(buffer.Start, buffer.End);
}
}

return (default, true);
}
}

private static Func<object?, HttpContext, Task> HandleRequestBodyAndCompileRequestDelegateForJson(Expression responseWritingMethodCall, FactoryContext factoryContext)
private static Func<object?, HttpContext, Task> HandleRequestBodyAndCompileRequestDelegateForJsonBody(Expression responseWritingMethodCall, FactoryContext factoryContext)
{
Debug.Assert(factoryContext.JsonRequestBodyParameter is not null, "factoryContext.JsonRequestBodyParameter is null for a JSON body.");
Debug.Assert(factoryContext.RequestBodyParameter is not null, "factoryContext.RequestBodyParameter is null for a body parameter.");

var bodyType = factoryContext.JsonRequestBodyParameter.ParameterType;
var parameterTypeName = TypeNameHelper.GetTypeDisplayName(factoryContext.JsonRequestBodyParameter.ParameterType, fullName: false);
var parameterName = factoryContext.JsonRequestBodyParameter.Name;
var bodyType = factoryContext.RequestBodyParameter.ParameterType;
var parameterTypeName = TypeNameHelper.GetTypeDisplayName(factoryContext.RequestBodyParameter.ParameterType, fullName: false);
var parameterName = factoryContext.RequestBodyParameter.Name;

Debug.Assert(parameterName is not null, "CreateArgument() should throw if parameter.Name is null.");

Expand Down Expand Up @@ -1157,7 +1254,7 @@ private static Expression BindParameterFromFormFile(

private static Expression BindParameterFromBody(ParameterInfo parameter, bool allowEmpty, FactoryContext factoryContext)
{
if (factoryContext.JsonRequestBodyParameter is not null)
if (factoryContext.RequestBodyParameter is not null)
{
factoryContext.HasMultipleBodyParameters = true;
var parameterName = parameter.Name;
Expand All @@ -1173,9 +1270,14 @@ private static Expression BindParameterFromBody(ParameterInfo parameter, bool al

var isOptional = IsOptionalParameter(parameter, factoryContext);

factoryContext.JsonRequestBodyParameter = parameter;
factoryContext.RequestBodyParameter = parameter;
factoryContext.AllowEmptyRequestBody = allowEmpty || isOptional;
factoryContext.Metadata.Add(new AcceptsMetadata(parameter.ParameterType, factoryContext.AllowEmptyRequestBody, DefaultAcceptsContentType));

// We can't infer the accept content type for raw bodies
if (!factoryContext.IsRawRequestBody)
{
factoryContext.Metadata.Add(new AcceptsMetadata(parameter.ParameterType, factoryContext.AllowEmptyRequestBody, DefaultAcceptsContentType));
}

if (!factoryContext.AllowEmptyRequestBody)
{
Expand All @@ -1188,6 +1290,8 @@ private static Expression BindParameterFromBody(ParameterInfo parameter, bool al
// }
factoryContext.ParamCheckExpressions.Add(Expression.Block(
Expression.IfThen(
factoryContext.IsRawRequestBody ?
RawBodyIsEmptyExpr :
Expression.Equal(BodyValueExpr, Expression.Constant(null)),
Expression.Block(
Expression.Assign(WasParamCheckFailureExpr, Expression.Constant(true)),
Expand All @@ -1212,7 +1316,9 @@ private static Expression BindParameterFromBody(ParameterInfo parameter, bool al
// }
var checkRequiredBodyBlock = Expression.Block(
Expression.IfThen(
Expression.Equal(BodyValueExpr, Expression.Constant(null)),
factoryContext.IsRawRequestBody ?
RawBodyIsEmptyExpr :
Expression.Equal(BodyValueExpr, Expression.Constant(null)),
Expression.Block(
Expression.Assign(WasParamCheckFailureExpr, Expression.Constant(true)),
Expression.Call(LogRequiredParameterNotProvidedMethod,
Expand All @@ -1230,12 +1336,22 @@ private static Expression BindParameterFromBody(ParameterInfo parameter, bool al
}
else if (parameter.HasDefaultValue)
{
if (factoryContext.IsRawRequestBody)
{
return Expression.Coalesce(RawBodyValueExpr, Expression.Constant(parameter.DefaultValue));
}

// Convert(bodyValue ?? SomeDefault, Todo)
return Expression.Convert(
Expression.Coalesce(BodyValueExpr, Expression.Constant(parameter.DefaultValue)),
parameter.ParameterType);
}

if (factoryContext.IsRawRequestBody)
{
return RawBodyValueExpr;
}

// Convert(bodyValue, Todo)
return Expression.Convert(BodyValueExpr, parameter.ParameterType);
}
Expand Down Expand Up @@ -1445,8 +1561,9 @@ private class FactoryContext
public bool DisableInferredFromBody { get; init; }

// Temporary State
public ParameterInfo? JsonRequestBodyParameter { get; set; }
public ParameterInfo? RequestBodyParameter { get; set; }
public bool AllowEmptyRequestBody { get; set; }
public bool IsRawRequestBody => RequestBodyParameter?.ParameterType == typeof(ReadOnlySequence<byte>);
Copy link
Contributor

Choose a reason for hiding this comment

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

It'd be interesting to see if people actually start using it. It's a somewhat obtuse type to actually use (particularly when you can bind the PipeReader instead).

Copy link
Member Author

Choose a reason for hiding this comment

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

It’s something we’d have to advertise. I had support for ReadOnlyMemory<byte> and byte[] but removed them.


public bool UsingTempSourceString { get; set; }
public List<ParameterExpression> ExtraLocals { get; } = new();
Expand Down Expand Up @@ -1682,10 +1799,10 @@ private static string BuildErrorMessageForInferredBodyParameter(FactoryContext f
return errorMessage.ToString();
}

private static string BuildErrorMessageForFormAndJsonBodyParameters(FactoryContext factoryContext)
private static string BuildErrorMessageForFormAndBodyParameters(FactoryContext factoryContext)
{
var errorMessage = new StringBuilder();
errorMessage.AppendLine("An action cannot use both form and JSON body parameters.");
errorMessage.AppendLine("An action cannot use both form and body parameters.");
errorMessage.AppendLine("Below is the list of parameters that we found: ");
errorMessage.AppendLine();
errorMessage.AppendLine(FormattableString.Invariant($"{"Parameter",-20}| {"Source",-30}"));
Expand Down
Loading