Fix #2776 - Add implicit [BindRequired] for value type properties

This commit is contained in:
Ryan Nowak 2015-08-11 08:37:32 -07:00
parent 3558c1d979
commit a6ce9abab1
15 changed files with 228 additions and 97 deletions

View File

@ -38,9 +38,11 @@ namespace Microsoft.AspNet.Mvc.ModelBinding.Metadata
/// <summary>
/// Gets or sets a value indicating whether or not the request must contain a value for the model.
/// Will be ignored if the model metadata being created does not represent a property.
/// See <see cref="ModelMetadata.IsBindingRequired"/>.
/// See <see cref="ModelMetadata.IsBindingRequired"/>. If <c>null</c>, the value of
/// <see cref="ModelMetadata.IsBindingRequired"/> will be computed based on
/// <see cref="ModelMetadata.ModelType"/>.
/// </summary>
public bool IsBindingRequired { get; set; }
public bool? IsBindingRequired { get; set; }
/// <summary>
/// Gets or sets a value indicating whether or not the model is read-only. Will be ignored

View File

@ -24,6 +24,7 @@ namespace Microsoft.AspNet.Mvc.ModelBinding.Metadata
private ReadOnlyDictionary<object, object> _additionalValues;
private ModelMetadata _elementMetadata;
private bool _haveCalculatedElementMetadata;
private bool? _isBindingRequired;
private bool? _isReadOnly;
private bool? _isRequired;
private ModelPropertyCollection _properties;
@ -334,14 +335,24 @@ namespace Microsoft.AspNet.Mvc.ModelBinding.Metadata
{
get
{
if (MetadataKind == ModelMetadataKind.Property)
if (!_isBindingRequired.HasValue)
{
return BindingMetadata.IsBindingRequired;
}
else
{
return false;
if (MetadataKind == ModelMetadataKind.Type)
{
_isBindingRequired = false;
}
else if (BindingMetadata.IsBindingRequired.HasValue)
{
_isBindingRequired = BindingMetadata.IsBindingRequired;
}
else
{
// Default to IsBindingRequired = true for value types.
_isBindingRequired = !AllowsNullValue(ModelType);
}
}
return _isBindingRequired.Value;
}
}
@ -370,14 +381,14 @@ namespace Microsoft.AspNet.Mvc.ModelBinding.Metadata
{
if (!_isReadOnly.HasValue)
{
if (BindingMetadata.IsReadOnly.HasValue)
{
_isReadOnly = BindingMetadata.IsReadOnly;
}
else if (MetadataKind == ModelMetadataKind.Type)
if (MetadataKind == ModelMetadataKind.Type)
{
_isReadOnly = false;
}
else if (BindingMetadata.IsReadOnly.HasValue)
{
_isReadOnly = BindingMetadata.IsReadOnly;
}
else
{
_isReadOnly = _details.PropertySetter == null;
@ -401,6 +412,7 @@ namespace Microsoft.AspNet.Mvc.ModelBinding.Metadata
}
else
{
// Default to IsRequired = true for value types.
_isRequired = !AllowsNullValue(ModelType);
}
}

View File

@ -22,7 +22,7 @@ namespace Microsoft.AspNet.Mvc.ModelBinding.Metadata
return;
}
if (context.BindingMetadata.IsBindingRequired)
if (context.BindingMetadata.IsBindingRequired == true)
{
// This value is already required, no need to look at attributes.
return;

View File

@ -211,11 +211,16 @@ namespace Microsoft.AspNet.Mvc.ModelBinding.Test
bool isReadOnly = false)
{
var metadataProvider = new TestModelMetadataProvider();
metadataProvider.ForType<int[]>().BindingDetails(bd => bd.IsReadOnly = isReadOnly);
metadataProvider.ForProperty(
typeof(ModelWithIntArrayProperty),
nameof(ModelWithIntArrayProperty.ArrayProperty)).BindingDetails(bd => bd.IsReadOnly = isReadOnly);
var modelMetadata = metadataProvider.GetMetadataForProperty(
typeof(ModelWithIntArrayProperty),
nameof(ModelWithIntArrayProperty.ArrayProperty));
var bindingContext = new ModelBindingContext
{
ModelMetadata = metadataProvider.GetMetadataForType(typeof(int[])),
ModelMetadata = modelMetadata,
ModelName = "someName",
ValueProvider = valueProvider,
OperationBindingContext = new OperationBindingContext
@ -245,6 +250,11 @@ namespace Microsoft.AspNet.Mvc.ModelBinding.Test
{
public string[] ArrayProperty { get; set; }
}
private class ModelWithIntArrayProperty
{
public int[] ArrayProperty { get; set; }
}
}
}
#endif

