Do not suppress ModelValidationState.Invalid entries

- #7992, #7963
aspnet · Jul 13, 2018 · f2608c2 · f2608c2
1 parent dfbdb37
commit f2608c2
Show file tree

Hide file tree

Showing 3 changed files with 193 additions and 23 deletions.
diff --git a/src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/Validation/ValidationVisitor.cs b/src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/Validation/ValidationVisitor.cs
@@ -296,7 +296,10 @@ protected virtual void SuppressValidation(string key)
             var entries = ModelState.FindKeysWithPrefix(key);
             foreach (var entry in entries)
             {
-                entry.Value.ValidationState = ModelValidationState.Skipped;
+                if (entry.Value.ValidationState != ModelValidationState.Invalid)
+                {
+                    entry.Value.ValidationState = ModelValidationState.Skipped;
+                }
             }
         }
 

diff --git a/test/Microsoft.AspNetCore.Mvc.Core.Test/Internal/DefaultObjectValidatorTests.cs b/test/Microsoft.AspNetCore.Mvc.Core.Test/Internal/DefaultObjectValidatorTests.cs
@@ -133,6 +133,27 @@ public void Validate_SimpleType_SuppressValidation()
             Assert.Empty(entry.Errors);
         }
 
+        // More like how product code does suppressions than Validate_SimpleType_SuppressValidation()
+        [Fact]
+        public void Validate_SimpleType_SuppressValidationWithNullKey()
+        {
+            // Arrange
+            var actionContext = new ActionContext();
+            var modelState = actionContext.ModelState;
+            var validator = CreateValidator();
+            var model = "test";
+            var validationState = new ValidationStateDictionary
+            {
+                { model, new ValidationStateEntry { SuppressValidation = true } }
+            };
+
+            // Act
+            validator.Validate(actionContext, validationState, "parameter", model);
+
+            // Assert
+            Assert.True(modelState.IsValid);
+            Assert.Empty(modelState);
+        }
 
         [Fact]
         public void Validate_ComplexValueType_Valid()
@@ -1195,6 +1216,48 @@ public void Validate_SuppressesValidation_ForExcludedType_Stream()
             Assert.Empty(entry.Value.Errors);
         }
 
+        // Regression test for aspnet/Mvc#7992
+        [Fact]
+        public void Validate_SuppressValidation_AfterHasReachedMaxErrors_Invalid()
+        {
+            // Arrange
+            var actionContext = new ActionContext();
+            var modelState = actionContext.ModelState;
+            modelState.MaxAllowedErrors = 2;
+            modelState.AddModelError(key: "one", errorMessage: "1");
+            modelState.AddModelError(key: "two", errorMessage: "2");
+
+            var validator = CreateValidator();
+            var model = (object)23; // Box ASAP
+            var validationState = new ValidationStateDictionary
+            {
+                { model, new ValidationStateEntry { SuppressValidation = true } }
+            };
+
+            // Act
+            validator.Validate(actionContext, validationState, prefix: string.Empty, model);
+
+            // Assert
+            Assert.False(modelState.IsValid);
+            Assert.True(modelState.HasReachedMaxErrors);
+            Assert.Collection(
+                modelState,
+                kvp =>
+                {
+                    Assert.Empty(kvp.Key);
+                    Assert.Equal(ModelValidationState.Invalid, kvp.Value.ValidationState);
+                    var error = Assert.Single(kvp.Value.Errors);
+                    Assert.IsType<TooManyModelErrorsException>(error.Exception);
+                },
+                kvp =>
+                {
+                    Assert.Equal("one", kvp.Key);
+                    Assert.Equal(ModelValidationState.Invalid, kvp.Value.ValidationState);
+                    var error = Assert.Single(kvp.Value.Errors);
+                    Assert.Equal("1", error.ErrorMessage);
+                });
+        }
+
         private static DefaultObjectValidator CreateValidator(Type excludedType)
         {
             var excludeFilters = new List<SuppressChildValidationMetadataProvider>();

diff --git a/test/Microsoft.AspNetCore.Mvc.Core.Test/ModelBinding/ParameterBinderTest.cs b/test/Microsoft.AspNetCore.Mvc.Core.Test/ModelBinding/ParameterBinderTest.cs
@@ -8,6 +8,7 @@
 using System.Reflection;
 using System.Threading.Tasks;
 using Microsoft.AspNetCore.Http;
+using Microsoft.AspNetCore.JsonPatch;
 using Microsoft.AspNetCore.Mvc.Abstractions;
 using Microsoft.AspNetCore.Mvc.Controllers;
 using Microsoft.AspNetCore.Mvc.DataAnnotations;
@@ -706,6 +707,131 @@ public async Task BindModelAsync_ForProperty_UsesValidationOnProperty_WhenDerive
                 });
         }
 
