From da3f97b0ad76c4d95c661e51c496fa214c41e824 Mon Sep 17 00:00:00 2001 From: Pranav K Date: Tue, 22 Sep 2020 22:02:49 -0700 Subject: [PATCH] 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/ModelBinding/ModelMetadata.cs | 23 +++++++- .../Validation/ClientValidatorCache.cs | 14 ++++- .../Validation/ClientValidatorCacheTest.cs | 54 ++++++++++++++++++- .../Mvc.FunctionalTests/HtmlGenerationTest.cs | 32 ++++++++++- .../HtmlGeneration_CustomerController.cs | 8 ++- .../Models/CustomerRecord.cs | 8 ++- .../Views/Shared/CustomerWithRecords.cshtml | 7 ++- .../dotnet-watch/test/BrowserLaunchTests.cs | 7 ++- 8 files changed, 141 insertions(+), 12 deletions(-) diff --git a/src/Mvc/Mvc.Abstractions/src/ModelBinding/ModelMetadata.cs b/src/Mvc/Mvc.Abstractions/src/ModelBinding/ModelMetadata.cs index d27f38d104..a693accb8b 100644 --- a/src/Mvc/Mvc.Abstractions/src/ModelBinding/ModelMetadata.cs +++ b/src/Mvc/Mvc.Abstractions/src/ModelBinding/ModelMetadata.cs @@ -31,6 +31,7 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding private int? _hashCode; private IReadOnlyList? _boundProperties; private IReadOnlyDictionary? _parameterMapping; + private IReadOnlyDictionary? _boundConstructorPropertyMapping; private Exception? _recordTypeValidatorsOnPropertiesError; private bool _recordTypeConstructorDetailsCalculated; @@ -153,6 +154,21 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding } } + /// + /// A mapping from properties to their corresponding constructor parameter on a record type. + /// This is the inverse mapping of . + /// + internal IReadOnlyDictionary BoundConstructorPropertyMapping + { + get + { + Debug.Assert(BoundConstructor != null, "This API can be only called for types with bound constructors."); + CalculateRecordTypeConstructorDetails(); + + return _boundConstructorPropertyMapping; + } + } + /// /// Gets instance for a constructor of a record type that is used during binding and validation. /// @@ -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(); + var propertyMapping = new Dictionary(); 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; } /// diff --git a/src/Mvc/Mvc.Core/src/ModelBinding/Validation/ClientValidatorCache.cs b/src/Mvc/Mvc.Core/src/ModelBinding/Validation/ClientValidatorCache.cs index 2697daec71..41faf4d8a3 100644 --- a/src/Mvc/Mvc.Core/src/ModelBinding/Validation/ClientValidatorCache.cs +++ b/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 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) diff --git a/src/Mvc/Mvc.Core/test/ModelBinding/Validation/ClientValidatorCacheTest.cs b/src/Mvc/Mvc.Core/test/ModelBinding/Validation/ClientValidatorCacheTest.cs index bb9c6f5c0c..7d4a8aeb38 100644 --- a/src/Mvc/Mvc.Core/test/ModelBinding/Validation/ClientValidatorCacheTest.cs +++ b/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())); // 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()); + var validator2 = Assert.Single(validators.OfType()); + 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()); + var validator2 = Assert.Single(validators.OfType()); + 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 } } } -} \ No newline at end of file +} diff --git a/src/Mvc/test/Mvc.FunctionalTests/HtmlGenerationTest.cs b/src/Mvc/test/Mvc.FunctionalTests/HtmlGenerationTest.cs index 98416f442a..dfaaa2fb5f 100644 --- a/src/Mvc/test/Mvc.FunctionalTests/HtmlGenerationTest.cs +++ b/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("Name", string.Empty), new KeyValuePair("Email", string.Empty), new KeyValuePair("PhoneNumber", string.Empty), - new KeyValuePair("Password", string.Empty) + new KeyValuePair("Password", string.Empty), + new KeyValuePair("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] diff --git a/src/Mvc/test/WebSites/HtmlGenerationWebSite/Areas/Customer/Controllers/HtmlGeneration_CustomerController.cs b/src/Mvc/test/WebSites/HtmlGenerationWebSite/Areas/Customer/Controllers/HtmlGeneration_CustomerController.cs index e79741b381..b1d021a62a 100644 --- a/src/Mvc/test/WebSites/HtmlGenerationWebSite/Areas/Customer/Controllers/HtmlGeneration_CustomerController.cs +++ b/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"); } } -} \ No newline at end of file +} diff --git a/src/Mvc/test/WebSites/HtmlGenerationWebSite/Models/CustomerRecord.cs b/src/Mvc/test/WebSites/HtmlGenerationWebSite/Models/CustomerRecord.cs index 3e2c6df9e9..d131f08484 100644 --- a/src/Mvc/test/WebSites/HtmlGenerationWebSite/Models/CustomerRecord.cs +++ b/src/Mvc/test/WebSites/HtmlGenerationWebSite/Models/CustomerRecord.cs @@ -25,5 +25,9 @@ namespace HtmlGenerationWebSite.Models string Email, string Key - ); -} \ No newline at end of file + ) + { + [Required] + public string Address { get; set; } + } +} diff --git a/src/Mvc/test/WebSites/HtmlGenerationWebSite/Views/Shared/CustomerWithRecords.cshtml b/src/Mvc/test/WebSites/HtmlGenerationWebSite/Views/Shared/CustomerWithRecords.cshtml index 13dddfc081..1edd936847 100644 --- a/src/Mvc/test/WebSites/HtmlGenerationWebSite/Views/Shared/CustomerWithRecords.cshtml +++ b/src/Mvc/test/WebSites/HtmlGenerationWebSite/Views/Shared/CustomerWithRecords.cshtml @@ -37,10 +37,15 @@ Male Female + +
+ + +
- \ No newline at end of file + diff --git a/src/Tools/dotnet-watch/test/BrowserLaunchTests.cs b/src/Tools/dotnet-watch/test/BrowserLaunchTests.cs index 77e9a65da6..aa13d6de60 100644 --- a/src/Tools/dotnet-watch/test/BrowserLaunchTests.cs +++ b/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(); + } } }