Fix client validation for record types (#26159)

* Fix client validation for record types

Server validation for record types uses metadata from parameters
when validating record type properties. However client validation
does not use the parameter to harvest client validation attributes.

In the absence of this change, validation on parameters would require server
round trips which is unexcepted and not at parity with validation applied
to properties on regular classes or record types.

Validation experience with record types is subpar and requires server
round trips.

No. This feature is new to 5.0.

Low. The change is isolated to record types and does not affect other code paths. We have
unit and functional test coverage to verify this change.

* Correctly dispose app after use

Author: pranavkm

src/Mvc/Mvc.Abstractions/src/ModelBinding/ModelMetadata.cs

@@ -31,6 +31,7 @@ public abstract class ModelMetadata : IEquatable<ModelMetadata?>, IModelMetadata
         private int? _hashCode;
         private IReadOnlyList<ModelMetadata>? _boundProperties;
         private IReadOnlyDictionary<ModelMetadata, ModelMetadata>? _parameterMapping;
+        private IReadOnlyDictionary<ModelMetadata, ModelMetadata>? _boundConstructorPropertyMapping;
         private Exception? _recordTypeValidatorsOnPropertiesError;
         private bool _recordTypeConstructorDetailsCalculated;
 
@@ -153,6 +154,21 @@ internal IReadOnlyDictionary<ModelMetadata, ModelMetadata> BoundConstructorParam
             }
         }
 
+        /// <summary>
+        /// A mapping from properties to their corresponding constructor parameter on a record type.
+        /// This is the inverse mapping of <see cref="BoundConstructorParameterMapping"/>.
+        /// </summary>
+        internal IReadOnlyDictionary<ModelMetadata, ModelMetadata> BoundConstructorPropertyMapping
+        {
+            get
+            {
+                Debug.Assert(BoundConstructor != null, "This API can be only called for types with bound constructors.");
+                CalculateRecordTypeConstructorDetails();
+
+                return _boundConstructorPropertyMapping;
+            }
+        }
+
         /// <summary>
         /// Gets <see cref="ModelMetadata"/> instance for a constructor of a record type that is used during binding and validation.
         /// </summary>
@@ -492,18 +508,19 @@ internal void ThrowIfRecordTypeHasValidationOnProperties()
             }
         }
 
