diff --git a/src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/Validation/ValidationVisitor.cs b/src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/Validation/ValidationVisitor.cs index f8c4c02dd7..ecf44973cc 100644 --- a/src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/Validation/ValidationVisitor.cs +++ b/src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/Validation/ValidationVisitor.cs @@ -296,7 +296,10 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding.Validation 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 index 5371633029..a9ee38cfc6 100644 --- a/test/Microsoft.AspNetCore.Mvc.Core.Test/Internal/DefaultObjectValidatorTests.cs +++ b/test/Microsoft.AspNetCore.Mvc.Core.Test/Internal/DefaultObjectValidatorTests.cs @@ -133,6 +133,27 @@ namespace Microsoft.AspNetCore.Mvc.Internal 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 @@ namespace Microsoft.AspNetCore.Mvc.Internal 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(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(); diff --git a/test/Microsoft.AspNetCore.Mvc.Core.Test/ModelBinding/ParameterBinderTest.cs b/test/Microsoft.AspNetCore.Mvc.Core.Test/ModelBinding/ParameterBinderTest.cs index 7a605c4640..8c0ea54528 100644 --- 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.Linq; 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 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding }); } + // 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().ValidationDetails(v => v.ValidateChildren = false); + var modelMetadata = modelMetadataProvider.GetMetadataForType(typeof(IJsonPatchDocument)); + + var parameterBinder = new ParameterBinder( + modelMetadataProvider, + Mock.Of(), + 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().ValidationDetails(v => v.ValidateChildren = false); + var modelMetadata = modelMetadataProvider.GetMetadataForType(typeof(IJsonPatchDocument)); + + var parameterBinder = new ParameterBinder( + modelMetadataProvider, + Mock.Of(), + 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 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding return mockValueProvider.Object; } - private static IModelValidatorProvider CreateMockValidatorProvider(IModelValidator validator = null) - { - var mockValidator = new Mock(); - mockValidator - .Setup(o => o.CreateValidators( - It.IsAny())) - .Callback(context => - { - if (validator != null) - { - foreach (var result in context.Results) - { - result.Validator = validator; - } - } - }); - return mockValidator.Object; - } - private class Person : IEquatable, IEquatable { public string Name { get; set; } @@ -862,9 +969,6 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding public string DerivedProperty { get; set; } } - [Required] - private Person PersonProperty { get; set; } - public abstract class FakeModelMetadata : ModelMetadata { public FakeModelMetadata()