Add support for optional FromBody parameters #22634

pranavkm · 2020-06-06T22:31:14Z

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 · 2020-06-08T17:50:29Z

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 · 2020-06-08T20:01:51Z

src/Mvc/Mvc.Core/src/FromBodyAttribute.cs

+        /// <remarks>
+        /// When configured, takes precedence over <see cref="MvcOptions.AllowEmptyInputInBodyModelBinding"/>.
+        /// </remarks>
+        public bool AllowEmptyInputInBodyModelBinding


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

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)

Then this property should be of type bool?

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.

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

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.

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

enum AllowEmptyBodyBehavior { Default, Reject, Allow }

dougbu · 2020-06-08T20:03:11Z

src/Mvc/Mvc.Core/src/ModelBinding/Binders/BodyModelBinder.cs


            _options = options;
+            _formatters = formatters;
+            _readerFactory = readerFactory.CreateReader;


Why the reordering?

Messed up. Will undo

src/Mvc/test/Mvc.FunctionalTests/InputFormatterTests.cs

pranavkm · 2020-06-08T22:36:10Z

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 · 2020-06-09T00:03:39Z

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

ghost · 2020-06-10T22:11:18Z

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.

src/Mvc/Mvc.Abstractions/ref/Microsoft.AspNetCore.Mvc.Abstractions.netcoreapp.cs

Fixes #6878

ghost · 2020-06-22T17:58:03Z

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 · 2020-06-22T19:09:53Z

src/Mvc/Mvc.Abstractions/src/ModelBinding/EmptyBodyBehavior.cs

+    {
+        /// <summary>
+        /// Uses the framework default behavior for processing empty bodies.
+        /// This is typically configured using <c>MvcOptions.AllowEmptyInputInBodyModelBinding</c>


End comment with another period

* Add support for optional FromBody parameters Fixes #6878 * Fixup nullable * Changes per API review

* Add support for optional FromBody parameters (#22634) * Add support for optional FromBody parameters Fixes #6878 * Fixup nullable * Changes per API review * Fixup doc comment (#23229)

ghost added the area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates label Jun 6, 2020

pranavkm force-pushed the prkrishn/6878 branch 5 times, most recently from 7c8d4ea to d57ed85 Compare June 7, 2020 01:11

jaredpar mentioned this pull request Jun 8, 2020

OSX machines are de-provisioned during CI / PR runs leading to failures dotnet/runtime#34472

Closed

pranavkm marked this pull request as ready for review June 8, 2020 17:49

pranavkm requested review from captainsafia, dougbu and javiercn June 8, 2020 17:50

pranavkm added the api-suggestion Early API idea and discussion, it is NOT ready for implementation label Jun 8, 2020

javiercn reviewed Jun 8, 2020

View reviewed changes

dougbu reviewed Jun 8, 2020

View reviewed changes

pranavkm force-pushed the prkrishn/6878 branch from d57ed85 to 309b910 Compare June 10, 2020 17:24

pranavkm added api-ready-for-review API is ready for formal API review - https://github.com/dotnet/apireviews and removed api-suggestion Early API idea and discussion, it is NOT ready for implementation labels Jun 10, 2020

halter73 reviewed Jun 11, 2020

View reviewed changes

src/Mvc/Mvc.Abstractions/ref/Microsoft.AspNetCore.Mvc.Abstractions.netcoreapp.cs Outdated Show resolved Hide resolved

pranavkm added 2 commits June 22, 2020 08:35

Add support for optional FromBody parameters

f3ebc05

Fixes #6878

Fixup nullable

0d0691b

pranavkm force-pushed the prkrishn/6878 branch from 309b910 to 4c0e1af Compare June 22, 2020 16:25

Changes per API review

7d2a94e

pranavkm force-pushed the prkrishn/6878 branch from 4c0e1af to 7d2a94e Compare June 22, 2020 16:28

pranavkm added api-approved API was approved in API review, it can be implemented and removed api-ready-for-review API is ready for formal API review - https://github.com/dotnet/apireviews labels Jun 22, 2020

pranavkm added this to the 5.0.0-preview7 milestone Jun 22, 2020

pranavkm added the auto-merge label Jun 22, 2020

dougbu approved these changes Jun 22, 2020

View reviewed changes

ghost merged commit f7d2fac into master Jun 22, 2020

ghost deleted the prkrishn/6878 branch June 22, 2020 19:11

pranavkm added a commit that referenced this pull request Jun 22, 2020

Add support for optional FromBody parameters (#22634)

0709629

* Add support for optional FromBody parameters Fixes #6878 * Fixup nullable * Changes per API review

pranavkm mentioned this pull request Jun 22, 2020

Add support for optional FromBody parameters (#22634) #23246

Merged

MaceWindu mentioned this pull request Mar 5, 2021

[Question/Bug] EmptyBodyBehavior.Allow behavior #30690

Closed

This pull request was closed.

Add support for optional FromBody parameters #22634

Add support for optional FromBody parameters #22634

Uh oh!

Conversation

pranavkm commented Jun 6, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pranavkm commented Jun 8, 2020

Uh oh!

javiercn left a comment

Choose a reason for hiding this comment

Uh oh!

dougbu left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pranavkm Jun 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

pranavkm commented Jun 8, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dougbu commented Jun 9, 2020

Uh oh!

ghost commented Jun 10, 2020

Uh oh!

Uh oh!

ghost commented Jun 22, 2020

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.

Uh oh!

dougbu left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

pranavkm commented Jun 6, 2020 •

edited

Loading

pranavkm Jun 15, 2020 •

edited

Loading

pranavkm commented Jun 8, 2020 •

edited

Loading

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.