Allowing empty FromBody parameters input based on nullability

[brunolins16]

• You've read the Contributor Guide and Code of Conduct.
• You've included unit or integration tests for your change, where applicable.
• You've included inline docs for your change, where applicable.
• There's an open issue for the PR that you are making. If you'd like to propose a new feature or change, please open an issue to discuss the change or find an existing issue.

Description

This PR introduces two breaking changes:

Empty Input body calculation (a5a47b0)

The biggest change is, in most cases, the Content-Length == null will now be detected as Empty Body.

aspnetcore/src/Http/Http.Features/src/IHttpRequestBodyDetectionFeature.cs

Line 11 in 15fa3ad

/// <summary>

Before:

csharp request.ContentLength == 0

After:

var hasBody = context.HttpContext.Features.Get<IHttpRequestBodyDetectionFeature>()?.CanHaveBody;
// In case the feature is not registered
hasBody ??= context.HttpContext.Request.ContentLength != 0;

hasBody == false

Allowing empty FromBody parameters input based on nullability

As described in the #39754 some parameters will now be allowed as empty input parameter.

Fixes #39754 #29570

[brunolins16]

/benchmark

[BrennanConroy]

That's not worked yet

Edit: Nvm?

[pr-benchmarks]

Crank - Pull Request Bot

/benchmark <benchmarks[,...]> <profiles[,...]> <components,[...]>

Benchmarks:

• plaintext: TechEmpower Plaintext Scenario - ASP.NET Platform implementation
• json: TechEmpower JSON Scenario - ASP.NET Platform implementation
• fortunes: TechEmpower Fortunes Scenario - ASP.NET Platform implementation
• yarp: YARP - http-http with 10 bytes

Profiles:

• aspnet-perf-lin: INTEL/Linux 12 Cores
• aspnet-perf-win: INTEL/Windows 12 Cores
• aspnet-citrine-lin: INTEL/Linux 28 Cores
• aspnet-citrine-win: INTEL/Windows 28 Cores
• aspnet-citrine-arm: ARM/Linux 32 Cores
• aspnet-citrine-amd: AMD/Linux 48 Cores

Components:

• kestrel
• mvc

[brunolins16]

/benchmark plaintext aspnet-perf-win,aspnet-perf-lin mvc

[pr-benchmarks]

Benchmark started for plaintext on aspnet-perf-win, aspnet-perf-lin with mvc

[pr-benchmarks]

Failed to benchmark PR #39917. Skipping... Details:

System.Net.Http.HttpRequestException: Response status code does not indicate success: 404 (There are no listeners connected for the endpoint. TrackingId:7703b534-a65c-43c1-b144-fbd0735e4e15_G69, SystemTracker:aspnetperf.servicebus.windows.net:perfwin/jobs, Timestamp:2022-02-02T01:10:45).
   at System.Net.Http.HttpResponseMessage.EnsureSuccessStatusCode()
   at Microsoft.Crank.Controller.JobConnection.GetQueueAsync() in D:\a\_work\1\s\crank\src\Microsoft.Crank.Controller\JobConnection.cs:line 1149
   at Microsoft.Crank.Controller.Program.<>c__DisplayClass45_1.<<Run>b__6>d.MoveNext() in D:\a\_work\1\s\crank\src\Microsoft.Crank.Controller\Program.cs:line 676
