Make [FromBody] treat empty request bodies as invalid #6055

SteveSandersonMS · 2017-03-31T12:28:12Z

As discussed, this changes the default behaviour of [FromBody] binding, so that a "no input" will register a validation error (e.g., if you get an empty PUT/POST body, or some other request body that the associated input formatter regards as "no input", such as pure whitespace in the case of JsonInputFormatter).

Also, this PR exposes a flag to let developers revert to the v1 behavior if they need to. The flag is on BodyModelBinderProvider - this lets you configure it on the MvcOptions passed to your AddMvc callback. Since this flag exists only for backward compatibilty (and strictly speaking, we don't know of anyone who's going to need to use it), we probably don't want to make it any more prominent than that. The flag is now on MvcOptions as per CR feedback.

dnfclas · 2017-03-31T12:28:16Z

@SteveSandersonMS,
Thanks for your contribution.
To ensure that the project team has proper rights to use your work, please complete the Contribution License Agreement at https://cla2.dotnetfoundation.org.

It will cover your contributions to all .NET Foundation-managed open source projects.
Thanks,
.NET Foundation Pull Request Bot

Eilon · 2017-03-31T16:20:44Z

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

-        public BodyModelBinder(IList<IInputFormatter> formatters, IHttpRequestStreamReaderFactory readerFactory)
-            : this(formatters, readerFactory, loggerFactory: null)
+        /// <param name="isBindingRequired">Specifies whether a null input (e.g., due to an empty request body) should be treated as a validation error.</param>
+        public BodyModelBinder(


Is this a breaking change? Can it be done safely in a non-breaking way, such as by adding a reasonable overload?

@Eilon we don't have to break anything here.

Have reverted this, and added a further constructor overload instead (one that takes options too).

Eilon · 2017-03-31T16:21:07Z

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

@@ -31,8 +32,12 @@ public class BodyModelBinder : IModelBinder
        /// The <see cref="IHttpRequestStreamReaderFactory"/>, used to create <see cref="System.IO.TextReader"/>
        /// instances for reading the request body.
        /// </param>
-        public BodyModelBinder(IList<IInputFormatter> formatters, IHttpRequestStreamReaderFactory readerFactory)
-            : this(formatters, readerFactory, loggerFactory: null)
+        /// <param name="isBindingRequired">Specifies whether a null input (e.g., due to an empty request body) should be treated as a validation error.</param>


Docs recommendation is to avoid Latin abbreviations, e.g. i.e. e.g. etc.

Laying down the law on documentation

#JustEilonThings

This parameter is now removed as per other feedback (we now have options instead).

dougbu · 2017-03-31T15:46:42Z

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

+            IList<IInputFormatter> formatters,
+            IHttpRequestStreamReaderFactory readerFactory,
+            bool isBindingRequired
+            ) : this(formatters, readerFactory, loggerFactory: null, isBindingRequired: isBindingRequired)


Closing parenthesis goes at the end of the previous line.

dougbu · 2017-03-31T15:49:06Z

src/Microsoft.AspNetCore.Mvc.Abstractions/ModelBinding/Metadata/IModelBindingMessageProvider.cs

+        /// but a value is required.
+        /// </summary>
+        /// <value>Default <see cref="string"/> is "A value for the request body was not provided.".</value>
+        Func<string> MissingRequestBodyRequiredValueAccessor { get; }


