Add `[ValidateNever]` and `IPropertyValidationFilter` #5676

dougbu · 2017-01-11T22:58:19Z

Need better way to suppress validation of properties/types during model binding #5642
lazy-load ValidationEntry.Model
- avoids Exceptions when moving to a property that will not be validated

nits:

remove duplicate code in ValidationVisitor
clarify "all properties of" doc comments
- also add missing <param> doc in ViewDataInfo

was:
~~See individual commits for details~~

~~first and third commits are about nits hit while doing this work~~
~~other early commits require users to subclass DefaultObjectValidator and ValidationVisitor~~
~~required a breaking change to the ValidationVisitor constructor because that's effectively internal now~~
~~can go back to that if we decide current approach has downsides~~

dougbu · 2017-01-11T22:59:37Z

src/Microsoft.AspNetCore.Mvc.Core/Internal/DefaultValidationMetadataProvider.cs

+                var shouldValidate = context.PropertyAttributes.OfType<IShouldValidate>().FirstOrDefault();
+                if (shouldValidate == null)
+                {
+                    // No [ValidateNever] on the property. Check if container has this attribute.


I'll update this comment

dougbu · 2017-01-12T00:37:48Z

src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/Validation/ValidateNeverAttribute.cs

+    /// <summary>
+    /// <see cref="IShouldValidate"/> implementation that unconditionally indicates a property should be excluded from
+    /// validation. When applied to a property, the validation system excludes that property. When applied to a type,
+    /// the validation system excludes all properties within that type.


When applied to a type, system behaves much the same as when an SuppressChildValidationMetadataProvider exists for the same type. Should probably recommend [ValidateNever] for types users own and SuppressChildValidationMetadataProvider for types they don't. For example, MVC doesn't use SuppressChildValidationMetadataProvider any place [ValidateNever] could be used.

That seems like good guidance to put in the docs!

Do you mean instead of or in addition to here?

rynowak

Both approaches shown here seem pretty reasonable.

I think we might need a hybrid - IE provide an attribute like [ValidateNever] but also an abstraction like IShouldValidate. The requests that we've gotten around "optional" properties fall into the latter, whereas simpler usage would lead to [ValidateNever].

rynowak · 2017-01-12T18:50:43Z

src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/Validation/ValidatorCache.cs

 {
+    /// <summary>
+    /// Provides and caches <see cref="IModelValidator"/>s for <see cref="ModelMetadata"/>.
+    /// </summary>
    public class ValidatorCache


We should probably clean this up a bit more before going public with it.

I'd do a few things to it:

Rename it to ValidatorFactory and make it abstract - this lets us hide the constructor

Make it live in DI

Make it the 'owner' of the the composite IValidatorProvider

This would then be similar in design to a few other things we already have in the system

Outdated comment. No longer make ValidatorCache public.

rynowak · 2017-01-12T18:50:53Z

src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/Validation/ValidationVisitor.cs

+        protected virtual bool ShouldValidateEntry(ValidationEntry entry, ValidationEntry parentEntry)
+        {
+            return true;
+        }


Outdated comment. No longer have (or need) this ValidationVisitor extensibility point. But, I'll assume you love the IShouldValidate interface 😺

rynowak · 2017-01-12T18:51:16Z

src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/Validation/DefaultObjectValidator.cs

+                MetadataProvider,
+                validationState);
+        }
+    }


I think this works out pretty well

rynowak · 2017-01-12T18:51:44Z

src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/Validation/ValidatorCache.cs

+        /// <returns>List of <see cref="IModelValidator"/>s for <paramref name="metadata"/>.</returns>
+        public IReadOnlyList<IModelValidator> GetValidators(
+            ModelMetadata metadata,
+            IModelValidatorProvider validatorProvider)


Is another validator provider besides the composite ever used here?

Nope. But, this isn't that relevant because the PR no longer makes ValidatorCache public.

rynowak · 2017-01-12T18:53:01Z

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

+        /// Gets a value that indicates whether this model should be validated.
+        /// </summary>
+        /// <value>Defaults to <c>true</c>.</value>
+        public abstract bool Validate { get; }


FYI this is a breaking change. To do it in a non-breaking way we'd make it virtual non-abstract

