Skip to content
This repository has been archived by the owner on Dec 14, 2018. It is now read-only.

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

Closed
bgeihsgt opened this issue Dec 11, 2015 · 6 comments
Assignees
Milestone

Comments

@bgeihsgt
Copy link

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
Copy link
Member

Eilon commented Dec 28, 2015

@rynowak any idea about this?

@rynowak
Copy link
Member

rynowak commented Dec 28, 2015

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
Copy link
Member

rynowak commented Dec 28, 2015

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

@Eilon Eilon added this to the 6.0.0-rc2 milestone Dec 28, 2015
@bgeihsgt
Copy link
Author

bgeihsgt commented Jan 7, 2016

@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
Copy link
Member

rynowak commented Jan 8, 2016

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 added a commit that referenced this issue Jan 28, 2016
This is a fix for the specific case reported in #3743 and also a few
related cases where the object being validated does not come from user
input.

Because the CancellationToken is bound using the 'empty prefix',
suppressing validation causes the validation system to suppress ALL keys
in the MSD. This will wipe out validation info for other, unrelated model
data. The same can occur for [FromServices], IFormCollection, IFormFile,
and HttpRequestMessage (anythere when MSD key == null and
SuppressValidation == true).

The change is to just ignore anything where the key is null and
SuppressValidation == true.
rynowak added a commit that referenced this issue Jan 29, 2016
This is a fix for the specific case reported in #3743 and also a few
related cases where the object being validated does not come from user
input.

Because the CancellationToken is bound using the 'empty prefix',
suppressing validation causes the validation system to suppress ALL keys
in the MSD. This will wipe out validation info for other, unrelated model
data. The same can occur for [FromServices], IFormCollection, IFormFile,
and HttpRequestMessage (anythere when MSD key == null and
SuppressValidation == true).

The other change here is related to the [FromBody]. We don't want to
create an entry in the model state for a 'valid' model with the empty
prefix. This can cause the system to miss validation errors. If you end up
in a situation where there's an entry with the empty prefix for an invalid
state, you won't accidentally miss validation errors.
rynowak added a commit that referenced this issue Jan 30, 2016
This is a fix for the specific case reported in #3743 and also a few
related cases where the object being validated does not come from user
input.

Because the CancellationToken is bound using the 'empty prefix',
suppressing validation causes the validation system to suppress ALL keys
in the MSD. This will wipe out validation info for other, unrelated model
data. The same can occur for [FromServices], IFormCollection, IFormFile,
and HttpRequestMessage (anythere when MSD key == null and
SuppressValidation == true).

The other change here is related to the [FromBody]. We don't want to
create an entry in the model state for a 'valid' model with the empty
prefix. This can cause the system to miss validation errors. If you end up
in a situation where there's an entry with the empty prefix for an invalid
state, you won't accidentally miss validation errors.
@rynowak
Copy link
Member

rynowak commented Feb 1, 2016

Fixed by faba952

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

3 participants