If API Check let you do this addition to an interface, something is very wrong. Execute dotnet msbuild /t:pack in this folder or run build.cmd in the repo root. (Unfortunately API Check isn't reliable off Windows.)

API Check is failing in AppVeyor build due to this addition and the change to an existing constructor in the BodyModelBinder.

C:\Users\appveyor\.nuget\packages\internal.aspnetcore.sdk\2.0.0-rc2-15220\build\ApiCheck.targets(19,5): error : ERROR: Missing type or member 'New members were added to the following interface => public interface Microsoft.AspNetCore.Mvc.ModelBinding.Metadata.IModelBindingMessageProvider' [C:\projects\mvc\src\Microsoft.AspNetCore.Mvc.Abstractions\Microsoft.AspNetCore.Mvc.Abstractions.csproj] C:\Users\appveyor\.nuget\packages\internal.aspnetcore.sdk\2.0.0-rc2-15220\build\ApiCheck.targets(19,5): error : ERROR: Missing type or member 'public class Microsoft.AspNetCore.Mvc.ModelBinding.Binders.BodyModelBinder : Microsoft.AspNetCore.Mvc.ModelBinding.IModelBinder => public .ctor(System.Collections.Generic.IList<Microsoft.AspNetCore.Mvc.Formatters.IInputFormatter> formatters, Microsoft.AspNetCore.Mvc.Internal.IHttpRequestStreamReaderFactory readerFactory)'

@Eilon do you want to avoid a breaking change here too? Could reuse MissingBindRequiredValueAccessor. That message (by default A value for the '{0}' property was not provided.) would be correct but doesn't mention the request body.

@dougbu What's the process for adding a new validation error message, if we're keen not to have a breaking change on this? Are we going to do IModelBindingMessageProvider2 and all that?

That message (by default A value for the '{0}' property was not provided.) would be correct but doesn't mention the request body.

Sort of correct, but not totally, because it's not a "property" that we're lacking a value for. It's either a method parameter or a request body.

I'm happy to use that message if we think it's good enough, though it does seem a bit odd.

The MissingBindRequiredValueAccessor default message is about the target property, not the source. It's already used in cases where property or parameter is more correct.

I'm fine switching to a class if we can get past whatever issue I hit with overriding get-only virtual properties and preserve the readonly-ness of these properties outside BindingMetadata (which is mostly-hidden infrastructure). Tried it before…

Yeah dump the interface. But yeah not sure what @dougbu ran into before but hopefully we can figure it out.

At the moment, IModelBindingMessageProvider is in ...Mvc.Abstractions, whereas the implementation ModelBindingMessageProvider is in ...Mvc.Core.

Other code in ...Abstractions references IModelBindingMessageProvider. So to remove that interface, we'd need to move ModelBindingMessageProvider across into ...Abstractions, along with its .resx file.

Of course we can do this, but I'd prefer to regard it as an independent change, because I don't personally know how we manage localisation (e.g., do we have some build process that's configured to do something with a known set of .resx files, to ensure all the different language translations get included in the final builds? how would they need to be updated if we moved a .resx file to a different project?).

So I'm regarding this as "won't fix" (as a CR feedback item on issue, that is) unless anyone feels strongly enough and can describe what would have to happen to keep localisation showing up properly in builds after moving the .resx file. Hope that's OK!

So, go ahead but you'll need to add an exceptions.netcore.json file with the right content to keep API Check happy.

Done - thanks for the info!

@SteveSandersonMS - Please log a bug for this.

Filed #6069

dougbu · 2017-03-31T15:51:04Z

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

+        /// <summary>
+        /// Gets a flag indicating whether the binder will require non-null input values to be supplied.
+        /// </summary>
+        public bool IsBindingRequired => _isBindingRequired;


Remove this property.

dougbu · 2017-03-31T15:53:22Z

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

@@ -132,7 +148,21 @@ public BodyModelBinder(IList<IInputFormatter> formatters, IHttpRequestStreamRead
                    return;
                }

-                bindingContext.Result = ModelBindingResult.Success(model);
+                if (_isBindingRequired && model == null)


model == null doesn't mean no value was provided. Serialization in the request body may indicate an explicit null. Instead, make changes at line 108 of InputFormatter. Then, would only need to change this class to not call ModelState.AddModelError() if the formatter already did i.e. check for an existing error in the ModelState for modelBindingKey.

Side note: Please kill previousCount with fire.

I did consider triggering this behaviour based on line 108 of InputFormatter, but it didn't seem correct. There might be a nonempty request body that an InputFormatter subclass regards as "no input". The primary example would be a request whose body is just whitespace (or maybe even the string null - haven't tried that though) - in that case, JsonInputFormatter will behave identically to as if it were zero-length (and return null, which the developer is explicitly trying to test for when they check ModelState.IsValid).

Ideally, InputFormatterResult would have an IsModelSet property on it (equivalent to what we do for regular model binding). Subclasses of InputFormatter would assign that property using their own logic, according to their own serialization rules. Then it could behave correctly for zero-length bodies, and other bodies that are regarded as "no input", whether the parameter is value-typed or not. However no such property exists, and adding one would be a breaking change if it defaults to false (because existing subclasses won't assign it). It seemed strange to add a new property there that defaults to true, but I guess we could do.

What do you think - should we add that property? Or do you have some other proposal about how we handle cases where the body content-length is nonzero but represents no value (according to the input formatter)?

Don't add a new property on the InputFormatterResult. It wouldn't help unless we forced changes in all of the input serializers and they're external. E.g. the formatters themselves don't check for whitespace.

We really have only two choices. I prefer (2) but want to hear a third opinion. @rynowak ??

Use the formatter returning null. Feels a bit like our previous mixups with [Required] semantics.

Update line 108 of InputFormatter which is the closest thing we have to IsModelSet.

BTW current (1) approach misses cases like [FromBody] int count. The value seen in the input formatter when the body is empty will be a boxed 0, not null.

And by this I'm saying that Line 108 should be pushed down to individual formatters.

I think that input formatter result needs to be tri-state. (Success, Failed, NoData)

I prefer the current location of the empty body check, mostly because the containing method (ReadAsync()) is virtual already. We don't have an example subclass that would return anything but InputFormatterResult.NoData() in that case. But, I agree that check should be changed to return NoData when the option says so.

If we want, say, JsonInputFormatter to also return NoData when the body contains only whitespace, we can do that now or later. Might be good to do it now because that's a case you're particularly concerned about @SteveSandersonMS.

Either way, pushing the empty body check into JsonInputFormatter etc. does not seem germane to the bug you're fixing here.

Put another way, the individual input formatters will be responsible for deciding whether or not they received data. The base just implements the one part all should agree on.

I'm with @dougbu on this - maybe if we were starting from scratch there'd be a case for not having 108, but it seems like an unnecessary breaking change to remove it.

Removing it means that everyone's custom InputFormatter subclasses would start getting calls to ReadRequestBodyAsync in cases where they wouldn't previously (when the ContentLength is zero), and everyone would have to duplicate the same logic about checking whether empty input is configured as allowed and if so, producing a default value for the result.

Given that (1) we have back-compatibility as a (soft) goal, and (2) the existing base-class behaviour is correct in virtually all cases, and (3) developers can always override ReadAsync completely if they really want to change this, I don't see how the benefits of pushing this check downstream would outweigh the costs.

Put another way, the individual input formatters will be responsible for deciding whether or not they received data

Yes, that, plus they are responsible for providing a default value for the model in cases where there's no input yet empty input is allowed. That's the existing 1.x design.

dougbu · 2017-03-31T16:27:20Z

src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/Binders/BodyModelBinderProvider.cs

+        /// as the value to bind (for example, if an incoming POST or PUT request supplies a
+        /// zero-length body), the binder will register a validation error.
+        /// </example>
+        public bool IsBindingRequired { get; set; } = true;


This cannot easily be set false.

Instead, use existing infrastructure that plumbs ModelMetadata.IsBindRequired through and base decisions in the InputFormatter on that property. Add a new MvcOptions property e.g. IgnoreEmptyBodyInInputFormatter that defaults to false (if you use something similar to that name). Also add an IBindingMetadataProvider implementation (or extend DefaultBindingMetadataProvider) similar to DataMemberRequiredBindingMetadataProvider but based on the calculated BindingSource and the new option.

To set it to false, you do:

services.AddMvc(options => { var bodyModelBinderProvider = options.ModelBinderProviders .OfType<BodyModelBinderProvider>().Single(); bodyModelBinderProvider.IsBindingRequired = false; });

Do we definitely want to make it easier than that? Low-discoverability is a feature here, because we don't think most people should mess with this.

Yes. Make this an option. We don't bury things by hanging them off providers and nowhere else.

In general I'd agree with Steve on this one except that this is compat break with a previous release. It seems good to have an option for this.

I think that input formatter result needs to be tri-state. (Success, Failed, NoData)

Also add an IBindingMetadataProvider implementation (or extend DefaultBindingMetadataProvider) similar to DataMemberRequiredBindingMetadataProvider but based on the calculated BindingSource and the new option.

Ignore this part. Don't know where my brain went. @rynowak reminded me this only works for properties and types. Our attributes that implement IBindingSourceMetadata can't be associated with a type and, as @SteveSandersonMS noted earlier, we don't get unique ModelMetadata for parameters -- just the type's info.

Instead, add constructors accepting MvcOptions parameters to BodyModelBinderProvider and BodyModelBinder. Also add a property of this type to the InputFormatterContext class.

Also add a property of this type to the InputFormatterContext class.

I take that back too. Instead have the input formatter(s) always return NoData in the empty body case. Copy GetDefaultValueForType() into BodyModelBinder and check the option there.

Have changed it to a new option on MvcOptions called AllowEmptyInputInInputFormatter.

Also add a property of this type to the InputFormatterContext class.

The new property on InputFormatterContext is just a bool flag called AllowEmptyInput, because InputFormatterContext is in Abstractions and hence doesn't know about MvcOptions.

Instead have the input formatter(s) always return NoData in the empty body case. Copy GetDefaultValueForType() into BodyModelBinder and check the option there.

That would be a further change of design that ends up being a very subtle breaking change. In MVC 1.x, it's the input formatter subclasses that have the responsibility of selecting a default value for the model type if there's no input (by overriding GetDefaultValueForType). If we make them always return NoData, then they lose the ability to choose the default value. So what I've done instead is let the input formatters look at the context.AllowEmptyInput flag and the whole request context, and then each of them decides for itself whether to return NoData or Success(defaultForModelType), since only the input formatter is able to supply a defaultForModelType (without the subtle breaking change).

dougbu · 2017-03-31T16:28:32Z

src/Microsoft.AspNetCore.Mvc.Core/Resources.resx

@@ -297,6 +297,9 @@
  <data name="ModelBinding_MissingBindRequiredMember" xml:space="preserve">
    <value>A value for the '{0}' property was not provided.</value>
  </data>
+    <data name="ModelBinding_MissingRequestBodyRequiredMember" xml:space="preserve">
+    <value>A value for the request body was not provided.</value>


Suggest A non-empty request body is required. because "value" doesn't mean much here.

dougbu · 2017-03-31T16:32:09Z

test/Microsoft.AspNetCore.Mvc.FunctionalTests/InputFormatterTests.cs

@@ -80,6 +80,24 @@ public InputFormatterTests(MvcTestFixture<FormatterWebSite.Startup> fixture)
        }

        [Theory]
+        [InlineData("application/json", "")]
+        [InlineData("application/json", "    ")]
+        public async Task JsonInputFormatter_IsModelStateValid_FalseForEmptyRequestBody(


Good test but the name is misleading (yes, that's a problem in this class) Suggest JsonInputFormatter_ReturnsBadRequest_ForEmptyRequestBody.

dougbu · 2017-03-31T16:36:30Z

src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/Metadata/ModelBindingMessageProvider.cs

@@ -91,6 +94,24 @@ public Func<string> MissingKeyOrValueAccessor
        }

        /// <inheritdoc/>
+        public Func<string> MissingRequestBodyRequiredValueAccessor


Look at existing tests that reference properties in this class. We test each to confirm changing them actually changes errors in the expected cases. In this case, it would be a check in the other InputFormatterTest class.

Thanks - done.

dougbu

In addition to comments, please add a couple of tests to BodyValidationIntegrationTests covering new end-to-end behaviour and behaviour with option enabled. (This test class has a lousy name because it covers BindModelAsync() exclusively.)

dougbu · 2017-04-04T19:42:58Z

src/Microsoft.AspNetCore.Mvc.Abstractions/Formatters/InputFormatterContext.cs

+        /// If <see langword="false"/>, the input formatter should handle empty input by returning
+        /// <see cref="InputFormatterResult.NoValueAsync()"/>. If <see langword="false"/>, the input
+        /// formatter should handle empty input by returning the default value for the type
+        /// <see cref="ModelType"/>.


Need a different name in this context. The input formatter unconditionally allows empty input; it's the model binder that disallows !IsModelSet and turns that result into an error.

One of these sentences should describe the true case.

Don't think we use langword elsewhere. Suggest <c>true</c> and <c>false</c> if I'm right.

Need a different name in this context

OK, renamed to TreatEmptyInputAsDefaultValue.

One of these sentences should describe the true case

Good catch - fixed!

Don't think we use langword elsewhere

We use it pretty widely so I'll leave it as-is. I guess the benefit over <c>false</c> is that IDEs and autogenerated docs know it's something they can optionally translate to the language that the user is working in (e.g., it could be presented as False in a VB project (if we supported VB)).

dougbu · 2017-04-04T22:19:37Z

src/Microsoft.AspNetCore.Mvc.Core/Formatters/InputFormatter.cs

+                else
+                {
+                    return InputFormatterResult.NoValueAsync();
+                }


To reduce unnecessary indentation and improve readability, we tend to avoid else after a conditional return e.g.

if (joe) { return "joe"; } return "not joe";

Think you've done this in a few other places in this PR.

dougbu · 2017-04-04T22:26:07Z

src/Microsoft.AspNetCore.Mvc.Core/Properties/Resources.Designer.cs

+        internal static string FormatModelBinding_MissingRequestBodyRequiredMember()
+        {
+            return GetString("ModelBinding_MissingRequestBodyRequiredMember");
+        }


Do not update *.designer.cs files manually. Instead run .\build.cmd resx in the repo root.

Ah, thanks for letting me know - didn't know that facility existed! Done.

(For anyone else referring to this, the syntax is build.cmd /t:resx.)

dougbu · 2017-04-04T22:36:08Z

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

+                    // If instead the input formatter wants to treat the input as optional, it must do so by
+                    // returning InputFormatterResult.Success(defaultForModelType), because input formatters
+                    // are responsible for choosing a default value for the model type.
+                    bindingContext.Result = ModelBindingResult.Failed();


Don't set Result unless you're indicating success. Result is already initialized to something that looks like Failed(). (Failed() exists for tests and may have had historical significance.)

dougbu · 2017-04-04T22:43:19Z

src/Microsoft.AspNetCore.Mvc.Core/MvcOptions.cs

@@ -35,6 +35,18 @@ public MvcOptions()
        }

        /// <summary>
+        /// Gets or sets the flag which decides whether input formatters (for example, using
+        /// <see cref="FromBodyAttribute"/>) should allow no input to be provided.
+        /// <see langword="false"/> by default.


Again, disallowing is implemented only in BodyModelBinder. Focus this description on externally-visible "systems" i.e. how the "model binding system" acts when the body is empty.

And, I can find a handful of langword instances in this repo but almost places where we use <c>(true|false)</c>. Stick with the normal way we do the documentation.

For descriptions like this, there's a tradeoff between technical accuracy and familiarity. It's more accurate to say "input formatters" because they are the ones that actually respond to the flag directly (whereas BodyModelBinder only responds to the result from the input formatters). So I erred on the side of accuracy in this case. But I appreciate that - as per your comment - most developers will interact with this system only on the level of model binding as a whole, so it's more familiar to phrase it in those terms. I think it's always going to be just a subjective call about how to pitch such a description. In this case I've rephrased in terms of "body model binding" ("model binding" alone would risk being too misleading, as this flag doesn't affect traditional ComplexModelBinder and friends).