👍 Good catch and it applies equally to the new ShouldValidate property addition. I'll file a bug on the API Checker that it misses this. I'll file the same bug on my brain but won't track that in GitHub.

rynowak · 2017-01-12T18:54:34Z

src/Microsoft.AspNetCore.Mvc.Core/Internal/DefaultValidationMetadataProvider.cs

@@ -34,6 +36,26 @@ public void CreateValidationMetadata(ValidationMetadataProviderContext context)
                    }
                }
            }
+
+            // [ValidateNever] on a type affects properties in that type, not properties that have that type. Thus,
+            // we ignore context.TypeAttributes for properties and don't check at all for types.


This is the opposite of what I'd imagine... is that consistent with BindNever and friends?

Yes, this is consistent with [BindNever] and friends. I copied the binding provider code from there. And, this is exactly the area where the [BindNever] docs were ambiguous.

rynowak · 2017-01-12T18:56:25Z

src/Microsoft.AspNetCore.Mvc.Core/Internal/DefaultValidationMetadataProvider.cs

+                {
+                    // No [ValidateNever] on the property. Check if container has this attribute.
+                    validateNever = context.Key.ContainerType.GetTypeInfo()
+                        .GetCustomAttributes(typeof(ValidateNeverAttribute), inherit: true)


would be better to use GetCustomAttribute (singular) if the attribute has AllowMultiple = false

OR the alternative would be to introduce an interface here that ValidateNeverAttribute implements - that's generally what we've tried to do so this policy can be combined with others

This is where we land with IShouldValidate.

rynowak · 2017-01-12T18:59:18Z

src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/Validation/ValidateNeverAttribute.cs

+    /// <summary>
+    /// Indicates that a property should be excluded from validation. When applied to a property, the validation
+    /// system excludes that property. When applied to a type, the validation system excludes all properties within
+    /// that type.


The value of usage on a type seems unclear to me because it's equivalent to ValidateChildren = false right?

Wondering if we should just have a separate attribute for that.

This still applies w/ IShouldValidate and the new [ValidateNever]. But, I'm not sure I agree this is a bad idea. For one thing, the choices exactly match how [BindNever] et cetera can be used. For another, ValidateChildren = false and its SuppressChildValidationMetadataProvider wrapper configure something that we use metadata for in most analogous situations.

As I said here, my recommendation would be to use [ValidateNever] on types you own and SuppressChildValidationMetadataProvider on types you don't.

rynowak · 2017-01-12T19:02:12Z

src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/BindNeverAttribute.cs

-    /// system excludes that property. When applied to a type, the model binding system excludes all properties of that
-    /// type.
+    /// system excludes that property. When applied to a type, the model binding system excludes all properties within
+    /// that type.


should this say defined by?

Could but I'm not sure it's much more clear than the with / within a type distinction I've made. I'll let you make the call…

"defined by" is the technically correct term so I'd rather us do that

rynowak · 2017-01-12T19:03:32Z

src/Microsoft.AspNetCore.Mvc.Core/Internal/DefaultComplexObjectValidationStrategy.cs

-                            throw ex.InnerException;
-                        }
-                    }
+                    _entry = new ValidationEntry(property, key, () => GetModelOnMono(_model, property.PropertyName));


Do we still need this?

Lost a comment somewhere: See ValidateNeverProperty_IsSkippedWithoutAccessingModel(). That and similar tests fail with NREs before the IShouldValidate implementation is called.

They fail on mono or on CLR?

The Model accesses fail everywhere.

But, I now suspect you were asking specifically about the Mono workaround. We have repeatedly chosen to leave all similar workarounds alone. I wasn't going to spend the time checking whether the problem has been fixed.

dougbu · 2017-01-12T19:52:49Z

IE provide an attribute like [ValidateNever] but also an abstraction like IShouldValidate.

That's where this PR lands. I reverted the "move stuff" commit and redid the rest of the first approach.

rynowak · 2017-01-12T20:46:12Z

BTW you are the one that told me to look at the individual commits 😆 If you are unhappy with the quality of feedback then you only have yourself to blame.

dougbu · 2017-01-12T20:50:57Z

If you are unhappy with the quality of feedback then you only have yourself to blame.

