-
Notifications
You must be signed in to change notification settings - Fork 2.1k
RC1: Model validation reports invalid models as valid when an a controller action contains a CancellationToken parameter #3743
Comments
@rynowak any idea about this? |
The issue with this is exactly as reported:
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 Because these keys overlap, clearing the validation state for the Two changes, second one might be optional depending on how we do the first:
Personally I'm in favor of using a magic token like
If |
@bgeihsgt - thanks for the detailed report and investigation. You should be able to work around this for now by using 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. |
@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? |
Sorry, I should have said more than just 'not useful'. The default 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.
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. |
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.
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.
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.
Fixed by faba952 |
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:
If you post an invalid model to the
without-cancellation
, it will properly return a 400. If you post an invalid model towith-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 ofstring.Empty
. We dug into why this was key was empty we found that theCompositeModelBinder.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.
The text was updated successfully, but these errors were encountered: