Add support for optional FromBody parameters

[pranavkm]

Fixes #6878

---

public enum EmptyBodyBehavior
{
    Default = 0,
    Allow = 1,
    Disallow = 2,
}

public partial class FromBodyAttribute : System.Attribute, Microsoft.AspNetCore.Mvc.ModelBinding.IBindingSourceMetadata
{
      public FromBodyAttribute() { }
+     public EmptyBodyBehavior EmptyBodyBehavior { get { throw null; } set { } }
}

public partial class BindingInfo
{
    public BindingInfo() { }
    public BindingInfo(Microsoft.AspNetCore.Mvc.ModelBinding.BindingInfo other) { }
+   public EmptyBodyBehavior EmptyBodyBehavior { [System.Runtime.CompilerServices.CompilerGeneratedAttribute] get { throw null; } [System.Runtime.CompilerServices.CompilerGeneratedAttribute] set { } }
}

public partial class ApiParameterDescription
{
    public ApiParameterDescription() { }
+    public Microsoft.AspNetCore.Mvc.ModelBinding.BindingInfo BindingInfo { [System.Runtime.CompilerServices.CompilerGeneratedAttribute] get { throw null; } [System.Runtime.CompilerServices.CompilerGeneratedAttribute] set { } }

[pranavkm]

I'll fix the source build. Just wanted to vet the design.

[javiercn]

Changes look solid, I would need to look a bit more in depth to see if we are missing something since I'm not an expert in this area.

[dougbu]

Please change the IAllowEmptyInputInBodyModelBinding definition.

More generally, I'm unsure whether this approach is better or worse than a new MvcOption.AllowEmptyInputInBodyModelBindingIfSet option. We could implement that using ApiParameterDescription.DefaultValue.

[dougbu]

Suggest the property in IAllowEmptyInputInBodyModelBinding should be non-nullable too. I don't see a use case for a tri-state property here.

[pranavkm]

I was looking at being able to distinguish these two scenario:

.AddControllers(options => options.AllowEmptyInputInBodyModelBinding = true);
...

public IActionResult Post([FromBody(AllowEmptyInputInBodyModelBinding = false)] /* this API always require a body */ LoginModel model)

public IActionResult Post([FromBody] /* I'm ok with whatever was configured in MvcOptions */ CommentModel model)

[dougbu]

Then this property should be of type bool?

[pranavkm]

Unfortunately attributes cannot have nullable properties that can be assigned to: https://docs.microsoft.com/en-us/dotnet/csharp/language-reference/language-specification/attributes#attribute-parameter-types. We could create an enum type to represent it. While it might be more correct to have an explicit distinction between fallback to 'system default' vs 'explicitly false', users don't really query the attribute so it seems like overkill.

[dougbu]

Perhaps overkill but the tri-state approach you want won't work as long as the only IAllowEmptyInputInBodyModelBinding implementation has a non-nullable bool.

[pranavkm]

The interface IAllowEmptyInputInBodyModelBinding is non-public. It's largely a result of weird layering - BindingInfo is in Abstractions, FromBody is in Mvc.Core. We're free to assign it the shape we think works best.

[pranavkm]

[FromBody(AllowEmpty = AllowEmptyBodyBehavior.Default)]
[FromBody(AllowEmpty = AllowEmptyBodyBehavior.Reject)]
[FromBody(AllowEmpty = AllowEmptyBodyBehavior.Allow)]

enum AllowEmptyBodyBehavior
{
  Default,
  Reject,
  Allow
}

[dougbu]

Why the reordering?

[pranavkm]

Messed up. Will undo

[pranavkm]

Thanks both of you for the quick review. As @dougbu, relying on the default value of the parameters would have been super nice and most intuitive. I had hoped that's what we could do but there are a few issues with it that made me use this approach:

a) We don't currently have access to parameter values in the binder \ binder factory. The best way I could think of resolving this is by adding bool HasDefaultValue \ object? DefaultValue to ModelMetadata. We could infer it when constructing metadata for parameters. Unfortunately, this leaves us with:

b) Relying on default parameter values alone doesn't help solve it for properties. It would be nice to solve the problem for both kinds of bound types - parameters and properties.

Using a property was something a user suggested - aspnet/Mvc#6920 (comment) and it has a fair number of upvotes so it doesn't seem entirely unappealing.

[dougbu]

@pranavkm got it, thanks for the explanation. That just leaves the IAllowEmptyInputInBodyModelBinding inconsistency from my PoV.

[ghost]

Thank you for submitting this for API review. This will be reviewed by @dotnet/aspnet-api-review at the next meeting of the ASP.NET Core API Review group. Please ensure you take a look at the API review process documentation and ensure that:

• The PR contains changes to the reference-assembly that describe the API change. Or, you have included a snippet of reference-assembly-style code that illustrates the API change.
• The PR describes the impact to users, both positive (useful new APIs) and negative (breaking changes).
• Someone is assigned to "champion" this change in the meeting, and they understand the impact and design of the change.

[ghost]

Hello @pranavkm!

Because this pull request has the auto-merge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

[dougbu]

One teensy fixup…

[dougbu]

End comment with another period