-        [MemberNotNull(nameof(_parameterMapping))]
+        [MemberNotNull(nameof(_parameterMapping), nameof(_boundConstructorPropertyMapping))]
         private void CalculateRecordTypeConstructorDetails()
         {
             if (_recordTypeConstructorDetailsCalculated)
             {
                 Debug.Assert(_parameterMapping != null);
+                Debug.Assert(_boundConstructorPropertyMapping != null);
                 return;
             }
 
-
             var boundParameters = BoundConstructor!.BoundConstructorParameters!;
             var parameterMapping = new Dictionary<ModelMetadata, ModelMetadata>();
+            var propertyMapping = new Dictionary<ModelMetadata, ModelMetadata>();
 
             foreach (var parameter in boundParameters)
             {
@@ -514,6 +531,7 @@ private void CalculateRecordTypeConstructorDetails()
                 if (property != null)
                 {
                     parameterMapping[parameter] = property;
+                    propertyMapping[property] = parameter;
 
                     if (property.PropertyHasValidators)
                     {
@@ -529,6 +547,7 @@ private void CalculateRecordTypeConstructorDetails()
 
              _recordTypeConstructorDetailsCalculated = true;
             _parameterMapping = parameterMapping;
+            _boundConstructorPropertyMapping = propertyMapping;
         }
 
         /// <summary>

src/Mvc/Mvc.Core/src/ModelBinding/Validation/ClientValidatorCache.cs

@@ -1,10 +1,11 @@
-// Copyright (c) .NET Foundation. All rights reserved.
+// 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.Concurrent;
 using System.Collections.Generic;
 using System.Diagnostics;
+using Microsoft.AspNetCore.Mvc.ModelBinding.Metadata;
 
 namespace Microsoft.AspNetCore.Mvc.ModelBinding.Validation
 {
@@ -14,6 +15,15 @@ public class ClientValidatorCache
 
         public IReadOnlyList<IClientModelValidator> GetValidators(ModelMetadata metadata, IClientModelValidatorProvider validatorProvider)
         {
+            if (metadata.MetadataKind == ModelMetadataKind.Property &&
+                metadata.ContainerMetadata?.BoundConstructor != null &&
+                metadata.ContainerMetadata.BoundConstructorPropertyMapping.TryGetValue(metadata, out var parameter))
+            {
+                // "metadata" typically points to properties. When working with record types, we want to read validation details from the
+                // constructor parameter instead. So let's switch it out.
+                metadata = parameter;
+            }
+
             if (_cacheEntries.TryGetValue(metadata, out var entry))
             {
                 return GetValidatorsFromEntry(entry, metadata, validatorProvider);
@@ -107,7 +117,7 @@ private IReadOnlyList<IClientModelValidator> ExtractValidators(List<ClientValida
 
             var validators = new IClientModelValidator[count];
             var clientValidatorIndex = 0;
-            for (int i = 0; i < items.Count; i++)
+            for (var i = 0; i < items.Count; i++)
             {
                 var validator = items[i].Validator;
                 if (validator != null)

src/Mvc/Mvc.Core/test/ModelBinding/Validation/ClientValidatorCacheTest.cs

@@ -1,4 +1,4 @@
-// Copyright (c) .NET Foundation. All rights reserved.
+// 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.ComponentModel.DataAnnotations;
@@ -64,13 +64,63 @@ public void GetValidators_DoesNotCacheValidatorsWithIsReusableFalse()
             Assert.NotSame(validator2, Assert.Single(validators2.OfType<StringLengthAttributeAdapter>())); // not cached
         }
 
+        [Fact]
+        public void GetValidators_ReadsValidatorsFromCorrespondingRecordTypeParameter()
+        {
+            // Arrange
+            var cache = new ClientValidatorCache();
+            var modelMetadataProvider = new TestModelMetadataProvider();
+            var metadata = modelMetadataProvider.GetMetadataForType(typeof(TestRecordType));
+            var property = metadata.Properties[nameof(TestRecordType.Property1)];
+            var parameter = metadata.BoundConstructor.BoundConstructorParameters.First(f => f.Name == nameof(TestRecordType.Property1));
+            var validatorProvider = new ProviderWithNonReusableValidators();
+
+            // Act
+            var validators = cache.GetValidators(property, validatorProvider);
+
+            // Assert
+            var validator1 = Assert.Single(validators.OfType<RequiredAttributeAdapter>());
+            var validator2 = Assert.Single(validators.OfType<StringLengthAttributeAdapter>());
+            Assert.Contains(validator1.Attribute, parameter.ValidatorMetadata); // Copied by provider
+            Assert.Contains(validator2.Attribute, parameter.ValidatorMetadata); // Copied by provider
+        }
+
+        [Fact]
+        public void GetValidators_ReadsValidatorsFromProperty_IfRecordTypeDoesNotHaveCorrespondingParameter()
+        {
+            // Arrange
+            var cache = new ClientValidatorCache();
+            var modelMetadataProvider = new TestModelMetadataProvider();
+            var metadata = modelMetadataProvider.GetMetadataForType(typeof(TestRecordTypeWithProperty));
+            var property = metadata.Properties[nameof(TestRecordTypeWithProperty.Property2)];
+            var validatorProvider = new ProviderWithNonReusableValidators();
+
+            // Act
+            var validators = cache.GetValidators(property, validatorProvider);
+
+            // Assert
+            var validator1 = Assert.Single(validators.OfType<RequiredAttributeAdapter>());
+            var validator2 = Assert.Single(validators.OfType<StringLengthAttributeAdapter>());
+            Assert.Contains(validator1.Attribute, property.ValidatorMetadata); // Copied by provider
+            Assert.Contains(validator2.Attribute, property.ValidatorMetadata); // Copied by provider
+        }
+
         private class TypeWithProperty
         {
             [Required]
             [StringLength(10)]
             public string Property1 { get; set; }
         }
 
+        private record TestRecordType([Required][StringLength(10)] string Property1);
+
+        private record TestRecordTypeWithProperty([Required][StringLength(10)] string Property1)
+        {
+            [Required]
+            [StringLength(10)]
+            public string Property2 { get; set; }
+        }
+
         private class ProviderWithNonReusableValidators : IClientModelValidatorProvider
         {
             public void CreateValidators(ClientValidatorProviderContext context)
@@ -101,4 +151,4 @@ public void CreateValidators(ClientValidatorProviderContext context)
             }
         }
     }
-}
+}

src/Mvc/test/Mvc.FunctionalTests/HtmlGenerationTest.cs

@@ -299,6 +299,32 @@ public async Task ValidationTagHelpers_GeneratesExpectedSpansAndDivs()
 #endif
         }
 
+        [Fact]
+        public async Task ClientValidators_AreGeneratedDuringInitialRender()
+        {
+            // Arrange
+            var request = new HttpRequestMessage(HttpMethod.Get, "http://localhost/Customer/HtmlGeneration_Customer/CustomerWithRecords");
+
+            // Act
+            var response = await Client.SendAsync(request);
+
+            // Assert
+            var document = await response.GetHtmlDocumentAsync();
+
+            var numberInput = document.RequiredQuerySelector("input[id=Number]");
+            Assert.Equal("true", numberInput.GetAttribute("data-val"));
+            Assert.Equal("The field Number must be between 1 and 100.", numberInput.GetAttribute("data-val-range"));
+            Assert.Equal("The Number field is required.", numberInput.GetAttribute("data-val-required"));
+
+            var passwordInput = document.RequiredQuerySelector("input[id=Password]");
+            Assert.Equal("true", passwordInput.GetAttribute("data-val"));
+            Assert.Equal("The Password field is required.", passwordInput.GetAttribute("data-val-required"));
+
+            var addressInput = document.RequiredQuerySelector("input[id=Address]");
+            Assert.Equal("true", addressInput.GetAttribute("data-val"));
+            Assert.Equal("The Address field is required.", addressInput.GetAttribute("data-val-required"));
+        }
+
         [Fact]
         public async Task ValidationTagHelpers_UsingRecords()
         {
@@ -310,7 +336,8 @@ public async Task ValidationTagHelpers_UsingRecords()
                 new KeyValuePair<string,string>("Name", string.Empty),
                 new KeyValuePair<string,string>("Email", string.Empty),
                 new KeyValuePair<string,string>("PhoneNumber", string.Empty),
-                new KeyValuePair<string,string>("Password", string.Empty)
+                new KeyValuePair<string,string>("Password", string.Empty),
+                new KeyValuePair<string,string>("Address", string.Empty),
             };
             request.Content = new FormUrlEncodedContent(nameValueCollection);
 
@@ -331,6 +358,9 @@ public async Task ValidationTagHelpers_UsingRecords()
 
             validation = document.QuerySelector("span[data-valmsg-for=Password]");
             Assert.Equal("The Password field is required.", validation.TextContent);
+
+            validation = document.QuerySelector("span[data-valmsg-for=Address]");
+            Assert.Equal("The Address field is required.", validation.TextContent);
         }
 
         [Fact]

src/Mvc/test/WebSites/HtmlGenerationWebSite/Areas/Customer/Controllers/HtmlGeneration_CustomerController.cs

@@ -13,9 +13,15 @@ public IActionResult Index(Models.Customer customer)
             return View("Customer");
         }
 
