From 17a6e5b7634ab385df028a9a2fa4d23d6ac4f72e Mon Sep 17 00:00:00 2001 From: Steve Sanderson Date: Wed, 31 Mar 2021 15:06:11 +0100 Subject: [PATCH 01/11] Add failing E2E test --- .../test/E2ETest/Tests/FormsTest.cs | 18 ++ .../TypicalValidationComponent.razor | 192 ++++++++++-------- src/Shared/E2ETesting/BrowserFixture.cs | 3 +- 3 files changed, 124 insertions(+), 89 deletions(-) diff --git a/src/Components/test/E2ETest/Tests/FormsTest.cs b/src/Components/test/E2ETest/Tests/FormsTest.cs index b7a799f69b41..e588a0c840a1 100644 --- a/src/Components/test/E2ETest/Tests/FormsTest.cs +++ b/src/Components/test/E2ETest/Tests/FormsTest.cs @@ -572,6 +572,24 @@ public void NavigateOnSubmitWorks() Assert.DoesNotContain(log, entry => entry.Level == LogLevel.Severe); } + [Fact] + public void EditFormSubscriptionsAreRemovedOnDisposal() + { + var appElement = MountTypicalValidationComponent(); + var messagesAccessor = CreateValidationMessagesAccessor(appElement); + + // Remove the old form and add a new one + appElement.FindElement(By.Id("recreate-edit-form")).Click(); + Browser.Equal("Recreated form", () => appElement.FindElement(By.CssSelector(".submission-log-entry:last-of-type")).Text); + + // Verify there's still only one copy of each validation message + var nameInput = appElement.FindElement(By.ClassName("name")).FindElement(By.TagName("input")); + nameInput.SendKeys("Bert\t"); + nameInput.Clear(); + nameInput.SendKeys("\t"); + Browser.Equal(new[] { "Enter a name" }, messagesAccessor); + } + private Func CreateValidationMessagesAccessor(IWebElement appElement) { return () => appElement.FindElements(By.ClassName("validation-message")) diff --git a/src/Components/test/testassets/BasicTestApp/FormsTest/TypicalValidationComponent.razor b/src/Components/test/testassets/BasicTestApp/FormsTest/TypicalValidationComponent.razor index 4f91e4653867..5194dce6792e 100644 --- a/src/Components/test/testassets/BasicTestApp/FormsTest/TypicalValidationComponent.razor +++ b/src/Components/test/testassets/BasicTestApp/FormsTest/TypicalValidationComponent.razor @@ -1,95 +1,101 @@ @using System.ComponentModel.DataAnnotations @using Microsoft.AspNetCore.Components.Forms - - - -

- Name: -

- - -

- Age (years): -

-

- Height (optional): -

-

- Description: -

-

- Renewal date: -

-

- Expiry date (optional): -

-

- Ticket class: - - - - - - - @person.TicketClass -

-

- - Airline: -
- @foreach (var airline in (Airline[])Enum.GetValues(typeof(Airline))) - { - - @airline.ToString(); +@if (showForm) +{ + + + +

+ Name: +

+ + +

+ Age (years): +

+

+ Height (optional): +

+

+ Description: +

+

+ Renewal date: +

+

+ Expiry date (optional): +

+

+ Ticket class: + + + + + + + @person.TicketClass +

+

+ + Airline:
- } - -

-

- Pick one color and one country: - - - red
- japan
- green
- yemen
- blue
- latvia
- orange
+ @foreach (var airline in (Airline[])Enum.GetValues(typeof(Airline))) + { + + @airline.ToString(); +
+ } - -

-

- Socks color: -

-

- Accepts terms: -

-

- Is evil: -

-

- Username (optional): - -

- - - -

- -

- - - - - +

+

+ Pick one color and one country: + + + red
+ japan
+ green
+ yemen
+ blue
+ latvia
+ orange
+ + +

+

+ Socks color: +

+

+ Accepts terms: +

+

+ Is evil: +

+

+ Username (optional): + +

+ + + +

+ +

+ + + +} + +
    @foreach (var entry in submissionLog) +{
  • @entry
    • }
+ + @code { protected virtual bool UseExperimentalValidator => false; @@ -97,6 +103,7 @@ Person person = new Person(); EditContext editContext; ValidationMessageStore customValidationMessageStore; + bool showForm = true; protected override void OnInitialized() { @@ -187,4 +194,15 @@ _ = InvokeAsync(editContext.NotifyValidationStateChanged); }); } + + async Task RecreateEditForm() + { + // This is to show that, if a form is removed and a new one is added, + // the subscriptions related to the old one are cleared. + // Represents the bug reported at https://github.com/dotnet/aspnetcore/issues/31027 + showForm = false; + await Task.Delay(1); + showForm = true; + submissionLog.Add("Recreated form"); + } } diff --git a/src/Shared/E2ETesting/BrowserFixture.cs b/src/Shared/E2ETesting/BrowserFixture.cs index 2e3838415b7c..2d53261be8c0 100644 --- a/src/Shared/E2ETesting/BrowserFixture.cs +++ b/src/Shared/E2ETesting/BrowserFixture.cs @@ -143,8 +143,7 @@ private async Task DeleteBrowserUserProfileDirectoriesAsync() // Force language to english for tests opts.AddUserProfilePreference("intl.accept_languages", "en"); - // Comment this out if you want to watch or interact with the browser (e.g., for debugging) - if (!Debugger.IsAttached) + if (!Debugger.IsAttached && !string.Equals(Environment.GetEnvironmentVariable("E2E_TEST_VISIBLE"), "true", StringComparison.OrdinalIgnoreCase)) { opts.AddArgument("--headless"); } From d91dd28911d3d5a99c387e4865a35e6d6640034e Mon Sep 17 00:00:00 2001 From: Steve Sanderson Date: Wed, 31 Mar 2021 15:36:33 +0100 Subject: [PATCH 02/11] Implement fix --- .../Forms/src/DataAnnotationsValidator.cs | 19 ++- .../EditContextDataAnnotationsExtensions.cs | 151 ++++++++++-------- .../Forms/src/PublicAPI.Unshipped.txt | 2 + ...fyPropertyChangedValidationComponent.razor | 3 +- 4 files changed, 109 insertions(+), 66 deletions(-) diff --git a/src/Components/Forms/src/DataAnnotationsValidator.cs b/src/Components/Forms/src/DataAnnotationsValidator.cs index 65e059d178a9..e08ff2332157 100644 --- a/src/Components/Forms/src/DataAnnotationsValidator.cs +++ b/src/Components/Forms/src/DataAnnotationsValidator.cs @@ -8,8 +8,10 @@ namespace Microsoft.AspNetCore.Components.Forms /// /// Adds Data Annotations validation support to an . /// - public class DataAnnotationsValidator : ComponentBase + public class DataAnnotationsValidator : ComponentBase, IDisposable { + private IDisposable? _subscriptions; + [CascadingParameter] EditContext? CurrentEditContext { get; set; } /// @@ -22,7 +24,20 @@ protected override void OnInitialized() $"inside an EditForm."); } - CurrentEditContext.AddDataAnnotationsValidation(); + _subscriptions = CurrentEditContext.EnableDataAnnotationsValidation(); + } + + /// + protected virtual void Dispose(bool disposing) + { + } + + void IDisposable.Dispose() + { + _subscriptions?.Dispose(); + _subscriptions = null; + + Dispose(disposing: true); } } } diff --git a/src/Components/Forms/src/EditContextDataAnnotationsExtensions.cs b/src/Components/Forms/src/EditContextDataAnnotationsExtensions.cs index 56c226455357..55d4e94f6676 100644 --- a/src/Components/Forms/src/EditContextDataAnnotationsExtensions.cs +++ b/src/Components/Forms/src/EditContextDataAnnotationsExtensions.cs @@ -16,98 +16,123 @@ namespace Microsoft.AspNetCore.Components.Forms /// public static class EditContextDataAnnotationsExtensions { - private static ConcurrentDictionary<(Type ModelType, string FieldName), PropertyInfo?> _propertyInfoCache - = new ConcurrentDictionary<(Type, string), PropertyInfo?>(); - /// /// Adds DataAnnotations validation support to the . /// /// The . + [Obsolete("Use " + nameof(EnableDataAnnotationsValidation) + " instead.")] public static EditContext AddDataAnnotationsValidation(this EditContext editContext) { - if (editContext == null) - { - throw new ArgumentNullException(nameof(editContext)); - } - - var messages = new ValidationMessageStore(editContext); - - // Perform object-level validation on request - editContext.OnValidationRequested += - (sender, eventArgs) => ValidateModel((EditContext)sender!, messages); - - // Perform per-field validation on each field edit - editContext.OnFieldChanged += - (sender, eventArgs) => ValidateField(editContext, messages, eventArgs.FieldIdentifier); - + EnableDataAnnotationsValidation(editContext); return editContext; } - private static void ValidateModel(EditContext editContext, ValidationMessageStore messages) + /// + /// Enables DataAnnotations validation support for the . + /// + /// The . + /// A disposable object whose disposal will remove DataAnnotations validation support from the . + public static IDisposable EnableDataAnnotationsValidation(this EditContext editContext) + { + return new DataAnnotationsEventSubscriptions(editContext); + } + + class DataAnnotationsEventSubscriptions : IDisposable { - var validationContext = new ValidationContext(editContext.Model); - var validationResults = new List(); - Validator.TryValidateObject(editContext.Model, validationContext, validationResults, true); + private static ConcurrentDictionary<(Type ModelType, string FieldName), PropertyInfo?> _propertyInfoCache + = new ConcurrentDictionary<(Type, string), PropertyInfo?>(); + + private readonly EditContext _editContext; + private readonly ValidationMessageStore _messages; - // Transfer results to the ValidationMessageStore - messages.Clear(); - foreach (var validationResult in validationResults) + public DataAnnotationsEventSubscriptions(EditContext editContext) { - if (validationResult == null) + if (editContext == null) { - continue; + throw new ArgumentNullException(nameof(editContext)); } - if (!validationResult.MemberNames.Any()) - { - messages.Add(new FieldIdentifier(editContext.Model, fieldName: string.Empty), validationResult.ErrorMessage!); - continue; - } + _editContext = editContext ?? throw new ArgumentNullException(nameof(editContext)); + _messages = new ValidationMessageStore(_editContext); - foreach (var memberName in validationResult.MemberNames) + _editContext.OnFieldChanged += OnFieldChanged; + _editContext.OnValidationRequested += OnValidationRequested; + } + + private void OnFieldChanged(object? sender, FieldChangedEventArgs eventArgs) + { + var fieldIdentifier = eventArgs.FieldIdentifier; + if (TryGetValidatableProperty(fieldIdentifier, out var propertyInfo)) { - messages.Add(editContext.Field(memberName), validationResult.ErrorMessage!); + var propertyValue = propertyInfo.GetValue(fieldIdentifier.Model); + var validationContext = new ValidationContext(fieldIdentifier.Model) + { + MemberName = propertyInfo.Name + }; + var results = new List(); + + Validator.TryValidateProperty(propertyValue, validationContext, results); + _messages.Clear(fieldIdentifier); + _messages.Add(fieldIdentifier, results.Select(result => result.ErrorMessage!)); + + // We have to notify even if there were no messages before and are still no messages now, + // because the "state" that changed might be the completion of some async validation task + _editContext.NotifyValidationStateChanged(); } } - editContext.NotifyValidationStateChanged(); - } - - private static void ValidateField(EditContext editContext, ValidationMessageStore messages, in FieldIdentifier fieldIdentifier) - { - if (TryGetValidatableProperty(fieldIdentifier, out var propertyInfo)) + private void OnValidationRequested(object? sender, ValidationRequestedEventArgs e) { - var propertyValue = propertyInfo.GetValue(fieldIdentifier.Model); - var validationContext = new ValidationContext(fieldIdentifier.Model) + var validationContext = new ValidationContext(_editContext.Model); + var validationResults = new List(); + Validator.TryValidateObject(_editContext.Model, validationContext, validationResults, true); + + // Transfer results to the ValidationMessageStore + _messages.Clear(); + foreach (var validationResult in validationResults) { - MemberName = propertyInfo.Name - }; - var results = new List(); + if (validationResult == null) + { + continue; + } + + if (!validationResult.MemberNames.Any()) + { + _messages.Add(new FieldIdentifier(_editContext.Model, fieldName: string.Empty), validationResult.ErrorMessage!); + continue; + } + + foreach (var memberName in validationResult.MemberNames) + { + _messages.Add(_editContext.Field(memberName), validationResult.ErrorMessage!); + } + } - Validator.TryValidateProperty(propertyValue, validationContext, results); - messages.Clear(fieldIdentifier); - messages.Add(fieldIdentifier, results.Select(result => result.ErrorMessage!)); + _editContext.NotifyValidationStateChanged(); + } - // We have to notify even if there were no messages before and are still no messages now, - // because the "state" that changed might be the completion of some async validation task - editContext.NotifyValidationStateChanged(); + public void Dispose() + { + _editContext.OnFieldChanged -= OnFieldChanged; + _editContext.OnValidationRequested -= OnValidationRequested; + _messages.Clear(); } - } - private static bool TryGetValidatableProperty(in FieldIdentifier fieldIdentifier, [NotNullWhen(true)] out PropertyInfo? propertyInfo) - { - var cacheKey = (ModelType: fieldIdentifier.Model.GetType(), fieldIdentifier.FieldName); - if (!_propertyInfoCache.TryGetValue(cacheKey, out propertyInfo)) + private static bool TryGetValidatableProperty(in FieldIdentifier fieldIdentifier, [NotNullWhen(true)] out PropertyInfo? propertyInfo) { - // DataAnnotations only validates public properties, so that's all we'll look for - // If we can't find it, cache 'null' so we don't have to try again next time - propertyInfo = cacheKey.ModelType.GetProperty(cacheKey.FieldName); + var cacheKey = (ModelType: fieldIdentifier.Model.GetType(), fieldIdentifier.FieldName); + if (!_propertyInfoCache.TryGetValue(cacheKey, out propertyInfo)) + { + // DataAnnotations only validates public properties, so that's all we'll look for + // If we can't find it, cache 'null' so we don't have to try again next time + propertyInfo = cacheKey.ModelType.GetProperty(cacheKey.FieldName); - // No need to lock, because it doesn't matter if we write the same value twice - _propertyInfoCache[cacheKey] = propertyInfo; - } + // No need to lock, because it doesn't matter if we write the same value twice + _propertyInfoCache[cacheKey] = propertyInfo; + } - return propertyInfo != null; + return propertyInfo != null; + } } } } diff --git a/src/Components/Forms/src/PublicAPI.Unshipped.txt b/src/Components/Forms/src/PublicAPI.Unshipped.txt index 7dc5c58110bf..a412262cbbf0 100644 --- a/src/Components/Forms/src/PublicAPI.Unshipped.txt +++ b/src/Components/Forms/src/PublicAPI.Unshipped.txt @@ -1 +1,3 @@ #nullable enable +static Microsoft.AspNetCore.Components.Forms.EditContextDataAnnotationsExtensions.EnableDataAnnotationsValidation(this Microsoft.AspNetCore.Components.Forms.EditContext! editContext) -> System.IDisposable! +virtual Microsoft.AspNetCore.Components.Forms.DataAnnotationsValidator.Dispose(bool disposing) -> void diff --git a/src/Components/test/testassets/BasicTestApp/FormsTest/NotifyPropertyChangedValidationComponent.razor b/src/Components/test/testassets/BasicTestApp/FormsTest/NotifyPropertyChangedValidationComponent.razor index a257c1f2c97b..936416016ab9 100644 --- a/src/Components/test/testassets/BasicTestApp/FormsTest/NotifyPropertyChangedValidationComponent.razor +++ b/src/Components/test/testassets/BasicTestApp/FormsTest/NotifyPropertyChangedValidationComponent.razor @@ -41,7 +41,8 @@ protected override void OnInitialized() { - editContext = new EditContext(person).AddDataAnnotationsValidation(); + editContext = new EditContext(person); + editContext.EnableDataAnnotationsValidation(); // Wire up INotifyPropertyChanged to the EditContext person.PropertyChanged += (sender, eventArgs) => From 01c7eedcf3c5e247afed79f325bb34888c00d1b4 Mon Sep 17 00:00:00 2001 From: Steve Sanderson Date: Wed, 31 Mar 2021 15:56:47 +0100 Subject: [PATCH 03/11] Fully update the validation state after disposal. Make the E2E test able to see this. --- .../EditContextDataAnnotationsExtensions.cs | 3 +- .../test/E2ETest/Tests/FormsTest.cs | 28 ++- .../TypicalValidationComponent.razor | 187 +++++++++--------- 3 files changed, 111 insertions(+), 107 deletions(-) diff --git a/src/Components/Forms/src/EditContextDataAnnotationsExtensions.cs b/src/Components/Forms/src/EditContextDataAnnotationsExtensions.cs index 55d4e94f6676..451f4fd2ff9d 100644 --- a/src/Components/Forms/src/EditContextDataAnnotationsExtensions.cs +++ b/src/Components/Forms/src/EditContextDataAnnotationsExtensions.cs @@ -113,9 +113,10 @@ private void OnValidationRequested(object? sender, ValidationRequestedEventArgs public void Dispose() { + _messages.Clear(); _editContext.OnFieldChanged -= OnFieldChanged; _editContext.OnValidationRequested -= OnValidationRequested; - _messages.Clear(); + _editContext.NotifyValidationStateChanged(); } private static bool TryGetValidatableProperty(in FieldIdentifier fieldIdentifier, [NotNullWhen(true)] out PropertyInfo? propertyInfo) diff --git a/src/Components/test/E2ETest/Tests/FormsTest.cs b/src/Components/test/E2ETest/Tests/FormsTest.cs index e588a0c840a1..847480b55c20 100644 --- a/src/Components/test/E2ETest/Tests/FormsTest.cs +++ b/src/Components/test/E2ETest/Tests/FormsTest.cs @@ -573,21 +573,29 @@ public void NavigateOnSubmitWorks() } [Fact] - public void EditFormSubscriptionsAreRemovedOnDisposal() + public void CanRemoveAndReAddDataAnnotationsSupport() { var appElement = MountTypicalValidationComponent(); var messagesAccessor = CreateValidationMessagesAccessor(appElement); + var nameInput = appElement.FindElement(By.ClassName("name")).FindElement(By.TagName("input")); + Func lastLogEntryAccessor = () => appElement.FindElement(By.CssSelector(".submission-log-entry:last-of-type")).Text; - // Remove the old form and add a new one - appElement.FindElement(By.Id("recreate-edit-form")).Click(); - Browser.Equal("Recreated form", () => appElement.FindElement(By.CssSelector(".submission-log-entry:last-of-type")).Text); + nameInput.SendKeys("01234567890123456789\t"); + Browser.Equal("modified invalid", () => nameInput.GetAttribute("class")); + Browser.Equal(new[] { "That name is too long" }, messagesAccessor); - // Verify there's still only one copy of each validation message - var nameInput = appElement.FindElement(By.ClassName("name")).FindElement(By.TagName("input")); - nameInput.SendKeys("Bert\t"); - nameInput.Clear(); - nameInput.SendKeys("\t"); - Browser.Equal(new[] { "Enter a name" }, messagesAccessor); + // Remove DataAnnotations support + appElement.FindElement(By.Id("toggle-dataannotations")).Click(); + Browser.Equal("DataAnnotations support is now disabled", lastLogEntryAccessor); + Browser.Equal("modified valid", () => nameInput.GetAttribute("class")); + Browser.Empty(messagesAccessor); + + // Re-add DataAnnotations support + appElement.FindElement(By.Id("toggle-dataannotations")).Click(); + nameInput.SendKeys("0\t"); + Browser.Equal("DataAnnotations support is now enabled", lastLogEntryAccessor); + Browser.Equal("modified invalid", () => nameInput.GetAttribute("class")); + Browser.Equal(new[] { "That name is too long" }, messagesAccessor); } private Func CreateValidationMessagesAccessor(IWebElement appElement) diff --git a/src/Components/test/testassets/BasicTestApp/FormsTest/TypicalValidationComponent.razor b/src/Components/test/testassets/BasicTestApp/FormsTest/TypicalValidationComponent.razor index 5194dce6792e..8bab42c8c3a6 100644 --- a/src/Components/test/testassets/BasicTestApp/FormsTest/TypicalValidationComponent.razor +++ b/src/Components/test/testassets/BasicTestApp/FormsTest/TypicalValidationComponent.razor @@ -1,101 +1,101 @@ @using System.ComponentModel.DataAnnotations @using Microsoft.AspNetCore.Components.Forms -@if (showForm) -{ - + + @if (enableDataAnnotationsSupport) + { + } -

- Name: -

- - -

- Age (years): -

-

- Height (optional): -

-

- Description: -

-

- Renewal date: -

-

- Expiry date (optional): -

-

- Ticket class: - - - - - - - @person.TicketClass -

-

- - Airline: +

+ Name: +

+ + +

+ Age (years): +

+

+ Height (optional): +

+

+ Description: +

+

+ Renewal date: +

+

+ Expiry date (optional): +

+

+ Ticket class: + + + + + + + @person.TicketClass +

+

+ + Airline: +
+ @foreach (var airline in (Airline[])Enum.GetValues(typeof(Airline))) + { + + @airline.ToString();
- @foreach (var airline in (Airline[])Enum.GetValues(typeof(Airline))) - { - - @airline.ToString(); -
- } + } + +

+

+ Pick one color and one country: + + + red
+ japan
+ green
+ yemen
+ blue
+ latvia
+ orange
 -

-

- Pick one color and one country: - - - red
- japan
- green
- yemen
- blue
- latvia
- orange
- - -

-

- Socks color: -

-

- Accepts terms: -

-

- Is evil: -

-

- Username (optional): - -

- - - -

- -

- - - -} + +

