ModelState.IsValid reports valid model while it is null

[janpieterz]

Possibly a dupe of #4607 but since it seemed to be (somehow) resolved there, I thought it smart to file another issue to validate if there is a difference.

Relevant bits:

public async Task<IActionResult> Login([FromBody]LoginViewModel model)
{
    if (ModelState.IsValid)
    {
        var result = await _signInManager.PasswordSignInAsync(model.Email, model.Password, true, lockoutOnFailure: false);
        if (result.Succeeded)
        {
            _logger.LogInformation(1, "User logged in.");
            return Json(User.Identity.Name);
        }
        else
        {
            return BadRequest("Invalid login attempt.");
        }
    }
    else if (HttpContext.User.Identity.IsAuthenticated)
    {
        return Json(User.Identity.Name);
    }
    return BadRequest("Cannot login.");
}

public class LoginViewModel
{
    [Required]
    [EmailAddress]
    public string Email { get; set; }

    [Required]
    [DataType(DataType.Password)]
    public string Password { get; set; }
}

services.AddMvc().AddJsonOptions(options =>
{
    options.SerializerSettings.ContractResolver = new CamelCasePropertyNamesContractResolver();
    options.SerializerSettings.ReferenceLoopHandling = ReferenceLoopHandling.Ignore;
    options.SerializerSettings.Converters.Add(new VersionConverter());
});

When calling the method without any parameters in the body, the model rightfully becomes null. I can make this a manual check of course, but it seems if there is a model state, that has parameters that are null but contain properties with the required attribute, that the model state shouldn't succeed.

Thoughts? Easy to circumvent with an extra null check, but it feels a bit counter intuitive (maybe I'm missing something though!)

[dougbu]

since it seemed to be (somehow) resolved there

? #4607 remains open.

In any case, I believe your request is for validating properties of null values in the model tree. Is that correct? If yes, where should validation stop?

[janpieterz]

It is, but the discussion seemed to venture more into 'documenting and error reporting'.

My line of thinking would be that if I have a model that has required properties, if I would feed it one property it 'fails' because it doesn't have the other.

So, with above code, the following fails/succeeds:

Body: '' (empty)
ModelState.IsValid: true

Body: '{}'
ModelState.IsValid: false

Body: '{"email": "[email protected]","password": "12345677Asasd"}'
ModelState.IsValid: true

The first one just doesn't make sense to me, if it 'works' when I'm feeding it an empty object, I think it should also 'work' while I (or an external party) feed it an empty body, since it is not fulfilling the required properties, at all.

I'm not sure where it should stop? I feel that certainly for API's having one way of describing required props and therefore errors makes it incredibly easy to pass these over to other people and maintain the same error quality. I'm probably missing complexities in the implementation and other consequences, but this is how I see it for the use cases I have.

[dougbu]

The cases you've described sound correct. The JSON deserializer returned null when the body was completely empty but created an uninitialized LoginViewModel when the body contained {}. Validation checks only properties of created objects.

[janpieterz]

Sweet, so code wise it is correct. Do you see the more abstracted 'issue' though? Not looking to what actually happens under the hood, but simply what happens in the controller?
I have a model that I want to be validated. There are multiple entries (no body, empty json object, object with missing, false or correct email, missing, false or correct password)... All of which I think are reasonable to expect the validation to error. Currently the model state reports valid while it obviously isn't, unless you take the hood off and look below and see that it only checks if an object isn't null (or something alike).

So to maybe put it in straight words, I'd expect Validation to also check if an object actually exists if it has things like required on one or more of it's properties.

[Bartmax]

If you add [BindRequired] does it help?
If it does, there is another reason why BindRequired should be the default.

you can see here how to apply it application-wise: #2615 (comment)

[janpieterz]

@Bartmax just tested, the same issue persists with BindRequired.

[Bartmax]

you did apply the BindRequired configuration and then

public async Task<IActionResult> Login([FromBody][Required]LoginViewModel model)

right?

[janpieterz]

No, I added BindRequired to the Class and properties (in all possible configs).

Just tried your full suggestion, but it threw an error in

if (context.PropertyAttributes.OfType<RequiredAttribute>().Any())

That I had to change to

if (context.PropertyAttributes != null && context.PropertyAttributes.OfType<RequiredAttribute>().Any())

since somehow it noted that LoginViewModel didn't have any property attributes (At least on one pass through). Obviously after this it didn't work either since it simply always skipped it ;)