--- End of stack trace from previous location ---
   at Microsoft.Crank.Controller.Program.Run(Configuration configuration, String[] dependencies, String session, Int32 iterations, ExcludeOptions exclude, IEnumerable`1 scripts) in D:\a\_work\1\s\crank\src\Microsoft.Crank.Controller\Program.cs:line 672
   at Microsoft.Crank.Controller.Program.<>c__DisplayClass44_0.<<Main>b__3>d.MoveNext() in D:\a\_work\1\s\crank\src\Microsoft.Crank.Controller\Program.cs:line 542
--- End of stack trace from previous location ---
   at McMaster.Extensions.CommandLineUtils.CommandLineApplication.ExecuteAsync(String[] args, CancellationToken cancellationToken)
   at McMaster.Extensions.CommandLineUtils.CommandLineApplication.Execute(String[] args)
   at Microsoft.Crank.Controller.Program.Main(String[] args)
   at Microsoft.Crank.PullRequestBot.Program.RunCrank(String[] args) in D:\a\_work\1\s\crank\src\Microsoft.Crank.PullRequestBot\Program.cs:line 627
   at Microsoft.Crank.PullRequestBot.Program.RunBenchmark(Command command) in D:\a\_work\1\s\crank\src\Microsoft.Crank.PullRequestBot\Program.cs:line 575
   at Microsoft.Crank.PullRequestBot.Program.RunBenchmark(Command command) in D:\a\_work\1\s\crank\src\Microsoft.Crank.PullRequestBot\Program.cs:line 609
   at Microsoft.Crank.PullRequestBot.Program.Controller(BotOptions options) in D:\a\_work\1\s\crank\src\Microsoft.Crank.PullRequestBot\Program.cs:line 252

[halter73]

/benchmark plaintext aspnet-perf-win,aspnet-perf-lin mvc

The mvc at the end of the command tells the bot to build MVC, but it's still running the plaintext benchmark specified at the beginning which is pure middleware.

AFAICT, the only per-request change is the change to InputFormatter to use IHttpRequestBodyDetectionFeature. @sebastienros It looks like we need to add the mvc/jsoninput scenario to the bot. Kestrel's IHttpRequestBodyDetectionFeature implementation is very efficient, so I don't expect to see anything though.

[sebastienros]

/benchmark mvcjsoninput2k aspnet-perf-lin mvc

[pr-benchmarks]

Benchmark started for mvcjsoninput2k on aspnet-perf-lin with mvc

[pr-benchmarks]

mvcjsoninput2k - aspnet-perf-lin

application	base	head
CPU Usage (%)	99	99	0.00%
Cores usage (%)	1,187	1,189	+0.17%
Working Set (MB)	186	187	+0.54%
Private Memory (MB)	926	960	+3.67%
Build Time (ms)	9,285	1,661	-82.11%
Start Time (ms)	217	231	+6.45%
Published Size (KB)	92,009	92,009	0.00%
.NET Core SDK Version	7.0.100-preview.2.22102.6	7.0.100-preview.2.22102.6

load	base	head
CPU Usage (%)	28	27	-3.57%
Cores usage (%)	335	328	-2.09%
Working Set (MB)	48	48	0.00%
Private Memory (MB)	358	358	0.00%
Build Time (ms)	5,139	0
Start Time (ms)	0	0
Published Size (KB)	76,413	0
.NET Core SDK Version	3.1.416	0
First Request (ms)	137	135	-1.46%
Requests/sec	123,976	124,070	+0.08%
Requests	1,871,923	1,873,343	+0.08%
Mean latency (ms)	2.10	2.11	+0.48%
Max latency (ms)	53.39	45.38	-15.00%
Bad responses	0	0
Socket errors	0	0
Read throughput (MB/s)	10.88	10.89	+0.09%
Latency 50th (ms)	2.03	2.03	0.00%
Latency 75th (ms)	2.27	2.26	-0.44%
Latency 90th (ms)	2.56	2.55	-0.39%
Latency 99th (ms)	3.81	3.79	-0.52%

[davidfowl]

nit:

Suggested change

	if (hasBody == false)
	if (!hasBody)

[halter73]

It's a Nullable<bool>. This is basically a saying "if we know for sure this request cannot have a body ..." Probably worth a comment.

[pranavkm]

I'd look at using ParameterDefaultValue - it might be that the bugs it was trying to workaround have since been resolved, but everything else looks great. Thanks for driving this!

[pranavkm]

This is a bad test because it's representing test branches as data. I realize you didn't write it, but could we split this in to two tests?

[brunolins16]

I have split the test into FromBodyWithEmptyBody_ModelStateIsInvalid_HasModelErrors and FromBodyWithEmptyBody_ModelStateIsValid_WhenAllowEmptyInput, is that good?