Not unhappy at all! Just trying to make it clear which comments on individual commits still apply and which don't.

frankabbruzzese · 2017-01-13T10:52:21Z

@dougbu, I dont see any modification to SimpleTypeModelBinder.cs. When no value is supplied for a value type a validation error is added there! So a check should be performed there, too!

rynowak · 2017-01-13T16:53:17Z

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

+        /// If <c>null</c>, properties with this <see cref="ModelMetadata"/> are validated.
+        /// </summary>
+        /// <value>Defaults to <c>null</c>.</value>
+        public abstract IShouldValidate ShouldValidate { get; }


We need to come up with a better name for this. Ideas I have IPropertyValidationFilterProvider - IPropertyValidationFilter. Basically I like the idea that this would become more similar to something already have in terms of naming and UI

The analogues to IPropertyFilterProvider aren't very solid. For one thing, what would the Attributes implement and how would users change the Func<T, bool>? Do you mean the Attributes would implement IPropertyValidationFilter and that would contain a method which returns a method which returns a bool? If that's the case, where would IPropertyValidationFilterProvider be used that Func<T, bool> wouldn't be more appropriate?

For another, the names get very confusing unless we rename IPropertyFilterProvider to IPropertyBindingFilterProvider.

And, this feels like it adds a third (counting ValidateChildren) "primary" mechanism to provide static control of property validation. Not sure we need anything extra just for custom IValidationMetadataProvider support.

And, this feels like we're creating another layer of indirection to ease configuration in a custom IValidationMetadataProvider. We don't have a requirement for two mechanisms here.

Let's take this offline…

Decided offline that only the name needs to change.

Going with IPropertyValidationFilter. If this was a(nother) MVC do-over, might consider a new name for IPropertyFilterProvider (something more obviously binding-related) but not touching that now.

rynowak · 2017-01-13T16:54:57Z

src/Microsoft.AspNetCore.Mvc.Core/Internal/DefaultComplexObjectValidationStrategy.cs

-                            throw ex.InnerException;
-                        }
-                    }
+                    _entry = new ValidationEntry(property, key, () => GetModelOnMono(_model, property.PropertyName));


They fail on mono or on CLR?

rynowak · 2017-01-13T17:26:10Z

src/Microsoft.AspNetCore.Mvc.Abstractions/ModelBinding/Validation/IShouldValidate.cs

+        /// <param name="entry"><see cref="ValidationEntry"/> to check.</param>
+        /// <param name="parentEntry"><see cref="ValidationEntry"/> containing <paramref name="entry"/>.</param>
+        /// <returns><c>true</c> if <paramref name="entry"/> should be validated; <c>false</c> otherwise.</returns>
+        bool ShouldValidateEntry(ValidationEntry entry, ValidationEntry parentEntry);


One thing that I don't love about this is that it doesn't provide you the opportunity to 'close over' the property you are attached to (if you are attached to a property) it's a parameter.

I also don't really love that we're making ValidationEntry more prominent.

For instance