+        [HttpGet]
+        public IActionResult CustomerWithRecords()
+        {
+            return View("CustomerWithRecords");
+        }
+
         public IActionResult CustomerWithRecords(Models.CustomerRecord customer)
         {
             return View("CustomerWithRecords");
         }
     }
-}
+}

src/Mvc/test/WebSites/HtmlGenerationWebSite/Models/CustomerRecord.cs

@@ -25,5 +25,9 @@ public record CustomerRecord
         string Email,
 
         string Key
-    );
-}
+    )
+    {
+        [Required]
+        public string Address { get; set; }
+    }
+}

src/Mvc/test/WebSites/HtmlGenerationWebSite/Views/Shared/CustomerWithRecords.cshtml

@@ -37,10 +37,15 @@
             <input asp-for="Gender" type="radio" value="Male" /> Male
             <input asp-for="Gender" type="radio" value="Female" /> Female
             <span asp-validation-for="Gender"></span>
+        </div>
+         <div>
+            <label asp-for="Address" class="order"></label>
+            <input asp-for="Address" type="text" />
+            <span asp-validation-for="Address"></span>
         </div>
         <div id="validation-summary-all" asp-validation-summary="All" class="order"></div>
         <div id="validation-summary-model" asp-validation-summary="ModelOnly" class="order"></div>
         <input type="submit"/>
     </form>
 </body>
-</html>
+</html>

src/Tools/dotnet-watch/test/BrowserLaunchTests.cs

@@ -9,7 +9,7 @@
 
 namespace Microsoft.DotNet.Watcher.Tools.FunctionalTests
 {
-    public class BrowserLaunchTests
+    public class BrowserLaunchTests : IDisposable
     {
         private readonly WatchableApp _app;
 
@@ -64,5 +64,10 @@ public async Task UsesBrowserSpecifiedInEnvironment()
             // Verify we launched the browser.
             await _app.Process.GetOutputLineStartsWithAsync(launchBrowserMessage, TimeSpan.FromMinutes(2));
         }
+
+         public void Dispose()
+        {
+            _app.Dispose();
+        }
     }
 }