See screenshot, no attributes but correct model.

[Bartmax]

That's unfortunate :( ok ... well ... it was worth a shot

[dougbu]

@janpieterz are you using RC2 or 1.0.0? You may be now hitting #4652. That was fixed in 1.0.0.

[janpieterz]

1.0.0

[dougbu]

MVC does not perform validation (of any kind) for parameters unless they are bound. This behaviour hasn't changed in forever as far as I know. (My mention of #4652 was wrong; that's not relevant here.)

@janpieterz should we turn this issue into a request for at least [BindRequired] and [Required] enforcement on parameters?

Could also change [BindRequired] and the underlying [BindingBehavior] to support AttributeTargets.Parameter but that seems optional. Do you agree?

One potential workaround would be to check model == null in the action. That should only be true when the input formatter is not called or fails. Since validation will of course occur when model != null, (model == null || !ModelState.IsValid) covers all the errors.

[rynowak]

I'm curious to know if anyone would ever want [FromBody] Person person == null to ever not be a validation error.

[MicahZoltu]

Personally, I wish C# generally didn't allow nulls unless I went out of my way to allow for them, so when it comes to validation, I think at the least it should default to not null and maybe provide an option for allowing null. In the case of [FromBody], I can see a poorly designed API allowing for null values. 😄

[janpieterz]

I'm happy turning this issue into something else @dougbu, but agree with @rynowak and I'm really curious to the use cases for it not being a validation error. I surely understand that it means diverting from previous ways (seems like a perfect moment to do it now), I get that it might rip open abstraction layers that are put in place (seems like 'new' insight shows they might need to be different), or any other kind of annoying / unpleasant consequence.

For me as an API developer using this, and probably everyone else, I had to find this out the hard way, and now have to extend the validation myself (the errors are not automatically generated etc etc).

I'd prefer to not have to change anything in my code in the above example to get it to work. If this really is impossible something like BindRequired enforcing this so it's at least as simple as throwing that on a parameter certainly will work!

As a side note, I just made a new project to look at the templates and even the default templates don't seem to take this into account. Most actions (AccountController for individual accounts) seem to omit this at all. I don't have the time to test currently, but it seems from glancing over it that these will have problems too.

[dougbu]

Cleared the Investigate label.

@triage the decisions here are whether to make a change, whether validation of a null parameter should be opt-in or implicit with [FromBody] (a slight behaviour breaking change), and when.

See #4971 for a few cases where ModelState.IsValid though parameters are left null. In still more IsValid cases, objects and collections are created for parameters but left uninitialized or empty.

/cc @danroth27 @Eilon

[dougbu]

Cleared assignee to make it clear @danroth27 needs to have a look.

[brockallen]

BTW this is how it worked in Web API 2.

[rynowak]

BTW, if you like the sound of null-body-is-validation-error you can step into that alternate universe pretty easily like so:

using System;
using System.Collections.Generic;
using System.Linq;
using System.Threading.Tasks;
using Microsoft.AspNetCore.Mvc;
using Microsoft.AspNetCore.Mvc.ApplicationModels;
using Microsoft.AspNetCore.Mvc.Filters;
using Microsoft.AspNetCore.Mvc.ModelBinding;

namespace MvcSandbox
{
    public class FromBodyRequiredConvention : IActionModelConvention
    {
        public void Apply(ActionModel action)
        {
            var parameterName = action.Parameters
                .Where(p => p.BindingInfo?.BindingSource.CanAcceptDataFrom(BindingSource.Body) ?? false)
                .Select(p => p.ParameterName)
                .SingleOrDefault();

            if (parameterName != null)
            {
                action.Filters.Add(new FromBodyRequiredActionFilter(parameterName));
            }
        }

        private class FromBodyRequiredActionFilter : IActionFilter
        {
            private readonly string _parameterName;

            public FromBodyRequiredActionFilter(string parameterName)
            {
                _parameterName = parameterName;
            }

