diff --git a/src/Mvc/Mvc.Core/src/ModelBinding/Validation/ValidationVisitor.cs b/src/Mvc/Mvc.Core/src/ModelBinding/Validation/ValidationVisitor.cs index 677f850aba..963db52af1 100644 --- a/src/Mvc/Mvc.Core/src/ModelBinding/Validation/ValidationVisitor.cs +++ b/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); } } } diff --git a/src/Mvc/Mvc.Core/test/ModelBinding/Validation/DefaultObjectValidatorTests.cs b/src/Mvc/Mvc.Core/test/ModelBinding/Validation/DefaultObjectValidatorTests.cs index b1a5254e02..3be2f0e55f 100644 --- a/src/Mvc/Mvc.Core/test/ModelBinding/Validation/DefaultObjectValidatorTests.cs +++ b/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] - public void Validate_Throws_IfValidationDepthExceedsMaxDepth() + [Theory] + [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(() => 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 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."; + var maxDepth = 4; _options.MaxValidationDepth = maxDepth; var actionContext = new ActionContext(); + actionContext.ModelState.MaxAllowedErrors = 2; var validator = CreateValidator(); - var model = new DepthObject(maxDepth + 1); + var model = new List + { + 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(() => validator.Validate(actionContext, validationState, prefix: string.Empty, model)); - Assert.Equal(expected, ex.Message); - Assert.NotNull(ex.HelpLink); + validator.Validate(actionContext, validationState, prefix: string.Empty, model); + Assert.False(actionContext.ModelState.IsValid); } [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; }