View File

@ -38,14 +38,15 @@ namespace Microsoft.AspNet.Mvc.ModelBinding.Metadata
Assert.False(metadata.HasNonDefaultEditFormat);
Assert.False(metadata.HideSurroundingHtml);
Assert.True(metadata.HtmlEncode);
Assert.False(metadata.IsBindingRequired);
Assert.True(metadata.IsBindingAllowed);
Assert.False(metadata.IsBindingRequired); // Defaults to false for reference types
Assert.False(metadata.IsComplexType);
Assert.False(metadata.IsCollectionType);
Assert.False(metadata.IsEnum);
Assert.False(metadata.IsFlagsEnum);
Assert.False(metadata.IsNullableValueType);
Assert.False(metadata.IsReadOnly);
Assert.False(metadata.IsRequired);
Assert.False(metadata.IsRequired); // Defaults to false for reference types
Assert.True(metadata.ShowForDisplay);
Assert.True(metadata.ShowForEdit);
@ -172,6 +173,99 @@ namespace Microsoft.AspNet.Mvc.ModelBinding.Metadata
{
}
[Theory]
[InlineData(typeof(string))]
[InlineData(typeof(int))]
public void IsBindingAllowed_ReturnsTrue_ForTypes(Type modelType)
{
// Arrange
var provider = new EmptyModelMetadataProvider();
var detailsProvider = new EmptyCompositeMetadataDetailsProvider();
var key = ModelMetadataIdentity.ForType(modelType);
var cache = new DefaultMetadataDetails(key, new ModelAttributes(new object[0]));
cache.BindingMetadata = new BindingMetadata()
{
IsBindingAllowed = false, // Will be ignored.
};
var metadata = new DefaultModelMetadata(provider, detailsProvider, cache);
// Act
var isBindingAllowed = metadata.IsBindingAllowed;
// Assert
Assert.True(isBindingAllowed);
}
[Theory]
[InlineData(typeof(string))]
[InlineData(typeof(int))]
public void IsBindingRequired_ReturnsFalse_ForTypes(Type modelType)
{
// Arrange
var provider = new EmptyModelMetadataProvider();
var detailsProvider = new EmptyCompositeMetadataDetailsProvider();
var key = ModelMetadataIdentity.ForType(modelType);
var cache = new DefaultMetadataDetails(key, new ModelAttributes(new object[0]));
cache.BindingMetadata = new BindingMetadata()
{
IsBindingRequired = true, // Will be ignored.
};
var metadata = new DefaultModelMetadata(provider, detailsProvider, cache);
// Act
var isBindingRequired = metadata.IsBindingRequired;
// Assert
Assert.False(isBindingRequired);
}
[Theory]
[InlineData(typeof(string))]
[InlineData(typeof(IDisposable))]
[InlineData(typeof(Nullable<int>))]
public void IsBindingRequired_ReturnsFalse_ForNullablePropertyTypes(Type modelType)
{
// Arrange
var provider = new EmptyModelMetadataProvider();
var detailsProvider = new EmptyCompositeMetadataDetailsProvider();
var key = ModelMetadataIdentity.ForProperty(modelType, "Test", typeof(string));
var cache = new DefaultMetadataDetails(key, new ModelAttributes(new object[0]));
var metadata = new DefaultModelMetadata(provider, detailsProvider, cache);
// Act
var isBindingRequired = metadata.IsBindingRequired;
// Assert
Assert.False(isBindingRequired);
}
[Theory]
[InlineData(typeof(int))]
[InlineData(typeof(DayOfWeek))]
public void IsBindingRequired_ReturnsTrue_ForNonNullablePropertyTypes(Type modelType)
{
// Arrange
var provider = new EmptyModelMetadataProvider();
var detailsProvider = new EmptyCompositeMetadataDetailsProvider();
var key = ModelMetadataIdentity.ForProperty(modelType, "Test", typeof(string));
var cache = new DefaultMetadataDetails(key, new ModelAttributes(new object[0]));
var metadata = new DefaultModelMetadata(provider, detailsProvider, cache);
// Act
var isBindingRequired = metadata.IsBindingRequired;
// Assert
Assert.True(isBindingRequired);
}
[Theory]
[InlineData(typeof(string))]
[InlineData(typeof(IDisposable))]
@ -486,6 +580,10 @@ namespace Microsoft.AspNet.Mvc.ModelBinding.Metadata
var key = ModelMetadataIdentity.ForType(typeof(int[]));
var cache = new DefaultMetadataDetails(key, new ModelAttributes(new object[0]));
cache.BindingMetadata = new BindingMetadata()
{
IsReadOnly = true, // Will be ignored.
};
var metadata = new DefaultModelMetadata(provider, detailsProvider, cache);

