Improve handling of custom `ValidationAttribute`s and their `ValidationResult`s

- #3595 sub-items 2 through 4
- handle an indexer name in `ValidationResult.MemberNames`
 - aligns `ModelNames.CreatePropertyModelName()` with `TemplateInfo.GetFullHtmlFieldName()`
- handle multiple elements in `ValidationResult.MemberNames`
 - later elements previously ignored
- set `ValidationContext.MemberName` to `null` when no property name is available
 - using type name for a member name was just wrong

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;
         }
     }
 }

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
+                string errorMessage;
-                // 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;
                 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);
+                var validationResults = new List<ModelValidationResult>();
-                return new ModelValidationResult[] { validationResult };
+                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<ModelValidationResult>();

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<string> Items { get; set; } = new List<string> { "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; }
+        }
     }
 }

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<object[]> Validate_SetsMemberName_OnValidationContext_ForProperties_Data
+        public static TheoryData Validate_SetsMemberName_AsExpectedData
         {
             get
             {
-                yield return new object[]
+                var array = new[] { new SampleModel { Name = "one" }, new SampleModel { Name = "two" } };
-                {
-                    _metadataProvider.GetMetadataForType(typeof(string)).Properties["Length"],
-                    "Hello",
-                    "Hello".Length,
-                    "Length",
-                };
 
-                yield return new object[]
+                // metadata, container, model, expected MemberName
+                return new TheoryData<ModelMetadata, object, object, string>
                 {
-                    _metadataProvider.GetMetadataForType(typeof(SampleModel)),
+                    {
-                    null,
+                        _metadataProvider.GetMetadataForProperty(typeof(string), nameof(string.Length)),
-                    15,
+                        "Hello",
-                    "SampleModel",
+                        "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))]
+        [MemberData(nameof(Validate_SetsMemberName_AsExpectedData))]
-        public void Validate_SetsMemberName_OnValidationContext_ForProperties(
+        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 static TheoryData<string, IEnumerable<string>, IEnumerable<ModelValidationResult>>
-        public void Validate_ReturnsSingleValidationResult_IfMemberNameSequenceIsEmpty()
+            Valdate_ReturnsExpectedResults_Data
+        {
+            get
+            {
+                var errorMessage = "Some error message";
+                return new TheoryData<string, IEnumerable<string>, IEnumerable<ModelValidationResult>>
+                {
+                    {
+                        errorMessage,
+                        null,
+                        new[] { new ModelValidationResult(memberName: string.Empty, message: errorMessage) } },
+                    {
+                        errorMessage,
+                        Enumerable.Empty<string>(),
+                        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<string> memberNames,
+            IEnumerable<ModelValidationResult> expectedResults)
         {
             // Arrange
-            const string errorMessage = "Some error message";
+            var metadata = _metadataProvider.GetMetadataForProperty(typeof(string), nameof(string.Length));
-
-            var metadata = _metadataProvider.GetMetadataForType(typeof(string));
             var container = "Hello";
             var model = container.Length;
 
             var attribute = new Mock<TestableValidationAttribute> { CallBase = true };
             attribute
                  .Setup(p => p.IsValidPublic(It.IsAny<object>(), It.IsAny<ValidationContext>()))
-                 .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(expectedResults, results, ModelValidationResultComparer.Instance);
-            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<TestableValidationAttribute> { CallBase = true };
-            attribute
-                 .Setup(p => p.IsValidPublic(It.IsAny<object>(), It.IsAny<ValidationContext>()))
-                 .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<TestableValidationAttribute> { CallBase = true };
-            attribute
-                 .Setup(p => p.IsValidPublic(It.IsAny<object>(), It.IsAny<ValidationContext>()))
-                 .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);
         }
 
         [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<ModelValidationResult>
+        {
+            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;
+            }
+        }
     }
 }