diff --git a/src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/ModelNames.cs b/src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/ModelNames.cs index 248cb50612..185c8345e0 100644 --- a/src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/ModelNames.cs +++ b/src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/ModelNames.cs @@ -1,6 +1,7 @@ // Copyright (c) .NET Foundation. All rights reserved. // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. +using System; using System.Globalization; namespace Microsoft.AspNetCore.Mvc.ModelBinding @@ -23,14 +24,20 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding { return propertyName ?? string.Empty; } - else if (string.IsNullOrEmpty(propertyName)) + + if (string.IsNullOrEmpty(propertyName)) { return prefix ?? string.Empty; } - else + + if (propertyName.StartsWith("[", StringComparison.Ordinal)) { - return prefix + "." + propertyName; + // The propertyName might represent an indexer access, in which case combining + // with a 'dot' would be invalid. This case occurs only when called from ValidationVisitor. + return prefix + propertyName; } + + return prefix + "." + propertyName; } } } diff --git a/src/Microsoft.AspNetCore.Mvc.DataAnnotations/Internal/DataAnnotationsModelValidator.cs b/src/Microsoft.AspNetCore.Mvc.DataAnnotations/Internal/DataAnnotationsModelValidator.cs index 31d9f6f904..ad992a0d4f 100644 --- a/src/Microsoft.AspNetCore.Mvc.DataAnnotations/Internal/DataAnnotationsModelValidator.cs +++ b/src/Microsoft.AspNetCore.Mvc.DataAnnotations/Internal/DataAnnotationsModelValidator.cs @@ -79,7 +79,7 @@ namespace Microsoft.AspNetCore.Mvc.DataAnnotations.Internal } var metadata = validationContext.ModelMetadata; - var memberName = metadata.PropertyName ?? metadata.ModelType.Name; + var memberName = metadata.PropertyName; var container = validationContext.Container; var context = new ValidationContext( @@ -94,31 +94,47 @@ namespace Microsoft.AspNetCore.Mvc.DataAnnotations.Internal var result = Attribute.GetValidationResult(validationContext.Model, context); if (result != ValidationResult.Success) { - // ModelValidationResult.MemberName is used by invoking validators (such as ModelValidator) to - // construct the ModelKey for ModelStateDictionary. When validating at type level we want to append - // the returned MemberNames if specified (e.g. person.Address.FirstName). For property validation, the - // ModelKey can be constructed using the ModelMetadata and we should ignore MemberName (we don't want - // (person.Name.Name). However the invoking validator does not have a way to distinguish between these - // two cases. Consequently we'll only set MemberName if this validation returns a MemberName that is - // different from the property being validated. - - var errorMemberName = result.MemberNames.FirstOrDefault(); - if (string.Equals(errorMemberName, memberName, StringComparison.Ordinal)) - { - errorMemberName = null; - } - - string errorMessage = null; + string errorMessage; if (_stringLocalizer != null && !string.IsNullOrEmpty(Attribute.ErrorMessage) && string.IsNullOrEmpty(Attribute.ErrorMessageResourceName) && Attribute.ErrorMessageResourceType == null) { - errorMessage = GetErrorMessage(validationContext); + errorMessage = GetErrorMessage(validationContext) ?? result.ErrorMessage; + } + else + { + errorMessage = result.ErrorMessage; } - var validationResult = new ModelValidationResult(errorMemberName, errorMessage ?? result.ErrorMessage); - return new ModelValidationResult[] { validationResult }; + var validationResults = new List(); + if (result.MemberNames != null) + { + foreach (var resultMemberName in result.MemberNames) + { + // ModelValidationResult.MemberName is used by invoking validators (such as ModelValidator) to + // append construct the ModelKey for ModelStateDictionary. When validating at type level we + // want the returned MemberNames if specified (e.g. "person.Address.FirstName"). For property + // validation, the ModelKey can be constructed using the ModelMetadata and we should ignore + // MemberName (we don't want "person.Name.Name"). However the invoking validator does not have + // a way to distinguish between these two cases. Consequently we'll only set MemberName if this + // validation returns a MemberName that is different from the property being validated. + var newMemberName = string.Equals(resultMemberName, memberName, StringComparison.Ordinal) ? + null : + resultMemberName; + var validationResult = new ModelValidationResult(newMemberName, errorMessage); + + validationResults.Add(validationResult); + } + } + + if (validationResults.Count == 0) + { + // result.MemberNames was null or empty. + validationResults.Add(new ModelValidationResult(memberName: null, message: errorMessage)); + } + + return validationResults; } return Enumerable.Empty(); diff --git a/test/Microsoft.AspNetCore.Mvc.Core.Test/Internal/DefaultObjectValidatorTests.cs b/test/Microsoft.AspNetCore.Mvc.Core.Test/Internal/DefaultObjectValidatorTests.cs index 99314425a6..17dda63446 100644 --- a/test/Microsoft.AspNetCore.Mvc.Core.Test/Internal/DefaultObjectValidatorTests.cs +++ b/test/Microsoft.AspNetCore.Mvc.Core.Test/Internal/DefaultObjectValidatorTests.cs @@ -374,6 +374,74 @@ namespace Microsoft.AspNetCore.Mvc.Internal Assert.Equal(ValidationAttributeUtil.GetRequiredErrorMessage("Profession"), error.ErrorMessage); } + [Fact] + public void Validate_ComplexType_MultipleInvalidProperties() + { + // Arrange + var actionContext = new ActionContext(); + var modelState = actionContext.ModelState; + var model = new InvalidProperties(); + var validationState = new ValidationStateDictionary + { + { model, new ValidationStateEntry() }, + }; + + var validator = CreateValidator(); + + // Act + validator.Validate(actionContext, validationState, prefix: null, model: model); + + // Assert + Assert.Collection( + modelState, + state => + { + Assert.Equal("FirstName", state.Key); + var error = Assert.Single(state.Value.Errors); + Assert.Equal("User object lacks some data.", error.ErrorMessage); + }, + state => + { + Assert.Equal("Address.City", state.Key); + var error = Assert.Single(state.Value.Errors); + Assert.Equal("User object lacks some data.", error.ErrorMessage); + }); + } + + [Fact] + public void Validate_ComplexType_MultipleInvalidProperties_WithPrefix() + { + // Arrange + var actionContext = new ActionContext(); + var modelState = actionContext.ModelState; + var model = new InvalidProperties(); + var validationState = new ValidationStateDictionary + { + { model, new ValidationStateEntry { Key = "invalidProperties" } }, + }; + + var validator = CreateValidator(); + + // Act + validator.Validate(actionContext, validationState, prefix: "invalidProperties", model: model); + + // Assert + Assert.Collection( + modelState, + state => + { + Assert.Equal("invalidProperties.FirstName", state.Key); + var error = Assert.Single(state.Value.Errors); + Assert.Equal("User object lacks some data.", error.ErrorMessage); + }, + state => + { + Assert.Equal("invalidProperties.Address.City", state.Key); + var error = Assert.Single(state.Value.Errors); + Assert.Equal("User object lacks some data.", error.ErrorMessage); + }); + } + // IValidatableObject is significant because the validators are on the object // itself, not just the properties. [Fact] @@ -435,7 +503,7 @@ namespace Microsoft.AspNetCore.Mvc.Internal httpContext.SetupGet(x => x.RequestServices).Returns(provider); var actionContext = new ActionContext { HttpContext = httpContext.Object }; - + var modelState = actionContext.ModelState; var validationState = new ValidationStateDictionary(); @@ -577,7 +645,7 @@ namespace Microsoft.AspNetCore.Mvc.Internal validationState.Add(model, new ValidationStateEntry() { Key = "parameter" }); // Act - validator.Validate(actionContext, validationState, "parameter", model); + validator.Validate(actionContext, validationState, "parameter", model); // Assert Assert.True(modelState.IsValid); @@ -727,7 +795,7 @@ namespace Microsoft.AspNetCore.Mvc.Internal validationState.Add(model, new ValidationStateEntry() { Key = "items", - + // Force the validator to treat it as the specified type. Metadata = MetadataProvider.GetMetadataForType(type), }); @@ -752,6 +820,40 @@ namespace Microsoft.AspNetCore.Mvc.Internal Assert.Empty(entry.Errors); } + [Fact] + public void Validate_CollectionType_MultipleInvalidItems() + { + // Arrange + var actionContext = new ActionContext(); + var modelState = actionContext.ModelState; + var model = new InvalidItemsContainer(); + var validationState = new ValidationStateDictionary + { + { model, new ValidationStateEntry() }, + }; + + var validator = CreateValidator(); + + // Act + validator.Validate(actionContext, validationState, prefix: null, model: model); + + // Assert + Assert.Collection( + modelState, + state => + { + Assert.Equal("Items[0]", state.Key); + var error = Assert.Single(state.Value.Errors); + Assert.Equal("Collection contains duplicate value 'Joe'.", error.ErrorMessage); + }, + state => + { + Assert.Equal("Items[2]", state.Key); + var error = Assert.Single(state.Value.Errors); + Assert.Equal("Collection contains duplicate value 'Joe'.", error.ErrorMessage); + }); + } + [Fact] public void Validate_CollectionType_DictionaryOfSimpleType_Invalid() { @@ -823,7 +925,7 @@ namespace Microsoft.AspNetCore.Mvc.Internal AssertKeysEqual( modelState, "[0].Key", - "[0].Value.Name", + "[0].Value.Name", "[0].Value.Profession", "[1].Key", "[1].Value.Name", @@ -1088,5 +1190,50 @@ namespace Microsoft.AspNetCore.Mvc.Internal { void DoSomething(); } + + // Custom validation attribute that returns multiple entries in ValidationResult.MemberNames and those member + // names are indexers. An example scenario is an attribute that confirms all entries in a list are unique. + private class InvalidItemsAttribute : ValidationAttribute + { + protected override ValidationResult IsValid(object value, ValidationContext validationContext) + { + return new ValidationResult( + "Collection contains duplicate value 'Joe'.", + new[] { "[0]", "[2]" }); + } + } + + private class InvalidItemsContainer + { + [InvalidItems] + public List Items { get; set; } = new List { "Joe", "Fred", "Joe", "Herman" }; + } + + // Custom validation attribute that returns multiple entries in ValidationResult.MemberNames. An example + // scenario is an attribute that confirms all properties in a complex type are non-empty. + private class InvalidPropertiesAttribute : ValidationAttribute + { + protected override ValidationResult IsValid(object value, ValidationContext validationContext) + { + return new ValidationResult( + "User object lacks some data.", + new[] { "FirstName", "Address.City" }); + } + } + + [InvalidProperties] + private class InvalidProperties + { + public string FirstName { get; set; } + + public string LastName { get; set; } = "IsSet"; + + public InvalidAddress Address { get; set; } + } + + private class InvalidAddress + { + public string City { get; set; } + } } } diff --git a/test/Microsoft.AspNetCore.Mvc.DataAnnotations.Test/Internal/DataAnnotationsModelValidatorTest.cs b/test/Microsoft.AspNetCore.Mvc.DataAnnotations.Test/Internal/DataAnnotationsModelValidatorTest.cs index c3385ac4e1..27e8ab60fb 100644 --- a/test/Microsoft.AspNetCore.Mvc.DataAnnotations.Test/Internal/DataAnnotationsModelValidatorTest.cs +++ b/test/Microsoft.AspNetCore.Mvc.DataAnnotations.Test/Internal/DataAnnotationsModelValidatorTest.cs @@ -1,13 +1,14 @@ // Copyright (c) .NET Foundation. All rights reserved. // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. +using System; using System.Collections.Generic; using System.ComponentModel.DataAnnotations; using System.Linq; using Microsoft.AspNetCore.Http; -using Microsoft.AspNetCore.Mvc.DataAnnotations; using Microsoft.AspNetCore.Mvc.ModelBinding; using Microsoft.AspNetCore.Mvc.ModelBinding.Validation; +using Microsoft.DotNet.InternalAbstractions; using Microsoft.Extensions.DependencyInjection; using Microsoft.Extensions.Localization; using Moq; @@ -35,31 +36,42 @@ namespace Microsoft.AspNetCore.Mvc.DataAnnotations.Internal Assert.Same(attribute, validator.Attribute); } - public static IEnumerable Validate_SetsMemberName_OnValidationContext_ForProperties_Data + public static TheoryData Validate_SetsMemberName_AsExpectedData { get { - yield return new object[] - { - _metadataProvider.GetMetadataForType(typeof(string)).Properties["Length"], - "Hello", - "Hello".Length, - "Length", - }; + var array = new[] { new SampleModel { Name = "one" }, new SampleModel { Name = "two" } }; - yield return new object[] + // metadata, container, model, expected MemberName + return new TheoryData { - _metadataProvider.GetMetadataForType(typeof(SampleModel)), - null, - 15, - "SampleModel", + { + _metadataProvider.GetMetadataForProperty(typeof(string), nameof(string.Length)), + "Hello", + "Hello".Length, + nameof(string.Length) + }, + { + // Validating a top-level model + _metadataProvider.GetMetadataForType(typeof(SampleModel)), + null, + 15, + null + }, + { + // Validating an element in a collection. + _metadataProvider.GetMetadataForType(typeof(SampleModel)), + array, + array[1], + null + }, }; } } [Theory] - [MemberData(nameof(Validate_SetsMemberName_OnValidationContext_ForProperties_Data))] - public void Validate_SetsMemberName_OnValidationContext_ForProperties( + [MemberData(nameof(Validate_SetsMemberName_AsExpectedData))] + public void Validate_SetsMemberName_AsExpected( ModelMetadata metadata, object container, object model, @@ -150,7 +162,7 @@ namespace Microsoft.AspNetCore.Mvc.DataAnnotations.Internal // Assert var validationResult = result.Single(); - Assert.Equal("", validationResult.MemberName); + Assert.Empty(validationResult.MemberName); Assert.Equal(attribute.Object.FormatErrorMessage("Length"), validationResult.Message); } @@ -184,25 +196,92 @@ namespace Microsoft.AspNetCore.Mvc.DataAnnotations.Internal Assert.Empty(result); } - [Fact] - public void Validate_ReturnsSingleValidationResult_IfMemberNameSequenceIsEmpty() + public static TheoryData, IEnumerable> + Valdate_ReturnsExpectedResults_Data + { + get + { + var errorMessage = "Some error message"; + return new TheoryData, IEnumerable> + { + { + errorMessage, + null, + new[] { new ModelValidationResult(memberName: string.Empty, message: errorMessage) } }, + { + errorMessage, + Enumerable.Empty(), + new[] { new ModelValidationResult(memberName: string.Empty, message: errorMessage) } + }, + { + errorMessage, + new[] { (string)null }, + new[] { new ModelValidationResult(memberName: string.Empty, message: errorMessage) } + }, + { + errorMessage, + new[] { string.Empty }, + new[] { new ModelValidationResult(memberName: string.Empty, message: errorMessage) } + }, + { + errorMessage, + // Name matches ValidationContext.MemberName. + new[] { nameof(string.Length) }, + new[] { new ModelValidationResult(memberName: string.Empty, message: errorMessage) } + }, + { + errorMessage, + new[] { "AnotherName" }, + new[] { new ModelValidationResult(memberName: "AnotherName", message: errorMessage) } + }, + { + errorMessage, + new[] { "[1]" }, + new[] { new ModelValidationResult(memberName: "[1]", message: errorMessage) } + }, + { + errorMessage, + new[] { "Name1", "Name2" }, + new[] + { + new ModelValidationResult(memberName: "Name1", message: errorMessage), + new ModelValidationResult(memberName: "Name2", message: errorMessage), + } + }, + { + errorMessage, + new[] { "[0]", "[2]" }, + new[] + { + new ModelValidationResult(memberName: "[0]", message: errorMessage), + new ModelValidationResult(memberName: "[2]", message: errorMessage), + } + }, + }; + } + } + + [Theory] + [MemberData(nameof(Valdate_ReturnsExpectedResults_Data))] + public void Valdate_ReturnsExpectedResults( + string errorMessage, + IEnumerable memberNames, + IEnumerable expectedResults) { // Arrange - const string errorMessage = "Some error message"; - - var metadata = _metadataProvider.GetMetadataForType(typeof(string)); + var metadata = _metadataProvider.GetMetadataForProperty(typeof(string), nameof(string.Length)); var container = "Hello"; var model = container.Length; var attribute = new Mock { CallBase = true }; attribute .Setup(p => p.IsValidPublic(It.IsAny(), It.IsAny())) - .Returns(new ValidationResult(errorMessage, memberNames: null)); + .Returns(new ValidationResult(errorMessage, memberNames)); + var validator = new DataAnnotationsModelValidator( new ValidationAttributeAdapterProvider(), attribute.Object, stringLocalizer: null); - var validationContext = new ModelValidationContext( actionContext: new ActionContext(), modelMetadata: metadata, @@ -214,74 +293,7 @@ namespace Microsoft.AspNetCore.Mvc.DataAnnotations.Internal var results = validator.Validate(validationContext); // Assert - var validationResult = Assert.Single(results); - Assert.Equal(errorMessage, validationResult.Message); - Assert.Empty(validationResult.MemberName); - } - - [Fact] - public void Validate_ReturnsSingleValidationResult_IfOneMemberNameIsSpecified() - { - // Arrange - const string errorMessage = "A different error message"; - - var metadata = _metadataProvider.GetMetadataForType(typeof(object)); - var model = new object(); - - var attribute = new Mock { CallBase = true }; - attribute - .Setup(p => p.IsValidPublic(It.IsAny(), It.IsAny())) - .Returns(new ValidationResult(errorMessage, new[] { "FirstName" })); - - var validator = new DataAnnotationsModelValidator( - new ValidationAttributeAdapterProvider(), - attribute.Object, - stringLocalizer: null); - var validationContext = new ModelValidationContext( - actionContext: new ActionContext(), - modelMetadata: metadata, - metadataProvider: _metadataProvider, - container: null, - model: model); - - // Act - var results = validator.Validate(validationContext); - - // Assert - ModelValidationResult validationResult = Assert.Single(results); - Assert.Equal(errorMessage, validationResult.Message); - Assert.Equal("FirstName", validationResult.MemberName); - } - - [Fact] - public void Validate_ReturnsMemberName_IfItIsDifferentFromDisplayName() - { - // Arrange - var metadata = _metadataProvider.GetMetadataForType(typeof(SampleModel)); - var model = new SampleModel(); - - var attribute = new Mock { CallBase = true }; - attribute - .Setup(p => p.IsValidPublic(It.IsAny(), It.IsAny())) - .Returns(new ValidationResult("Name error", new[] { "Name" })); - - var validator = new DataAnnotationsModelValidator( - new ValidationAttributeAdapterProvider(), - attribute.Object, - stringLocalizer: null); - var validationContext = new ModelValidationContext( - actionContext: new ActionContext(), - modelMetadata: metadata, - metadataProvider: _metadataProvider, - container: null, - model: model); - - // Act - var results = validator.Validate(validationContext); - - // Assert - ModelValidationResult validationResult = Assert.Single(results); - Assert.Equal("Name", validationResult.MemberName); + Assert.Equal(expectedResults, results, ModelValidationResultComparer.Instance); } [Fact] @@ -314,7 +326,7 @@ namespace Microsoft.AspNetCore.Mvc.DataAnnotations.Internal // Assert var validationResult = result.Single(); - Assert.Equal("", validationResult.MemberName); + Assert.Empty(validationResult.MemberName); Assert.Equal("Longueur est invalide : 4", validationResult.Message); } @@ -404,7 +416,7 @@ namespace Microsoft.AspNetCore.Mvc.DataAnnotations.Internal }, { new StringLengthAttribute(length) { ErrorMessage = LocalizationKey, MinimumLength = 1}, - "", + string.Empty, new object[] { nameof(SampleModel), 1, length } }, { @@ -482,5 +494,39 @@ namespace Microsoft.AspNetCore.Mvc.DataAnnotations.Internal { void DoSomething(); } + + private class ModelValidationResultComparer : IEqualityComparer + { + public static readonly ModelValidationResultComparer Instance = new ModelValidationResultComparer(); + + private ModelValidationResultComparer() + { + } + + public bool Equals(ModelValidationResult x, ModelValidationResult y) + { + if (x == null || y == null) + { + return x == null && y == null; + } + + return string.Equals(x.MemberName, y.MemberName, StringComparison.Ordinal) && + string.Equals(x.Message, y.Message, StringComparison.Ordinal); + } + + public int GetHashCode(ModelValidationResult obj) + { + if (obj == null) + { + throw new ArgumentNullException(nameof(obj)); + } + + var hashCodeCombiner = HashCodeCombiner.Start(); + hashCodeCombiner.Add(obj.MemberName, StringComparer.Ordinal); + hashCodeCombiner.Add(obj.Message, StringComparer.Ordinal); + + return hashCodeCombiner.CombinedHash; + } + } } }