View File

@ -762,8 +762,7 @@ namespace Microsoft.AspNet.Mvc.ModelBinding
// Arrange
var bindingContext = new ModelBindingContext
{
// Any type, even an otherwise-simple POCO with an indexer property, would do here.
ModelMetadata = GetMetadataForType(typeof(List<Person>)),
ModelMetadata = GetMetadataForType(typeof(PersonCollection)),
OperationBindingContext = new OperationBindingContext
{
ValidatorProvider = Mock.Of<IModelValidatorProvider>(),
@ -942,16 +941,13 @@ namespace Microsoft.AspNet.Mvc.ModelBinding
}
[Fact]
[ReplaceCulture]
public void ProcessDto_MissingDataForRequiredFields_NoErrors()
public void ProcessDto_ValueTypeProperty_WithBindingOptional_NoValueSet_NoError()
{
// Arrange
var model = new ModelWithRequired();
var model = new BindingOptionalProperty();
var containerMetadata = GetMetadataForType(model.GetType());
var bindingContext = CreateContext(containerMetadata, model);
// Set no properties though Age (a non-Nullable struct) and City (a class) properties are required.
var dto = new ComplexModelDto(containerMetadata, containerMetadata.Properties);
var testableBinder = new TestableMutableObjectModelBinder();
var modelValidationNode = new ModelValidationNode(string.Empty, containerMetadata, model);
@ -962,53 +958,16 @@ namespace Microsoft.AspNet.Mvc.ModelBinding
// Assert
var modelStateDictionary = bindingContext.ModelState;
Assert.True(modelStateDictionary.IsValid);
Assert.Empty(modelStateDictionary);
}
[Fact]
[ReplaceCulture]
public void ProcessDto_ValueTypeProperty_WithRequiredAttribute_SetToNull_NoError()
public void ProcessDto_NullableValueTypeProperty_NoValueSet_NoError()
{
// Arrange
var model = new ModelWithRequired();
var model = new NullableValueTypeProperty();
var containerMetadata = GetMetadataForType(model.GetType());
var bindingContext = CreateContext(containerMetadata, model);
var dto = new ComplexModelDto(containerMetadata, containerMetadata.Properties);
var testableBinder = new TestableMutableObjectModelBinder();
// Make Age valid and City invalid.
var propertyMetadata = dto.PropertyMetadata.Single(p => p.PropertyName == "Age");
dto.Results[propertyMetadata] = new ModelBindingResult(
23,
isModelSet: true,
key: "theModel.Age");
propertyMetadata = dto.PropertyMetadata.Single(p => p.PropertyName == "City");
dto.Results[propertyMetadata] = new ModelBindingResult(
null,
isModelSet: true,
key: "theModel.City");
var modelValidationNode = new ModelValidationNode(string.Empty, containerMetadata, model);
// Act
testableBinder.ProcessDto(bindingContext, dto, modelValidationNode);
// Assert
var modelStateDictionary = bindingContext.ModelState;
Assert.True(modelStateDictionary.IsValid);
Assert.Empty(modelStateDictionary);
}
[Fact]
public void ProcessDto_PropertyWithRequiredAttribute_NoPropertiesSet_NoError()
{
// Arrange
var model = new Person();
var containerMetadata = GetMetadataForType(model.GetType());
var bindingContext = CreateContext(containerMetadata, model);
// Set no properties though ValueTypeRequired (a non-Nullable struct) property is required.
var dto = new ComplexModelDto(containerMetadata, containerMetadata.Properties);
var testableBinder = new TestableMutableObjectModelBinder();
var modelValidationNode = new ModelValidationNode(string.Empty, containerMetadata, model);
@ -1091,14 +1050,14 @@ namespace Microsoft.AspNet.Mvc.ModelBinding
}
[Fact]
public void ProcessDto_ValueTypeProperty_NoValue_NoError()
public void ProcessDto_ValueTypeProperty_NoValue_Error()
{
// Arrange
var model = new Person();
var containerMetadata = GetMetadataForType(model.GetType());
var bindingContext = CreateContext(containerMetadata, model);
var modelStateDictionary = bindingContext.ModelState;
var modelState = bindingContext.ModelState;
var dto = new ComplexModelDto(containerMetadata, containerMetadata.Properties);
var testableBinder = new TestableMutableObjectModelBinder();
@ -1124,8 +1083,14 @@ namespace Microsoft.AspNet.Mvc.ModelBinding
testableBinder.ProcessDto(bindingContext, dto, modelValidationNode);
// Assert
Assert.True(modelStateDictionary.IsValid);
Assert.Empty(modelStateDictionary);
Assert.False(modelState.IsValid);
var entry = modelState["theModel." + nameof(Person.ValueTypeRequiredWithDefaultValue)];
var error = Assert.Single(entry.Errors);
Assert.Equal(
$"A value for the '{nameof(Person.ValueTypeRequiredWithDefaultValue)}' property was not provided.",
error.ErrorMessage);
Assert.Null(error.Exception);
}
[Fact]
@ -1763,10 +1728,23 @@ namespace Microsoft.AspNet.Mvc.ModelBinding
{
}
private class BindingOptionalProperty
{
[BindingBehavior(BindingBehavior.Optional)]
public int ValueTypeRequired { get; set; }
}
private class NullableValueTypeProperty
{
[BindingBehavior(BindingBehavior.Optional)]
public int? NullableValueType { get; set; }
}
private class Person
{
private DateTime? _dateOfDeath;
[BindingBehavior(BindingBehavior.Optional)]
public DateTime DateOfBirth { get; set; }
public DateTime? DateOfDeath
@ -1793,6 +1771,7 @@ namespace Microsoft.AspNet.Mvc.ModelBinding
public string LastName { get; set; }
public string NonUpdateableProperty { get; private set; }
[BindingBehavior(BindingBehavior.Optional)]
[DefaultValue(typeof(decimal), "123.456")]
public decimal PropertyWithDefaultValue { get; set; }
@ -1820,17 +1799,6 @@ namespace Microsoft.AspNet.Mvc.ModelBinding
public string NonUpdateableProperty { get; private set; }
}
private class ModelWithRequired
{
public string Name { get; set; }
[Required]
public int Age { get; set; }
[Required]
public string City { get; set; }
}
private class ModelWithBindRequired
{
public string Name { get; set; }
@ -1993,6 +1961,17 @@ namespace Microsoft.AspNet.Mvc.ModelBinding
public string Name { get; set; }
}
private class PersonCollection
{
public Person this[int index]
{
get
{
return null;
}
}
}
private class CollectionContainer
{
public int[] ReadOnlyArray { get; } = new int[4];

View File

@ -346,7 +346,7 @@ namespace Microsoft.AspNet.Mvc.ModelBinding.Metadata
// Arrange
var attributes = new[] { attribute };
var provider = CreateProvider(attributes);
var metadata = provider.GetMetadataForType(typeof(string));
var metadata = provider.GetMetadataForProperty(typeof(CoolUser), nameof(CoolUser.Name));
// Act
var result = accessor(metadata);
@ -932,6 +932,11 @@ namespace Microsoft.AspNet.Mvc.ModelBinding.Metadata
}
}
public class CoolUser
{
public string Name { get; set; }
}
public class TypeBasedBinderAttribute : Attribute, IModelNameProvider
{
public string Name { get; set; }
@ -1038,6 +1043,18 @@ namespace Microsoft.AspNet.Mvc.ModelBinding.Metadata
key,
new ModelAttributes(_attributes.Concat(entry.ModelAttributes.TypeAttributes).ToArray()));
}
protected override DefaultMetadataDetails[] CreatePropertyDetails([NotNull] ModelMetadataIdentity key)
{
var entries = base.CreatePropertyDetails(key);
return entries.Select(e =>
{
return new DefaultMetadataDetails(
e.Key,
new ModelAttributes(_attributes.Concat(e.ModelAttributes.PropertyAttributes), e.ModelAttributes.TypeAttributes));
})
.ToArray();
}
}
}
}

