Validation fixes for Blazor #14972

pranavkm · 2019-10-13T22:14:26Z

Ensure validation result that are not associated with a member are recorded. Fixes CompareAttribute ignored with OnValidSubmit EditForm #10643
Add support for showing model-specific errors to ValidationSummary
Add support for nested validation and a more suitable CompareAttribute. Fixes Validation doesn't automatically traverse members #10526

eng/ProjectReferences.props

javiercn · 2019-10-15T13:30:37Z

eng/ProjectReferences.props

@@ -137,6 +137,7 @@
    <ProjectReferenceProvider Include="Microsoft.AspNetCore.Blazor" ProjectPath="$(RepoRoot)src\Components\Blazor\Blazor\src\Microsoft.AspNetCore.Blazor.csproj" RefProjectPath="$(RepoRoot)src\Components\Blazor\Blazor\ref\Microsoft.AspNetCore.Blazor.csproj" />
    <ProjectReferenceProvider Include="Microsoft.AspNetCore.Blazor.HttpClient" ProjectPath="$(RepoRoot)src\Components\Blazor\Http\src\Microsoft.AspNetCore.Blazor.HttpClient.csproj" RefProjectPath="$(RepoRoot)src\Components\Blazor\Http\ref\Microsoft.AspNetCore.Blazor.HttpClient.csproj" />
    <ProjectReferenceProvider Include="Microsoft.AspNetCore.Blazor.Server" ProjectPath="$(RepoRoot)src\Components\Blazor\Server\src\Microsoft.AspNetCore.Blazor.Server.csproj" RefProjectPath="$(RepoRoot)src\Components\Blazor\Server\ref\Microsoft.AspNetCore.Blazor.Server.csproj" />
+    <ProjectReferenceProvider Include="Microsoft.AspNetCore.Blazor.DataAnnotations.Validation" ProjectPath="$(RepoRoot)src\Components\Blazor\Validation\src\Microsoft.AspNetCore.Blazor.DataAnnotations.Validation.csproj" />


Is this a new package we are RTMing in 3.1?

This would be an experimental package in the same vein as HttpClient. The plan is to leave this as a preview \ experimental package in 3.x and collaborate with CoreFx to figure out solutions for 5.0.

javiercn · 2019-10-15T13:34:18Z

src/Components/Blazor/Validation/src/BlazorCompareAttribute.cs

+    /// <summary>
+    /// A <see cref="ValidationAttribute"/> that compares two properties
+    /// </summary>
+    public sealed class BlazorCompareAttribute : CompareAttribute


Is this because the original CompareAttribute doesn't include the member name?

If this is something that users are expected to use in their models, can we find a better name for this? BlazorCompareAttribute is highly confusing.

Is this because the original CompareAttribute doesn't include the member name?

Yup. I'm open to suggestions. I kinda went with a whimsical name to indicate this is meant to go away at some point.

javiercn · 2019-10-16T12:26:58Z

src/Components/Blazor/Validation/src/BlazorDataAnnotationsValidator.cs

+
+namespace Microsoft.AspNetCore.Components.Forms
+{
+    public class BlazorDataAnnotationsValidator : ComponentBase


Is this meant to appear in the user markup?

I would try to pick a better name that makes it clear what it does, even if its a bit longer than the current identifier. Some options are:

GraphDataAnnotationsValidator

ObjectGraphDataAnnotationsValidator

RecursiveDataAnnotationsValidator

In essence, something that clearly states what this does.

I wanted to avoid Recursive because we don't recurse in to the model like MVC does. We only navigate the object graph if you encounter the attribute. The other two sound neat, I'll run it by @danroth27

javiercn · 2019-10-16T12:34:37Z

src/Components/Blazor/Validation/src/BlazorDataAnnotationsValidator.cs

+                ValidateField(EditContext, _validationMessageStore, eventArgs.FieldIdentifier);
+        }
+
+        internal void ValidateObject(object value)


Don't you have to keep track of the objects you've already inspected to prevent potential infinite recursion issues?

