Mvc: ApiController: Problems with Parameter Validation

[Tornhoof]

The new [ApiController] attribute is supposed to enable automatic modelstate validation, but it does not work as I would expect for the following controller.

[ApiController]
[Route("api/test")]
public class RequiredController : Controller
{
    [HttpGet]
    [Route("NotRequired/{value:guid}")]
    public IActionResult NotRequired(Guid value)
    {
        return Ok($"Hello World from {value}");
    }

    [HttpGet]
    [Route("Required/{value:guid}")]
    public IActionResult Required([Required] Guid value)
    {
        return Ok($"Hello World from {value}");
    }

    [HttpGet]
    [Route("BindRequired/{value:guid}")]
    public IActionResult BindRequired([BindRequired] Guid value)
    {
        return Ok($"Hello World from {value}");
    }

    [HttpGet]
    [Route("StringLength/{value}")]
    public IActionResult StringLength([Required, StringLength(2)] string value)
    {
        return Ok($"Hello World from {value}");
    }
}

Examples:

• Invoke: https://localhost:44370/api/test/BindRequired/00000000-0000-0000-0000-000000000000 - StatusCode: Ok - Output is Hello World from 00000000-0000-0000-0000-000000000000
• Invoke: https://localhost:44370/api/test/StringLength/Tornhoof - StatusCode: Ok - Output is Hello World from Tornhoof

I would have expected a validation error from both of the above calls.

Repro:
RequiredAttributeRepro.zip

• Unpack
• Build & Run
• Invoke the methods above.

[khellang]

I think you need to make value type parameters nullable in order for the Required/BindRequired attributes to kick in, otherwise it'll think the default value is a "real" value.

[Tornhoof]

Yes for the value types you might be right. My assumption that [BindRequired] rejects default too was mainly based on @filipw post https://www.strathweb.com/2017/12/required-and-bindrequired-in-asp-net-core-mvc.

But since I can't even get StringLength to work or any custom validationAttribute (like a custom NotDefaultAttribute to make certain my GUIDs are not default), I think the ParameterValidation is currently broken with ApiController.

[Tornhoof]

Tagging @mkArtakMSFT as this is probably again a binding issue.

[shravan2x]

@khellang Isn't the absence of the field in the request enough to trigger an error? Making it nullable and checking for null is trivial to do even without having an attribute for it.

[khellang]

No, because validation and deserialization are two distinct things. The former runs after the latter, with no information about "missing fields". It just gets an object with some properties with attributes on them.

If you have an int property that's missing from the payload, it'll be initialized to its default value 0. How would validation know if that represents a missing value or actual user input? It can't.

[khellang]

Making it nullable and checking for null is trivial to do even without having an attribute for it.

Sure, most validation is trivial to do. It's just much nicer to have a declarative way of expressing it, with consistent validation messages and composabilit 😊

[khellang]

Oh, I guess you might be referring to the BindRequired case? That's probably true.

[shravan2x]

Yeah I'm referring to the BindRequired case. There are situations where it would be useful to enforce that a client has explicitly set a parameter.

[shravan2x]

I think this is another instance of the same issue (please point out if it isn't). I have this code:

```c#
[Route("{id}/online")]
[Consumes("application/json")]
[Produces("application/json")]
public async Task SetOnline(uint id, [FromBody] SetOnlineData data)
{
return Json(new { test = "ok " + id + " " + data.Online, ModelState.IsValid });
}

public class SetOnlineData
{
[Required]
[BindRequired]
[JsonProperty("online")]
public bool Online { get; private set; }
}

I would expect that `ModelState.IsValid` is `false` when an empty JSON object (`{}`)is passed as a body, but it is still true. The output is:

```js
{
    "test": "ok 3 False",
    "isValid": true
}

[mkArtakMSFT]

@dougbu, can you please look into this one?
Specifically, the case with StringLength restriction in place is what seems to be a bug.

[dougbu]

@Tornhoof what version of MVC are you testing? Validation of "top-level" nodes such as action parameters is a new feature in 2.1 Preview 1.

Separately,

I would expect that ModelState.IsValid is false when an empty JSON object ({})is passed as a body,

MVC's model binding system supports [BindRequired]. But, Json.NET's deserializers take over when the model binding system encounters [FromBody]. MVC no longer has information about whether Online was in the request.

[shravan2x]

@dougbu Imo this is still a design flaw. It's a technical detail reducing usability of the platform. Could MVC use a custom deserializer to fix this?

[dougbu]

Could MVC use a custom deserializer to fix this?

Recreating a system as complex as Json.NET is not in our plans. You could file a bug in https://github.com/JamesNK/Newtonsoft.Json/issues about supporting [BindRequired].

However I suggest testing [JsonRequired] to confirm it affects deserialization as well. If it does, apply both [BindRequired] and [JsonRequired] to properties that are both model bound and input formatted.

[Tornhoof]

I'm testing 2.1-preview1-final.

[shravan2x]

@dougbu [JsonRequired] seems to work.

Recreating a system as complex as Json.NET is not in our plans.

I think you misunderstood what I said. I was asking if Json.NET supported some interface we could implement (like IContractResolver, but I'm not too familiar with all of Json.NET's features) which would check [BindRequired] attributes for us.

[dougbu]

@Tornhoof your original RequiredAttributeRepro application is simply not configured to support anything from System.ComponentModel.DataAnnotations. AddMvcCore() only turns on the bare minimum. Change your Startup to use
c# services .AddMvcCore() .AddDataAnnotations() .AddJsonFormatters();
and you'll see the expected validation error. (Probably best to use AddMvc() since your controller is a Controller subclass, not a ControllerBase subclass.)

---

Separately, the other actions in RequiredController do not test [BindRequired] or [Required]. The route template ({value:guid}) requires value and ensures its format matches a Guid. Change the template to {value:guid?} and value will be optional. At that point, [Required] will do nothing because Guid is a simple type. But, MVC will enforce [BindRequired].

[Tornhoof]

Thanks for the clarification. Closing this.