dougbu · 2017-04-04T22:46:51Z

src/Microsoft.AspNetCore.Mvc.Core/MvcOptions.cs

+        /// <example>
+        /// When <see langword="false"/>, actions that model bind the request body (for example,
+        /// using <see cref="FromBodyAttribute"/>) will register a validation error if the incoming
+        /// request body is empty.


Confusing to call this a "validation error"; the validation system isn't involved at all. We shouldn't be using "validation error" for other "errors the model binding system detects". Use that second phrase unless you see "validation error" used in this way elsewhere and feel that's correct.

The net result is that ModelState.IsValid becomes false. Don't developers think of that as a "validation error"? It's called IsValid, after all.

Have rephrased to "register an error in the ModelStateDictionary".

dougbu · 2017-04-04T22:49:24Z

src/Microsoft.AspNetCore.Mvc.Formatters.Json/JsonInputFormatter.cs

@@ -158,7 +158,18 @@ public class JsonInputFormatter : TextInputFormatter

                    if (successful)
                    {
-                        return InputFormatterResult.SuccessAsync(model);
+                        if (model == null && !context.AllowEmptyInput)


This condition is too general. What we discussed was an explicit check for a body containing only whitespace. Don't want to disallow the null JSON keyword.

I feel we should expand on the check InputFormatter does separately, after we've buttoned this PR up. For example, if we do this here, does the JsonPatchInputFormatter also need to change?

Don't want to disallow the null JSON keyword.

Here, JsonInputFormatter is not disallowing null; it's regarding it as "no input" (like it does an empty request body, or a request body that only contains whitespace). This fits within the general design we've come to, which is that input formatters make their own judgement about what constitutes "input". What then happens with "no input" depends on how you've configured the new global flag.

JsonInputFormatter is unique among our built-in input formatters in that it has these surprising cases where it might otherwise supply null even when the request body is nonempty. The original issue (#4750) was due to someone getting tripped up by receiving null when ModelState.IsValid was true, so I would have thought a large part of our goal would be to stop the surprising cases happening out-of-the-box. Of course, a developer can make their own input formatter that sometimes returns null as "non-empty" if they want, but since our built-in XML and text ones don't, it's much less surprising if the built-in JSON one doesn't either.

If I'm wrong and there are cases where other built-in formatters will return null as a "success" result, then I guess more handling is needed, but I'm not aware of that happening.

@rynowak, @Eilon: do you have an opinion on whether stopping the JSON input formatter from supplying null alongside ModelState.IsValid==true like this does is a desirable result?

For example, if we do this here, does the JsonPatchInputFormatter also need to change?

JsonPatchInputFormatter inherits this from JsonInputFormatter.

The original issue (#4750) was due to someone getting tripped up by receiving null when ModelState.IsValid was true,

The original case is already handled by your changes in InputFormatter. The client posted no content, not the JSON null keyword. As I said, let's discuss going beyond the ContentLength == 0 check in a new issue. Checking for whitespace or treating everything the JSON deserializer converts to null as if the client posted nothing is separable and controversial, at least to me.

Is there a clear case where this change in behavior is either clearly right or clearly wrong? In other words, what "harm" would happen if we make this change, vs. if we don't? In what circumstance would someone be surprised by this behavior?

Example cases, basically caused by pushing behaviour we didn't want in BodyModelBinder down into JsonInputFormatter:

Client submits an explicit JSON null body payload and a model binding feature described as disallowing "empty" input prevents binding.

Server property of type int is successfully bound to 0 but server property of type string is not when body contains whitespace. That sounds like the opposite of the expected behaviour.

In what circumstance would someone be surprised by this behavior?

We've been saying since the start of this issue that "[FromBody] should imply 'required'". If we teach people that, and they also believe that "required means I get non-null or IsValid==false", then they may well write code that assumes their [FromBody] param isn't going to receive null when IsValid==true. They might even test it with a blank request and see the framework appears to be protecting them, and be happy. But they will not be happy if they later find that a crafty user sends either whitespace or the literal text "null" and manages to get a null into the method while also IsValid==true.

Are there other common cases where a property marked as required for model binding might arrive as null while also IsValid==true? If so, maybe this isn't a big deal. But if this is the only way that happens, it would be pretty nasty.

I think @SteveSandersonMS 's point about reasonably expected behavior about what is "empty data" and what IsValid should return makes sense here, so let's go with that.

@Eilon Sounds good - will do.

dougbu · 2017-04-04T22:53:29Z

test/Microsoft.AspNetCore.Mvc.Core.Test/Formatters/InputFormatterTest.cs

+            var formatter = new TestFormatter();
+            var context = new InputFormatterContext(
+                new DefaultHttpContext(),
+                "",


string.Empty (all day, every day … except where a const is required)

dougbu · 2017-04-04T22:57:22Z

test/Microsoft.AspNetCore.Mvc.Core.Test/ModelBinding/Binders/BodyModelBinderTests.cs

+            mockInputFormatter.Setup(f => f.CanRead(It.IsAny<InputFormatterContext>()))
+                .Returns(true);
+            mockInputFormatter.Setup(o => o.ReadAsync(It.IsAny<InputFormatterContext>()))
+                              .Returns(InputFormatterResult.NoValueAsync());


Don't add un-maintainable indentation. (May have a few lines that look like this. Don't search them out but feel free to hurt them, badly, if you see them.) Same thing in a few other parts of this PR.

Fixed here and in BindModel_PassesAllowEmptyInputOptionViaContext. Scanned the PR and didn't spot any others, but please let me know if you notice any!

SteveSandersonMS · 2017-04-07T15:41:28Z

@Eilon @dougbu @rynowak It looks like all the review comments are covered. Are we good to ?

Eilon

The change looks quite surprisingly straight-forward... it's almost suspicious 😄

dougbu

Just small comments on naming. Might be good to rephrase the two "validation error" mentions in regular comments but that's up to you.

dougbu · 2017-04-07T16:34:07Z

src/Microsoft.AspNetCore.Mvc.Abstractions/Formatters/InputFormatterContext.cs

+            ModelStateDictionary modelState,
+            ModelMetadata metadata,
+            Func<Stream, Encoding, TextReader> readerFactory,
+            bool allowEmptyInput)


Rename parameter to match the property

dougbu · 2017-04-07T16:36:24Z

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

@@ -89,12 +114,15 @@ public BodyModelBinder(IList<IInputFormatter> formatters, IHttpRequestStreamRead

            var httpContext = bindingContext.HttpContext;

+            var allowEmptyInput = _options?.AllowEmptyInputInBodyModelBinding == true;


Not mandatory (of course) but the variable name should be more like the property.

Sounds good - done.

rynowak

from me. I didn't dig into details, but looks good overall

SteveSandersonMS · 2017-04-10T14:09:33Z

Just small comments on naming. Might be good to rephrase the two "validation error" mentions in regular comments but that's up to you.

OK, rephrased as "model binding error".

SteveSandersonMS · 2017-04-10T16:42:03Z

Merged. Thanks everyone!

dnfclas added the cla-required label Mar 31, 2017

SteveSandersonMS requested review from dougbu and rynowak March 31, 2017 12:29

SteveSandersonMS force-pushed the stevesa/4750-frombody-no-body-handling branch from 5cfeac5 to 7719108 Compare March 31, 2017 12:38

Eilon suggested changes Mar 31, 2017

View reviewed changes

dougbu suggested changes Mar 31, 2017

View reviewed changes

SteveSandersonMS force-pushed the stevesa/4750-frombody-no-body-handling branch from 85372ac to 0dbd12c Compare April 4, 2017 12:27

SteveSandersonMS mentioned this pull request Apr 4, 2017

Remove IModelBindingMessageProvider interface #6069

Closed

dougbu suggested changes Apr 4, 2017

View reviewed changes

SteveSandersonMS force-pushed the stevesa/4750-frombody-no-body-handling branch from 4bee37b to be7e9d2 Compare April 5, 2017 17:53

Eilon approved these changes Apr 7, 2017

View reviewed changes

dougbu approved these changes Apr 7, 2017

View reviewed changes

rynowak approved these changes Apr 7, 2017

View reviewed changes

SteveSandersonMS force-pushed the stevesa/4750-frombody-no-body-handling branch from be7e9d2 to c2041b6 Compare April 10, 2017 14:11

SteveSandersonMS mentioned this pull request Apr 10, 2017

BindParameter_WithTypeProperty_IsNotBound test looks bogus #6110

Closed

Make [FromBody] treat empty request bodies as invalid (#4750)

90acd05

SteveSandersonMS force-pushed the stevesa/4750-frombody-no-body-handling branch from c2041b6 to 90acd05 Compare April 10, 2017 15:56

SteveSandersonMS merged commit 90acd05 into dev Apr 10, 2017

SteveSandersonMS deleted the stevesa/4750-frombody-no-body-handling branch April 10, 2017 16:41

303248153 mentioned this pull request Jul 20, 2017

Post large body cause model mark with [FromBody] become null #6568

Closed

		@@ -89,12 +114,15 @@ public BodyModelBinder(IList<IInputFormatter> formatters, IHttpRequestStreamRead

		var httpContext = bindingContext.HttpContext;

		var allowEmptyInput = _options?.AllowEmptyInputInBodyModelBinding == true;

Make [FromBody] treat empty request bodies as invalid #6055

Make [FromBody] treat empty request bodies as invalid #6055

Conversation

SteveSandersonMS commented Mar 31, 2017 • edited Loading

dnfclas commented Mar 31, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SteveSandersonMS Apr 4, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SteveSandersonMS Apr 4, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SteveSandersonMS Mar 31, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SteveSandersonMS Apr 4, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dougbu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SteveSandersonMS Apr 5, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SteveSandersonMS commented Mar 31, 2017 •

edited

Loading

SteveSandersonMS Apr 4, 2017 •

edited

Loading

SteveSandersonMS Apr 4, 2017 •

edited

Loading

SteveSandersonMS Mar 31, 2017 •

edited

Loading

SteveSandersonMS Apr 4, 2017 •

edited

Loading

SteveSandersonMS Apr 5, 2017 •

edited

Loading