Model validation: Exception thrown when there are more than (MaxAllowedErrors + MaxValidationDepth) errors

[doominator42]

Describe the bug

When a model is validated and there is more than (MaxAllowedErrors + MaxValidationDepth) errors, the MaxValidationDepth exception is thrown, regardless of the actual depth.

To Reproduce

Steps to reproduce the behavior:

1. Run project https://github.com/doominator42/aspnet-core-bug-repro
2. Navigate to home page and click button "Click here to reproduce bug"
3. See exception page showing:

InvalidOperationException: ValidationVisitor exceeded the maximum configured validation depth '32' when validating type 'MaxValidationDepthBugRepro.Models.BugReproModel'. This may indicate a very deep or infinitely recursive object graph. Consider modifying 'MvcOptions.MaxValidationDepth' or suppressing validation on the model type.

    Microsoft.AspNetCore.Mvc.ModelBinding.Validation.ValidationVisitor.Visit(ModelMetadata metadata, string key, object model)
    Microsoft.AspNetCore.Mvc.ModelBinding.Validation.ValidationVisitor.VisitChildren(IValidationStrategy strategy)
    Microsoft.AspNetCore.Mvc.ModelBinding.Validation.ValidationVisitor.VisitComplexType(IValidationStrategy defaultStrategy)
    Microsoft.AspNetCore.Mvc.ModelBinding.Validation.ValidationVisitor.Visit(ModelMetadata metadata, string key, object model)
    Microsoft.AspNetCore.Mvc.ModelBinding.Validation.ValidationVisitor.Validate(ModelMetadata metadata, string key, object model, bool alwaysValidateAtTopLevel)
    Microsoft.AspNetCore.Mvc.ModelBinding.ObjectModelValidator.Validate(ActionContext actionContext, ValidationStateDictionary validationState, string prefix, object model, ModelMetadata metadata)
    Microsoft.AspNetCore.Mvc.ModelBinding.ParameterBinder.EnforceBindRequiredAndValidate(ObjectModelValidator baseObjectValidator, ActionContext actionContext, ParameterDescriptor parameter, ModelMetadata metadata, ModelBindingContext modelBindingContext, ModelBindingResult modelBindingResult)
    Microsoft.AspNetCore.Mvc.ModelBinding.ParameterBinder.BindModelAsync(ActionContext actionContext, IModelBinder modelBinder, IValueProvider valueProvider, ParameterDescriptor parameter, ModelMetadata metadata, object value)
    Microsoft.AspNetCore.Mvc.Internal.ControllerBinderDelegateProvider+<>c__DisplayClass0_0+<<CreateBinderDelegate>g__Bind|0>d.MoveNext()
    Microsoft.AspNetCore.Mvc.Internal.ControllerActionInvoker.InvokeInnerFilterAsync()
    Microsoft.AspNetCore.Mvc.Internal.ResourceInvoker.InvokeNextResourceFilter()
    Microsoft.AspNetCore.Mvc.Internal.ResourceInvoker.Rethrow(ResourceExecutedContext context)
    Microsoft.AspNetCore.Mvc.Internal.ResourceInvoker.Next(ref State next, ref Scope scope, ref object state, ref bool isCompleted)
    Microsoft.AspNetCore.Mvc.Internal.ResourceInvoker.InvokeFilterPipelineAsync()
    Microsoft.AspNetCore.Mvc.Internal.ResourceInvoker.InvokeAsync()
    Microsoft.AspNetCore.Routing.EndpointMiddleware.Invoke(HttpContext httpContext)
    Microsoft.AspNetCore.Routing.EndpointRoutingMiddleware.Invoke(HttpContext httpContext)
    Microsoft.AspNetCore.StaticFiles.StaticFileMiddleware.Invoke(HttpContext context)
    Microsoft.AspNetCore.Diagnostics.DeveloperExceptionPageMiddleware.Invoke(HttpContext context)

Expected behavior

Validation should continue, ignore further errors and then execute the action to be able to display errors to the user.

Additional context

The repro project is the default "Web Application (Model-View-Controller)" template with the BugRepro action and form added.
This bug also occurs with WebApi.

My investigation

In ValidationVisitor.Visit(), line 219, model is pushed on the validation depth stack, and is popped after validation. But when ModelState.HasReachedMaxErrors is true, the model is not popped from the stack and the validation continues. So the stack grows to reaching the MaxValidationDepth.
From what I understand popping the model from the stack right before line 260 should fix the issue, but some input would be appreciated before doing a PR.

[mkArtakMSFT]

@ryanbrandenburg can you please verify the repro?
If you can confirm this behavior let's take it for 5.0.

[ryanbrandenburg]

Before I investigate further, wouldn't this be "by design" as stated? The idea behind MaxAllowedErrors and MaxValidationDepth seems to be to avoid recursive error adding infinitely. So when this happens it's a code problem rather than a user problem and the data you would show the user is likely crazy (16 instances each of the same two error messages for example). If it is intended that there be this much depth then you would modify the value as the error message suggests.

@pranavkm is my thinking here correct?

[pranavkm]

If you look at the model @doominator42 shared, there isn't a recursive model, just a list of models with a single property each. The expectation here would be that model validation fails and you get a report with MaxAllowedErrors number of errors, but not a 500 exceeding the validation depth.

[doominator42]

Thanks for looking into this.

@mkArtakMSFT can this be added to 3.0 or the release is already done?

I know it is not a priority issue, but I could not find any workaround except to increase MaxAllowedErrors or MaxValidationDepth which is really not ideal. I also tried to create an ExceptionFilter, but I did not find how to retrieve the ModelState within the filter. At least if we could find a workaround it would be good.

[rynowak]

3.0 released today 😆

[ericl85]

https://github.com/aspnet/AspNetCore/blob/602cb1dea59751a1297d674cddf7f0729c23e6ca/src/Mvc/Mvc.Core/src/ModelBinding/Validation/ValidationVisitor.cs#L258-L269

I think I just ran into this today. After digging a bit, I believe the issue is the lack of _currentPath.Pop(model) in the if (ModelState.HasReachMaxedErrors) branch.

If you had -- for example -- a large array of objects with validation errors (as I have), I think it would continue validating each object even after the max errors had been hit and you'd eventually hit MaxValidationDepth.

I don't understand the code well enough to know if that is intentional though.

I worked around it by suppressing the validations on that object and my larger fix is avoiding the validation errors altogether. But it was definitely a confusing error on a model with a maximum depth of 3.