ModelValidation: Fixed MaxDepth exception when there are more items left then MaxValidationDepth after reaching MaxAllowedErrors (#18212)

* Consolidated similar tests in a theory

* Fix for InvalidOperationException when reached maxAllowedErrors and there are more items left then max depth

* Refactored Visitor logic to reduce the risk of a missing Pop.

* Added comment on when it can throw

* Updated reference assembly

* Reverted publid API change

src/Mvc/Mvc.Core/src/ModelBinding/Validation/ValidationVisitor.cs

@@ -223,13 +223,25 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding.Validation
                 return true;
             }
 
+            bool result;
+            try
+            {
+                // Throws InvalidOperationException if the object graph is too deep
+                result = VisitImplementation(ref metadata, ref key, model);
+            }
+            finally
+            {
+                _currentPath.Pop(model);
+            }
+            return result;
+        }
+
+        private bool VisitImplementation(ref ModelMetadata metadata, ref string key, object model)
+        {
             if (MaxValidationDepth != null && _currentPath.Count > MaxValidationDepth)
             {
                 // Non cyclic but too deep an object graph.
 
-                // Pop the current model to make ValidationStack.Dispose happy
-                _currentPath.Pop(model);
-
                 string message;
                 switch (metadata.MetadataKind)
                 {
@@ -264,7 +276,6 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding.Validation
             {
                 // Use the key on the entry, because we might not have entries in model state.
                 SuppressValidation(entry.Key);
-                _currentPath.Pop(model);
                 return true;
             }
             // If the metadata indicates that no validators exist AND the aggregate state for the key says that the model graph
@@ -282,7 +293,6 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding.Validation
                     }
                 }
 
-                _currentPath.Pop(model);
                 return true;
             }
 
@@ -401,7 +411,6 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding.Validation
             private readonly string _key;
             private readonly ModelMetadata _metadata;
             private readonly object _model;
-            private readonly object _newModel;
             private readonly IValidationStrategy _strategy;
 
             public static StateManager Recurse(
@@ -411,7 +420,7 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding.Validation
                 object model,
                 IValidationStrategy strategy)
             {
-                var recursifier = new StateManager(visitor, model);
+                var recursifier = new StateManager(visitor, null);
 
                 visitor.Container = visitor.Model;
                 visitor.Key = key;
@@ -425,7 +434,6 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding.Validation
             public StateManager(ValidationVisitor visitor, object newModel)
             {
                 _visitor = visitor;
-                _newModel = newModel;
 
                 _container = _visitor.Container;
                 _key = _visitor.Key;
@@ -441,8 +449,6 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding.Validation
                 _visitor.Metadata = _metadata;
                 _visitor.Model = _model;
                 _visitor.Strategy = _strategy;
-
-                _visitor._currentPath.Pop(_newModel);
             }
         }
     }

src/Mvc/Mvc.Core/test/ModelBinding/Validation/DefaultObjectValidatorTests.cs

@@ -6,7 +6,6 @@ using System.Collections.Generic;
 using System.ComponentModel.DataAnnotations;
 using System.IO;
 using System.Linq;
-using System.Reflection;
 using System.Text;
 using Microsoft.AspNetCore.Http;
 using Microsoft.AspNetCore.Mvc.Infrastructure;
@@ -1264,11 +1263,12 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding.Validation
                 });
         }
 
-        [Fact]
+        [Theory]
-        public void Validate_Throws_IfValidationDepthExceedsMaxDepth()
+        [InlineData(1)]
+        [InlineData(5)]
+        public void Validate_Throws_IfValidationDepthExceedsMaxDepth(int maxDepth)
         {
             // Arrange
-            var maxDepth = 5;
             var expected = $"ValidationVisitor exceeded the maximum configured validation depth '{maxDepth}' when validating property '{nameof(DepthObject.Depth)}' on type '{typeof(DepthObject)}'. " +
                 "This may indicate a very deep or infinitely recursive object graph. Consider modifying 'MvcOptions.MaxValidationDepth' or suppressing validation on the model type.";
             _options.MaxValidationDepth = maxDepth;
@@ -1283,6 +1283,7 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding.Validation
             // Act & Assert
             var ex = Assert.Throws<InvalidOperationException>(() => validator.Validate(actionContext, validationState, prefix: string.Empty, model));
             Assert.Equal(expected, ex.Message);
+            Assert.Equal("https://aka.ms/AA21ue1", ex.HelpLink);
         }
 
         [Fact]
@@ -1305,26 +1306,32 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding.Validation
         }
 
         [Fact]
-        public void Validate_Throws_WithMaxDepth_1()
+        public void Validate_DoesNotThrow_IfNumberOfErrorsAfterReachingMaxAllowedErrors_GoesOverMaxDepth()
         {
             // Arrange
-            var maxDepth = 1;
+            var maxDepth = 4;
-            var expected = $"ValidationVisitor exceeded the maximum configured validation depth '{maxDepth}' when validating property '{nameof(DepthObject.Depth)}' on type '{typeof(DepthObject)}'. " +
-                "This may indicate a very deep or infinitely recursive object graph. Consider modifying 'MvcOptions.MaxValidationDepth' or suppressing validation on the model type.";
             _options.MaxValidationDepth = maxDepth;
             var actionContext = new ActionContext();
+            actionContext.ModelState.MaxAllowedErrors = 2;
             var validator = CreateValidator();
-            var model = new DepthObject(maxDepth + 1);
+            var model = new List<ModelWithRequiredProperty>
+            {
+                new ModelWithRequiredProperty(), new ModelWithRequiredProperty(),
+                // After the first 2 items we will reach MaxAllowedErrors
+                // If we add items without popping after having reached max validation,
+                // with 4 more items (on top of the list) we would go over max depth of 4
+                new ModelWithRequiredProperty(), new ModelWithRequiredProperty(),
+                new ModelWithRequiredProperty(), new ModelWithRequiredProperty(),
+            };
+
             var validationState = new ValidationStateDictionary
             {
                 { model, new ValidationStateEntry() }
             };
-            var method = GetType().GetMethod(nameof(Validate_Throws_ForTopLevelMetadataData), BindingFlags.NonPublic | BindingFlags.Instance);
 
             // Act & Assert
-            var ex = Assert.Throws<InvalidOperationException>(() => validator.Validate(actionContext, validationState, prefix: string.Empty, model));
+            validator.Validate(actionContext, validationState, prefix: string.Empty, model);
-            Assert.Equal(expected, ex.Message);
+            Assert.False(actionContext.ModelState.IsValid);
-            Assert.NotNull(ex.HelpLink);
         }
 
         [Theory]
@@ -1443,6 +1450,12 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding.Validation
             Assert.Equal(ModelValidationState.Invalid, entry.ValidationState);
         }
 
+        public class ModelWithRequiredProperty
+        {
+            [Required]
+            public string MyProperty { get; set; }
+        }
+
         private class ModelWithoutValidation
         {
             public string Property1 { get; set; }