View File

@ -196,11 +196,12 @@ namespace Microsoft.AspNet.Mvc.FunctionalTests
var request = new HttpRequestMessage(HttpMethod.Post, "http://localhost/Customer/HtmlGeneration_Customer");
var nameValueCollection = new List<KeyValuePair<string, string>>
{
new KeyValuePair<string,string>("Number", string.Empty),
new KeyValuePair<string,string>("Number", "0"),
new KeyValuePair<string,string>("Name", string.Empty),
new KeyValuePair<string,string>("Email", string.Empty),
new KeyValuePair<string,string>("PhoneNumber", string.Empty),
new KeyValuePair<string,string>("Password", string.Empty)
new KeyValuePair<string,string>("Password", string.Empty),
new KeyValuePair<string, string>("Gender", "Female"),
};
request.Content = new FormUrlEncodedContent(nameValueCollection);

View File

@ -122,8 +122,12 @@ namespace Microsoft.AspNet.Mvc.FunctionalTests
// Assert
var body = await response.Content.ReadAsStringAsync();
var result = JsonConvert.DeserializeObject<Result>(body);
var error = Assert.Single(result.ModelStateErrors);
Assert.Equal("TestEmployees[0].EmployeeId", error);
Assert.Collection(
result.ModelStateErrors,
e => Assert.Equal("TestEmployees[0].EmployeeId", e),
e => Assert.Equal("TestEmployees[0].EmployeeTaxId", e),
e => Assert.Equal("TestEmployees[0].Age", e));
}
}
}

