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).

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).

Closing parenthesis goes at the end of the previous line.

Fixed

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

Remove this property.

Done

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 ??

1. Use the formatter returning null. Feels a bit like our previous mixups with [Required] semantics.
2. 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.

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).

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

Done

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

Done

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.

• 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)).

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.

Changed it

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.)

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.)

Fixed

• 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).

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".

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:

1. Client submits an explicit JSON null body payload and a model binding feature described as disallowing "empty" input prevents binding.
2. 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.

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