Add DataAnnotations based validation #272

HaoK · 2018-08-07T21:29:19Z

Add DataAnnotations validator support: (dotnet/aspnetcore#3385)

Basic support works pretty much as expected:

        [AttributeUsage(AttributeTargets.Field | AttributeTargets.Property, AllowMultiple = false, Inherited = true)]
        public class FromAttribute : ValidationAttribute
        {
            public string Accepted { get; set; }

            public override bool IsValid(object value)
                => value == null || value.ToString() == Accepted;
        }

        private class AnnotatedOptions
        {
            [Required]
            public string Required { get; set; }

            [StringLength(5, ErrorMessage = "Too long.")]
            public string StringLength { get; set; }

            [Range(-5, 5, ErrorMessage = "Out of range.")]
            public int IntRange { get; set; }

            [From(Accepted = "USA")]
            public string Custom { get; set; }
        }

        [Fact]
        public void CanValidateDataAnnotations()
        {
            var services = new ServiceCollection();
            services.AddOptions<AnnotatedOptions>()
                .Configure(o =>
                {
                    o.StringLength = "111111";
                    o.IntRange = 10;
                    o.Custom = "nowhere";
                })
                .ValidateDataAnnotations();

            var sp = services.BuildServiceProvider();

            var error = Assert.Throws<OptionsValidationException>(() => sp.GetRequiredService<IOptions<AnnotatedOptions>>().Value);
            ValidateFailure<AnnotatedOptions>(error, Options.DefaultName, "The Required field is required. : Too long. : Out of range. : The field Custom is invalid.");
        }

Basically you get a single error message with all of the data validation errors smushed together, since its considered a single options validator.

Thoughts? @ajcvickers @davidfowl @Eilon @DamianEdwards

HaoK · 2018-08-07T21:31:47Z

We can split this out into a separate Options.DataAnnotations package if we still care about keeping dependencies of the core options package lean...

ajcvickers · 2018-08-08T00:16:59Z

Code seems good, but not sure about taking a dependency on DataAnnotations--we probably shouldn't do it now unless we want to keep it in 3.0. But if other things already have the dependency and will continue to have it in 3.0, then their probably won't be a problem.

davidfowl · 2018-08-08T03:42:59Z

No not in core options. This should be another package reference unlesss we make it part of net standard

DamianEdwards · 2018-08-08T03:45:36Z

+1 for new package layer

HaoK · 2018-08-16T18:26:43Z

Microsoft.Extensions.Options.DataAnnotations for the new package name?

Eilon

Code looks pretty straightforward and simple. Just some doc comment comments!

Eilon · 2018-08-21T20:41:25Z

src/Microsoft.Extensions.Options.DataAnnotations/DataAnnotationValidateOptions.cs

+namespace Microsoft.Extensions.Options
+{
+    /// <summary>
+    /// Implementation of <see cref="IValidateOptions{TOptions}"/>


Can maybe elaborate on this a bit more? This comment is just stating what can be seen from the type declaration anyway.

Eilon · 2018-08-21T20:41:31Z

src/Microsoft.Extensions.Options.DataAnnotations/DataAnnotationValidateOptions.cs

+    /// <typeparam name="TOptions">The instance being validated.</typeparam>
+    public class DataAnnotationValidateOptions<TOptions> : IValidateOptions<TOptions> where TOptions : class
+    {
+        public DataAnnotationValidateOptions(string name)


Need doc comments on all public APIs.

Eilon · 2018-08-21T20:42:41Z

src/Microsoft.Extensions.Options.DataAnnotations/OptionsBuilderDataAnnotationsExtensions.cs

+    public static class OptionsBuilderDataAnnotationsExtensions
+    {
+        /// <summary>
+        /// Register this options instance for the built in DataAnnotationsAttribute


The term DataAnnotationsAttribute looks like it's being used as a type name, but it isn't a type. Rephrase somehow?

HaoK · 2018-08-24T21:25:54Z

Fixed new doc comments and removed the doc comment excludes so added all the missing doc comments as well

Eilon · 2018-08-27T16:26:22Z

src/Microsoft.Extensions.Options.DataAnnotations/DataAnnotationValidateOptions.cs

+                }
+
+
+                var errors = String.Join(" : ", validationResults.Select(r => r.ErrorMessage));


This is a bit icky. I'm guessing there's no "list of errors" that we can send a list of these failures to? How about put one on each line instead?

"Validation of the object XYZ failed: {reason 1} {reason 2} {reason 3}"

I think it would be also useful here to use ValidationResult.MemberNames and format that in some way so that the user knows which property(s) failed.

Okay I'll try to clean this up, yeah the data annotation validations are treated as a single validaiton failure along side an arbitrary amount of other validators, which is why it gets turned into a single string, since it potentially would need to be aggregated with other non data-annotation based validation errors.

Yeah I think at least putting 1 per line would be a good separation.

Specifically only for data annotations errors? or all of the aggregated errors as well? I'll add a new test that demonstrates what it looks like so its easier to review in the PR

Yeah let's see it in the test then I'll review.

Eilon · 2018-08-27T16:28:10Z

....Options.ConfigurationExtensions/Microsoft.Extensions.Options.ConfigurationExtensions.csproj

@@ -3,7 +3,7 @@
  <PropertyGroup>
    <Description>Provides additional configuration specific functionality related to Options.</Description>
    <TargetFramework>netstandard2.0</TargetFramework>
-    <NoWarn>$(NoWarn);CS1591</NoWarn>


YES! Another project finally has full docs!

HaoK · 2018-08-27T20:35:14Z

Updated the new errors are more wordy now but hopefully more clear:

"DataAnnotation validation failed for members {Required} with error 'The Required field is required.'.
DataAnnotation validation failed for members {StringLength} with error 'Too long.'.
DataAnnotation validation failed for members {IntRange} with error 'Out of range.'.
DataAnnotation validation failed for members {Custom} with error 'The field Custom is invalid.'."

Eilon

Just a few small comments, then looks good to me.

Eilon · 2018-08-28T17:28:05Z

src/Microsoft.Extensions.Options.DataAnnotations/DataAnnotationValidateOptions.cs

@@ -49,7 +49,10 @@ public ValidateOptionsResult Validate(string name, TOptions options)
                }


-                var errors = String.Join(" : ", validationResults.Select(r => r.ErrorMessage));
+                var errors = String.Join(Environment.NewLine, 
+                    validationResults.Select(r => "DataAnnotation validation failed for members {" + 


Maybe get rid of the {? A bit too nerdy 😄

Eilon · 2018-08-28T17:28:23Z

src/Microsoft.Extensions.Options.DataAnnotations/DataAnnotationValidateOptions.cs

+                var errors = String.Join(Environment.NewLine, 
+                    validationResults.Select(r => "DataAnnotation validation failed for members {" + 
+                    String.Join(",", r.MemberNames) + 
+                    "} with error '" + r.ErrorMessage + "'."));


with *the* error

Eilon · 2018-08-28T17:29:03Z

test/Microsoft.Extensions.Options.Test/OptionsBuilderTest.cs

+DataAnnotation validation failed for members {IntRange} with error 'Out of range.'.
+DataAnnotation validation failed for members {Custom} with error 'The field Custom is invalid.'.",
+                "I don't want to go to nowhere!");
+        }


Could we add a case where there is more than 1 member as well? Maybe CompareValidator does that? Or write a dummy custom validator that does that?

HaoK · 2018-08-28T19:56:29Z

Just as FYI, multi member error looks like:

DataAnnotation validation failed for members Dep1,Dep2 with the error 'Dep1 != Dep2'.

Eilon

OK literally just asking for 1 more space, then good to go as far as I'm concerned. THanks!

Eilon · 2018-08-28T22:27:40Z

src/Microsoft.Extensions.Options.DataAnnotations/DataAnnotationValidateOptions.cs

-                return ValidateOptionsResult.Fail(errors);
+                return ValidateOptionsResult.Fail(String.Join(Environment.NewLine,
+                    validationResults.Select(r => "DataAnnotation validation failed for members " +
+                        String.Join(",", r.MemberNames) +


OK last nit, a space after the comma 😄

Add DataAnnotations based validation

be3630d

HaoK added this to the 2.0.0-preview2 milestone Aug 7, 2018

Move data annotations into its own package

991099d

Eilon suggested changes Aug 21, 2018

View reviewed changes

HaoK added 2 commits August 24, 2018 12:42

Update doc comments

b7bccab

CR tweaks

44bf9e5

Eilon suggested changes Aug 27, 2018

View reviewed changes

Tweak data annotation failure message

80c3dba

Cleanup baseline

7b41ce5

Eilon approved these changes Aug 28, 2018

View reviewed changes

Add multi field validator test

4f7c044

Eilon approved these changes Aug 28, 2018

View reviewed changes

Space

48aab0c

HaoK merged commit dfcde1b into release/2.2 Aug 28, 2018

HaoK deleted the haok/data branch August 28, 2018 22:46

HaoK mentioned this pull request Aug 28, 2018

Options Validation: Support DataAnnotations dotnet/aspnetcore#3385

Closed

		}


		var errors = String.Join(" : ", validationResults.Select(r => r.ErrorMessage));

Add DataAnnotations based validation #272

Add DataAnnotations based validation #272

Uh oh!

Conversation

HaoK commented Aug 7, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

HaoK commented Aug 7, 2018

Uh oh!

ajcvickers commented Aug 8, 2018

Uh oh!

davidfowl commented Aug 8, 2018

Uh oh!

DamianEdwards commented Aug 8, 2018

Uh oh!

HaoK commented Aug 16, 2018

Uh oh!

Eilon left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

HaoK commented Aug 24, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

HaoK Aug 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

HaoK commented Aug 27, 2018

Uh oh!

Eilon left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

HaoK commented Aug 28, 2018

Uh oh!

Eilon left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

HaoK commented Aug 7, 2018 •

edited

Loading

HaoK Aug 27, 2018 •

edited

Loading