View File

@ -2218,7 +2218,7 @@ namespace Microsoft.AspNet.Mvc.FunctionalTests
// Arrange
var server = TestHelper.CreateServer(_app, SiteName, _configureServices);
var client = server.CreateClient();
var url = "http://localhost/TryUpdateModel/TryUpdateModel_ClearsModelStateEntries";
var url = "http://localhost/TryUpdateModel/TryUpdateModel_ClearsModelStateEntries?id=5&price=1";
// Act
var response = await client.GetAsync(url);

View File

@ -3,8 +3,8 @@
<form action="/Customer/HtmlGeneration_Customer" method="post">
<div>
<label class="order" for="Number">Number</label>
<input class="form-control input-validation-error" type="number" data-val="true" data-val-range="The field Number must be between 1 and 100." data-val-range-max="100" data-val-range-min="1" data-val-required="The Number field is required." id="Number" name="Number" value="" />
<span class="field-validation-error" data-valmsg-for="Number" data-valmsg-replace="true">The value &#x27;&#x27; is invalid.</span>
<input class="form-control input-validation-error" type="number" data-val="true" data-val-range="The field Number must be between 1 and 100." data-val-range-max="100" data-val-range-min="1" data-val-required="The Number field is required." id="Number" name="Number" value="0" />
<span class="field-validation-error" data-valmsg-for="Number" data-valmsg-replace="true">The field Number must be between 1 and 100.</span>
</div>
<div>
<label class="order" for="Name">Name</label>
@ -27,10 +27,10 @@
<div>
<label class="order" for="Gender">Gender</label>
<input type="radio" value="Male" data-val="true" data-val-required="The Gender field is required." id="Gender" name="Gender" /> Male
<input type="radio" value="Female" id="Gender" name="Gender" /> Female
<input type="radio" value="Female" checked="checked" id="Gender" name="Gender" /> Female
<span class="field-validation-valid" data-valmsg-for="Gender" data-valmsg-replace="true"></span>
</div>
<div class="order validation-summary-errors" data-valmsg-summary="true"><ul><li>The value &#x27;&#x27; is invalid.</li>
<div class="order validation-summary-errors" data-valmsg-summary="true"><ul><li>The field Number must be between 1 and 100.</li>
<li>The Password field is required.</li>
</ul></div>
<div class="order validation-summary-errors"><ul><li style="display:none"></li>