Yeah, you're right. Unlike MVC, the user determines what objects the validator needs to traverse, so it's much more difficult to get in to a recursion. However, you could have a trivial model like this and get in to to recursion with no other way to represent it:

public class Person { [Required] public string Name { get; set; } [ValidateComplexType] public Person Spouse { get; set; } }

src/Components/test/E2ETest/Tests/FormsTestWithExperimentalValiator.cs

src/Components/test/testassets/BasicTestApp/FormsTest/ExperimentalValidationComponent.razor

javiercn · 2019-10-16T13:00:14Z

src/Components/test/testassets/BasicTestApp/Index.razor

        <option value="BasicTestApp.FormsTest.TypicalValidationComponent">Typical validation</option>
+        <option value="BasicTestApp.FormsTest.TypicalValidationComponentUsingExperimentalValidator">Typical validation using experimental validator</option>
+        <option value="BasicTestApp.FormsTest.ExperimentalValidationComponent">Experimental validation</option>
        <option value="BasicTestApp.GlobalizationBindCases">Globalization Bind Cases</option>


Wouldn't it be better to duplicate the tests for the experimental validator completely instead of doing if...else in some of the infrastructure if the goal here is that for 5.0 this becomes the de-facto framework behavior? That way then its a matter of removing code.

I thought it'd be less work than maintaining two copies of these. Do you feel strongly about this?

javiercn

Overall looks good to me, there are some small details we might need to polish, but this is a good start.

javiercn · 2019-10-16T20:16:55Z

src/Components/Blazor/Validation/src/ComparePropertyAttribute.cs

+    /// A <see cref="ValidationAttribute"/> that compares two properties
+    /// </summary>
+    [AttributeUsage(AttributeTargets.Property, AllowMultiple = false)]
+    public sealed class ComparePropertyAttribute : CompareAttribute


This class should be in the System.ComponentModel.DataAnnotations project, not Blazor.

To add this validation to a property in MyApp.Shared means the shared app has to reference the Blazor assemblies - which isn't good for a contracts assembly that could be used in different types of client apps.

pranavkm · 2019-10-16T20:47:57Z

@dougbu I removed the ProjectReferences.props file, but it causes code check to fail:

##[error]eng\ProjectReferences.props(,): error : Generated code is not up to date in eng/ProjectReferences.props. You might need to regenerate the reference assemblies or project list (see docs/ReferenceAssemblies.md and docs/ReferenceResolution.md)
error : Generated code is not up to date in eng/ProjectReferences.props. You might need to regenerate the reference assemblies or project list (see docs/ReferenceAssemblies.md and docs/ReferenceResolution.md) [F:\workspace.1\_work\1\s\eng\ProjectReferences.props]
diff --git a/eng/ProjectReferences.props b/eng/ProjectReferences.props
index 16f0ef714a..37704e933e 100644
--- a/eng/ProjectReferences.props
+++ b/eng/ProjectReferences.props
@@ -137,7 +137,7 @@
     <ProjectReferenceProvider Include="Microsoft.AspNetCore.Blazor" ProjectPath="$(RepoRoot)src\Components\Blazor\Blazor\src\Microsoft.AspNetCore.Blazor.csproj" RefProjectPath="$(RepoRoot)src\Components\Blazor\Blazor\ref\Microsoft.AspNetCore.Blazor.csproj" />
     <ProjectReferenceProvider Include="Microsoft.AspNetCore.Blazor.HttpClient" ProjectPath="$(RepoRoot)src\Components\Blazor\Http\src\Microsoft.AspNetCore.Blazor.HttpClient.csproj" RefProjectPath="$(RepoRoot)src\Components\Blazor\Http\ref\Microsoft.AspNetCore.Blazor.HttpClient.csproj" />
     <ProjectReferenceProvider Include="Microsoft.AspNetCore.Blazor.Server" ProjectPath="$(RepoRoot)src\Components\Blazor\Server\src\Microsoft.AspNetCore.Blazor.Server.csproj" RefProjectPath="$(RepoRoot)src\Components\Blazor\Server\ref\Microsoft.AspNetCore.Blazor.Server.csproj" />
