From 8b1487d1c2e2732af5a91b69a07eec9a740333bb Mon Sep 17 00:00:00 2001 From: Doug Bunting Date: Tue, 3 Jan 2017 15:22:09 -0800 Subject: [PATCH 1/2] Add `[ValidateNever]` and `IShouldValidate` - #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 `` doc in `ViewDataInfo` --- .../ModelBinding/ModelMetadata.cs | 8 + .../Validation/IShouldValidate.cs | 22 ++ .../Validation/ValidationEntry.cs | 49 ++- .../DefaultComplexObjectValidationStrategy.cs | 52 +-- .../DefaultValidationMetadataProvider.cs | 19 + .../ModelBinding/BindNeverAttribute.cs | 4 +- .../ModelBinding/BindRequiredAttribute.cs | 2 +- .../Metadata/DefaultModelMetadata.cs | 10 + .../ExcludeBindingMetadataProvider.cs | 2 +- .../Metadata/ValidationMetadata.cs | 9 +- .../Validation/ValidateNeverAttribute.cs | 22 ++ .../Validation/ValidationVisitor.cs | 56 +-- .../HiddenInputAttribute.cs | 4 +- .../ViewFeatures/ViewDataInfo.cs | 6 +- .../ModelBinding/ModelMetadataTest.cs | 9 + ...aultComplexObjectValidationStrategyTest.cs | 112 +++++- .../Metadata/DefaultModelMetadataTest.cs | 41 ++- .../DefaultValidationMetadataProviderTest.cs | 109 ++++++ .../ValidationIntegrationTests.cs | 343 ++++++++++++++++++ 19 files changed, 791 insertions(+), 88 deletions(-) create mode 100644 src/Microsoft.AspNetCore.Mvc.Abstractions/ModelBinding/Validation/IShouldValidate.cs create mode 100644 src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/Validation/ValidateNeverAttribute.cs diff --git a/src/Microsoft.AspNetCore.Mvc.Abstractions/ModelBinding/ModelMetadata.cs b/src/Microsoft.AspNetCore.Mvc.Abstractions/ModelBinding/ModelMetadata.cs index f0abc8f4a5..2a94b7dbf2 100644 --- a/src/Microsoft.AspNetCore.Mvc.Abstractions/ModelBinding/ModelMetadata.cs +++ b/src/Microsoft.AspNetCore.Mvc.Abstractions/ModelBinding/ModelMetadata.cs @@ -8,6 +8,7 @@ using System.Diagnostics; using System.Reflection; using Microsoft.AspNetCore.Mvc.ModelBinding.Metadata; +using Microsoft.AspNetCore.Mvc.ModelBinding.Validation; using Microsoft.Extensions.Internal; namespace Microsoft.AspNetCore.Mvc.ModelBinding @@ -295,6 +296,13 @@ public string PropertyName /// public abstract string TemplateHint { get; } + /// + /// Gets an implementation that indicates whether this model should be validated. + /// If null, properties with this are validated. + /// + /// Defaults to null. + public abstract IShouldValidate ShouldValidate { get; } + /// /// Gets a value that indicates whether properties or elements of the model should be validated. /// diff --git a/src/Microsoft.AspNetCore.Mvc.Abstractions/ModelBinding/Validation/IShouldValidate.cs b/src/Microsoft.AspNetCore.Mvc.Abstractions/ModelBinding/Validation/IShouldValidate.cs new file mode 100644 index 0000000000..7b0dc909bc --- /dev/null +++ b/src/Microsoft.AspNetCore.Mvc.Abstractions/ModelBinding/Validation/IShouldValidate.cs @@ -0,0 +1,22 @@ +// 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. + +namespace Microsoft.AspNetCore.Mvc.ModelBinding.Validation +{ + /// + /// Contract for attributes that determine whether associated properties should be validated. When the attribute is + /// applied to a property, the validation system calls to determine whether to + /// validate that property. When applied to a type, the validation system calls + /// for each property within that type to determine whether to validate it. + /// + public interface IShouldValidate + { + /// + /// Gets an indication whether the should be validated. + /// + /// to check. + /// containing . + /// true if should be validated; false otherwise. + bool ShouldValidateEntry(ValidationEntry entry, ValidationEntry parentEntry); + } +} diff --git a/src/Microsoft.AspNetCore.Mvc.Abstractions/ModelBinding/Validation/ValidationEntry.cs b/src/Microsoft.AspNetCore.Mvc.Abstractions/ModelBinding/Validation/ValidationEntry.cs index d757239356..a873273ca7 100644 --- a/src/Microsoft.AspNetCore.Mvc.Abstractions/ModelBinding/Validation/ValidationEntry.cs +++ b/src/Microsoft.AspNetCore.Mvc.Abstractions/ModelBinding/Validation/ValidationEntry.cs @@ -10,6 +10,9 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding.Validation /// public struct ValidationEntry { + private object _model; + private Func _modelAccessor; + /// /// Creates a new . /// @@ -30,7 +33,37 @@ public ValidationEntry(ModelMetadata metadata, string key, object model) Metadata = metadata; Key = key; - Model = model; + _model = model; + _modelAccessor = null; + } + + /// + /// Creates a new . + /// + /// The associated with the . + /// The model prefix associated with the . + /// A delegate that will return the . + public ValidationEntry(ModelMetadata metadata, string key, Func modelAccessor) + { + if (metadata == null) + { + throw new ArgumentNullException(nameof(metadata)); + } + + if (key == null) + { + throw new ArgumentNullException(nameof(key)); + } + + if (modelAccessor == null) + { + throw new ArgumentNullException(nameof(modelAccessor)); + } + + Metadata = metadata; + Key = key; + _model = null; + _modelAccessor = modelAccessor; } /// @@ -46,6 +79,18 @@ public ValidationEntry(ModelMetadata metadata, string key, object model) /// /// The model object. /// - public object Model { get; } + public object Model + { + get + { + if (_modelAccessor != null) + { + _model = _modelAccessor(); + _modelAccessor = null; + } + + return _model; + } + } } } diff --git a/src/Microsoft.AspNetCore.Mvc.Core/Internal/DefaultComplexObjectValidationStrategy.cs b/src/Microsoft.AspNetCore.Mvc.Core/Internal/DefaultComplexObjectValidationStrategy.cs index a0744bba58..135b13d2cf 100644 --- a/src/Microsoft.AspNetCore.Mvc.Core/Internal/DefaultComplexObjectValidationStrategy.cs +++ b/src/Microsoft.AspNetCore.Mvc.Core/Internal/DefaultComplexObjectValidationStrategy.cs @@ -84,36 +84,20 @@ public bool MoveNext() var propertyName = property.BinderModelName ?? property.PropertyName; var key = ModelNames.CreatePropertyModelName(_key, propertyName); - object model; - - // Our property accessors don't work on Mono 4.0.4 - see https://github.com/aspnet/External/issues/44 - // This is a workaround for what the PropertyGetter does in the background. - if (IsMono) + if (_model == null) + { + // Performance: Never create a delegate when container is null. + _entry = new ValidationEntry(property, key, model: null); + } + else if (IsMono) { - if (_model == null) - { - model = null; - } - else - { - var propertyInfo = _model.GetType().GetRuntimeProperty(property.PropertyName); - try - { - model = propertyInfo.GetValue(_model); - } - catch (TargetInvocationException ex) - { - throw ex.InnerException; - } - } + _entry = new ValidationEntry(property, key, () => GetModelOnMono(_model, property.PropertyName)); } else { - model = property.PropertyGetter(_model); + _entry = new ValidationEntry(property, key, () => GetModel(_model, property)); } - _entry = new ValidationEntry(property, key, model); - return true; } @@ -125,6 +109,26 @@ public void Reset() { throw new NotImplementedException(); } + + private static object GetModel(object container, ModelMetadata property) + { + return property.PropertyGetter(container); + } + + // Our property accessors don't work on Mono 4.0.4 - see https://github.com/aspnet/External/issues/44 + // This is a workaround for what the PropertyGetter does in the background. + private static object GetModelOnMono(object container, string propertyName) + { + var propertyInfo = container.GetType().GetRuntimeProperty(propertyName); + try + { + return propertyInfo.GetValue(container); + } + catch (TargetInvocationException ex) + { + throw ex.InnerException; + } + } } } } diff --git a/src/Microsoft.AspNetCore.Mvc.Core/Internal/DefaultValidationMetadataProvider.cs b/src/Microsoft.AspNetCore.Mvc.Core/Internal/DefaultValidationMetadataProvider.cs index 2e3116185e..edb6c89b14 100644 --- a/src/Microsoft.AspNetCore.Mvc.Core/Internal/DefaultValidationMetadataProvider.cs +++ b/src/Microsoft.AspNetCore.Mvc.Core/Internal/DefaultValidationMetadataProvider.cs @@ -2,6 +2,8 @@ // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. using System; +using System.Linq; +using System.Reflection; using Microsoft.AspNetCore.Mvc.ModelBinding.Metadata; using Microsoft.AspNetCore.Mvc.ModelBinding.Validation; @@ -34,6 +36,23 @@ public void CreateValidationMetadata(ValidationMetadataProviderContext context) } } } + + // IShouldValidate attributes on a type affect 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. + if (context.Key.MetadataKind == ModelMetadataKind.Property) + { + var shouldValidate = context.PropertyAttributes.OfType().FirstOrDefault(); + if (shouldValidate == null) + { + // No [ValidateNever] on the property. Check if container has this attribute. + shouldValidate = context.Key.ContainerType.GetTypeInfo() + .GetCustomAttributes(inherit: true) + .OfType() + .FirstOrDefault(); + } + + context.ValidationMetadata.ShouldValidate = shouldValidate; + } } } } \ No newline at end of file diff --git a/src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/BindNeverAttribute.cs b/src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/BindNeverAttribute.cs index 6fa4dcf60a..6c1c962973 100644 --- a/src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/BindNeverAttribute.cs +++ b/src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/BindNeverAttribute.cs @@ -7,8 +7,8 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding { /// /// Indicates that a property should be excluded from model binding. When applied to a property, the model binding - /// 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. /// [AttributeUsage(AttributeTargets.Class | AttributeTargets.Property, AllowMultiple = false, Inherited = true)] public sealed class BindNeverAttribute : BindingBehaviorAttribute diff --git a/src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/BindRequiredAttribute.cs b/src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/BindRequiredAttribute.cs index 0770991b54..7c7ccf5530 100644 --- a/src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/BindRequiredAttribute.cs +++ b/src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/BindRequiredAttribute.cs @@ -8,7 +8,7 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding /// /// Indicates that a property is required for model binding. When applied to a property, the model binding system /// requires a value for that property. When applied to a type, the model binding system requires values for all - /// properties of that type. + /// properties within that type. /// [AttributeUsage(AttributeTargets.Class | AttributeTargets.Property, AllowMultiple = false, Inherited = true)] public sealed class BindRequiredAttribute : BindingBehaviorAttribute diff --git a/src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/Metadata/DefaultModelMetadata.cs b/src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/Metadata/DefaultModelMetadata.cs index b99d128441..401eb18e2d 100644 --- a/src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/Metadata/DefaultModelMetadata.cs +++ b/src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/Metadata/DefaultModelMetadata.cs @@ -5,6 +5,7 @@ using System.Collections.Generic; using System.Collections.ObjectModel; using System.Linq; +using Microsoft.AspNetCore.Mvc.ModelBinding.Validation; namespace Microsoft.AspNetCore.Mvc.ModelBinding.Metadata { @@ -531,6 +532,15 @@ public override string TemplateHint } } + /// + public override IShouldValidate ShouldValidate + { + get + { + return ValidationMetadata.ShouldValidate; + } + } + /// public override bool ValidateChildren { diff --git a/src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/Metadata/ExcludeBindingMetadataProvider.cs b/src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/Metadata/ExcludeBindingMetadataProvider.cs index f0c236cc21..a4f4a51550 100644 --- a/src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/Metadata/ExcludeBindingMetadataProvider.cs +++ b/src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/Metadata/ExcludeBindingMetadataProvider.cs @@ -18,7 +18,7 @@ public class ExcludeBindingMetadataProvider : IBindingMetadataProvider /// Creates a new for the given . /// /// - /// The . All properties of this will have + /// The . All properties with this will have /// set to false. /// public ExcludeBindingMetadataProvider(Type type) diff --git a/src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/Metadata/ValidationMetadata.cs b/src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/Metadata/ValidationMetadata.cs index 57c38cef5f..5b2c46f019 100644 --- a/src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/Metadata/ValidationMetadata.cs +++ b/src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/Metadata/ValidationMetadata.cs @@ -2,6 +2,7 @@ // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. using System.Collections.Generic; +using Microsoft.AspNetCore.Mvc.ModelBinding.Validation; namespace Microsoft.AspNetCore.Mvc.ModelBinding.Metadata { @@ -18,9 +19,15 @@ public class ValidationMetadata /// public bool? IsRequired { get; set; } + /// + /// Gets or sets an implementation that indicates whether this model should be + /// validated. See . + /// + public IShouldValidate ShouldValidate { get; set; } + /// /// Gets or sets a value that indicates whether children of the model should be validated. If null - /// then will be true if either of + /// then will be true if either of /// or is true; /// false otherwise. /// diff --git a/src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/Validation/ValidateNeverAttribute.cs b/src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/Validation/ValidateNeverAttribute.cs new file mode 100644 index 0000000000..e11a9355fc --- /dev/null +++ b/src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/Validation/ValidateNeverAttribute.cs @@ -0,0 +1,22 @@ +// 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; + +namespace Microsoft.AspNetCore.Mvc.ModelBinding.Validation +{ + /// + /// 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. + /// + [AttributeUsage(AttributeTargets.Class | AttributeTargets.Property, AllowMultiple = false, Inherited = true)] + public sealed class ValidateNeverAttribute : Attribute, IShouldValidate + { + /// + public bool ShouldValidateEntry(ValidationEntry entry, ValidationEntry parentEntry) + { + return false; + } + } +} diff --git a/src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/Validation/ValidationVisitor.cs b/src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/Validation/ValidationVisitor.cs index 7ad300021f..a2f52d96c1 100644 --- a/src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/Validation/ValidationVisitor.cs +++ b/src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/Validation/ValidationVisitor.cs @@ -185,52 +185,26 @@ private bool Visit(ModelMetadata metadata, string key, object model) { if (_metadata.IsEnumerableType) { - return VisitEnumerableType(); + return VisitComplexType(DefaultCollectionValidationStrategy.Instance); } - else if (_metadata.IsComplexType) - { - return VisitComplexType(); - } - else + + if (_metadata.IsComplexType) { - return VisitSimpleType(); + return VisitComplexType(DefaultComplexObjectValidationStrategy.Instance); } - } - } - - private bool VisitEnumerableType() - { - var isValid = true; - if (_model != null && _metadata.ValidateChildren) - { - var strategy = _strategy ?? DefaultCollectionValidationStrategy.Instance; - isValid = VisitChildren(strategy); + return VisitSimpleType(); } - else if (_model != null) - { - // Suppress validation for the entries matching this prefix. This will temporarily set - // the current node to 'skipped' but we're going to visit it right away, so subsequent - // code will set it to 'valid' or 'invalid' - SuppressValidation(_key); - } - - // Double-checking HasReachedMaxErrors just in case this model has no elements. - if (isValid && !_modelState.HasReachedMaxErrors) - { - isValid &= ValidateNode(); - } - - return isValid; } - private bool VisitComplexType() + // Covers everything VisitSimpleType does not i.e. both enumerations and complex types. + private bool VisitComplexType(IValidationStrategy defaultStrategy) { var isValid = true; if (_model != null && _metadata.ValidateChildren) { - var strategy = _strategy ?? DefaultComplexObjectValidationStrategy.Instance; + var strategy = _strategy ?? defaultStrategy; isValid = VisitChildren(strategy); } else if (_model != null) @@ -265,14 +239,20 @@ private bool VisitChildren(IValidationStrategy strategy) { var isValid = true; var enumerator = strategy.GetChildren(_metadata, _key, _model); + var parentEntry = new ValidationEntry(_metadata, _key, _model); while (enumerator.MoveNext()) { - var metadata = enumerator.Current.Metadata; - var model = enumerator.Current.Model; - var key = enumerator.Current.Key; + var entry = enumerator.Current; + var metadata = entry.Metadata; + var key = entry.Key; + if (metadata.ShouldValidate?.ShouldValidateEntry(entry, parentEntry) == false) + { + SuppressValidation(key); + continue; + } - isValid &= Visit(metadata, key, model); + isValid &= Visit(metadata, key, entry.Model); } return isValid; diff --git a/src/Microsoft.AspNetCore.Mvc.DataAnnotations/HiddenInputAttribute.cs b/src/Microsoft.AspNetCore.Mvc.DataAnnotations/HiddenInputAttribute.cs index ea5525f470..e88393d48c 100644 --- a/src/Microsoft.AspNetCore.Mvc.DataAnnotations/HiddenInputAttribute.cs +++ b/src/Microsoft.AspNetCore.Mvc.DataAnnotations/HiddenInputAttribute.cs @@ -6,8 +6,8 @@ namespace Microsoft.AspNetCore.Mvc { /// - /// Indicates associated property or all properties of associated type should be edited using an <input> - /// element of type "hidden". + /// Indicates associated property or all properties with the associated type should be edited using an + /// <input> element of type "hidden". /// /// /// When overriding a inherited from a base class, should apply both diff --git a/src/Microsoft.AspNetCore.Mvc.ViewFeatures/ViewFeatures/ViewDataInfo.cs b/src/Microsoft.AspNetCore.Mvc.ViewFeatures/ViewFeatures/ViewDataInfo.cs index 62e273a4ae..d2f5b928d3 100644 --- a/src/Microsoft.AspNetCore.Mvc.ViewFeatures/ViewFeatures/ViewDataInfo.cs +++ b/src/Microsoft.AspNetCore.Mvc.ViewFeatures/ViewFeatures/ViewDataInfo.cs @@ -27,8 +27,8 @@ public ViewDataInfo(object container, object value) /// /// Initializes a new instance of the class with info about a - /// lookup which is evaluated when is read. - /// It uses on + /// lookup which is evaluated when is read. + /// It uses on /// passing parameter to lazily evaluate the value. /// /// The that will be evaluated from. @@ -45,7 +45,7 @@ public ViewDataInfo(object container, PropertyInfo propertyInfo) /// /// The that has the . /// The that represents 's property. - /// + /// A delegate that will return the . public ViewDataInfo(object container, PropertyInfo propertyInfo, Func valueAccessor) { Container = container; diff --git a/test/Microsoft.AspNetCore.Mvc.Abstractions.Test/ModelBinding/ModelMetadataTest.cs b/test/Microsoft.AspNetCore.Mvc.Abstractions.Test/ModelBinding/ModelMetadataTest.cs index 823bb15efc..9b01691add 100644 --- a/test/Microsoft.AspNetCore.Mvc.Abstractions.Test/ModelBinding/ModelMetadataTest.cs +++ b/test/Microsoft.AspNetCore.Mvc.Abstractions.Test/ModelBinding/ModelMetadataTest.cs @@ -6,6 +6,7 @@ using System.Collections.Generic; using System.Collections.ObjectModel; using Microsoft.AspNetCore.Mvc.ModelBinding.Metadata; +using Microsoft.AspNetCore.Mvc.ModelBinding.Validation; using Xunit; namespace Microsoft.AspNetCore.Mvc.ModelBinding @@ -576,6 +577,14 @@ public override string TemplateHint } } + public override IShouldValidate ShouldValidate + { + get + { + throw new NotImplementedException(); + } + } + public override bool ValidateChildren { get diff --git a/test/Microsoft.AspNetCore.Mvc.Core.Test/Internal/DefaultComplexObjectValidationStrategyTest.cs b/test/Microsoft.AspNetCore.Mvc.Core.Test/Internal/DefaultComplexObjectValidationStrategyTest.cs index 2cc4168685..9ed691d26d 100644 --- a/test/Microsoft.AspNetCore.Mvc.Core.Test/Internal/DefaultComplexObjectValidationStrategyTest.cs +++ b/test/Microsoft.AspNetCore.Mvc.Core.Test/Internal/DefaultComplexObjectValidationStrategyTest.cs @@ -1,6 +1,7 @@ // 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.Generic; using System.Linq; using Microsoft.AspNetCore.Mvc.ModelBinding; @@ -12,7 +13,7 @@ namespace Microsoft.AspNetCore.Mvc.Internal public class DefaultComplexObjectValidationStrategyTest { [Fact] - public void EnumerateElements() + public void GetChildren_ReturnsExpectedElements() { // Arrange var model = new Person() @@ -31,23 +32,92 @@ public void EnumerateElements() // Assert Assert.Collection( BufferEntries(enumerator).OrderBy(e => e.Key), - e => + entry => { - Assert.Equal("prefix.Age", e.Key); - Assert.Equal(23, e.Model); - Assert.Same(metadata.Properties["Age"], e.Metadata); + Assert.Equal("prefix.Age", entry.Key); + Assert.Equal(23, entry.Model); + Assert.Same(metadata.Properties["Age"], entry.Metadata); }, - e => + entry => { - Assert.Equal("prefix.Id", e.Key); - Assert.Equal(1, e.Model); - Assert.Same(metadata.Properties["Id"], e.Metadata); + Assert.Equal("prefix.Id", entry.Key); + Assert.Equal(1, entry.Model); + Assert.Same(metadata.Properties["Id"], entry.Metadata); }, - e => + entry => { - Assert.Equal("prefix.Name", e.Key); - Assert.Equal("Joey", e.Model); - Assert.Same(metadata.Properties["Name"], e.Metadata); + Assert.Equal("prefix.Name", entry.Key); + Assert.Equal("Joey", entry.Model); + Assert.Same(metadata.Properties["Name"], entry.Metadata); + }); + } + + [Fact] + public void GetChildren_SetsModelNull_IfContainerNull() + { + // Arrange + Person model = null; + var metadata = TestModelMetadataProvider.CreateDefaultProvider().GetMetadataForType(typeof(Person)); + var strategy = DefaultComplexObjectValidationStrategy.Instance; + + // Act + var enumerator = strategy.GetChildren(metadata, "prefix", model); + + // Assert + Assert.Collection( + BufferEntries(enumerator).OrderBy(e => e.Key), + entry => + { + Assert.Equal("prefix.Age", entry.Key); + Assert.Null(entry.Model); + Assert.Same(metadata.Properties["Age"], entry.Metadata); + }, + entry => + { + Assert.Equal("prefix.Id", entry.Key); + Assert.Null(entry.Model); + Assert.Same(metadata.Properties["Id"], entry.Metadata); + }, + entry => + { + Assert.Equal("prefix.Name", entry.Key); + Assert.Null(entry.Model); + Assert.Same(metadata.Properties["Name"], entry.Metadata); + }); + } + + [Fact] + public void GetChildren_LazyLoadsModel() + { + // Arrange + var model = new LazyPerson(input: null); + var metadata = TestModelMetadataProvider.CreateDefaultProvider().GetMetadataForType(typeof(LazyPerson)); + var strategy = DefaultComplexObjectValidationStrategy.Instance; + + // Act + var enumerator = strategy.GetChildren(metadata, "prefix", model); + + // Assert + // Note: NREs are not thrown until the Model property is accessed. + Assert.Collection( + BufferEntries(enumerator).OrderBy(e => e.Key), + entry => + { + Assert.Equal("prefix.Age", entry.Key); + Assert.Equal(23, entry.Model); + Assert.Same(metadata.Properties["Age"], entry.Metadata); + }, + entry => + { + Assert.Equal("prefix.Id", entry.Key); + Assert.Throws(() => entry.Model); + Assert.Same(metadata.Properties["Id"], entry.Metadata); + }, + entry => + { + Assert.Equal("prefix.Name", entry.Key); + Assert.Throws(() => entry.Model); + Assert.Same(metadata.Properties["Name"], entry.Metadata); }); } @@ -70,5 +140,21 @@ private class Person public string Name { get; set; } } + + private class LazyPerson + { + private string _string; + + public LazyPerson(string input) + { + _string = input; + } + + public int Id => _string.Length; + + public int Age => 23; + + public string Name => _string.Substring(3, 5); + } } } diff --git a/test/Microsoft.AspNetCore.Mvc.Core.Test/ModelBinding/Metadata/DefaultModelMetadataTest.cs b/test/Microsoft.AspNetCore.Mvc.Core.Test/ModelBinding/Metadata/DefaultModelMetadataTest.cs index 57def74719..58f06e2819 100644 --- a/test/Microsoft.AspNetCore.Mvc.Core.Test/ModelBinding/Metadata/DefaultModelMetadataTest.cs +++ b/test/Microsoft.AspNetCore.Mvc.Core.Test/ModelBinding/Metadata/DefaultModelMetadataTest.cs @@ -8,6 +8,7 @@ using System.Linq; using System.Xml; using Microsoft.AspNetCore.Mvc.Internal; +using Microsoft.AspNetCore.Mvc.ModelBinding.Validation; using Moq; using Xunit; @@ -50,6 +51,7 @@ public void DefaultValues() Assert.False(metadata.IsRequired); // Defaults to false for reference types Assert.True(metadata.ShowForDisplay); Assert.True(metadata.ShowForEdit); + Assert.False(metadata.ValidateChildren); // Defaults to true for complex and enumerable types. Assert.Null(metadata.DataTypeName); Assert.Null(metadata.Description); @@ -60,9 +62,10 @@ public void DefaultValues() Assert.Null(metadata.EnumGroupedDisplayNamesAndValues); Assert.Null(metadata.EnumNamesAndValues); Assert.Null(metadata.NullDisplayText); - Assert.Null(metadata.TemplateHint); + Assert.Null(metadata.ShouldValidate); Assert.Null(metadata.SimpleDisplayProperty); Assert.Null(metadata.Placeholder); + Assert.Null(metadata.TemplateHint); Assert.Equal(10000, ModelMetadata.DefaultOrder); Assert.Equal(ModelMetadata.DefaultOrder, metadata.Order); @@ -659,6 +662,42 @@ public void ValidateChildren_ComplexAndEnumerableTypes(Type modelType) Assert.True(validateChildren); } + public static TheoryData ShouldValidateData + { + get + { + return new TheoryData + { + null, + new ValidateNeverAttribute(), + }; + } + } + + [Theory] + [MemberData(nameof(ShouldValidateData))] + public void ShouldValidate_ReflectsShouldValidate_FromValidationMetadata(IShouldValidate value) + { + // Arrange + var detailsProvider = new EmptyCompositeMetadataDetailsProvider(); + var provider = new DefaultModelMetadataProvider(detailsProvider); + + var key = ModelMetadataIdentity.ForType(typeof(int)); + var cache = new DefaultMetadataDetails(key, new ModelAttributes(new object[0])); + cache.ValidationMetadata = new ValidationMetadata + { + ShouldValidate = value, + }; + + var metadata = new DefaultModelMetadata(provider, detailsProvider, cache); + + // Act + var shouldValidate = metadata.ShouldValidate; + + // Assert + Assert.Same(value, shouldValidate); + } + [Fact] public void ValidateChildren_OverrideTrue() { diff --git a/test/Microsoft.AspNetCore.Mvc.Core.Test/ModelBinding/Metadata/DefaultValidationMetadataProviderTest.cs b/test/Microsoft.AspNetCore.Mvc.Core.Test/ModelBinding/Metadata/DefaultValidationMetadataProviderTest.cs index 475c92f11b..7970d3baaa 100644 --- a/test/Microsoft.AspNetCore.Mvc.Core.Test/ModelBinding/Metadata/DefaultValidationMetadataProviderTest.cs +++ b/test/Microsoft.AspNetCore.Mvc.Core.Test/ModelBinding/Metadata/DefaultValidationMetadataProviderTest.cs @@ -11,6 +11,104 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding.Metadata { public class DefaultValidationMetadataProviderTest { + [Fact] + public void ShouldValidate_ShouldValidateEntry_False_IfPropertyHasValidateNever() + { + // Arrange + var provider = new DefaultValidationMetadataProvider(); + + var attributes = new Attribute[] { new ValidateNeverAttribute() }; + var key = ModelMetadataIdentity.ForProperty(typeof(int), "Length", typeof(string)); + var context = new ValidationMetadataProviderContext(key, new ModelAttributes(attributes, new object[0])); + + // Act + provider.CreateValidationMetadata(context); + + // Assert + Assert.NotNull(context.ValidationMetadata.ShouldValidate); + Assert.False(context.ValidationMetadata.ShouldValidate.ShouldValidateEntry( + new ValidationEntry(), + new ValidationEntry())); + } + + [Fact] + public void ShouldValidate_Null_IfPropertyHasValidateNeverOnItsType() + { + // Arrange + var provider = new DefaultValidationMetadataProvider(); + + var attributes = new Attribute[] { new ValidateNeverAttribute() }; + var key = ModelMetadataIdentity.ForProperty(typeof(int), "Length", typeof(string)); + var context = new ValidationMetadataProviderContext(key, new ModelAttributes(new object[0], attributes)); + + // Act + provider.CreateValidationMetadata(context); + + // Assert + Assert.Null(context.ValidationMetadata.ShouldValidate); + } + + [Fact] + public void ShouldValidate_Null_ForType() + { + // Arrange + var provider = new DefaultValidationMetadataProvider(); + + var attributes = new Attribute[] { new ValidateNeverAttribute() }; + var key = ModelMetadataIdentity.ForType(typeof(ValidateNeverClass)); + var context = new ValidationMetadataProviderContext(key, new ModelAttributes(attributes)); + + // Act + provider.CreateValidationMetadata(context); + + // Assert + Assert.Null(context.ValidationMetadata.ShouldValidate); + } + + [Fact] + public void ShouldValidate_ShouldValidateEntry_False_IfContainingTypeHasValidateNever() + { + // Arrange + var provider = new DefaultValidationMetadataProvider(); + + var key = ModelMetadataIdentity.ForProperty( + typeof(string), + nameof(ValidateNeverClass.ClassName), + typeof(ValidateNeverClass)); + var context = new ValidationMetadataProviderContext(key, new ModelAttributes(new object[0], new object[0])); + + // Act + provider.CreateValidationMetadata(context); + + // Assert + Assert.NotNull(context.ValidationMetadata.ShouldValidate); + Assert.False(context.ValidationMetadata.ShouldValidate.ShouldValidateEntry( + new ValidationEntry(), + new ValidationEntry())); + } + + [Fact] + public void ShouldValidate_ShouldValidateEntry_False_IfContainingTypeInheritsValidateNever() + { + // Arrange + var provider = new DefaultValidationMetadataProvider(); + + var key = ModelMetadataIdentity.ForProperty( + typeof(string), + nameof(ValidateNeverSubclass.SubclassName), + typeof(ValidateNeverSubclass)); + var context = new ValidationMetadataProviderContext(key, new ModelAttributes(new object[0], new object[0])); + + // Act + provider.CreateValidationMetadata(context); + + // Assert + Assert.NotNull(context.ValidationMetadata.ShouldValidate); + Assert.False(context.ValidationMetadata.ShouldValidate.ShouldValidateEntry( + new ValidationEntry(), + new ValidationEntry())); + } + [Fact] public void GetValidationDetails_MarkedWithClientValidator_ReturnsValidator() { @@ -69,6 +167,17 @@ public void GetValidationDetails_Validator_AlreadyInContext_Ignores() Assert.Same(attribute, validatorMetadata); } + [ValidateNever] + private class ValidateNeverClass + { + public string ClassName { get; set; } + } + + private class ValidateNeverSubclass : ValidateNeverClass + { + public string SubclassName { get; set; } + } + private class TestModelValidationAttribute : Attribute, IModelValidator { public IEnumerable Validate(ModelValidationContext context) diff --git a/test/Microsoft.AspNetCore.Mvc.IntegrationTests/ValidationIntegrationTests.cs b/test/Microsoft.AspNetCore.Mvc.IntegrationTests/ValidationIntegrationTests.cs index d55d09496b..fc69907540 100644 --- a/test/Microsoft.AspNetCore.Mvc.IntegrationTests/ValidationIntegrationTests.cs +++ b/test/Microsoft.AspNetCore.Mvc.IntegrationTests/ValidationIntegrationTests.cs @@ -12,6 +12,8 @@ using Microsoft.AspNetCore.Mvc.Abstractions; using Microsoft.AspNetCore.Mvc.Controllers; using Microsoft.AspNetCore.Mvc.ModelBinding; +using Microsoft.AspNetCore.Mvc.ModelBinding.Metadata; +using Microsoft.AspNetCore.Mvc.ModelBinding.Validation; using Newtonsoft.Json.Linq; using Xunit; @@ -1222,6 +1224,347 @@ public async Task Validation_OverflowException_ShowsInvalidValueMessage_OnSimple Assert.Equal("The value '-123' is not valid for Zip.", error.ErrorMessage); } + private class NeverValid : IValidatableObject + { + public string NeverValidProperty { get; set; } + + public IEnumerable Validate(ValidationContext validationContext) + { + return new[] { new ValidationResult("This is not valid.") }; + } + } + + private class NeverValidAttribute : ValidationAttribute + { + protected override ValidationResult IsValid(object value, ValidationContext validationContext) + { + // By default, ValidationVisitor visits _all_ properties within a non-null complex object. + // But, like most reasonable ValidationAttributes, NeverValidAttribute ignores null property values. + if (value == null) + { + return ValidationResult.Success; + } + + return new ValidationResult("Properties with this are not valid."); + } + } + + private class ValidateSomeProperties + { + public NeverValid NeverValid { get; set; } + + [NeverValid] + public string NeverValidBecauseAttribute { get; set; } + + [ValidateNever] + [NeverValid] + public string ValidateNever { get; set; } + + [ValidateNever] + public int ValidateNeverLength => ValidateNever.Length; + } + + [ValidateNever] + private class ValidateNoProperties : ValidateSomeProperties + { + } + + [Fact] + public async Task IValidatableObject_IsValidated() + { + // Arrange + var parameter = new ParameterDescriptor + { + Name = "parameter", + ParameterType = typeof(ValidateSomeProperties), + }; + + var testContext = ModelBindingTestHelper.GetTestContext( + request => request.QueryString + = new QueryString($"?{nameof(ValidateSomeProperties.NeverValid)}.{nameof(NeverValid.NeverValidProperty)}=1")); + + var argumentBinder = ModelBindingTestHelper.GetArgumentBinder(); + var modelState = testContext.ModelState; + + // Act + var result = await argumentBinder.BindModelAsync(parameter, testContext); + + // Assert + Assert.True(result.IsModelSet); + var model = Assert.IsType(result.Model); + Assert.Equal("1", model.NeverValid.NeverValidProperty); + + Assert.False(modelState.IsValid); + Assert.Equal(1, modelState.ErrorCount); + Assert.Collection( + modelState, + state => + { + Assert.Equal(nameof(ValidateSomeProperties.NeverValid), state.Key); + Assert.Equal(ModelValidationState.Invalid, state.Value.ValidationState); + + var error = Assert.Single(state.Value.Errors); + Assert.Equal("This is not valid.", error.ErrorMessage); + Assert.Null(error.Exception); + }, + state => + { + Assert.Equal( + $"{nameof(ValidateSomeProperties.NeverValid)}.{nameof(NeverValid.NeverValidProperty)}", + state.Key); + Assert.Equal(ModelValidationState.Valid, state.Value.ValidationState); + }); + } + + [Fact] + public async Task CustomValidationAttribute_IsValidated() + { + // Arrange + var parameter = new ParameterDescriptor + { + Name = "parameter", + ParameterType = typeof(ValidateSomeProperties), + }; + + var testContext = ModelBindingTestHelper.GetTestContext( + request => request.QueryString + = new QueryString($"?{nameof(ValidateSomeProperties.NeverValidBecauseAttribute)}=1")); + + var argumentBinder = ModelBindingTestHelper.GetArgumentBinder(); + var modelState = testContext.ModelState; + + // Act + var result = await argumentBinder.BindModelAsync(parameter, testContext); + + // Assert + Assert.True(result.IsModelSet); + var model = Assert.IsType(result.Model); + Assert.Equal("1", model.NeverValidBecauseAttribute); + + Assert.False(modelState.IsValid); + Assert.Equal(1, modelState.ErrorCount); + var kvp = Assert.Single(modelState); + Assert.Equal(nameof(ValidateSomeProperties.NeverValidBecauseAttribute), kvp.Key); + var state = kvp.Value; + Assert.NotNull(state); + Assert.Equal(ModelValidationState.Invalid, state.ValidationState); + var error = Assert.Single(state.Errors); + Assert.Equal("Properties with this are not valid.", error.ErrorMessage); + Assert.Null(error.Exception); + } + + [Fact] + public async Task ValidateNeverProperty_IsSkipped() + { + // Arrange + var parameter = new ParameterDescriptor + { + Name = "parameter", + ParameterType = typeof(ValidateSomeProperties), + }; + + var testContext = ModelBindingTestHelper.GetTestContext( + request => request.QueryString + = new QueryString($"?{nameof(ValidateSomeProperties.ValidateNever)}=1")); + + var argumentBinder = ModelBindingTestHelper.GetArgumentBinder(); + var modelState = testContext.ModelState; + + // Act + var result = await argumentBinder.BindModelAsync(parameter, testContext); + + // Assert + Assert.True(result.IsModelSet); + var model = Assert.IsType(result.Model); + Assert.Equal("1", model.ValidateNever); + + Assert.True(modelState.IsValid); + var kvp = Assert.Single(modelState); + Assert.Equal(nameof(ValidateSomeProperties.ValidateNever), kvp.Key); + var state = kvp.Value; + Assert.NotNull(state); + Assert.Equal(ModelValidationState.Skipped, state.ValidationState); + } + + [Fact] + public async Task ValidateNeverProperty_IsSkippedWithoutAccessingModel() + { + // Arrange + var parameter = new ParameterDescriptor + { + Name = "parameter", + ParameterType = typeof(ValidateSomeProperties), + }; + + var testContext = ModelBindingTestHelper.GetTestContext(); + var argumentBinder = ModelBindingTestHelper.GetArgumentBinder(); + var modelState = testContext.ModelState; + + // Act + var result = await argumentBinder.BindModelAsync(parameter, testContext); + + // Assert + Assert.True(result.IsModelSet); + var model = Assert.IsType(result.Model); + + // Note this Exception is not thrown earlier. + Assert.Throws(() => model.ValidateNeverLength); + + Assert.True(modelState.IsValid); + Assert.Empty(modelState); + } + + [Theory] + [InlineData(nameof(ValidateSomeProperties.NeverValid) + "." + nameof(NeverValid.NeverValidProperty))] + [InlineData(nameof(ValidateSomeProperties.NeverValidBecauseAttribute))] + [InlineData(nameof(ValidateSomeProperties.ValidateNever))] + public async Task PropertyWithinValidateNeverType_IsSkipped(string propertyName) + { + // Arrange + var parameter = new ParameterDescriptor + { + Name = "parameter", + ParameterType = typeof(ValidateNoProperties), + }; + + var testContext = ModelBindingTestHelper.GetTestContext( + request => request.QueryString = new QueryString($"?{propertyName}=1")); + + var argumentBinder = ModelBindingTestHelper.GetArgumentBinder(); + var modelState = testContext.ModelState; + + // Act + var result = await argumentBinder.BindModelAsync(parameter, testContext); + + // Assert + Assert.True(result.IsModelSet); + Assert.IsType(result.Model); + + Assert.True(modelState.IsValid); + var kvp = Assert.Single(modelState); + Assert.Equal(propertyName, kvp.Key); + var state = kvp.Value; + Assert.NotNull(state); + Assert.Equal(ModelValidationState.Skipped, state.ValidationState); + } + + private class ValidateSometimesAttribute : Attribute, IShouldValidate + { + private readonly string _otherProperty; + + public ValidateSometimesAttribute(string otherProperty) + { + // Would null-check otherProperty in real life. + _otherProperty = otherProperty; + } + + public bool ShouldValidateEntry(ValidationEntry entry, ValidationEntry parentEntry) + { + if (entry.Metadata.MetadataKind == ModelMetadataKind.Property && + parentEntry.Metadata != null) + { + // In real life, would throw an InvalidOperationException if otherProperty were null i.e. the + // property was not known. Could also assert container is non-null (see ValidationVisitor). + var container = parentEntry.Model; + var otherProperty = parentEntry.Metadata.Properties[_otherProperty]; + if (otherProperty.PropertyGetter(container) == null) + { + return false; + } + } + + return true; + } + } + + private class ValidateSomePropertiesSometimes + { + public string Control { get; set; } + + [ValidateSometimes(nameof(Control))] + public int ControlLength => Control.Length; + } + + [Fact] + public async Task PropertyToSometimesSkip_IsSkipped_IfControlIsNull() + { + // Arrange + var parameter = new ParameterDescriptor + { + Name = "parameter", + ParameterType = typeof(ValidateSomePropertiesSometimes), + }; + + var testContext = ModelBindingTestHelper.GetTestContext(); + var argumentBinder = ModelBindingTestHelper.GetArgumentBinder(); + var modelState = testContext.ModelState; + + // Add an entry for the ControlLength property so that we can observe Skipped versus Valid states. + modelState.SetModelValue( + nameof(ValidateSomePropertiesSometimes.ControlLength), + rawValue: null, + attemptedValue: null); + + // Act + var result = await argumentBinder.BindModelAsync(parameter, testContext); + + // Assert + Assert.True(result.IsModelSet); + var model = Assert.IsType(result.Model); + Assert.Null(model.Control); + + // Note this Exception is not thrown earlier. + Assert.Throws(() => model.ControlLength); + + Assert.True(modelState.IsValid); + var kvp = Assert.Single(modelState); + Assert.Equal(nameof(ValidateSomePropertiesSometimes.ControlLength), kvp.Key); + Assert.Equal(ModelValidationState.Skipped, kvp.Value.ValidationState); + } + + [Fact] + public async Task PropertyToSometimesSkip_IsValidated_IfControlIsNotNull() + { + // Arrange + var parameter = new ParameterDescriptor + { + Name = "parameter", + ParameterType = typeof(ValidateSomePropertiesSometimes), + }; + + var testContext = ModelBindingTestHelper.GetTestContext( + request => request.QueryString = new QueryString( + $"?{nameof(ValidateSomePropertiesSometimes.Control)}=1")); + + var argumentBinder = ModelBindingTestHelper.GetArgumentBinder(); + var modelState = testContext.ModelState; + + // Add an entry for the ControlLength property so that we can observe Skipped versus Valid states. + modelState.SetModelValue( + nameof(ValidateSomePropertiesSometimes.ControlLength), + rawValue: null, + attemptedValue: null); + + // Act + var result = await argumentBinder.BindModelAsync(parameter, testContext); + + // Assert + Assert.True(result.IsModelSet); + var model = Assert.IsType(result.Model); + Assert.Equal("1", model.Control); + Assert.Equal(1, model.ControlLength); + + Assert.True(modelState.IsValid); + Assert.Collection( + modelState, + state => Assert.Equal(nameof(ValidateSomePropertiesSometimes.Control), state.Key), + state => + { + Assert.Equal(nameof(ValidateSomePropertiesSometimes.ControlLength), state.Key); + Assert.Equal(ModelValidationState.Valid, state.Value.ValidationState); + }); + } + private class Order11 { public IEnumerable
ShippingAddresses { get; set; } From 722eeef5aea5c0fdc127a41eb1529c160f34d7ee Mon Sep 17 00:00:00 2001 From: Doug Bunting Date: Wed, 11 Jan 2017 16:23:18 -0800 Subject: [PATCH 2/2] Address PR comments - undo breaking change! - `[I]ShouldValidate` -> `[I]PropertyValidationFilter` - self-imposed comment on comment --- .../ModelBinding/ModelMetadata.cs | 6 ++--- ...lidate.cs => IPropertyValidationFilter.cs} | 6 ++--- .../DefaultValidationMetadataProvider.cs | 17 ++++++------ .../ModelBinding/BindNeverAttribute.cs | 4 +-- .../ModelBinding/BindRequiredAttribute.cs | 2 +- .../Metadata/DefaultModelMetadata.cs | 4 +-- .../Metadata/ValidationMetadata.cs | 6 ++--- .../Validation/ValidateNeverAttribute.cs | 8 +++--- .../Validation/ValidationVisitor.cs | 2 +- .../ModelBinding/ModelMetadataTest.cs | 2 +- .../Metadata/DefaultModelMetadataTest.cs | 16 ++++++------ .../DefaultValidationMetadataProviderTest.cs | 26 +++++++++---------- .../ValidationIntegrationTests.cs | 2 +- 13 files changed, 51 insertions(+), 50 deletions(-) rename src/Microsoft.AspNetCore.Mvc.Abstractions/ModelBinding/Validation/{IShouldValidate.cs => IPropertyValidationFilter.cs} (90%) diff --git a/src/Microsoft.AspNetCore.Mvc.Abstractions/ModelBinding/ModelMetadata.cs b/src/Microsoft.AspNetCore.Mvc.Abstractions/ModelBinding/ModelMetadata.cs index 2a94b7dbf2..47080996bf 100644 --- a/src/Microsoft.AspNetCore.Mvc.Abstractions/ModelBinding/ModelMetadata.cs +++ b/src/Microsoft.AspNetCore.Mvc.Abstractions/ModelBinding/ModelMetadata.cs @@ -297,11 +297,11 @@ public string PropertyName public abstract string TemplateHint { get; } /// - /// Gets an implementation that indicates whether this model should be validated. - /// If null, properties with this are validated. + /// Gets an implementation that indicates whether this model should be + /// validated. If null, properties with this are validated. /// /// Defaults to null. - public abstract IShouldValidate ShouldValidate { get; } + public virtual IPropertyValidationFilter PropertyValidationFilter => null; /// /// Gets a value that indicates whether properties or elements of the model should be validated. diff --git a/src/Microsoft.AspNetCore.Mvc.Abstractions/ModelBinding/Validation/IShouldValidate.cs b/src/Microsoft.AspNetCore.Mvc.Abstractions/ModelBinding/Validation/IPropertyValidationFilter.cs similarity index 90% rename from src/Microsoft.AspNetCore.Mvc.Abstractions/ModelBinding/Validation/IShouldValidate.cs rename to src/Microsoft.AspNetCore.Mvc.Abstractions/ModelBinding/Validation/IPropertyValidationFilter.cs index 7b0dc909bc..622b41f70c 100644 --- a/src/Microsoft.AspNetCore.Mvc.Abstractions/ModelBinding/Validation/IShouldValidate.cs +++ b/src/Microsoft.AspNetCore.Mvc.Abstractions/ModelBinding/Validation/IPropertyValidationFilter.cs @@ -7,9 +7,9 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding.Validation /// Contract for attributes that determine whether associated properties should be validated. When the attribute is /// applied to a property, the validation system calls to determine whether to /// validate that property. When applied to a type, the validation system calls - /// for each property within that type to determine whether to validate it. + /// for each property that type defines to determine whether to validate it. /// - public interface IShouldValidate + public interface IPropertyValidationFilter { /// /// Gets an indication whether the should be validated. @@ -19,4 +19,4 @@ public interface IShouldValidate /// true if should be validated; false otherwise. bool ShouldValidateEntry(ValidationEntry entry, ValidationEntry parentEntry); } -} +} \ No newline at end of file diff --git a/src/Microsoft.AspNetCore.Mvc.Core/Internal/DefaultValidationMetadataProvider.cs b/src/Microsoft.AspNetCore.Mvc.Core/Internal/DefaultValidationMetadataProvider.cs index edb6c89b14..03faa4be62 100644 --- a/src/Microsoft.AspNetCore.Mvc.Core/Internal/DefaultValidationMetadataProvider.cs +++ b/src/Microsoft.AspNetCore.Mvc.Core/Internal/DefaultValidationMetadataProvider.cs @@ -37,21 +37,22 @@ public void CreateValidationMetadata(ValidationMetadataProviderContext context) } } - // IShouldValidate attributes on a type affect 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. + // IPropertyValidationFilter attributes on a type affect properties in that type, not properties that have + // that type. Thus, we ignore context.TypeAttributes for properties and not check at all for types. if (context.Key.MetadataKind == ModelMetadataKind.Property) { - var shouldValidate = context.PropertyAttributes.OfType().FirstOrDefault(); - if (shouldValidate == null) + var validationFilter = context.PropertyAttributes.OfType().FirstOrDefault(); + if (validationFilter == null) { - // No [ValidateNever] on the property. Check if container has this attribute. - shouldValidate = context.Key.ContainerType.GetTypeInfo() + // No IPropertyValidationFilter attributes on the property. + // Check if container has such an attribute. + validationFilter = context.Key.ContainerType.GetTypeInfo() .GetCustomAttributes(inherit: true) - .OfType() + .OfType() .FirstOrDefault(); } - context.ValidationMetadata.ShouldValidate = shouldValidate; + context.ValidationMetadata.PropertyValidationFilter = validationFilter; } } } diff --git a/src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/BindNeverAttribute.cs b/src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/BindNeverAttribute.cs index 6c1c962973..3bcdfab9ff 100644 --- a/src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/BindNeverAttribute.cs +++ b/src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/BindNeverAttribute.cs @@ -7,8 +7,8 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding { /// /// Indicates that a property should be excluded from model binding. When applied to a property, the model binding - /// system excludes that property. When applied to a type, the model binding system excludes all properties within - /// that type. + /// system excludes that property. When applied to a type, the model binding system excludes all properties that + /// type defines. /// [AttributeUsage(AttributeTargets.Class | AttributeTargets.Property, AllowMultiple = false, Inherited = true)] public sealed class BindNeverAttribute : BindingBehaviorAttribute diff --git a/src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/BindRequiredAttribute.cs b/src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/BindRequiredAttribute.cs index 7c7ccf5530..b98f120e12 100644 --- a/src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/BindRequiredAttribute.cs +++ b/src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/BindRequiredAttribute.cs @@ -8,7 +8,7 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding /// /// Indicates that a property is required for model binding. When applied to a property, the model binding system /// requires a value for that property. When applied to a type, the model binding system requires values for all - /// properties within that type. + /// properties that type defines. /// [AttributeUsage(AttributeTargets.Class | AttributeTargets.Property, AllowMultiple = false, Inherited = true)] public sealed class BindRequiredAttribute : BindingBehaviorAttribute diff --git a/src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/Metadata/DefaultModelMetadata.cs b/src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/Metadata/DefaultModelMetadata.cs index 401eb18e2d..f9ef945887 100644 --- a/src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/Metadata/DefaultModelMetadata.cs +++ b/src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/Metadata/DefaultModelMetadata.cs @@ -533,11 +533,11 @@ public override string TemplateHint } /// - public override IShouldValidate ShouldValidate + public override IPropertyValidationFilter PropertyValidationFilter { get { - return ValidationMetadata.ShouldValidate; + return ValidationMetadata.PropertyValidationFilter; } } diff --git a/src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/Metadata/ValidationMetadata.cs b/src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/Metadata/ValidationMetadata.cs index 5b2c46f019..bd4ff327aa 100644 --- a/src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/Metadata/ValidationMetadata.cs +++ b/src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/Metadata/ValidationMetadata.cs @@ -20,10 +20,10 @@ public class ValidationMetadata public bool? IsRequired { get; set; } /// - /// Gets or sets an implementation that indicates whether this model should be - /// validated. See . + /// Gets or sets an implementation that indicates whether this model + /// should be validated. See . /// - public IShouldValidate ShouldValidate { get; set; } + public IPropertyValidationFilter PropertyValidationFilter { get; set; } /// /// Gets or sets a value that indicates whether children of the model should be validated. If null diff --git a/src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/Validation/ValidateNeverAttribute.cs b/src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/Validation/ValidateNeverAttribute.cs index e11a9355fc..a0c7181f3f 100644 --- a/src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/Validation/ValidateNeverAttribute.cs +++ b/src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/Validation/ValidateNeverAttribute.cs @@ -6,12 +6,12 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding.Validation { /// - /// 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. + /// 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. /// [AttributeUsage(AttributeTargets.Class | AttributeTargets.Property, AllowMultiple = false, Inherited = true)] - public sealed class ValidateNeverAttribute : Attribute, IShouldValidate + public sealed class ValidateNeverAttribute : Attribute, IPropertyValidationFilter { /// public bool ShouldValidateEntry(ValidationEntry entry, ValidationEntry parentEntry) diff --git a/src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/Validation/ValidationVisitor.cs b/src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/Validation/ValidationVisitor.cs index a2f52d96c1..356a51800c 100644 --- a/src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/Validation/ValidationVisitor.cs +++ b/src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/Validation/ValidationVisitor.cs @@ -246,7 +246,7 @@ private bool VisitChildren(IValidationStrategy strategy) var entry = enumerator.Current; var metadata = entry.Metadata; var key = entry.Key; - if (metadata.ShouldValidate?.ShouldValidateEntry(entry, parentEntry) == false) + if (metadata.PropertyValidationFilter?.ShouldValidateEntry(entry, parentEntry) == false) { SuppressValidation(key); continue; diff --git a/test/Microsoft.AspNetCore.Mvc.Abstractions.Test/ModelBinding/ModelMetadataTest.cs b/test/Microsoft.AspNetCore.Mvc.Abstractions.Test/ModelBinding/ModelMetadataTest.cs index 9b01691add..1f93ca93f0 100644 --- a/test/Microsoft.AspNetCore.Mvc.Abstractions.Test/ModelBinding/ModelMetadataTest.cs +++ b/test/Microsoft.AspNetCore.Mvc.Abstractions.Test/ModelBinding/ModelMetadataTest.cs @@ -577,7 +577,7 @@ public override string TemplateHint } } - public override IShouldValidate ShouldValidate + public override IPropertyValidationFilter PropertyValidationFilter { get { diff --git a/test/Microsoft.AspNetCore.Mvc.Core.Test/ModelBinding/Metadata/DefaultModelMetadataTest.cs b/test/Microsoft.AspNetCore.Mvc.Core.Test/ModelBinding/Metadata/DefaultModelMetadataTest.cs index 58f06e2819..62a0bfbcca 100644 --- a/test/Microsoft.AspNetCore.Mvc.Core.Test/ModelBinding/Metadata/DefaultModelMetadataTest.cs +++ b/test/Microsoft.AspNetCore.Mvc.Core.Test/ModelBinding/Metadata/DefaultModelMetadataTest.cs @@ -62,7 +62,7 @@ public void DefaultValues() Assert.Null(metadata.EnumGroupedDisplayNamesAndValues); Assert.Null(metadata.EnumNamesAndValues); Assert.Null(metadata.NullDisplayText); - Assert.Null(metadata.ShouldValidate); + Assert.Null(metadata.PropertyValidationFilter); Assert.Null(metadata.SimpleDisplayProperty); Assert.Null(metadata.Placeholder); Assert.Null(metadata.TemplateHint); @@ -662,11 +662,11 @@ public void ValidateChildren_ComplexAndEnumerableTypes(Type modelType) Assert.True(validateChildren); } - public static TheoryData ShouldValidateData + public static TheoryData ValidationFilterData { get { - return new TheoryData + return new TheoryData { null, new ValidateNeverAttribute(), @@ -675,8 +675,8 @@ public static TheoryData ShouldValidateData } [Theory] - [MemberData(nameof(ShouldValidateData))] - public void ShouldValidate_ReflectsShouldValidate_FromValidationMetadata(IShouldValidate value) + [MemberData(nameof(ValidationFilterData))] + public void PropertyValidationFilter_ReflectsFilter_FromValidationMetadata(IPropertyValidationFilter value) { // Arrange var detailsProvider = new EmptyCompositeMetadataDetailsProvider(); @@ -686,16 +686,16 @@ public void ShouldValidate_ReflectsShouldValidate_FromValidationMetadata(IShould var cache = new DefaultMetadataDetails(key, new ModelAttributes(new object[0])); cache.ValidationMetadata = new ValidationMetadata { - ShouldValidate = value, + PropertyValidationFilter = value, }; var metadata = new DefaultModelMetadata(provider, detailsProvider, cache); // Act - var shouldValidate = metadata.ShouldValidate; + var validationFilter = metadata.PropertyValidationFilter; // Assert - Assert.Same(value, shouldValidate); + Assert.Same(value, validationFilter); } [Fact] diff --git a/test/Microsoft.AspNetCore.Mvc.Core.Test/ModelBinding/Metadata/DefaultValidationMetadataProviderTest.cs b/test/Microsoft.AspNetCore.Mvc.Core.Test/ModelBinding/Metadata/DefaultValidationMetadataProviderTest.cs index 7970d3baaa..16fe4f3b37 100644 --- a/test/Microsoft.AspNetCore.Mvc.Core.Test/ModelBinding/Metadata/DefaultValidationMetadataProviderTest.cs +++ b/test/Microsoft.AspNetCore.Mvc.Core.Test/ModelBinding/Metadata/DefaultValidationMetadataProviderTest.cs @@ -12,7 +12,7 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding.Metadata public class DefaultValidationMetadataProviderTest { [Fact] - public void ShouldValidate_ShouldValidateEntry_False_IfPropertyHasValidateNever() + public void PropertyValidationFilter_ShouldValidateEntry_False_IfPropertyHasValidateNever() { // Arrange var provider = new DefaultValidationMetadataProvider(); @@ -25,14 +25,14 @@ public void ShouldValidate_ShouldValidateEntry_False_IfPropertyHasValidateNever( provider.CreateValidationMetadata(context); // Assert - Assert.NotNull(context.ValidationMetadata.ShouldValidate); - Assert.False(context.ValidationMetadata.ShouldValidate.ShouldValidateEntry( + Assert.NotNull(context.ValidationMetadata.PropertyValidationFilter); + Assert.False(context.ValidationMetadata.PropertyValidationFilter.ShouldValidateEntry( new ValidationEntry(), new ValidationEntry())); } [Fact] - public void ShouldValidate_Null_IfPropertyHasValidateNeverOnItsType() + public void PropertyValidationFilter_Null_IfPropertyHasValidateNeverOnItsType() { // Arrange var provider = new DefaultValidationMetadataProvider(); @@ -45,11 +45,11 @@ public void ShouldValidate_Null_IfPropertyHasValidateNeverOnItsType() provider.CreateValidationMetadata(context); // Assert - Assert.Null(context.ValidationMetadata.ShouldValidate); + Assert.Null(context.ValidationMetadata.PropertyValidationFilter); } [Fact] - public void ShouldValidate_Null_ForType() + public void PropertyValidationFilter_Null_ForType() { // Arrange var provider = new DefaultValidationMetadataProvider(); @@ -62,11 +62,11 @@ public void ShouldValidate_Null_ForType() provider.CreateValidationMetadata(context); // Assert - Assert.Null(context.ValidationMetadata.ShouldValidate); + Assert.Null(context.ValidationMetadata.PropertyValidationFilter); } [Fact] - public void ShouldValidate_ShouldValidateEntry_False_IfContainingTypeHasValidateNever() + public void PropertyValidationFilter_ShouldValidateEntry_False_IfContainingTypeHasValidateNever() { // Arrange var provider = new DefaultValidationMetadataProvider(); @@ -81,14 +81,14 @@ public void ShouldValidate_ShouldValidateEntry_False_IfContainingTypeHasValidate provider.CreateValidationMetadata(context); // Assert - Assert.NotNull(context.ValidationMetadata.ShouldValidate); - Assert.False(context.ValidationMetadata.ShouldValidate.ShouldValidateEntry( + Assert.NotNull(context.ValidationMetadata.PropertyValidationFilter); + Assert.False(context.ValidationMetadata.PropertyValidationFilter.ShouldValidateEntry( new ValidationEntry(), new ValidationEntry())); } [Fact] - public void ShouldValidate_ShouldValidateEntry_False_IfContainingTypeInheritsValidateNever() + public void PropertyValidationFilter_ShouldValidateEntry_False_IfContainingTypeInheritsValidateNever() { // Arrange var provider = new DefaultValidationMetadataProvider(); @@ -103,8 +103,8 @@ public void ShouldValidate_ShouldValidateEntry_False_IfContainingTypeInheritsVal provider.CreateValidationMetadata(context); // Assert - Assert.NotNull(context.ValidationMetadata.ShouldValidate); - Assert.False(context.ValidationMetadata.ShouldValidate.ShouldValidateEntry( + Assert.NotNull(context.ValidationMetadata.PropertyValidationFilter); + Assert.False(context.ValidationMetadata.PropertyValidationFilter.ShouldValidateEntry( new ValidationEntry(), new ValidationEntry())); } diff --git a/test/Microsoft.AspNetCore.Mvc.IntegrationTests/ValidationIntegrationTests.cs b/test/Microsoft.AspNetCore.Mvc.IntegrationTests/ValidationIntegrationTests.cs index fc69907540..6f66a9e0e2 100644 --- a/test/Microsoft.AspNetCore.Mvc.IntegrationTests/ValidationIntegrationTests.cs +++ b/test/Microsoft.AspNetCore.Mvc.IntegrationTests/ValidationIntegrationTests.cs @@ -1448,7 +1448,7 @@ public async Task PropertyWithinValidateNeverType_IsSkipped(string propertyName) Assert.Equal(ModelValidationState.Skipped, state.ValidationState); } - private class ValidateSometimesAttribute : Attribute, IShouldValidate + private class ValidateSometimesAttribute : Attribute, IPropertyValidationFilter { private readonly string _otherProperty;