Mvc: Add support for validation attributes on parameters

Created on 8 Sep 2017  路  9Comments  路  Source: aspnet/Mvc


Update: As of 2.1.0, validation attributes are allowed on controller parameters, bound controller parameters, page model properties and handler method parameters. To use this feature, the application must opt in to 2.1 (or newer) compatibility behavior.

services.AddMvc()
                .SetCompatibilityVersion(CompatibilityVersion.Version_2_1);
3 - Done enhancement

Most helpful comment

@rynowak This is slightly off-topic but wanted to get a quick sanity check before taking the thought any further. Once again I had to do research to remember the difference between [Required] and [BindRequired]. The only blog post on the subject has no clear conclusion, so it wasn't until finding Doug's comments that it became clear. So now I remember how they serve different purposes, but it's super-non-obvious from the outside.

To avoid all this, has there ever been consideration for a "strict" binding mode, where all non-nullable properties implicitly have [BindRequired]? This feels more intuitive, in that the developer can only allow properties to be optional by using nullable types. As long as you understand C#, you understand which inputs you're going to get. And then [Required] reliably means what you expect, since it behaves the same on int? as it does on string (as opposed to on int, where has no effect). Thinking ahead further, this would also mean that C#8 non-nullable reference types would fit into the pattern: a string! would automatically be binding-required, as opposed to in the current system where MVC would have to supply string.Empty or something, and claim it was valid, which would be pretty lame.

All 9 comments

I am very excited for this. So many times I have wanted the following to work:

[HttpGet("test/{testId}")]
public IActionResult Get([FromRoute] string testId, [Required][FromQuery]string name)

or

[HttpGet("test/{testId}")]
public IActionResult Get([FromRoute] string testId, [FromQuery(Required = true)]string name)

Where ModelState would be invalid if name was null or empty. It would be even better, as proposed, to detect invalid state before the action is called. Also having the ability to easily define custom validation parameter attributes is a _must_, IMO.

I prefer [FromQuery(Required = true)] over a separate [Required] attribute. I propose that Required defaults to true and the developer explicitly states _false_ when not required. I propose also adding a DefaultValue for fallback.

@rynowak This is slightly off-topic but wanted to get a quick sanity check before taking the thought any further. Once again I had to do research to remember the difference between [Required] and [BindRequired]. The only blog post on the subject has no clear conclusion, so it wasn't until finding Doug's comments that it became clear. So now I remember how they serve different purposes, but it's super-non-obvious from the outside.

To avoid all this, has there ever been consideration for a "strict" binding mode, where all non-nullable properties implicitly have [BindRequired]? This feels more intuitive, in that the developer can only allow properties to be optional by using nullable types. As long as you understand C#, you understand which inputs you're going to get. And then [Required] reliably means what you expect, since it behaves the same on int? as it does on string (as opposed to on int, where has no effect). Thinking ahead further, this would also mean that C#8 non-nullable reference types would fit into the pattern: a string! would automatically be binding-required, as opposed to in the current system where MVC would have to supply string.Empty or something, and claim it was valid, which would be pretty lame.

To avoid all this, has there ever been consideration for a "strict" binding mode, where all non-nullable properties implicitly have

There are lots of ideas, but few of them about how to rationalize this with MVC's current and past behavior. For instance the concept of 'model binding' is hardly relevant to APIs but it's critical for browsers/forms. You will extremely rarely see a complex type used with model binding in an API.

OTOH browers/forms always send a value (empty string) for every input in the form, so what does this proposed behavior add in that case?

Another vote for [BindRequired] to be implicit on value types.

There is a nice blog post on how you can force the [Required] attribute to also behave like [BindRequired], thus removing the need to use [BindRequired] completely. Also, this has the added benefit of removing the dependency on MVC, since [Required] is in the System namespace.

Given

[HttpGet("action")]
public IActionResult Get([BindRequired] int? intVal)
{
    if(!ModelState.IsValid)
    {
        return BadRequest(ModelState);
    }

    return Ok(intVal);
}

What should the responses be for the following requests?

  1. GET api/controller/action
  2. GET api/controller/action?intVal
  3. GET api/controller/action?intVal=
  4. GET api/controller/action?intVal=null
  5. GET api/controller/action?intVal=1

One would think this, or am I being naive?

  1. GET api/controller/action => 404
  2. GET api/controller/action?intVal => 200 null
  3. GET api/controller/action?intVal= => 200 null
  4. GET api/controller/action?intVal=null => 200 null
  5. GET api/controller/action?intVal=1 => 200 1

Instead what I get back is this:

  1. GET api/controller/action => 400
{
  "intVal": [
    "A value for the 'intVal' property was not provided."
  ]
}
  1. GET api/controller/action?intVal => 204
  2. GET api/controller/action?intVal= => 204
  3. GET api/controller/action?intVal=null => 400
{
  "intVal": [
    "The value 'null' is not valid for intVal.",
    "A value for the 'intVal' property was not provided."
  ]
}
  1. GET api/controller/action?intVal=1 => 200 1
  1. Should be a 404 since that route does not exist
  2. Should return null, the key exists, but it has no value, which we can assume to be null. It surely shouldn't send back 204?!
  3. Same as 2.
  4. Should return 200 null, It's valid to put prop: null in body requests, so why not in the URL?
  5. No problem here 馃槃

Should only return a 400 if validation rules are violated, e.g. if I put a range constraint on intVal for instance.


Regarding point 2, given a BindRequired non-nullable, e.g.

[HttpGet("action2")]
public IActionResult Get([BindRequired] int intVal)
{
    if(!ModelState.IsValid)
    {
        return BadRequest(ModelState);
    }

    return Ok(intVal);
}

For GET /action2?intVal and GET /action2?intVal= _should_ return a 400. Since a null value to an int is invalid. (And for GET /action2?intVal={anything that isn't an int})

GET /action2 should return 404.


Furthermore, the inclusion of a required attribute (whether this be Required, BindRequired or a future addition) should work as an action constraint. This would allow for nice, neat, and arguably more correct, apis. The following of which is currently not possible.

GET /folders => Gets entire tree
GET /folders?parentId=null => Gets root folders
GET /folders?parentId=1 => Gets child folders of 1

[HttpGet("folders")]
public IActionResult GetAll()
{
    var folders = context.Folders.ToList();

    return Ok(folders);
}

[HttpGet("folders")]
public IActionResult GetChildren([BindRequired] int? parentId)
{
    var folders = context.Folders.Where(x => x.parentId == parentId)
        .ToList();

    return Ok(folders);
}

Hope this makes sense!

N.B. this is 2.1.0-preview1-final

Can I create my own filter to validate a primitive parameter, e.g. Get([MyValidation]string name)?

Hi, it looks like you are posting on a closed issue/PR/commit!

We're very likely to lose track of your bug/feedback/question unless you:

  1. Open a new issue
  2. Explain very clearly what you need help with
  3. If you think you have found a bug, include detailed repro steps so that we can investigate the problem

Thanks!

Is this feature included in Microsoft.AspNetCore.All package version 2.1.2?
Just upgraded, and can not see any effect....

You're likely missing the 2.1 compat flag. See https://docs.microsoft.com/en-us/aspnet/core/migration/20_21?view=aspnetcore-2.1#changes-to-startup for migration instructions.

Was this page helpful?
0 / 5 - 0 ratings