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; }