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

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

@@ -31,6 +31,7 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding
         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 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding
             }
         }
 
+        /// <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 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding
             }
         }
 
-        [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 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding
                 if (property != null)
                 {
                     parameterMapping[parameter] = property;
+                    propertyMapping[property] = parameter;
 
                     if (property.PropertyHasValidators)
                     {
@@ -529,6 +547,7 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding
 
              _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 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding.Validation
 
         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 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding.Validation
 
             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,6 +64,47 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding.Validation
             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]
@@ -71,6 +112,15 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding.Validation
             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 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding.Validation
             }
         }
     }
-}
+}

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

@@ -299,6 +299,32 @@ namespace Microsoft.AspNetCore.Mvc.FunctionalTests
 #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 @@ namespace Microsoft.AspNetCore.Mvc.FunctionalTests
                 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 @@ namespace Microsoft.AspNetCore.Mvc.FunctionalTests
 
             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 @@ namespace HtmlGenerationWebSite.Areas.Customer.Controllers
             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 @@ namespace HtmlGenerationWebSite.Models
         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 @@ using Xunit.Abstractions;
 
 namespace Microsoft.DotNet.Watcher.Tools.FunctionalTests
 {
-    public class BrowserLaunchTests
+    public class BrowserLaunchTests : IDisposable
     {
         private readonly WatchableApp _app;
 
@@ -64,5 +64,10 @@ namespace Microsoft.DotNet.Watcher.Tools.FunctionalTests
             // Verify we launched the browser.
             await _app.Process.GetOutputLineStartsWithAsync(launchBrowserMessage, TimeSpan.FromMinutes(2));
         }
+
+         public void Dispose()
+        {
+            _app.Dispose();
+        }
     }
 }