-    <ProjectReferenceProvider Include="Microsoft.AspNetCore.Blazor.DataAnnotations.Validation" ProjectPath="$(RepoRoot)src\Components\Blazor\Validation\src\Microsoft.AspNetCore.Blazor.DataAnnotations.Validation.csproj" />
+    <ProjectReferenceProvider Include="Microsoft.AspNetCore.Blazor.DataAnnotations.Validation" ProjectPath="$(RepoRoot)src\Components\Blazor\Validation\src\Microsoft.AspNetCore.Blazor.DataAnnotations.Validation.csproj" RefProjectPath="$(RepoRoot)src\Components\Blazor\Validation\ref\Microsoft.AspNetCore.Blazor.DataAnnotations.Validation.csproj" />
     <ProjectReferenceProvider Include="Microsoft.AspNetCore.Components" ProjectPath="$(RepoRoot)src\Components\Components\src\Microsoft.AspNetCore.Components.csproj" RefProjectPath="$(RepoRoot)src\Components\Components\ref\Microsoft.AspNetCore.Components.csproj" />

dougbu · 2019-10-17T01:50:04Z

I removed the ProjectReferences.props file, but it causes code check to fail

This is expected because you didn't revert the change to the src/ project stating that it has an associated ref/ project. Get rid of all ref/-related changes unless a ref/ assembly is mandatory e.g. because the assembly will now ship in SharedFx.

SteveSandersonMS · 2019-10-17T04:25:41Z

Ensure validation result that are not associated with a member are recorded. Fixes #10643
Add support for showing model-specific errors to ValidationSummary

These parts of the PR seem totally clear and look perfectly good to proceed with as-is.

Add support for nested validation and a more suitable CompareAttribute. Fixes #10526

For this part, I'd be interested to discuss the design. The main thing for me is I'm surprised we want to add an "experimental" validator that people have to decide to use or not. What's the reason why we can't bake the support for a [ValidateComplexObject] attribute directly into our existing DataAnnotationsValidator?

The idea of having a .Blazor-branded package for this seems pretty odd. This functionality equally makes sense for both Blazor Server and Blazor WebAssembly, and our normal naming for such things is .Components.

Could you clarify what level of consideration was given to baking this feature directly into 3.1? I think it would be really neat and clean if we could do that instead.

a more suitable CompareAttribute

I agree that if we are shipping an unsupported "experimental validation" package then it's totally fine to put this in here. But as per my comment above, I'm still wondering if we could put your ValidateComplexObject feature directly into Microsoft.AspNetCore.Components.Forms. If we did that there's less of a reason to ship any new "experimental validation" package. So, for "compare", we could reduce this to be just a sample or something that appears in docs. It's not a lot of code.

pranavkm · 2019-10-17T13:39:09Z

For this part, I'd be interested to discuss the design. The main thing for me is I'm surprised we want to add an "experimental" validator that people have to decide to use or not. What's the reason why we can't bake the support for a [ValidateComplexObject] attribute directly into our existing DataAnnotationsValidator?

My plan is to work with CoreFx and move these types - ValidateComplexObjectAttribute, the more sensible ComparePropertyAttribute, and the understanding of these in to System.ComponentModel.DataAnnotations.Validator in the 5.0 time frame. That should allow us to remove this package in entirety. Putting the types in the a shipping package \ shared fx makes that much more difficult.

javiercn · 2019-10-17T19:08:09Z

My plan is to work with CoreFx and move these types - ValidateComplexObjectAttribute, the more sensible ComparePropertyAttribute, and the understanding of these in to System.ComponentModel.DataAnnotations.Validator in the 5.0 time frame. That should allow us to remove this package in entirety. Putting the types in the a shipping package \ shared fx makes that much more difficult.

This sounds like a good plan. If you ask me, I'd rather have these in core fx than have us own them and maintain them.

If we change our minds, we can do the move in preview3 or after 3.1

javiercn · 2019-10-17T19:15:50Z

src/Components/Blazor/Validation/src/ObjectGraphDataAnnotationsValidator.cs

