From daf6a8695774744e58955f3cc26b8f6211d5a50c Mon Sep 17 00:00:00 2001 From: David Fowler Date: Sat, 8 Jan 2022 21:38:28 -0800 Subject: [PATCH 01/24] Added support for binding the raw request body - Added support for Stream, ReadOnlySequence, ReadOnlyMemory and byte[] - Added test --- .../src/RequestDelegateFactory.cs | 61 +++++++++++++--- .../test/RequestDelegateFactoryTests.cs | 69 +++++++++++++++++++ 2 files changed, 121 insertions(+), 9 deletions(-) diff --git a/src/Http/Http.Extensions/src/RequestDelegateFactory.cs b/src/Http/Http.Extensions/src/RequestDelegateFactory.cs index 5eefca8f24c0..d32aa4948533 100644 --- a/src/Http/Http.Extensions/src/RequestDelegateFactory.cs +++ b/src/Http/Http.Extensions/src/RequestDelegateFactory.cs @@ -1,6 +1,7 @@ // 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.Linq; @@ -61,6 +62,7 @@ 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 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>(() => Task.CompletedTask)); @@ -199,7 +201,7 @@ 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); @@ -302,6 +304,10 @@ 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 (ParameterBindingMethodCache.HasBindAsyncMethod(parameter)) { return BindParameterFromBindAsync(parameter, factoryContext); @@ -549,7 +555,7 @@ private static Expression AddResponseWritingToMethodCall(Expression methodCall, private static Func HandleRequestBodyAndCompileRequestDelegate(Expression responseWritingMethodCall, FactoryContext factoryContext) { - if (factoryContext.JsonRequestBodyParameter is null && !factoryContext.ReadForm) + if (factoryContext.RequestBodyParameter is null && !factoryContext.ReadForm) { if (factoryContext.ParameterBinders.Count > 0) { @@ -590,11 +596,14 @@ private static Expression AddResponseWritingToMethodCall(Expression methodCall, private static Func HandleRequestBodyAndCompileRequestDelegateForJson(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.JsonRequestBodyParameter is null for a JSON body."); - 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 isRawBodyType = bodyType == typeof(byte[]) || + bodyType == typeof(ReadOnlyMemory) || + bodyType == typeof(ReadOnlySequence); + 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."); @@ -621,6 +630,7 @@ private static Expression AddResponseWritingToMethodCall(Expression methodCall, var (bodyValue, successful) = await TryReadBodyAsync( httpContext, bodyType, + isRawBodyType, parameterTypeName, parameterName, factoryContext.AllowEmptyRequestBody, @@ -645,6 +655,7 @@ private static Expression AddResponseWritingToMethodCall(Expression methodCall, var (bodyValue, successful) = await TryReadBodyAsync( httpContext, bodyType, + isRawBodyType, parameterTypeName, parameterName, factoryContext.AllowEmptyRequestBody, @@ -662,6 +673,7 @@ private static Expression AddResponseWritingToMethodCall(Expression methodCall, static async Task<(object? FormValue, bool Successful)> TryReadBodyAsync( HttpContext httpContext, Type bodyType, + bool isRawBodyType, string parameterTypeName, string parameterName, bool allowEmptyRequestBody, @@ -679,6 +691,37 @@ private static Expression AddResponseWritingToMethodCall(Expression methodCall, if (feature?.CanHaveBody == true) { + if (isRawBodyType) + { + var bodyReader = httpContext.Request.BodyReader; + + while (true) + { + var result = await bodyReader.ReadAsync(); + var buffer = result.Buffer; + + if (result.IsCompleted) + { + if (bodyType == typeof(ReadOnlySequence)) + { + // REVIEW: Does this need to be a copy? We can tell users to consume the buffer + // immediately in the action + return (buffer, true); + } + + // LOH be damned + if (bodyType == typeof(ReadOnlyMemory)) + { + return (new ReadOnlyMemory(buffer.ToArray()), true); + } + + return (buffer.ToArray(), true); + } + + bodyReader.AdvanceTo(buffer.Start, buffer.End); + } + } + if (!httpContext.Request.HasJsonContentType()) { Log.UnexpectedJsonContentType(httpContext, httpContext.Request.ContentType, throwOnBadRequest); @@ -1157,7 +1200,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; @@ -1173,7 +1216,7 @@ 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)); @@ -1445,7 +1488,7 @@ 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 UsingTempSourceString { get; set; } diff --git a/src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs b/src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs index b659ba4c152c..b6ef2488273e 100644 --- a/src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs +++ b/src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs @@ -3,6 +3,7 @@ #nullable enable +using System.Buffers; using System.Globalization; using System.IO.Pipelines; using System.Linq.Expressions; @@ -1376,6 +1377,74 @@ public async Task RequestDelegatePopulatesFromBodyParameter(Delegate action) Assert.Equal(originalTodo.Name, ((ITodo)deserializedRequestBody!).Name); } + public static object[][] RawFromBodyActions + { + get + { + void TestByteArray(HttpContext httpContext, byte[] body) + { + httpContext.Items.Add("body", body); + } + + void TestReadOnlyMemory(HttpContext httpContext, ReadOnlyMemory body) + { + httpContext.Items.Add("body", body.ToArray()); + } + + void TestReadOnlySequence(HttpContext httpContext, ReadOnlySequence body) + { + httpContext.Items.Add("body", body.ToArray()); + } + + void TestStream(HttpContext httpContext, Stream stream) + { + var ms = new MemoryStream(); + stream.CopyTo(ms); + httpContext.Items.Add("body", ms.ToArray()); + } + + return new[] + { + new[] { (Action)TestByteArray }, + new[] { (Action>)TestReadOnlyMemory }, + new object[] { (Action>)TestReadOnlySequence }, + new object[] { (Action)TestStream }, + }; + } + } + + + [Theory] + [MemberData(nameof(RawFromBodyActions))] + public async Task RequestDelegatePopulatesFromRawBodyParameter(Delegate action) + { + var httpContext = CreateHttpContext(); + + var requestBodyBytes = JsonSerializer.SerializeToUtf8Bytes(new + { + Name = "Write more tests!" + }); + + var stream = new MemoryStream(requestBodyBytes); + httpContext.Request.Body = stream; + + httpContext.Request.Headers["Content-Length"] = stream.Length.ToString(CultureInfo.InvariantCulture); + httpContext.Features.Set(new RequestBodyDetectionFeature(true)); + + var mock = new Mock(); + httpContext.RequestServices = mock.Object; + + var factoryResult = RequestDelegateFactory.Create(action); + + var requestDelegate = factoryResult.RequestDelegate; + + await requestDelegate(httpContext); + + var rawRequestBody = httpContext.Items["body"]; + Assert.NotNull(rawRequestBody); + Assert.Equal(requestBodyBytes, (byte[])rawRequestBody!); + } + [Theory] [MemberData(nameof(ExplicitFromBodyActions))] public async Task RequestDelegateRejectsEmptyBodyGivenExplicitFromBodyParameter(Delegate action) From 154abd16f570a83647c006202c7558baa921a9b7 Mon Sep 17 00:00:00 2001 From: David Fowler Date: Sun, 9 Jan 2022 08:51:29 -0800 Subject: [PATCH 02/24] Fix some issues add more features - Advance the PipeReader preemptively in the cases where we copy the buffer. - Add support for PipeReader as an argument. --- .../src/RequestDelegateFactory.cs | 20 ++++-- .../test/RequestDelegateFactoryTests.cs | 63 +++++++++++++++++++ 2 files changed, 79 insertions(+), 4 deletions(-) diff --git a/src/Http/Http.Extensions/src/RequestDelegateFactory.cs b/src/Http/Http.Extensions/src/RequestDelegateFactory.cs index d32aa4948533..68304e9008f8 100644 --- a/src/Http/Http.Extensions/src/RequestDelegateFactory.cs +++ b/src/Http/Http.Extensions/src/RequestDelegateFactory.cs @@ -4,6 +4,7 @@ using System.Buffers; using System.Diagnostics; using System.Globalization; +using System.IO.Pipelines; using System.Linq; using System.Linq.Expressions; using System.Reflection; @@ -63,6 +64,7 @@ public static partial class RequestDelegateFactory 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>(() => Task.CompletedTask)); @@ -308,6 +310,10 @@ private static Expression CreateArgument(ParameterInfo parameter, FactoryContext { return RequestStreamExpr; } + else if (parameter.ParameterType == typeof(PipeReader)) + { + return RequestPipeReaderExpr; + } else if (ParameterBindingMethodCache.HasBindAsyncMethod(parameter)) { return BindParameterFromBindAsync(parameter, factoryContext); @@ -705,19 +711,25 @@ private static Expression AddResponseWritingToMethodCall(Expression methodCall, if (bodyType == typeof(ReadOnlySequence)) { // REVIEW: Does this need to be a copy? We can tell users to consume the buffer - // immediately in the action + // immediately in the action (they can copy if they need to). return (buffer, true); } - // LOH be damned + // This is very LOH unfriendly + var copiedBuffer = buffer.ToArray(); + + // Consume the whole thing + bodyReader.AdvanceTo(buffer.End); + if (bodyType == typeof(ReadOnlyMemory)) { - return (new ReadOnlyMemory(buffer.ToArray()), true); + return (new ReadOnlyMemory(copiedBuffer), true); } - return (buffer.ToArray(), true); + return (copiedBuffer, true); } + // Buffer the body bodyReader.AdvanceTo(buffer.Start, buffer.End); } } diff --git a/src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs b/src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs index b6ef2488273e..6e04735260b2 100644 --- a/src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs +++ b/src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs @@ -1403,12 +1403,20 @@ void TestStream(HttpContext httpContext, Stream stream) httpContext.Items.Add("body", ms.ToArray()); } + async Task TestPipeReader(HttpContext httpContext, PipeReader reader) + { + var ms = new MemoryStream(); + await reader.CopyToAsync(ms); + httpContext.Items.Add("body", ms.ToArray()); + } + return new[] { new[] { (Action)TestByteArray }, new[] { (Action>)TestReadOnlyMemory }, new object[] { (Action>)TestReadOnlySequence }, new object[] { (Action)TestStream }, + new object[] { (Func)TestPipeReader }, }; } } @@ -1425,6 +1433,8 @@ public async Task RequestDelegatePopulatesFromRawBodyParameter(Delegate action) Name = "Write more tests!" }); + var responseFeature = (TestHttpResponseFeature)httpContext.Features.Get()!; + var stream = new MemoryStream(requestBodyBytes); httpContext.Request.Body = stream; @@ -1440,6 +1450,47 @@ public async Task RequestDelegatePopulatesFromRawBodyParameter(Delegate action) await requestDelegate(httpContext); + await responseFeature.RunOnCompletedCallbacks(); + + var rawRequestBody = httpContext.Items["body"]; + Assert.NotNull(rawRequestBody); + Assert.Equal(requestBodyBytes, (byte[])rawRequestBody!); + } + + [Fact] + public async Task BindingReadOnlySequenceConsumedAfterDelegateExecuted() + { + var httpContext = CreateHttpContext(); + + var requestBodyBytes = JsonSerializer.SerializeToUtf8Bytes(new + { + Name = "Write more tests!" + }); + + var responseFeature = (TestHttpResponseFeature)httpContext.Features.Get()!; + + var mockReader = new Mock(); + + var stream = new MemoryStream(requestBodyBytes); + httpContext.Request.Body = stream; + + httpContext.Request.Headers["Content-Length"] = stream.Length.ToString(CultureInfo.InvariantCulture); + httpContext.Features.Set(new RequestBodyDetectionFeature(true)); + + var mock = new Mock(); + httpContext.RequestServices = mock.Object; + + var factoryResult = RequestDelegateFactory.Create((HttpContext context, ReadOnlySequence body) => + { + httpContext.Items["body"] = body.ToArray(); + }); + + var requestDelegate = factoryResult.RequestDelegate; + + await requestDelegate(httpContext); + + await responseFeature.RunOnCompletedCallbacks(); + var rawRequestBody = httpContext.Items["body"]; Assert.NotNull(rawRequestBody); Assert.Equal(requestBodyBytes, (byte[])rawRequestBody!); @@ -4134,6 +4185,8 @@ public void Abort() private class TestHttpResponseFeature : IHttpResponseFeature, IHttpResponseBodyFeature { + private Stack<(Func, object)> _onCompleted = new(); + public int StatusCode { get; set; } = 200; public string? ReasonPhrase { get; set; } public IHeaderDictionary Headers { get; set; } = new HeaderDictionary(); @@ -4199,6 +4252,16 @@ public void OnStarting(Func callback, object state) public void OnCompleted(Func callback, object state) { + _onCompleted.Push((callback, state)); + } + + public async Task RunOnCompletedCallbacks() + { + while (_onCompleted.TryPop(out var result)) + { + var (callback, state) = result; + await callback(state); + } } } From d08f132d29bb581a061b8a44fce02dd58c0deb38 Mon Sep 17 00:00:00 2001 From: David Fowler Date: Sun, 9 Jan 2022 08:53:49 -0800 Subject: [PATCH 03/24] Random clean up --- src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs b/src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs index 6e04735260b2..b4a790e91769 100644 --- a/src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs +++ b/src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs @@ -1469,8 +1469,6 @@ public async Task BindingReadOnlySequenceConsumedAfterDelegateExecuted() var responseFeature = (TestHttpResponseFeature)httpContext.Features.Get()!; - var mockReader = new Mock(); - var stream = new MemoryStream(requestBodyBytes); httpContext.Request.Body = stream; From 3e2fff8cba37d657d3049e70d476f55f50f4c7e3 Mon Sep 17 00:00:00 2001 From: David Fowler Date: Sun, 9 Jan 2022 13:12:07 -0800 Subject: [PATCH 04/24] Remove support for byte[] and ReadOnlyMemory --- .../src/RequestDelegateFactory.cs | 24 +-------- .../test/RequestDelegateFactoryTests.cs | 49 ------------------- 2 files changed, 2 insertions(+), 71 deletions(-) diff --git a/src/Http/Http.Extensions/src/RequestDelegateFactory.cs b/src/Http/Http.Extensions/src/RequestDelegateFactory.cs index 68304e9008f8..d20da7bd593f 100644 --- a/src/Http/Http.Extensions/src/RequestDelegateFactory.cs +++ b/src/Http/Http.Extensions/src/RequestDelegateFactory.cs @@ -605,9 +605,7 @@ private static Expression AddResponseWritingToMethodCall(Expression methodCall, Debug.Assert(factoryContext.RequestBodyParameter is not null, "factoryContext.JsonRequestBodyParameter is null for a JSON body."); var bodyType = factoryContext.RequestBodyParameter.ParameterType; - var isRawBodyType = bodyType == typeof(byte[]) || - bodyType == typeof(ReadOnlyMemory) || - bodyType == typeof(ReadOnlySequence); + var isRawBodyType = bodyType == typeof(ReadOnlySequence); var parameterTypeName = TypeNameHelper.GetTypeDisplayName(factoryContext.RequestBodyParameter.ParameterType, fullName: false); var parameterName = factoryContext.RequestBodyParameter.Name; @@ -708,25 +706,7 @@ private static Expression AddResponseWritingToMethodCall(Expression methodCall, if (result.IsCompleted) { - if (bodyType == typeof(ReadOnlySequence)) - { - // REVIEW: Does this need to be a copy? We can tell users to consume the buffer - // immediately in the action (they can copy if they need to). - return (buffer, true); - } - - // This is very LOH unfriendly - var copiedBuffer = buffer.ToArray(); - - // Consume the whole thing - bodyReader.AdvanceTo(buffer.End); - - if (bodyType == typeof(ReadOnlyMemory)) - { - return (new ReadOnlyMemory(copiedBuffer), true); - } - - return (copiedBuffer, true); + return (buffer, true); } // Buffer the body diff --git a/src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs b/src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs index b4a790e91769..ffeeeb23988a 100644 --- a/src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs +++ b/src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs @@ -1381,16 +1381,6 @@ public static object[][] RawFromBodyActions { get { - void TestByteArray(HttpContext httpContext, byte[] body) - { - httpContext.Items.Add("body", body); - } - - void TestReadOnlyMemory(HttpContext httpContext, ReadOnlyMemory body) - { - httpContext.Items.Add("body", body.ToArray()); - } - void TestReadOnlySequence(HttpContext httpContext, ReadOnlySequence body) { httpContext.Items.Add("body", body.ToArray()); @@ -1412,8 +1402,6 @@ async Task TestPipeReader(HttpContext httpContext, PipeReader reader) return new[] { - new[] { (Action)TestByteArray }, - new[] { (Action>)TestReadOnlyMemory }, new object[] { (Action>)TestReadOnlySequence }, new object[] { (Action)TestStream }, new object[] { (Func)TestPipeReader }, @@ -1457,43 +1445,6 @@ public async Task RequestDelegatePopulatesFromRawBodyParameter(Delegate action) Assert.Equal(requestBodyBytes, (byte[])rawRequestBody!); } - [Fact] - public async Task BindingReadOnlySequenceConsumedAfterDelegateExecuted() - { - var httpContext = CreateHttpContext(); - - var requestBodyBytes = JsonSerializer.SerializeToUtf8Bytes(new - { - Name = "Write more tests!" - }); - - var responseFeature = (TestHttpResponseFeature)httpContext.Features.Get()!; - - var stream = new MemoryStream(requestBodyBytes); - httpContext.Request.Body = stream; - - httpContext.Request.Headers["Content-Length"] = stream.Length.ToString(CultureInfo.InvariantCulture); - httpContext.Features.Set(new RequestBodyDetectionFeature(true)); - - var mock = new Mock(); - httpContext.RequestServices = mock.Object; - - var factoryResult = RequestDelegateFactory.Create((HttpContext context, ReadOnlySequence body) => - { - httpContext.Items["body"] = body.ToArray(); - }); - - var requestDelegate = factoryResult.RequestDelegate; - - await requestDelegate(httpContext); - - await responseFeature.RunOnCompletedCallbacks(); - - var rawRequestBody = httpContext.Items["body"]; - Assert.NotNull(rawRequestBody); - Assert.Equal(requestBodyBytes, (byte[])rawRequestBody!); - } - [Theory] [MemberData(nameof(ExplicitFromBodyActions))] public async Task RequestDelegateRejectsEmptyBodyGivenExplicitFromBodyParameter(Delegate action) From 547ce9b52206a0a7d15376646968c7a312672af4 Mon Sep 17 00:00:00 2001 From: David Fowler Date: Sun, 9 Jan 2022 14:06:17 -0800 Subject: [PATCH 05/24] Fix method names and remove extra fluff --- .../src/RequestDelegateFactory.cs | 6 +++--- .../test/RequestDelegateFactoryTests.cs | 16 ---------------- 2 files changed, 3 insertions(+), 19 deletions(-) diff --git a/src/Http/Http.Extensions/src/RequestDelegateFactory.cs b/src/Http/Http.Extensions/src/RequestDelegateFactory.cs index d20da7bd593f..4e3d4adcc84a 100644 --- a/src/Http/Http.Extensions/src/RequestDelegateFactory.cs +++ b/src/Http/Http.Extensions/src/RequestDelegateFactory.cs @@ -596,13 +596,13 @@ private static Expression AddResponseWritingToMethodCall(Expression methodCall, } else { - return HandleRequestBodyAndCompileRequestDelegateForJson(responseWritingMethodCall, factoryContext); + return HandleRequestBodyAndCompileRequestDelegateForOtherBody(responseWritingMethodCall, factoryContext); } } - private static Func HandleRequestBodyAndCompileRequestDelegateForJson(Expression responseWritingMethodCall, FactoryContext factoryContext) + private static Func HandleRequestBodyAndCompileRequestDelegateForOtherBody(Expression responseWritingMethodCall, FactoryContext factoryContext) { - Debug.Assert(factoryContext.RequestBodyParameter 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.RequestBodyParameter.ParameterType; var isRawBodyType = bodyType == typeof(ReadOnlySequence); diff --git a/src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs b/src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs index ffeeeb23988a..c6a5b226c965 100644 --- a/src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs +++ b/src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs @@ -1421,8 +1421,6 @@ public async Task RequestDelegatePopulatesFromRawBodyParameter(Delegate action) Name = "Write more tests!" }); - var responseFeature = (TestHttpResponseFeature)httpContext.Features.Get()!; - var stream = new MemoryStream(requestBodyBytes); httpContext.Request.Body = stream; @@ -1438,8 +1436,6 @@ public async Task RequestDelegatePopulatesFromRawBodyParameter(Delegate action) await requestDelegate(httpContext); - await responseFeature.RunOnCompletedCallbacks(); - var rawRequestBody = httpContext.Items["body"]; Assert.NotNull(rawRequestBody); Assert.Equal(requestBodyBytes, (byte[])rawRequestBody!); @@ -4134,8 +4130,6 @@ public void Abort() private class TestHttpResponseFeature : IHttpResponseFeature, IHttpResponseBodyFeature { - private Stack<(Func, object)> _onCompleted = new(); - public int StatusCode { get; set; } = 200; public string? ReasonPhrase { get; set; } public IHeaderDictionary Headers { get; set; } = new HeaderDictionary(); @@ -4201,16 +4195,6 @@ public void OnStarting(Func callback, object state) public void OnCompleted(Func callback, object state) { - _onCompleted.Push((callback, state)); - } - - public async Task RunOnCompletedCallbacks() - { - while (_onCompleted.TryPop(out var result)) - { - var (callback, state) = result; - await callback(state); - } } } From c7d11689a11a15bcea28fe2e09982241d1944994 Mon Sep 17 00:00:00 2001 From: David Fowler Date: Sun, 9 Jan 2022 14:07:01 -0800 Subject: [PATCH 06/24] Removed extra space --- src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs | 1 - 1 file changed, 1 deletion(-) diff --git a/src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs b/src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs index c6a5b226c965..57d2c6460be3 100644 --- a/src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs +++ b/src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs @@ -1409,7 +1409,6 @@ async Task TestPipeReader(HttpContext httpContext, PipeReader reader) } } - [Theory] [MemberData(nameof(RawFromBodyActions))] public async Task RequestDelegatePopulatesFromRawBodyParameter(Delegate action) From 8a17a3ba1ee1f553724ccda7a51cb8ee51ebba35 Mon Sep 17 00:00:00 2001 From: David Fowler Date: Sun, 9 Jan 2022 14:14:03 -0800 Subject: [PATCH 07/24] Removed JSON from the error message --- src/Http/Http.Extensions/src/RequestDelegateFactory.cs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/Http/Http.Extensions/src/RequestDelegateFactory.cs b/src/Http/Http.Extensions/src/RequestDelegateFactory.cs index 4e3d4adcc84a..7fc8063249fc 100644 --- a/src/Http/Http.Extensions/src/RequestDelegateFactory.cs +++ b/src/Http/Http.Extensions/src/RequestDelegateFactory.cs @@ -206,7 +206,7 @@ private static Expression[] CreateArguments(ParameterInfo[]? parameters, Factory 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) @@ -1717,10 +1717,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}")); From 34244b10cd55aff2baca357612bcd1d679bb51f7 Mon Sep 17 00:00:00 2001 From: David Fowler Date: Sun, 9 Jan 2022 14:17:40 -0800 Subject: [PATCH 08/24] Formatting --- .../Http.Extensions/test/RequestDelegateFactoryTests.cs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs b/src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs index 57d2c6460be3..d5ae35b95d03 100644 --- a/src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs +++ b/src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs @@ -1402,10 +1402,10 @@ async Task TestPipeReader(HttpContext httpContext, PipeReader reader) return new[] { - new object[] { (Action>)TestReadOnlySequence }, - new object[] { (Action)TestStream }, - new object[] { (Func)TestPipeReader }, - }; + new object[] { (Action>)TestReadOnlySequence }, + new object[] { (Action)TestStream }, + new object[] { (Func)TestPipeReader }, + }; } } From 7c9b45e5496a19ec25c376f75b652607924ec4d8 Mon Sep 17 00:00:00 2001 From: David Fowler Date: Sun, 9 Jan 2022 16:19:08 -0800 Subject: [PATCH 09/24] Remove the boxing for raw body binding - Split the code path for raw body binding with ReadOnlySequence to avoid boxing. --- .../src/RequestDelegateFactory.cs | 139 ++++++++++++++---- 1 file changed, 112 insertions(+), 27 deletions(-) diff --git a/src/Http/Http.Extensions/src/RequestDelegateFactory.cs b/src/Http/Http.Extensions/src/RequestDelegateFactory.cs index 7fc8063249fc..1ea8370d147c 100644 --- a/src/Http/Http.Extensions/src/RequestDelegateFactory.cs +++ b/src/Http/Http.Extensions/src/RequestDelegateFactory.cs @@ -50,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), "bodyValue"); + private static readonly MemberExpression RawBodyIsEmptyExpr = Expression.Property(RawBodyValueExpr, typeof(ReadOnlySequence).GetProperty(nameof(ReadOnlySequence.IsEmpty))!); private static readonly ParameterExpression WasParamCheckFailureExpr = Expression.Variable(typeof(bool), "wasParamCheckFailure"); private static readonly ParameterExpression BoundValuesArrayExpr = Expression.Parameter(typeof(object[]), "boundValues"); @@ -594,18 +596,103 @@ 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 HandleRequestBodyAndCompileRequestDelegateForRawBody(Expression responseWritingMethodCall, FactoryContext factoryContext) + { + Debug.Assert(factoryContext.RequestBodyParameter is not null, "factoryContext.RequestBodyParameter is null for a body parameter."); + + var bodyType = factoryContext.RequestBodyParameter.ParameterType; + + 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, object?[], Task>>( + responseWritingMethodCall, TargetExpr, HttpContextExpr, RawBodyValueExpr, BoundValuesArrayExpr).Compile(); + + // Looping over arrays is faster + var binders = factoryContext.ParameterBinders.ToArray(); + var count = binders.Length; + + 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 HandleRequestBodyAndCompileRequestDelegateForOtherBody(responseWritingMethodCall, factoryContext); + // We need to generate the code for reading from the body before calling into the delegate + var continuation = Expression.Lambda, 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, bool)> TryReadBodyAsync(HttpContext httpContext) + { + var feature = httpContext.Features.Get(); + + if (feature?.CanHaveBody == true) + { + 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 HandleRequestBodyAndCompileRequestDelegateForOtherBody(Expression responseWritingMethodCall, FactoryContext factoryContext) + private static Func HandleRequestBodyAndCompileRequestDelegateForJsonBody(Expression responseWritingMethodCall, FactoryContext factoryContext) { Debug.Assert(factoryContext.RequestBodyParameter is not null, "factoryContext.RequestBodyParameter is null for a body parameter."); var bodyType = factoryContext.RequestBodyParameter.ParameterType; - var isRawBodyType = bodyType == typeof(ReadOnlySequence); var parameterTypeName = TypeNameHelper.GetTypeDisplayName(factoryContext.RequestBodyParameter.ParameterType, fullName: false); var parameterName = factoryContext.RequestBodyParameter.Name; @@ -634,7 +721,6 @@ private static Expression AddResponseWritingToMethodCall(Expression methodCall, var (bodyValue, successful) = await TryReadBodyAsync( httpContext, bodyType, - isRawBodyType, parameterTypeName, parameterName, factoryContext.AllowEmptyRequestBody, @@ -659,7 +745,6 @@ private static Expression AddResponseWritingToMethodCall(Expression methodCall, var (bodyValue, successful) = await TryReadBodyAsync( httpContext, bodyType, - isRawBodyType, parameterTypeName, parameterName, factoryContext.AllowEmptyRequestBody, @@ -677,7 +762,6 @@ private static Expression AddResponseWritingToMethodCall(Expression methodCall, static async Task<(object? FormValue, bool Successful)> TryReadBodyAsync( HttpContext httpContext, Type bodyType, - bool isRawBodyType, string parameterTypeName, string parameterName, bool allowEmptyRequestBody, @@ -695,25 +779,6 @@ private static Expression AddResponseWritingToMethodCall(Expression methodCall, if (feature?.CanHaveBody == true) { - if (isRawBodyType) - { - 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); - } - } - if (!httpContext.Request.HasJsonContentType()) { Log.UnexpectedJsonContentType(httpContext, httpContext.Request.ContentType, throwOnBadRequest); @@ -1210,7 +1275,12 @@ private static Expression BindParameterFromBody(ParameterInfo parameter, bool al 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) { @@ -1223,6 +1293,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)), @@ -1247,7 +1319,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, @@ -1265,12 +1339,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); } @@ -1482,6 +1566,7 @@ private class FactoryContext // Temporary State public ParameterInfo? RequestBodyParameter { get; set; } public bool AllowEmptyRequestBody { get; set; } + public bool IsRawRequestBody => RequestBodyParameter?.ParameterType == typeof(ReadOnlySequence); public bool UsingTempSourceString { get; set; } public List ExtraLocals { get; } = new(); From d2085fdc9fe1c9616ae04c6958e707e67e762874 Mon Sep 17 00:00:00 2001 From: David Fowler Date: Sun, 9 Jan 2022 16:22:15 -0800 Subject: [PATCH 10/24] Added more tests --- .../Http.Extensions/test/RequestDelegateFactoryTests.cs | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs b/src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs index d5ae35b95d03..1ef30d19a64e 100644 --- a/src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs +++ b/src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs @@ -1400,11 +1400,18 @@ async Task TestPipeReader(HttpContext httpContext, PipeReader reader) httpContext.Items.Add("body", ms.ToArray()); } + void TestPipeReaderAndROS(HttpContext httpContext, ReadOnlySequence body, PipeReader reader) + { + httpContext.Items.Add("body", body.ToArray()); + reader.AdvanceTo(body.End); + } + return new[] { new object[] { (Action>)TestReadOnlySequence }, new object[] { (Action)TestStream }, new object[] { (Func)TestPipeReader }, + new object[] { (Action, PipeReader>)TestPipeReaderAndROS }, }; } } From f45db2b5b7cb61aca0d6a1fe8d99ade726e358c2 Mon Sep 17 00:00:00 2001 From: David Fowler Date: Sun, 9 Jan 2022 16:36:49 -0800 Subject: [PATCH 11/24] Small nit --- src/Http/Http.Extensions/src/RequestDelegateFactory.cs | 3 --- 1 file changed, 3 deletions(-) diff --git a/src/Http/Http.Extensions/src/RequestDelegateFactory.cs b/src/Http/Http.Extensions/src/RequestDelegateFactory.cs index 1ea8370d147c..245518730690 100644 --- a/src/Http/Http.Extensions/src/RequestDelegateFactory.cs +++ b/src/Http/Http.Extensions/src/RequestDelegateFactory.cs @@ -607,9 +607,6 @@ private static Expression AddResponseWritingToMethodCall(Expression methodCall, private static Func HandleRequestBodyAndCompileRequestDelegateForRawBody(Expression responseWritingMethodCall, FactoryContext factoryContext) { Debug.Assert(factoryContext.RequestBodyParameter is not null, "factoryContext.RequestBodyParameter is null for a body parameter."); - - var bodyType = factoryContext.RequestBodyParameter.ParameterType; - Debug.Assert(factoryContext.RequestBodyParameter.Name is not null, "CreateArgument() should throw if parameter.Name is null."); if (factoryContext.ParameterBinders.Count > 0) From a0711dc2a118aa591893d6333f7e1a9d80592f63 Mon Sep 17 00:00:00 2001 From: David Fowler Date: Sun, 9 Jan 2022 20:20:14 -0800 Subject: [PATCH 12/24] Added more tests --- .../test/RequestDelegateFactoryTests.cs | 51 +++++++++++++++++++ 1 file changed, 51 insertions(+) diff --git a/src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs b/src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs index 1ef30d19a64e..ac0ac0cd57c4 100644 --- a/src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs +++ b/src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs @@ -1381,6 +1381,11 @@ public static object[][] RawFromBodyActions { get { + void ExplicitTestReadOnlySequence(HttpContext httpContext, [FromBody] ReadOnlySequence body) + { + httpContext.Items.Add("body", body.ToArray()); + } + void TestReadOnlySequence(HttpContext httpContext, ReadOnlySequence body) { httpContext.Items.Add("body", body.ToArray()); @@ -1408,6 +1413,7 @@ void TestPipeReaderAndROS(HttpContext httpContext, ReadOnlySequence body, return new[] { + new object[] { (Action>)ExplicitTestReadOnlySequence }, new object[] { (Action>)TestReadOnlySequence }, new object[] { (Action)TestStream }, new object[] { (Func)TestPipeReader }, @@ -1507,6 +1513,51 @@ void TestAction([FromBody(AllowEmpty = true)] Todo todo) Assert.Null(todoToBecomeNull); } + [Fact] + public async Task RequestDelegateAllowsEmptyRawBodyGivenCorrectyConfiguredFromBodyParameter() + { + ReadOnlySequence bodyResult = new(new byte[] { 1, 2, 3, 4 }); + + void TestAction([FromBody(AllowEmpty = true)] ReadOnlySequence body) + { + bodyResult = body; + } + + var httpContext = CreateHttpContext(); + httpContext.Request.Headers["Content-Type"] = "application/json"; + httpContext.Request.Headers["Content-Length"] = "0"; + + var factoryResult = RequestDelegateFactory.Create(TestAction); + var requestDelegate = factoryResult.RequestDelegate; + + await requestDelegate(httpContext); + + Assert.True(bodyResult.IsEmpty); + } + + [Fact] + public async Task RequestDelegateRawBodyNotInvokedIfBodyIsEmptyAndNotAllowed() + { + ReadOnlySequence bodyResult = new(new byte[] { 1, 2, 3, 4 }); + + void TestAction(ReadOnlySequence body) + { + bodyResult = body; + } + + var httpContext = CreateHttpContext(); + httpContext.Request.Headers["Content-Type"] = "application/json"; + httpContext.Request.Headers["Content-Length"] = "0"; + httpContext.Features.Set(new RequestBodyDetectionFeature(true)); + + var factoryResult = RequestDelegateFactory.Create(TestAction); + var requestDelegate = factoryResult.RequestDelegate; + + await requestDelegate(httpContext); + + Assert.False(bodyResult.IsEmpty); + } + [Fact] public async Task RequestDelegateAllowsEmptyBodyStructGivenCorrectyConfiguredFromBodyParameter() { From caf7d66587160034dd4fbe695eba17ca7481fd39 Mon Sep 17 00:00:00 2001 From: David Fowler Date: Mon, 10 Jan 2022 09:55:39 -0800 Subject: [PATCH 13/24] Remove tuple and use ValueTask instead of Task --- .../src/RequestDelegateFactory.cs | 20 +++++-------------- 1 file changed, 5 insertions(+), 15 deletions(-) diff --git a/src/Http/Http.Extensions/src/RequestDelegateFactory.cs b/src/Http/Http.Extensions/src/RequestDelegateFactory.cs index 245518730690..7cb39b51a3ef 100644 --- a/src/Http/Http.Extensions/src/RequestDelegateFactory.cs +++ b/src/Http/Http.Extensions/src/RequestDelegateFactory.cs @@ -629,12 +629,7 @@ private static Expression AddResponseWritingToMethodCall(Expression methodCall, boundValues[i] = await binders[i](httpContext); } - var (bodyValue, successful) = await TryReadBodyAsync(httpContext); - - if (!successful) - { - return; - } + var bodyValue = await ReadBodyAsync(httpContext); await continuation(target, httpContext, bodyValue, boundValues); }; @@ -647,18 +642,13 @@ private static Expression AddResponseWritingToMethodCall(Expression methodCall, return async (target, httpContext) => { - var (bodyValue, successful) = await TryReadBodyAsync(httpContext); - - if (!successful) - { - return; - } + var bodyValue = await ReadBodyAsync(httpContext); await continuation(target, httpContext, bodyValue); }; } - static async Task<(ReadOnlySequence, bool)> TryReadBodyAsync(HttpContext httpContext) + static async ValueTask> ReadBodyAsync(HttpContext httpContext) { var feature = httpContext.Features.Get(); @@ -673,7 +663,7 @@ private static Expression AddResponseWritingToMethodCall(Expression methodCall, if (result.IsCompleted) { - return (buffer, true); + return buffer; } // Buffer the body @@ -681,7 +671,7 @@ private static Expression AddResponseWritingToMethodCall(Expression methodCall, } } - return (default, true); + return default; } } From df767a76a7794d9a9d868f3ca3659d223d4d278d Mon Sep 17 00:00:00 2001 From: David Fowler Date: Tue, 11 Jan 2022 11:53:22 -0800 Subject: [PATCH 14/24] Set the body to a null stream after consuming it - This prevents code that would otherwise read the body from doing so after consuming it. --- .../src/RequestDelegateFactory.cs | 4 ++++ .../test/RequestDelegateFactoryTests.cs | 20 ++++++++++++++++--- 2 files changed, 21 insertions(+), 3 deletions(-) diff --git a/src/Http/Http.Extensions/src/RequestDelegateFactory.cs b/src/Http/Http.Extensions/src/RequestDelegateFactory.cs index 7cb39b51a3ef..64e93e5a4006 100644 --- a/src/Http/Http.Extensions/src/RequestDelegateFactory.cs +++ b/src/Http/Http.Extensions/src/RequestDelegateFactory.cs @@ -663,6 +663,10 @@ static async ValueTask> ReadBodyAsync(HttpContext httpCon if (result.IsCompleted) { + // We're not buffering the body so we want to block consuming code from reading again + // and getting weird errors. Treat further reads as a fully consumed body. + httpContext.Request.Body = Stream.Null; + return buffer; } diff --git a/src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs b/src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs index ac0ac0cd57c4..7539e1604dda 100644 --- a/src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs +++ b/src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs @@ -1405,10 +1405,19 @@ async Task TestPipeReader(HttpContext httpContext, PipeReader reader) httpContext.Items.Add("body", ms.ToArray()); } - void TestPipeReaderAndROS(HttpContext httpContext, ReadOnlySequence body, PipeReader reader) + async Task TestPipeReaderAndROS(HttpContext httpContext, ReadOnlySequence body, PipeReader reader) { + var result = await reader.ReadAsync(); httpContext.Items.Add("body", body.ToArray()); - reader.AdvanceTo(body.End); + httpContext.Items.Add("bodyIsEmpty", result.IsCompleted); + reader.AdvanceTo(result.Buffer.End); + } + + async Task TestStreamAndROS(HttpContext httpContext, ReadOnlySequence body, Stream stream) + { + int read = await stream.ReadAsync(new byte[1].AsMemory()); + httpContext.Items.Add("body", body.ToArray()); + httpContext.Items.Add("bodyIsEmpty", read == 0); } return new[] @@ -1417,7 +1426,8 @@ void TestPipeReaderAndROS(HttpContext httpContext, ReadOnlySequence body, new object[] { (Action>)TestReadOnlySequence }, new object[] { (Action)TestStream }, new object[] { (Func)TestPipeReader }, - new object[] { (Action, PipeReader>)TestPipeReaderAndROS }, + new object[] { (Func, PipeReader, Task>)TestPipeReaderAndROS }, + new object[] { (Func, Stream, Task>)TestStreamAndROS }, }; } } @@ -1451,6 +1461,10 @@ public async Task RequestDelegatePopulatesFromRawBodyParameter(Delegate action) var rawRequestBody = httpContext.Items["body"]; Assert.NotNull(rawRequestBody); Assert.Equal(requestBodyBytes, (byte[])rawRequestBody!); + if (httpContext.Items.TryGetValue("bodyIsEmpty", out var bodyIsEmpty)) + { + Assert.True((bool)bodyIsEmpty!); + } } [Theory] From 191a9cfd5c9fd77cd681fd7b5f38f7f4f4118e39 Mon Sep 17 00:00:00 2001 From: David Fowler Date: Tue, 11 Jan 2022 16:25:12 -0800 Subject: [PATCH 15/24] PR feedback - Restore the original stream after the handler executes. --- .../src/RequestDelegateFactory.cs | 26 ++++++++++++++++--- .../test/RequestDelegateFactoryTests.cs | 2 ++ 2 files changed, 24 insertions(+), 4 deletions(-) diff --git a/src/Http/Http.Extensions/src/RequestDelegateFactory.cs b/src/Http/Http.Extensions/src/RequestDelegateFactory.cs index 64e93e5a4006..56d51bb9412a 100644 --- a/src/Http/Http.Extensions/src/RequestDelegateFactory.cs +++ b/src/Http/Http.Extensions/src/RequestDelegateFactory.cs @@ -629,9 +629,18 @@ private static Expression AddResponseWritingToMethodCall(Expression methodCall, boundValues[i] = await binders[i](httpContext); } - var bodyValue = await ReadBodyAsync(httpContext); + var stream = httpContext.Request.Body; - await continuation(target, httpContext, bodyValue, boundValues); + try + { + var bodyValue = await ReadBodyAsync(httpContext); + + await continuation(target, httpContext, bodyValue, boundValues); + } + finally + { + httpContext.Request.Body = stream; + } }; } else @@ -642,9 +651,18 @@ private static Expression AddResponseWritingToMethodCall(Expression methodCall, return async (target, httpContext) => { - var bodyValue = await ReadBodyAsync(httpContext); + var stream = httpContext.Request.Body; - await continuation(target, httpContext, bodyValue); + try + { + var bodyValue = await ReadBodyAsync(httpContext); + + await continuation(target, httpContext, bodyValue); + } + finally + { + httpContext.Request.Body = stream; + } }; } diff --git a/src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs b/src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs index 7539e1604dda..46b0355b43ea 100644 --- a/src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs +++ b/src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs @@ -1458,6 +1458,8 @@ public async Task RequestDelegatePopulatesFromRawBodyParameter(Delegate action) await requestDelegate(httpContext); + Assert.Same(httpContext.Request.Body, stream); + var rawRequestBody = httpContext.Items["body"]; Assert.NotNull(rawRequestBody); Assert.Equal(requestBodyBytes, (byte[])rawRequestBody!); From a6637e0f7e348705d173dfe79ee094b12fd267f6 Mon Sep 17 00:00:00 2001 From: David Fowler Date: Tue, 11 Jan 2022 16:38:42 -0800 Subject: [PATCH 16/24] Make sure we can read from the original pipereader when we unwind from the call. --- .../test/RequestDelegateFactoryTests.cs | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs b/src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs index 46b0355b43ea..5f46b1d6cd94 100644 --- a/src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs +++ b/src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs @@ -1460,6 +1460,19 @@ public async Task RequestDelegatePopulatesFromRawBodyParameter(Delegate action) Assert.Same(httpContext.Request.Body, stream); + // Assert that we can read the body from both the pipe reader and Stream after executing + httpContext.Request.Body.Position = 0; + byte[] data = new byte[requestBodyBytes.Length]; + int read = await httpContext.Request.Body.ReadAsync(data.AsMemory()); + Assert.Equal(read, data.Length); + Assert.Equal(requestBodyBytes, data); + + httpContext.Request.Body.Position = 0; + var result = await httpContext.Request.BodyReader.ReadAsync(); + Assert.Equal(requestBodyBytes.Length, result.Buffer.Length); + Assert.Equal(requestBodyBytes, result.Buffer.ToArray()); + httpContext.Request.BodyReader.AdvanceTo(result.Buffer.End); + var rawRequestBody = httpContext.Items["body"]; Assert.NotNull(rawRequestBody); Assert.Equal(requestBodyBytes, (byte[])rawRequestBody!); From 39152b3e73df559f0caace14570ad0d55e01ecb3 Mon Sep 17 00:00:00 2001 From: David Fowler Date: Tue, 11 Jan 2022 17:58:33 -0800 Subject: [PATCH 17/24] Support scenarios where nulling out the stream doesn't change the pipereader --- .../src/RequestDelegateFactory.cs | 71 +++++++++++++++---- .../test/RequestDelegateFactoryTests.cs | 58 +++++++++++++++ 2 files changed, 116 insertions(+), 13 deletions(-) diff --git a/src/Http/Http.Extensions/src/RequestDelegateFactory.cs b/src/Http/Http.Extensions/src/RequestDelegateFactory.cs index 56d51bb9412a..a82ae6594c92 100644 --- a/src/Http/Http.Extensions/src/RequestDelegateFactory.cs +++ b/src/Http/Http.Extensions/src/RequestDelegateFactory.cs @@ -630,16 +630,37 @@ private static Expression AddResponseWritingToMethodCall(Expression methodCall, } var stream = httpContext.Request.Body; + IRequestBodyPipeFeature? requestBodyPipe = null; try { - var bodyValue = await ReadBodyAsync(httpContext); + var reader = httpContext.Request.BodyReader; - await continuation(target, httpContext, bodyValue, boundValues); + var body = await ReadBodyAsync(httpContext, reader); + + // We're not buffering the body so we want to block consuming code from reading again + // and getting weird errors. Treat further reads as a fully consumed body. + httpContext.Request.Body = Stream.Null; + + // User code overrode the PipeReader + if (reader == httpContext.Request.BodyReader) + { + requestBodyPipe = httpContext.Features.Get(); + httpContext.Features.Set(new RequestBodyPipeFeature(PipeReader.Create(Stream.Null))); + } + + await continuation(target, httpContext, body, boundValues); + + reader.AdvanceTo(body.End); } finally { httpContext.Request.Body = stream; + + if (requestBodyPipe is not null) + { + httpContext.Features.Set(requestBodyPipe); + } } }; } @@ -652,44 +673,59 @@ private static Expression AddResponseWritingToMethodCall(Expression methodCall, return async (target, httpContext) => { var stream = httpContext.Request.Body; + IRequestBodyPipeFeature? requestBodyPipe = null; try { - var bodyValue = await ReadBodyAsync(httpContext); + var reader = httpContext.Request.BodyReader; + + var body = await ReadBodyAsync(httpContext, reader); - await continuation(target, httpContext, bodyValue); + // We're not buffering the body so we want to block consuming code from reading again + // and getting weird errors. Treat further reads as a fully consumed body. + httpContext.Request.Body = Stream.Null; + + // User code overrode the PipeReader + if (reader == httpContext.Request.BodyReader) + { + requestBodyPipe = httpContext.Features.Get(); + httpContext.Features.Set(new RequestBodyPipeFeature(PipeReader.Create(Stream.Null))); + } + + await continuation(target, httpContext, body); + + reader.AdvanceTo(body.End); } finally { httpContext.Request.Body = stream; + + if (requestBodyPipe is not null) + { + httpContext.Features.Set(requestBodyPipe); + } } }; } - static async ValueTask> ReadBodyAsync(HttpContext httpContext) + static async ValueTask> ReadBodyAsync(HttpContext httpContext, PipeReader reader) { var feature = httpContext.Features.Get(); if (feature?.CanHaveBody == true) { - var bodyReader = httpContext.Request.BodyReader; - while (true) { - var result = await bodyReader.ReadAsync(); + var result = await reader.ReadAsync(); var buffer = result.Buffer; if (result.IsCompleted) { - // We're not buffering the body so we want to block consuming code from reading again - // and getting weird errors. Treat further reads as a fully consumed body. - httpContext.Request.Body = Stream.Null; - return buffer; } // Buffer the body - bodyReader.AdvanceTo(buffer.Start, buffer.End); + reader.AdvanceTo(buffer.Start, buffer.End); } } @@ -1564,6 +1600,15 @@ private static async Task ExecuteResultWriteResponse(IResult? result, HttpContex await EnsureRequestResultNotNull(result).ExecuteAsync(httpContext); } + private sealed class RequestBodyPipeFeature : IRequestBodyPipeFeature + { + public RequestBodyPipeFeature(PipeReader pipeReader) + { + Reader = pipeReader; + } + public PipeReader Reader { get; set; } + } + private class FactoryContext { // Options diff --git a/src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs b/src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs index 5f46b1d6cd94..c57f5c41720f 100644 --- a/src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs +++ b/src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs @@ -1482,6 +1482,64 @@ public async Task RequestDelegatePopulatesFromRawBodyParameter(Delegate action) } } + [Theory] + [MemberData(nameof(RawFromBodyActions))] + public async Task RequestDelegatePopulatesFromRawBodyParameterPipeReader(Delegate action) + { + var httpContext = CreateHttpContext(); + + var requestBodyBytes = JsonSerializer.SerializeToUtf8Bytes(new + { + Name = "Write more tests!" + }); + + var pipeReader = PipeReader.Create(new MemoryStream(requestBodyBytes)); + var stream = pipeReader.AsStream(); + httpContext.Features.Set(new PipeRequestBodyFeature(pipeReader)); + httpContext.Request.Body = stream; + + httpContext.Request.Headers["Content-Length"] = requestBodyBytes.Length.ToString(CultureInfo.InvariantCulture); + httpContext.Features.Set(new RequestBodyDetectionFeature(true)); + + var mock = new Mock(); + httpContext.RequestServices = mock.Object; + + var factoryResult = RequestDelegateFactory.Create(action); + + var requestDelegate = factoryResult.RequestDelegate; + + await requestDelegate(httpContext); + + Assert.Same(httpContext.Request.Body, stream); + Assert.Same(httpContext.Request.BodyReader, pipeReader); + + // Assert that we can read the body from both the pipe reader and Stream after executing and verify that they are empty (the pipe reader isn't seekable here) + int read = await httpContext.Request.Body.ReadAsync(new byte[requestBodyBytes.Length].AsMemory()); + Assert.Equal(0, read); + + var result = await httpContext.Request.BodyReader.ReadAsync(); + Assert.Equal(0, result.Buffer.Length); + Assert.True(result.IsCompleted); + httpContext.Request.BodyReader.AdvanceTo(result.Buffer.End); + + var rawRequestBody = httpContext.Items["body"]; + Assert.NotNull(rawRequestBody); + Assert.Equal(requestBodyBytes, (byte[])rawRequestBody!); + if (httpContext.Items.TryGetValue("bodyIsEmpty", out var bodyIsEmpty)) + { + Assert.True((bool)bodyIsEmpty!); + } + } + + class PipeRequestBodyFeature : IRequestBodyPipeFeature + { + public PipeRequestBodyFeature(PipeReader pipeReader) + { + Reader = pipeReader; + } + public PipeReader Reader { get; set; } + } + [Theory] [MemberData(nameof(ExplicitFromBodyActions))] public async Task RequestDelegateRejectsEmptyBodyGivenExplicitFromBodyParameter(Delegate action) From 0210b28b359cbf9ec00ee844f84463c74d756781 Mon Sep 17 00:00:00 2001 From: David Fowler Date: Wed, 19 Jan 2022 19:44:16 -0800 Subject: [PATCH 18/24] Remove support for the raw body as a ROS --- .../src/RequestDelegateFactory.cs | 152 +----------------- .../test/RequestDelegateFactoryTests.cs | 76 +-------- 2 files changed, 2 insertions(+), 226 deletions(-) diff --git a/src/Http/Http.Extensions/src/RequestDelegateFactory.cs b/src/Http/Http.Extensions/src/RequestDelegateFactory.cs index a82ae6594c92..ae46e7ecf424 100644 --- a/src/Http/Http.Extensions/src/RequestDelegateFactory.cs +++ b/src/Http/Http.Extensions/src/RequestDelegateFactory.cs @@ -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.Buffers; using System.Diagnostics; using System.Globalization; using System.IO.Pipelines; @@ -50,8 +49,6 @@ 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), "bodyValue"); - private static readonly MemberExpression RawBodyIsEmptyExpr = Expression.Property(RawBodyValueExpr, typeof(ReadOnlySequence).GetProperty(nameof(ReadOnlySequence.IsEmpty))!); private static readonly ParameterExpression WasParamCheckFailureExpr = Expression.Variable(typeof(bool), "wasParamCheckFailure"); private static readonly ParameterExpression BoundValuesArrayExpr = Expression.Parameter(typeof(object[]), "boundValues"); @@ -596,143 +593,10 @@ 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 HandleRequestBodyAndCompileRequestDelegateForRawBody(Expression responseWritingMethodCall, FactoryContext factoryContext) - { - Debug.Assert(factoryContext.RequestBodyParameter is not null, "factoryContext.RequestBodyParameter is null for a body parameter."); - 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, object?[], Task>>( - responseWritingMethodCall, TargetExpr, HttpContextExpr, RawBodyValueExpr, BoundValuesArrayExpr).Compile(); - - // Looping over arrays is faster - var binders = factoryContext.ParameterBinders.ToArray(); - var count = binders.Length; - - 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 stream = httpContext.Request.Body; - IRequestBodyPipeFeature? requestBodyPipe = null; - - try - { - var reader = httpContext.Request.BodyReader; - - var body = await ReadBodyAsync(httpContext, reader); - - // We're not buffering the body so we want to block consuming code from reading again - // and getting weird errors. Treat further reads as a fully consumed body. - httpContext.Request.Body = Stream.Null; - - // User code overrode the PipeReader - if (reader == httpContext.Request.BodyReader) - { - requestBodyPipe = httpContext.Features.Get(); - httpContext.Features.Set(new RequestBodyPipeFeature(PipeReader.Create(Stream.Null))); - } - - await continuation(target, httpContext, body, boundValues); - - reader.AdvanceTo(body.End); - } - finally - { - httpContext.Request.Body = stream; - - if (requestBodyPipe is not null) - { - httpContext.Features.Set(requestBodyPipe); - } - } - }; - } - else - { - // We need to generate the code for reading from the body before calling into the delegate - var continuation = Expression.Lambda, Task>>( - responseWritingMethodCall, TargetExpr, HttpContextExpr, RawBodyValueExpr).Compile(); - - return async (target, httpContext) => - { - var stream = httpContext.Request.Body; - IRequestBodyPipeFeature? requestBodyPipe = null; - - try - { - var reader = httpContext.Request.BodyReader; - - var body = await ReadBodyAsync(httpContext, reader); - - // We're not buffering the body so we want to block consuming code from reading again - // and getting weird errors. Treat further reads as a fully consumed body. - httpContext.Request.Body = Stream.Null; - - // User code overrode the PipeReader - if (reader == httpContext.Request.BodyReader) - { - requestBodyPipe = httpContext.Features.Get(); - httpContext.Features.Set(new RequestBodyPipeFeature(PipeReader.Create(Stream.Null))); - } - - await continuation(target, httpContext, body); - - reader.AdvanceTo(body.End); - } - finally - { - httpContext.Request.Body = stream; - - if (requestBodyPipe is not null) - { - httpContext.Features.Set(requestBodyPipe); - } - } - }; - } - - static async ValueTask> ReadBodyAsync(HttpContext httpContext, PipeReader reader) - { - var feature = httpContext.Features.Get(); - - if (feature?.CanHaveBody == true) - { - while (true) - { - var result = await reader.ReadAsync(); - var buffer = result.Buffer; - - if (result.IsCompleted) - { - return buffer; - } - - // Buffer the body - reader.AdvanceTo(buffer.Start, buffer.End); - } - } - - return default; - } - } - private static Func HandleRequestBodyAndCompileRequestDelegateForJsonBody(Expression responseWritingMethodCall, FactoryContext factoryContext) { Debug.Assert(factoryContext.RequestBodyParameter is not null, "factoryContext.RequestBodyParameter is null for a body parameter."); @@ -1338,8 +1202,6 @@ 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)), @@ -1364,8 +1226,6 @@ private static Expression BindParameterFromBody(ParameterInfo parameter, bool al // } var checkRequiredBodyBlock = Expression.Block( Expression.IfThen( - factoryContext.IsRawRequestBody ? - RawBodyIsEmptyExpr : Expression.Equal(BodyValueExpr, Expression.Constant(null)), Expression.Block( Expression.Assign(WasParamCheckFailureExpr, Expression.Constant(true)), @@ -1384,22 +1244,12 @@ 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); } @@ -1620,7 +1470,7 @@ private class FactoryContext // Temporary State public ParameterInfo? RequestBodyParameter { get; set; } public bool AllowEmptyRequestBody { get; set; } - public bool IsRawRequestBody => RequestBodyParameter?.ParameterType == typeof(ReadOnlySequence); + public bool IsRawRequestBody => RequestBodyParameter?.ParameterType == typeof(ReadOnlySpan); public bool UsingTempSourceString { get; set; } public List ExtraLocals { get; } = new(); diff --git a/src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs b/src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs index c57f5c41720f..7cb2f87805f1 100644 --- a/src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs +++ b/src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs @@ -1381,16 +1381,6 @@ public static object[][] RawFromBodyActions { get { - void ExplicitTestReadOnlySequence(HttpContext httpContext, [FromBody] ReadOnlySequence body) - { - httpContext.Items.Add("body", body.ToArray()); - } - - void TestReadOnlySequence(HttpContext httpContext, ReadOnlySequence body) - { - httpContext.Items.Add("body", body.ToArray()); - } - void TestStream(HttpContext httpContext, Stream stream) { var ms = new MemoryStream(); @@ -1405,29 +1395,10 @@ async Task TestPipeReader(HttpContext httpContext, PipeReader reader) httpContext.Items.Add("body", ms.ToArray()); } - async Task TestPipeReaderAndROS(HttpContext httpContext, ReadOnlySequence body, PipeReader reader) - { - var result = await reader.ReadAsync(); - httpContext.Items.Add("body", body.ToArray()); - httpContext.Items.Add("bodyIsEmpty", result.IsCompleted); - reader.AdvanceTo(result.Buffer.End); - } - - async Task TestStreamAndROS(HttpContext httpContext, ReadOnlySequence body, Stream stream) - { - int read = await stream.ReadAsync(new byte[1].AsMemory()); - httpContext.Items.Add("body", body.ToArray()); - httpContext.Items.Add("bodyIsEmpty", read == 0); - } - return new[] { - new object[] { (Action>)ExplicitTestReadOnlySequence }, - new object[] { (Action>)TestReadOnlySequence }, new object[] { (Action)TestStream }, - new object[] { (Func)TestPipeReader }, - new object[] { (Func, PipeReader, Task>)TestPipeReaderAndROS }, - new object[] { (Func, Stream, Task>)TestStreamAndROS }, + new object[] { (Func)TestPipeReader } }; } } @@ -1600,51 +1571,6 @@ void TestAction([FromBody(AllowEmpty = true)] Todo todo) Assert.Null(todoToBecomeNull); } - [Fact] - public async Task RequestDelegateAllowsEmptyRawBodyGivenCorrectyConfiguredFromBodyParameter() - { - ReadOnlySequence bodyResult = new(new byte[] { 1, 2, 3, 4 }); - - void TestAction([FromBody(AllowEmpty = true)] ReadOnlySequence body) - { - bodyResult = body; - } - - var httpContext = CreateHttpContext(); - httpContext.Request.Headers["Content-Type"] = "application/json"; - httpContext.Request.Headers["Content-Length"] = "0"; - - var factoryResult = RequestDelegateFactory.Create(TestAction); - var requestDelegate = factoryResult.RequestDelegate; - - await requestDelegate(httpContext); - - Assert.True(bodyResult.IsEmpty); - } - - [Fact] - public async Task RequestDelegateRawBodyNotInvokedIfBodyIsEmptyAndNotAllowed() - { - ReadOnlySequence bodyResult = new(new byte[] { 1, 2, 3, 4 }); - - void TestAction(ReadOnlySequence body) - { - bodyResult = body; - } - - var httpContext = CreateHttpContext(); - httpContext.Request.Headers["Content-Type"] = "application/json"; - httpContext.Request.Headers["Content-Length"] = "0"; - httpContext.Features.Set(new RequestBodyDetectionFeature(true)); - - var factoryResult = RequestDelegateFactory.Create(TestAction); - var requestDelegate = factoryResult.RequestDelegate; - - await requestDelegate(httpContext); - - Assert.False(bodyResult.IsEmpty); - } - [Fact] public async Task RequestDelegateAllowsEmptyBodyStructGivenCorrectyConfiguredFromBodyParameter() { From f5c305a6dd51571a2e6664a78f92a129daf63e15 Mon Sep 17 00:00:00 2001 From: David Fowler Date: Wed, 19 Jan 2022 20:00:30 -0800 Subject: [PATCH 19/24] Small reverts --- .../src/RequestDelegateFactory.cs | 36 ++++++------------- 1 file changed, 11 insertions(+), 25 deletions(-) diff --git a/src/Http/Http.Extensions/src/RequestDelegateFactory.cs b/src/Http/Http.Extensions/src/RequestDelegateFactory.cs index ae46e7ecf424..b846422a8ad5 100644 --- a/src/Http/Http.Extensions/src/RequestDelegateFactory.cs +++ b/src/Http/Http.Extensions/src/RequestDelegateFactory.cs @@ -202,7 +202,7 @@ private static Expression[] CreateArguments(ParameterInfo[]? parameters, Factory var errorMessage = BuildErrorMessageForInferredBodyParameter(factoryContext); throw new InvalidOperationException(errorMessage); } - if (factoryContext.RequestBodyParameter is not null && + if (factoryContext.JsonRequestBodyParameter is not null && factoryContext.FirstFormRequestBodyParameter is not null) { var errorMessage = BuildErrorMessageForFormAndBodyParameters(factoryContext); @@ -560,7 +560,7 @@ private static Expression AddResponseWritingToMethodCall(Expression methodCall, private static Func HandleRequestBodyAndCompileRequestDelegate(Expression responseWritingMethodCall, FactoryContext factoryContext) { - if (factoryContext.RequestBodyParameter is null && !factoryContext.ReadForm) + if (factoryContext.JsonRequestBodyParameter is null && !factoryContext.ReadForm) { if (factoryContext.ParameterBinders.Count > 0) { @@ -599,11 +599,11 @@ private static Expression AddResponseWritingToMethodCall(Expression methodCall, private static Func HandleRequestBodyAndCompileRequestDelegateForJsonBody(Expression responseWritingMethodCall, FactoryContext factoryContext) { - Debug.Assert(factoryContext.RequestBodyParameter is not null, "factoryContext.RequestBodyParameter is null for a body parameter."); + Debug.Assert(factoryContext.JsonRequestBodyParameter is not null, "factoryContext.RequestBodyParameter is null for a body parameter."); - var bodyType = factoryContext.RequestBodyParameter.ParameterType; - var parameterTypeName = TypeNameHelper.GetTypeDisplayName(factoryContext.RequestBodyParameter.ParameterType, fullName: false); - var parameterName = factoryContext.RequestBodyParameter.Name; + var bodyType = factoryContext.JsonRequestBodyParameter.ParameterType; + var parameterTypeName = TypeNameHelper.GetTypeDisplayName(factoryContext.JsonRequestBodyParameter.ParameterType, fullName: false); + var parameterName = factoryContext.JsonRequestBodyParameter.Name; Debug.Assert(parameterName is not null, "CreateArgument() should throw if parameter.Name is null."); @@ -1166,7 +1166,7 @@ private static Expression BindParameterFromFormFile( private static Expression BindParameterFromBody(ParameterInfo parameter, bool allowEmpty, FactoryContext factoryContext) { - if (factoryContext.RequestBodyParameter is not null) + if (factoryContext.JsonRequestBodyParameter is not null) { factoryContext.HasMultipleBodyParameters = true; var parameterName = parameter.Name; @@ -1182,14 +1182,10 @@ private static Expression BindParameterFromBody(ParameterInfo parameter, bool al var isOptional = IsOptionalParameter(parameter, factoryContext); - factoryContext.RequestBodyParameter = parameter; + factoryContext.JsonRequestBodyParameter = parameter; factoryContext.AllowEmptyRequestBody = allowEmpty || isOptional; - // We can't infer the accept content type for raw bodies - if (!factoryContext.IsRawRequestBody) - { - factoryContext.Metadata.Add(new AcceptsMetadata(parameter.ParameterType, factoryContext.AllowEmptyRequestBody, DefaultAcceptsContentType)); - } + factoryContext.Metadata.Add(new AcceptsMetadata(parameter.ParameterType, factoryContext.AllowEmptyRequestBody, DefaultAcceptsContentType)); if (!factoryContext.AllowEmptyRequestBody) { @@ -1450,15 +1446,6 @@ private static async Task ExecuteResultWriteResponse(IResult? result, HttpContex await EnsureRequestResultNotNull(result).ExecuteAsync(httpContext); } - private sealed class RequestBodyPipeFeature : IRequestBodyPipeFeature - { - public RequestBodyPipeFeature(PipeReader pipeReader) - { - Reader = pipeReader; - } - public PipeReader Reader { get; set; } - } - private class FactoryContext { // Options @@ -1468,9 +1455,8 @@ private class FactoryContext public bool DisableInferredFromBody { get; init; } // Temporary State - public ParameterInfo? RequestBodyParameter { get; set; } + public ParameterInfo? JsonRequestBodyParameter { get; set; } public bool AllowEmptyRequestBody { get; set; } - public bool IsRawRequestBody => RequestBodyParameter?.ParameterType == typeof(ReadOnlySpan); public bool UsingTempSourceString { get; set; } public List ExtraLocals { get; } = new(); @@ -1709,7 +1695,7 @@ private static string BuildErrorMessageForInferredBodyParameter(FactoryContext f private static string BuildErrorMessageForFormAndBodyParameters(FactoryContext factoryContext) { var errorMessage = new StringBuilder(); - errorMessage.AppendLine("An action cannot use both form and body parameters."); + errorMessage.AppendLine("An action cannot use both form and JSON body parameters."); errorMessage.AppendLine("Below is the list of parameters that we found: "); errorMessage.AppendLine(); errorMessage.AppendLine(FormattableString.Invariant($"{"Parameter",-20}| {"Source",-30}")); From 5b03d5433ee34b63defdeab2c4a3a803fe060e26 Mon Sep 17 00:00:00 2001 From: David Fowler Date: Wed, 19 Jan 2022 20:03:29 -0800 Subject: [PATCH 20/24] Fixed more things --- src/Http/Http.Extensions/src/RequestDelegateFactory.cs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/Http/Http.Extensions/src/RequestDelegateFactory.cs b/src/Http/Http.Extensions/src/RequestDelegateFactory.cs index b846422a8ad5..2a6eb1fe84e4 100644 --- a/src/Http/Http.Extensions/src/RequestDelegateFactory.cs +++ b/src/Http/Http.Extensions/src/RequestDelegateFactory.cs @@ -205,7 +205,7 @@ private static Expression[] CreateArguments(ParameterInfo[]? parameters, Factory if (factoryContext.JsonRequestBodyParameter is not null && factoryContext.FirstFormRequestBodyParameter is not null) { - var errorMessage = BuildErrorMessageForFormAndBodyParameters(factoryContext); + var errorMessage = BuildErrorMessageForFormAndJsonBodyParameters(factoryContext); throw new InvalidOperationException(errorMessage); } if (factoryContext.HasMultipleBodyParameters) @@ -594,10 +594,10 @@ private static Expression AddResponseWritingToMethodCall(Expression methodCall, return HandleRequestBodyAndCompileRequestDelegateForForm(responseWritingMethodCall, factoryContext); } - return HandleRequestBodyAndCompileRequestDelegateForJsonBody(responseWritingMethodCall, factoryContext); + return HandleRequestBodyAndCompileRequestDelegateForJson(responseWritingMethodCall, factoryContext); } - private static Func HandleRequestBodyAndCompileRequestDelegateForJsonBody(Expression responseWritingMethodCall, FactoryContext factoryContext) + private static Func HandleRequestBodyAndCompileRequestDelegateForJson(Expression responseWritingMethodCall, FactoryContext factoryContext) { Debug.Assert(factoryContext.JsonRequestBodyParameter is not null, "factoryContext.RequestBodyParameter is null for a body parameter."); @@ -1692,7 +1692,7 @@ private static string BuildErrorMessageForInferredBodyParameter(FactoryContext f return errorMessage.ToString(); } - private static string BuildErrorMessageForFormAndBodyParameters(FactoryContext factoryContext) + private static string BuildErrorMessageForFormAndJsonBodyParameters(FactoryContext factoryContext) { var errorMessage = new StringBuilder(); errorMessage.AppendLine("An action cannot use both form and JSON body parameters."); From c1f40630f3ffe49356a4d7329a2a501b55e08dc5 Mon Sep 17 00:00:00 2001 From: David Fowler Date: Wed, 19 Jan 2022 20:07:47 -0800 Subject: [PATCH 21/24] Moar --- src/Http/Http.Extensions/src/RequestDelegateFactory.cs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/Http/Http.Extensions/src/RequestDelegateFactory.cs b/src/Http/Http.Extensions/src/RequestDelegateFactory.cs index 2a6eb1fe84e4..60bc794ea856 100644 --- a/src/Http/Http.Extensions/src/RequestDelegateFactory.cs +++ b/src/Http/Http.Extensions/src/RequestDelegateFactory.cs @@ -599,7 +599,7 @@ private static Expression AddResponseWritingToMethodCall(Expression methodCall, private static Func HandleRequestBodyAndCompileRequestDelegateForJson(Expression responseWritingMethodCall, FactoryContext factoryContext) { - Debug.Assert(factoryContext.JsonRequestBodyParameter is not null, "factoryContext.RequestBodyParameter is null for a body parameter."); + Debug.Assert(factoryContext.JsonRequestBodyParameter is not null, "factoryContext.JsonRequestBodyParameter is null for a JSON body."); var bodyType = factoryContext.JsonRequestBodyParameter.ParameterType; var parameterTypeName = TypeNameHelper.GetTypeDisplayName(factoryContext.JsonRequestBodyParameter.ParameterType, fullName: false); @@ -1184,7 +1184,6 @@ private static Expression BindParameterFromBody(ParameterInfo parameter, bool al factoryContext.JsonRequestBodyParameter = parameter; factoryContext.AllowEmptyRequestBody = allowEmpty || isOptional; - factoryContext.Metadata.Add(new AcceptsMetadata(parameter.ParameterType, factoryContext.AllowEmptyRequestBody, DefaultAcceptsContentType)); if (!factoryContext.AllowEmptyRequestBody) From 1cd97354c97ffb5b0ea1d3e847a9d4de5be8bcf6 Mon Sep 17 00:00:00 2001 From: David Fowler Date: Wed, 19 Jan 2022 20:09:39 -0800 Subject: [PATCH 22/24] Reverted more stuff --- src/Http/Http.Extensions/src/RequestDelegateFactory.cs | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/Http/Http.Extensions/src/RequestDelegateFactory.cs b/src/Http/Http.Extensions/src/RequestDelegateFactory.cs index 60bc794ea856..46fc76a6009b 100644 --- a/src/Http/Http.Extensions/src/RequestDelegateFactory.cs +++ b/src/Http/Http.Extensions/src/RequestDelegateFactory.cs @@ -593,8 +593,10 @@ private static Expression AddResponseWritingToMethodCall(Expression methodCall, { return HandleRequestBodyAndCompileRequestDelegateForForm(responseWritingMethodCall, factoryContext); } - - return HandleRequestBodyAndCompileRequestDelegateForJson(responseWritingMethodCall, factoryContext); + else + { + return HandleRequestBodyAndCompileRequestDelegateForJson(responseWritingMethodCall, factoryContext); + } } private static Func HandleRequestBodyAndCompileRequestDelegateForJson(Expression responseWritingMethodCall, FactoryContext factoryContext) From 27c3f48b1977d58489506cd49c04293db98e1096 Mon Sep 17 00:00:00 2001 From: David Fowler Date: Wed, 19 Jan 2022 20:10:30 -0800 Subject: [PATCH 23/24] One more I hope --- src/Http/Http.Extensions/src/RequestDelegateFactory.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Http/Http.Extensions/src/RequestDelegateFactory.cs b/src/Http/Http.Extensions/src/RequestDelegateFactory.cs index 46fc76a6009b..cde00d5be754 100644 --- a/src/Http/Http.Extensions/src/RequestDelegateFactory.cs +++ b/src/Http/Http.Extensions/src/RequestDelegateFactory.cs @@ -1223,7 +1223,7 @@ private static Expression BindParameterFromBody(ParameterInfo parameter, bool al // } var checkRequiredBodyBlock = Expression.Block( Expression.IfThen( - Expression.Equal(BodyValueExpr, Expression.Constant(null)), + Expression.Equal(BodyValueExpr, Expression.Constant(null)), Expression.Block( Expression.Assign(WasParamCheckFailureExpr, Expression.Constant(true)), Expression.Call(LogRequiredParameterNotProvidedMethod, From 9a7f5934048113264f116574e01573d09605899f Mon Sep 17 00:00:00 2001 From: David Fowler Date: Wed, 19 Jan 2022 20:12:10 -0800 Subject: [PATCH 24/24] Fixed the test --- .../Http.Extensions/test/RequestDelegateFactoryTests.cs | 8 -------- 1 file changed, 8 deletions(-) diff --git a/src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs b/src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs index 7cb2f87805f1..ef22b01c11a0 100644 --- a/src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs +++ b/src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs @@ -1447,10 +1447,6 @@ public async Task RequestDelegatePopulatesFromRawBodyParameter(Delegate action) var rawRequestBody = httpContext.Items["body"]; Assert.NotNull(rawRequestBody); Assert.Equal(requestBodyBytes, (byte[])rawRequestBody!); - if (httpContext.Items.TryGetValue("bodyIsEmpty", out var bodyIsEmpty)) - { - Assert.True((bool)bodyIsEmpty!); - } } [Theory] @@ -1496,10 +1492,6 @@ public async Task RequestDelegatePopulatesFromRawBodyParameterPipeReader(Delegat var rawRequestBody = httpContext.Items["body"]; Assert.NotNull(rawRequestBody); Assert.Equal(requestBodyBytes, (byte[])rawRequestBody!); - if (httpContext.Items.TryGetValue("bodyIsEmpty", out var bodyIsEmpty)) - { - Assert.True((bool)bodyIsEmpty!); - } } class PipeRequestBodyFeature : IRequestBodyPipeFeature