RC1: Model validation reports invalid models as valid when an a controller action contains a CancellationToken parameter

[bgeihsgt]

We were upgrading our project to RC1 and noticed model validation stopped working after the upgrade. The short version is that having a CancellationToken in your action method will cause all models to get marked with ModelValidationState.Skipped.

Here's a simple setup that demonstrates the problem:

    [Route("api/foo")]
    public class FooController : Controller
    {
        [Route("with-cancellation")]
        [HttpPost]
        public async Task<IActionResult> CreateWithCancellation([FromBody] Foo foo, CancellationToken cancellationToken)
        {
            if (!ModelState.IsValid)
                return new BadRequestResult();

            await DoStuff();

            return Ok(foo);
        }

        [Route("without-cancellation")]
        [HttpPost]
        public async Task<IActionResult> CreateWithoutCancellation([FromBody] Foo foo)
        {
            if (!ModelState.IsValid)
                return new BadRequestResult();

            await DoStuff();

            return Ok(foo);
        }

        public Task DoStuff()
        {
            return Task.FromResult(0);
        }
    }

    public class Foo
    {
        [Required]
        public string requiredProperty { get; set; }
    }

If you post an invalid model to the without-cancellation, it will properly return a 400. If you post an invalid model to with-cancellation it will return a 200 with an invalid model.

After debugging through the problem, we found that the ValidationVisitor for the cancellation token was overwriting all of the model validation results to ModelValidationResult.Skipped.

SuppressValidation rightly gets called for CancellationToken, but it wrongly suppresses it for all entries because the CancellationToken entry has a key of string.Empty. We dug into why this was key was empty we found that the CompositeModelBinder.CreateNewBindingContext is the culprit (see https://github.com/aspnet/Mvc/blob/6.0.0-rc1/src/Microsoft.AspNet.Mvc.Core/ModelBinding/CompositeModelBinder.cs#L101-L168).

In the CancellationToken case, it is falling through to the else block which sets ModelName to string.Empty when it really should be the old binding context's name.

[Eilon]

@rynowak any idea about this?

[rynowak]

The issue with this is exactly as reported:

SuppressValidation rightly gets called for CancellationToken, but it wrongly suppresses it for all entries because the CancellationToken entry has a key of string.Empty.

The problem here is overlap of model state keys between the things that aren't bound using traditional model binding. In this case, both the [FromBody] model object and the CancellationToken use the string.Empty model binding key. In any case this really isn't relevant, it's just how the system works.

Because these keys overlap, clearing the validation state for the CancellationToken clears the validation state for the actual model.

Two changes, second one might be optional depending on how we do the first:

• Don't use string.Empty when the traditional concept of model binding 'key' doesn't apply - we could use null or $body or $header:Foo or $cancellationToken.
• ValidationVisitor should ignore things that have a null key because we know it's not user input

Personally I'm in favor of using a magic token like $body or $header for things that are user input, and null for things that are not. Otherwise, you could still have a problem in a case like this:

[HttpPost]
public async Task<IActionResult> CreateWithCancellation([FromBody] Foo foo, Bar bar)
{
}

If bar does fallback-to-empty-prefix, then you still have overlap of keys, since foo will generally always end up with the empty prefix.

[rynowak]

@bgeihsgt - thanks for the detailed report and investigation. You should be able to work around this for now by using [ModelBinder(Name = "whatever")] on your cancellation token parameter.

I'd also be cautious about doing to much with the cancellation token in your actions. Unless you're wiring up your own token, there's not much useful you can do with the built-in one.

Our output stream will blow up if you try to write to it after the client has disconnected, so you're unlikely to really save yourself any trouble.

[bgeihsgt]

@rynowak I'm curious, if the built-in CancellationToken isn't very useful, what is the intended purpose for it? And how would our own token solve the output stream issue you mentioned?

[rynowak]

I'm curious, if the build-in CancellationToken isn't very useful, what is the intended purpose for it? And how would our own token solve the output stream issue you mentioned?

Sorry, I should have said more than just 'not useful'.

The default CancellationToken provided by model binding is HttpContext.RequestAborted - which is tripped when request.Abort() is called. So first-off, if you're not calling request.Abort() you won't ever see it tripped. You need to implement a policy if you want to have cancellation.

---

Secondly, some history: WebAPI provided a different token, one that would be tripped when the client disconnects. This is a bit misleading, because it's really rare that you would want to cancel an operation because the client disconnected in the middle. Would you rollback a database transaction because the user closed the tab after hitting submit?

The idea here was that you'd want to stop doing something like writing out a response if no one is going to see it.

In practice this caused a lot of trouble. Cancellation results in an exception, and you'd most frequently encounter this while serializing the output. Throwing can often be more expensive than just piping the output to /dev/null. Additionally, this makes life harder for production monitoring, as you've now got a bunch of 'unhandled' exceptions showing up in logs.

---

And how would our own token solve the output stream issue you mentioned?

I just talked to @halter73 - it sounds like it's not super likely you'll hit exceptions writing the response if the client disconnects halfway through. We've discussed making a change to have the response stream just go to /dev/null after the client disconnects, which would prevent you from seeing these errors.

It's not really a problem 'per-se' just something to be aware of.

[rynowak]

Fixed by faba952