View File

@ -108,7 +108,7 @@ namespace Microsoft.AspNet.Mvc.IntegrationTests
private class Person4
{
[FromBody]
[Required]
[BindingBehavior(BindingBehavior.Optional)]
public int Address { get; set; }
}

View File

@ -396,10 +396,14 @@ namespace Microsoft.AspNet.Mvc.IntegrationTests
entry = Assert.Single(modelState, kvp => kvp.Key == "parameter[0].Name").Value;
Assert.Null(entry.Value);
Assert.Equal(ModelValidationState.Invalid, entry.ValidationState);
var error = Assert.Single(entry.Errors);
Assert.Equal("A value for the 'Name' property was not provided.", error.ErrorMessage);
entry = Assert.Single(modelState, kvp => kvp.Key == "parameter[1].Name").Value;
Assert.Null(entry.Value);
Assert.Equal(ModelValidationState.Invalid, entry.ValidationState);
error = Assert.Single(entry.Errors);
Assert.Equal("A value for the 'Name' property was not provided.", error.ErrorMessage);
}
[Fact]
@ -548,6 +552,7 @@ namespace Microsoft.AspNet.Mvc.IntegrationTests
private class Address4
{
[BindingBehavior(BindingBehavior.Optional)]
public int Zip { get; set; }
public string Street { get; set; }
@ -606,7 +611,7 @@ namespace Microsoft.AspNet.Mvc.IntegrationTests
private class Address5
{
public int Zip { get; set; }
public int? Zip { get; set; }
[StringLength(3)]
public string Street { get; set; }

View File

@ -26,6 +26,7 @@ namespace Microsoft.AspNet.Mvc.IntegrationTests
private class Order1
{
[BindingBehavior(BindingBehavior.Optional)]
public int ProductId { get; set; }
public Person1 Customer { get; set; }
@ -261,7 +262,7 @@ namespace Microsoft.AspNet.Mvc.IntegrationTests
private class Order2
{
public int ProductId { get; set; }
public int? ProductId { get; set; }
public Person2 Customer { get; set; }
}
@ -435,6 +436,7 @@ namespace Microsoft.AspNet.Mvc.IntegrationTests
private class Order3
{
[BindingBehavior(BindingBehavior.Optional)]
public int ProductId { get; set; }
public Person3 Customer { get; set; }
@ -578,7 +580,7 @@ namespace Microsoft.AspNet.Mvc.IntegrationTests
private class Order4
{
public int ProductId { get; set; }
public int? ProductId { get; set; }
public Person4 Customer { get; set; }
}
@ -1341,6 +1343,7 @@ namespace Microsoft.AspNet.Mvc.IntegrationTests
{
public string Name { get; set; }
[BindingBehavior(BindingBehavior.Optional)]
public KeyValuePair<string, int> ProductId { get; set; }
}

View File

@ -195,7 +195,7 @@ namespace Microsoft.AspNet.Mvc.IntegrationTests
private class Person3
{
public int Age { get; set; }
public int? Age { get; set; }
[Required]
public string Name { get; set; }
@ -1018,12 +1018,12 @@ namespace Microsoft.AspNet.Mvc.IntegrationTests
private class Address
{
public int Street { get; set; }
public int? Street { get; set; }
public string State { get; set; }
[Range(10000, 99999)]
public int Zip { get; set; }
public int? Zip { get; set; }
public Country Country { get; set; }
}