Bug fixes to ObservableValidator (#3764)

## Fixes #3763
<!-- Add the relevant issue number after the "#" mentioned above (for ex: Fixes #1234) which will automatically close the issue once the PR is merged. -->

<!-- Add a brief overview here of the feature/bug & fix. -->

## PR Type
What kind of change does this PR introduce?
<!-- Please uncomment one or more that apply to this PR. -->

- Bugfix
<!-- - Feature -->
<!-- - Code style update (formatting) -->
<!-- - Refactoring (no functional changes, no api changes) -->
<!-- - Build or CI related changes -->
<!-- - Documentation content changes -->
<!-- - Sample app changes -->
<!-- - Other... Please describe: -->


## What is the current behavior?
<!-- Please describe the current behavior that you are modifying, or link to a relevant issue. -->
- Incorrect loading of display names for validated properties
- Repeated error messages after validation

## What is the new behavior?
<!-- Describe how was this issue resolved or changed? -->
Fixed the errors above.

Opening as a draft to have the CI run, but I'm still investigating the second issue from the list above.

## PR Checklist

Please check if your PR fulfills the following requirements:

- [X] Tested code with current [supported SDKs](../readme.md#supported)
- [ ] ~~Pull Request has been submitted to the documentation repository [instructions](..\contributing.md#docs). Link: <!-- docs PR link -->~~
- [ ] ~~Sample in sample app has been added / updated (for bug fixes / features)~~
    - [ ] ~~Icon has been created (if new sample) following the [Thumbnail Style Guide and templates](https://github.com/windows-toolkit/WindowsCommunityToolkit-design-assets)~~
- [X] New major technical changes in the toolkit have or will be added to the [Wiki](https://github.com/windows-toolkit/WindowsCommunityToolkit/wiki) e.g. build changes, source generators, testing infrastructure, sample creation changes, etc...
- [X] Tests for the changes have been added (for bug fixes / features) (if applicable)
- [X] Header has been added to all new source files (run *build/UpdateHeaders.bat*)
- [X] Contains **NO** breaking changes

Author: msftbot[bot]

Microsoft.Toolkit.Mvvm/ComponentModel/ObservableValidator.cs

@@ -26,6 +26,18 @@ public abstract class ObservableValidator : ObservableObject, INotifyDataErrorIn
         /// </summary>
         private static readonly ConditionalWeakTable<Type, Action<object>> EntityValidatorMap = new();
 
+        /// <summary>
+        /// The <see cref="ConditionalWeakTable{TKey, TValue}"/> instance used to track display names for properties to validate.
+        /// </summary>
+        /// <remarks>
+        /// This is necessary because we want to reuse the same <see cref="ValidationContext"/> instance for all validations, but
+        /// with the same behavior with repsect to formatted names that new instances would have provided. The issue is that the
+        /// <see cref="ValidationContext.DisplayName"/> property is not refreshed when we set <see cref="ValidationContext.MemberName"/>,
+        /// so we need to replicate the same logic to retrieve the right display name for properties to validate and update that
+        /// property manually right before passing the context to <see cref="Validator"/> and proceed with the normal functionality.
+        /// </remarks>
+        private static readonly ConditionalWeakTable<Type, Dictionary<string, string>> DisplayNamesMap = new();
+
         /// <summary>
         /// The cached <see cref="PropertyChangedEventArgs"/> for <see cref="HasErrors"/>.
         /// </summary>
@@ -68,7 +80,7 @@ protected ObservableValidator()
         /// be used to validate all properties, which will reference the current instance.
         /// </summary>
         /// <param name="items">A set of key/value pairs to make available to consumers.</param>
-        protected ObservableValidator(IDictionary<object, object?> items)
+        protected ObservableValidator(IDictionary<object, object?>? items)
         {
             this.validationContext = new ValidationContext(this, items);
         }
@@ -80,7 +92,7 @@ protected ObservableValidator(IDictionary<object, object?> items)
         /// </summary>
         /// <param name="serviceProvider">An <see cref="IServiceProvider"/> instance to make available during validation.</param>
         /// <param name="items">A set of key/value pairs to make available to consumers.</param>
-        protected ObservableValidator(IServiceProvider serviceProvider, IDictionary<object, object?> items)
+        protected ObservableValidator(IServiceProvider? serviceProvider, IDictionary<object, object?>? items)
         {
             this.validationContext = new ValidationContext(this, serviceProvider, items);
         }
@@ -541,6 +553,7 @@ protected void ValidateProperty(object? value, [CallerMemberName] string? proper
 
             // Validate the property, by adding new errors to the existing list
             this.validationContext.MemberName = propertyName;
+            this.validationContext.DisplayName = GetDisplayNameForProperty(propertyName!);
 
             bool isValid = Validator.TryValidateProperty(value, this.validationContext, propertyErrors);
 
@@ -611,6 +624,7 @@ private bool TryValidateProperty(object? value, string? propertyName, out IReadO
 
             // Validate the property, by adding new errors to the local list
             this.validationContext.MemberName = propertyName;
+            this.validationContext.DisplayName = GetDisplayNameForProperty(propertyName!);
 
             bool isValid = Validator.TryValidateProperty(value, this.validationContext, localErrors);
 
@@ -688,6 +702,36 @@ private void ClearErrorsForProperty(string propertyName)
             ErrorsChanged?.Invoke(this, new DataErrorsChangedEventArgs(propertyName));
         }
 
+        /// <summary>
+        /// Gets the display name for a given property. It could be a custom name or just the property name.
+        /// </summary>
+        /// <param name="propertyName">The target property name being validated.</param>
+        /// <returns>The display name for the property.</returns>
+        private string GetDisplayNameForProperty(string propertyName)
+        {
+            static Dictionary<string, string> GetDisplayNames(Type type)
+            {
+                Dictionary<string, string> displayNames = new();
+
+                foreach (PropertyInfo property in type.GetProperties(BindingFlags.Instance | BindingFlags.Public))
+                {
+                    if (property.GetCustomAttribute<DisplayAttribute>() is DisplayAttribute attribute &&
+                        attribute.GetName() is string displayName)
+                    {
+                        displayNames.Add(property.Name, displayName);
+                    }
+                }
+
+                return displayNames;
+            }
+
+            // This method replicates the logic of DisplayName and GetDisplayName from the
+            // ValidationContext class. See the original source in the BCL for more details.
+            DisplayNamesMap.GetValue(GetType(), static t => GetDisplayNames(t)).TryGetValue(propertyName, out string? displayName);
+
+            return displayName ?? propertyName;
+        }
+
 #pragma warning disable SA1204
         /// <summary>
         /// Throws an <see cref="ArgumentNullException"/> when a property name given as input is <see langword="null"/>.

UnitTests/UnitTests.Shared/Mvvm/Test_ObservableValidator.cs

@@ -392,6 +392,28 @@ public void Test_ObservableValidator_CustomValidationWithInjectedService()
             Assert.AreEqual(model.GetErrors(nameof(ValidationWithServiceModel.Name)).ToArray().Length, 2);
         }
 
+        [TestCategory("Mvvm")]
+        [TestMethod]
+        public void Test_ObservableValidator_ValidationWithFormattedDisplayName()
+        {
+            var model = new ValidationWithDisplayName();
+
+            Assert.IsTrue(model.HasErrors);
+
+            // We need to order because there is no guaranteed order on the members of a type
+            ValidationResult[] allErrors = model.GetErrors().OrderBy(error => error.ErrorMessage).ToArray();
+
+            Assert.AreEqual(allErrors.Length, 2);
+
+            Assert.AreEqual(allErrors[0].MemberNames.Count(), 1);
+            Assert.AreEqual(allErrors[0].MemberNames.Single(), nameof(ValidationWithDisplayName.StringMayNotBeEmpty));
+            Assert.AreEqual(allErrors[0].ErrorMessage, $"FIRST: {nameof(ValidationWithDisplayName.StringMayNotBeEmpty)}.");
+
+            Assert.AreEqual(allErrors[1].MemberNames.Count(), 1);
+            Assert.AreEqual(allErrors[1].MemberNames.Single(), nameof(ValidationWithDisplayName.AnotherRequiredField));
+            Assert.AreEqual(allErrors[1].ErrorMessage, $"SECOND: {nameof(ValidationWithDisplayName.AnotherRequiredField)}.");
+        }
+
         public class Person : ObservableValidator
         {
             private string name;
@@ -579,5 +601,35 @@ public static ValidationResult ValidateName(string name, ValidationContext conte
                 return new ValidationResult("The name contains invalid characters");
             }
         }
+
+        /// <summary>
+        /// Test model for validation with a formatted display name string on each property.
+        /// This is issue #1 from https://github.com/windows-toolkit/WindowsCommunityToolkit/issues/3763.
+        /// </summary>
+        public class ValidationWithDisplayName : ObservableValidator
+        {
+            public ValidationWithDisplayName()
+            {
+                ValidateAllProperties();
+            }
+
+            private string stringMayNotBeEmpty;
+
+            [Required(AllowEmptyStrings = false, ErrorMessage = "FIRST: {0}.")]
+            public string StringMayNotBeEmpty
+            {
+                get => this.stringMayNotBeEmpty;
+                set => SetProperty(ref this.stringMayNotBeEmpty, value, true);
+            }
+
+            private string anotherRequiredField;
+
+            [Required(AllowEmptyStrings = false, ErrorMessage = "SECOND: {0}.")]
+            public string AnotherRequiredField
+            {
+                get => this.anotherRequiredField;
+                set => SetProperty(ref this.anotherRequiredField, value, true);
+            }
+        }
     }
 }