diff --git a/src/Microsoft.AspNet.Mvc.Abstractions/ModelBinding/Validation/IModelValidator.cs b/src/Microsoft.AspNet.Mvc.Abstractions/ModelBinding/Validation/IModelValidator.cs index 0657a98ea9..3c5bcb920f 100644 --- a/src/Microsoft.AspNet.Mvc.Abstractions/ModelBinding/Validation/IModelValidator.cs +++ b/src/Microsoft.AspNet.Mvc.Abstractions/ModelBinding/Validation/IModelValidator.cs @@ -10,12 +10,6 @@ namespace Microsoft.AspNet.Mvc.ModelBinding.Validation /// public interface IModelValidator { - /// - /// Gets a value indicating whether or not this validator validates that a required value - /// has been provided for the model. - /// - bool IsRequired { get; } - /// /// Validates the model value. /// diff --git a/src/Microsoft.AspNet.Mvc.Abstractions/ModelBinding/Validation/ModelValiatorProviderContext.cs b/src/Microsoft.AspNet.Mvc.Abstractions/ModelBinding/Validation/ModelValiatorProviderContext.cs index 96f6b233ea..25a37d8baa 100644 --- a/src/Microsoft.AspNet.Mvc.Abstractions/ModelBinding/Validation/ModelValiatorProviderContext.cs +++ b/src/Microsoft.AspNet.Mvc.Abstractions/ModelBinding/Validation/ModelValiatorProviderContext.cs @@ -22,7 +22,7 @@ namespace Microsoft.AspNet.Mvc.ModelBinding.Validation /// /// Gets the . /// - public ModelMetadata ModelMetadata { get; } + public ModelMetadata ModelMetadata { get; set; } /// /// Gets the validator metadata. diff --git a/src/Microsoft.AspNet.Mvc.Core/ModelBinding/Validation/ValidationVisitor.cs b/src/Microsoft.AspNet.Mvc.Core/ModelBinding/Validation/ValidationVisitor.cs index 1f88660da3..e232caad79 100644 --- a/src/Microsoft.AspNet.Mvc.Core/ModelBinding/Validation/ValidationVisitor.cs +++ b/src/Microsoft.AspNet.Mvc.Core/ModelBinding/Validation/ValidationVisitor.cs @@ -3,7 +3,6 @@ using System; using System.Collections.Generic; -using System.Linq; using System.Runtime.CompilerServices; namespace Microsoft.AspNet.Mvc.ModelBinding.Validation @@ -23,6 +22,7 @@ namespace Microsoft.AspNet.Mvc.ModelBinding.Validation private string _key; private object _model; private ModelMetadata _metadata; + private ModelValidatorProviderContext _context; private IValidationStrategy _strategy; private HashSet _currentPath; @@ -94,7 +94,7 @@ namespace Microsoft.AspNet.Mvc.ModelBinding.Validation var state = _modelState.GetValidationState(_key); if (state == ModelValidationState.Unvalidated) { - var validators = GetValidators(_metadata); + var validators = GetValidators(); var count = validators.Count; if (count > 0) @@ -255,11 +255,23 @@ namespace Microsoft.AspNet.Mvc.ModelBinding.Validation return isValid; } - private IList GetValidators(ModelMetadata metadata) + private IList GetValidators() { - var context = new ModelValidatorProviderContext(metadata); - _validatorProvider.GetValidators(context); - return context.Validators.OrderBy(v => v, ValidatorOrderComparer.Instance).ToList(); + if (_context == null) + { + _context = new ModelValidatorProviderContext(_metadata); + } + else + { + // Reusing the context so we don't allocate a new context and list + // for every property that gets validated. + _context.ModelMetadata = _metadata; + _context.Validators.Clear(); + } + + _validatorProvider.GetValidators(_context); + + return _context.Validators; } private void SuppressValidation(string key) @@ -348,19 +360,5 @@ namespace Microsoft.AspNet.Mvc.ModelBinding.Validation _visitor._currentPath.Remove(_newModel); } } - - // Sorts validators based on whether or not they are 'required'. We want to run - // 'required' validators first so that we get the best possible error message. - private class ValidatorOrderComparer : IComparer - { - public static readonly ValidatorOrderComparer Instance = new ValidatorOrderComparer(); - - public int Compare(IModelValidator x, IModelValidator y) - { - var xScore = x.IsRequired ? 0 : 1; - var yScore = y.IsRequired ? 0 : 1; - return xScore.CompareTo(yScore); - } - } } } diff --git a/src/Microsoft.AspNet.Mvc.DataAnnotations/DataAnnotationsModelValidator.cs b/src/Microsoft.AspNet.Mvc.DataAnnotations/DataAnnotationsModelValidator.cs index c2d5a16728..b4c73c0455 100644 --- a/src/Microsoft.AspNet.Mvc.DataAnnotations/DataAnnotationsModelValidator.cs +++ b/src/Microsoft.AspNet.Mvc.DataAnnotations/DataAnnotationsModelValidator.cs @@ -26,11 +26,6 @@ namespace Microsoft.AspNet.Mvc.ModelBinding.Validation public ValidationAttribute Attribute { get; } - public bool IsRequired - { - get { return Attribute is RequiredAttribute; } - } - public IEnumerable Validate(ModelValidationContext validationContext) { var metadata = validationContext.Metadata; diff --git a/src/Microsoft.AspNet.Mvc.DataAnnotations/DataAnnotationsModelValidatorProvider.cs b/src/Microsoft.AspNet.Mvc.DataAnnotations/DataAnnotationsModelValidatorProvider.cs index a0d871bc74..719b1e3a39 100644 --- a/src/Microsoft.AspNet.Mvc.DataAnnotations/DataAnnotationsModelValidatorProvider.cs +++ b/src/Microsoft.AspNet.Mvc.DataAnnotations/DataAnnotationsModelValidatorProvider.cs @@ -1,9 +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.ComponentModel.DataAnnotations; -using System.Linq; using System.Reflection; using Microsoft.Extensions.Localization; using Microsoft.Extensions.OptionsModel; @@ -43,9 +41,26 @@ namespace Microsoft.AspNet.Mvc.ModelBinding.Validation _stringLocalizerFactory); } - foreach (var attribute in context.ValidatorMetadata.OfType()) + for (var i = 0; i < context.ValidatorMetadata.Count; i++) { - context.Validators.Add(new DataAnnotationsModelValidator(attribute, stringLocalizer)); + var attribute = context.ValidatorMetadata[i] as ValidationAttribute; + if (attribute == null) + { + continue; + } + + var validator = new DataAnnotationsModelValidator(attribute, stringLocalizer); + + // Inserts validators based on whether or not they are 'required'. We want to run + // 'required' validators first so that we get the best possible error message. + if (attribute is RequiredAttribute) + { + context.Validators.Insert(0, validator); + } + else + { + context.Validators.Add(validator); + } } // Produce a validator if the type supports IValidatableObject diff --git a/src/Microsoft.AspNet.Mvc.DataAnnotations/ValidatableObjectAdapter.cs b/src/Microsoft.AspNet.Mvc.DataAnnotations/ValidatableObjectAdapter.cs index c43cd8704e..fad8f98f7e 100644 --- a/src/Microsoft.AspNet.Mvc.DataAnnotations/ValidatableObjectAdapter.cs +++ b/src/Microsoft.AspNet.Mvc.DataAnnotations/ValidatableObjectAdapter.cs @@ -11,11 +11,6 @@ namespace Microsoft.AspNet.Mvc.ModelBinding.Validation { public class ValidatableObjectAdapter : IModelValidator { - public bool IsRequired - { - get { return false; } - } - public IEnumerable Validate(ModelValidationContext context) { var model = context.Model; diff --git a/test/Microsoft.AspNet.Mvc.Core.Test/ModelBinding/Metadata/DefaultValidationMetadataTest.cs b/test/Microsoft.AspNet.Mvc.Core.Test/ModelBinding/Metadata/DefaultValidationMetadataTest.cs index b9923c34b9..b1bbf1b768 100644 --- a/test/Microsoft.AspNet.Mvc.Core.Test/ModelBinding/Metadata/DefaultValidationMetadataTest.cs +++ b/test/Microsoft.AspNet.Mvc.Core.Test/ModelBinding/Metadata/DefaultValidationMetadataTest.cs @@ -70,14 +70,6 @@ namespace Microsoft.AspNet.Mvc.ModelBinding.Metadata private class TestModelValidationAttribute : Attribute, IModelValidator { - public bool IsRequired - { - get - { - throw new NotImplementedException(); - } - } - public IEnumerable Validate(ModelValidationContext context) { throw new NotImplementedException(); @@ -94,14 +86,6 @@ namespace Microsoft.AspNet.Mvc.ModelBinding.Metadata private class TestValidationAttribute : Attribute, IModelValidator, IClientModelValidator { - public bool IsRequired - { - get - { - throw new NotImplementedException(); - } - } - public IEnumerable GetClientValidationRules(ClientModelValidationContext context) { throw new NotImplementedException(); diff --git a/test/Microsoft.AspNet.Mvc.Core.Test/ModelBinding/Validation/DefaultModelValidatorProviderTest.cs b/test/Microsoft.AspNet.Mvc.Core.Test/ModelBinding/Validation/DefaultModelValidatorProviderTest.cs index a03bb010fa..5201e92c4b 100644 --- a/test/Microsoft.AspNet.Mvc.Core.Test/ModelBinding/Validation/DefaultModelValidatorProviderTest.cs +++ b/test/Microsoft.AspNet.Mvc.Core.Test/ModelBinding/Validation/DefaultModelValidatorProviderTest.cs @@ -171,14 +171,6 @@ namespace Microsoft.AspNet.Mvc.ModelBinding.Validation { public string Tag { get; set; } - public bool IsRequired - { - get - { - throw new NotImplementedException(); - } - } - public IEnumerable Validate(ModelValidationContext context) { throw new NotImplementedException(); diff --git a/test/Microsoft.AspNet.Mvc.DataAnnotations.Test/DataAnnotationsModelValidatorProviderTest.cs b/test/Microsoft.AspNet.Mvc.DataAnnotations.Test/DataAnnotationsModelValidatorProviderTest.cs index 68b3b84c55..eb9982a2ff 100644 --- a/test/Microsoft.AspNet.Mvc.DataAnnotations.Test/DataAnnotationsModelValidatorProviderTest.cs +++ b/test/Microsoft.AspNet.Mvc.DataAnnotations.Test/DataAnnotationsModelValidatorProviderTest.cs @@ -38,6 +38,27 @@ namespace Microsoft.AspNet.Mvc.ModelBinding.Validation } #endif + [Fact] + public void GetValidators_InsertsRequiredValidatorsFirst() + { + var provider = new DataAnnotationsModelValidatorProvider( + new TestOptionsManager(), + stringLocalizerFactory: null); + var metadata = _metadataProvider.GetMetadataForProperty( + typeof(ClassWithProperty), + "PropertyWithMultipleValidationAttributes"); + + var providerContext = new ModelValidatorProviderContext(metadata); + + // Act + provider.GetValidators(providerContext); + + // Assert + Assert.Equal(4, providerContext.Validators.Count); + Assert.IsAssignableFrom(((DataAnnotationsModelValidator)providerContext.Validators[0]).Attribute); + Assert.IsAssignableFrom(((DataAnnotationsModelValidator)providerContext.Validators[1]).Attribute); + } + [Fact] public void UnknownValidationAttributeGetsDefaultAdapter() { @@ -130,5 +151,30 @@ namespace Microsoft.AspNet.Mvc.ModelBinding.Validation public int WithoutAttribute { get; set; } } + + private class ClassWithProperty + { + [CustomNonRequiredAttribute1] + [CustomNonRequiredAttribute2] + [CustomRequiredAttribute1] + [CustomRequiredAttribute2] + public string PropertyWithMultipleValidationAttributes { get; set; } + } + + public class CustomRequiredAttribute1 : RequiredAttribute + { + } + + public class CustomRequiredAttribute2 : RequiredAttribute + { + } + + public class CustomNonRequiredAttribute1 : ValidationAttribute + { + } + + public class CustomNonRequiredAttribute2 : ValidationAttribute + { + } } } diff --git a/test/Microsoft.AspNet.Mvc.DataAnnotations.Test/DataAnnotationsModelValidatorTest.cs b/test/Microsoft.AspNet.Mvc.DataAnnotations.Test/DataAnnotationsModelValidatorTest.cs index 8d871b03d4..46cfa16039 100644 --- a/test/Microsoft.AspNet.Mvc.DataAnnotations.Test/DataAnnotationsModelValidatorTest.cs +++ b/test/Microsoft.AspNet.Mvc.DataAnnotations.Test/DataAnnotationsModelValidatorTest.cs @@ -294,17 +294,6 @@ namespace Microsoft.AspNet.Mvc.ModelBinding.Validation } #endif - [Fact] - public void IsRequiredTests() - { - // Arrange & Act & Assert - Assert.False(new DataAnnotationsModelValidator(new RangeAttribute(10, 20), stringLocalizer: null) - .IsRequired); - Assert.True(new DataAnnotationsModelValidator(new RequiredAttribute(), stringLocalizer: null).IsRequired); - Assert.True(new DataAnnotationsModelValidator(new DerivedRequiredAttribute(), stringLocalizer: null) - .IsRequired); - } - private class DerivedRequiredAttribute : RequiredAttribute { } diff --git a/test/Microsoft.AspNet.Mvc.DataAnnotations.Test/DefaultModelClientValidatorProviderTest.cs b/test/Microsoft.AspNet.Mvc.DataAnnotations.Test/DefaultModelClientValidatorProviderTest.cs index ae6a7b3642..43b3878280 100644 --- a/test/Microsoft.AspNet.Mvc.DataAnnotations.Test/DefaultModelClientValidatorProviderTest.cs +++ b/test/Microsoft.AspNet.Mvc.DataAnnotations.Test/DefaultModelClientValidatorProviderTest.cs @@ -218,14 +218,6 @@ namespace Microsoft.AspNet.Mvc.ModelBinding.Validation { public string Tag { get; set; } - public bool IsRequired - { - get - { - throw new NotImplementedException(); - } - } - public IEnumerable Validate(ModelValidationContext context) { throw new NotImplementedException();