From fa56df93c3e8eb335b8f9dd6a7cf66a9fa572d72 Mon Sep 17 00:00:00 2001 From: Ryan Nowak Date: Mon, 18 May 2015 15:27:56 -0700 Subject: [PATCH] Fix #2407 - Part 1 - Make model binding behavior for [Required] compatible with MVC5. This change removes the behavior in model binding to validate values 'on the wire' for requiredness instead of the looking at the model. This restores the behavior of [Required] for model binding to the MVC5 semantics. --- .../ModelBinding/MutableObjectModelBinder.cs | 80 +--- .../Validation/DefaultObjectValidator.cs | 20 +- .../MutableObjectModelBinderTest.cs | 396 +++++++++--------- .../ModelBindingFromHeaderTest.cs | 7 +- ...rsWebSite.MvcTagHelper_Customer.Index.html | 5 +- ...elpersWebSite.Employee.Create.Invalid.html | 2 +- .../TagHelpersWebSite/Models/Employee.cs | 2 +- 7 files changed, 240 insertions(+), 272 deletions(-) diff --git a/src/Microsoft.AspNet.Mvc.Core/ModelBinding/MutableObjectModelBinder.cs b/src/Microsoft.AspNet.Mvc.Core/ModelBinding/MutableObjectModelBinder.cs index 0fd66a0f25..6cf9707ad1 100644 --- a/src/Microsoft.AspNet.Mvc.Core/ModelBinding/MutableObjectModelBinder.cs +++ b/src/Microsoft.AspNet.Mvc.Core/ModelBinding/MutableObjectModelBinder.cs @@ -349,17 +349,6 @@ namespace Microsoft.AspNet.Mvc.ModelBinding { validationInfo.RequiredProperties.Add(propertyName); } - - var validatorProviderContext = new ModelValidatorProviderContext(propertyMetadata); - bindingContext.OperationBindingContext.ValidatorProvider.GetValidators(validatorProviderContext); - - var requiredValidator = validatorProviderContext.Validators - .FirstOrDefault(v => v != null && v.IsRequired); - if (requiredValidator != null) - { - validationInfo.RequiredValidators[propertyName] = requiredValidator; - validationInfo.RequiredProperties.Add(propertyName); - } } return validationInfo; @@ -393,15 +382,18 @@ namespace Microsoft.AspNet.Mvc.ModelBinding bindingContext.ModelName, propertyName); - // Execute validator (if any) to get custom error message. - IModelValidator validator; - if (validationInfo.RequiredValidators.TryGetValue(missingRequiredProperty, out validator)) + // Get the first 'required' validator (if any) to get custom error message. + var validatorProviderContext = new ModelValidatorProviderContext(propertyExplorer.Metadata); + bindingContext.OperationBindingContext.ValidatorProvider.GetValidators(validatorProviderContext); + var validator = validatorProviderContext.Validators.FirstOrDefault(v => v.IsRequired); + + if (validator != null) { addedError = RunValidator(validator, bindingContext, propertyExplorer, modelStateKey); } - // Fall back to default message if BindingBehaviorAttribute required this property or validator - // (oddly) succeeded. + // Fall back to default message if BindingBehaviorAttribute required this property and we have no + // actual validator for it. if (!addedError) { bindingContext.ModelState.TryAddModelError( @@ -418,12 +410,7 @@ namespace Microsoft.AspNet.Mvc.ModelBinding if (dtoResult != null) { var propertyMetadata = entry.Key; - IModelValidator requiredValidator; - validationInfo.RequiredValidators.TryGetValue( - propertyMetadata.PropertyName, - out requiredValidator); - - SetProperty(bindingContext, modelExplorer, propertyMetadata, dtoResult, requiredValidator); + SetProperty(bindingContext, modelExplorer, propertyMetadata, dtoResult); var dtoValidationNode = dtoResult.ValidationNode; if (dtoValidationNode == null) @@ -458,8 +445,7 @@ namespace Microsoft.AspNet.Mvc.ModelBinding [NotNull] ModelBindingContext bindingContext, [NotNull] ModelExplorer modelExplorer, [NotNull] ModelMetadata propertyMetadata, - [NotNull] ModelBindingResult dtoResult, - IModelValidator requiredValidator) + [NotNull] ModelBindingResult dtoResult) { var bindingFlags = BindingFlags.Instance | BindingFlags.Public | BindingFlags.IgnoreCase; var property = bindingContext.ModelType.GetProperty( @@ -485,26 +471,6 @@ namespace Microsoft.AspNet.Mvc.ModelBinding value = dtoResult.Model; } - // 'Required' validators need to run first so that we can provide useful error messages if - // the property setters throw, e.g. if we're setting entity keys to null. - if (value == null) - { - var modelStateKey = dtoResult.Key; - var validationState = bindingContext.ModelState.GetFieldValidationState(modelStateKey); - if (validationState == ModelValidationState.Unvalidated) - { - if (requiredValidator != null) - { - var propertyExplorer = modelExplorer.GetExplorerForExpression(propertyMetadata, model: null); - var validationContext = new ModelValidationContext(bindingContext, propertyExplorer); - foreach (var validationResult in requiredValidator.Validate(validationContext)) - { - bindingContext.ModelState.TryAddModelError(modelStateKey, validationResult.Message); - } - } - } - } - if (!dtoResult.IsModelSet) { // If we don't have a value, don't set it on the model and trounce a pre-initialized @@ -512,31 +478,16 @@ namespace Microsoft.AspNet.Mvc.ModelBinding return; } - if (value != null || TypeHelper.AllowsNullValue(property.PropertyType)) + try { - try - { - propertyMetadata.PropertySetter(bindingContext.Model, value); - } - catch (Exception exception) - { - AddModelError(exception, bindingContext, dtoResult); - } + propertyMetadata.PropertySetter(bindingContext.Model, value); } - else + catch (Exception exception) { - // trying to set a non-nullable value type to null, need to make sure there's a message - var modelStateKey = dtoResult.Key; - var validationState = bindingContext.ModelState.GetFieldValidationState(modelStateKey); - if (validationState == ModelValidationState.Unvalidated) - { - var errorMessage = Resources.ModelBinding_ValueRequired; - bindingContext.ModelState.TryAddModelError(modelStateKey, errorMessage); - } + AddModelError(exception, bindingContext, dtoResult); } } - // Neither [DefaultValue] nor [Required] is relevant for a non-settable collection. private void AddToProperty( ModelBindingContext bindingContext, ModelExplorer modelExplorer, @@ -642,14 +593,11 @@ namespace Microsoft.AspNet.Mvc.ModelBinding public PropertyValidationInfo() { RequiredProperties = new HashSet(StringComparer.OrdinalIgnoreCase); - RequiredValidators = new Dictionary(StringComparer.OrdinalIgnoreCase); SkipProperties = new HashSet(StringComparer.OrdinalIgnoreCase); } public HashSet RequiredProperties { get; private set; } - public Dictionary RequiredValidators { get; private set; } - public HashSet SkipProperties { get; private set; } } } diff --git a/src/Microsoft.AspNet.Mvc.Core/ModelBinding/Validation/DefaultObjectValidator.cs b/src/Microsoft.AspNet.Mvc.Core/ModelBinding/Validation/DefaultObjectValidator.cs index 571b17db05..57fd2d6f11 100644 --- a/src/Microsoft.AspNet.Mvc.Core/ModelBinding/Validation/DefaultObjectValidator.cs +++ b/src/Microsoft.AspNet.Mvc.Core/ModelBinding/Validation/DefaultObjectValidator.cs @@ -153,7 +153,11 @@ namespace Microsoft.AspNet.Mvc.ModelBinding.Validation { var validatorProviderContext = new ModelValidatorProviderContext(metadata); provider.GetValidators(validatorProviderContext); - return validatorProviderContext.Validators; + + return validatorProviderContext + .Validators + .OrderBy(v => v, ValidatorOrderComparer.Instance) + .ToList(); } private bool ValidateChildNodes( @@ -345,5 +349,19 @@ namespace Microsoft.AspNet.Mvc.ModelBinding.Validation public ModelValidationNode ValidationNode { get; set; } } + + // 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/test/Microsoft.AspNet.Mvc.Core.Test/ModelBinding/MutableObjectModelBinderTest.cs b/test/Microsoft.AspNet.Mvc.Core.Test/ModelBinding/MutableObjectModelBinderTest.cs index 2c8d23e119..2910113789 100644 --- a/test/Microsoft.AspNet.Mvc.Core.Test/ModelBinding/MutableObjectModelBinderTest.cs +++ b/test/Microsoft.AspNet.Mvc.Core.Test/ModelBinding/MutableObjectModelBinderTest.cs @@ -849,7 +849,7 @@ namespace Microsoft.AspNet.Mvc.ModelBinding [Fact] [ReplaceCulture] - public void ProcessDto_BindRequiredFieldNull_RaisesModelError() + public void ProcessDto_ValueTypePropertyWithBindRequired_SetToNull_CapturesException() { // Arrange var model = new ModelWithBindRequired @@ -905,14 +905,13 @@ namespace Microsoft.AspNet.Mvc.ModelBinding Assert.Equal(ModelValidationState.Invalid, modelState.ValidationState); var modelError = Assert.Single(modelState.Errors); - Assert.Null(modelError.Exception); - Assert.NotNull(modelError.ErrorMessage); - Assert.Equal("A value is required.", modelError.ErrorMessage); + Assert.Equal(string.Empty, modelError.ErrorMessage); + Assert.IsType(modelError.Exception); } [Fact] [ReplaceCulture] - public void ProcessDto_RequiredFieldMissing_RaisesModelError() + public void ProcessDto_MissingDataForRequiredFields_NoErrors() { // Arrange var model = new ModelWithRequired(); @@ -930,32 +929,13 @@ namespace Microsoft.AspNet.Mvc.ModelBinding // Assert var modelStateDictionary = bindingContext.ModelState; - Assert.False(modelStateDictionary.IsValid); - Assert.Equal(2, modelStateDictionary.Count); - - // Check Age error. - ModelState modelState; - Assert.True(modelStateDictionary.TryGetValue("theModel." + nameof(ModelWithRequired.Age), out modelState)); - - var modelError = Assert.Single(modelState.Errors); - Assert.Null(modelError.Exception); - Assert.NotNull(modelError.ErrorMessage); - var expected = ValidationAttributeUtil.GetRequiredErrorMessage(nameof(ModelWithRequired.Age)); - Assert.Equal(expected, modelError.ErrorMessage); - - // Check City error. - Assert.True(modelStateDictionary.TryGetValue("theModel." + nameof(ModelWithRequired.City), out modelState)); - - modelError = Assert.Single(modelState.Errors); - Assert.Null(modelError.Exception); - Assert.NotNull(modelError.ErrorMessage); - expected = ValidationAttributeUtil.GetRequiredErrorMessage(nameof(ModelWithRequired.City)); - Assert.Equal(expected, modelError.ErrorMessage); + Assert.True(modelStateDictionary.IsValid); + Assert.Empty(modelStateDictionary); } [Fact] [ReplaceCulture] - public void ProcessDto_RequiredFieldNull_RaisesModelError() + public void ProcessDto_ValueTypeProperty_WithRequiredAttribute_SetToNull_NoError() { // Arrange var model = new ModelWithRequired(); @@ -984,22 +964,12 @@ namespace Microsoft.AspNet.Mvc.ModelBinding // Assert var modelStateDictionary = bindingContext.ModelState; - Assert.False(modelStateDictionary.IsValid); - Assert.Single(modelStateDictionary); - - // Check City error. - ModelState modelState; - Assert.True(modelStateDictionary.TryGetValue("theModel." + nameof(ModelWithRequired.City), out modelState)); - - var modelError = Assert.Single(modelState.Errors); - Assert.Null(modelError.Exception); - Assert.NotNull(modelError.ErrorMessage); - var expected = ValidationAttributeUtil.GetRequiredErrorMessage(nameof(ModelWithRequired.City)); - Assert.Equal(expected, modelError.ErrorMessage); + Assert.True(modelStateDictionary.IsValid); + Assert.Empty(modelStateDictionary); } [Fact] - public void ProcessDto_RequiredFieldMissing_RaisesModelErrorWithMessage() + public void ProcessDto_PropertyWithRequiredAttribute_NoPropertiesSet_NoError() { // Arrange var model = new Person(); @@ -1016,42 +986,11 @@ namespace Microsoft.AspNet.Mvc.ModelBinding // Assert var modelStateDictionary = bindingContext.ModelState; - Assert.False(modelStateDictionary.IsValid); - Assert.Equal(2, modelStateDictionary.Count); - - // Check ValueTypeRequired error. - var modelStateEntry = Assert.Single( - modelStateDictionary, - entry => entry.Key == "theModel." + nameof(Person.ValueTypeRequired)); - Assert.Equal("theModel." + nameof(Person.ValueTypeRequired), modelStateEntry.Key); - - var modelState = modelStateEntry.Value; - Assert.Equal(ModelValidationState.Invalid, modelState.ValidationState); - - var modelError = Assert.Single(modelState.Errors); - Assert.Null(modelError.Exception); - Assert.NotNull(modelError.ErrorMessage); - Assert.Equal("Sample message", modelError.ErrorMessage); - - // Check ValueTypeRequiredWithDefaultValue error. - modelStateEntry = Assert.Single( - modelStateDictionary, - entry => entry.Key == "theModel." + nameof(Person.ValueTypeRequiredWithDefaultValue)); - Assert.Equal("theModel." + nameof(Person.ValueTypeRequiredWithDefaultValue), modelStateEntry.Key); - - modelState = modelStateEntry.Value; - Assert.Equal(ModelValidationState.Invalid, modelState.ValidationState); - - modelError = Assert.Single(modelState.Errors); - Assert.Null(modelError.Exception); - Assert.NotNull(modelError.ErrorMessage); - Assert.Equal("Another sample message", modelError.ErrorMessage); + Assert.True(modelStateDictionary.IsValid); } - [Theory] - [InlineData(false)] - [InlineData(true)] - public void ProcessDto_RequiredFieldNull_RaisesModelErrorWithMessage(bool isModelSet) + [Fact] + public void ProcessDto_ValueTypeProperty_TriesToSetNullModel_CapturesException() { // Arrange var model = new Person(); @@ -1070,7 +1009,7 @@ namespace Microsoft.AspNet.Mvc.ModelBinding var propertyMetadata = dto.PropertyMetadata.Single(p => p.PropertyName == nameof(Person.ValueTypeRequired)); dto.Results[propertyMetadata] = new ModelBindingResult( null, - isModelSet: isModelSet, + isModelSet: true, key: "theModel." + nameof(Person.ValueTypeRequired)); // Make ValueTypeRequiredWithDefaultValue invalid @@ -1078,8 +1017,9 @@ namespace Microsoft.AspNet.Mvc.ModelBinding .Single(p => p.PropertyName == nameof(Person.ValueTypeRequiredWithDefaultValue)); dto.Results[propertyMetadata] = new ModelBindingResult( model: null, - isModelSet: isModelSet, + isModelSet: true, key: "theModel." + nameof(Person.ValueTypeRequiredWithDefaultValue)); + var modelValidationNode = new ModelValidationNode(string.Empty, containerMetadata, model); // Act @@ -1097,10 +1037,9 @@ namespace Microsoft.AspNet.Mvc.ModelBinding var modelState = modelStateEntry.Value; Assert.Equal(ModelValidationState.Invalid, modelState.ValidationState); - var modelError = Assert.Single(modelState.Errors); - Assert.Null(modelError.Exception); - Assert.NotNull(modelError.ErrorMessage); - Assert.Equal("Sample message", modelError.ErrorMessage); + var error = Assert.Single(modelState.Errors); + Assert.Equal(string.Empty, error.ErrorMessage); + Assert.IsType(error.Exception); // Check ValueTypeRequiredWithDefaultValue error. modelStateEntry = Assert.Single( @@ -1111,15 +1050,55 @@ namespace Microsoft.AspNet.Mvc.ModelBinding modelState = modelStateEntry.Value; Assert.Equal(ModelValidationState.Invalid, modelState.ValidationState); - modelError = Assert.Single(modelState.Errors); - Assert.Null(modelError.Exception); - Assert.NotNull(modelError.ErrorMessage); - Assert.Equal("Another sample message", modelError.ErrorMessage); + error = Assert.Single(modelState.Errors); + Assert.Equal(string.Empty, error.ErrorMessage); + Assert.IsType(error.Exception); Assert.Equal(0, model.ValueTypeRequired); Assert.Equal(expectedValue, model.ValueTypeRequiredWithDefaultValue); } + [Fact] + public void ProcessDto_ValueTypeProperty_NoValue_NoError() + { + // Arrange + var model = new Person(); + var containerMetadata = GetMetadataForType(model.GetType()); + + var bindingContext = CreateContext(containerMetadata, model); + var modelStateDictionary = bindingContext.ModelState; + + var dto = new ComplexModelDto(containerMetadata, containerMetadata.Properties); + var testableBinder = new TestableMutableObjectModelBinder(); + + // The [DefaultValue] on ValueTypeRequiredWithDefaultValue is ignored by model binding. + var expectedValue = 0; + + // Make ValueTypeRequired invalid. + var propertyMetadata = dto.PropertyMetadata.Single(p => p.PropertyName == nameof(Person.ValueTypeRequired)); + dto.Results[propertyMetadata] = new ModelBindingResult( + null, + isModelSet: false, + key: "theModel." + nameof(Person.ValueTypeRequired)); + + // Make ValueTypeRequiredWithDefaultValue invalid + propertyMetadata = dto.PropertyMetadata + .Single(p => p.PropertyName == nameof(Person.ValueTypeRequiredWithDefaultValue)); + dto.Results[propertyMetadata] = new ModelBindingResult( + model: null, + isModelSet: false, + key: "theModel." + nameof(Person.ValueTypeRequiredWithDefaultValue)); + + var modelValidationNode = new ModelValidationNode(string.Empty, containerMetadata, model); + + // Act + testableBinder.ProcessDto(bindingContext, dto, modelValidationNode); + + // Assert + Assert.True(modelStateDictionary.IsValid); + Assert.Empty(modelStateDictionary); + } + [Fact] public void ProcessDto_ProvideRequiredFields_Success() { @@ -1171,6 +1150,109 @@ namespace Microsoft.AspNet.Mvc.ModelBinding Assert.Equal(0m, model.PropertyWithDefaultValue); // [DefaultValue] has no effect } + // This uses [Required] with [BindRequired] to provide a custom validation messsage. + [Fact] + public void ProcessDto_ValueTypePropertyWithBindRequired_CustomValidationMessage() + { + // Arrange + var model = new ModelWithBindRequiredAndRequiredAttribute(); + var containerMetadata = GetMetadataForType(model.GetType()); + + var bindingContext = CreateContext(containerMetadata, model); + var modelStateDictionary = bindingContext.ModelState; + + var dto = new ComplexModelDto(containerMetadata, containerMetadata.Properties); + var testableBinder = new TestableMutableObjectModelBinder(); + + // Make ValueTypeProperty not have a value. + var propertyMetadata = containerMetadata + .Properties[nameof(ModelWithBindRequiredAndRequiredAttribute.ValueTypeProperty)]; + dto.Results[propertyMetadata] = new ModelBindingResult( + null, + isModelSet: false, + key: "theModel." + nameof(ModelWithBindRequiredAndRequiredAttribute.ValueTypeProperty)); + + // Make ReferenceTypeProperty have a value. + propertyMetadata = containerMetadata + .Properties[nameof(ModelWithBindRequiredAndRequiredAttribute.ReferenceTypeProperty)]; + dto.Results[propertyMetadata] = new ModelBindingResult( + model: "value", + isModelSet: true, + key: "theModel." + nameof(ModelWithBindRequiredAndRequiredAttribute.ReferenceTypeProperty)); + + var modelValidationNode = new ModelValidationNode(string.Empty, containerMetadata, model); + + // Act + testableBinder.ProcessDto(bindingContext, dto, modelValidationNode); + + // Assert + Assert.False(modelStateDictionary.IsValid); + + var entry = Assert.Single( + modelStateDictionary, + kvp => kvp.Key == "theModel." + nameof(ModelWithBindRequiredAndRequiredAttribute.ValueTypeProperty)) + .Value; + var error = Assert.Single(entry.Errors); + Assert.Null(error.Exception); + Assert.Equal("Custom Message ValueTypeProperty", error.ErrorMessage); + + // Model gets provided values. + Assert.Equal(0, model.ValueTypeProperty); + Assert.Equal("value", model.ReferenceTypeProperty); + } + + // This uses [Required] with [BindRequired] to provide a custom validation messsage. + [Fact] + public void ProcessDto_ReferenceTypePropertyWithBindRequired_CustomValidationMessage() + { + // Arrange + var model = new ModelWithBindRequiredAndRequiredAttribute(); + var containerMetadata = GetMetadataForType(model.GetType()); + + var bindingContext = CreateContext(containerMetadata, model); + var modelStateDictionary = bindingContext.ModelState; + + var dto = new ComplexModelDto(containerMetadata, containerMetadata.Properties); + var testableBinder = new TestableMutableObjectModelBinder(); + + // Make ValueTypeProperty have a value. + var propertyMetadata = containerMetadata + .Properties[nameof(ModelWithBindRequiredAndRequiredAttribute.ValueTypeProperty)]; + dto.Results[propertyMetadata] = new ModelBindingResult( + 17, + isModelSet: true, + key: "theModel." + nameof(ModelWithBindRequiredAndRequiredAttribute.ValueTypeProperty)); + + // Make ReferenceTypeProperty not have a value. + propertyMetadata = containerMetadata + .Properties[nameof(ModelWithBindRequiredAndRequiredAttribute.ReferenceTypeProperty)]; + dto.Results[propertyMetadata] = new ModelBindingResult( + model: null, + isModelSet: false, + key: "theModel." + nameof(ModelWithBindRequiredAndRequiredAttribute.ReferenceTypeProperty)); + + var modelValidationNode = new ModelValidationNode(string.Empty, containerMetadata, model); + + // Act + testableBinder.ProcessDto(bindingContext, dto, modelValidationNode); + + // Assert + Assert.False(modelStateDictionary.IsValid); + + var entry = Assert.Single( + modelStateDictionary, + kvp => kvp.Key == "theModel." + nameof(ModelWithBindRequiredAndRequiredAttribute.ReferenceTypeProperty)) + .Value; + var error = Assert.Single(entry.Errors); + Assert.Null(error.Exception); + Assert.Equal("Custom Message ReferenceTypeProperty", error.ErrorMessage); + + // Model gets provided values. + Assert.Equal(17, model.ValueTypeProperty); + Assert.Null(model.ReferenceTypeProperty); + } + + [Fact] public void ProcessDto_Success() { @@ -1241,12 +1323,6 @@ namespace Microsoft.AspNet.Mvc.ModelBinding isModelSet: false, key: "foo"); - var validatorProvider = bindingContext.OperationBindingContext.ValidatorProvider; - var validatorProviderContext = new ModelValidatorProviderContext(propertyMetadata); - validatorProvider.GetValidators(validatorProviderContext); - - var requiredValidator = validatorProviderContext.Validators.FirstOrDefault(v => v.IsRequired); - var testableBinder = new TestableMutableObjectModelBinder(); // Act @@ -1254,8 +1330,7 @@ namespace Microsoft.AspNet.Mvc.ModelBinding bindingContext, modelExplorer, propertyMetadata, - dtoResult, - requiredValidator); + dtoResult); // Assert var person = Assert.IsType(bindingContext.Model); @@ -1287,8 +1362,7 @@ namespace Microsoft.AspNet.Mvc.ModelBinding bindingContext, modelExplorer, propertyMetadata, - dtoResult, - requiredValidator: null); + dtoResult); // Assert var person = Assert.IsType(bindingContext.Model); @@ -1320,8 +1394,7 @@ namespace Microsoft.AspNet.Mvc.ModelBinding bindingContext, modelExplorer, propertyMetadata, - dtoResult, - requiredValidator: null); + dtoResult); // Assert var person = Assert.IsType(bindingContext.Model); @@ -1352,8 +1425,7 @@ namespace Microsoft.AspNet.Mvc.ModelBinding bindingContext, modelExplorer, propertyMetadata, - dtoResult, - requiredValidator: null); + dtoResult); // Assert // If didn't throw, success! @@ -1406,8 +1478,7 @@ namespace Microsoft.AspNet.Mvc.ModelBinding bindingContext, modelExplorer, propertyMetadata, - dtoResult, - requiredValidator: null); + dtoResult); // Assert Assert.Equal("Joe", propertAccessor(model)); @@ -1486,8 +1557,7 @@ namespace Microsoft.AspNet.Mvc.ModelBinding bindingContext, modelExplorer, propertyMetadata, - dtoResult, - requiredValidator: null); + dtoResult); // Assert Assert.Equal(collection, propertyAccessor(model)); @@ -1511,12 +1581,6 @@ namespace Microsoft.AspNet.Mvc.ModelBinding key: "foo", isModelSet: true); - var validatorProvider = bindingContext.OperationBindingContext.ValidatorProvider; - var validatorProviderContext = new ModelValidatorProviderContext(propertyMetadata); - validatorProvider.GetValidators(validatorProviderContext); - - var requiredValidator = validatorProviderContext.Validators.FirstOrDefault(v => v.IsRequired); - var testableBinder = new TestableMutableObjectModelBinder(); // Act @@ -1524,8 +1588,7 @@ namespace Microsoft.AspNet.Mvc.ModelBinding bindingContext, modelExplorer, propertyMetadata, - dtoResult, - requiredValidator); + dtoResult); // Assert Assert.True(bindingContext.ModelState.IsValid); @@ -1560,8 +1623,7 @@ namespace Microsoft.AspNet.Mvc.ModelBinding bindingContext, modelExplorer, propertyMetadata, - dtoResult, - requiredValidator: null); + dtoResult); // Assert Assert.Equal("Date of death can't be before date of birth." + Environment.NewLine @@ -1569,8 +1631,10 @@ namespace Microsoft.AspNet.Mvc.ModelBinding bindingContext.ModelState["foo"].Errors[0].Exception.Message); } + // This can only really be done by writing an invalid model binder and returning 'isModelSet: true' + // with a null model for a value type. [Fact] - public void SetProperty_SettingNonNullableValueTypeToNull_RequiredValidatorNotPresent_RegistersValidationCallback() + public void SetProperty_SettingNonNullableValueTypeToNull_CapturesException() { // Arrange var model = new Person(); @@ -1583,9 +1647,7 @@ namespace Microsoft.AspNet.Mvc.ModelBinding var dtoResult = new ModelBindingResult( model: null, isModelSet: true, - key: "foo"); - - var requiredValidator = GetRequiredValidator(bindingContext, propertyMetadata); + key: "foo.DateOfBirth"); var testableBinder = new TestableMutableObjectModelBinder(); @@ -1594,50 +1656,20 @@ namespace Microsoft.AspNet.Mvc.ModelBinding bindingContext, modelExplorer, propertyMetadata, - dtoResult, - requiredValidator); + dtoResult); // Assert Assert.False(bindingContext.ModelState.IsValid); - } - [Fact] - public void SetProperty_SettingNonNullableValueTypeToNull_RequiredValidatorPresent_AddsModelError() - { - // Arrange - var model = new Person(); - var bindingContext = CreateContext(GetMetadataForType(model.GetType()), model); - bindingContext.ModelName = " foo"; - - var metadataProvider = bindingContext.OperationBindingContext.MetadataProvider; - var modelExplorer = metadataProvider.GetModelExplorerForType(typeof(Person), model); - var propertyMetadata = bindingContext.ModelMetadata.Properties["ValueTypeRequired"]; - - var dtoResult = new ModelBindingResult( - model: null, - isModelSet: true, - key: "foo.ValueTypeRequired"); - - var requiredValidator = GetRequiredValidator(bindingContext, propertyMetadata); - - var testableBinder = new TestableMutableObjectModelBinder(); - - // Act - testableBinder.SetProperty( - bindingContext, - modelExplorer, - propertyMetadata, - dtoResult, - requiredValidator); - - // Assert - Assert.False(bindingContext.ModelState.IsValid); - Assert.Equal("Sample message", bindingContext.ModelState["foo.ValueTypeRequired"].Errors[0].ErrorMessage); + var entry = Assert.Single(bindingContext.ModelState, kvp => kvp.Key == "foo.DateOfBirth").Value; + var error = Assert.Single(entry.Errors); + Assert.Equal(string.Empty, error.ErrorMessage); + Assert.IsType(error.Exception); } [Fact] [ReplaceCulture] - public void SetProperty_SettingNullableTypeToNull_RequiredValidatorNotPresent_PropertySetterThrows_AddsRequiredMessageString() + public void SetProperty_PropertySetterThrows_CapturesException() { // Arrange var model = new ModelWhosePropertySetterThrows(); @@ -1645,7 +1677,7 @@ namespace Microsoft.AspNet.Mvc.ModelBinding bindingContext.ModelName = "foo"; var metadataProvider = bindingContext.OperationBindingContext.MetadataProvider; - var modelExplorer = metadataProvider.GetModelExplorerForType(typeof(Person), model); + var modelExplorer = metadataProvider.GetModelExplorerForType(typeof(ModelWhosePropertySetterThrows), model); var propertyMetadata = bindingContext.ModelMetadata.Properties["NameNoAttribute"]; var dtoResult = new ModelBindingResult( @@ -1653,8 +1685,6 @@ namespace Microsoft.AspNet.Mvc.ModelBinding isModelSet: true, key: "foo.NameNoAttribute"); - var requiredValidator = GetRequiredValidator(bindingContext, propertyMetadata); - var testableBinder = new TestableMutableObjectModelBinder(); // Act @@ -1662,8 +1692,7 @@ namespace Microsoft.AspNet.Mvc.ModelBinding bindingContext, modelExplorer, propertyMetadata, - dtoResult, - requiredValidator); + dtoResult); // Assert Assert.False(bindingContext.ModelState.IsValid); @@ -1673,41 +1702,6 @@ namespace Microsoft.AspNet.Mvc.ModelBinding bindingContext.ModelState["foo.NameNoAttribute"].Errors[0].Exception.Message); } - [Fact] - public void SetProperty_SettingNullableTypeToNull_RequiredValidatorPresent_PropertySetterThrows_AddsRequiredMessageString() - { - // Arrange - var model = new ModelWhosePropertySetterThrows(); - var bindingContext = CreateContext(GetMetadataForType(model.GetType()), model); - bindingContext.ModelName = "foo"; - - var metadataProvider = bindingContext.OperationBindingContext.MetadataProvider; - var modelExplorer = metadataProvider.GetModelExplorerForType(typeof(Person), model); - var propertyMetadata = bindingContext.ModelMetadata.Properties["Name"]; - - var dtoResult = new ModelBindingResult( - model: null, - isModelSet: true, - key: "foo.Name"); - - var requiredValidator = GetRequiredValidator(bindingContext, propertyMetadata); - - var testableBinder = new TestableMutableObjectModelBinder(); - - // Act - testableBinder.SetProperty( - bindingContext, - modelExplorer, - propertyMetadata, - dtoResult, - requiredValidator); - - // Assert - Assert.False(bindingContext.ModelState.IsValid); - var error = Assert.Single(bindingContext.ModelState["foo.Name"].Errors); - Assert.Equal("This message comes from the [Required] attribute.", error.ErrorMessage); - } - private static ModelBindingContext CreateContext(ModelMetadata metadata, object model) { return new ModelBindingContext @@ -1841,6 +1835,18 @@ namespace Microsoft.AspNet.Mvc.ModelBinding public string Optional { get; set; } } + [BindRequired] + private class ModelWithBindRequiredAndRequiredAttribute + { + [Range(5, 20)] + [Required(ErrorMessage = "Custom Message {0}")] + public int ValueTypeProperty { get; set; } + + [StringLength(25)] + [Required(ErrorMessage = "Custom Message {0}")] + public string ReferenceTypeProperty { get; set; } + } + private sealed class MyModelTestingCanUpdateProperty { public int ReadOnlyInt { get; private set; } @@ -2030,15 +2036,13 @@ namespace Microsoft.AspNet.Mvc.ModelBinding ModelBindingContext bindingContext, ModelExplorer modelExplorer, ModelMetadata propertyMetadata, - ModelBindingResult dtoResult, - IModelValidator requiredValidator) + ModelBindingResult dtoResult) { base.SetProperty( bindingContext, modelExplorer, propertyMetadata, - dtoResult, - requiredValidator); + dtoResult); } } } diff --git a/test/Microsoft.AspNet.Mvc.FunctionalTests/ModelBindingFromHeaderTest.cs b/test/Microsoft.AspNet.Mvc.FunctionalTests/ModelBindingFromHeaderTest.cs index 9867bebd41..11fa0c57c7 100644 --- a/test/Microsoft.AspNet.Mvc.FunctionalTests/ModelBindingFromHeaderTest.cs +++ b/test/Microsoft.AspNet.Mvc.FunctionalTests/ModelBindingFromHeaderTest.cs @@ -295,9 +295,9 @@ namespace Microsoft.AspNet.Mvc.FunctionalTests // This model sets a value for 'Title', and the model binder won't trounce it. // - // There's a validation error because we validate the value in the request (not present). + // There's no validation error because we validate the value on the model. [Fact] - public async Task FromHeader_BindHeader_ToModel_NoValues_InitializedValue_ValidationError() + public async Task FromHeader_BindHeader_ToModel_NoValues_InitializedValue_NoValidationError() { // Arrange var server = TestHelper.CreateServer(_app, SiteName, _configureServices); @@ -321,8 +321,7 @@ namespace Microsoft.AspNet.Mvc.FunctionalTests Assert.Equal("How to Make Soup", result.HeaderValue); Assert.Equal(new[] { "Cooking" }, result.HeaderValues); - var error = Assert.Single(result.ModelStateErrors); - Assert.Equal("Title", error); + Assert.Empty(result.ModelStateErrors); } private class Result diff --git a/test/Microsoft.AspNet.Mvc.FunctionalTests/compiler/resources/MvcTagHelpersWebSite.MvcTagHelper_Customer.Index.html b/test/Microsoft.AspNet.Mvc.FunctionalTests/compiler/resources/MvcTagHelpersWebSite.MvcTagHelper_Customer.Index.html index 8b44bbcb07..6561ab094a 100644 --- a/test/Microsoft.AspNet.Mvc.FunctionalTests/compiler/resources/MvcTagHelpersWebSite.MvcTagHelper_Customer.Index.html +++ b/test/Microsoft.AspNet.Mvc.FunctionalTests/compiler/resources/MvcTagHelpersWebSite.MvcTagHelper_Customer.Index.html @@ -7,7 +7,7 @@
- A value is required. + The value '' is invalid.
@@ -33,8 +33,7 @@ Female
-
  • A value is required.
    • -
  • The Password field is required.
    • +
    • The Password field is required.
    diff --git a/test/Microsoft.AspNet.Mvc.FunctionalTests/compiler/resources/TagHelpersWebSite.Employee.Create.Invalid.html b/test/Microsoft.AspNet.Mvc.FunctionalTests/compiler/resources/TagHelpersWebSite.Employee.Create.Invalid.html index 99144d46c1..99ca104941 100644 --- a/test/Microsoft.AspNet.Mvc.FunctionalTests/compiler/resources/TagHelpersWebSite.Employee.Create.Invalid.html +++ b/test/Microsoft.AspNet.Mvc.FunctionalTests/compiler/resources/TagHelpersWebSite.Employee.Create.Invalid.html @@ -55,7 +55,7 @@
    - + The JoinDate field is required.
    diff --git a/test/WebSites/TagHelpersWebSite/Models/Employee.cs b/test/WebSites/TagHelpersWebSite/Models/Employee.cs index 3c62d9aa24..556d3ba955 100644 --- a/test/WebSites/TagHelpersWebSite/Models/Employee.cs +++ b/test/WebSites/TagHelpersWebSite/Models/Employee.cs @@ -21,7 +21,7 @@ namespace TagHelpersWebSite.Models [Required] [DataType(DataType.Date)] - public DateTimeOffset JoinDate { get; set; } + public DateTimeOffset? JoinDate { get; set; } [DataType(DataType.EmailAddress)] public string Email { get; set; }