+        // Regression test 1 for aspnet/Mvc#7963. ModelState should never be valid.
+        [Fact]
+        public async Task BindModelAsync_ForOverlappingParametersWithSuppressions_InValid_WithValidSecondParameter()
+        {
+            // Arrange
+            var parameterDescriptor = new ParameterDescriptor
+            {
+                Name = "patchDocument",
+                ParameterType = typeof(IJsonPatchDocument),
+            };
+
+            var actionContext = GetControllerContext();
+            var modelState = actionContext.ModelState;
+
+            // First ModelState key is not empty to match SimpleTypeModelBinder.
+            modelState.SetModelValue("id", "notAGuid", "notAGuid");
+            modelState.AddModelError("id", "This is not valid.");
+
+            var modelMetadataProvider = new TestModelMetadataProvider();
+            modelMetadataProvider.ForType<IJsonPatchDocument>().ValidationDetails(v => v.ValidateChildren = false);
+            var modelMetadata = modelMetadataProvider.GetMetadataForType(typeof(IJsonPatchDocument));
+
+            var parameterBinder = new ParameterBinder(
+                modelMetadataProvider,
+                Mock.Of<IModelBinderFactory>(),
+                new DefaultObjectValidator(
+                    modelMetadataProvider,
+                    new[] { TestModelValidatorProvider.CreateDefaultProvider() }),
+                _optionsAccessor,
+                NullLoggerFactory.Instance);
+
+            // BodyModelBinder does not update ModelState in success case.
+            var modelBindingResult = ModelBindingResult.Success(new JsonPatchDocument());
+            var modelBinder = CreateMockModelBinder(modelBindingResult);
+
+            // Act
+            var result = await parameterBinder.BindModelAsync(
+                actionContext,
+                modelBinder,
+                new SimpleValueProvider(),
+                parameterDescriptor,
+                modelMetadata,
+                value: null);
+
+            // Assert
+            Assert.True(result.IsModelSet);
+            Assert.False(modelState.IsValid);
+            Assert.Collection(
+                modelState,
+                kvp =>
+                {
+                    Assert.Equal("id", kvp.Key);
+                    Assert.Equal(ModelValidationState.Invalid, kvp.Value.ValidationState);
+                    var error = Assert.Single(kvp.Value.Errors);
+                    Assert.Equal("This is not valid.", error.ErrorMessage);
+                });
+        }
+
+        // Regression test 2 for aspnet/Mvc#7963. ModelState should never be valid.
+        [Fact]
+        public async Task BindModelAsync_ForOverlappingParametersWithSuppressions_InValid_WithInValidSecondParameter()
+        {
+            // Arrange
+            var parameterDescriptor = new ParameterDescriptor
+            {
+                Name = "patchDocument",
+                ParameterType = typeof(IJsonPatchDocument),
+            };
+
+            var actionContext = GetControllerContext();
+            var modelState = actionContext.ModelState;
+
+            // First ModelState key is not empty to match SimpleTypeModelBinder.
+            modelState.SetModelValue("id", "notAGuid", "notAGuid");
+            modelState.AddModelError("id", "This is not valid.");
+
+            // Second ModelState key is empty to match BodyModelBinder.
+            modelState.AddModelError(string.Empty, "This is also not valid.");
+
+            var modelMetadataProvider = new TestModelMetadataProvider();
+            modelMetadataProvider.ForType<IJsonPatchDocument>().ValidationDetails(v => v.ValidateChildren = false);
+            var modelMetadata = modelMetadataProvider.GetMetadataForType(typeof(IJsonPatchDocument));
+
+            var parameterBinder = new ParameterBinder(
+                modelMetadataProvider,
+                Mock.Of<IModelBinderFactory>(),
+                new DefaultObjectValidator(
+                    modelMetadataProvider,
+                    new[] { TestModelValidatorProvider.CreateDefaultProvider() }),
+                _optionsAccessor,
+                NullLoggerFactory.Instance);
+
+            var modelBindingResult = ModelBindingResult.Failed();
+            var modelBinder = CreateMockModelBinder(modelBindingResult);
+
+            // Act
+            var result = await parameterBinder.BindModelAsync(
+                actionContext,
+                modelBinder,
+                new SimpleValueProvider(),
+                parameterDescriptor,
+                modelMetadata,
+                value: null);
+
+            // Assert
+            Assert.False(result.IsModelSet);
+            Assert.False(modelState.IsValid);
+            Assert.Collection(
+                modelState,
+                kvp =>
+                {
+                    Assert.Empty(kvp.Key);
+                    Assert.Equal(ModelValidationState.Invalid, kvp.Value.ValidationState);
+                    var error = Assert.Single(kvp.Value.Errors);
+                    Assert.Equal("This is also not valid.", error.ErrorMessage);
+                },
+                kvp =>
+                {
+                    Assert.Equal("id", kvp.Key);
+                    Assert.Equal(ModelValidationState.Invalid, kvp.Value.ValidationState);
+                    var error = Assert.Single(kvp.Value.Errors);
+                    Assert.Equal("This is not valid.", error.ErrorMessage);
+                });
+        }
+
         private static ControllerContext GetControllerContext()
         {
             var services = new ServiceCollection();
@@ -813,25 +939,6 @@ private static IValueProvider CreateMockValueProvider()
             return mockValueProvider.Object;
         }
 
-        private static IModelValidatorProvider CreateMockValidatorProvider(IModelValidator validator = null)
-        {
-            var mockValidator = new Mock<IModelValidatorProvider>();
-            mockValidator
-                .Setup(o => o.CreateValidators(
-                    It.IsAny<ModelValidatorProviderContext>()))
-                .Callback<ModelValidatorProviderContext>(context =>
-                {
-                    if (validator != null)
-                    {
-                        foreach (var result in context.Results)
-                        {
-                            result.Validator = validator;
-                        }
-                    }
-                });
-            return mockValidator.Object;
-        }
-
         private class Person : IEquatable<Person>, IEquatable<object>
         {
             public string Name { get; set; }
@@ -862,9 +969,6 @@ private class DerivedPerson : Person
             public string DerivedProperty { get; set; }
         }
 
-        [Required]
-        private Person PersonProperty { get; set; }
-
         public abstract class FakeModelMetadata : ModelMetadata
         {
             public FakeModelMetadata()