+

+ Socks color: +

+

+ Accepts terms: +

+

+ Is evil: +

+

+ Username (optional): + +

+ + + +

+ +

+ + +
    @foreach (var entry in submissionLog) {
  • @entry
    • }
- + @code { protected virtual bool UseExperimentalValidator => false; @@ -103,7 +103,7 @@ Person person = new Person(); EditContext editContext; ValidationMessageStore customValidationMessageStore; - bool showForm = true; + bool enableDataAnnotationsSupport = true; protected override void OnInitialized() { @@ -195,14 +195,9 @@ }); } - async Task RecreateEditForm() + void ToggleDataAnnotations() { - // This is to show that, if a form is removed and a new one is added, - // the subscriptions related to the old one are cleared. - // Represents the bug reported at https://github.com/dotnet/aspnetcore/issues/31027 - showForm = false; - await Task.Delay(1); - showForm = true; - submissionLog.Add("Recreated form"); + enableDataAnnotationsSupport = !enableDataAnnotationsSupport; + submissionLog.Add($"DataAnnotations support is now {(enableDataAnnotationsSupport ? "enabled" : "disabled")}"); } } From ea62a4b7735ead45f10556a07a622e71aec7b148 Mon Sep 17 00:00:00 2001 From: Steve Sanderson Date: Wed, 31 Mar 2021 16:04:56 +0100 Subject: [PATCH 04/11] Spacing fixes --- .../FormsTest/TypicalValidationComponent.razor | 7 +++---- src/Shared/E2ETesting/BrowserFixture.cs | 3 ++- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/src/Components/test/testassets/BasicTestApp/FormsTest/TypicalValidationComponent.razor b/src/Components/test/testassets/BasicTestApp/FormsTest/TypicalValidationComponent.razor index 8bab42c8c3a6..c6be1322d73c 100644 --- a/src/Components/test/testassets/BasicTestApp/FormsTest/TypicalValidationComponent.razor +++ b/src/Components/test/testassets/BasicTestApp/FormsTest/TypicalValidationComponent.razor @@ -48,11 +48,11 @@ Airline:
@foreach (var airline in (Airline[])Enum.GetValues(typeof(Airline))) - { + { @airline.ToString();
- } + }

@@ -92,8 +92,7 @@ -

    @foreach (var entry in submissionLog) -{
  • @entry
    • }
+
    @foreach (var entry in submissionLog) {
  • @entry
    • }
diff --git a/src/Shared/E2ETesting/BrowserFixture.cs b/src/Shared/E2ETesting/BrowserFixture.cs index 2d53261be8c0..f7d5c8d62a44 100644 --- a/src/Shared/E2ETesting/BrowserFixture.cs +++ b/src/Shared/E2ETesting/BrowserFixture.cs @@ -143,7 +143,8 @@ private async Task DeleteBrowserUserProfileDirectoriesAsync() // Force language to english for tests opts.AddUserProfilePreference("intl.accept_languages", "en"); - if (!Debugger.IsAttached && !string.Equals(Environment.GetEnvironmentVariable("E2E_TEST_VISIBLE"), "true", StringComparison.OrdinalIgnoreCase)) + if (!Debugger.IsAttached && + !string.Equals(Environment.GetEnvironmentVariable("E2E_TEST_VISIBLE"), "true", StringComparison.OrdinalIgnoreCase)) { opts.AddArgument("--headless"); } From 52f11c8830ada5cd04ec2bc1e605b0b3a9234a2a Mon Sep 17 00:00:00 2001 From: Steve Sanderson Date: Thu, 1 Apr 2021 17:00:11 +0100 Subject: [PATCH 05/11] Update unit tests --- ...ditContextDataAnnotationsExtensionsTest.cs | 39 +++++++++++++++---- 1 file changed, 32 insertions(+), 7 deletions(-) diff --git a/src/Components/Forms/test/EditContextDataAnnotationsExtensionsTest.cs b/src/Components/Forms/test/EditContextDataAnnotationsExtensionsTest.cs index bb7837e2f08f..111c87e2529f 100644 --- a/src/Components/Forms/test/EditContextDataAnnotationsExtensionsTest.cs +++ b/src/Components/Forms/test/EditContextDataAnnotationsExtensionsTest.cs @@ -13,15 +13,17 @@ public class EditContextDataAnnotationsExtensionsTest public void CannotUseNullEditContext() { var editContext = (EditContext)null; - var ex = Assert.Throws(() => editContext.AddDataAnnotationsValidation()); + var ex = Assert.Throws(() => editContext.EnableDataAnnotationsValidation()); Assert.Equal("editContext", ex.ParamName); } [Fact] - public void ReturnsEditContextForChaining() + public void ObsoleteApiReturnsEditContextForChaining() { var editContext = new EditContext(new object()); +#pragma warning disable 0618 var returnValue = editContext.AddDataAnnotationsValidation(); +#pragma warning restore 0618 Assert.Same(editContext, returnValue); } @@ -30,7 +32,8 @@ public void GetsValidationMessagesFromDataAnnotations() { // Arrange var model = new TestModel { IntFrom1To100 = 101 }; - var editContext = new EditContext(model).AddDataAnnotationsValidation(); + var editContext = new EditContext(model); + editContext.EnableDataAnnotationsValidation(); // Act var isValid = editContext.Validate(); @@ -59,7 +62,8 @@ public void ClearsExistingValidationMessagesOnFurtherRuns() { // Arrange var model = new TestModel { IntFrom1To100 = 101 }; - var editContext = new EditContext(model).AddDataAnnotationsValidation(); + var editContext = new EditContext(model); + editContext.EnableDataAnnotationsValidation(); // Act/Assert 1: Initially invalid Assert.False(editContext.Validate()); @@ -75,7 +79,8 @@ public void NotifiesValidationStateChangedAfterObjectValidation() { // Arrange var model = new TestModel { IntFrom1To100 = 101 }; - var editContext = new EditContext(model).AddDataAnnotationsValidation(); + var editContext = new EditContext(model); + editContext.EnableDataAnnotationsValidation(); var onValidationStateChangedCount = 0; editContext.OnValidationStateChanged += (sender, eventArgs) => onValidationStateChangedCount++; @@ -102,7 +107,8 @@ public void PerformsPerPropertyValidationOnFieldChange() // Arrange var model = new TestModel { IntFrom1To100 = 101 }; var independentTopLevelModel = new object(); // To show we can validate things on any model, not just the top-level one - var editContext = new EditContext(independentTopLevelModel).AddDataAnnotationsValidation(); + var editContext = new EditContext(independentTopLevelModel); + editContext.EnableDataAnnotationsValidation(); var onValidationStateChangedCount = 0; var requiredStringIdentifier = new FieldIdentifier(model, nameof(TestModel.RequiredString)); var intFrom1To100Identifier = new FieldIdentifier(model, nameof(TestModel.IntFrom1To100)); @@ -141,7 +147,8 @@ public void PerformsPerPropertyValidationOnFieldChange() public void IgnoresFieldChangesThatDoNotCorrespondToAValidatableProperty(string fieldName) { // Arrange - var editContext = new EditContext(new TestModel()).AddDataAnnotationsValidation(); + var editContext = new EditContext(new TestModel()); + editContext.EnableDataAnnotationsValidation(); var onValidationStateChangedCount = 0; editContext.OnValidationStateChanged += (sender, eventArgs) => onValidationStateChangedCount++; @@ -154,6 +161,24 @@ public void IgnoresFieldChangesThatDoNotCorrespondToAValidatableProperty(string Assert.Equal(1, onValidationStateChangedCount); } + [Fact] + public void CanDetachFromEditContext() + { + // Arrange + var model = new TestModel { IntFrom1To100 = 101 }; + var editContext = new EditContext(model); + var subscription = editContext.EnableDataAnnotationsValidation(); + + // Act/Assert 1: when we're attached + Assert.False(editContext.Validate()); + Assert.NotEmpty(editContext.GetValidationMessages()); + + // Act/Assert 2: when wer're detached + subscription.Dispose(); + Assert.True(editContext.Validate()); + Assert.Empty(editContext.GetValidationMessages()); + } + class TestModel { [Required(ErrorMessage = "RequiredString:required")] public string RequiredString { get; set; } From 0b891f1585872f6ad02acdf5434e798e83561a68 Mon Sep 17 00:00:00 2001 From: Steve Sanderson Date: Thu, 1 Apr 2021 17:16:52 +0100 Subject: [PATCH 06/11] Typo --- .../Forms/test/EditContextDataAnnotationsExtensionsTest.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Components/Forms/test/EditContextDataAnnotationsExtensionsTest.cs b/src/Components/Forms/test/EditContextDataAnnotationsExtensionsTest.cs index 111c87e2529f..80feb0d20a8d 100644 --- a/src/Components/Forms/test/EditContextDataAnnotationsExtensionsTest.cs +++ b/src/Components/Forms/test/EditContextDataAnnotationsExtensionsTest.cs @@ -173,7 +173,7 @@ public void CanDetachFromEditContext() Assert.False(editContext.Validate()); Assert.NotEmpty(editContext.GetValidationMessages()); - // Act/Assert 2: when wer're detached + // Act/Assert 2: when we're detached subscription.Dispose(); Assert.True(editContext.Validate()); Assert.Empty(editContext.GetValidationMessages()); From 304063ca3cbc5476c8566c5347566830a58729d7 Mon Sep 17 00:00:00 2001 From: Steve Sanderson Date: Thu, 1 Apr 2021 17:53:09 +0100 Subject: [PATCH 07/11] Update src/Components/Forms/src/EditContextDataAnnotationsExtensions.cs Co-authored-by: Pranav K --- .../Forms/src/EditContextDataAnnotationsExtensions.cs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/Components/Forms/src/EditContextDataAnnotationsExtensions.cs b/src/Components/Forms/src/EditContextDataAnnotationsExtensions.cs index 451f4fd2ff9d..4e4b43a94b7b 100644 --- a/src/Components/Forms/src/EditContextDataAnnotationsExtensions.cs +++ b/src/Components/Forms/src/EditContextDataAnnotationsExtensions.cs @@ -39,8 +39,7 @@ public static IDisposable EnableDataAnnotationsValidation(this EditContext editC class DataAnnotationsEventSubscriptions : IDisposable { - private static ConcurrentDictionary<(Type ModelType, string FieldName), PropertyInfo?> _propertyInfoCache - = new ConcurrentDictionary<(Type, string), PropertyInfo?>(); + private static readonly ConcurrentDictionary<(Type ModelType, string FieldName), PropertyInfo?> _propertyInfoCache = new(); private readonly EditContext _editContext; private readonly ValidationMessageStore _messages; From 1dfc4a54a638498792a777177bf513f4f9aa7db8 Mon Sep 17 00:00:00 2001 From: Steve Sanderson Date: Thu, 1 Apr 2021 17:53:38 +0100 Subject: [PATCH 08/11] Update src/Components/Forms/src/EditContextDataAnnotationsExtensions.cs Co-authored-by: Pranav K --- .../Forms/src/EditContextDataAnnotationsExtensions.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Components/Forms/src/EditContextDataAnnotationsExtensions.cs b/src/Components/Forms/src/EditContextDataAnnotationsExtensions.cs index 4e4b43a94b7b..aaf2e2ea7528 100644 --- a/src/Components/Forms/src/EditContextDataAnnotationsExtensions.cs +++ b/src/Components/Forms/src/EditContextDataAnnotationsExtensions.cs @@ -37,7 +37,7 @@ public static IDisposable EnableDataAnnotationsValidation(this EditContext editC return new DataAnnotationsEventSubscriptions(editContext); } - class DataAnnotationsEventSubscriptions : IDisposable + private sealed class DataAnnotationsEventSubscriptions : IDisposable { private static readonly ConcurrentDictionary<(Type ModelType, string FieldName), PropertyInfo?> _propertyInfoCache = new(); From 871909546e1600e5fb22b8184e59a3e65f6b552c Mon Sep 17 00:00:00 2001 From: Steve Sanderson Date: Thu, 1 Apr 2021 17:54:47 +0100 Subject: [PATCH 09/11] CR: Remove redundant check --- .../Forms/src/EditContextDataAnnotationsExtensions.cs | 5 ----- 1 file changed, 5 deletions(-) diff --git a/src/Components/Forms/src/EditContextDataAnnotationsExtensions.cs b/src/Components/Forms/src/EditContextDataAnnotationsExtensions.cs index aaf2e2ea7528..258d4cb56ea3 100644 --- a/src/Components/Forms/src/EditContextDataAnnotationsExtensions.cs +++ b/src/Components/Forms/src/EditContextDataAnnotationsExtensions.cs @@ -46,11 +46,6 @@ private sealed class DataAnnotationsEventSubscriptions : IDisposable public DataAnnotationsEventSubscriptions(EditContext editContext) { - if (editContext == null) - { - throw new ArgumentNullException(nameof(editContext)); - } - _editContext = editContext ?? throw new ArgumentNullException(nameof(editContext)); _messages = new ValidationMessageStore(_editContext); From fe0d674103b535df1b3052175cb5d4acea2dfe01 Mon Sep 17 00:00:00 2001 From: Steve Sanderson Date: Thu, 1 Apr 2021 19:00:17 +0100 Subject: [PATCH 10/11] Update warning suppressions --- ...soft.AspNetCore.Components.Forms.WarningSuppressions.xml | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/Components/Forms/src/Microsoft.AspNetCore.Components.Forms.WarningSuppressions.xml b/src/Components/Forms/src/Microsoft.AspNetCore.Components.Forms.WarningSuppressions.xml index a1d67690b415..e19838cbe120 100644 --- a/src/Components/Forms/src/Microsoft.AspNetCore.Components.Forms.WarningSuppressions.xml +++ b/src/Components/Forms/src/Microsoft.AspNetCore.Components.Forms.WarningSuppressions.xml @@ -5,19 +5,19 @@ ILLink IL2026 member - M:Microsoft.AspNetCore.Components.Forms.EditContextDataAnnotationsExtensions.ValidateField(Microsoft.AspNetCore.Components.Forms.EditContext,Microsoft.AspNetCore.Components.Forms.ValidationMessageStore,Microsoft.AspNetCore.Components.Forms.FieldIdentifier@) + M:Microsoft.AspNetCore.Components.Forms.EditContextDataAnnotationsExtensions.DataAnnotationsEventSubscriptions.OnFieldChanged(System.Object,Microsoft.AspNetCore.Components.Forms.FieldChangedEventArgs) ILLink IL2026 member - M:Microsoft.AspNetCore.Components.Forms.EditContextDataAnnotationsExtensions.ValidateModel(Microsoft.AspNetCore.Components.Forms.EditContext,Microsoft.AspNetCore.Components.Forms.ValidationMessageStore) + M:Microsoft.AspNetCore.Components.Forms.EditContextDataAnnotationsExtensions.DataAnnotationsEventSubscriptions.OnValidationRequested(System.Object,Microsoft.AspNetCore.Components.Forms.ValidationRequestedEventArgs) ILLink IL2080 member - M:Microsoft.AspNetCore.Components.Forms.EditContextDataAnnotationsExtensions.TryGetValidatableProperty(Microsoft.AspNetCore.Components.Forms.FieldIdentifier@,System.Reflection.PropertyInfo@) + M:Microsoft.AspNetCore.Components.Forms.EditContextDataAnnotationsExtensions.DataAnnotationsEventSubscriptions.TryGetValidatableProperty(Microsoft.AspNetCore.Components.Forms.FieldIdentifier@,System.Reflection.PropertyInfo@) \ No newline at end of file From c987b907337d8b02983d8cf05360dd669948e27a Mon Sep 17 00:00:00 2001 From: Steve Sanderson Date: Thu, 1 Apr 2021 19:00:45 +0100 Subject: [PATCH 11/11] CR: Add extra failure case --- .../Forms/src/DataAnnotationsValidator.cs | 14 ++++++++++++++ src/Components/Forms/src/PublicAPI.Unshipped.txt | 1 + 2 files changed, 15 insertions(+) diff --git a/src/Components/Forms/src/DataAnnotationsValidator.cs b/src/Components/Forms/src/DataAnnotationsValidator.cs index e08ff2332157..583c5886200d 100644 --- a/src/Components/Forms/src/DataAnnotationsValidator.cs +++ b/src/Components/Forms/src/DataAnnotationsValidator.cs @@ -11,6 +11,7 @@ namespace Microsoft.AspNetCore.Components.Forms public class DataAnnotationsValidator : ComponentBase, IDisposable { private IDisposable? _subscriptions; + private EditContext? _originalEditContext; [CascadingParameter] EditContext? CurrentEditContext { get; set; } @@ -25,6 +26,19 @@ protected override void OnInitialized() } _subscriptions = CurrentEditContext.EnableDataAnnotationsValidation(); + _originalEditContext = CurrentEditContext; + } + + /// + protected override void OnParametersSet() + { + if (CurrentEditContext != _originalEditContext) + { + // While we could support this, there's no known use case presently. Since InputBase doesn't support it, + // it's more understandable to have the same restriction. + throw new InvalidOperationException($"{GetType()} does not support changing the " + + $"{nameof(EditContext)} dynamically."); + } } /// diff --git a/src/Components/Forms/src/PublicAPI.Unshipped.txt b/src/Components/Forms/src/PublicAPI.Unshipped.txt index a412262cbbf0..31beb9c94faf 100644 --- a/src/Components/Forms/src/PublicAPI.Unshipped.txt +++ b/src/Components/Forms/src/PublicAPI.Unshipped.txt @@ -1,3 +1,4 @@ #nullable enable +override Microsoft.AspNetCore.Components.Forms.DataAnnotationsValidator.OnParametersSet() -> void static Microsoft.AspNetCore.Components.Forms.EditContextDataAnnotationsExtensions.EnableDataAnnotationsValidation(this Microsoft.AspNetCore.Components.Forms.EditContext! editContext) -> System.IDisposable! virtual Microsoft.AspNetCore.Components.Forms.DataAnnotationsValidator.Dispose(bool disposing) -> void