+        }
+
+        internal void ValidateObject(object value, HashSet<object> visited)
+        {


Do we have a test for this that validates object graphs with cycles? (I haven't been able to find it).

I added a couple here: https://github.com/aspnet/AspNetCore/pull/14972/files#diff-a3fb926db31109f966f332738bc6f29bR309-R426

javiercn

Looks good to me.

I'd rather keep this as an experimental package for preview2 and then figure out how to merge it.

* Ensure validation result that are not associated with a member are recorded. Fixes #10643 * Add support for showing model-specific errors to ValidationSummary * Add support for nested validation and a more suitable CompareAttribute. Fixes #10526

eng/ProjectReferences.props

mrpmorris · 2019-10-25T08:00:35Z

src/Components/Blazor/Validation/src/ObjectGraphDataAnnotationsValidator.cs

+                return;
+            }
+
+            var validationResults = new List<ValidationResult>();


You need to add the following code....

if (value.GetType().Assembly == typeof(object).Assembly) return;

Otherwise you will validate every string in a string[], and then every character of every string.

mrpmorris · 2019-10-25T08:13:08Z

@danroth27 @SteveSandersonMS @rynowak

I don't think this approach is ideal.

It solves the problem of nested objects not being validated, however, if you are writing something like a wizard UI then you will want to only validate the current UI. This PR approach results in properties being validated that cannot be corrected by the user, or forces the developer to use DataAnnotationsValidator which has the problem of not being able to validate nested objects referenced by inputs.

I suggested a potential solution here
#14426 (comment)

This proposal would not only solve the nested-validation problem, but would solve #14426 too (validated fields based on the UI rather than the object).

Please do not go with the approach in this PR. It adds an alternative inflexible solution, resulting in two inflexible solutions where neither of them are suitable in all use cases, and neither of them are suitable for Wizard UIs.

DataAnnotations: Cannot validate x=> x.Address.PostalCode
ObjectGraphDataAnnotationsValidator: Cannot avoid validating properties that are in the next tab of a wizard UI.
My proposal: Always validates properties in the UI no matter how deeply they are nested (fixes the DataAnnotationsValidator problem), optionally allows the developer to specify that they want to do full object graph validation (enables the validation approach in this PR).

If we go with this approach I think we are going to see questions about how to validate wizard UI appearing repeatedly - and the suggestion to write a class per wizard step will be frowned upon because

1: It's more work to write classes for your UI than it is to just use the object that is sent to the Server.
2: The Server should not have to split up classes to appease the user interface.

Please take another look at my suggestion, I gave consideration to a solution that works in all situations. Even if you don't use my approach verbatim, it might give you an alternative idea. Either way, the approach in this PR is going to cause problems. Wizard UIs are very common, and Blazor will not support their development without pain to the developer.

benm-eras · 2020-06-05T12:21:03Z

Fogive me if I am asking in the wrong place, but I need to validate nested objects and can't find anything in the official docs, and all threads I find here have been closed or state that the work was completed in 3.1. I have 3.1 and don't seem to be able to validate nested object properties.

pranavkm · 2020-06-05T13:50:06Z

@benm-eras it's easy for us to miss comments on closed issues \ PRs. I'd recommend filing a new issue the next time around. Docs for validation are available here: https://docs.microsoft.com/en-us/aspnet/core/blazor/forms-validation?view=aspnetcore-3.1#nested-models-collection-types-and-complex-types

mrpmorris · 2020-06-05T19:17:25Z

@benm-eras my library will validate a tree, specific properties, and even integrate with FluentValidation.

https://github.com/mrpmorris/blazor-validation/blob/master/README.md

pranavkm requested review from SteveSandersonMS and a team as code owners October 13, 2019 22:14

dougbu requested changes Oct 13, 2019

View reviewed changes

eng/ProjectReferences.props Outdated Show resolved Hide resolved

pranavkm force-pushed the prkrishn/deep-validation branch from 9365a69 to 04d1ebe Compare October 14, 2019 19:21

pranavkm requested a review from javiercn October 14, 2019 20:32

javiercn reviewed Oct 15, 2019

View reviewed changes

javiercn reviewed Oct 16, 2019

View reviewed changes

src/Components/test/E2ETest/Tests/FormsTestWithExperimentalValiator.cs Outdated Show resolved Hide resolved

javiercn reviewed Oct 16, 2019

View reviewed changes

src/Components/test/testassets/BasicTestApp/FormsTest/ExperimentalValidationComponent.razor Show resolved Hide resolved

javiercn reviewed Oct 16, 2019

View reviewed changes

javiercn added the area-blazor Includes: Blazor, Razor Components label Oct 16, 2019

pranavkm force-pushed the prkrishn/deep-validation branch 3 times, most recently from b9c1d01 to 3a2a956 Compare October 16, 2019 20:15

javiercn reviewed Oct 16, 2019

View reviewed changes

javiercn reviewed Oct 17, 2019

View reviewed changes

javiercn approved these changes Oct 17, 2019

View reviewed changes

pranavkm added 5 commits October 17, 2019 16:38

Validation fixes for Blazor

0c3f759

* Ensure validation result that are not associated with a member are recorded. Fixes #10643 * Add support for showing model-specific errors to ValidationSummary * Add support for nested validation and a more suitable CompareAttribute. Fixes #10526

fixup

75a8633

Update ref asm

75fde42

Changes per PR

44de036

Fixup projectreferences.props

e3ea4cb

pranavkm added 2 commits October 17, 2019 16:38

sigh

d780aac

Fixup

648b559

pranavkm force-pushed the prkrishn/deep-validation branch from 88e7b7c to 648b559 Compare October 17, 2019 23:44

pranavkm added 2 commits October 18, 2019 05:58

Update ProjectReferences.props

58e5613

Update ProjectReferences.props

b89535c

pranavkm commented Oct 18, 2019

View reviewed changes

eng/ProjectReferences.props Show resolved Hide resolved

pranavkm merged commit c298c94 into release/3.1 Oct 18, 2019

pranavkm deleted the prkrishn/deep-validation branch October 18, 2019 18:26

mrpmorris reviewed Oct 25, 2019

View reviewed changes

mrpmorris mentioned this pull request Nov 5, 2019

Ensure it's possible to validate based on the fields in the UI #14426

Open

boukenka mentioned this pull request Jan 7, 2020

No boilerplate option for the Blazor templates #18137

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Validation fixes for Blazor #14972

Validation fixes for Blazor #14972

pranavkm commented Oct 13, 2019

javiercn Oct 15, 2019

pranavkm Oct 15, 2019

javiercn Oct 15, 2019

pranavkm Oct 15, 2019

javiercn Oct 16, 2019

pranavkm Oct 16, 2019

javiercn Oct 16, 2019

pranavkm Oct 16, 2019

javiercn Oct 16, 2019

pranavkm Oct 16, 2019

javiercn left a comment

javiercn Oct 16, 2019

mrpmorris Oct 25, 2019

pranavkm commented Oct 16, 2019

dougbu commented Oct 17, 2019

SteveSandersonMS commented Oct 17, 2019

pranavkm commented Oct 17, 2019

javiercn commented Oct 17, 2019 •

edited

Loading

javiercn Oct 17, 2019

pranavkm Oct 17, 2019

javiercn left a comment

mrpmorris Oct 25, 2019

mrpmorris commented Oct 25, 2019 •

edited

Loading

benm-eras commented Jun 5, 2020

pranavkm commented Jun 5, 2020

mrpmorris commented Jun 5, 2020

Validation fixes for Blazor #14972

Validation fixes for Blazor #14972

Conversation

pranavkm commented Oct 13, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

javiercn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pranavkm commented Oct 16, 2019

dougbu commented Oct 17, 2019

SteveSandersonMS commented Oct 17, 2019

pranavkm commented Oct 17, 2019

javiercn commented Oct 17, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

javiercn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mrpmorris commented Oct 25, 2019 • edited Loading

benm-eras commented Jun 5, 2020

pranavkm commented Jun 5, 2020

mrpmorris commented Jun 5, 2020

javiercn commented Oct 17, 2019 •

edited

Loading

mrpmorris commented Oct 25, 2019 •

edited

Loading