From 9b1f55e94221007ed4a88b3f52e500b0180cf181 Mon Sep 17 00:00:00 2001 From: Stephen Halter Date: Fri, 2 Apr 2021 13:01:56 -0700 Subject: [PATCH 01/35] Move and update MapActionSample --- AspNetCore.sln | 33 ++++++------- src/Http/HttpAbstractions.slnf | 14 +++--- .../samples/MapActionSample/Program.cs | 26 ---------- .../samples/MapActionSample/Startup.cs | 48 ------------------- .../Routing/samples/MapActionSample/Todo.cs | 9 ---- .../MapActionSample/MapActionSample.csproj | 0 src/Http/samples/MapActionSample/Program.cs | 33 +++++++++++++ .../Properties/launchSettings.json | 0 .../appsettings.Development.json | 0 .../samples/MapActionSample/appsettings.json | 0 10 files changed, 54 insertions(+), 109 deletions(-) delete mode 100644 src/Http/Routing/samples/MapActionSample/Program.cs delete mode 100644 src/Http/Routing/samples/MapActionSample/Startup.cs delete mode 100644 src/Http/Routing/samples/MapActionSample/Todo.cs rename src/Http/{Routing => }/samples/MapActionSample/MapActionSample.csproj (100%) create mode 100644 src/Http/samples/MapActionSample/Program.cs rename src/Http/{Routing => }/samples/MapActionSample/Properties/launchSettings.json (100%) rename src/Http/{Routing => }/samples/MapActionSample/appsettings.Development.json (100%) rename src/Http/{Routing => }/samples/MapActionSample/appsettings.json (100%) diff --git a/AspNetCore.sln b/AspNetCore.sln index 4f798d3ca2d3..168a880c6d1e 100644 --- a/AspNetCore.sln +++ b/AspNetCore.sln @@ -1572,12 +1572,8 @@ Project("{2150E333-8FDC-42A3-9474-1A3956D46DE8}") = "BrowserTesting", "BrowserTe EndProject Project("{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}") = "Microsoft.AspNetCore.BrowserTesting", "src\Shared\BrowserTesting\src\Microsoft.AspNetCore.BrowserTesting.csproj", "{B739074E-6652-4F5B-B37E-775DC2245FEC}" EndProject -Project("{2150E333-8FDC-42A3-9474-1A3956D46DE8}") = "samples", "samples", "{722E5A66-D84A-4689-AA87-7197FF5D7070}" -EndProject Project("{9A19103F-16F7-4668-BE54-9A1E7A4F7556}") = "WasmLinkerTest", "src\Components\WebAssembly\testassets\WasmLinkerTest\WasmLinkerTest.csproj", "{3B375FFC-1E38-453E-A26D-A510CCD3339E}" EndProject -Project("{9A19103F-16F7-4668-BE54-9A1E7A4F7556}") = "MapActionSample", "src\Http\Routing\samples\MapActionSample\MapActionSample.csproj", "{8F510BAA-FA6B-4648-8F98-28DF5C69DBB2}" -EndProject Project("{2150E333-8FDC-42A3-9474-1A3956D46DE8}") = "Samples", "Samples", "{71287382-95EF-490D-A285-87196E29E88A}" EndProject Project("{2150E333-8FDC-42A3-9474-1A3956D46DE8}") = "HostedBlazorWebassemblyApp", "HostedBlazorWebassemblyApp", "{B4226BE2-DCB7-40C5-93F2-94C9BD6F4394}" @@ -1618,6 +1614,8 @@ Project("{9A19103F-16F7-4668-BE54-9A1E7A4F7556}") = "BlazorWinFormsApp", "src\Co EndProject Project("{9A19103F-16F7-4668-BE54-9A1E7A4F7556}") = "Microsoft.AspNetCore.Http.Abstractions.Microbenchmarks", "src\Http\Http.Abstractions\perf\Microbenchmarks\Microsoft.AspNetCore.Http.Abstractions.Microbenchmarks.csproj", "{3F752B48-2936-4FCA-B0DC-4AB0F788F897}" EndProject +Project("{9A19103F-16F7-4668-BE54-9A1E7A4F7556}") = "MapActionSample", "src\Http\samples\MapActionSample\MapActionSample.csproj", "{A661D867-708A-494E-8B6B-6558804F9A3F}" +EndProject Project("{2150E333-8FDC-42A3-9474-1A3956D46DE8}") = "testassets", "testassets", "{F0849E7E-61DB-4849-9368-9E7BC125DCB0}" EndProject Project("{9A19103F-16F7-4668-BE54-9A1E7A4F7556}") = "WinFormsTestApp", "src\Components\WebView\Platforms\WindowsForms\testassets\WinFormsTestApp\WinFormsTestApp.csproj", "{99EE7769-3C81-477B-B947-0A5CBCD5B27D}" @@ -7521,18 +7519,6 @@ Global {3B375FFC-1E38-453E-A26D-A510CCD3339E}.Release|x64.Build.0 = Release|Any CPU {3B375FFC-1E38-453E-A26D-A510CCD3339E}.Release|x86.ActiveCfg = Release|Any CPU {3B375FFC-1E38-453E-A26D-A510CCD3339E}.Release|x86.Build.0 = Release|Any CPU - {8F510BAA-FA6B-4648-8F98-28DF5C69DBB2}.Debug|Any CPU.ActiveCfg = Debug|Any CPU - {8F510BAA-FA6B-4648-8F98-28DF5C69DBB2}.Debug|Any CPU.Build.0 = Debug|Any CPU - {8F510BAA-FA6B-4648-8F98-28DF5C69DBB2}.Debug|x64.ActiveCfg = Debug|Any CPU - {8F510BAA-FA6B-4648-8F98-28DF5C69DBB2}.Debug|x64.Build.0 = Debug|Any CPU - {8F510BAA-FA6B-4648-8F98-28DF5C69DBB2}.Debug|x86.ActiveCfg = Debug|Any CPU - {8F510BAA-FA6B-4648-8F98-28DF5C69DBB2}.Debug|x86.Build.0 = Debug|Any CPU - {8F510BAA-FA6B-4648-8F98-28DF5C69DBB2}.Release|Any CPU.ActiveCfg = Release|Any CPU - {8F510BAA-FA6B-4648-8F98-28DF5C69DBB2}.Release|Any CPU.Build.0 = Release|Any CPU - {8F510BAA-FA6B-4648-8F98-28DF5C69DBB2}.Release|x64.ActiveCfg = Release|Any CPU - {8F510BAA-FA6B-4648-8F98-28DF5C69DBB2}.Release|x64.Build.0 = Release|Any CPU - {8F510BAA-FA6B-4648-8F98-28DF5C69DBB2}.Release|x86.ActiveCfg = Release|Any CPU - {8F510BAA-FA6B-4648-8F98-28DF5C69DBB2}.Release|x86.Build.0 = Release|Any CPU {8F6F73F7-0DDA-4AA3-9887-2FB0141786AC}.Debug|Any CPU.ActiveCfg = Debug|Any CPU {8F6F73F7-0DDA-4AA3-9887-2FB0141786AC}.Debug|Any CPU.Build.0 = Debug|Any CPU {8F6F73F7-0DDA-4AA3-9887-2FB0141786AC}.Debug|x64.ActiveCfg = Debug|Any CPU @@ -7665,6 +7651,18 @@ Global {3F752B48-2936-4FCA-B0DC-4AB0F788F897}.Release|x64.Build.0 = Release|Any CPU {3F752B48-2936-4FCA-B0DC-4AB0F788F897}.Release|x86.ActiveCfg = Release|Any CPU {3F752B48-2936-4FCA-B0DC-4AB0F788F897}.Release|x86.Build.0 = Release|Any CPU + {A661D867-708A-494E-8B6B-6558804F9A3F}.Debug|Any CPU.ActiveCfg = Debug|Any CPU + {A661D867-708A-494E-8B6B-6558804F9A3F}.Debug|Any CPU.Build.0 = Debug|Any CPU + {A661D867-708A-494E-8B6B-6558804F9A3F}.Debug|x64.ActiveCfg = Debug|Any CPU + {A661D867-708A-494E-8B6B-6558804F9A3F}.Debug|x64.Build.0 = Debug|Any CPU + {A661D867-708A-494E-8B6B-6558804F9A3F}.Debug|x86.ActiveCfg = Debug|Any CPU + {A661D867-708A-494E-8B6B-6558804F9A3F}.Debug|x86.Build.0 = Debug|Any CPU + {A661D867-708A-494E-8B6B-6558804F9A3F}.Release|Any CPU.ActiveCfg = Release|Any CPU + {A661D867-708A-494E-8B6B-6558804F9A3F}.Release|Any CPU.Build.0 = Release|Any CPU + {A661D867-708A-494E-8B6B-6558804F9A3F}.Release|x64.ActiveCfg = Release|Any CPU + {A661D867-708A-494E-8B6B-6558804F9A3F}.Release|x64.Build.0 = Release|Any CPU + {A661D867-708A-494E-8B6B-6558804F9A3F}.Release|x86.ActiveCfg = Release|Any CPU + {A661D867-708A-494E-8B6B-6558804F9A3F}.Release|x86.Build.0 = Release|Any CPU {99EE7769-3C81-477B-B947-0A5CBCD5B27D}.Debug|Any CPU.ActiveCfg = Debug|Any CPU {99EE7769-3C81-477B-B947-0A5CBCD5B27D}.Debug|Any CPU.Build.0 = Debug|Any CPU {99EE7769-3C81-477B-B947-0A5CBCD5B27D}.Debug|x64.ActiveCfg = Debug|Any CPU @@ -8489,9 +8487,7 @@ Global {22EA0993-8DFC-40C2-8481-8E85E21EFB56} = {41BB7BA4-AC08-4E9A-83EA-6D587A5B951C} {8F33439F-5532-45D6-8A44-20EF9104AA9D} = {5F0044F2-4C66-46A8-BD79-075F001AA034} {B739074E-6652-4F5B-B37E-775DC2245FEC} = {8F33439F-5532-45D6-8A44-20EF9104AA9D} - {722E5A66-D84A-4689-AA87-7197FF5D7070} = {54C42F57-5447-4C21-9812-4AF665567566} {3B375FFC-1E38-453E-A26D-A510CCD3339E} = {7D2B0799-A634-42AC-AE77-5D167BA51389} - {8F510BAA-FA6B-4648-8F98-28DF5C69DBB2} = {722E5A66-D84A-4689-AA87-7197FF5D7070} {71287382-95EF-490D-A285-87196E29E88A} = {562D5067-8CD8-4F19-BCBB-873204932C61} {B4226BE2-DCB7-40C5-93F2-94C9BD6F4394} = {71287382-95EF-490D-A285-87196E29E88A} {8F6F73F7-0DDA-4AA3-9887-2FB0141786AC} = {B4226BE2-DCB7-40C5-93F2-94C9BD6F4394} @@ -8512,6 +8508,7 @@ Global {3BA297F8-1CA1-492D-AE64-A60B825D8501} = {D4E9A2C5-0838-42DF-BC80-C829C4C9137E} {CC740832-D268-47A3-9058-B9054F8397E2} = {D3B76F4E-A980-45BF-AEA1-EA3175B0B5A1} {3F752B48-2936-4FCA-B0DC-4AB0F788F897} = {DCBBDB52-4A49-4141-8F4D-81C0FFFB7BD5} + {A661D867-708A-494E-8B6B-6558804F9A3F} = {EB5E294B-9ED5-43BF-AFA9-1CD2327F3DC1} {F0849E7E-61DB-4849-9368-9E7BC125DCB0} = {D4E9A2C5-0838-42DF-BC80-C829C4C9137E} {99EE7769-3C81-477B-B947-0A5CBCD5B27D} = {F0849E7E-61DB-4849-9368-9E7BC125DCB0} {94D0D6F3-8632-41DE-908B-47A787D570FF} = {5241CF68-66A0-4724-9BAA-36DB959A5B11} diff --git a/src/Http/HttpAbstractions.slnf b/src/Http/HttpAbstractions.slnf index 58b8f6f80b9d..6fad000bf864 100644 --- a/src/Http/HttpAbstractions.slnf +++ b/src/Http/HttpAbstractions.slnf @@ -1,4 +1,4 @@ -{ +{ "solution": { "path": "..\\..\\AspNetCore.sln", "projects": [ @@ -11,13 +11,14 @@ "src\\Http\\Authentication.Core\\test\\Microsoft.AspNetCore.Authentication.Core.Test.csproj", "src\\Http\\Headers\\src\\Microsoft.Net.Http.Headers.csproj", "src\\Http\\Headers\\test\\Microsoft.Net.Http.Headers.Tests.csproj", + "src\\Http\\Http.Abstractions\\perf\\Microbenchmarks\\Microsoft.AspNetCore.Http.Abstractions.Microbenchmarks.csproj", "src\\Http\\Http.Abstractions\\src\\Microsoft.AspNetCore.Http.Abstractions.csproj", "src\\Http\\Http.Abstractions\\test\\Microsoft.AspNetCore.Http.Abstractions.Tests.csproj", "src\\Http\\Http.Extensions\\src\\Microsoft.AspNetCore.Http.Extensions.csproj", "src\\Http\\Http.Extensions\\test\\Microsoft.AspNetCore.Http.Extensions.Tests.csproj", "src\\Http\\Http.Features\\src\\Microsoft.AspNetCore.Http.Features.csproj", "src\\Http\\Http.Features\\test\\Microsoft.AspNetCore.Http.Features.Tests.csproj", - "src\\Http\\Http\\perf\\Microsoft.AspNetCore.Http.Performance.csproj", + "src\\Http\\Http\\perf\\Microbenchmarks\\Microsoft.AspNetCore.Http.Microbenchmarks.csproj", "src\\Http\\Http\\src\\Microsoft.AspNetCore.Http.csproj", "src\\Http\\Http\\test\\Microsoft.AspNetCore.Http.Tests.csproj", "src\\Http\\Metadata\\src\\Microsoft.AspNetCore.Metadata.csproj", @@ -25,19 +26,17 @@ "src\\Http\\Owin\\test\\Microsoft.AspNetCore.Owin.Tests.csproj", "src\\Http\\Routing.Abstractions\\src\\Microsoft.AspNetCore.Routing.Abstractions.csproj", "src\\Http\\Routing.Abstractions\\test\\Microsoft.AspNetCore.Mvc.Routing.Abstractions.Tests.csproj", - "src\\Http\\Http\\perf\\Microbenchmarks\\Microsoft.AspNetCore.Http.Microbenchmarks.csproj", - "src\\Http\\Http.Abstractions\\perf\\Microbenchmarks\\Microsoft.AspNetCore.Http.Abstractions.Microbenchmarks.csproj", "src\\Http\\Routing\\perf\\Microbenchmarks\\Microsoft.AspNetCore.Routing.Microbenchmarks.csproj", - "src\\Http\\Routing\\samples\\MapActionSample\\MapActionSample.csproj", "src\\Http\\Routing\\src\\Microsoft.AspNetCore.Routing.csproj", "src\\Http\\Routing\\test\\FunctionalTests\\Microsoft.AspNetCore.Routing.FunctionalTests.csproj", "src\\Http\\Routing\\test\\UnitTests\\Microsoft.AspNetCore.Routing.Tests.csproj", "src\\Http\\Routing\\test\\testassets\\Benchmarks\\Benchmarks.csproj", "src\\Http\\Routing\\test\\testassets\\RoutingSandbox\\RoutingSandbox.csproj", "src\\Http\\Routing\\test\\testassets\\RoutingWebSite\\RoutingWebSite.csproj", - "src\\Servers\\Kestrel\\Kestrel\\src\\Microsoft.AspNetCore.Server.Kestrel.csproj", + "src\\Http\\WebUtilities\\perf\\Microbenchmarks\\Microsoft.AspNetCore.WebUtilities.Microbenchmarks.csproj", "src\\Http\\WebUtilities\\src\\Microsoft.AspNetCore.WebUtilities.csproj", "src\\Http\\WebUtilities\\test\\Microsoft.AspNetCore.WebUtilities.Tests.csproj", + "src\\Http\\samples\\MapActionSample\\MapActionSample.csproj", "src\\Http\\samples\\SampleApp\\HttpAbstractions.SampleApp.csproj", "src\\Middleware\\CORS\\src\\Microsoft.AspNetCore.Cors.csproj", "src\\Middleware\\HttpOverrides\\src\\Microsoft.AspNetCore.HttpOverrides.csproj", @@ -50,8 +49,7 @@ "src\\Servers\\Kestrel\\Kestrel\\src\\Microsoft.AspNetCore.Server.Kestrel.csproj", "src\\Servers\\Kestrel\\Transport.Sockets\\src\\Microsoft.AspNetCore.Server.Kestrel.Transport.Sockets.csproj", "src\\Testing\\src\\Microsoft.AspNetCore.Testing.csproj", - "src\\WebEncoders\\src\\Microsoft.Extensions.WebEncoders.csproj", - "src\\Http\\WebUtilities\\perf\\Microbenchmarks\\Microsoft.AspNetCore.WebUtilities.Microbenchmarks.csproj" + "src\\WebEncoders\\src\\Microsoft.Extensions.WebEncoders.csproj" ] } } \ No newline at end of file diff --git a/src/Http/Routing/samples/MapActionSample/Program.cs b/src/Http/Routing/samples/MapActionSample/Program.cs deleted file mode 100644 index 0e33ff8e1f6f..000000000000 --- a/src/Http/Routing/samples/MapActionSample/Program.cs +++ /dev/null @@ -1,26 +0,0 @@ -using System; -using System.Collections.Generic; -using System.Linq; -using System.Threading.Tasks; -using Microsoft.AspNetCore.Hosting; -using Microsoft.Extensions.Configuration; -using Microsoft.Extensions.Hosting; -using Microsoft.Extensions.Logging; - -namespace HttpApiSampleApp -{ - public class Program - { - public static void Main(string[] args) - { - CreateHostBuilder(args).Build().Run(); - } - - public static IHostBuilder CreateHostBuilder(string[] args) => - Host.CreateDefaultBuilder(args) - .ConfigureWebHostDefaults(webBuilder => - { - webBuilder.UseStartup(); - }); - } -} diff --git a/src/Http/Routing/samples/MapActionSample/Startup.cs b/src/Http/Routing/samples/MapActionSample/Startup.cs deleted file mode 100644 index e3c44398834b..000000000000 --- a/src/Http/Routing/samples/MapActionSample/Startup.cs +++ /dev/null @@ -1,48 +0,0 @@ -using System; -using Microsoft.AspNetCore.Builder; -using Microsoft.AspNetCore.Hosting; -using Microsoft.AspNetCore.Http; -using Microsoft.AspNetCore.Mvc; -using Microsoft.Extensions.DependencyInjection; -using Microsoft.Extensions.Hosting; - -namespace HttpApiSampleApp -{ - public class Startup - { - // This method gets called by the runtime. Use this method to add services to the container. - // For more information on how to configure your application, visit https://go.microsoft.com/fwlink/?LinkID=398940 - public void ConfigureServices(IServiceCollection services) - { - } - - // This method gets called by the runtime. Use this method to configure the HTTP request pipeline. - public void Configure(IApplicationBuilder app, IWebHostEnvironment env) - { - if (env.IsDevelopment()) - { - app.UseDeveloperExceptionPage(); - } - - app.UseRouting(); - - app.UseEndpoints(endpoints => - { - JsonResult EchoTodo([FromBody] Todo todo) => new(todo); - - endpoints.MapPost("/EchoTodo", (Func)EchoTodo); - - endpoints.MapPost("/EchoTodoProto", async httpContext => - { - var todo = await httpContext.Request.ReadFromJsonAsync(); - await httpContext.Response.WriteAsJsonAsync(todo); - }); - - endpoints.MapGet("/", async context => - { - await context.Response.WriteAsync("Hello World!"); - }); - }); - } - } -} diff --git a/src/Http/Routing/samples/MapActionSample/Todo.cs b/src/Http/Routing/samples/MapActionSample/Todo.cs deleted file mode 100644 index 2bcc698c8e5a..000000000000 --- a/src/Http/Routing/samples/MapActionSample/Todo.cs +++ /dev/null @@ -1,9 +0,0 @@ -namespace HttpApiSampleApp -{ - public class Todo - { - public int Id { get; set; } - public string Name { get; set; } - public bool IsComplete { get; set; } - } -} diff --git a/src/Http/Routing/samples/MapActionSample/MapActionSample.csproj b/src/Http/samples/MapActionSample/MapActionSample.csproj similarity index 100% rename from src/Http/Routing/samples/MapActionSample/MapActionSample.csproj rename to src/Http/samples/MapActionSample/MapActionSample.csproj diff --git a/src/Http/samples/MapActionSample/Program.cs b/src/Http/samples/MapActionSample/Program.cs new file mode 100644 index 000000000000..1df9c761fcb2 --- /dev/null +++ b/src/Http/samples/MapActionSample/Program.cs @@ -0,0 +1,33 @@ +using System; +using Microsoft.AspNetCore.Builder; +using Microsoft.AspNetCore.Hosting; +using Microsoft.AspNetCore.Mvc; +using Microsoft.Extensions.Hosting; +using Microsoft.Extensions.Logging; + +using var host = Host.CreateDefaultBuilder(args) + .ConfigureWebHostDefaults(webBuilder => + { + webBuilder.Configure(app => + { + app.UseRouting(); + + app.UseEndpoints(endpoints => + { + Todo EchoTodo([FromBody] Todo todo) => todo; + endpoints.MapPost("/EchoTodo", (Func)EchoTodo); + + string Plaintext() => "Hello, World!"; + endpoints.MapGet("/plaintext", (Func)Plaintext); + + object Json() => new { message = "Hello, World!" }; + endpoints.MapGet("/json", (Func)Json); + }); + + }); + }) + .Build(); + +await host.RunAsync(); + +record Todo(int Id, string Name, bool IsComplete); diff --git a/src/Http/Routing/samples/MapActionSample/Properties/launchSettings.json b/src/Http/samples/MapActionSample/Properties/launchSettings.json similarity index 100% rename from src/Http/Routing/samples/MapActionSample/Properties/launchSettings.json rename to src/Http/samples/MapActionSample/Properties/launchSettings.json diff --git a/src/Http/Routing/samples/MapActionSample/appsettings.Development.json b/src/Http/samples/MapActionSample/appsettings.Development.json similarity index 100% rename from src/Http/Routing/samples/MapActionSample/appsettings.Development.json rename to src/Http/samples/MapActionSample/appsettings.Development.json diff --git a/src/Http/Routing/samples/MapActionSample/appsettings.json b/src/Http/samples/MapActionSample/appsettings.json similarity index 100% rename from src/Http/Routing/samples/MapActionSample/appsettings.json rename to src/Http/samples/MapActionSample/appsettings.json From dfc2ae4975eea23bb82e5efcfa0f4ce72932eee4 Mon Sep 17 00:00:00 2001 From: Stephen Halter Date: Fri, 2 Apr 2021 14:37:54 -0700 Subject: [PATCH 02/35] Remove redundant test cases --- .../test/RequestDelegateFactoryTests.cs | 80 ++++--------------- 1 file changed, 14 insertions(+), 66 deletions(-) diff --git a/src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs b/src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs index 36d4004cfbe3..9372c92b5020 100644 --- a/src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs +++ b/src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs @@ -185,103 +185,51 @@ public void BuildRequestDelegateThrowsArgumentNullExceptions() Assert.Equal("targetFactory", exNullTargetFactory.ParamName); } - public static IEnumerable FromRouteResult - { - get - { - void TestAction(HttpContext httpContext, [FromRoute] int value) - { - StoreInput(httpContext, value); - }; - - Task TaskTestAction(HttpContext httpContext, [FromRoute] int value) - { - StoreInput(httpContext, value); - return Task.CompletedTask; - } - - ValueTask ValueTaskTestAction(HttpContext httpContext, [FromRoute] int value) - { - StoreInput(httpContext, value); - return ValueTask.CompletedTask; - } - - return new List - { - new object[] { (Action)TestAction }, - new object[] { (Func)TaskTestAction }, - new object[] { (Func)ValueTaskTestAction }, - }; - } - } private static void StoreInput(HttpContext httpContext, object value) { httpContext.Items.Add("input", value); } - [Theory] - [MemberData(nameof(FromRouteResult))] - public async Task RequestDelegatePopulatesFromRouteParameterBasedOnParameterName(Delegate @delegate) + [Fact] + public async Task RequestDelegatePopulatesFromRouteParameterBasedOnParameterName() { const string paramName = "value"; const int originalRouteParam = 42; + void TestAction(HttpContext httpContext, [FromRoute] int value) + { + StoreInput(httpContext, value); + } + var httpContext = new DefaultHttpContext(); httpContext.Request.RouteValues[paramName] = originalRouteParam.ToString(NumberFormatInfo.InvariantInfo); - var requestDelegate = RequestDelegateFactory.Create(@delegate); + var requestDelegate = RequestDelegateFactory.Create((Action)TestAction); await requestDelegate(httpContext); Assert.Equal(originalRouteParam, httpContext.Items["input"] as int?); } - public static IEnumerable FromRouteOptionalResult - { - get - { - return new List - { - new object[] { (Action)TestAction }, - new object[] { (Func)TaskTestAction }, - new object[] { (Func)ValueTaskTestAction } - }; - } - } - private static void TestAction(HttpContext httpContext, [FromRoute] int value = 42) { StoreInput(httpContext, value); } - private static Task TaskTestAction(HttpContext httpContext, [FromRoute] int value = 42) - { - StoreInput(httpContext, value); - return Task.CompletedTask; - } - - private static ValueTask ValueTaskTestAction(HttpContext httpContext, [FromRoute] int value = 42) - { - StoreInput(httpContext, value); - return ValueTask.CompletedTask; - } - - [Theory] - [MemberData(nameof(FromRouteOptionalResult))] - public async Task RequestDelegatePopulatesFromRouteOptionalParameter(Delegate @delegate) + [Fact] + public async Task RequestDelegatePopulatesFromRouteOptionalParameter() { var httpContext = new DefaultHttpContext(); - var requestDelegate = RequestDelegateFactory.Create(@delegate); + var requestDelegate = RequestDelegateFactory.Create((Action)TestAction); await requestDelegate(httpContext); Assert.Equal(42, httpContext.Items["input"] as int?); } - [Theory] - [MemberData(nameof(FromRouteOptionalResult))] - public async Task RequestDelegatePopulatesFromRouteOptionalParameterBasedOnParameterName(Delegate @delegate) + [Fact] + public async Task RequestDelegatePopulatesFromRouteOptionalParameterBasedOnParameterName() { const string paramName = "value"; const int originalRouteParam = 47; @@ -290,7 +238,7 @@ public async Task RequestDelegatePopulatesFromRouteOptionalParameterBasedOnParam httpContext.Request.RouteValues[paramName] = originalRouteParam.ToString(NumberFormatInfo.InvariantInfo); - var requestDelegate = RequestDelegateFactory.Create(@delegate); + var requestDelegate = RequestDelegateFactory.Create((Action)TestAction); await requestDelegate(httpContext); From e3f9632b9faa1bef616b8666794490560530992d Mon Sep 17 00:00:00 2001 From: Stephen Halter Date: Fri, 2 Apr 2021 14:59:38 -0700 Subject: [PATCH 03/35] TestImpliedFromService --- .../test/RequestDelegateFactoryTests.cs | 52 ++++++++++++------- 1 file changed, 33 insertions(+), 19 deletions(-) diff --git a/src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs b/src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs index 9372c92b5020..d368303c99b7 100644 --- a/src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs +++ b/src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs @@ -185,11 +185,6 @@ public void BuildRequestDelegateThrowsArgumentNullExceptions() Assert.Equal("targetFactory", exNullTargetFactory.ParamName); } - private static void StoreInput(HttpContext httpContext, object value) - { - httpContext.Items.Add("input", value); - } - [Fact] public async Task RequestDelegatePopulatesFromRouteParameterBasedOnParameterName() { @@ -198,7 +193,7 @@ public async Task RequestDelegatePopulatesFromRouteParameterBasedOnParameterName void TestAction(HttpContext httpContext, [FromRoute] int value) { - StoreInput(httpContext, value); + httpContext.Items.Add("input", value); } var httpContext = new DefaultHttpContext(); @@ -208,12 +203,12 @@ void TestAction(HttpContext httpContext, [FromRoute] int value) await requestDelegate(httpContext); - Assert.Equal(originalRouteParam, httpContext.Items["input"] as int?); + Assert.Equal(originalRouteParam, httpContext.Items["input"]); } private static void TestAction(HttpContext httpContext, [FromRoute] int value = 42) { - StoreInput(httpContext, value); + httpContext.Items.Add("input", value); } [Fact] @@ -225,7 +220,7 @@ public async Task RequestDelegatePopulatesFromRouteOptionalParameter() await requestDelegate(httpContext); - Assert.Equal(42, httpContext.Items["input"] as int?); + Assert.Equal(42, httpContext.Items["input"]); } [Fact] @@ -242,7 +237,7 @@ public async Task RequestDelegatePopulatesFromRouteOptionalParameterBasedOnParam await requestDelegate(httpContext); - Assert.Equal(47, httpContext.Items["input"] as int?); + Assert.Equal(47, httpContext.Items["input"]); } [Fact] @@ -291,6 +286,8 @@ void TestAction([FromRoute] int foo) Assert.Equal(0, deserializedRouteParam); } + // Insert new RouteValueTests. + [Fact] public async Task RequestDelegatePopulatesFromQueryParameterBasedOnParameterName() { @@ -624,16 +621,33 @@ void TestAction([FromBody] int value1, [FromBody] int value2) { } Assert.Throws(() => RequestDelegateFactory.Create((Action)TestAction)); } - [Fact] - public async Task RequestDelegatePopulatesFromServiceParameterBasedOnParameterType() + public static object[][] FromServiceParameter { - var myOriginalService = new MyService(); - MyService? injectedService = null; - - void TestAction([FromService] MyService myService) + get { - injectedService = myService; + void TestExplicitFromService(HttpContext httpContext, [FromService] MyService myService) + { + httpContext.Items.Add("service", myService); + } + + void TestImpliedFromService(HttpContext httpContext, MyService myService) + { + httpContext.Items.Add("service", myService); + } + + return new[] + { + new[] { (Action)TestExplicitFromService }, + new[] { (Action)TestImpliedFromService }, + }; } + } + + [Theory] + [MemberData(nameof(FromServiceParameter))] + public async Task RequestDelegatePopulatesFromServiceParameterBasedOnParameterType(Delegate @delegate) + { + var myOriginalService = new MyService(); var serviceCollection = new ServiceCollection(); serviceCollection.AddSingleton(myOriginalService); @@ -641,11 +655,11 @@ void TestAction([FromService] MyService myService) var httpContext = new DefaultHttpContext(); httpContext.RequestServices = serviceCollection.BuildServiceProvider(); - var requestDelegate = RequestDelegateFactory.Create((Action)TestAction); + var requestDelegate = RequestDelegateFactory.Create((Action)@delegate); await requestDelegate(httpContext); - Assert.Same(myOriginalService, injectedService); + Assert.Same(myOriginalService, httpContext.Items["service"]); } [Fact] From f156d54ddc91812c31419aa9b0e6911be59efa86 Mon Sep 17 00:00:00 2001 From: Stephen Halter Date: Mon, 5 Apr 2021 18:44:23 -0700 Subject: [PATCH 04/35] Add RequestDelegatePopulatesUnattributedTryParseableParametersFromRouteValue --- .../test/RequestDelegateFactoryTests.cs | 48 ++++++++++++++++++- 1 file changed, 47 insertions(+), 1 deletion(-) diff --git a/src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs b/src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs index d368303c99b7..c9239d82a644 100644 --- a/src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs +++ b/src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs @@ -286,7 +286,53 @@ void TestAction([FromRoute] int foo) Assert.Equal(0, deserializedRouteParam); } - // Insert new RouteValueTests. + public static object[][] FromTryParsableParameter + { + get + { + void StoreTryParsableParameter(HttpContext httpContext, T tryParsable) + { + httpContext.Items["tryParsable"] = tryParsable; + } + + return new[] + { + new object[] { (Action)StoreTryParsableParameter, "42", 42 }, + // Byte + // Int16 + // Int64 + // IntPtr + // Unsigned versions of above + // Char + // Single + // Double + // Half + // Enums + // DateTime + // DateTimeOffset + // TimeSpan + // Guid + // Version + // BigInteger + // IPAddress + // IPEndpoint + }; + } + } + + [Theory] + [MemberData(nameof(FromTryParsableParameter))] + public async Task RequestDelegatePopulatesUnattributedTryParseableParametersFromRouteValue(Delegate action, string routeValue, object expectedParameterValue) + { + var httpContext = new DefaultHttpContext(); + httpContext.Request.RouteValues["tryParsable"] = routeValue; + + var requestDelegate = RequestDelegateFactory.Create(action); + + await requestDelegate(httpContext); + + Assert.Equal(expectedParameterValue, httpContext.Items["tryParsable"]); + } [Fact] public async Task RequestDelegatePopulatesFromQueryParameterBasedOnParameterName() From 552b2ab0209aa0232a1690f077dc75c598838ecc Mon Sep 17 00:00:00 2001 From: Stephen Halter Date: Mon, 5 Apr 2021 19:02:29 -0700 Subject: [PATCH 05/35] Get one test passing --- .../src/RequestDelegateFactory.cs | 30 ++++++++++++++++++- 1 file changed, 29 insertions(+), 1 deletion(-) diff --git a/src/Http/Http.Extensions/src/RequestDelegateFactory.cs b/src/Http/Http.Extensions/src/RequestDelegateFactory.cs index 948c77fd094c..5616daa0e177 100644 --- a/src/Http/Http.Extensions/src/RequestDelegateFactory.cs +++ b/src/Http/Http.Extensions/src/RequestDelegateFactory.cs @@ -226,6 +226,11 @@ public static RequestDelegate Create(MethodInfo methodInfo, Func(); return loggerFactory.CreateLogger("Microsoft.AspNetCore.Routing.MapAction"); } - private static Expression BindParamenter(Expression sourceExpression, ParameterInfo parameter, string? name) + private static Expression BindParamenter(Expression sourceExpression, ParameterInfo parameter, string? name = null) { var key = name ?? parameter.Name; var type = Nullable.GetUnderlyingType(parameter.ParameterType) ?? parameter.ParameterType; From 7d0c813e5b8ae1e833ea1fa3ce090d452ec1bdf4 Mon Sep 17 00:00:00 2001 From: Stephen Halter Date: Mon, 5 Apr 2021 19:09:11 -0700 Subject: [PATCH 06/35] Add RequestDelegatePopulatesUnattributedTryParseableParametersFromQueryString --- .../test/RequestDelegateFactoryTests.cs | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs b/src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs index c9239d82a644..a6535b31a191 100644 --- a/src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs +++ b/src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs @@ -334,6 +334,23 @@ public async Task RequestDelegatePopulatesUnattributedTryParseableParametersFrom Assert.Equal(expectedParameterValue, httpContext.Items["tryParsable"]); } + [Theory] + [MemberData(nameof(FromTryParsableParameter))] + public async Task RequestDelegatePopulatesUnattributedTryParseableParametersFromQueryString(Delegate action, string routeValue, object expectedParameterValue) + { + var httpContext = new DefaultHttpContext(); + httpContext.Request.Query = new QueryCollection(new Dictionary + { + ["tryParsable"] = routeValue + }); + + var requestDelegate = RequestDelegateFactory.Create(action); + + await requestDelegate(httpContext); + + Assert.Equal(expectedParameterValue, httpContext.Items["tryParsable"]); + } + [Fact] public async Task RequestDelegatePopulatesFromQueryParameterBasedOnParameterName() { From edbeffaac032f2693c0947e152a04cfdeb8681e1 Mon Sep 17 00:00:00 2001 From: Stephen Halter Date: Mon, 5 Apr 2021 19:26:13 -0700 Subject: [PATCH 07/35] refactor --- .../src/RequestDelegateFactory.cs | 37 +++++++++++-------- 1 file changed, 21 insertions(+), 16 deletions(-) diff --git a/src/Http/Http.Extensions/src/RequestDelegateFactory.cs b/src/Http/Http.Extensions/src/RequestDelegateFactory.cs index 5616daa0e177..01b1a0171412 100644 --- a/src/Http/Http.Extensions/src/RequestDelegateFactory.cs +++ b/src/Http/Http.Extensions/src/RequestDelegateFactory.cs @@ -162,17 +162,17 @@ public static RequestDelegate Create(MethodInfo methodInfo, Func().FirstOrDefault() is { } routeAttribute) { var routeValuesProperty = Expression.Property(HttpRequestExpr, nameof(HttpRequest.RouteValues)); - paramterExpression = BindParamenter(routeValuesProperty, parameter, routeAttribute.Name); + paramterExpression = BindParamenterFromSource(routeValuesProperty, parameter, routeAttribute.Name); } else if (parameterCustomAttributes.OfType().FirstOrDefault() is { } queryAttribute) { var queryProperty = Expression.Property(HttpRequestExpr, nameof(HttpRequest.Query)); - paramterExpression = BindParamenter(queryProperty, parameter, queryAttribute.Name); + paramterExpression = BindParamenterFromSource(queryProperty, parameter, queryAttribute.Name); } else if (parameterCustomAttributes.OfType().FirstOrDefault() is { } headerAttribute) { var headersProperty = Expression.Property(HttpRequestExpr, nameof(HttpRequest.Headers)); - paramterExpression = BindParamenter(headersProperty, parameter, headerAttribute.Name); + paramterExpression = BindParamenterFromSource(headersProperty, parameter, headerAttribute.Name); } else if (parameterCustomAttributes.OfType().FirstOrDefault() is { } bodyAttribute) { @@ -201,7 +201,7 @@ public static RequestDelegate Create(MethodInfo methodInfo, Func typeof(IFromServiceMetadata).IsAssignableFrom(a.AttributeType))) { @@ -229,7 +229,7 @@ public static RequestDelegate Create(MethodInfo methodInfo, Func Date: Mon, 5 Apr 2021 19:27:20 -0700 Subject: [PATCH 08/35] RequestDelegateFactory logger category --- src/Http/Http.Extensions/src/RequestDelegateFactory.cs | 4 ++-- .../Http.Extensions/test/RequestDelegateFactoryTests.cs | 9 +++++---- 2 files changed, 7 insertions(+), 6 deletions(-) diff --git a/src/Http/Http.Extensions/src/RequestDelegateFactory.cs b/src/Http/Http.Extensions/src/RequestDelegateFactory.cs index 01b1a0171412..a4b907df1ea3 100644 --- a/src/Http/Http.Extensions/src/RequestDelegateFactory.cs +++ b/src/Http/Http.Extensions/src/RequestDelegateFactory.cs @@ -229,7 +229,7 @@ public static RequestDelegate Create(MethodInfo methodInfo, Func(); - return loggerFactory.CreateLogger("Microsoft.AspNetCore.Routing.MapAction"); + return loggerFactory.CreateLogger(typeof(RequestDelegateFactory)); } private static Expression BindParamenterFromSource(Expression sourceExpression, ParameterInfo parameter, string? name = null) diff --git a/src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs b/src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs index a6535b31a191..ae444c1ae89d 100644 --- a/src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs +++ b/src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs @@ -297,6 +297,7 @@ void StoreTryParsableParameter(HttpContext httpContext, T tryParsable) return new[] { + // User defined! new object[] { (Action)StoreTryParsableParameter, "42", 42 }, // Byte // Int16 @@ -497,7 +498,7 @@ public async Task RequestDelegateLogsFromBodyIOExceptionsAsDebugAndAborts() { var invoked = false; - var sink = new TestSink(context => context.LoggerName == "Microsoft.AspNetCore.Routing.MapAction"); + var sink = new TestSink(context => context.LoggerName == "Microsoft.AspNetCore.Http.RequestDelegateFactory"); var testLoggerFactory = new TestLoggerFactory(sink, enabled: true); void TestAction([FromBody] Todo todo) @@ -533,7 +534,7 @@ public async Task RequestDelegateLogsFromBodyInvalidDataExceptionsAsDebugAndSets { var invoked = false; - var sink = new TestSink(context => context.LoggerName == "Microsoft.AspNetCore.Routing.MapAction"); + var sink = new TestSink(context => context.LoggerName == "Microsoft.AspNetCore.Http.RequestDelegateFactory"); var testLoggerFactory = new TestLoggerFactory(sink, enabled: true); void TestAction([FromBody] Todo todo) @@ -598,7 +599,7 @@ public async Task RequestDelegateLogsFromFormIOExceptionsAsDebugAndAborts() { var invoked = false; - var sink = new TestSink(context => context.LoggerName == "Microsoft.AspNetCore.Routing.MapAction"); + var sink = new TestSink(context => context.LoggerName == "Microsoft.AspNetCore.Http.RequestDelegateFactory"); var testLoggerFactory = new TestLoggerFactory(sink, enabled: true); void TestAction([FromForm] int value) @@ -634,7 +635,7 @@ public async Task RequestDelegateLogsFromFormInvalidDataExceptionsAsDebugAndSets { var invoked = false; - var sink = new TestSink(context => context.LoggerName == "Microsoft.AspNetCore.Routing.MapAction"); + var sink = new TestSink(context => context.LoggerName == "Microsoft.AspNetCore.Http.RequestDelegateFactory"); var testLoggerFactory = new TestLoggerFactory(sink, enabled: true); void TestAction([FromForm] int value) From ae4b823f7ffedaea4715bc7c8a1ece9711e3bafc Mon Sep 17 00:00:00 2001 From: Stephen Halter Date: Tue, 6 Apr 2021 13:29:41 -0700 Subject: [PATCH 09/35] refactor parameter binding --- .../src/RequestDelegateFactory.cs | 93 +++++++++++-------- 1 file changed, 52 insertions(+), 41 deletions(-) diff --git a/src/Http/Http.Extensions/src/RequestDelegateFactory.cs b/src/Http/Http.Extensions/src/RequestDelegateFactory.cs index a4b907df1ea3..09da694afdbd 100644 --- a/src/Http/Http.Extensions/src/RequestDelegateFactory.cs +++ b/src/Http/Http.Extensions/src/RequestDelegateFactory.cs @@ -45,6 +45,10 @@ public static class RequestDelegateFactory private static readonly MemberExpression HttpRequestExpr = Expression.Property(HttpContextParameter, nameof(HttpContext.Request)); private static readonly MemberExpression HttpResponseExpr = Expression.Property(HttpContextParameter, nameof(HttpContext.Response)); private static readonly MemberExpression RequestAbortedExpr = Expression.Property(HttpContextParameter, nameof(HttpContext.RequestAborted)); + private static readonly MemberExpression RouteValuesProperty = Expression.Property(HttpRequestExpr, nameof(HttpRequest.RouteValues)); + private static readonly MemberExpression QueryProperty = Expression.Property(HttpRequestExpr, nameof(HttpRequest.Query)); + private static readonly MemberExpression HeadersProperty = Expression.Property(HttpRequestExpr, nameof(HttpRequest.Headers)); + private static readonly MemberExpression FormProperty = Expression.Property(HttpRequestExpr, nameof(HttpRequest.Form)); /// /// Creates a implementation for . @@ -74,7 +78,7 @@ public static RequestDelegate Create(Delegate action) /// /// Creates a implementation for . - /// + /// Microsoft.AspNetCore.Routing.MapAction" /// A static request handler with any number of custom parameters that often produces a response with its return value. /// The . public static RequestDelegate Create(MethodInfo methodInfo) @@ -155,24 +159,27 @@ public static RequestDelegate Create(MethodInfo methodInfo, Func().FirstOrDefault() is { } routeAttribute) { - var routeValuesProperty = Expression.Property(HttpRequestExpr, nameof(HttpRequest.RouteValues)); - paramterExpression = BindParamenterFromSource(routeValuesProperty, parameter, routeAttribute.Name); + paramterExpression = BindParameterFromProperty(parameter, RouteValuesProperty, routeAttribute.Name ?? parameter.Name); } else if (parameterCustomAttributes.OfType().FirstOrDefault() is { } queryAttribute) { - var queryProperty = Expression.Property(HttpRequestExpr, nameof(HttpRequest.Query)); - paramterExpression = BindParamenterFromSource(queryProperty, parameter, queryAttribute.Name); + paramterExpression = BindParameterFromProperty(parameter, QueryProperty, queryAttribute.Name ?? parameter.Name); } else if (parameterCustomAttributes.OfType().FirstOrDefault() is { } headerAttribute) { - var headersProperty = Expression.Property(HttpRequestExpr, nameof(HttpRequest.Headers)); - paramterExpression = BindParamenterFromSource(headersProperty, parameter, headerAttribute.Name); + paramterExpression = BindParameterFromProperty(parameter, HeadersProperty, headerAttribute.Name ?? parameter.Name); } else if (parameterCustomAttributes.OfType().FirstOrDefault() is { } bodyAttribute) { @@ -199,9 +206,7 @@ public static RequestDelegate Create(MethodInfo methodInfo, Func typeof(IFromServiceMetadata).IsAssignableFrom(a.AttributeType))) { @@ -226,10 +231,13 @@ public static RequestDelegate Create(MethodInfo methodInfo, Func + BindParameterFromValue(parameter, GetValueFromProperty(property, key)); + + private static Expression BindParameterFromRouteValueOrQueryString(ParameterInfo parameter, string key, MethodInfo? parseMethod = null) + { + var routeValue = GetValueFromProperty(RouteValuesProperty, key); + var queryValue = GetValueFromProperty(QueryProperty, key); + return BindParameterFromValue(parameter, Expression.Coalesce(routeValue, queryValue), parseMethod); + } + private static MethodInfo GetMethodInfo(Expression expr) { var mc = (MethodCallExpression)expr.Body; From 44e854a7b4449203248ab5043e3d7f4f095c5e60 Mon Sep 17 00:00:00 2001 From: Stephen Halter Date: Tue, 6 Apr 2021 14:19:21 -0700 Subject: [PATCH 10/35] more refactoring --- .../src/RequestDelegateFactory.cs | 161 +++++++++++------- 1 file changed, 95 insertions(+), 66 deletions(-) diff --git a/src/Http/Http.Extensions/src/RequestDelegateFactory.cs b/src/Http/Http.Extensions/src/RequestDelegateFactory.cs index 09da694afdbd..5f0f77a094de 100644 --- a/src/Http/Http.Extensions/src/RequestDelegateFactory.cs +++ b/src/Http/Http.Extensions/src/RequestDelegateFactory.cs @@ -146,116 +146,130 @@ public static RequestDelegate Create(MethodInfo methodInfo, Func(methodParameters.Length); + var args = CreateArgumentExpresssions(methodParameters, context); - foreach (var parameter in methodParameters) + MethodCallExpression methodCall; + + if (targetExpression is null) + { + methodCall = Expression.Call(methodInfo, args); + } + else + { + methodCall = Expression.Call(targetExpression, methodInfo, args); + } + + var body = CreateRequestDelegateBody(methodInfo, methodCall); + return CreateRequestDelegateFromBodyExpression(methodInfo, body, context); + } + + private static Expression[] CreateArgumentExpresssions(ParameterInfo[]? parameters, RequestBodyContext context) + { + if (parameters is null || parameters.Length == 0) { + return Array.Empty(); + } + + var args = new Expression[parameters.Length]; + + for (int i = 0; i < parameters.Length; i++) + { + var parameter = parameters[i]; + if (parameter.Name is null) { // TODO: Add test! throw new InvalidOperationException($"Parameter {parameter} does not have a name! All parameters must be named."); } - Expression paramterExpression = Expression.Default(parameter.ParameterType); + Expression argumentExpression = Expression.Default(parameter.ParameterType); var parameterCustomAttributes = parameter.GetCustomAttributes(); if (parameterCustomAttributes.OfType().FirstOrDefault() is { } routeAttribute) { - paramterExpression = BindParameterFromProperty(parameter, RouteValuesProperty, routeAttribute.Name ?? parameter.Name); + argumentExpression = BindParameterFromProperty(parameter, RouteValuesProperty, routeAttribute.Name ?? parameter.Name); } else if (parameterCustomAttributes.OfType().FirstOrDefault() is { } queryAttribute) { - paramterExpression = BindParameterFromProperty(parameter, QueryProperty, queryAttribute.Name ?? parameter.Name); + argumentExpression = BindParameterFromProperty(parameter, QueryProperty, queryAttribute.Name ?? parameter.Name); } else if (parameterCustomAttributes.OfType().FirstOrDefault() is { } headerAttribute) { - paramterExpression = BindParameterFromProperty(parameter, HeadersProperty, headerAttribute.Name ?? parameter.Name); + argumentExpression = BindParameterFromProperty(parameter, HeadersProperty, headerAttribute.Name ?? parameter.Name); } else if (parameterCustomAttributes.OfType().FirstOrDefault() is { } bodyAttribute) { - if (consumeBodyDirectly) + if (context.Mode is RequestBodyMode.AsJson) { throw new InvalidOperationException("Action cannot have more than one FromBody attribute."); } - if (consumeBodyAsForm) + if (context.Mode is RequestBodyMode.AsForm) { ThrowCannotReadBodyDirectlyAndAsForm(); } - consumeBodyDirectly = true; - allowEmptyBody = bodyAttribute.AllowEmpty; - bodyType = parameter.ParameterType; - paramterExpression = Expression.Convert(DeserializedBodyArg, bodyType); + context.Mode = RequestBodyMode.AsJson; + context.JsonBodyType = parameter.ParameterType; + context.AllowEmptyBody = bodyAttribute.AllowEmpty; + argumentExpression = Expression.Convert(DeserializedBodyArg, parameter.ParameterType); } else if (parameterCustomAttributes.OfType().FirstOrDefault() is { } formAttribute) { - if (consumeBodyDirectly) + if (context.Mode is RequestBodyMode.AsJson) { ThrowCannotReadBodyDirectlyAndAsForm(); } - consumeBodyAsForm = true; - paramterExpression = BindParameterFromProperty(parameter, FormProperty, formAttribute.Name ?? parameter.Name); + context.Mode = RequestBodyMode.AsForm; + argumentExpression = BindParameterFromProperty(parameter, FormProperty, formAttribute.Name ?? parameter.Name); } else if (parameter.CustomAttributes.Any(a => typeof(IFromServiceMetadata).IsAssignableFrom(a.AttributeType))) { - paramterExpression = Expression.Call(GetRequiredServiceMethodInfo.MakeGenericMethod(parameter.ParameterType), RequestServicesExpr); + argumentExpression = Expression.Call(GetRequiredServiceMethodInfo.MakeGenericMethod(parameter.ParameterType), RequestServicesExpr); } else if (parameter.ParameterType == typeof(IFormCollection)) { - if (consumeBodyDirectly) + if (context.Mode is RequestBodyMode.AsJson) { ThrowCannotReadBodyDirectlyAndAsForm(); } - consumeBodyAsForm = true; - - paramterExpression = Expression.Property(HttpRequestExpr, nameof(HttpRequest.Form)); + context.Mode = RequestBodyMode.AsForm; + argumentExpression = Expression.Property(HttpRequestExpr, nameof(HttpRequest.Form)); } else if (parameter.ParameterType == typeof(HttpContext)) { - paramterExpression = HttpContextParameter; + argumentExpression = HttpContextParameter; } else if (parameter.ParameterType == typeof(CancellationToken)) { - paramterExpression = RequestAbortedExpr; + argumentExpression = RequestAbortedExpr; } else if (parameter.ParameterType == typeof(string)) { - paramterExpression = BindParameterFromRouteValueOrQueryString(parameter, parameter.Name); + argumentExpression = BindParameterFromRouteValueOrQueryString(parameter, parameter.Name); } else if (FindParseMethod(parameter) is { } parseMethod) { - paramterExpression = BindParameterFromRouteValueOrQueryString(parameter, parameter.Name, parseMethod); + argumentExpression = BindParameterFromRouteValueOrQueryString(parameter, parameter.Name, parseMethod); } - args.Add(paramterExpression); + args[i] = argumentExpression; } - Expression? body = null; - - MethodCallExpression methodCall; - - if (targetExpression is null) - { - methodCall = Expression.Call(methodInfo, args); - } - else - { - methodCall = Expression.Call(targetExpression, methodInfo, args); - } + return args; + } + private static Expression CreateRequestDelegateBody(MethodInfo methodInfo, Expression methodCall) + { // Exact request delegate match if (methodInfo.ReturnType == typeof(void)) { @@ -265,17 +279,17 @@ public static RequestDelegate Create(MethodInfo methodInfo, Func(action(..), httpContext); if (typeArg == typeof(string)) { - body = Expression.Call( + return Expression.Call( ExecuteTaskOfStringMethodInfo, methodCall, HttpContextParameter); } else { - body = Expression.Call( + return Expression.Call( ExecuteTaskOfTMethodInfo.MakeGenericMethod(typeArg), methodCall, HttpContextParameter); @@ -317,7 +331,7 @@ public static RequestDelegate Create(MethodInfo methodInfo, Func(action(..), httpContext); if (typeArg == typeof(string)) { - body = Expression.Call( + return Expression.Call( ExecuteValueTaskOfStringMethodInfo, methodCall, HttpContextParameter); } else { - body = Expression.Call( + return Expression.Call( ExecuteValueTaskOfTMethodInfo.MakeGenericMethod(typeArg), methodCall, HttpContextParameter); @@ -349,41 +363,44 @@ public static RequestDelegate Create(MethodInfo methodInfo, Func? requestDelegate = null; - - if (consumeBodyDirectly) + private static Func CreateRequestDelegateFromBodyExpression(MethodInfo methodInfo, Expression body, RequestBodyContext context) + { + if (context.Mode is RequestBodyMode.AsJson) { // We need to generate the code for reading from the body before calling into the delegate var lambda = Expression.Lambda>(body, TargetArg, HttpContextParameter, DeserializedBodyArg); var invoker = lambda.Compile(); + + var bodyType = context.JsonBodyType!; object? defaultBodyValue = null; - if (allowEmptyBody && bodyType!.IsValueType) + if (context.AllowEmptyBody && bodyType.IsValueType) { defaultBodyValue = Activator.CreateInstance(bodyType); } - requestDelegate = async (target, httpContext) => + return async (target, httpContext) => { object? bodyValue; - if (allowEmptyBody && httpContext.Request.ContentLength == 0) + if (context.AllowEmptyBody && httpContext.Request.ContentLength == 0) { bodyValue = defaultBodyValue; } @@ -391,7 +408,7 @@ public static RequestDelegate Create(MethodInfo methodInfo, Func>(body, TargetArg, HttpContextParameter); var invoker = lambda.Compile(); - requestDelegate = async (target, httpContext) => + return async (target, httpContext) => { // Generating async code would just be insane so if the method needs the form populate it here // so the within the method it's cached @@ -444,10 +461,8 @@ public static RequestDelegate Create(MethodInfo methodInfo, Func>(body, TargetArg, HttpContextParameter); var invoker = lambda.Compile(); - requestDelegate = invoker; + return invoker; } - - return requestDelegate; } private static MethodInfo? FindParseMethod(ParameterInfo parameter) @@ -655,6 +670,20 @@ private static void ThrowCannotReadBodyDirectlyAndAsForm() throw new InvalidOperationException("Action cannot mix FromBody and FromForm on the same method."); } + private class RequestBodyContext + { + public RequestBodyMode Mode { get; set; } + public Type? JsonBodyType { get; set; } + public bool AllowEmptyBody { get; set; } + } + + private enum RequestBodyMode + { + None, + AsJson, + AsForm, + } + private static class Log { private static readonly Action _requestBodyIOException = LoggerMessage.Define( From b67a2331a9e0c3ea704d12e098fddf51badc5d5d Mon Sep 17 00:00:00 2001 From: Stephen Halter Date: Tue, 6 Apr 2021 14:26:24 -0700 Subject: [PATCH 11/35] Get RequestDelegatePopulatesFromServiceParameterBasedOnParameterType passing --- .../src/RequestDelegateFactory.cs | 24 +++++++++++-------- .../test/RequestDelegateFactoryTests.cs | 2 +- 2 files changed, 15 insertions(+), 11 deletions(-) diff --git a/src/Http/Http.Extensions/src/RequestDelegateFactory.cs b/src/Http/Http.Extensions/src/RequestDelegateFactory.cs index 5f0f77a094de..afc6b8fc3482 100644 --- a/src/Http/Http.Extensions/src/RequestDelegateFactory.cs +++ b/src/Http/Http.Extensions/src/RequestDelegateFactory.cs @@ -68,11 +68,11 @@ public static RequestDelegate Create(Delegate action) null => null, }; - var untargetedRequestDelegate = CreateRequestDelegate(action.Method, targetExpression); + var targettableRequestDelegate = CreateTargettableRequestDelegate(action.Method, targetExpression); return httpContext => { - return untargetedRequestDelegate(action.Target, httpContext); + return targettableRequestDelegate(action.Target, httpContext); }; } @@ -88,11 +88,11 @@ public static RequestDelegate Create(MethodInfo methodInfo) throw new ArgumentNullException(nameof(methodInfo)); } - var untargetedRequestDelegate = CreateRequestDelegate(methodInfo, targetExpression: null); + var targettableRequestDelegate = CreateTargettableRequestDelegate(methodInfo, targetExpression: null); return httpContext => { - return untargetedRequestDelegate(null, httpContext); + return targettableRequestDelegate(null, httpContext); }; } @@ -120,15 +120,15 @@ public static RequestDelegate Create(MethodInfo methodInfo, Func { - return untargetedRequestDelegate(targetFactory(httpContext), httpContext); + return targettableRequestDelegate(targetFactory(httpContext), httpContext); }; } - private static Func CreateRequestDelegate(MethodInfo methodInfo, Expression? targetExpression) + private static Func CreateTargettableRequestDelegate(MethodInfo methodInfo, Expression? targetExpression) { // Non void return type @@ -166,7 +166,7 @@ public static RequestDelegate Create(MethodInfo methodInfo, Func CreateRequestDelegateFromBodyExpression(MethodInfo methodInfo, Expression body, RequestBodyContext context) + private static Func CreateTargettableRequestDelegateFromBodyExpression(MethodInfo methodInfo, Expression body, RequestBodyContext context) { if (context.Mode is RequestBodyMode.AsJson) { diff --git a/src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs b/src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs index ae444c1ae89d..c96159b652d9 100644 --- a/src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs +++ b/src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs @@ -719,7 +719,7 @@ public async Task RequestDelegatePopulatesFromServiceParameterBasedOnParameterTy var httpContext = new DefaultHttpContext(); httpContext.RequestServices = serviceCollection.BuildServiceProvider(); - var requestDelegate = RequestDelegateFactory.Create((Action)@delegate); + var requestDelegate = RequestDelegateFactory.Create((Action)@delegate); await requestDelegate(httpContext); From f590c8981afb67db47cf12f75cdc9884d1149cf3 Mon Sep 17 00:00:00 2001 From: Stephen Halter Date: Tue, 6 Apr 2021 14:52:15 -0700 Subject: [PATCH 12/35] cleanup --- .../src/RequestDelegateFactory.cs | 207 ++++++++---------- .../test/RequestDelegateFactoryTests.cs | 8 +- 2 files changed, 95 insertions(+), 120 deletions(-) diff --git a/src/Http/Http.Extensions/src/RequestDelegateFactory.cs b/src/Http/Http.Extensions/src/RequestDelegateFactory.cs index afc6b8fc3482..7eda9b90cd89 100644 --- a/src/Http/Http.Extensions/src/RequestDelegateFactory.cs +++ b/src/Http/Http.Extensions/src/RequestDelegateFactory.cs @@ -146,31 +146,21 @@ public static RequestDelegate Create(MethodInfo methodInfo, Func(); @@ -178,7 +168,7 @@ private static Expression[] CreateArgumentExpresssions(ParameterInfo[]? paramete var args = new Expression[parameters.Length]; - for (int i = 0; i < parameters.Length; i++) + for (var i = 0; i < parameters.Length; i++) { var parameter = parameters[i]; @@ -206,29 +196,29 @@ private static Expression[] CreateArgumentExpresssions(ParameterInfo[]? paramete } else if (parameterCustomAttributes.OfType().FirstOrDefault() is { } bodyAttribute) { - if (context.Mode is RequestBodyMode.AsJson) + if (requestBodyContext.Mode is RequestBodyMode.AsJson) { throw new InvalidOperationException("Action cannot have more than one FromBody attribute."); } - if (context.Mode is RequestBodyMode.AsForm) + if (requestBodyContext.Mode is RequestBodyMode.AsForm) { ThrowCannotReadBodyDirectlyAndAsForm(); } - context.Mode = RequestBodyMode.AsJson; - context.JsonBodyType = parameter.ParameterType; - context.AllowEmptyBody = bodyAttribute.AllowEmpty; + requestBodyContext.Mode = RequestBodyMode.AsJson; + requestBodyContext.JsonBodyType = parameter.ParameterType; + requestBodyContext.AllowEmptyBody = bodyAttribute.AllowEmpty; argumentExpression = Expression.Convert(DeserializedBodyArg, parameter.ParameterType); } else if (parameterCustomAttributes.OfType().FirstOrDefault() is { } formAttribute) { - if (context.Mode is RequestBodyMode.AsJson) + if (requestBodyContext.Mode is RequestBodyMode.AsJson) { ThrowCannotReadBodyDirectlyAndAsForm(); } - context.Mode = RequestBodyMode.AsForm; + requestBodyContext.Mode = RequestBodyMode.AsForm; argumentExpression = BindParameterFromProperty(parameter, FormProperty, formAttribute.Name ?? parameter.Name); } else if (parameter.CustomAttributes.Any(a => typeof(IFromServiceMetadata).IsAssignableFrom(a.AttributeType))) @@ -237,12 +227,12 @@ private static Expression[] CreateArgumentExpresssions(ParameterInfo[]? paramete } else if (parameter.ParameterType == typeof(IFormCollection)) { - if (context.Mode is RequestBodyMode.AsJson) + if (requestBodyContext.Mode is RequestBodyMode.AsJson) { ThrowCannotReadBodyDirectlyAndAsForm(); } - context.Mode = RequestBodyMode.AsForm; + requestBodyContext.Mode = RequestBodyMode.AsForm; argumentExpression = Expression.Property(HttpRequestExpr, nameof(HttpRequest.Form)); } else if (parameter.ParameterType == typeof(HttpContext)) @@ -272,108 +262,100 @@ private static Expression[] CreateArgumentExpresssions(ParameterInfo[]? paramete return args; } - private static Expression CreateRequestDelegateBody(MethodInfo methodInfo, Expression methodCall) + private static Expression CreateMethodCallingAndResponseWritingExpression(Expression methodCall, Type returnType) { // Exact request delegate match - if (methodInfo.ReturnType == typeof(void)) + if (returnType == typeof(void)) { - var bodyExpressions = new List + return Expression.Block(new[] { methodCall, Expression.Property(null, (PropertyInfo)CompletedTaskMemberInfo) - }; - - return Expression.Block(bodyExpressions); + }); } - else if (AwaitableInfo.IsTypeAwaitable(methodInfo.ReturnType, out var info)) + else if (AwaitableInfo.IsTypeAwaitable(returnType, out _)) { - if (methodInfo.ReturnType == typeof(Task)) + if (returnType == typeof(Task)) { return methodCall; } - else if (methodInfo.ReturnType == typeof(ValueTask)) + else if (returnType == typeof(ValueTask)) { return Expression.Call( - ExecuteValueTaskMethodInfo, - methodCall); + ExecuteValueTaskMethodInfo, + methodCall); } - else if (methodInfo.ReturnType.IsGenericType && - methodInfo.ReturnType.GetGenericTypeDefinition() == typeof(Task<>)) + else if (returnType.IsGenericType && + returnType.GetGenericTypeDefinition() == typeof(Task<>)) { - var typeArg = methodInfo.ReturnType.GetGenericArguments()[0]; + var typeArg = returnType.GetGenericArguments()[0]; if (typeof(IResult).IsAssignableFrom(typeArg)) { return Expression.Call( - ExecuteTaskResultOfTMethodInfo.MakeGenericMethod(typeArg), - methodCall, - HttpContextParameter); + ExecuteTaskResultOfTMethodInfo.MakeGenericMethod(typeArg), + methodCall, + HttpContextParameter); + } + // ExecuteTask(action(..), httpContext); + else if (typeArg == typeof(string)) + { + return Expression.Call( + ExecuteTaskOfStringMethodInfo, + methodCall, + HttpContextParameter); } else { - // ExecuteTask(action(..), httpContext); - if (typeArg == typeof(string)) - { - return Expression.Call( - ExecuteTaskOfStringMethodInfo, - methodCall, - HttpContextParameter); - } - else - { - return Expression.Call( - ExecuteTaskOfTMethodInfo.MakeGenericMethod(typeArg), - methodCall, - HttpContextParameter); - } + return Expression.Call( + ExecuteTaskOfTMethodInfo.MakeGenericMethod(typeArg), + methodCall, + HttpContextParameter); } } - else if (methodInfo.ReturnType.IsGenericType && - methodInfo.ReturnType.GetGenericTypeDefinition() == typeof(ValueTask<>)) + else if (returnType.IsGenericType && + returnType.GetGenericTypeDefinition() == typeof(ValueTask<>)) { - var typeArg = methodInfo.ReturnType.GetGenericArguments()[0]; + var typeArg = returnType.GetGenericArguments()[0]; if (typeof(IResult).IsAssignableFrom(typeArg)) { return Expression.Call( - ExecuteValueResultTaskOfTMethodInfo.MakeGenericMethod(typeArg), - methodCall, - HttpContextParameter); + ExecuteValueResultTaskOfTMethodInfo.MakeGenericMethod(typeArg), + methodCall, + HttpContextParameter); + } + // ExecuteTask(action(..), httpContext); + else if (typeArg == typeof(string)) + { + return Expression.Call( + ExecuteValueTaskOfStringMethodInfo, + methodCall, + HttpContextParameter); } else { - // ExecuteTask(action(..), httpContext); - if (typeArg == typeof(string)) - { - return Expression.Call( - ExecuteValueTaskOfStringMethodInfo, - methodCall, - HttpContextParameter); - } - else - { - return Expression.Call( - ExecuteValueTaskOfTMethodInfo.MakeGenericMethod(typeArg), - methodCall, - HttpContextParameter); - } + return Expression.Call( + ExecuteValueTaskOfTMethodInfo.MakeGenericMethod(typeArg), + methodCall, + HttpContextParameter); } } else { // TODO: Handle custom awaitables - throw new NotSupportedException($"Unsupported return type: {methodInfo.ReturnType}"); + throw new NotSupportedException($"Unsupported return type: {returnType}"); } } - else if (typeof(IResult).IsAssignableFrom(methodInfo.ReturnType)) + else if (typeof(IResult).IsAssignableFrom(returnType)) { return Expression.Call(methodCall, ResultWriteResponseAsync, HttpContextParameter); } - else if (methodInfo.ReturnType == typeof(string)) + else if (returnType == typeof(string)) { return Expression.Call(StringResultWriteResponseAsync, HttpResponseExpr, methodCall, Expression.Constant(CancellationToken.None)); } - else if (methodInfo.ReturnType.IsValueType) + else if (returnType.IsValueType) { var box = Expression.TypeAs(methodCall, typeof(object)); return Expression.Call(JsonResultWriteResponseAsync, HttpResponseExpr, box, Expression.Constant(CancellationToken.None)); @@ -384,18 +366,17 @@ private static Expression CreateRequestDelegateBody(MethodInfo methodInfo, Expre } } - private static Func CreateTargettableRequestDelegateFromBodyExpression(MethodInfo methodInfo, Expression body, RequestBodyContext context) + private static Func HandleRequestBodyAndCompileRequestDelegate(Expression body, RequestBodyContext requestBodyContext) { - if (context.Mode is RequestBodyMode.AsJson) + if (requestBodyContext.Mode is RequestBodyMode.AsJson) { // We need to generate the code for reading from the body before calling into the delegate - var lambda = Expression.Lambda>(body, TargetArg, HttpContextParameter, DeserializedBodyArg); - var invoker = lambda.Compile(); + var invoker = Expression.Lambda>(body, TargetArg, HttpContextParameter, DeserializedBodyArg).Compile(); - var bodyType = context.JsonBodyType!; + var bodyType = requestBodyContext.JsonBodyType!; object? defaultBodyValue = null; - if (context.AllowEmptyBody && bodyType.IsValueType) + if (requestBodyContext.AllowEmptyBody && bodyType.IsValueType) { defaultBodyValue = Activator.CreateInstance(bodyType); } @@ -404,7 +385,7 @@ private static Expression CreateRequestDelegateBody(MethodInfo methodInfo, Expre { object? bodyValue; - if (context.AllowEmptyBody && httpContext.Request.ContentLength == 0) + if (requestBodyContext.AllowEmptyBody && httpContext.Request.ContentLength == 0) { bodyValue = defaultBodyValue; } @@ -417,7 +398,6 @@ private static Expression CreateRequestDelegateBody(MethodInfo methodInfo, Expre catch (IOException ex) { Log.RequestBodyIOException(GetLogger(httpContext), ex); - httpContext.Abort(); return; } catch (InvalidDataException ex) @@ -431,10 +411,9 @@ private static Expression CreateRequestDelegateBody(MethodInfo methodInfo, Expre await invoker(target, httpContext, bodyValue); }; } - else if (context.Mode is RequestBodyMode.AsForm) + else if (requestBodyContext.Mode is RequestBodyMode.AsForm) { - var lambda = Expression.Lambda>(body, TargetArg, HttpContextParameter); - var invoker = lambda.Compile(); + var invoker = Expression.Lambda>(body, TargetArg, HttpContextParameter).Compile(); return async (target, httpContext) => { @@ -462,10 +441,7 @@ private static Expression CreateRequestDelegateBody(MethodInfo methodInfo, Expre } else { - var lambda = Expression.Lambda>(body, TargetArg, HttpContextParameter); - var invoker = lambda.Compile(); - - return invoker; + return Expression.Lambda>(body, TargetArg, HttpContextParameter).Compile(); } } @@ -492,12 +468,6 @@ private static Expression CreateRequestDelegateBody(MethodInfo methodInfo, Expre return null; } - private static ILogger GetLogger(HttpContext httpContext) - { - var loggerFactory = httpContext.RequestServices.GetRequiredService(); - return loggerFactory.CreateLogger(typeof(RequestDelegateFactory)); - } - private static Expression GetValueFromProperty(Expression sourceExpression, string key) { var itemProperty = sourceExpression.Type.GetProperty("Item"); @@ -508,11 +478,11 @@ private static Expression GetValueFromProperty(Expression sourceExpression, stri private static Expression BindParameterFromValue(ParameterInfo parameter, Expression valueExpression, MethodInfo? parseMethod = null) { - Expression? expr = null; + Expression argumentExpression; if (parameter.ParameterType == typeof(string)) { - expr = valueExpression; + argumentExpression = valueExpression; } else { @@ -524,15 +494,16 @@ private static Expression BindParameterFromValue(ParameterInfo parameter, Expres throw new InvalidOperationException($"No valid static {parameter.ParameterType}.Parse(string) method for {parameter} was found."); } - expr = Expression.Call(parseMethod, valueExpression); + argumentExpression = Expression.Call(parseMethod, valueExpression); } - if (expr.Type != parameter.ParameterType) + if (argumentExpression.Type != parameter.ParameterType) { - expr = Expression.Convert(expr, parameter.ParameterType); + argumentExpression = Expression.Convert(argumentExpression, parameter.ParameterType); } Expression defaultExpression; + if (parameter.HasDefaultValue) { defaultExpression = Expression.Constant(parameter.DefaultValue); @@ -543,12 +514,10 @@ private static Expression BindParameterFromValue(ParameterInfo parameter, Expres } // property[key] == null ? default : (ParameterType){Type}.Parse(property[key]); - expr = Expression.Condition( + return Expression.Condition( Expression.Equal(valueExpression, Expression.Constant(null)), defaultExpression, - expr); - - return expr; + argumentExpression); } private static Expression BindParameterFromProperty(ParameterInfo parameter, MemberExpression property, string key) => @@ -561,6 +530,12 @@ private static Expression BindParameterFromRouteValueOrQueryString(ParameterInfo return BindParameterFromValue(parameter, Expression.Coalesce(routeValue, queryValue), parseMethod); } + private static ILogger GetLogger(HttpContext httpContext) + { + var loggerFactory = httpContext.RequestServices.GetRequiredService(); + return loggerFactory.CreateLogger(typeof(RequestDelegateFactory)); + } + private static MethodInfo GetMethodInfo(Expression expr) { var mc = (MethodCallExpression)expr.Body; diff --git a/src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs b/src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs index c96159b652d9..9e3558e712ce 100644 --- a/src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs +++ b/src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs @@ -494,7 +494,7 @@ void TestAction([FromBody(AllowEmpty = true)] BodyStruct bodyStruct) } [Fact] - public async Task RequestDelegateLogsFromBodyIOExceptionsAsDebugAndAborts() + public async Task RequestDelegateLogsFromBodyIOExceptionsAsDebugAndDoesNotAbort() { var invoked = false; @@ -521,7 +521,7 @@ void TestAction([FromBody] Todo todo) await requestDelegate(httpContext); Assert.False(invoked); - Assert.True(httpContext.RequestAborted.IsCancellationRequested); + Assert.False(httpContext.RequestAborted.IsCancellationRequested); var logMessage = Assert.Single(sink.Writes); Assert.Equal(new EventId(1, "RequestBodyIOException"), logMessage.EventId); @@ -709,7 +709,7 @@ void TestImpliedFromService(HttpContext httpContext, MyService myService) [Theory] [MemberData(nameof(FromServiceParameter))] - public async Task RequestDelegatePopulatesFromServiceParameterBasedOnParameterType(Delegate @delegate) + public async Task RequestDelegatePopulatesFromServiceParameterBasedOnParameterType(Delegate action) { var myOriginalService = new MyService(); @@ -719,7 +719,7 @@ public async Task RequestDelegatePopulatesFromServiceParameterBasedOnParameterTy var httpContext = new DefaultHttpContext(); httpContext.RequestServices = serviceCollection.BuildServiceProvider(); - var requestDelegate = RequestDelegateFactory.Create((Action)@delegate); + var requestDelegate = RequestDelegateFactory.Create((Action)action); await requestDelegate(httpContext); From b113bc2be27504a0762151abb191e564e27bb412 Mon Sep 17 00:00:00 2001 From: Stephen Halter Date: Tue, 6 Apr 2021 15:25:41 -0700 Subject: [PATCH 13/35] Add CreateArgumentExpression --- .../src/RequestDelegateFactory.cs | 162 +++++++++--------- 1 file changed, 78 insertions(+), 84 deletions(-) diff --git a/src/Http/Http.Extensions/src/RequestDelegateFactory.cs b/src/Http/Http.Extensions/src/RequestDelegateFactory.cs index 7eda9b90cd89..cd22dc716589 100644 --- a/src/Http/Http.Extensions/src/RequestDelegateFactory.cs +++ b/src/Http/Http.Extensions/src/RequestDelegateFactory.cs @@ -2,7 +2,6 @@ // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. using System; -using System.Collections.Generic; using System.Diagnostics; using System.Globalization; using System.IO; @@ -170,96 +169,98 @@ private static Expression[] CreateArgumentExpresssions(ParameterInfo[]? paramete for (var i = 0; i < parameters.Length; i++) { - var parameter = parameters[i]; + args[i] = CreateArgumentExpression(parameters[i], requestBodyContext); + } - if (parameter.Name is null) - { - // TODO: Add test! - throw new InvalidOperationException($"Parameter {parameter} does not have a name! All parameters must be named."); - } + return args; + } - Expression argumentExpression; + private static Expression CreateArgumentExpression(ParameterInfo parameter, RequestBodyContext requestBodyContext) + { + if (parameter.Name is null) + { + // TODO: Add test! + throw new InvalidOperationException($"Parameter {parameter} does not have a name! All parameters must be named."); + } - var parameterCustomAttributes = parameter.GetCustomAttributes(); + var parameterCustomAttributes = parameter.GetCustomAttributes(); - if (parameterCustomAttributes.OfType().FirstOrDefault() is { } routeAttribute) - { - argumentExpression = BindParameterFromProperty(parameter, RouteValuesProperty, routeAttribute.Name ?? parameter.Name); - } - else if (parameterCustomAttributes.OfType().FirstOrDefault() is { } queryAttribute) + if (parameterCustomAttributes.OfType().FirstOrDefault() is { } routeAttribute) + { + return BindParameterFromProperty(parameter, RouteValuesProperty, routeAttribute.Name ?? parameter.Name); + } + else if (parameterCustomAttributes.OfType().FirstOrDefault() is { } queryAttribute) + { + return BindParameterFromProperty(parameter, QueryProperty, queryAttribute.Name ?? parameter.Name); + } + else if (parameterCustomAttributes.OfType().FirstOrDefault() is { } headerAttribute) + { + return BindParameterFromProperty(parameter, HeadersProperty, headerAttribute.Name ?? parameter.Name); + } + else if (parameterCustomAttributes.OfType().FirstOrDefault() is { } bodyAttribute) + { + if (requestBodyContext.Mode is RequestBodyMode.AsJson) { - argumentExpression = BindParameterFromProperty(parameter, QueryProperty, queryAttribute.Name ?? parameter.Name); + throw new InvalidOperationException("Action cannot have more than one FromBody attribute."); } - else if (parameterCustomAttributes.OfType().FirstOrDefault() is { } headerAttribute) + + if (requestBodyContext.Mode is RequestBodyMode.AsForm) { - argumentExpression = BindParameterFromProperty(parameter, HeadersProperty, headerAttribute.Name ?? parameter.Name); + ThrowCannotReadBodyDirectlyAndAsForm(); } - else if (parameterCustomAttributes.OfType().FirstOrDefault() is { } bodyAttribute) - { - if (requestBodyContext.Mode is RequestBodyMode.AsJson) - { - throw new InvalidOperationException("Action cannot have more than one FromBody attribute."); - } - if (requestBodyContext.Mode is RequestBodyMode.AsForm) - { - ThrowCannotReadBodyDirectlyAndAsForm(); - } + requestBodyContext.Mode = RequestBodyMode.AsJson; + requestBodyContext.JsonBodyType = parameter.ParameterType; + requestBodyContext.AllowEmptyBody = bodyAttribute.AllowEmpty; - requestBodyContext.Mode = RequestBodyMode.AsJson; - requestBodyContext.JsonBodyType = parameter.ParameterType; - requestBodyContext.AllowEmptyBody = bodyAttribute.AllowEmpty; - argumentExpression = Expression.Convert(DeserializedBodyArg, parameter.ParameterType); - } - else if (parameterCustomAttributes.OfType().FirstOrDefault() is { } formAttribute) - { - if (requestBodyContext.Mode is RequestBodyMode.AsJson) - { - ThrowCannotReadBodyDirectlyAndAsForm(); - } - - requestBodyContext.Mode = RequestBodyMode.AsForm; - argumentExpression = BindParameterFromProperty(parameter, FormProperty, formAttribute.Name ?? parameter.Name); - } - else if (parameter.CustomAttributes.Any(a => typeof(IFromServiceMetadata).IsAssignableFrom(a.AttributeType))) + return Expression.Convert(DeserializedBodyArg, parameter.ParameterType); + } + else if (parameterCustomAttributes.OfType().FirstOrDefault() is { } formAttribute) + { + if (requestBodyContext.Mode is RequestBodyMode.AsJson) { - argumentExpression = Expression.Call(GetRequiredServiceMethodInfo.MakeGenericMethod(parameter.ParameterType), RequestServicesExpr); + ThrowCannotReadBodyDirectlyAndAsForm(); } - else if (parameter.ParameterType == typeof(IFormCollection)) - { - if (requestBodyContext.Mode is RequestBodyMode.AsJson) - { - ThrowCannotReadBodyDirectlyAndAsForm(); - } - requestBodyContext.Mode = RequestBodyMode.AsForm; - argumentExpression = Expression.Property(HttpRequestExpr, nameof(HttpRequest.Form)); - } - else if (parameter.ParameterType == typeof(HttpContext)) - { - argumentExpression = HttpContextParameter; - } - else if (parameter.ParameterType == typeof(CancellationToken)) - { - argumentExpression = RequestAbortedExpr; - } - else if (parameter.ParameterType == typeof(string)) - { - argumentExpression = BindParameterFromRouteValueOrQueryString(parameter, parameter.Name); - } - else if (FindParseMethod(parameter) is { } parseMethod) - { - argumentExpression = BindParameterFromRouteValueOrQueryString(parameter, parameter.Name, parseMethod); - } - else + requestBodyContext.Mode = RequestBodyMode.AsForm; + + return BindParameterFromProperty(parameter, FormProperty, formAttribute.Name ?? parameter.Name); + } + else if (parameter.CustomAttributes.Any(a => typeof(IFromServiceMetadata).IsAssignableFrom(a.AttributeType))) + { + return Expression.Call(GetRequiredServiceMethodInfo.MakeGenericMethod(parameter.ParameterType), RequestServicesExpr); + } + else if (parameter.ParameterType == typeof(IFormCollection)) + { + if (requestBodyContext.Mode is RequestBodyMode.AsJson) { - argumentExpression = Expression.Call(GetRequiredServiceMethodInfo.MakeGenericMethod(parameter.ParameterType), RequestServicesExpr); + ThrowCannotReadBodyDirectlyAndAsForm(); } - args[i] = argumentExpression; - } + requestBodyContext.Mode = RequestBodyMode.AsForm; - return args; + return Expression.Property(HttpRequestExpr, nameof(HttpRequest.Form)); + } + else if (parameter.ParameterType == typeof(HttpContext)) + { + return HttpContextParameter; + } + else if (parameter.ParameterType == typeof(CancellationToken)) + { + return RequestAbortedExpr; + } + else if (parameter.ParameterType == typeof(string)) + { + return BindParameterFromRouteValueOrQueryString(parameter, parameter.Name); + } + else if (FindParseMethod(parameter) is { } parseMethod) + { + return BindParameterFromRouteValueOrQueryString(parameter, parameter.Name, parseMethod); + } + else + { + return Expression.Call(GetRequiredServiceMethodInfo.MakeGenericMethod(parameter.ParameterType), RequestServicesExpr); + } } private static Expression CreateMethodCallingAndResponseWritingExpression(Expression methodCall, Type returnType) @@ -502,16 +503,9 @@ private static Expression BindParameterFromValue(ParameterInfo parameter, Expres argumentExpression = Expression.Convert(argumentExpression, parameter.ParameterType); } - Expression defaultExpression; - - if (parameter.HasDefaultValue) - { - defaultExpression = Expression.Constant(parameter.DefaultValue); - } - else - { - defaultExpression = Expression.Default(parameter.ParameterType); - } + Expression defaultExpression = parameter.HasDefaultValue ? + Expression.Constant(parameter.DefaultValue) : + Expression.Default(parameter.ParameterType); // property[key] == null ? default : (ParameterType){Type}.Parse(property[key]); return Expression.Condition( From 962fa6592a8ea89f76f56e0a367374d909b6b98d Mon Sep 17 00:00:00 2001 From: Stephen Halter Date: Tue, 6 Apr 2021 15:36:02 -0700 Subject: [PATCH 14/35] Add RequestDelegateRequiresServiceForAllFromServiceParameters --- .../test/RequestDelegateFactoryTests.cs | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs b/src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs index 9e3558e712ce..0eccc4f8f5bf 100644 --- a/src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs +++ b/src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs @@ -709,7 +709,7 @@ void TestImpliedFromService(HttpContext httpContext, MyService myService) [Theory] [MemberData(nameof(FromServiceParameter))] - public async Task RequestDelegatePopulatesFromServiceParameterBasedOnParameterType(Delegate action) + public async Task RequestDelegatePopulatesParametersFromServiceWithAndWithoutAttribute(Delegate action) { var myOriginalService = new MyService(); @@ -726,6 +726,18 @@ public async Task RequestDelegatePopulatesFromServiceParameterBasedOnParameterTy Assert.Same(myOriginalService, httpContext.Items["service"]); } + [Theory] + [MemberData(nameof(FromServiceParameter))] + public async Task RequestDelegateRequiresServiceForAllFromServiceParameters(Delegate action) + { + var httpContext = new DefaultHttpContext(); + httpContext.RequestServices = (new ServiceCollection()).BuildServiceProvider(); + + var requestDelegate = RequestDelegateFactory.Create((Action)action); + + await Assert.ThrowsAsync(() => requestDelegate(httpContext)); + } + [Fact] public async Task RequestDelegatePopulatesHttpContextParameterWithoutAttribute() { From 299985fb30e8116059e1d23b8e344c99bea30ba9 Mon Sep 17 00:00:00 2001 From: Stephen Halter Date: Tue, 6 Apr 2021 16:03:54 -0700 Subject: [PATCH 15/35] Add tests --- .../test/RequestDelegateFactoryTests.cs | 42 +++++++++++++++++-- 1 file changed, 39 insertions(+), 3 deletions(-) diff --git a/src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs b/src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs index 0eccc4f8f5bf..86b6f23b74e2 100644 --- a/src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs +++ b/src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs @@ -124,7 +124,7 @@ public TestNonStaticActionClass(object invokedValue) _invokedValue = invokedValue; } - private void NonStaticTestAction(HttpContext httpContext) + public void NonStaticTestAction(HttpContext httpContext) { httpContext.Items.Add("invoked", _invokedValue); } @@ -132,10 +132,11 @@ private void NonStaticTestAction(HttpContext httpContext) [Fact] public async Task NonStaticMethodInfoOverloadWorksWithBasicReflection() + { var methodInfo = typeof(TestNonStaticActionClass).GetMethod( - "NonStaticTestAction", - BindingFlags.NonPublic | BindingFlags.Instance, + nameof(TestNonStaticActionClass.NonStaticTestAction), + BindingFlags.Public | BindingFlags.Instance, new[] { typeof(HttpContext) }); var invoked = false; @@ -352,6 +353,41 @@ public async Task RequestDelegatePopulatesUnattributedTryParseableParametersFrom Assert.Equal(expectedParameterValue, httpContext.Items["tryParsable"]); } + [Theory] + [MemberData(nameof(FromTryParsableParameter))] + public async Task RequestDelegatePopulatesUnattributedTryParseableParametersFromRouteValueBeforeQueryString(Delegate action, string routeValue, object expectedParameterValue) + { + var httpContext = new DefaultHttpContext(); + httpContext.Request.RouteValues["tryParsable"] = routeValue; + httpContext.Request.Query = new QueryCollection(new Dictionary + { + ["tryParsable"] = "invalid!" + }); + + var requestDelegate = RequestDelegateFactory.Create(action); + + await requestDelegate(httpContext); + + Assert.Equal(expectedParameterValue, httpContext.Items["tryParsable"]); + } + + [Theory] + [MemberData(nameof(FromTryParsableParameter))] + public async Task RequestDelegateLogsTryParsableFailuresAsDebugAndSets400Response(Delegate action, string routeValue, object expectedParameterValue) + { + // disabling error xUnit1026 the "right" way looks ugly + var ignore = routeValue; + + var httpContext = new DefaultHttpContext(); + httpContext.Request.RouteValues["tryParsable"] = "invalid!"; + + var requestDelegate = RequestDelegateFactory.Create(action); + + await requestDelegate(httpContext); + + Assert.Equal(expectedParameterValue, httpContext.Items["tryParsable"]); + } + [Fact] public async Task RequestDelegatePopulatesFromQueryParameterBasedOnParameterName() { From 46714f196fed255da84530091bd8c9f4284de2c4 Mon Sep 17 00:00:00 2001 From: Stephen Halter Date: Tue, 6 Apr 2021 16:45:40 -0700 Subject: [PATCH 16/35] Start using TryParse --- .../src/RequestDelegateFactory.cs | 48 +++++++++++-------- 1 file changed, 28 insertions(+), 20 deletions(-) diff --git a/src/Http/Http.Extensions/src/RequestDelegateFactory.cs b/src/Http/Http.Extensions/src/RequestDelegateFactory.cs index cd22dc716589..47103b19ae9d 100644 --- a/src/Http/Http.Extensions/src/RequestDelegateFactory.cs +++ b/src/Http/Http.Extensions/src/RequestDelegateFactory.cs @@ -239,7 +239,7 @@ private static Expression CreateArgumentExpression(ParameterInfo parameter, Requ requestBodyContext.Mode = RequestBodyMode.AsForm; - return Expression.Property(HttpRequestExpr, nameof(HttpRequest.Form)); + return Expression.Property(HttpRequestExpr, nameof(HttpRequest.Form)); } else if (parameter.ParameterType == typeof(HttpContext)) { @@ -253,7 +253,7 @@ private static Expression CreateArgumentExpression(ParameterInfo parameter, Requ { return BindParameterFromRouteValueOrQueryString(parameter, parameter.Name); } - else if (FindParseMethod(parameter) is { } parseMethod) + else if (FindTryParseMethod(parameter) is { } parseMethod) { return BindParameterFromRouteValueOrQueryString(parameter, parameter.Name, parseMethod); } @@ -268,11 +268,9 @@ private static Expression CreateMethodCallingAndResponseWritingExpression(Expres // Exact request delegate match if (returnType == typeof(void)) { - return Expression.Block(new[] - { + return Expression.Block( methodCall, - Expression.Property(null, (PropertyInfo)CompletedTaskMemberInfo) - }); + Expression.Property(null, (PropertyInfo)CompletedTaskMemberInfo)); } else if (AwaitableInfo.IsTypeAwaitable(returnType, out _)) { @@ -372,7 +370,8 @@ private static Expression CreateMethodCallingAndResponseWritingExpression(Expres if (requestBodyContext.Mode is RequestBodyMode.AsJson) { // We need to generate the code for reading from the body before calling into the delegate - var invoker = Expression.Lambda>(body, TargetArg, HttpContextParameter, DeserializedBodyArg).Compile(); + var invoker = Expression.Lambda>( + body, TargetArg, HttpContextParameter, DeserializedBodyArg).Compile(); var bodyType = requestBodyContext.JsonBodyType!; object? defaultBodyValue = null; @@ -414,7 +413,8 @@ private static Expression CreateMethodCallingAndResponseWritingExpression(Expres } else if (requestBodyContext.Mode is RequestBodyMode.AsForm) { - var invoker = Expression.Lambda>(body, TargetArg, HttpContextParameter).Compile(); + var invoker = Expression.Lambda>( + body, TargetArg, HttpContextParameter).Compile(); return async (target, httpContext) => { @@ -442,25 +442,29 @@ private static Expression CreateMethodCallingAndResponseWritingExpression(Expres } else { - return Expression.Lambda>(body, TargetArg, HttpContextParameter).Compile(); + return Expression.Lambda>( + body, TargetArg, HttpContextParameter).Compile(); } } - private static MethodInfo? FindParseMethod(ParameterInfo parameter) + private static MethodInfo? FindTryParseMethod(ParameterInfo parameter) { var type = Nullable.GetUnderlyingType(parameter.ParameterType) ?? parameter.ParameterType; var methods = type.GetMethods(BindingFlags.Public | BindingFlags.Static); foreach (var method in methods) { - if (method.Name != "Parse" || method.ReturnType != type) + if (method.Name != "TryParse" || method.ReturnType != typeof(bool)) { continue; } - var parameters = method.GetParameters(); + var tryParseParameters = method.GetParameters(); - if (parameters.Length == 1 && parameters[0].ParameterType == typeof(string)) + if (tryParseParameters.Length == 2 && + tryParseParameters[0].ParameterType == typeof(string) && + tryParseParameters[1].IsOut && + tryParseParameters[1].ParameterType == type.MakeByRefType()) { return method; } @@ -477,7 +481,7 @@ private static Expression GetValueFromProperty(Expression sourceExpression, stri return Expression.Convert(getIndex, typeof(string)); } - private static Expression BindParameterFromValue(ParameterInfo parameter, Expression valueExpression, MethodInfo? parseMethod = null) + private static Expression BindParameterFromValue(ParameterInfo parameter, Expression valueExpression, MethodInfo? tryParseMethod = null) { Expression argumentExpression; @@ -487,15 +491,19 @@ private static Expression BindParameterFromValue(ParameterInfo parameter, Expres } else { - parseMethod ??= FindParseMethod(parameter); + tryParseMethod ??= FindTryParseMethod(parameter); - if (parseMethod is null) + if (tryParseMethod is null) { // TODO: Add test! - throw new InvalidOperationException($"No valid static {parameter.ParameterType}.Parse(string) method for {parameter} was found."); + throw new InvalidOperationException($"No public static {parameter.ParameterType}.TryParse(string, out {parameter.ParameterType}) method found for {parameter}."); } - argumentExpression = Expression.Call(parseMethod, valueExpression); + var parsedValue = Expression.Variable(parameter.ParameterType); + + argumentExpression = Expression.Block(new[] { parsedValue }, + Expression.Call(tryParseMethod, valueExpression, parsedValue), + parsedValue); } if (argumentExpression.Type != parameter.ParameterType) @@ -517,11 +525,11 @@ private static Expression BindParameterFromValue(ParameterInfo parameter, Expres private static Expression BindParameterFromProperty(ParameterInfo parameter, MemberExpression property, string key) => BindParameterFromValue(parameter, GetValueFromProperty(property, key)); - private static Expression BindParameterFromRouteValueOrQueryString(ParameterInfo parameter, string key, MethodInfo? parseMethod = null) + private static Expression BindParameterFromRouteValueOrQueryString(ParameterInfo parameter, string key, MethodInfo? tryParseMethod = null) { var routeValue = GetValueFromProperty(RouteValuesProperty, key); var queryValue = GetValueFromProperty(QueryProperty, key); - return BindParameterFromValue(parameter, Expression.Coalesce(routeValue, queryValue), parseMethod); + return BindParameterFromValue(parameter, Expression.Coalesce(routeValue, queryValue), tryParseMethod); } private static ILogger GetLogger(HttpContext httpContext) From bd6087cb8409cff89d9526f7ac8dc1114ea2bdd4 Mon Sep 17 00:00:00 2001 From: Stephen Halter Date: Tue, 6 Apr 2021 17:38:57 -0700 Subject: [PATCH 17/35] Log TryParse Failures --- .../src/RequestDelegateFactory.cs | 64 +++++++++++++------ .../test/RequestDelegateFactoryTests.cs | 34 ++++++++-- 2 files changed, 73 insertions(+), 25 deletions(-) diff --git a/src/Http/Http.Extensions/src/RequestDelegateFactory.cs b/src/Http/Http.Extensions/src/RequestDelegateFactory.cs index 47103b19ae9d..575bbb0e029a 100644 --- a/src/Http/Http.Extensions/src/RequestDelegateFactory.cs +++ b/src/Http/Http.Extensions/src/RequestDelegateFactory.cs @@ -36,6 +36,9 @@ public static class RequestDelegateFactory private static readonly MethodInfo JsonResultWriteResponseAsync = GetMethodInfo>((response, value) => HttpResponseJsonExtensions.WriteAsJsonAsync(response, value, default)); private static readonly MemberInfo CompletedTaskMemberInfo = GetMemberInfo>(() => Task.CompletedTask); + private static readonly MethodInfo LogParameterBindingFailure = GetMethodInfo>((httpContext, parameterType, parameterName, sourceValue) => + Log.ParameterBindingFailed(httpContext, parameterType, parameterName, sourceValue)); + private static readonly ParameterExpression TargetArg = Expression.Parameter(typeof(object), "target"); private static readonly ParameterExpression HttpContextParameter = Expression.Parameter(typeof(HttpContext), "httpContext"); private static readonly ParameterExpression DeserializedBodyArg = Expression.Parameter(typeof(object), "bodyValue"); @@ -397,12 +400,12 @@ private static Expression CreateMethodCallingAndResponseWritingExpression(Expres } catch (IOException ex) { - Log.RequestBodyIOException(GetLogger(httpContext), ex); + Log.RequestBodyIOException(httpContext, ex); return; } catch (InvalidDataException ex) { - Log.RequestBodyInvalidDataException(GetLogger(httpContext), ex); + Log.RequestBodyInvalidDataException(httpContext, ex); httpContext.Response.StatusCode = 400; return; } @@ -426,13 +429,13 @@ private static Expression CreateMethodCallingAndResponseWritingExpression(Expres } catch (IOException ex) { - Log.RequestBodyIOException(GetLogger(httpContext), ex); + Log.RequestBodyIOException(httpContext, ex); httpContext.Abort(); return; } catch (InvalidDataException ex) { - Log.RequestBodyInvalidDataException(GetLogger(httpContext), ex); + Log.RequestBodyInvalidDataException(httpContext, ex); httpContext.Response.StatusCode = 400; return; } @@ -447,12 +450,13 @@ private static Expression CreateMethodCallingAndResponseWritingExpression(Expres } } - private static MethodInfo? FindTryParseMethod(ParameterInfo parameter) + private static MethodInfo? FindTryParseMethod(ParameterInfo parameter, Type? parameterType = null) { - var type = Nullable.GetUnderlyingType(parameter.ParameterType) ?? parameter.ParameterType; - var methods = type.GetMethods(BindingFlags.Public | BindingFlags.Static); + parameterType ??= Nullable.GetUnderlyingType(parameter.ParameterType) ?? parameter.ParameterType; + + var staticMethods = parameterType.GetMethods(BindingFlags.Public | BindingFlags.Static); - foreach (var method in methods) + foreach (var method in staticMethods) { if (method.Name != "TryParse" || method.ReturnType != typeof(bool)) { @@ -464,7 +468,7 @@ private static Expression CreateMethodCallingAndResponseWritingExpression(Expres if (tryParseParameters.Length == 2 && tryParseParameters[0].ParameterType == typeof(string) && tryParseParameters[1].IsOut && - tryParseParameters[1].ParameterType == type.MakeByRefType()) + tryParseParameters[1].ParameterType == parameterType.MakeByRefType()) { return method; } @@ -483,6 +487,7 @@ private static Expression GetValueFromProperty(Expression sourceExpression, stri private static Expression BindParameterFromValue(ParameterInfo parameter, Expression valueExpression, MethodInfo? tryParseMethod = null) { + var parameterType = Nullable.GetUnderlyingType(parameter.ParameterType) ?? parameter.ParameterType; Expression argumentExpression; if (parameter.ParameterType == typeof(string)) @@ -491,7 +496,7 @@ private static Expression BindParameterFromValue(ParameterInfo parameter, Expres } else { - tryParseMethod ??= FindTryParseMethod(parameter); + tryParseMethod ??= FindTryParseMethod(parameter, parameterType); if (tryParseMethod is null) { @@ -501,8 +506,16 @@ private static Expression BindParameterFromValue(ParameterInfo parameter, Expres var parsedValue = Expression.Variable(parameter.ParameterType); + var tryParseCall = Expression.Call(tryParseMethod, valueExpression, parsedValue); + + var parameterTypeConstant = Expression.Constant(parameterType.Name); + var parameterNameConstant = Expression.Constant(parameter.Name); + + var ifExpression = Expression.IfThen(Expression.Not(tryParseCall), + Expression.Call(LogParameterBindingFailure, HttpContextParameter, parameterTypeConstant, parameterNameConstant, valueExpression)); + argumentExpression = Expression.Block(new[] { parsedValue }, - Expression.Call(tryParseMethod, valueExpression, parsedValue), + ifExpression, parsedValue); } @@ -532,10 +545,9 @@ private static Expression BindParameterFromRouteValueOrQueryString(ParameterInfo return BindParameterFromValue(parameter, Expression.Coalesce(routeValue, queryValue), tryParseMethod); } - private static ILogger GetLogger(HttpContext httpContext) + private static void LogTryParseFailure(HttpContext httpContext, string parameterName) { - var loggerFactory = httpContext.RequestServices.GetRequiredService(); - return loggerFactory.CreateLogger(typeof(RequestDelegateFactory)); + } private static MethodInfo GetMethodInfo(Expression expr) @@ -677,14 +689,30 @@ private static class Log new EventId(2, "RequestBodyInvalidDataException"), "Reading the request body failed with an InvalidDataException."); - public static void RequestBodyIOException(ILogger logger, IOException exception) + private static readonly Action _parameterBindingFailed = LoggerMessage.Define( + LogLevel.Debug, + new EventId(3, "ParamaterBindingFailed"), + @"Failed to bind parameter ""{ParameterType} {ParameterName}"" from ""{SourceValue}""."); + + public static void RequestBodyIOException(HttpContext httpContext, IOException exception) + { + _requestBodyIOException(GetLogger(httpContext), exception); + } + + public static void RequestBodyInvalidDataException(HttpContext httpContext, InvalidDataException exception) + { + _requestBodyInvalidDataException(GetLogger(httpContext), exception); + } + + public static void ParameterBindingFailed(HttpContext httpContext, string parameterType, string parameterName, string sourceValue) { - _requestBodyIOException(logger, exception); + _parameterBindingFailed(GetLogger(httpContext), parameterType, parameterName, sourceValue, null); } - public static void RequestBodyInvalidDataException(ILogger logger, InvalidDataException exception) + private static ILogger GetLogger(HttpContext httpContext) { - _requestBodyInvalidDataException(logger, exception); + var loggerFactory = httpContext.RequestServices.GetRequiredService(); + return loggerFactory.CreateLogger(typeof(RequestDelegateFactory)); } } } diff --git a/src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs b/src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs index 86b6f23b74e2..427140a99778 100644 --- a/src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs +++ b/src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs @@ -371,21 +371,41 @@ public async Task RequestDelegatePopulatesUnattributedTryParseableParametersFrom Assert.Equal(expectedParameterValue, httpContext.Items["tryParsable"]); } - [Theory] - [MemberData(nameof(FromTryParsableParameter))] - public async Task RequestDelegateLogsTryParsableFailuresAsDebugAndSets400Response(Delegate action, string routeValue, object expectedParameterValue) + [Fact] + public async Task RequestDelegateLogsTryParsableFailuresAsDebugAndSets400Response() { - // disabling error xUnit1026 the "right" way looks ugly - var ignore = routeValue; + var invoked = false; + + var sink = new TestSink(context => context.LoggerName == "Microsoft.AspNetCore.Http.RequestDelegateFactory"); + var testLoggerFactory = new TestLoggerFactory(sink, enabled: true); + + void TestAction([FromRoute] int tryParsable) + { + invoked = true; + } + + var invalidDataException = new InvalidDataException(); + var serviceCollection = new ServiceCollection(); + serviceCollection.AddSingleton(testLoggerFactory); var httpContext = new DefaultHttpContext(); httpContext.Request.RouteValues["tryParsable"] = "invalid!"; + httpContext.Features.Set(new TestHttpRequestLifetimeFeature()); + httpContext.RequestServices = serviceCollection.BuildServiceProvider(); - var requestDelegate = RequestDelegateFactory.Create(action); + var requestDelegate = RequestDelegateFactory.Create((Action)TestAction); await requestDelegate(httpContext); - Assert.Equal(expectedParameterValue, httpContext.Items["tryParsable"]); + //Assert.False(invoked); + //Assert.False(httpContext.RequestAborted.IsCancellationRequested); + //Assert.Equal(400, httpContext.Response.StatusCode); + Assert.True(invoked); + + var log = Assert.Single(sink.Writes); + Assert.Equal(new EventId(3, "ParamaterBindingFailed"), log.EventId); + Assert.Equal(LogLevel.Debug, log.LogLevel); + Assert.Equal(@"Failed to bind parameter ""Int32 tryParsable"" from ""invalid!"".", log.Message); } [Fact] From 1a0609d92d098cb760afdf9a3ff14f437e6129bd Mon Sep 17 00:00:00 2001 From: Stephen Halter Date: Tue, 6 Apr 2021 19:28:50 -0700 Subject: [PATCH 18/35] Add WasTryParseFailureVariable - fix tests --- .../src/RequestDelegateFactory.cs | 177 +++++++++++------- 1 file changed, 109 insertions(+), 68 deletions(-) diff --git a/src/Http/Http.Extensions/src/RequestDelegateFactory.cs b/src/Http/Http.Extensions/src/RequestDelegateFactory.cs index 575bbb0e029a..e7994ca16015 100644 --- a/src/Http/Http.Extensions/src/RequestDelegateFactory.cs +++ b/src/Http/Http.Extensions/src/RequestDelegateFactory.cs @@ -42,6 +42,8 @@ public static class RequestDelegateFactory private static readonly ParameterExpression TargetArg = Expression.Parameter(typeof(object), "target"); private static readonly ParameterExpression HttpContextParameter = Expression.Parameter(typeof(HttpContext), "httpContext"); private static readonly ParameterExpression DeserializedBodyArg = Expression.Parameter(typeof(object), "bodyValue"); + private static readonly ParameterExpression WasTryParseFailureVariable = Expression.Variable(typeof(bool), "wasTryParseFailureVariable"); + private static readonly ParameterExpression WasTryParseFailureParameter = Expression.Parameter(typeof(bool), "wasTryParseFailureParameter"); private static readonly MemberExpression RequestServicesExpr = Expression.Property(HttpContextParameter, nameof(HttpContext.RequestServices)); private static readonly MemberExpression HttpRequestExpr = Expression.Property(HttpContextParameter, nameof(HttpContext.Request)); @@ -70,11 +72,11 @@ public static RequestDelegate Create(Delegate action) null => null, }; - var targettableRequestDelegate = CreateTargettableRequestDelegate(action.Method, targetExpression); + var targetableRequestDelegate = CreateTargetableRequestDelegate(action.Method, targetExpression); return httpContext => { - return targettableRequestDelegate(action.Target, httpContext); + return targetableRequestDelegate(action.Target, httpContext); }; } @@ -90,11 +92,11 @@ public static RequestDelegate Create(MethodInfo methodInfo) throw new ArgumentNullException(nameof(methodInfo)); } - var targettableRequestDelegate = CreateTargettableRequestDelegate(methodInfo, targetExpression: null); + var targetableRequestDelegate = CreateTargetableRequestDelegate(methodInfo, targetExpression: null); return httpContext => { - return targettableRequestDelegate(null, httpContext); + return targetableRequestDelegate(null, httpContext); }; } @@ -122,15 +124,15 @@ public static RequestDelegate Create(MethodInfo methodInfo, Func { - return targettableRequestDelegate(targetFactory(httpContext), httpContext); + return targetableRequestDelegate(targetFactory(httpContext), httpContext); }; } - private static Func CreateTargettableRequestDelegate(MethodInfo methodInfo, Expression? targetExpression) + private static Func CreateTargetableRequestDelegate(MethodInfo methodInfo, Expression? targetExpression) { // Non void return type @@ -148,21 +150,19 @@ public static RequestDelegate Create(MethodInfo methodInfo, Func(); @@ -172,13 +172,13 @@ private static Expression[] CreateArgumentExpresssions(ParameterInfo[]? paramete for (var i = 0; i < parameters.Length; i++) { - args[i] = CreateArgumentExpression(parameters[i], requestBodyContext); + args[i] = CreateArgumentExpression(parameters[i], factoryContext); } return args; } - private static Expression CreateArgumentExpression(ParameterInfo parameter, RequestBodyContext requestBodyContext) + private static Expression CreateArgumentExpression(ParameterInfo parameter, FactoryContext factoryContext) { if (parameter.Name is null) { @@ -190,44 +190,44 @@ private static Expression CreateArgumentExpression(ParameterInfo parameter, Requ if (parameterCustomAttributes.OfType().FirstOrDefault() is { } routeAttribute) { - return BindParameterFromProperty(parameter, RouteValuesProperty, routeAttribute.Name ?? parameter.Name); + return BindParameterFromProperty(parameter, RouteValuesProperty, routeAttribute.Name ?? parameter.Name, factoryContext); } else if (parameterCustomAttributes.OfType().FirstOrDefault() is { } queryAttribute) { - return BindParameterFromProperty(parameter, QueryProperty, queryAttribute.Name ?? parameter.Name); + return BindParameterFromProperty(parameter, QueryProperty, queryAttribute.Name ?? parameter.Name, factoryContext); } else if (parameterCustomAttributes.OfType().FirstOrDefault() is { } headerAttribute) { - return BindParameterFromProperty(parameter, HeadersProperty, headerAttribute.Name ?? parameter.Name); + return BindParameterFromProperty(parameter, HeadersProperty, headerAttribute.Name ?? parameter.Name, factoryContext); } else if (parameterCustomAttributes.OfType().FirstOrDefault() is { } bodyAttribute) { - if (requestBodyContext.Mode is RequestBodyMode.AsJson) + if (factoryContext.RequestBodyMode is RequestBodyMode.AsJson) { throw new InvalidOperationException("Action cannot have more than one FromBody attribute."); } - if (requestBodyContext.Mode is RequestBodyMode.AsForm) + if (factoryContext.RequestBodyMode is RequestBodyMode.AsForm) { ThrowCannotReadBodyDirectlyAndAsForm(); } - requestBodyContext.Mode = RequestBodyMode.AsJson; - requestBodyContext.JsonBodyType = parameter.ParameterType; - requestBodyContext.AllowEmptyBody = bodyAttribute.AllowEmpty; + factoryContext.RequestBodyMode = RequestBodyMode.AsJson; + factoryContext.JsonRequestBodyType = parameter.ParameterType; + factoryContext.AllowEmptyRequestBody = bodyAttribute.AllowEmpty; return Expression.Convert(DeserializedBodyArg, parameter.ParameterType); } else if (parameterCustomAttributes.OfType().FirstOrDefault() is { } formAttribute) { - if (requestBodyContext.Mode is RequestBodyMode.AsJson) + if (factoryContext.RequestBodyMode is RequestBodyMode.AsJson) { ThrowCannotReadBodyDirectlyAndAsForm(); } - requestBodyContext.Mode = RequestBodyMode.AsForm; + factoryContext.RequestBodyMode = RequestBodyMode.AsForm; - return BindParameterFromProperty(parameter, FormProperty, formAttribute.Name ?? parameter.Name); + return BindParameterFromProperty(parameter, FormProperty, formAttribute.Name ?? parameter.Name, factoryContext); } else if (parameter.CustomAttributes.Any(a => typeof(IFromServiceMetadata).IsAssignableFrom(a.AttributeType))) { @@ -235,12 +235,12 @@ private static Expression CreateArgumentExpression(ParameterInfo parameter, Requ } else if (parameter.ParameterType == typeof(IFormCollection)) { - if (requestBodyContext.Mode is RequestBodyMode.AsJson) + if (factoryContext.RequestBodyMode is RequestBodyMode.AsJson) { ThrowCannotReadBodyDirectlyAndAsForm(); } - requestBodyContext.Mode = RequestBodyMode.AsForm; + factoryContext.RequestBodyMode = RequestBodyMode.AsForm; return Expression.Property(HttpRequestExpr, nameof(HttpRequest.Form)); } @@ -254,11 +254,11 @@ private static Expression CreateArgumentExpression(ParameterInfo parameter, Requ } else if (parameter.ParameterType == typeof(string)) { - return BindParameterFromRouteValueOrQueryString(parameter, parameter.Name); + return BindParameterFromRouteValueOrQueryString(parameter, parameter.Name, factoryContext); } - else if (FindTryParseMethod(parameter) is { } parseMethod) + else if (HasTryParseMethod(parameter)) { - return BindParameterFromRouteValueOrQueryString(parameter, parameter.Name, parseMethod); + return BindParameterFromRouteValueOrQueryString(parameter, parameter.Name, factoryContext); } else { @@ -266,7 +266,43 @@ private static Expression CreateArgumentExpression(ParameterInfo parameter, Requ } } - private static Expression CreateMethodCallingAndResponseWritingExpression(Expression methodCall, Type returnType) + private static Expression CreateMethodCallExpression(MethodInfo methodInfo, Expression? target, Expression[] arguments, FactoryContext factoryContext) + { + Expression CreateInnerMethodCall(Expression[] innerArguments) + { + return target is null ? + Expression.Call(methodInfo, innerArguments) : + Expression.Call(target, methodInfo, innerArguments); + } + + if (!factoryContext.CheckForTryParseFailure) + { + return CreateInnerMethodCall(arguments); + } + + // If we're calling TryParse and the WasTryParseFailureVariable indicates it failed, set a 400 StatusCode instead of calling the method. + var parameters = methodInfo.GetParameters(); + var lambdaParameters = new ParameterExpression[parameters.Length + 1]; + + for (var i = 0; i < parameters.Length; i++) + { + lambdaParameters[i] = Expression.Parameter(parameters[i].ParameterType); + } + + lambdaParameters[parameters.Length] = WasTryParseFailureParameter; + + var lambdaArgs = new Expression[lambdaParameters.Length]; + Array.Copy(arguments, lambdaArgs, parameters.Length); + lambdaArgs[parameters.Length] = WasTryParseFailureVariable; + + var innerCall = CreateInnerMethodCall(lambdaParameters.AsMemory(0, parameters.Length).ToArray()); + var checkTryParseBlock = innerCall; + + var lambda = Expression.Lambda(checkTryParseBlock, lambdaParameters); + return Expression.Block(new[] { WasTryParseFailureVariable }, Expression.Invoke(lambda, lambdaArgs)); + } + + private static Expression CreateResponseWritingExpression(Expression methodCall, Type returnType) { // Exact request delegate match if (returnType == typeof(void)) @@ -368,18 +404,18 @@ private static Expression CreateMethodCallingAndResponseWritingExpression(Expres } } - private static Func HandleRequestBodyAndCompileRequestDelegate(Expression body, RequestBodyContext requestBodyContext) + private static Func HandleRequestBodyAndCompileRequestDelegate(Expression callMethodAndWriteResponse, FactoryContext factoryContext) { - if (requestBodyContext.Mode is RequestBodyMode.AsJson) + if (factoryContext.RequestBodyMode is RequestBodyMode.AsJson) { // We need to generate the code for reading from the body before calling into the delegate var invoker = Expression.Lambda>( - body, TargetArg, HttpContextParameter, DeserializedBodyArg).Compile(); + callMethodAndWriteResponse, TargetArg, HttpContextParameter, DeserializedBodyArg).Compile(); - var bodyType = requestBodyContext.JsonBodyType!; + var bodyType = factoryContext.JsonRequestBodyType!; object? defaultBodyValue = null; - if (requestBodyContext.AllowEmptyBody && bodyType.IsValueType) + if (factoryContext.AllowEmptyRequestBody && bodyType.IsValueType) { defaultBodyValue = Activator.CreateInstance(bodyType); } @@ -388,7 +424,7 @@ private static Expression CreateMethodCallingAndResponseWritingExpression(Expres { object? bodyValue; - if (requestBodyContext.AllowEmptyBody && httpContext.Request.ContentLength == 0) + if (factoryContext.AllowEmptyRequestBody && httpContext.Request.ContentLength == 0) { bodyValue = defaultBodyValue; } @@ -414,10 +450,10 @@ private static Expression CreateMethodCallingAndResponseWritingExpression(Expres await invoker(target, httpContext, bodyValue); }; } - else if (requestBodyContext.Mode is RequestBodyMode.AsForm) + else if (factoryContext.RequestBodyMode is RequestBodyMode.AsForm) { var invoker = Expression.Lambda>( - body, TargetArg, HttpContextParameter).Compile(); + callMethodAndWriteResponse, TargetArg, HttpContextParameter).Compile(); return async (target, httpContext) => { @@ -446,15 +482,14 @@ private static Expression CreateMethodCallingAndResponseWritingExpression(Expres else { return Expression.Lambda>( - body, TargetArg, HttpContextParameter).Compile(); + callMethodAndWriteResponse, TargetArg, HttpContextParameter).Compile(); } } - private static MethodInfo? FindTryParseMethod(ParameterInfo parameter, Type? parameterType = null) + // Todo: Cache this. + private static MethodInfo? FindTryParseMethod(Type type) { - parameterType ??= Nullable.GetUnderlyingType(parameter.ParameterType) ?? parameter.ParameterType; - - var staticMethods = parameterType.GetMethods(BindingFlags.Public | BindingFlags.Static); + var staticMethods = type.GetMethods(BindingFlags.Public | BindingFlags.Static); foreach (var method in staticMethods) { @@ -468,7 +503,7 @@ private static Expression CreateMethodCallingAndResponseWritingExpression(Expres if (tryParseParameters.Length == 2 && tryParseParameters[0].ParameterType == typeof(string) && tryParseParameters[1].IsOut && - tryParseParameters[1].ParameterType == parameterType.MakeByRefType()) + tryParseParameters[1].ParameterType == type.MakeByRefType()) { return method; } @@ -477,6 +512,12 @@ private static Expression CreateMethodCallingAndResponseWritingExpression(Expres return null; } + private static bool HasTryParseMethod(ParameterInfo parameter) + { + var parameterType = Nullable.GetUnderlyingType(parameter.ParameterType) ?? parameter.ParameterType; + return FindTryParseMethod(parameterType) is not null; + } + private static Expression GetValueFromProperty(Expression sourceExpression, string key) { var itemProperty = sourceExpression.Type.GetProperty("Item"); @@ -485,9 +526,8 @@ private static Expression GetValueFromProperty(Expression sourceExpression, stri return Expression.Convert(getIndex, typeof(string)); } - private static Expression BindParameterFromValue(ParameterInfo parameter, Expression valueExpression, MethodInfo? tryParseMethod = null) + private static Expression BindParameterFromValue(ParameterInfo parameter, Expression valueExpression, FactoryContext factoryContext) { - var parameterType = Nullable.GetUnderlyingType(parameter.ParameterType) ?? parameter.ParameterType; Expression argumentExpression; if (parameter.ParameterType == typeof(string)) @@ -496,7 +536,8 @@ private static Expression BindParameterFromValue(ParameterInfo parameter, Expres } else { - tryParseMethod ??= FindTryParseMethod(parameter, parameterType); + var parameterType = Nullable.GetUnderlyingType(parameter.ParameterType) ?? parameter.ParameterType; + var tryParseMethod = FindTryParseMethod(parameterType); if (tryParseMethod is null) { @@ -504,18 +545,22 @@ private static Expression BindParameterFromValue(ParameterInfo parameter, Expres throw new InvalidOperationException($"No public static {parameter.ParameterType}.TryParse(string, out {parameter.ParameterType}) method found for {parameter}."); } + factoryContext.CheckForTryParseFailure = true; + var parsedValue = Expression.Variable(parameter.ParameterType); var tryParseCall = Expression.Call(tryParseMethod, valueExpression, parsedValue); var parameterTypeConstant = Expression.Constant(parameterType.Name); var parameterNameConstant = Expression.Constant(parameter.Name); - - var ifExpression = Expression.IfThen(Expression.Not(tryParseCall), + var failBlock = Expression.Block( + Expression.Assign(WasTryParseFailureVariable, Expression.Constant(true)), Expression.Call(LogParameterBindingFailure, HttpContextParameter, parameterTypeConstant, parameterNameConstant, valueExpression)); + var ifFailExpression = Expression.IfThen(Expression.Not(tryParseCall), failBlock); + argumentExpression = Expression.Block(new[] { parsedValue }, - ifExpression, + ifFailExpression, parsedValue); } @@ -535,19 +580,14 @@ private static Expression BindParameterFromValue(ParameterInfo parameter, Expres argumentExpression); } - private static Expression BindParameterFromProperty(ParameterInfo parameter, MemberExpression property, string key) => - BindParameterFromValue(parameter, GetValueFromProperty(property, key)); + private static Expression BindParameterFromProperty(ParameterInfo parameter, MemberExpression property, string key, FactoryContext factoryContext) => + BindParameterFromValue(parameter, GetValueFromProperty(property, key), factoryContext); - private static Expression BindParameterFromRouteValueOrQueryString(ParameterInfo parameter, string key, MethodInfo? tryParseMethod = null) + private static Expression BindParameterFromRouteValueOrQueryString(ParameterInfo parameter, string key, FactoryContext factoryContext) { var routeValue = GetValueFromProperty(RouteValuesProperty, key); var queryValue = GetValueFromProperty(QueryProperty, key); - return BindParameterFromValue(parameter, Expression.Coalesce(routeValue, queryValue), tryParseMethod); - } - - private static void LogTryParseFailure(HttpContext httpContext, string parameterName) - { - + return BindParameterFromValue(parameter, Expression.Coalesce(routeValue, queryValue), factoryContext); } private static MethodInfo GetMethodInfo(Expression expr) @@ -663,11 +703,12 @@ private static void ThrowCannotReadBodyDirectlyAndAsForm() throw new InvalidOperationException("Action cannot mix FromBody and FromForm on the same method."); } - private class RequestBodyContext + private class FactoryContext { - public RequestBodyMode Mode { get; set; } - public Type? JsonBodyType { get; set; } - public bool AllowEmptyBody { get; set; } + public RequestBodyMode RequestBodyMode { get; set; } + public Type? JsonRequestBodyType { get; set; } + public bool AllowEmptyRequestBody { get; set; } + public bool CheckForTryParseFailure { get; set; } } private enum RequestBodyMode From 6781c2ecc64f08ddb1fa3d964f885aac7fe66d2c Mon Sep 17 00:00:00 2001 From: Stephen Halter Date: Tue, 6 Apr 2021 20:38:01 -0700 Subject: [PATCH 19/35] Add comments --- .../src/RequestDelegateFactory.cs | 89 +++++++++++++++---- 1 file changed, 71 insertions(+), 18 deletions(-) diff --git a/src/Http/Http.Extensions/src/RequestDelegateFactory.cs b/src/Http/Http.Extensions/src/RequestDelegateFactory.cs index e7994ca16015..c85c79122546 100644 --- a/src/Http/Http.Extensions/src/RequestDelegateFactory.cs +++ b/src/Http/Http.Extensions/src/RequestDelegateFactory.cs @@ -41,9 +41,10 @@ public static class RequestDelegateFactory private static readonly ParameterExpression TargetArg = Expression.Parameter(typeof(object), "target"); private static readonly ParameterExpression HttpContextParameter = Expression.Parameter(typeof(HttpContext), "httpContext"); - private static readonly ParameterExpression DeserializedBodyArg = Expression.Parameter(typeof(object), "bodyValue"); - private static readonly ParameterExpression WasTryParseFailureVariable = Expression.Variable(typeof(bool), "wasTryParseFailureVariable"); + + private static readonly ParameterExpression DeserializedBodyParameter = Expression.Parameter(typeof(object), "bodyParameter"); private static readonly ParameterExpression WasTryParseFailureParameter = Expression.Parameter(typeof(bool), "wasTryParseFailureParameter"); + private static readonly ParameterExpression WasTryParseFailureVariable = Expression.Variable(typeof(bool), "wasTryParseFailureVariable"); private static readonly MemberExpression RequestServicesExpr = Expression.Property(HttpContextParameter, nameof(HttpContext.RequestServices)); private static readonly MemberExpression HttpRequestExpr = Expression.Property(HttpContextParameter, nameof(HttpContext.Request)); @@ -216,7 +217,7 @@ private static Expression CreateArgumentExpression(ParameterInfo parameter, Fact factoryContext.JsonRequestBodyType = parameter.ParameterType; factoryContext.AllowEmptyRequestBody = bodyAttribute.AllowEmpty; - return Expression.Convert(DeserializedBodyArg, parameter.ParameterType); + return Expression.Convert(DeserializedBodyParameter, parameter.ParameterType); } else if (parameterCustomAttributes.OfType().FirstOrDefault() is { } formAttribute) { @@ -266,21 +267,55 @@ private static Expression CreateArgumentExpression(ParameterInfo parameter, Fact } } - private static Expression CreateMethodCallExpression(MethodInfo methodInfo, Expression? target, Expression[] arguments, FactoryContext factoryContext) - { - Expression CreateInnerMethodCall(Expression[] innerArguments) - { - return target is null ? - Expression.Call(methodInfo, innerArguments) : - Expression.Call(target, methodInfo, innerArguments); - } + private static Expression CreateMethodCallExpression(MethodInfo methodInfo, Expression? target, Expression[] arguments, FactoryContext factoryContext) => + factoryContext.CheckForTryParseFailure ? + CreateTryParseCheckingMethodCallExpression(methodInfo, target, arguments) : + CreateInnerMethodCallExpression(methodInfo, target, arguments); - if (!factoryContext.CheckForTryParseFailure) - { - return CreateInnerMethodCall(arguments); - } + private static Expression CreateInnerMethodCallExpression(MethodInfo methodInfo, Expression? target, Expression[] arguments) => + target is null ? + Expression.Call(methodInfo, arguments) : + Expression.Call(target, methodInfo, arguments); + + // If we're calling TryParse and the WasTryParseFailureVariable indicates it failed, set a 400 StatusCode instead of calling the method. + private static Expression CreateTryParseCheckingMethodCallExpression(MethodInfo methodInfo, Expression? target, Expression[] arguments) + { + // { + // bool wasTryParseFailureVariable = false; + // + // // Assume "id" is the first parameter. There could be more. + // int ParseIdFromRouteValue() + // { + // var sourceValue = httpContext.Request.RouteValue["id"]; + // int parsedValue = default; + // + // if (!int.TryParse(sourceValue, out parsedValue)) + // { + // Log.ParameterBindingFailed(httpContext, "int", "id", sourceValue) + // wasTryParseFailureVariable = true; + // } + // + // return parsedValue; + // } + // + // // More Parse methods... + // + // // "Lambda" allows us to check for failure after evaluating parameters but before invoking the action + // // without a heap allocation by inserting a stack frame in-between. + // static TResult Lambda(int id, ..., bool wasTryParseFailureParameter) + // { + // if (wasTryParseFailureParameter) + // { + // httpContext.Response.StatusCode = 400; + // return default; + // } + // + // return action(id, ...); + // } + // + // return Lambda(ParseIdFromRouteValue(), ..., wasTryParseFailureVariable); + // } - // If we're calling TryParse and the WasTryParseFailureVariable indicates it failed, set a 400 StatusCode instead of calling the method. var parameters = methodInfo.GetParameters(); var lambdaParameters = new ParameterExpression[parameters.Length + 1]; @@ -295,7 +330,9 @@ Expression CreateInnerMethodCall(Expression[] innerArguments) Array.Copy(arguments, lambdaArgs, parameters.Length); lambdaArgs[parameters.Length] = WasTryParseFailureVariable; - var innerCall = CreateInnerMethodCall(lambdaParameters.AsMemory(0, parameters.Length).ToArray()); + var passthroughArgs = lambdaParameters.AsMemory(0, parameters.Length).ToArray(); + var innerCall = CreateInnerMethodCallExpression(methodInfo, target, passthroughArgs); + var checkTryParseBlock = innerCall; var lambda = Expression.Lambda(checkTryParseBlock, lambdaParameters); @@ -410,7 +447,7 @@ private static Expression CreateResponseWritingExpression(Expression methodCall, { // We need to generate the code for reading from the body before calling into the delegate var invoker = Expression.Lambda>( - callMethodAndWriteResponse, TargetArg, HttpContextParameter, DeserializedBodyArg).Compile(); + callMethodAndWriteResponse, TargetArg, HttpContextParameter, DeserializedBodyParameter).Compile(); var bodyType = factoryContext.JsonRequestBodyType!; object? defaultBodyValue = null; @@ -545,6 +582,22 @@ private static Expression BindParameterFromValue(ParameterInfo parameter, Expres throw new InvalidOperationException($"No public static {parameter.ParameterType}.TryParse(string, out {parameter.ParameterType}) method found for {parameter}."); } + // bool wasTryParseFailureVariable = false; + // + // int ParseIdFromRouteValue() + // { + // var sourceValue = httpContext.Request.RouteValue["id"]; + // int parsedValue = default; + // + // if (!int.TryParse(sourceValue, out parsedValue)) + // { + // Log.ParameterBindingFailed(httpContext, "int", "id", sourceValue) + // wasTryParseFailureVariable = true; + // } + // + // return parsedValue; + // } + factoryContext.CheckForTryParseFailure = true; var parsedValue = Expression.Variable(parameter.ParameterType); From 37823bd83eb63e4ed0a7814f1d7268bb65df19d4 Mon Sep 17 00:00:00 2001 From: Stephen Halter Date: Tue, 6 Apr 2021 22:38:23 -0700 Subject: [PATCH 20/35] Set 400 StatusCode for TryParse failure --- .../src/RequestDelegateFactory.cs | 143 +++++++++--------- .../test/RequestDelegateFactoryTests.cs | 7 +- 2 files changed, 78 insertions(+), 72 deletions(-) diff --git a/src/Http/Http.Extensions/src/RequestDelegateFactory.cs b/src/Http/Http.Extensions/src/RequestDelegateFactory.cs index c85c79122546..de516f2761b5 100644 --- a/src/Http/Http.Extensions/src/RequestDelegateFactory.cs +++ b/src/Http/Http.Extensions/src/RequestDelegateFactory.cs @@ -34,7 +34,8 @@ public static class RequestDelegateFactory private static readonly MethodInfo ResultWriteResponseAsync = typeof(IResult).GetMethod(nameof(IResult.ExecuteAsync), BindingFlags.Public | BindingFlags.Instance)!; private static readonly MethodInfo StringResultWriteResponseAsync = GetMethodInfo>((response, text) => HttpResponseWritingExtensions.WriteAsync(response, text, default)); private static readonly MethodInfo JsonResultWriteResponseAsync = GetMethodInfo>((response, value) => HttpResponseJsonExtensions.WriteAsJsonAsync(response, value, default)); - private static readonly MemberInfo CompletedTaskMemberInfo = GetMemberInfo>(() => Task.CompletedTask); + + private static readonly PropertyInfo CompletedTaskPropertyInfo = (PropertyInfo)GetMemberInfo>(() => Task.CompletedTask); private static readonly MethodInfo LogParameterBindingFailure = GetMethodInfo>((httpContext, parameterType, parameterName, sourceValue) => Log.ParameterBindingFailed(httpContext, parameterType, parameterName, sourceValue)); @@ -42,9 +43,8 @@ public static class RequestDelegateFactory private static readonly ParameterExpression TargetArg = Expression.Parameter(typeof(object), "target"); private static readonly ParameterExpression HttpContextParameter = Expression.Parameter(typeof(HttpContext), "httpContext"); - private static readonly ParameterExpression DeserializedBodyParameter = Expression.Parameter(typeof(object), "bodyParameter"); - private static readonly ParameterExpression WasTryParseFailureParameter = Expression.Parameter(typeof(bool), "wasTryParseFailureParameter"); - private static readonly ParameterExpression WasTryParseFailureVariable = Expression.Variable(typeof(bool), "wasTryParseFailureVariable"); + private static readonly ParameterExpression DeserializedBodyParameter = Expression.Parameter(typeof(object), "bodyValue"); + private static readonly ParameterExpression WasTryParseFailureVariable = Expression.Variable(typeof(bool), "wasTryParseFailure"); private static readonly MemberExpression RequestServicesExpr = Expression.Property(HttpContextParameter, nameof(HttpContext.RequestServices)); private static readonly MemberExpression HttpRequestExpr = Expression.Property(HttpContextParameter, nameof(HttpContext.Request)); @@ -54,6 +54,7 @@ public static class RequestDelegateFactory private static readonly MemberExpression QueryProperty = Expression.Property(HttpRequestExpr, nameof(HttpRequest.Query)); private static readonly MemberExpression HeadersProperty = Expression.Property(HttpRequestExpr, nameof(HttpRequest.Headers)); private static readonly MemberExpression FormProperty = Expression.Property(HttpRequestExpr, nameof(HttpRequest.Form)); + private static readonly MemberExpression StatusCodeProperty = Expression.Property(HttpResponseExpr, nameof(HttpResponse.StatusCode)); /// /// Creates a implementation for . @@ -153,16 +154,16 @@ public static RequestDelegate Create(MethodInfo methodInfo, Func - factoryContext.CheckForTryParseFailure ? - CreateTryParseCheckingMethodCallExpression(methodInfo, target, arguments) : - CreateInnerMethodCallExpression(methodInfo, target, arguments); - - private static Expression CreateInnerMethodCallExpression(MethodInfo methodInfo, Expression? target, Expression[] arguments) => + private static Expression CreateMethodCall(MethodInfo methodInfo, Expression? target, Expression[] arguments) => target is null ? Expression.Call(methodInfo, arguments) : Expression.Call(target, methodInfo, arguments); + private static Expression CreateResponseWritingMethodCall(MethodInfo methodInfo, Expression? target, Expression[] arguments) + { + var callMethod = CreateMethodCall(methodInfo, target, arguments); + return AddResponseWritingToMethodCall(callMethod, methodInfo.ReturnType); + } + // If we're calling TryParse and the WasTryParseFailureVariable indicates it failed, set a 400 StatusCode instead of calling the method. - private static Expression CreateTryParseCheckingMethodCallExpression(MethodInfo methodInfo, Expression? target, Expression[] arguments) + private static Expression CreateTryParseCheckingResponseWritingMethodCall(MethodInfo methodInfo, Expression? target, Expression[] arguments) { // { - // bool wasTryParseFailureVariable = false; + // bool wasTryParseFailure = false; // - // // Assume "id" is the first parameter. There could be more. - // int ParseIdFromRouteValue() + // // Assume "int id" is the first parameter. + // int param1 = // { // var sourceValue = httpContext.Request.RouteValue["id"]; // int parsedValue = default; @@ -292,61 +294,63 @@ private static Expression CreateTryParseCheckingMethodCallExpression(MethodInfo // if (!int.TryParse(sourceValue, out parsedValue)) // { // Log.ParameterBindingFailed(httpContext, "int", "id", sourceValue) - // wasTryParseFailureVariable = true; + // wasTryParseFailure = true; // } // // return parsedValue; - // } + // }; + // // ... // - // // More Parse methods... - // - // // "Lambda" allows us to check for failure after evaluating parameters but before invoking the action - // // without a heap allocation by inserting a stack frame in-between. - // static TResult Lambda(int id, ..., bool wasTryParseFailureParameter) - // { - // if (wasTryParseFailureParameter) + // return wasTryParseFailure ? // { - // httpContext.Response.StatusCode = 400; - // return default; - // } - // - // return action(id, ...); - // } - // - // return Lambda(ParseIdFromRouteValue(), ..., wasTryParseFailureVariable); + // httpContext.Response.StatusCode = 400; + // return Task.CompletedTask; + // } : + // { + // // Logic generated by AddResponseWritingToMethodCall() that calls action(param1, ...) + // }; // } var parameters = methodInfo.GetParameters(); - var lambdaParameters = new ParameterExpression[parameters.Length + 1]; + var storedArguments = new ParameterExpression[parameters.Length]; + var localVariables = new ParameterExpression[parameters.Length + 1]; + + for (var i = 0; i < parameters.Length; i++) + { + storedArguments[i] = localVariables[i] = Expression.Parameter(parameters[i].ParameterType); + } + + localVariables[parameters.Length] = WasTryParseFailureVariable; + var assignAndCall = new Expression[parameters.Length + 1]; for (var i = 0; i < parameters.Length; i++) { - lambdaParameters[i] = Expression.Parameter(parameters[i].ParameterType); + assignAndCall[i] = Expression.Assign(localVariables[i], arguments[i]); } - lambdaParameters[parameters.Length] = WasTryParseFailureParameter; + var set400StatusAndReturnCompletedTask = Expression.Block( + Expression.Assign(StatusCodeProperty, Expression.Constant(400)), + Expression.Property(null, CompletedTaskPropertyInfo)); - var lambdaArgs = new Expression[lambdaParameters.Length]; - Array.Copy(arguments, lambdaArgs, parameters.Length); - lambdaArgs[parameters.Length] = WasTryParseFailureVariable; + var methodCall = CreateMethodCall(methodInfo, target, storedArguments); - var passthroughArgs = lambdaParameters.AsMemory(0, parameters.Length).ToArray(); - var innerCall = CreateInnerMethodCallExpression(methodInfo, target, passthroughArgs); + var checkWasTryParseFailure = Expression.Condition(WasTryParseFailureVariable, + set400StatusAndReturnCompletedTask, + AddResponseWritingToMethodCall(methodCall, methodInfo.ReturnType)); - var checkTryParseBlock = innerCall; + assignAndCall[parameters.Length] = checkWasTryParseFailure; - var lambda = Expression.Lambda(checkTryParseBlock, lambdaParameters); - return Expression.Block(new[] { WasTryParseFailureVariable }, Expression.Invoke(lambda, lambdaArgs)); + return Expression.Block(localVariables, assignAndCall); } - private static Expression CreateResponseWritingExpression(Expression methodCall, Type returnType) + private static Expression AddResponseWritingToMethodCall(Expression methodCall, Type returnType) { // Exact request delegate match if (returnType == typeof(void)) { return Expression.Block( methodCall, - Expression.Property(null, (PropertyInfo)CompletedTaskMemberInfo)); + Expression.Property(null, CompletedTaskPropertyInfo)); } else if (AwaitableInfo.IsTypeAwaitable(returnType, out _)) { @@ -441,13 +445,13 @@ private static Expression CreateResponseWritingExpression(Expression methodCall, } } - private static Func HandleRequestBodyAndCompileRequestDelegate(Expression callMethodAndWriteResponse, FactoryContext factoryContext) + private static Func HandleRequestBodyAndCompileRequestDelegate(Expression responseWritingMethodCall, FactoryContext factoryContext) { if (factoryContext.RequestBodyMode is RequestBodyMode.AsJson) { // We need to generate the code for reading from the body before calling into the delegate var invoker = Expression.Lambda>( - callMethodAndWriteResponse, TargetArg, HttpContextParameter, DeserializedBodyParameter).Compile(); + responseWritingMethodCall, TargetArg, HttpContextParameter, DeserializedBodyParameter).Compile(); var bodyType = factoryContext.JsonRequestBodyType!; object? defaultBodyValue = null; @@ -490,7 +494,7 @@ private static Expression CreateResponseWritingExpression(Expression methodCall, else if (factoryContext.RequestBodyMode is RequestBodyMode.AsForm) { var invoker = Expression.Lambda>( - callMethodAndWriteResponse, TargetArg, HttpContextParameter).Compile(); + responseWritingMethodCall, TargetArg, HttpContextParameter).Compile(); return async (target, httpContext) => { @@ -519,11 +523,12 @@ private static Expression CreateResponseWritingExpression(Expression methodCall, else { return Expression.Lambda>( - callMethodAndWriteResponse, TargetArg, HttpContextParameter).Compile(); + responseWritingMethodCall, TargetArg, HttpContextParameter).Compile(); } } // Todo: Cache this. + // Todo: Use CultureInfo.InvariantCulture where possible. private static MethodInfo? FindTryParseMethod(Type type) { var staticMethods = type.GetMethods(BindingFlags.Public | BindingFlags.Static); @@ -558,9 +563,9 @@ private static bool HasTryParseMethod(ParameterInfo parameter) private static Expression GetValueFromProperty(Expression sourceExpression, string key) { var itemProperty = sourceExpression.Type.GetProperty("Item"); - var getIndexArgs = new[] { Expression.Constant(key) }; - var getIndex = Expression.MakeIndex(sourceExpression, itemProperty, getIndexArgs); - return Expression.Convert(getIndex, typeof(string)); + var indexArguments = new[] { Expression.Constant(key) }; + var indexExpression = Expression.MakeIndex(sourceExpression, itemProperty, indexArguments); + return Expression.Convert(indexExpression, typeof(string)); } private static Expression BindParameterFromValue(ParameterInfo parameter, Expression valueExpression, FactoryContext factoryContext) @@ -584,7 +589,8 @@ private static Expression BindParameterFromValue(ParameterInfo parameter, Expres // bool wasTryParseFailureVariable = false; // - // int ParseIdFromRouteValue() + // // Assume "int id" is the first parameter. + // int param1 = // { // var sourceValue = httpContext.Request.RouteValue["id"]; // int parsedValue = default; @@ -596,7 +602,7 @@ private static Expression BindParameterFromValue(ParameterInfo parameter, Expres // } // // return parsedValue; - // } + // }; factoryContext.CheckForTryParseFailure = true; @@ -756,19 +762,20 @@ private static void ThrowCannotReadBodyDirectlyAndAsForm() throw new InvalidOperationException("Action cannot mix FromBody and FromForm on the same method."); } + private enum RequestBodyMode + { + None, + AsJson, + AsForm, + } + private class FactoryContext { public RequestBodyMode RequestBodyMode { get; set; } public Type? JsonRequestBodyType { get; set; } public bool AllowEmptyRequestBody { get; set; } - public bool CheckForTryParseFailure { get; set; } - } - private enum RequestBodyMode - { - None, - AsJson, - AsForm, + public bool CheckForTryParseFailure { get; set; } } private static class Log diff --git a/src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs b/src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs index 427140a99778..3dfe404679d5 100644 --- a/src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs +++ b/src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs @@ -397,10 +397,9 @@ void TestAction([FromRoute] int tryParsable) await requestDelegate(httpContext); - //Assert.False(invoked); - //Assert.False(httpContext.RequestAborted.IsCancellationRequested); - //Assert.Equal(400, httpContext.Response.StatusCode); - Assert.True(invoked); + Assert.False(invoked); + Assert.False(httpContext.RequestAborted.IsCancellationRequested); + Assert.Equal(400, httpContext.Response.StatusCode); var log = Assert.Single(sink.Writes); Assert.Equal(new EventId(3, "ParamaterBindingFailed"), log.EventId); From 828e4dc858028aea390b1656d8226d181aa501e9 Mon Sep 17 00:00:00 2001 From: Stephen Halter Date: Tue, 6 Apr 2021 22:55:03 -0700 Subject: [PATCH 21/35] WIP --- src/Http/Http.Extensions/src/RequestDelegateFactory.cs | 4 ++-- src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs | 1 + 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/src/Http/Http.Extensions/src/RequestDelegateFactory.cs b/src/Http/Http.Extensions/src/RequestDelegateFactory.cs index de516f2761b5..0a5d8991d637 100644 --- a/src/Http/Http.Extensions/src/RequestDelegateFactory.cs +++ b/src/Http/Http.Extensions/src/RequestDelegateFactory.cs @@ -293,7 +293,7 @@ private static Expression CreateTryParseCheckingResponseWritingMethodCall(Method // // if (!int.TryParse(sourceValue, out parsedValue)) // { - // Log.ParameterBindingFailed(httpContext, "int", "id", sourceValue) + // Log.ParameterBindingFailed(httpContext, "Int32", "id", sourceValue) // wasTryParseFailure = true; // } // @@ -597,7 +597,7 @@ private static Expression BindParameterFromValue(ParameterInfo parameter, Expres // // if (!int.TryParse(sourceValue, out parsedValue)) // { - // Log.ParameterBindingFailed(httpContext, "int", "id", sourceValue) + // Log.ParameterBindingFailed(httpContext, "Int32", "id", sourceValue) // wasTryParseFailureVariable = true; // } // diff --git a/src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs b/src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs index 3dfe404679d5..546834f8fdfe 100644 --- a/src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs +++ b/src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs @@ -300,6 +300,7 @@ void StoreTryParsableParameter(HttpContext httpContext, T tryParsable) { // User defined! new object[] { (Action)StoreTryParsableParameter, "42", 42 }, + new object[] { (Action)StoreTryParsableParameter, "42", 42 }, // Byte // Int16 // Int64 From 1b33b3a28434d20cdebea14c73d7851d7fca273d Mon Sep 17 00:00:00 2001 From: Stephen Halter Date: Wed, 7 Apr 2021 12:29:29 -0700 Subject: [PATCH 22/35] Fix nullable handling. Add test cases --- .../src/RequestDelegateFactory.cs | 31 +++++++------- .../test/RequestDelegateFactoryTests.cs | 40 ++++++++++++++++--- 2 files changed, 50 insertions(+), 21 deletions(-) diff --git a/src/Http/Http.Extensions/src/RequestDelegateFactory.cs b/src/Http/Http.Extensions/src/RequestDelegateFactory.cs index 0a5d8991d637..0dd776e8f17f 100644 --- a/src/Http/Http.Extensions/src/RequestDelegateFactory.cs +++ b/src/Http/Http.Extensions/src/RequestDelegateFactory.cs @@ -285,16 +285,16 @@ private static Expression CreateTryParseCheckingResponseWritingMethodCall(Method // { // bool wasTryParseFailure = false; // - // // Assume "int id" is the first parameter. - // int param1 = + // // Assume "[FromRoute] int id" is the first parameter. + // int param1 = httpContext.Request.RouteValue["id"] == null ? default : // { // var sourceValue = httpContext.Request.RouteValue["id"]; // int parsedValue = default; // // if (!int.TryParse(sourceValue, out parsedValue)) // { - // Log.ParameterBindingFailed(httpContext, "Int32", "id", sourceValue) // wasTryParseFailure = true; + // Log.ParameterBindingFailed(httpContext, "Int32", "id", sourceValue) // } // // return parsedValue; @@ -527,8 +527,8 @@ private static Expression AddResponseWritingToMethodCall(Expression methodCall, } } - // Todo: Cache this. - // Todo: Use CultureInfo.InvariantCulture where possible. + // TODO: Cache this. + // TODO: Use InvariantCulture where possible? Or is CurrentCulture fine because it's more flexible? private static MethodInfo? FindTryParseMethod(Type type) { var staticMethods = type.GetMethods(BindingFlags.Public | BindingFlags.Static); @@ -556,8 +556,8 @@ private static Expression AddResponseWritingToMethodCall(Expression methodCall, private static bool HasTryParseMethod(ParameterInfo parameter) { - var parameterType = Nullable.GetUnderlyingType(parameter.ParameterType) ?? parameter.ParameterType; - return FindTryParseMethod(parameterType) is not null; + var nonNullableParameterType = Nullable.GetUnderlyingType(parameter.ParameterType) ?? parameter.ParameterType; + return FindTryParseMethod(nonNullableParameterType) is not null; } private static Expression GetValueFromProperty(Expression sourceExpression, string key) @@ -578,8 +578,8 @@ private static Expression BindParameterFromValue(ParameterInfo parameter, Expres } else { - var parameterType = Nullable.GetUnderlyingType(parameter.ParameterType) ?? parameter.ParameterType; - var tryParseMethod = FindTryParseMethod(parameterType); + var nonNullableParameterType = Nullable.GetUnderlyingType(parameter.ParameterType) ?? parameter.ParameterType; + var tryParseMethod = FindTryParseMethod(nonNullableParameterType); if (tryParseMethod is null) { @@ -589,16 +589,16 @@ private static Expression BindParameterFromValue(ParameterInfo parameter, Expres // bool wasTryParseFailureVariable = false; // - // // Assume "int id" is the first parameter. - // int param1 = + // // Assume "[FromRoute] int id" is the first parameter. + // int param1 = httpContext.Request.RouteValue["id"] == null ? default : // { // var sourceValue = httpContext.Request.RouteValue["id"]; // int parsedValue = default; // // if (!int.TryParse(sourceValue, out parsedValue)) // { - // Log.ParameterBindingFailed(httpContext, "Int32", "id", sourceValue) // wasTryParseFailureVariable = true; + // Log.ParameterBindingFailed(httpContext, "Int32", "id", sourceValue) // } // // return parsedValue; @@ -606,11 +606,11 @@ private static Expression BindParameterFromValue(ParameterInfo parameter, Expres factoryContext.CheckForTryParseFailure = true; - var parsedValue = Expression.Variable(parameter.ParameterType); + var parsedValue = Expression.Variable(nonNullableParameterType); var tryParseCall = Expression.Call(tryParseMethod, valueExpression, parsedValue); - var parameterTypeConstant = Expression.Constant(parameterType.Name); + var parameterTypeConstant = Expression.Constant(nonNullableParameterType.Name); var parameterNameConstant = Expression.Constant(parameter.Name); var failBlock = Expression.Block( Expression.Assign(WasTryParseFailureVariable, Expression.Constant(true)), @@ -623,6 +623,7 @@ private static Expression BindParameterFromValue(ParameterInfo parameter, Expres parsedValue); } + // Convert to nullable if necessary. if (argumentExpression.Type != parameter.ParameterType) { argumentExpression = Expression.Convert(argumentExpression, parameter.ParameterType); @@ -632,7 +633,7 @@ private static Expression BindParameterFromValue(ParameterInfo parameter, Expres Expression.Constant(parameter.DefaultValue) : Expression.Default(parameter.ParameterType); - // property[key] == null ? default : (ParameterType){Type}.Parse(property[key]); + // int param1 = httpContext.Request.RouteValue["id"] == null ? default : ... return Expression.Condition( Expression.Equal(valueExpression, Expression.Constant(null)), defaultExpression, diff --git a/src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs b/src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs index 546834f8fdfe..abda092a4f6b 100644 --- a/src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs +++ b/src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs @@ -7,6 +7,8 @@ using System.Collections.Generic; using System.Globalization; using System.IO; +using System.Net; +using System.Numerics; using System.Reflection; using System.Text; using System.Text.Json; @@ -296,29 +298,55 @@ void StoreTryParsableParameter(HttpContext httpContext, T tryParsable) httpContext.Items["tryParsable"] = tryParsable; } + var today = DateTime.Now; + var todayOffset = DateTimeOffset.UtcNow; + var guid = Guid.NewGuid(); + return new[] { - // User defined! - new object[] { (Action)StoreTryParsableParameter, "42", 42 }, - new object[] { (Action)StoreTryParsableParameter, "42", 42 }, + // User defined class! + // User define Enums + // Int32 + new object[] { (Action)StoreTryParsableParameter, "-42", -42 }, + new object[] { (Action)StoreTryParsableParameter, "42", 42U }, // Byte + new object[] { (Action)StoreTryParsableParameter, "true", true }, // Int16 + new object[] { (Action)StoreTryParsableParameter, "-42", (short)-42 }, + new object[] { (Action)StoreTryParsableParameter, "42", (ushort)42 }, // Int64 + new object[] { (Action)StoreTryParsableParameter, "-42", -42L }, + new object[] { (Action)StoreTryParsableParameter, "42", 42UL }, // IntPtr - // Unsigned versions of above + new object[] { (Action)StoreTryParsableParameter, "-42", new IntPtr(-42) }, // Char - // Single + new object[] { (Action)StoreTryParsableParameter, "A", 'A' }, // Double + new object[] { (Action)StoreTryParsableParameter, "0.5", 0.5 }, + // Single + new object[] { (Action)StoreTryParsableParameter, "0.5", 0.5f }, // Half - // Enums + new object[] { (Action)StoreTryParsableParameter, "0.5", (Half)0.5f }, + // Decimal + new object[] { (Action)StoreTryParsableParameter, "0.5", 0.5m }, // DateTime + new object[] { (Action)StoreTryParsableParameter, today.ToString("o"), today }, // DateTimeOffset + new object[] { (Action)StoreTryParsableParameter, todayOffset.ToString("o"), todayOffset }, // TimeSpan + new object[] { (Action)StoreTryParsableParameter, TimeSpan.FromSeconds(42).ToString(), TimeSpan.FromSeconds(42) }, // Guid + new object[] { (Action)StoreTryParsableParameter, guid.ToString(), guid }, // Version + new object[] { (Action)StoreTryParsableParameter, "6.0.0.42", new Version("6.0.0.42") }, // BigInteger + new object[] { (Action)StoreTryParsableParameter, "-42", new BigInteger(-42) }, // IPAddress + //new object[] { (Action)StoreTryParsableParameter, "-42", new BigInteger(-42) }, // IPEndpoint + // System Enums + // Nullable + new object[] { (Action)StoreTryParsableParameter, "42", 42 }, }; } } From a849e03d8d820792a87eb1e5a1836ffaf7839b58 Mon Sep 17 00:00:00 2001 From: Stephen Halter Date: Wed, 7 Apr 2021 14:40:06 -0700 Subject: [PATCH 23/35] Add enum support --- .../src/RequestDelegateFactory.cs | 31 +++++++ .../test/RequestDelegateFactoryTests.cs | 90 +++++++++++-------- 2 files changed, 86 insertions(+), 35 deletions(-) diff --git a/src/Http/Http.Extensions/src/RequestDelegateFactory.cs b/src/Http/Http.Extensions/src/RequestDelegateFactory.cs index 0dd776e8f17f..a061bcb51265 100644 --- a/src/Http/Http.Extensions/src/RequestDelegateFactory.cs +++ b/src/Http/Http.Extensions/src/RequestDelegateFactory.cs @@ -31,6 +31,8 @@ public static class RequestDelegateFactory private static readonly MethodInfo ExecuteTaskResultOfTMethodInfo = typeof(RequestDelegateFactory).GetMethod(nameof(ExecuteTaskResult), BindingFlags.NonPublic | BindingFlags.Static)!; private static readonly MethodInfo ExecuteValueResultTaskOfTMethodInfo = typeof(RequestDelegateFactory).GetMethod(nameof(ExecuteValueTaskResult), BindingFlags.NonPublic | BindingFlags.Static)!; private static readonly MethodInfo GetRequiredServiceMethodInfo = typeof(ServiceProviderServiceExtensions).GetMethod(nameof(ServiceProviderServiceExtensions.GetRequiredService), BindingFlags.Public | BindingFlags.Static, new Type[] { typeof(IServiceProvider) })!; + private static readonly MethodInfo EnumTryParseMethodInfo = GetEnumTryParseMethod(); + private static readonly MethodInfo ResultWriteResponseAsync = typeof(IResult).GetMethod(nameof(IResult.ExecuteAsync), BindingFlags.Public | BindingFlags.Instance)!; private static readonly MethodInfo StringResultWriteResponseAsync = GetMethodInfo>((response, text) => HttpResponseWritingExtensions.WriteAsync(response, text, default)); private static readonly MethodInfo JsonResultWriteResponseAsync = GetMethodInfo>((response, value) => HttpResponseJsonExtensions.WriteAsJsonAsync(response, value, default)); @@ -527,10 +529,39 @@ private static Expression AddResponseWritingToMethodCall(Expression methodCall, } } + private static MethodInfo GetEnumTryParseMethod() + { + var staticEnumMethods = typeof(Enum).GetMethods(BindingFlags.Public | BindingFlags.Static); + + foreach (var method in staticEnumMethods) + { + if (!method.IsGenericMethod || method.Name != "TryParse" || method.ReturnType != typeof(bool)) + { + continue; + } + + var tryParseParameters = method.GetParameters(); + + if (tryParseParameters.Length == 2 && + tryParseParameters[0].ParameterType == typeof(string) && + tryParseParameters[1].IsOut) + { + return method; + } + } + + throw new Exception("static bool System.Enum.TryParse(string? value, out TEnum result) does not exist!!?!?"); + } + // TODO: Cache this. // TODO: Use InvariantCulture where possible? Or is CurrentCulture fine because it's more flexible? private static MethodInfo? FindTryParseMethod(Type type) { + if (type.IsEnum) + { + return EnumTryParseMethodInfo.MakeGenericMethod(type); + } + var staticMethods = type.GetMethods(BindingFlags.Public | BindingFlags.Static); foreach (var method in staticMethods) diff --git a/src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs b/src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs index abda092a4f6b..286a4ca2077c 100644 --- a/src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs +++ b/src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs @@ -8,8 +8,10 @@ using System.Globalization; using System.IO; using System.Net; +using System.Net.Sockets; using System.Numerics; using System.Reflection; +using System.Reflection.Metadata; using System.Text; using System.Text.Json; using System.Threading; @@ -17,6 +19,7 @@ using Microsoft.AspNetCore.Http; using Microsoft.AspNetCore.Http.Features; using Microsoft.AspNetCore.Http.Metadata; +using Microsoft.AspNetCore.Testing; using Microsoft.Extensions.DependencyInjection; using Microsoft.Extensions.Logging; using Microsoft.Extensions.Logging.Testing; @@ -25,7 +28,7 @@ namespace Microsoft.AspNetCore.Routing.Internal { - public class RequestDelegateFactoryTests + public class RequestDelegateFactoryTests : LoggedTest { public static IEnumerable NoResult { @@ -289,74 +292,91 @@ void TestAction([FromRoute] int foo) Assert.Equal(0, deserializedRouteParam); } - public static object[][] FromTryParsableParameter + public static object?[][] TryParsableParameters { get { - void StoreTryParsableParameter(HttpContext httpContext, T tryParsable) + void Store(HttpContext httpContext, T tryParsable) { httpContext.Items["tryParsable"] = tryParsable; } - var today = DateTime.Now; - var todayOffset = DateTimeOffset.UtcNow; + var now = DateTime.Now; + var nowOffset = DateTimeOffset.UtcNow; var guid = Guid.NewGuid(); return new[] { - // User defined class! - // User define Enums + // String (technically not "TryParseable", but it's the the special case) + new object[] { (Action)Store, "plain string", "plain string" }, // Int32 - new object[] { (Action)StoreTryParsableParameter, "-42", -42 }, - new object[] { (Action)StoreTryParsableParameter, "42", 42U }, + new object[] { (Action)Store, "-42", -42 }, + new object[] { (Action)Store, "42", 42U }, // Byte - new object[] { (Action)StoreTryParsableParameter, "true", true }, + new object[] { (Action)Store, "true", true }, // Int16 - new object[] { (Action)StoreTryParsableParameter, "-42", (short)-42 }, - new object[] { (Action)StoreTryParsableParameter, "42", (ushort)42 }, + new object[] { (Action)Store, "-42", (short)-42 }, + new object[] { (Action)Store, "42", (ushort)42 }, // Int64 - new object[] { (Action)StoreTryParsableParameter, "-42", -42L }, - new object[] { (Action)StoreTryParsableParameter, "42", 42UL }, + new object[] { (Action)Store, "-42", -42L }, + new object[] { (Action)Store, "42", 42UL }, // IntPtr - new object[] { (Action)StoreTryParsableParameter, "-42", new IntPtr(-42) }, + new object[] { (Action)Store, "-42", new IntPtr(-42) }, // Char - new object[] { (Action)StoreTryParsableParameter, "A", 'A' }, + new object[] { (Action)Store, "A", 'A' }, // Double - new object[] { (Action)StoreTryParsableParameter, "0.5", 0.5 }, + new object[] { (Action)Store, "0.5", 0.5 }, // Single - new object[] { (Action)StoreTryParsableParameter, "0.5", 0.5f }, + new object[] { (Action)Store, "0.5", 0.5f }, // Half - new object[] { (Action)StoreTryParsableParameter, "0.5", (Half)0.5f }, + new object[] { (Action)Store, "0.5", (Half)0.5f }, // Decimal - new object[] { (Action)StoreTryParsableParameter, "0.5", 0.5m }, + new object[] { (Action)Store, "0.5", 0.5m }, // DateTime - new object[] { (Action)StoreTryParsableParameter, today.ToString("o"), today }, + new object[] { (Action)Store, now.ToString("o"), now }, // DateTimeOffset - new object[] { (Action)StoreTryParsableParameter, todayOffset.ToString("o"), todayOffset }, + new object[] { (Action)Store, nowOffset.ToString("o"), nowOffset }, // TimeSpan - new object[] { (Action)StoreTryParsableParameter, TimeSpan.FromSeconds(42).ToString(), TimeSpan.FromSeconds(42) }, + new object[] { (Action)Store, TimeSpan.FromSeconds(42).ToString(), TimeSpan.FromSeconds(42) }, // Guid - new object[] { (Action)StoreTryParsableParameter, guid.ToString(), guid }, + new object[] { (Action)Store, guid.ToString(), guid }, // Version - new object[] { (Action)StoreTryParsableParameter, "6.0.0.42", new Version("6.0.0.42") }, + new object[] { (Action)Store, "6.0.0.42", new Version("6.0.0.42") }, // BigInteger - new object[] { (Action)StoreTryParsableParameter, "-42", new BigInteger(-42) }, + new object[] { (Action)Store, "-42", new BigInteger(-42) }, // IPAddress - //new object[] { (Action)StoreTryParsableParameter, "-42", new BigInteger(-42) }, - // IPEndpoint + new object[] { (Action)Store, "127.0.0.1", IPAddress.Loopback }, + // IPEndPoint + new object[] { (Action)Store, "127.0.0.1:80", new IPEndPoint(IPAddress.Loopback, 80) }, // System Enums + new object[] { (Action)Store, "Unix", AddressFamily.Unix }, + new object[] { (Action)Store, "Nop", ILOpCode.Nop }, + new object[] { (Action)Store, "PublicKey,Retargetable", AssemblyFlags.PublicKey | AssemblyFlags.Retargetable }, // Nullable - new object[] { (Action)StoreTryParsableParameter, "42", 42 }, + new object[] { (Action)Store, "42", 42 }, + + // Null route and/or query value gives default value. + new object?[] { (Action)Store, null, 0 }, + new object?[] { (Action)Store, null, null }, + + // User defined class! + // User defined Enums }; } } [Theory] - [MemberData(nameof(FromTryParsableParameter))] - public async Task RequestDelegatePopulatesUnattributedTryParseableParametersFromRouteValue(Delegate action, string routeValue, object expectedParameterValue) + [MemberData(nameof(TryParsableParameters))] + public async Task RequestDelegatePopulatesUnattributedTryParseableParametersFromRouteValue(Delegate action, string? routeValue, object? expectedParameterValue) { + var invalidDataException = new InvalidDataException(); + var serviceCollection = new ServiceCollection(); + serviceCollection.AddSingleton(LoggerFactory); + var httpContext = new DefaultHttpContext(); httpContext.Request.RouteValues["tryParsable"] = routeValue; + httpContext.Features.Set(new TestHttpRequestLifetimeFeature()); + httpContext.RequestServices = serviceCollection.BuildServiceProvider(); var requestDelegate = RequestDelegateFactory.Create(action); @@ -366,8 +386,8 @@ public async Task RequestDelegatePopulatesUnattributedTryParseableParametersFrom } [Theory] - [MemberData(nameof(FromTryParsableParameter))] - public async Task RequestDelegatePopulatesUnattributedTryParseableParametersFromQueryString(Delegate action, string routeValue, object expectedParameterValue) + [MemberData(nameof(TryParsableParameters))] + public async Task RequestDelegatePopulatesUnattributedTryParseableParametersFromQueryString(Delegate action, string? routeValue, object? expectedParameterValue) { var httpContext = new DefaultHttpContext(); httpContext.Request.Query = new QueryCollection(new Dictionary @@ -383,8 +403,8 @@ public async Task RequestDelegatePopulatesUnattributedTryParseableParametersFrom } [Theory] - [MemberData(nameof(FromTryParsableParameter))] - public async Task RequestDelegatePopulatesUnattributedTryParseableParametersFromRouteValueBeforeQueryString(Delegate action, string routeValue, object expectedParameterValue) + [MemberData(nameof(TryParsableParameters))] + public async Task RequestDelegatePopulatesUnattributedTryParseableParametersFromRouteValueBeforeQueryString(Delegate action, string? routeValue, object? expectedParameterValue) { var httpContext = new DefaultHttpContext(); httpContext.Request.RouteValues["tryParsable"] = routeValue; From 5d7f7f466264b37d9d10f8b29fcd6615297321f2 Mon Sep 17 00:00:00 2001 From: Stephen Halter Date: Wed, 7 Apr 2021 14:45:58 -0700 Subject: [PATCH 24/35] Less Info, More Expr --- .../src/RequestDelegateFactory.cs | 144 +++++++++--------- 1 file changed, 69 insertions(+), 75 deletions(-) diff --git a/src/Http/Http.Extensions/src/RequestDelegateFactory.cs b/src/Http/Http.Extensions/src/RequestDelegateFactory.cs index a061bcb51265..c2575b2a998a 100644 --- a/src/Http/Http.Extensions/src/RequestDelegateFactory.cs +++ b/src/Http/Http.Extensions/src/RequestDelegateFactory.cs @@ -22,41 +22,36 @@ namespace Microsoft.AspNetCore.Http /// public static class RequestDelegateFactory { - private static readonly MethodInfo ChangeTypeMethodInfo = GetMethodInfo>((value, type) => Convert.ChangeType(value, type, CultureInfo.InvariantCulture)); - private static readonly MethodInfo ExecuteTaskOfTMethodInfo = typeof(RequestDelegateFactory).GetMethod(nameof(ExecuteTask), BindingFlags.NonPublic | BindingFlags.Static)!; - private static readonly MethodInfo ExecuteTaskOfStringMethodInfo = typeof(RequestDelegateFactory).GetMethod(nameof(ExecuteTaskOfString), BindingFlags.NonPublic | BindingFlags.Static)!; - private static readonly MethodInfo ExecuteValueTaskOfTMethodInfo = typeof(RequestDelegateFactory).GetMethod(nameof(ExecuteValueTaskOfT), BindingFlags.NonPublic | BindingFlags.Static)!; - private static readonly MethodInfo ExecuteValueTaskMethodInfo = typeof(RequestDelegateFactory).GetMethod(nameof(ExecuteValueTask), BindingFlags.NonPublic | BindingFlags.Static)!; - private static readonly MethodInfo ExecuteValueTaskOfStringMethodInfo = typeof(RequestDelegateFactory).GetMethod(nameof(ExecuteValueTaskOfString), BindingFlags.NonPublic | BindingFlags.Static)!; - private static readonly MethodInfo ExecuteTaskResultOfTMethodInfo = typeof(RequestDelegateFactory).GetMethod(nameof(ExecuteTaskResult), BindingFlags.NonPublic | BindingFlags.Static)!; - private static readonly MethodInfo ExecuteValueResultTaskOfTMethodInfo = typeof(RequestDelegateFactory).GetMethod(nameof(ExecuteValueTaskResult), BindingFlags.NonPublic | BindingFlags.Static)!; - private static readonly MethodInfo GetRequiredServiceMethodInfo = typeof(ServiceProviderServiceExtensions).GetMethod(nameof(ServiceProviderServiceExtensions.GetRequiredService), BindingFlags.Public | BindingFlags.Static, new Type[] { typeof(IServiceProvider) })!; - private static readonly MethodInfo EnumTryParseMethodInfo = GetEnumTryParseMethod(); - - private static readonly MethodInfo ResultWriteResponseAsync = typeof(IResult).GetMethod(nameof(IResult.ExecuteAsync), BindingFlags.Public | BindingFlags.Instance)!; - private static readonly MethodInfo StringResultWriteResponseAsync = GetMethodInfo>((response, text) => HttpResponseWritingExtensions.WriteAsync(response, text, default)); - private static readonly MethodInfo JsonResultWriteResponseAsync = GetMethodInfo>((response, value) => HttpResponseJsonExtensions.WriteAsJsonAsync(response, value, default)); - - private static readonly PropertyInfo CompletedTaskPropertyInfo = (PropertyInfo)GetMemberInfo>(() => Task.CompletedTask); - - private static readonly MethodInfo LogParameterBindingFailure = GetMethodInfo>((httpContext, parameterType, parameterName, sourceValue) => + private static readonly MethodInfo ExecuteTaskOfTMethod = typeof(RequestDelegateFactory).GetMethod(nameof(ExecuteTask), BindingFlags.NonPublic | BindingFlags.Static)!; + private static readonly MethodInfo ExecuteTaskOfStringMethod = typeof(RequestDelegateFactory).GetMethod(nameof(ExecuteTaskOfString), BindingFlags.NonPublic | BindingFlags.Static)!; + private static readonly MethodInfo ExecuteValueTaskOfTMethod = typeof(RequestDelegateFactory).GetMethod(nameof(ExecuteValueTaskOfT), BindingFlags.NonPublic | BindingFlags.Static)!; + private static readonly MethodInfo ExecuteValueTaskMethod = typeof(RequestDelegateFactory).GetMethod(nameof(ExecuteValueTask), BindingFlags.NonPublic | BindingFlags.Static)!; + private static readonly MethodInfo ExecuteValueTaskOfStringMethod = typeof(RequestDelegateFactory).GetMethod(nameof(ExecuteValueTaskOfString), BindingFlags.NonPublic | BindingFlags.Static)!; + private static readonly MethodInfo ExecuteTaskResultOfTMethod = typeof(RequestDelegateFactory).GetMethod(nameof(ExecuteTaskResult), BindingFlags.NonPublic | BindingFlags.Static)!; + private static readonly MethodInfo ExecuteValueResultTaskOfTMethod = typeof(RequestDelegateFactory).GetMethod(nameof(ExecuteValueTaskResult), BindingFlags.NonPublic | BindingFlags.Static)!; + private static readonly MethodInfo GetRequiredServiceMethod = typeof(ServiceProviderServiceExtensions).GetMethod(nameof(ServiceProviderServiceExtensions.GetRequiredService), BindingFlags.Public | BindingFlags.Static, new Type[] { typeof(IServiceProvider) })!; + private static readonly MethodInfo ResultWriteResponseAsyncMethod = typeof(IResult).GetMethod(nameof(IResult.ExecuteAsync), BindingFlags.Public | BindingFlags.Instance)!; + private static readonly MethodInfo StringResultWriteResponseAsyncMethod = GetMethodInfo>((response, text) => HttpResponseWritingExtensions.WriteAsync(response, text, default)); + private static readonly MethodInfo JsonResultWriteResponseAsyncMethod = GetMethodInfo>((response, value) => HttpResponseJsonExtensions.WriteAsJsonAsync(response, value, default)); + private static readonly MethodInfo EnumTryParseMethod = GetEnumTryParseMethod(); + private static readonly MethodInfo LogParameterBindingFailureMethod = GetMethodInfo>((httpContext, parameterType, parameterName, sourceValue) => Log.ParameterBindingFailed(httpContext, parameterType, parameterName, sourceValue)); - private static readonly ParameterExpression TargetArg = Expression.Parameter(typeof(object), "target"); - private static readonly ParameterExpression HttpContextParameter = Expression.Parameter(typeof(HttpContext), "httpContext"); - - private static readonly ParameterExpression DeserializedBodyParameter = Expression.Parameter(typeof(object), "bodyValue"); - private static readonly ParameterExpression WasTryParseFailureVariable = Expression.Variable(typeof(bool), "wasTryParseFailure"); - - private static readonly MemberExpression RequestServicesExpr = Expression.Property(HttpContextParameter, nameof(HttpContext.RequestServices)); - private static readonly MemberExpression HttpRequestExpr = Expression.Property(HttpContextParameter, nameof(HttpContext.Request)); - private static readonly MemberExpression HttpResponseExpr = Expression.Property(HttpContextParameter, nameof(HttpContext.Response)); - private static readonly MemberExpression RequestAbortedExpr = Expression.Property(HttpContextParameter, nameof(HttpContext.RequestAborted)); - private static readonly MemberExpression RouteValuesProperty = Expression.Property(HttpRequestExpr, nameof(HttpRequest.RouteValues)); - private static readonly MemberExpression QueryProperty = Expression.Property(HttpRequestExpr, nameof(HttpRequest.Query)); - private static readonly MemberExpression HeadersProperty = Expression.Property(HttpRequestExpr, nameof(HttpRequest.Headers)); - private static readonly MemberExpression FormProperty = Expression.Property(HttpRequestExpr, nameof(HttpRequest.Form)); - private static readonly MemberExpression StatusCodeProperty = Expression.Property(HttpResponseExpr, nameof(HttpResponse.StatusCode)); + private static readonly ParameterExpression TargetExpr = Expression.Parameter(typeof(object), "target"); + private static readonly ParameterExpression HttpContextExpr = Expression.Parameter(typeof(HttpContext), "httpContext"); + private static readonly ParameterExpression BodyValueExpr = Expression.Parameter(typeof(object), "bodyValue"); + private static readonly ParameterExpression WasTryParseFailureExpr = Expression.Variable(typeof(bool), "wasTryParseFailure"); + + private static readonly MemberExpression RequestServicesExpr = Expression.Property(HttpContextExpr, nameof(HttpContext.RequestServices)); + private static readonly MemberExpression HttpRequestExpr = Expression.Property(HttpContextExpr, nameof(HttpContext.Request)); + private static readonly MemberExpression HttpResponseExpr = Expression.Property(HttpContextExpr, nameof(HttpContext.Response)); + private static readonly MemberExpression RequestAbortedExpr = Expression.Property(HttpContextExpr, nameof(HttpContext.RequestAborted)); + private static readonly MemberExpression RouteValuesExpr = Expression.Property(HttpRequestExpr, nameof(HttpRequest.RouteValues)); + private static readonly MemberExpression QueryExpr = Expression.Property(HttpRequestExpr, nameof(HttpRequest.Query)); + private static readonly MemberExpression HeadersExpr = Expression.Property(HttpRequestExpr, nameof(HttpRequest.Headers)); + private static readonly MemberExpression FormExpr = Expression.Property(HttpRequestExpr, nameof(HttpRequest.Form)); + private static readonly MemberExpression StatusCodeExpr = Expression.Property(HttpResponseExpr, nameof(HttpResponse.StatusCode)); + private static readonly MemberExpression CompletedTaskExpr = Expression.Property(null, (PropertyInfo)GetMemberInfo>(() => Task.CompletedTask)); /// /// Creates a implementation for . @@ -72,7 +67,7 @@ public static RequestDelegate Create(Delegate action) var targetExpression = action.Target switch { - object => Expression.Convert(TargetArg, action.Target.GetType()), + object => Expression.Convert(TargetExpr, action.Target.GetType()), null => null, }; @@ -127,7 +122,7 @@ public static RequestDelegate Create(MethodInfo methodInfo, Func @@ -194,15 +189,15 @@ private static Expression CreateArgument(ParameterInfo parameter, FactoryContext if (parameterCustomAttributes.OfType().FirstOrDefault() is { } routeAttribute) { - return BindParameterFromProperty(parameter, RouteValuesProperty, routeAttribute.Name ?? parameter.Name, factoryContext); + return BindParameterFromProperty(parameter, RouteValuesExpr, routeAttribute.Name ?? parameter.Name, factoryContext); } else if (parameterCustomAttributes.OfType().FirstOrDefault() is { } queryAttribute) { - return BindParameterFromProperty(parameter, QueryProperty, queryAttribute.Name ?? parameter.Name, factoryContext); + return BindParameterFromProperty(parameter, QueryExpr, queryAttribute.Name ?? parameter.Name, factoryContext); } else if (parameterCustomAttributes.OfType().FirstOrDefault() is { } headerAttribute) { - return BindParameterFromProperty(parameter, HeadersProperty, headerAttribute.Name ?? parameter.Name, factoryContext); + return BindParameterFromProperty(parameter, HeadersExpr, headerAttribute.Name ?? parameter.Name, factoryContext); } else if (parameterCustomAttributes.OfType().FirstOrDefault() is { } bodyAttribute) { @@ -220,7 +215,7 @@ private static Expression CreateArgument(ParameterInfo parameter, FactoryContext factoryContext.JsonRequestBodyType = parameter.ParameterType; factoryContext.AllowEmptyRequestBody = bodyAttribute.AllowEmpty; - return Expression.Convert(DeserializedBodyParameter, parameter.ParameterType); + return Expression.Convert(BodyValueExpr, parameter.ParameterType); } else if (parameterCustomAttributes.OfType().FirstOrDefault() is { } formAttribute) { @@ -231,11 +226,11 @@ private static Expression CreateArgument(ParameterInfo parameter, FactoryContext factoryContext.RequestBodyMode = RequestBodyMode.AsForm; - return BindParameterFromProperty(parameter, FormProperty, formAttribute.Name ?? parameter.Name, factoryContext); + return BindParameterFromProperty(parameter, FormExpr, formAttribute.Name ?? parameter.Name, factoryContext); } else if (parameter.CustomAttributes.Any(a => typeof(IFromServiceMetadata).IsAssignableFrom(a.AttributeType))) { - return Expression.Call(GetRequiredServiceMethodInfo.MakeGenericMethod(parameter.ParameterType), RequestServicesExpr); + return Expression.Call(GetRequiredServiceMethod.MakeGenericMethod(parameter.ParameterType), RequestServicesExpr); } else if (parameter.ParameterType == typeof(IFormCollection)) { @@ -250,7 +245,7 @@ private static Expression CreateArgument(ParameterInfo parameter, FactoryContext } else if (parameter.ParameterType == typeof(HttpContext)) { - return HttpContextParameter; + return HttpContextExpr; } else if (parameter.ParameterType == typeof(CancellationToken)) { @@ -266,7 +261,7 @@ private static Expression CreateArgument(ParameterInfo parameter, FactoryContext } else { - return Expression.Call(GetRequiredServiceMethodInfo.MakeGenericMethod(parameter.ParameterType), RequestServicesExpr); + return Expression.Call(GetRequiredServiceMethod.MakeGenericMethod(parameter.ParameterType), RequestServicesExpr); } } @@ -322,7 +317,7 @@ private static Expression CreateTryParseCheckingResponseWritingMethodCall(Method storedArguments[i] = localVariables[i] = Expression.Parameter(parameters[i].ParameterType); } - localVariables[parameters.Length] = WasTryParseFailureVariable; + localVariables[parameters.Length] = WasTryParseFailureExpr; var assignAndCall = new Expression[parameters.Length + 1]; for (var i = 0; i < parameters.Length; i++) @@ -331,12 +326,12 @@ private static Expression CreateTryParseCheckingResponseWritingMethodCall(Method } var set400StatusAndReturnCompletedTask = Expression.Block( - Expression.Assign(StatusCodeProperty, Expression.Constant(400)), - Expression.Property(null, CompletedTaskPropertyInfo)); + Expression.Assign(StatusCodeExpr, Expression.Constant(400)), + CompletedTaskExpr); var methodCall = CreateMethodCall(methodInfo, target, storedArguments); - var checkWasTryParseFailure = Expression.Condition(WasTryParseFailureVariable, + var checkWasTryParseFailure = Expression.Condition(WasTryParseFailureExpr, set400StatusAndReturnCompletedTask, AddResponseWritingToMethodCall(methodCall, methodInfo.ReturnType)); @@ -350,9 +345,7 @@ private static Expression AddResponseWritingToMethodCall(Expression methodCall, // Exact request delegate match if (returnType == typeof(void)) { - return Expression.Block( - methodCall, - Expression.Property(null, CompletedTaskPropertyInfo)); + return Expression.Block(methodCall, CompletedTaskExpr); } else if (AwaitableInfo.IsTypeAwaitable(returnType, out _)) { @@ -363,7 +356,7 @@ private static Expression AddResponseWritingToMethodCall(Expression methodCall, else if (returnType == typeof(ValueTask)) { return Expression.Call( - ExecuteValueTaskMethodInfo, + ExecuteValueTaskMethod, methodCall); } else if (returnType.IsGenericType && @@ -374,24 +367,24 @@ private static Expression AddResponseWritingToMethodCall(Expression methodCall, if (typeof(IResult).IsAssignableFrom(typeArg)) { return Expression.Call( - ExecuteTaskResultOfTMethodInfo.MakeGenericMethod(typeArg), + ExecuteTaskResultOfTMethod.MakeGenericMethod(typeArg), methodCall, - HttpContextParameter); + HttpContextExpr); } // ExecuteTask(action(..), httpContext); else if (typeArg == typeof(string)) { return Expression.Call( - ExecuteTaskOfStringMethodInfo, + ExecuteTaskOfStringMethod, methodCall, - HttpContextParameter); + HttpContextExpr); } else { return Expression.Call( - ExecuteTaskOfTMethodInfo.MakeGenericMethod(typeArg), + ExecuteTaskOfTMethod.MakeGenericMethod(typeArg), methodCall, - HttpContextParameter); + HttpContextExpr); } } else if (returnType.IsGenericType && @@ -402,24 +395,24 @@ private static Expression AddResponseWritingToMethodCall(Expression methodCall, if (typeof(IResult).IsAssignableFrom(typeArg)) { return Expression.Call( - ExecuteValueResultTaskOfTMethodInfo.MakeGenericMethod(typeArg), + ExecuteValueResultTaskOfTMethod.MakeGenericMethod(typeArg), methodCall, - HttpContextParameter); + HttpContextExpr); } // ExecuteTask(action(..), httpContext); else if (typeArg == typeof(string)) { return Expression.Call( - ExecuteValueTaskOfStringMethodInfo, + ExecuteValueTaskOfStringMethod, methodCall, - HttpContextParameter); + HttpContextExpr); } else { return Expression.Call( - ExecuteValueTaskOfTMethodInfo.MakeGenericMethod(typeArg), + ExecuteValueTaskOfTMethod.MakeGenericMethod(typeArg), methodCall, - HttpContextParameter); + HttpContextExpr); } } else @@ -430,20 +423,20 @@ private static Expression AddResponseWritingToMethodCall(Expression methodCall, } else if (typeof(IResult).IsAssignableFrom(returnType)) { - return Expression.Call(methodCall, ResultWriteResponseAsync, HttpContextParameter); + return Expression.Call(methodCall, ResultWriteResponseAsyncMethod, HttpContextExpr); } else if (returnType == typeof(string)) { - return Expression.Call(StringResultWriteResponseAsync, HttpResponseExpr, methodCall, Expression.Constant(CancellationToken.None)); + return Expression.Call(StringResultWriteResponseAsyncMethod, HttpResponseExpr, methodCall, Expression.Constant(CancellationToken.None)); } else if (returnType.IsValueType) { var box = Expression.TypeAs(methodCall, typeof(object)); - return Expression.Call(JsonResultWriteResponseAsync, HttpResponseExpr, box, Expression.Constant(CancellationToken.None)); + return Expression.Call(JsonResultWriteResponseAsyncMethod, HttpResponseExpr, box, Expression.Constant(CancellationToken.None)); } else { - return Expression.Call(JsonResultWriteResponseAsync, HttpResponseExpr, methodCall, Expression.Constant(CancellationToken.None)); + return Expression.Call(JsonResultWriteResponseAsyncMethod, HttpResponseExpr, methodCall, Expression.Constant(CancellationToken.None)); } } @@ -453,7 +446,7 @@ private static Expression AddResponseWritingToMethodCall(Expression methodCall, { // We need to generate the code for reading from the body before calling into the delegate var invoker = Expression.Lambda>( - responseWritingMethodCall, TargetArg, HttpContextParameter, DeserializedBodyParameter).Compile(); + responseWritingMethodCall, TargetExpr, HttpContextExpr, BodyValueExpr).Compile(); var bodyType = factoryContext.JsonRequestBodyType!; object? defaultBodyValue = null; @@ -496,7 +489,7 @@ private static Expression AddResponseWritingToMethodCall(Expression methodCall, else if (factoryContext.RequestBodyMode is RequestBodyMode.AsForm) { var invoker = Expression.Lambda>( - responseWritingMethodCall, TargetArg, HttpContextParameter).Compile(); + responseWritingMethodCall, TargetExpr, HttpContextExpr).Compile(); return async (target, httpContext) => { @@ -525,7 +518,7 @@ private static Expression AddResponseWritingToMethodCall(Expression methodCall, else { return Expression.Lambda>( - responseWritingMethodCall, TargetArg, HttpContextParameter).Compile(); + responseWritingMethodCall, TargetExpr, HttpContextExpr).Compile(); } } @@ -559,7 +552,7 @@ private static MethodInfo GetEnumTryParseMethod() { if (type.IsEnum) { - return EnumTryParseMethodInfo.MakeGenericMethod(type); + return EnumTryParseMethod.MakeGenericMethod(type); } var staticMethods = type.GetMethods(BindingFlags.Public | BindingFlags.Static); @@ -644,8 +637,9 @@ private static Expression BindParameterFromValue(ParameterInfo parameter, Expres var parameterTypeConstant = Expression.Constant(nonNullableParameterType.Name); var parameterNameConstant = Expression.Constant(parameter.Name); var failBlock = Expression.Block( - Expression.Assign(WasTryParseFailureVariable, Expression.Constant(true)), - Expression.Call(LogParameterBindingFailure, HttpContextParameter, parameterTypeConstant, parameterNameConstant, valueExpression)); + Expression.Assign(WasTryParseFailureExpr, Expression.Constant(true)), + Expression.Call(LogParameterBindingFailureMethod, + HttpContextExpr, parameterTypeConstant, parameterNameConstant, valueExpression)); var ifFailExpression = Expression.IfThen(Expression.Not(tryParseCall), failBlock); @@ -676,8 +670,8 @@ private static Expression BindParameterFromProperty(ParameterInfo parameter, Mem private static Expression BindParameterFromRouteValueOrQueryString(ParameterInfo parameter, string key, FactoryContext factoryContext) { - var routeValue = GetValueFromProperty(RouteValuesProperty, key); - var queryValue = GetValueFromProperty(QueryProperty, key); + var routeValue = GetValueFromProperty(RouteValuesExpr, key); + var queryValue = GetValueFromProperty(QueryExpr, key); return BindParameterFromValue(parameter, Expression.Coalesce(routeValue, queryValue), factoryContext); } From ff6b84355c8031c8acc6b7406188bc59365c3601 Mon Sep 17 00:00:00 2001 From: Stephen Halter Date: Wed, 7 Apr 2021 15:07:51 -0700 Subject: [PATCH 25/35] Add custom record and enum to TryParse tests --- .../src/RequestDelegateFactory.cs | 1 - .../test/RequestDelegateFactoryTests.cs | 33 ++++++++++++++----- 2 files changed, 25 insertions(+), 9 deletions(-) diff --git a/src/Http/Http.Extensions/src/RequestDelegateFactory.cs b/src/Http/Http.Extensions/src/RequestDelegateFactory.cs index c2575b2a998a..3a786db34026 100644 --- a/src/Http/Http.Extensions/src/RequestDelegateFactory.cs +++ b/src/Http/Http.Extensions/src/RequestDelegateFactory.cs @@ -3,7 +3,6 @@ using System; using System.Diagnostics; -using System.Globalization; using System.IO; using System.Linq; using System.Linq.Expressions; diff --git a/src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs b/src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs index 286a4ca2077c..3634c014f7a5 100644 --- a/src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs +++ b/src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs @@ -307,7 +307,7 @@ void Store(HttpContext httpContext, T tryParsable) return new[] { - // String (technically not "TryParseable", but it's the the special case) + // String (technically not "TryParsable", but it's the the special case) new object[] { (Action)Store, "plain string", "plain string" }, // Int32 new object[] { (Action)Store, "-42", -42 }, @@ -354,20 +354,37 @@ void Store(HttpContext httpContext, T tryParsable) new object[] { (Action)Store, "PublicKey,Retargetable", AssemblyFlags.PublicKey | AssemblyFlags.Retargetable }, // Nullable new object[] { (Action)Store, "42", 42 }, - // Null route and/or query value gives default value. new object?[] { (Action)Store, null, 0 }, new object?[] { (Action)Store, null, null }, - - // User defined class! - // User defined Enums + // Custom Enum + new object[] { (Action)Store, "ValueB", MyEnum.ValueB }, + // Custom record with static TryParse method! + new object[] { (Action)Store, "42", new MyTryParsableRecord(42) }, }; } } + private enum MyEnum { ValueA, ValueB, } + + private record MyTryParsableRecord(int State) + { + public static bool TryParse(string value, out MyTryParsableRecord? result) + { + if (!int.TryParse(value, out var state)) + { + result = null; + return false; + } + + result = new MyTryParsableRecord(state); + return true; + } + } + [Theory] [MemberData(nameof(TryParsableParameters))] - public async Task RequestDelegatePopulatesUnattributedTryParseableParametersFromRouteValue(Delegate action, string? routeValue, object? expectedParameterValue) + public async Task RequestDelegatePopulatesUnattributedTryParsableParametersFromRouteValue(Delegate action, string? routeValue, object? expectedParameterValue) { var invalidDataException = new InvalidDataException(); var serviceCollection = new ServiceCollection(); @@ -387,7 +404,7 @@ public async Task RequestDelegatePopulatesUnattributedTryParseableParametersFrom [Theory] [MemberData(nameof(TryParsableParameters))] - public async Task RequestDelegatePopulatesUnattributedTryParseableParametersFromQueryString(Delegate action, string? routeValue, object? expectedParameterValue) + public async Task RequestDelegatePopulatesUnattributedTryParsableParametersFromQueryString(Delegate action, string? routeValue, object? expectedParameterValue) { var httpContext = new DefaultHttpContext(); httpContext.Request.Query = new QueryCollection(new Dictionary @@ -404,7 +421,7 @@ public async Task RequestDelegatePopulatesUnattributedTryParseableParametersFrom [Theory] [MemberData(nameof(TryParsableParameters))] - public async Task RequestDelegatePopulatesUnattributedTryParseableParametersFromRouteValueBeforeQueryString(Delegate action, string? routeValue, object? expectedParameterValue) + public async Task RequestDelegatePopulatesUnattributedTryParsableParametersFromRouteValueBeforeQueryString(Delegate action, string? routeValue, object? expectedParameterValue) { var httpContext = new DefaultHttpContext(); httpContext.Request.RouteValues["tryParsable"] = routeValue; From 0b283d162c8b2e1288e7248fba9ea2231c8866cf Mon Sep 17 00:00:00 2001 From: Stephen Halter Date: Wed, 7 Apr 2021 15:23:49 -0700 Subject: [PATCH 26/35] Add tests for DelegatesWithInvalidAttributes --- .../src/RequestDelegateFactory.cs | 3 +-- .../test/RequestDelegateFactoryTests.cs | 27 +++++++++++++++++++ 2 files changed, 28 insertions(+), 2 deletions(-) diff --git a/src/Http/Http.Extensions/src/RequestDelegateFactory.cs b/src/Http/Http.Extensions/src/RequestDelegateFactory.cs index 3a786db34026..2e4e0e294ba1 100644 --- a/src/Http/Http.Extensions/src/RequestDelegateFactory.cs +++ b/src/Http/Http.Extensions/src/RequestDelegateFactory.cs @@ -606,8 +606,7 @@ private static Expression BindParameterFromValue(ParameterInfo parameter, Expres if (tryParseMethod is null) { - // TODO: Add test! - throw new InvalidOperationException($"No public static {parameter.ParameterType}.TryParse(string, out {parameter.ParameterType}) method found for {parameter}."); + throw new InvalidOperationException($"No public static bool {parameter.ParameterType.Name}.TryParse(string, out {parameter.ParameterType.Name}) method found for {parameter.Name}."); } // bool wasTryParseFailureVariable = false; diff --git a/src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs b/src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs index 3634c014f7a5..aef416bb1205 100644 --- a/src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs +++ b/src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs @@ -437,6 +437,33 @@ public async Task RequestDelegatePopulatesUnattributedTryParsableParametersFromR Assert.Equal(expectedParameterValue, httpContext.Items["tryParsable"]); } + public static object?[][] DelegatesWithInvalidAttributes + { + get + { + void InvalidFromRoute([FromRoute] object notTryParsable) { } + void InvalidFromQuery([FromQuery] object notTryParsable) { } + void InvalidFromHeader([FromHeader] object notTryParsable) { } + void InvalidFromForm([FromForm] object notTryParsable) { } + + return new[] + { + new object[] { (Action)InvalidFromRoute }, + new object[] { (Action)InvalidFromQuery }, + new object[] { (Action)InvalidFromHeader }, + new object[] { (Action)InvalidFromForm }, + }; + } + } + + [Theory] + [MemberData(nameof(DelegatesWithInvalidAttributes))] + public void CreateThrowsInvalidOperationExceptionWhenAttributeRequiresTryParseMethodThatDoesNotExist(Delegate action) + { + var ex = Assert.Throws(() => RequestDelegateFactory.Create(action)); + Assert.Equal("No public static bool Object.TryParse(string, out Object) method found for notTryParsable.", ex.Message); + } + [Fact] public async Task RequestDelegateLogsTryParsableFailuresAsDebugAndSets400Response() { From f7601b9d659cfcf1a0b07e6f4f30588cd2136fc1 Mon Sep 17 00:00:00 2001 From: Stephen Halter Date: Wed, 7 Apr 2021 15:37:19 -0700 Subject: [PATCH 27/35] Test cleanup --- .../test/RequestDelegateFactoryTests.cs | 69 +++++++++---------- 1 file changed, 32 insertions(+), 37 deletions(-) diff --git a/src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs b/src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs index aef416bb1205..8ba72351f97c 100644 --- a/src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs +++ b/src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs @@ -22,7 +22,6 @@ using Microsoft.AspNetCore.Testing; using Microsoft.Extensions.DependencyInjection; using Microsoft.Extensions.Logging; -using Microsoft.Extensions.Logging.Testing; using Microsoft.Extensions.Primitives; using Xunit; @@ -419,22 +418,25 @@ public async Task RequestDelegatePopulatesUnattributedTryParsableParametersFromQ Assert.Equal(expectedParameterValue, httpContext.Items["tryParsable"]); } - [Theory] - [MemberData(nameof(TryParsableParameters))] - public async Task RequestDelegatePopulatesUnattributedTryParsableParametersFromRouteValueBeforeQueryString(Delegate action, string? routeValue, object? expectedParameterValue) + [Fact] + public async Task RequestDelegatePopulatesUnattributedTryParsableParametersFromRouteValueBeforeQueryString() { var httpContext = new DefaultHttpContext(); - httpContext.Request.RouteValues["tryParsable"] = routeValue; + + httpContext.Request.RouteValues["tryParsable"] = "42"; httpContext.Request.Query = new QueryCollection(new Dictionary { ["tryParsable"] = "invalid!" }); - var requestDelegate = RequestDelegateFactory.Create(action); + var requestDelegate = RequestDelegateFactory.Create((Action)((httpContext, tryParsable) => + { + httpContext.Items["tryParsable"] = tryParsable; + })); await requestDelegate(httpContext); - Assert.Equal(expectedParameterValue, httpContext.Items["tryParsable"]); + Assert.Equal(42, httpContext.Items["tryParsable"]); } public static object?[][] DelegatesWithInvalidAttributes @@ -469,24 +471,22 @@ public async Task RequestDelegateLogsTryParsableFailuresAsDebugAndSets400Respons { var invoked = false; - var sink = new TestSink(context => context.LoggerName == "Microsoft.AspNetCore.Http.RequestDelegateFactory"); - var testLoggerFactory = new TestLoggerFactory(sink, enabled: true); - - void TestAction([FromRoute] int tryParsable) + void TestAction([FromRoute] int tryParsable, [FromRoute] int tryParsable2) { invoked = true; } var invalidDataException = new InvalidDataException(); var serviceCollection = new ServiceCollection(); - serviceCollection.AddSingleton(testLoggerFactory); + serviceCollection.AddSingleton(LoggerFactory); var httpContext = new DefaultHttpContext(); httpContext.Request.RouteValues["tryParsable"] = "invalid!"; + httpContext.Request.RouteValues["tryParsable2"] = "invalid again!"; httpContext.Features.Set(new TestHttpRequestLifetimeFeature()); httpContext.RequestServices = serviceCollection.BuildServiceProvider(); - var requestDelegate = RequestDelegateFactory.Create((Action)TestAction); + var requestDelegate = RequestDelegateFactory.Create((Action)TestAction); await requestDelegate(httpContext); @@ -494,10 +494,17 @@ void TestAction([FromRoute] int tryParsable) Assert.False(httpContext.RequestAborted.IsCancellationRequested); Assert.Equal(400, httpContext.Response.StatusCode); - var log = Assert.Single(sink.Writes); - Assert.Equal(new EventId(3, "ParamaterBindingFailed"), log.EventId); - Assert.Equal(LogLevel.Debug, log.LogLevel); - Assert.Equal(@"Failed to bind parameter ""Int32 tryParsable"" from ""invalid!"".", log.Message); + var logs = TestSink.Writes.ToArray(); + + Assert.Equal(2, logs.Length); + + Assert.Equal(new EventId(3, "ParamaterBindingFailed"), logs[0].EventId); + Assert.Equal(LogLevel.Debug, logs[0].LogLevel); + Assert.Equal(@"Failed to bind parameter ""Int32 tryParsable"" from ""invalid!"".", logs[0].Message); + + Assert.Equal(new EventId(3, "ParamaterBindingFailed"), logs[0].EventId); + Assert.Equal(LogLevel.Debug, logs[0].LogLevel); + Assert.Equal(@"Failed to bind parameter ""Int32 tryParsable2"" from ""invalid again!"".", logs[1].Message); } [Fact] @@ -646,9 +653,6 @@ public async Task RequestDelegateLogsFromBodyIOExceptionsAsDebugAndDoesNotAbort( { var invoked = false; - var sink = new TestSink(context => context.LoggerName == "Microsoft.AspNetCore.Http.RequestDelegateFactory"); - var testLoggerFactory = new TestLoggerFactory(sink, enabled: true); - void TestAction([FromBody] Todo todo) { invoked = true; @@ -656,7 +660,7 @@ void TestAction([FromBody] Todo todo) var ioException = new IOException(); var serviceCollection = new ServiceCollection(); - serviceCollection.AddSingleton(testLoggerFactory); + serviceCollection.AddSingleton(LoggerFactory); var httpContext = new DefaultHttpContext(); httpContext.Request.Headers["Content-Type"] = "application/json"; @@ -671,7 +675,7 @@ void TestAction([FromBody] Todo todo) Assert.False(invoked); Assert.False(httpContext.RequestAborted.IsCancellationRequested); - var logMessage = Assert.Single(sink.Writes); + var logMessage = Assert.Single(TestSink.Writes); Assert.Equal(new EventId(1, "RequestBodyIOException"), logMessage.EventId); Assert.Equal(LogLevel.Debug, logMessage.LogLevel); Assert.Same(ioException, logMessage.Exception); @@ -682,9 +686,6 @@ public async Task RequestDelegateLogsFromBodyInvalidDataExceptionsAsDebugAndSets { var invoked = false; - var sink = new TestSink(context => context.LoggerName == "Microsoft.AspNetCore.Http.RequestDelegateFactory"); - var testLoggerFactory = new TestLoggerFactory(sink, enabled: true); - void TestAction([FromBody] Todo todo) { invoked = true; @@ -692,7 +693,7 @@ void TestAction([FromBody] Todo todo) var invalidDataException = new InvalidDataException(); var serviceCollection = new ServiceCollection(); - serviceCollection.AddSingleton(testLoggerFactory); + serviceCollection.AddSingleton(LoggerFactory); var httpContext = new DefaultHttpContext(); httpContext.Request.Headers["Content-Type"] = "application/json"; @@ -708,7 +709,7 @@ void TestAction([FromBody] Todo todo) Assert.False(httpContext.RequestAborted.IsCancellationRequested); Assert.Equal(400, httpContext.Response.StatusCode); - var logMessage = Assert.Single(sink.Writes); + var logMessage = Assert.Single(TestSink.Writes); Assert.Equal(new EventId(2, "RequestBodyInvalidDataException"), logMessage.EventId); Assert.Equal(LogLevel.Debug, logMessage.LogLevel); Assert.Same(invalidDataException, logMessage.Exception); @@ -747,9 +748,6 @@ public async Task RequestDelegateLogsFromFormIOExceptionsAsDebugAndAborts() { var invoked = false; - var sink = new TestSink(context => context.LoggerName == "Microsoft.AspNetCore.Http.RequestDelegateFactory"); - var testLoggerFactory = new TestLoggerFactory(sink, enabled: true); - void TestAction([FromForm] int value) { invoked = true; @@ -757,7 +755,7 @@ void TestAction([FromForm] int value) var ioException = new IOException(); var serviceCollection = new ServiceCollection(); - serviceCollection.AddSingleton(testLoggerFactory); + serviceCollection.AddSingleton(LoggerFactory); var httpContext = new DefaultHttpContext(); httpContext.Request.Headers["Content-Type"] = "application/x-www-form-urlencoded"; @@ -772,7 +770,7 @@ void TestAction([FromForm] int value) Assert.False(invoked); Assert.True(httpContext.RequestAborted.IsCancellationRequested); - var logMessage = Assert.Single(sink.Writes); + var logMessage = Assert.Single(TestSink.Writes); Assert.Equal(new EventId(1, "RequestBodyIOException"), logMessage.EventId); Assert.Equal(LogLevel.Debug, logMessage.LogLevel); Assert.Same(ioException, logMessage.Exception); @@ -783,9 +781,6 @@ public async Task RequestDelegateLogsFromFormInvalidDataExceptionsAsDebugAndSets { var invoked = false; - var sink = new TestSink(context => context.LoggerName == "Microsoft.AspNetCore.Http.RequestDelegateFactory"); - var testLoggerFactory = new TestLoggerFactory(sink, enabled: true); - void TestAction([FromForm] int value) { invoked = true; @@ -793,7 +788,7 @@ void TestAction([FromForm] int value) var invalidDataException = new InvalidDataException(); var serviceCollection = new ServiceCollection(); - serviceCollection.AddSingleton(testLoggerFactory); + serviceCollection.AddSingleton(LoggerFactory); var httpContext = new DefaultHttpContext(); httpContext.Request.Headers["Content-Type"] = "application/x-www-form-urlencoded"; @@ -809,7 +804,7 @@ void TestAction([FromForm] int value) Assert.False(httpContext.RequestAborted.IsCancellationRequested); Assert.Equal(400, httpContext.Response.StatusCode); - var logMessage = Assert.Single(sink.Writes); + var logMessage = Assert.Single(TestSink.Writes); Assert.Equal(new EventId(2, "RequestBodyInvalidDataException"), logMessage.EventId); Assert.Equal(LogLevel.Debug, logMessage.LogLevel); Assert.Same(invalidDataException, logMessage.Exception); From 1aabf295be99483342983cbce8645fc2e3efde63 Mon Sep 17 00:00:00 2001 From: Stephen Halter Date: Wed, 7 Apr 2021 15:49:22 -0700 Subject: [PATCH 28/35] Add CreateThrowsInvalidOperationExceptionGivenUnnamedArgument --- .../Http.Extensions/src/RequestDelegateFactory.cs | 3 +-- .../test/RequestDelegateFactoryTests.cs | 12 +++++++++++- 2 files changed, 12 insertions(+), 3 deletions(-) diff --git a/src/Http/Http.Extensions/src/RequestDelegateFactory.cs b/src/Http/Http.Extensions/src/RequestDelegateFactory.cs index 2e4e0e294ba1..2dac22724fe7 100644 --- a/src/Http/Http.Extensions/src/RequestDelegateFactory.cs +++ b/src/Http/Http.Extensions/src/RequestDelegateFactory.cs @@ -180,8 +180,7 @@ private static Expression CreateArgument(ParameterInfo parameter, FactoryContext { if (parameter.Name is null) { - // TODO: Add test! - throw new InvalidOperationException($"Parameter {parameter} does not have a name! All parameters must be named."); + throw new InvalidOperationException("A parameter does not have a name! Was it genererated? All parameters must be named."); } var parameterCustomAttributes = parameter.GetCustomAttributes(); diff --git a/src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs b/src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs index 8ba72351f97c..6b4182c49c81 100644 --- a/src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs +++ b/src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs @@ -7,6 +7,7 @@ using System.Collections.Generic; using System.Globalization; using System.IO; +using System.Linq.Expressions; using System.Net; using System.Net.Sockets; using System.Numerics; @@ -439,7 +440,7 @@ public async Task RequestDelegatePopulatesUnattributedTryParsableParametersFromR Assert.Equal(42, httpContext.Items["tryParsable"]); } - public static object?[][] DelegatesWithInvalidAttributes + public static object[][] DelegatesWithInvalidAttributes { get { @@ -466,6 +467,15 @@ public void CreateThrowsInvalidOperationExceptionWhenAttributeRequiresTryParseMe Assert.Equal("No public static bool Object.TryParse(string, out Object) method found for notTryParsable.", ex.Message); } + [Fact] + public void CreateThrowsInvalidOperationExceptionGivenUnnamedArgument() + { + var unnamedParameter = Expression.Parameter(typeof(int)); + var lambda = Expression.Lambda(Expression.Block(), unnamedParameter); + var ex = Assert.Throws(() => RequestDelegateFactory.Create((Action)lambda.Compile())); + Assert.Equal("A parameter does not have a name! Was it genererated? All parameters must be named.", ex.Message); + } + [Fact] public async Task RequestDelegateLogsTryParsableFailuresAsDebugAndSets400Response() { From 25e98badc511a568df815c28305539d358de4312 Mon Sep 17 00:00:00 2001 From: Stephen Halter Date: Wed, 7 Apr 2021 16:30:15 -0700 Subject: [PATCH 29/35] Add tempSourceString to avoid reevaluation --- .../src/RequestDelegateFactory.cs | 128 ++++++++++-------- 1 file changed, 71 insertions(+), 57 deletions(-) diff --git a/src/Http/Http.Extensions/src/RequestDelegateFactory.cs b/src/Http/Http.Extensions/src/RequestDelegateFactory.cs index 2dac22724fe7..419f2c03ff75 100644 --- a/src/Http/Http.Extensions/src/RequestDelegateFactory.cs +++ b/src/Http/Http.Extensions/src/RequestDelegateFactory.cs @@ -40,6 +40,7 @@ public static class RequestDelegateFactory private static readonly ParameterExpression HttpContextExpr = Expression.Parameter(typeof(HttpContext), "httpContext"); private static readonly ParameterExpression BodyValueExpr = Expression.Parameter(typeof(object), "bodyValue"); private static readonly ParameterExpression WasTryParseFailureExpr = Expression.Variable(typeof(bool), "wasTryParseFailure"); + private static readonly ParameterExpression TempSourceStringExpr = Expression.Variable(typeof(string), "tempSourceString"); private static readonly MemberExpression RequestServicesExpr = Expression.Property(HttpContextExpr, nameof(HttpContext.RequestServices)); private static readonly MemberExpression HttpRequestExpr = Expression.Property(HttpContextExpr, nameof(HttpContext.Request)); @@ -277,23 +278,30 @@ private static Expression CreateResponseWritingMethodCall(MethodInfo methodInfo, // If we're calling TryParse and the WasTryParseFailureVariable indicates it failed, set a 400 StatusCode instead of calling the method. private static Expression CreateTryParseCheckingResponseWritingMethodCall(MethodInfo methodInfo, Expression? target, Expression[] arguments) { + // { - // bool wasTryParseFailure = false; + // bool wasTryParseFailureVariable = false; + // string tempSourceString; // // // Assume "[FromRoute] int id" is the first parameter. - // int param1 = httpContext.Request.RouteValue["id"] == null ? default : + // + // tempSourceString = httpContext.RequestValue["id"]; + // + // int param1 = tempSourceString == null ? default : // { - // var sourceValue = httpContext.Request.RouteValue["id"]; // int parsedValue = default; // - // if (!int.TryParse(sourceValue, out parsedValue)) + // if (!int.TryParse(tempSourceString, out parsedValue)) // { - // wasTryParseFailure = true; + // wasTryParseFailureVariable = true; // Log.ParameterBindingFailed(httpContext, "Int32", "id", sourceValue) // } // // return parsedValue; // }; + // + // tempSourceString = httpContext.RequestValue["param2"]; + // int param2 = tempSourceString == null ? default : // // ... // // return wasTryParseFailure ? @@ -302,13 +310,13 @@ private static Expression CreateTryParseCheckingResponseWritingMethodCall(Method // return Task.CompletedTask; // } : // { - // // Logic generated by AddResponseWritingToMethodCall() that calls action(param1, ...) + // // Logic generated by AddResponseWritingToMethodCall() that calls action(param1, parm2, ...) // }; // } var parameters = methodInfo.GetParameters(); var storedArguments = new ParameterExpression[parameters.Length]; - var localVariables = new ParameterExpression[parameters.Length + 1]; + var localVariables = new ParameterExpression[parameters.Length + 2]; for (var i = 0; i < parameters.Length; i++) { @@ -316,6 +324,7 @@ private static Expression CreateTryParseCheckingResponseWritingMethodCall(Method } localVariables[parameters.Length] = WasTryParseFailureExpr; + localVariables[parameters.Length + 1] = TempSourceStringExpr; var assignAndCall = new Expression[parameters.Length + 1]; for (var i = 0; i < parameters.Length; i++) @@ -592,74 +601,79 @@ private static Expression GetValueFromProperty(Expression sourceExpression, stri private static Expression BindParameterFromValue(ParameterInfo parameter, Expression valueExpression, FactoryContext factoryContext) { - Expression argumentExpression; - if (parameter.ParameterType == typeof(string)) { - argumentExpression = valueExpression; + return valueExpression; } - else - { - var nonNullableParameterType = Nullable.GetUnderlyingType(parameter.ParameterType) ?? parameter.ParameterType; - var tryParseMethod = FindTryParseMethod(nonNullableParameterType); - if (tryParseMethod is null) - { - throw new InvalidOperationException($"No public static bool {parameter.ParameterType.Name}.TryParse(string, out {parameter.ParameterType.Name}) method found for {parameter.Name}."); - } + var underlyingNullableType = Nullable.GetUnderlyingType(parameter.ParameterType); + var nonNullableParameterType = underlyingNullableType ?? parameter.ParameterType; + var tryParseMethod = FindTryParseMethod(nonNullableParameterType); + + if (tryParseMethod is null) + { + throw new InvalidOperationException($"No public static bool {parameter.ParameterType.Name}.TryParse(string, out {parameter.ParameterType.Name}) method found for {parameter.Name}."); + } - // bool wasTryParseFailureVariable = false; - // - // // Assume "[FromRoute] int id" is the first parameter. - // int param1 = httpContext.Request.RouteValue["id"] == null ? default : - // { - // var sourceValue = httpContext.Request.RouteValue["id"]; - // int parsedValue = default; - // - // if (!int.TryParse(sourceValue, out parsedValue)) - // { - // wasTryParseFailureVariable = true; - // Log.ParameterBindingFailed(httpContext, "Int32", "id", sourceValue) - // } - // - // return parsedValue; - // }; + // bool wasTryParseFailureVariable = false; + // string tempSourceString; + // + // // Assume "[FromRoute] int id" is the first parameter. + // + // tempSourceString = httpContext.RequestValue["id"]; + // + // int param1 = tempSourceString == null ? default : + // { + // int parsedValue = default; + // + // if (!int.TryParse(tempSourceString, out parsedValue)) + // { + // wasTryParseFailureVariable = true; + // Log.ParameterBindingFailed(httpContext, "Int32", "id", sourceValue) + // } + // + // return parsedValue; + // }; - factoryContext.CheckForTryParseFailure = true; + factoryContext.CheckForTryParseFailure = true; - var parsedValue = Expression.Variable(nonNullableParameterType); + var parsedValue = Expression.Variable(nonNullableParameterType); - var tryParseCall = Expression.Call(tryParseMethod, valueExpression, parsedValue); + var parameterTypeNameConstant = Expression.Constant(parameter.ParameterType.Name); + var parameterNameConstant = Expression.Constant(parameter.Name); - var parameterTypeConstant = Expression.Constant(nonNullableParameterType.Name); - var parameterNameConstant = Expression.Constant(parameter.Name); - var failBlock = Expression.Block( - Expression.Assign(WasTryParseFailureExpr, Expression.Constant(true)), - Expression.Call(LogParameterBindingFailureMethod, - HttpContextExpr, parameterTypeConstant, parameterNameConstant, valueExpression)); + var failBlock = Expression.Block( + Expression.Assign(WasTryParseFailureExpr, Expression.Constant(true)), + Expression.Call(LogParameterBindingFailureMethod, + HttpContextExpr, parameterTypeNameConstant, parameterNameConstant, TempSourceStringExpr)); - var ifFailExpression = Expression.IfThen(Expression.Not(tryParseCall), failBlock); + var tryParseCall = Expression.Call(tryParseMethod, TempSourceStringExpr, parsedValue); + var ifFailExpression = Expression.IfThen(Expression.Not(tryParseCall), failBlock); - argumentExpression = Expression.Block(new[] { parsedValue }, - ifFailExpression, - parsedValue); - } + Expression parsedValueExpression = Expression.Block(new[] { parsedValue }, + ifFailExpression, + parsedValue); - // Convert to nullable if necessary. - if (argumentExpression.Type != parameter.ParameterType) + // Convert back to nullable if necessary. + if (underlyingNullableType is not null) { - argumentExpression = Expression.Convert(argumentExpression, parameter.ParameterType); + parsedValueExpression = Expression.Convert(parsedValueExpression, parameter.ParameterType); } Expression defaultExpression = parameter.HasDefaultValue ? Expression.Constant(parameter.DefaultValue) : Expression.Default(parameter.ParameterType); - // int param1 = httpContext.Request.RouteValue["id"] == null ? default : ... - return Expression.Condition( - Expression.Equal(valueExpression, Expression.Constant(null)), + // tempSourceString = httpContext.RequestValue["id]; + var storeValueToTemp = Expression.Assign(TempSourceStringExpr, valueExpression); + + // int param1 = tempSourcString == null ? default : ... + var ternary = Expression.Condition( + Expression.Equal(TempSourceStringExpr, Expression.Constant(null)), defaultExpression, - argumentExpression); + parsedValueExpression); + + return Expression.Block(storeValueToTemp, ternary); } private static Expression BindParameterFromProperty(ParameterInfo parameter, MemberExpression property, string key, FactoryContext factoryContext) => @@ -828,9 +842,9 @@ public static void RequestBodyInvalidDataException(HttpContext httpContext, Inva _requestBodyInvalidDataException(GetLogger(httpContext), exception); } - public static void ParameterBindingFailed(HttpContext httpContext, string parameterType, string parameterName, string sourceValue) + public static void ParameterBindingFailed(HttpContext httpContext, string parameterTypeName, string parameterName, string sourceValue) { - _parameterBindingFailed(GetLogger(httpContext), parameterType, parameterName, sourceValue, null); + _parameterBindingFailed(GetLogger(httpContext), parameterTypeName, parameterName, sourceValue, null); } private static ILogger GetLogger(HttpContext httpContext) From 7b50d32aaa37c513e6249e2bbdc2fca5fd9144aa Mon Sep 17 00:00:00 2001 From: Stephen Halter Date: Wed, 7 Apr 2021 16:36:54 -0700 Subject: [PATCH 30/35] Add TryParseMethodCache --- .../Http.Extensions/src/RequestDelegateFactory.cs | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/src/Http/Http.Extensions/src/RequestDelegateFactory.cs b/src/Http/Http.Extensions/src/RequestDelegateFactory.cs index 419f2c03ff75..20d6a153f950 100644 --- a/src/Http/Http.Extensions/src/RequestDelegateFactory.cs +++ b/src/Http/Http.Extensions/src/RequestDelegateFactory.cs @@ -2,6 +2,7 @@ // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. using System; +using System.Collections.Concurrent; using System.Diagnostics; using System.IO; using System.Linq; @@ -53,6 +54,8 @@ public static class RequestDelegateFactory private static readonly MemberExpression StatusCodeExpr = Expression.Property(HttpResponseExpr, nameof(HttpResponse.StatusCode)); private static readonly MemberExpression CompletedTaskExpr = Expression.Property(null, (PropertyInfo)GetMemberInfo>(() => Task.CompletedTask)); + private static ConcurrentDictionary TryParseMethodCache = new(); + /// /// Creates a implementation for . /// @@ -278,7 +281,6 @@ private static Expression CreateResponseWritingMethodCall(MethodInfo methodInfo, // If we're calling TryParse and the WasTryParseFailureVariable indicates it failed, set a 400 StatusCode instead of calling the method. private static Expression CreateTryParseCheckingResponseWritingMethodCall(MethodInfo methodInfo, Expression? target, Expression[] arguments) { - // { // bool wasTryParseFailureVariable = false; // string tempSourceString; @@ -553,9 +555,8 @@ private static MethodInfo GetEnumTryParseMethod() throw new Exception("static bool System.Enum.TryParse(string? value, out TEnum result) does not exist!!?!?"); } - // TODO: Cache this. // TODO: Use InvariantCulture where possible? Or is CurrentCulture fine because it's more flexible? - private static MethodInfo? FindTryParseMethod(Type type) + private static MethodInfo? FindTryParseMethodUncached(Type type) { if (type.IsEnum) { @@ -585,6 +586,11 @@ private static MethodInfo GetEnumTryParseMethod() return null; } + private static MethodInfo? FindTryParseMethod(Type type) + { + return TryParseMethodCache.GetOrAdd(type, FindTryParseMethodUncached); + } + private static bool HasTryParseMethod(ParameterInfo parameter) { var nonNullableParameterType = Nullable.GetUnderlyingType(parameter.ParameterType) ?? parameter.ParameterType; From 4060f1b0bc01cff6377f665965714e5083d81066 Mon Sep 17 00:00:00 2001 From: Stephen Halter Date: Wed, 7 Apr 2021 17:25:39 -0700 Subject: [PATCH 31/35] oops --- .../Http.Extensions/src/RequestDelegateFactory.cs | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-) diff --git a/src/Http/Http.Extensions/src/RequestDelegateFactory.cs b/src/Http/Http.Extensions/src/RequestDelegateFactory.cs index 20d6a153f950..5b8bcd5fbd5a 100644 --- a/src/Http/Http.Extensions/src/RequestDelegateFactory.cs +++ b/src/Http/Http.Extensions/src/RequestDelegateFactory.cs @@ -84,7 +84,7 @@ public static RequestDelegate Create(Delegate action) /// /// Creates a implementation for . - /// Microsoft.AspNetCore.Routing.MapAction" + /// /// A static request handler with any number of custom parameters that often produces a response with its return value. /// The . public static RequestDelegate Create(MethodInfo methodInfo) @@ -253,11 +253,7 @@ private static Expression CreateArgument(ParameterInfo parameter, FactoryContext { return RequestAbortedExpr; } - else if (parameter.ParameterType == typeof(string)) - { - return BindParameterFromRouteValueOrQueryString(parameter, parameter.Name, factoryContext); - } - else if (HasTryParseMethod(parameter)) + else if (parameter.ParameterType == typeof(string) || HasTryParseMethod(parameter)) { return BindParameterFromRouteValueOrQueryString(parameter, parameter.Name, factoryContext); } @@ -296,7 +292,7 @@ private static Expression CreateTryParseCheckingResponseWritingMethodCall(Method // if (!int.TryParse(tempSourceString, out parsedValue)) // { // wasTryParseFailureVariable = true; - // Log.ParameterBindingFailed(httpContext, "Int32", "id", sourceValue) + // Log.ParameterBindingFailed(httpContext, "Int32", "id", tempSourceString) // } // // return parsedValue; @@ -635,7 +631,7 @@ private static Expression BindParameterFromValue(ParameterInfo parameter, Expres // if (!int.TryParse(tempSourceString, out parsedValue)) // { // wasTryParseFailureVariable = true; - // Log.ParameterBindingFailed(httpContext, "Int32", "id", sourceValue) + // Log.ParameterBindingFailed(httpContext, "Int32", "id", tempSourceString) // } // // return parsedValue; From 3fc546ebb27fdec03c4bee24b00299c97e11ce33 Mon Sep 17 00:00:00 2001 From: Stephen Halter Date: Wed, 7 Apr 2021 18:08:34 -0700 Subject: [PATCH 32/35] cleanup --- .../src/RequestDelegateFactory.cs | 50 +++++++++---------- .../test/RequestDelegateFactoryTests.cs | 29 ++--------- 2 files changed, 27 insertions(+), 52 deletions(-) diff --git a/src/Http/Http.Extensions/src/RequestDelegateFactory.cs b/src/Http/Http.Extensions/src/RequestDelegateFactory.cs index 5b8bcd5fbd5a..26dadf5dc501 100644 --- a/src/Http/Http.Extensions/src/RequestDelegateFactory.cs +++ b/src/Http/Http.Extensions/src/RequestDelegateFactory.cs @@ -284,7 +284,6 @@ private static Expression CreateTryParseCheckingResponseWritingMethodCall(Method // // Assume "[FromRoute] int id" is the first parameter. // // tempSourceString = httpContext.RequestValue["id"]; - // // int param1 = tempSourceString == null ? default : // { // int parsedValue = default; @@ -552,39 +551,39 @@ private static MethodInfo GetEnumTryParseMethod() } // TODO: Use InvariantCulture where possible? Or is CurrentCulture fine because it's more flexible? - private static MethodInfo? FindTryParseMethodUncached(Type type) + private static MethodInfo? FindTryParseMethod(Type type) { - if (type.IsEnum) - { - return EnumTryParseMethod.MakeGenericMethod(type); - } - - var staticMethods = type.GetMethods(BindingFlags.Public | BindingFlags.Static); - - foreach (var method in staticMethods) + static MethodInfo? Finder(Type type) { - if (method.Name != "TryParse" || method.ReturnType != typeof(bool)) + if (type.IsEnum) { - continue; + return EnumTryParseMethod.MakeGenericMethod(type); } - var tryParseParameters = method.GetParameters(); + var staticMethods = type.GetMethods(BindingFlags.Public | BindingFlags.Static); - if (tryParseParameters.Length == 2 && - tryParseParameters[0].ParameterType == typeof(string) && - tryParseParameters[1].IsOut && - tryParseParameters[1].ParameterType == type.MakeByRefType()) + foreach (var method in staticMethods) { - return method; + if (method.Name != "TryParse" || method.ReturnType != typeof(bool)) + { + continue; + } + + var tryParseParameters = method.GetParameters(); + + if (tryParseParameters.Length == 2 && + tryParseParameters[0].ParameterType == typeof(string) && + tryParseParameters[1].IsOut && + tryParseParameters[1].ParameterType == type.MakeByRefType()) + { + return method; + } } - } - return null; - } + return null; + } - private static MethodInfo? FindTryParseMethod(Type type) - { - return TryParseMethodCache.GetOrAdd(type, FindTryParseMethodUncached); + return TryParseMethodCache.GetOrAdd(type, Finder); } private static bool HasTryParseMethod(ParameterInfo parameter) @@ -621,7 +620,6 @@ private static Expression BindParameterFromValue(ParameterInfo parameter, Expres // string tempSourceString; // // // Assume "[FromRoute] int id" is the first parameter. - // // tempSourceString = httpContext.RequestValue["id"]; // // int param1 = tempSourceString == null ? default : @@ -666,7 +664,7 @@ private static Expression BindParameterFromValue(ParameterInfo parameter, Expres Expression.Constant(parameter.DefaultValue) : Expression.Default(parameter.ParameterType); - // tempSourceString = httpContext.RequestValue["id]; + // tempSourceString = httpContext.RequestValue["id"]; var storeValueToTemp = Expression.Assign(TempSourceStringExpr, valueExpression); // int param1 = tempSourcString == null ? default : ... diff --git a/src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs b/src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs index 6b4182c49c81..702afcbb7ac0 100644 --- a/src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs +++ b/src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs @@ -307,60 +307,37 @@ void Store(HttpContext httpContext, T tryParsable) return new[] { - // String (technically not "TryParsable", but it's the the special case) + // string is not technically "TryParsable", but it's the special case. new object[] { (Action)Store, "plain string", "plain string" }, - // Int32 new object[] { (Action)Store, "-42", -42 }, new object[] { (Action)Store, "42", 42U }, - // Byte new object[] { (Action)Store, "true", true }, - // Int16 new object[] { (Action)Store, "-42", (short)-42 }, new object[] { (Action)Store, "42", (ushort)42 }, - // Int64 new object[] { (Action)Store, "-42", -42L }, new object[] { (Action)Store, "42", 42UL }, - // IntPtr new object[] { (Action)Store, "-42", new IntPtr(-42) }, - // Char new object[] { (Action)Store, "A", 'A' }, - // Double new object[] { (Action)Store, "0.5", 0.5 }, - // Single new object[] { (Action)Store, "0.5", 0.5f }, - // Half new object[] { (Action)Store, "0.5", (Half)0.5f }, - // Decimal new object[] { (Action)Store, "0.5", 0.5m }, - // DateTime new object[] { (Action)Store, now.ToString("o"), now }, - // DateTimeOffset new object[] { (Action)Store, nowOffset.ToString("o"), nowOffset }, - // TimeSpan new object[] { (Action)Store, TimeSpan.FromSeconds(42).ToString(), TimeSpan.FromSeconds(42) }, - // Guid new object[] { (Action)Store, guid.ToString(), guid }, - // Version new object[] { (Action)Store, "6.0.0.42", new Version("6.0.0.42") }, - // BigInteger new object[] { (Action)Store, "-42", new BigInteger(-42) }, - // IPAddress new object[] { (Action)Store, "127.0.0.1", IPAddress.Loopback }, - // IPEndPoint new object[] { (Action)Store, "127.0.0.1:80", new IPEndPoint(IPAddress.Loopback, 80) }, - // System Enums new object[] { (Action)Store, "Unix", AddressFamily.Unix }, new object[] { (Action)Store, "Nop", ILOpCode.Nop }, new object[] { (Action)Store, "PublicKey,Retargetable", AssemblyFlags.PublicKey | AssemblyFlags.Retargetable }, - // Nullable new object[] { (Action)Store, "42", 42 }, - // Null route and/or query value gives default value. - new object?[] { (Action)Store, null, 0 }, - new object?[] { (Action)Store, null, null }, - // Custom Enum new object[] { (Action)Store, "ValueB", MyEnum.ValueB }, - // Custom record with static TryParse method! new object[] { (Action)Store, "42", new MyTryParsableRecord(42) }, + new object?[] { (Action)Store, null, 0 }, + new object?[] { (Action)Store, null, null }, }; } } From d3cf3a86d9d58d25192e74b4e0e57540494d1a30 Mon Sep 17 00:00:00 2001 From: Stephen Halter Date: Thu, 8 Apr 2021 00:43:33 -0700 Subject: [PATCH 33/35] simplify test cases --- src/Http/Http.Extensions/src/RequestDelegateFactory.cs | 2 +- .../test/RequestDelegateFactoryTests.cs | 10 ++++------ 2 files changed, 5 insertions(+), 7 deletions(-) diff --git a/src/Http/Http.Extensions/src/RequestDelegateFactory.cs b/src/Http/Http.Extensions/src/RequestDelegateFactory.cs index 26dadf5dc501..b0c92289d802 100644 --- a/src/Http/Http.Extensions/src/RequestDelegateFactory.cs +++ b/src/Http/Http.Extensions/src/RequestDelegateFactory.cs @@ -54,7 +54,7 @@ public static class RequestDelegateFactory private static readonly MemberExpression StatusCodeExpr = Expression.Property(HttpResponseExpr, nameof(HttpResponse.StatusCode)); private static readonly MemberExpression CompletedTaskExpr = Expression.Property(null, (PropertyInfo)GetMemberInfo>(() => Task.CompletedTask)); - private static ConcurrentDictionary TryParseMethodCache = new(); + private static readonly ConcurrentDictionary TryParseMethodCache = new(); /// /// Creates a implementation for . diff --git a/src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs b/src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs index 702afcbb7ac0..e849c887c6d3 100644 --- a/src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs +++ b/src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs @@ -296,14 +296,12 @@ public static object?[][] TryParsableParameters { get { - void Store(HttpContext httpContext, T tryParsable) + static void Store(HttpContext httpContext, T tryParsable) { httpContext.Items["tryParsable"] = tryParsable; } var now = DateTime.Now; - var nowOffset = DateTimeOffset.UtcNow; - var guid = Guid.NewGuid(); return new[] { @@ -323,9 +321,9 @@ void Store(HttpContext httpContext, T tryParsable) new object[] { (Action)Store, "0.5", (Half)0.5f }, new object[] { (Action)Store, "0.5", 0.5m }, new object[] { (Action)Store, now.ToString("o"), now }, - new object[] { (Action)Store, nowOffset.ToString("o"), nowOffset }, - new object[] { (Action)Store, TimeSpan.FromSeconds(42).ToString(), TimeSpan.FromSeconds(42) }, - new object[] { (Action)Store, guid.ToString(), guid }, + new object[] { (Action)Store, "1970-01-01T00:00:00.0000000+00:00", DateTimeOffset.UnixEpoch }, + new object[] { (Action)Store, "00:00:42", TimeSpan.FromSeconds(42) }, + new object[] { (Action)Store, "00000000-0000-0000-0000-000000000000", Guid.Empty }, new object[] { (Action)Store, "6.0.0.42", new Version("6.0.0.42") }, new object[] { (Action)Store, "-42", new BigInteger(-42) }, new object[] { (Action)Store, "127.0.0.1", IPAddress.Loopback }, From 1b2e1a292af636bc25747a5706f77f8baba5574b Mon Sep 17 00:00:00 2001 From: Stephen Halter Date: Thu, 8 Apr 2021 00:59:02 -0700 Subject: [PATCH 34/35] wasTryParseFailureVariable -> wasTryParseFailure --- src/Http/Http.Extensions/src/RequestDelegateFactory.cs | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/Http/Http.Extensions/src/RequestDelegateFactory.cs b/src/Http/Http.Extensions/src/RequestDelegateFactory.cs index b0c92289d802..d96cddbdfc69 100644 --- a/src/Http/Http.Extensions/src/RequestDelegateFactory.cs +++ b/src/Http/Http.Extensions/src/RequestDelegateFactory.cs @@ -274,11 +274,11 @@ private static Expression CreateResponseWritingMethodCall(MethodInfo methodInfo, return AddResponseWritingToMethodCall(callMethod, methodInfo.ReturnType); } - // If we're calling TryParse and the WasTryParseFailureVariable indicates it failed, set a 400 StatusCode instead of calling the method. + // If we're calling TryParse and wasTryParseFailure indicates it failed, set a 400 StatusCode instead of calling the method. private static Expression CreateTryParseCheckingResponseWritingMethodCall(MethodInfo methodInfo, Expression? target, Expression[] arguments) { // { - // bool wasTryParseFailureVariable = false; + // bool wasTryParseFailure = false; // string tempSourceString; // // // Assume "[FromRoute] int id" is the first parameter. @@ -290,7 +290,7 @@ private static Expression CreateTryParseCheckingResponseWritingMethodCall(Method // // if (!int.TryParse(tempSourceString, out parsedValue)) // { - // wasTryParseFailureVariable = true; + // wasTryParseFailure = true; // Log.ParameterBindingFailed(httpContext, "Int32", "id", tempSourceString) // } // @@ -616,7 +616,7 @@ private static Expression BindParameterFromValue(ParameterInfo parameter, Expres throw new InvalidOperationException($"No public static bool {parameter.ParameterType.Name}.TryParse(string, out {parameter.ParameterType.Name}) method found for {parameter.Name}."); } - // bool wasTryParseFailureVariable = false; + // bool wasTryParseFailure = false; // string tempSourceString; // // // Assume "[FromRoute] int id" is the first parameter. @@ -628,7 +628,7 @@ private static Expression BindParameterFromValue(ParameterInfo parameter, Expres // // if (!int.TryParse(tempSourceString, out parsedValue)) // { - // wasTryParseFailureVariable = true; + // wasTryParseFailure = true; // Log.ParameterBindingFailed(httpContext, "Int32", "id", tempSourceString) // } // From 8e53897b6f55b861ca60a6595cacbb09a57c4725 Mon Sep 17 00:00:00 2001 From: Stephen Halter Date: Thu, 8 Apr 2021 17:02:58 -0700 Subject: [PATCH 35/35] Use Uri in MyTryParsableRecord test case --- .../test/RequestDelegateFactoryTests.cs | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs b/src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs index e849c887c6d3..80e4532746fe 100644 --- a/src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs +++ b/src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs @@ -333,7 +333,7 @@ static void Store(HttpContext httpContext, T tryParsable) new object[] { (Action)Store, "PublicKey,Retargetable", AssemblyFlags.PublicKey | AssemblyFlags.Retargetable }, new object[] { (Action)Store, "42", 42 }, new object[] { (Action)Store, "ValueB", MyEnum.ValueB }, - new object[] { (Action)Store, "42", new MyTryParsableRecord(42) }, + new object[] { (Action)Store, "https://example.org", new MyTryParsableRecord(new Uri("https://example.org")) }, new object?[] { (Action)Store, null, 0 }, new object?[] { (Action)Store, null, null }, }; @@ -342,17 +342,17 @@ static void Store(HttpContext httpContext, T tryParsable) private enum MyEnum { ValueA, ValueB, } - private record MyTryParsableRecord(int State) + private record MyTryParsableRecord(Uri Uri) { - public static bool TryParse(string value, out MyTryParsableRecord? result) + public static bool TryParse(string? value, out MyTryParsableRecord? result) { - if (!int.TryParse(value, out var state)) + if (!Uri.TryCreate(value, UriKind.Absolute, out var uri)) { result = null; return false; } - result = new MyTryParsableRecord(state); + result = new MyTryParsableRecord(uri); return true; } }