[MyShouldValidate] public class Widget { public string Name { get; set; } } public class MyShouldValidateAttribute : Attribute { public bool ShouldValidateEntry(ValidationEntry entry, ValidationEntry parentEntry) { // right here I need to look at entry.Metadata to figure out what property we're looking at } }

But then again if you're attached to a property you will always be called with the same metadata each time. These feel like different tasks and a lot of the information that's relevant for one is irrelevant for the other.

What if we did something like this? GetPropertyValidationFilter would be called once for each property and could close over any metadata details that are significant.

public class OptionalOfTValidateAttribute : Attribute, IPropertyValidationFilter { public Func<object, bool> GetPropertyValidationFilter(ModelMetadata property) { return container => property.ValueGetter(container) != default(Optional<T>); } } public class ValidateNeverAttribute : Attribute, IPropertyValidationFilter { public Func<object, bool> GetPropertyValidationFilter(ModelMetadata property) { return container => false; } }

The requirement is to support dynamic decisions about property validation. ModelMetadata is insufficient and any ValidationEntry.Model access before the filter says we should validate is problematic. Note ValidationEntry is already a public abstraction.

Again, let's take this offline.

🙈 was the (considered) offline decision

rynowak · 2017-01-13T17:29:42Z

src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/Validation/ValidateNeverAttribute.cs

+    /// <summary>
+    /// <see cref="IShouldValidate"/> implementation that unconditionally indicates a property should be excluded from
+    /// validation. When applied to a property, the validation system excludes that property. When applied to a type,
+    /// the validation system excludes all properties within that type.


That seems like good guidance to put in the docs!

dougbu · 2017-01-13T19:47:21Z

@frankabbruzzese,

I dont see any modification to SimpleTypeModelBinder.cs. When no value is supplied for a value type a validation error is added there! So a check should be performed there, too!

That's not technically a validation error. The model binding system runs before validation and also adds errors to ModelState. In this case, the SimpleTypeModelBinder was unable to set a property of a simple type (a ValueType). The situation is very similar to the error added when a user submits "Fred" for a DateTimeOffset property i.e. it's about getting values on the wire and setting properties, not about ValidationAttribute and other metadata.

There's a simple workaround: Make the optional int (or whatever) property Nullable<int> / int?.

If you'd like to discuss this particular scenario further, please open a separate issue.

- #5642 - lazy-load `ValidationEntry.Model` - avoids `Exception`s when moving to a property that will not be validated nits: - remove duplicate code in `ValidationVisitor` - clarify "all properties of" doc comments - also add missing `<param>` doc in `ViewDataInfo`

- undo breaking change! - `[I]ShouldValidate` -> `[I]PropertyValidationFilter` - self-imposed comment on comment

dougbu · 2017-01-14T00:36:21Z

🆙📅

merged all the original commits
addressed PR comments
rebased
renamed the PR to match new names

dougbu · 2017-01-16T18:51:58Z

ce53675

dnfclas added the cla-already-signed label Jan 11, 2017

dougbu commented Jan 11, 2017

View reviewed changes

dougbu requested a review from rynowak January 11, 2017 23:01

dougbu commented Jan 12, 2017

View reviewed changes

dougbu mentioned this pull request Jan 12, 2017

Need better way to suppress validation of properties/types during model binding #5642

Closed

rynowak reviewed Jan 12, 2017

View reviewed changes

dougbu mentioned this pull request Jan 13, 2017

API Check should catch abstract additions to public classes aspnet/BuildTools#146

Closed

rynowak reviewed Jan 13, 2017

View reviewed changes

dougbu mentioned this pull request Jan 13, 2017

Extend ModelMetadata to reduce ugliness #5686

Closed

dougbu added 2 commits January 13, 2017 16:14

Address PR comments

722eeef

- undo breaking change! - `[I]ShouldValidate` -> `[I]PropertyValidationFilter` - self-imposed comment on comment

dougbu force-pushed the dougbu/validate.never.sometimes.5642 branch from 90edf48 to 722eeef Compare January 14, 2017 00:15

dougbu changed the title ~~Add IShouldValidate and [ValidateNever]~~ Add [ValidateNever] and IShouldValidate Jan 14, 2017

dougbu assigned rynowak Jan 14, 2017

rynowak approved these changes Jan 14, 2017

View reviewed changes

dougbu changed the title ~~Add [ValidateNever] and IShouldValidate~~ Add [ValidateNever] and IPropertyValidationFilter Jan 16, 2017

dougbu closed this Jan 16, 2017

dougbu deleted the dougbu/validate.never.sometimes.5642 branch January 17, 2017 16:54

Add [ValidateNever] and IPropertyValidationFilter #5676

Add [ValidateNever] and IPropertyValidationFilter #5676

Conversation

dougbu commented Jan 11, 2017 • edited Loading

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

rynowak left a 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

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

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

dougbu commented Jan 12, 2017

rynowak commented Jan 12, 2017

dougbu commented Jan 12, 2017

frankabbruzzese commented Jan 13, 2017 • edited Loading

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

dougbu commented Jan 13, 2017

dougbu commented Jan 14, 2017

dougbu commented Jan 16, 2017

Add `[ValidateNever]` and `IPropertyValidationFilter` #5676

Add `[ValidateNever]` and `IPropertyValidationFilter` #5676

dougbu commented Jan 11, 2017 •

edited

Loading

frankabbruzzese commented Jan 13, 2017 •

edited

Loading