            public void OnActionExecuting(ActionExecutingContext context)
            {
                object value;
                context.ActionArguments.TryGetValue(_parameterName, out value);

                if (value == null)
                {
                    // Now uncomment one of these depending on what you want to do.
                    //
                    // This will return a 400 with an error in json.
                    //context.Result = new BadRequestObjectResult(new { message = "The body is required" });

                    // This will add a validation error and let the action decide what to do
                    //context.ModelState.AddModelError(_parameterName, "The body is required");
                }
            }

            public void OnActionExecuted(ActionExecutedContext context)
            {
            }
        }
    }
}

Add the convention to MvcOptions.

[Eilon]

@rynowak / @dougbu what is your proposal that we do here?

[dougbu]

@rynowak @Eilon my proposal is to update DefaultControllerArgumentBinder.BindModelAsync() to do the Right Thing^™️ when !IsModelSet. May be necessary to futz with a couple of the arguments to IObjectModelValidator.Validate() and will probably need to handle [BindRequired] separately. Otherwise, fairly straightforward.

[Bartmax]

I think it's fine if we also add BodyRequired option to make [FromBody] default to required, but there needs to also be a per-action way to disable it (e.g., [BindingBehavior(BindingBehavior.Optional)]?), otherwise it becomes a trap you get stuck in.

The global flag as required = true doesn't get you into a trap.
You annotate your bindings as Required or BindRequired and that's it.

The only thing that won't be supported is the option to have a binding as Required but you want to not trigger a validation error if the property is not on the payload which doesn't make any sense.

[Bartmax]

One more thing, [FromBody] doesn't perform any validation per se but requires a valid body content (which is a common pitfall people run into). So making a request of application/json with an empty body won't be valid but with a content of { } will be valid.

[dougbu]

A few clarifications:

• @Eilon's suggestion feels like an answer to @rynowak's question: Nobody would ever want [FromBody] to succeed when no body was provided. But, we need to hide the new behaviour behind a flag to avoid breaking changes.
• My suggestion was about us not validating anything at the top level i.e. MVC ignores [Required] on a parameter. IOW a tangent to the main issue here.
• @Bartmax's description of the current behaviour seems about right. In a quick read, the only thing I noticed is that [FromHeader] is not a default binding source.
• If the answer to @rynowak's question is actually "yes, people want to require data some places", simplest thing to do is add [BindRequired] support in BodyModelBinder. Would copy what's in SimpleTypeModelBinder.

[SteveSandersonMS]

Following a quick discussion just now, we're going to ensure that if you use [BindRequired] on classes bound with [FromBody], that an empty POST/PUT body (or whatever makes the InputFormatter believe there was no input) registers a ModelState error, so that ModelState.IsValid would be false. It's kind of strange that [BindRequired] doesn't already have this effect on [FromBody]-bound params, so this is a gap we want to plug.

As a secondary goal, if it turns out that we can also extend BindRequiredAttribute so that you can use it on parameters, we'll also do that and will again treat this as a validation failure if there's no input when you have a [FromBody] [BindRequired] parameter.

This is the raw ingredient that will let you control whether or not POST/PUT bodies are actually required (versus it being legit to supply no input, making the parameter value is null, as we do today). Based on this, if you want to implement an IActionModelConvention that dynamically adds a [BindRequired] to all [FromBody] parameters as a global convention, it will be straightforward to do so.

[dougbu]

@SteveSandersonMS I didn't mention IActionModelConvention for the final point. Better (more straightforward) to implement IBindingMetadataProvider and change BindingMetadata.IsBindingRequired. That controls ModelMetadata.IsBindingRequired which is what ComplexTypeModelBinder checks (and BodyModelBinder does not -- today). Have a look at DataMemberRequiredBindingMetadataProvider for a related IBindingMetadataProvider implementation.

[rynowak]

I realize that I missed the discussion about this, but I have bone to pick over:

which is off by default (to preserve current behavior)

I don't think the current behavior is in any way reasonable or useful and the reactions on my comment corroborate this. No one has put forth a scenario where an API has a optional body.

This is 2.0.0 so we should make the default experience be what we think is the best for our users. /cc @Eilon

[rynowak]

Also FYI for the discussion around [Required] there's currently no way for validation attributes to be set on a parameter. It just doesn't exist in the plumbing. We could do something gross here to hack it, but it's never been supported in MVC, which is surprising to some.

[Eilon]

I'd be ok with having this be on by default. I think this is an acceptable breaking change, and there's a way to turn it off.

[dougbu]

there's currently no way for validation attributes to be set on a parameter

Slight clarification @rynowak: It's entirely possible to associate most validation attributes with a parameter; their [AttributeUsage]s usually include the Parameter target. However, MVC currently ignores such associations.

(Separately, [Required] would almost never be enforced for a parameter because the ComplexTypeModelBinder creates an uninitialized instance before validation starts. I.e. if we validated parameters, [Required] might only work in a [FromBody] case. So, one surprise is likely to remain.)

But, we're not changing anything about [Required] or other ValidationAttributes for this issue.

[Eilon]

@dougbu I don't think anything thinks there's an issue with applying the attribute; the problem is an inability to retrieve the metadata using the existing pipes that we have.

[SteveSandersonMS]

OK, having started on implementation, I see that we're not currently in a position to have per-usage-site overrides for this, because:

• [BindRequired] on the class corresponding to the [FromBody] property won't work, because DefaultBindingMetadataProvider only reads it when it's on a property, not on a type.
  • Technically we could change that, but I don't think it would be a good design. It makes sense to control on a per-property or per-parameter level whether input is required, but not on a per-type level (the type itself doesn't know who's going to use it, or whether they have a use case for null). I suppose that's why DefaultBindingMetadataProvider doesn't already read it from types.
• [BindRequired] on the parameter being bound doesn't work either. As @rynowak has repeatedly told us, there's no mechanism for this. IModelMetadataProvider declares methods GetMetadataForType and GetMetadataForProperties, but no corresponding method for getting metadata from properties.
  • Again, we can extend the metadata system to allow for metadata on parameters if we want, but that would be a much bigger feature than this.
• Controlling it using something like [FromBody(Behavior = FromBodyBehavior.Optional)] doesn't work either, because IBindingSourceMetadata is just a way of setting properties on BindingInfo. By the time binding actually happens, we don't have access to the FromBodyAttribute. There's no place for this information to go on BindingInfo.
  • Similarly, we could extend BindingInfo to have a property like BodyBindingIsOptional. That's not too bad, but is a bit awkward in that it's very specific to one kind of binder.

Proposal

Since virtually in all cases we think people will want to just keep the default behaviour on all actions (and the new default behaviour will be "[FromBody] means body is required"), we probably don't desperately need per-usage-site overrides right now. So I just propose to do the flag on MvcOptions, and have that control all instances of [FromBody].

In the future, if we implement parameter-level model metadata, then it would be trivial to use that to control [FromBody] behaviour per-usage-site. So I suggest we leave this open as a possible future enhancement.

@Eilon / @dougbu / @rynowak - does this sound reasonable and sufficient?

[Eilon]

I think this is perfectly acceptable, considering that it's all we initially wanted 😄 It's unfortunate that there's no straightforward way to make it per-usage, but I won't lose sleep over that.

[dougbu]

Sounds sad but reasonable.

Will [BindRequired] at least work on [FromBody] properties when the global option is off?

[SteveSandersonMS]

Will [BindRequired] at least work on [FromBody] properties when the global option is off?

If you mean properties on the type that is being bound via [FromBody], then you'd need to use [Required] (not [BindRequired]) in that case.

[Eilon]

Remove 1 - Ready and add 3 - Done label to indicate the work item was completed (it's used for the release notes).

[SteveSandersonMS]

Ah I see. Done!

[minhhuynh1812]

hi all. i have problem. i hope everybody can help me. i want validate for each attribute of a object and notifi error for each attribute. thanks you!

[dougbu]

@minhhuynh1812 please open a new issue if you have a specific scenario that is not working as you wish. You may want to start by reviewing the docs e.g. https://docs.microsoft.com/en-us/aspnet/core/mvc/models/model-binding or asking a question on https://stackoverflow.com. In fact, your question may match something already asked, perhaps in the https://stackoverflow.com/questions/tagged/asp.net-core-mvc tag.

[RodrigoFCampos]

I found this link and I hope it can guide you in the right direction:

https://stackoverflow.com/questions/43694106/asp-net-core-webapi-modelstate-not-working