From f77493dffeef0ab3ddf3ec75147cb8530489d88d Mon Sep 17 00:00:00 2001 From: Ryan Nowak Date: Tue, 14 Apr 2015 13:08:37 -0700 Subject: [PATCH] Part 1 of #1712 - Remove reflection in MutableObjectModelBinder This change introduces a new property to ModelMetadata called IsBindingRequired, which specifies whether or not a model value must be present on the wire during model binding. [DataMember(IsRequired = true)] is currently the only thing that will set this property. Updated tests and documentation for clarity on the difference in meaning between MM.IsRequired and MM.IsBindingRequired. Moved setting for IsRequired to ValidationMetadata which is a better fit. Also added functional tests for [BindingBehavior] and [DataMember] in model binding because they were totally missing. --- .../Binders/MutableObjectModelBinder.cs | 4 + .../Metadata/BindingMetadata.cs | 15 +- .../DataAnnotationsMetadataProvider.cs | 14 +- ...taMemberRequiredBindingMetadataProvider.cs | 4 +- .../Metadata/DefaultModelMetadata.cs | 13 +- .../Metadata/ValidationMetadata.cs | 8 ++ .../ModelMetadata.cs | 19 +++ .../ModelBindingBindingBehaviorTest.cs | 128 ++++++++++++++++++ .../ModelBindingDataMemberRequiredTest.cs | 87 ++++++++++++ .../Binders/MutableObjectModelBinderTest.cs | 60 ++++++++ .../DataAnnotationsMetadataProviderTest.cs | 75 +++++----- ...mberRequiredBindingMetadataProviderTest.cs | 32 ++--- .../Metadata/DefaultModelMetadataTest.cs | 1 + .../Metadata/ModelMetadataProviderTest.cs | 27 +--- .../Metadata/ModelMetadataTest.cs | 8 ++ .../Controllers/BindingBehaviorController.cs | 23 ++++ .../DataMemberRequiredController.cs | 23 ++++ .../Models/BindingBehaviorModel.cs | 25 ++++ .../Models/DataMemberRequiredModel.cs | 20 +++ 19 files changed, 493 insertions(+), 93 deletions(-) create mode 100644 test/Microsoft.AspNet.Mvc.FunctionalTests/ModelBindingBindingBehaviorTest.cs create mode 100644 test/Microsoft.AspNet.Mvc.FunctionalTests/ModelBindingDataMemberRequiredTest.cs create mode 100644 test/WebSites/ModelBindingWebSite/Controllers/BindingBehaviorController.cs create mode 100644 test/WebSites/ModelBindingWebSite/Controllers/DataMemberRequiredController.cs create mode 100644 test/WebSites/ModelBindingWebSite/Models/BindingBehaviorModel.cs create mode 100644 test/WebSites/ModelBindingWebSite/Models/DataMemberRequiredModel.cs diff --git a/src/Microsoft.AspNet.Mvc.ModelBinding/Binders/MutableObjectModelBinder.cs b/src/Microsoft.AspNet.Mvc.ModelBinding/Binders/MutableObjectModelBinder.cs index f589c33eb4..064473fdd9 100644 --- a/src/Microsoft.AspNet.Mvc.ModelBinding/Binders/MutableObjectModelBinder.cs +++ b/src/Microsoft.AspNet.Mvc.ModelBinding/Binders/MutableObjectModelBinder.cs @@ -355,6 +355,10 @@ namespace Microsoft.AspNet.Mvc.ModelBinding { validationInfo.RequiredProperties.Add(propertyName); } + else if (propertyMetadata.IsBindingRequired) + { + validationInfo.RequiredProperties.Add(propertyName); + } } return validationInfo; diff --git a/src/Microsoft.AspNet.Mvc.ModelBinding/Metadata/BindingMetadata.cs b/src/Microsoft.AspNet.Mvc.ModelBinding/Metadata/BindingMetadata.cs index 7a82f22c94..d44cc74175 100644 --- a/src/Microsoft.AspNet.Mvc.ModelBinding/Metadata/BindingMetadata.cs +++ b/src/Microsoft.AspNet.Mvc.ModelBinding/Metadata/BindingMetadata.cs @@ -28,6 +28,13 @@ namespace Microsoft.AspNet.Mvc.ModelBinding.Metadata /// public Type BinderType { get; set; } + /// + /// 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 is not a property. + /// See . + /// + public bool IsBindingRequired { get; set; } + /// /// Gets or sets a value indicating whether or not the model is read-only. Will be ignored /// if the model metadata being created is not a property. If null then @@ -36,14 +43,6 @@ namespace Microsoft.AspNet.Mvc.ModelBinding.Metadata /// public bool? IsReadOnly { get; set; } - /// - /// Gets or sets a value indicating whether or not the model is a required value. Will be ignored - /// if the model metadata being created is not a property. If null then - /// will be computed based on the model . - /// See . - /// - public bool? IsRequired { get; set; } - /// /// Gets or sets the . /// See . diff --git a/src/Microsoft.AspNet.Mvc.ModelBinding/Metadata/DataAnnotationsMetadataProvider.cs b/src/Microsoft.AspNet.Mvc.ModelBinding/Metadata/DataAnnotationsMetadataProvider.cs index 6f5a2d0ab3..28f29e7bc3 100644 --- a/src/Microsoft.AspNet.Mvc.ModelBinding/Metadata/DataAnnotationsMetadataProvider.cs +++ b/src/Microsoft.AspNet.Mvc.ModelBinding/Metadata/DataAnnotationsMetadataProvider.cs @@ -22,12 +22,6 @@ namespace Microsoft.AspNet.Mvc.ModelBinding.Metadata /// public void GetBindingMetadata([NotNull] BindingMetadataProviderContext context) { - var requiredAttribute = context.Attributes.OfType().FirstOrDefault(); - if (requiredAttribute != null) - { - context.BindingMetadata.IsRequired = true; - } - var editableAttribute = context.Attributes.OfType().FirstOrDefault(); if (editableAttribute != null) { @@ -211,6 +205,14 @@ namespace Microsoft.AspNet.Mvc.ModelBinding.Metadata /// public void GetValidationMetadata([NotNull] ValidationMetadataProviderContext context) { + // RequiredAttribute marks a property as required by validation - this means that it + // must have a non-null value on the model during validation. + var requiredAttribute = context.Attributes.OfType().FirstOrDefault(); + if (requiredAttribute != null) + { + context.ValidationMetadata.IsRequired = true; + } + foreach (var attribute in context.Attributes.OfType()) { // If another provider has already added this attribute, do not repeat it. diff --git a/src/Microsoft.AspNet.Mvc.ModelBinding/Metadata/DataMemberRequiredBindingMetadataProvider.cs b/src/Microsoft.AspNet.Mvc.ModelBinding/Metadata/DataMemberRequiredBindingMetadataProvider.cs index 3d848a6c50..81db4cf98e 100644 --- a/src/Microsoft.AspNet.Mvc.ModelBinding/Metadata/DataMemberRequiredBindingMetadataProvider.cs +++ b/src/Microsoft.AspNet.Mvc.ModelBinding/Metadata/DataMemberRequiredBindingMetadataProvider.cs @@ -22,7 +22,7 @@ namespace Microsoft.AspNet.Mvc.ModelBinding.Metadata return; } - if (context.BindingMetadata.IsRequired == true) + if (context.BindingMetadata.IsBindingRequired) { // This value is already required, no need to look at attributes. return; @@ -44,7 +44,7 @@ namespace Microsoft.AspNet.Mvc.ModelBinding.Metadata { // We don't need to add a validator, just to set IsRequired = true. The validation // system will do the right thing. - context.BindingMetadata.IsRequired = true; + context.BindingMetadata.IsBindingRequired = true; } } } diff --git a/src/Microsoft.AspNet.Mvc.ModelBinding/Metadata/DefaultModelMetadata.cs b/src/Microsoft.AspNet.Mvc.ModelBinding/Metadata/DefaultModelMetadata.cs index a1f7857865..dffe4a422f 100644 --- a/src/Microsoft.AspNet.Mvc.ModelBinding/Metadata/DefaultModelMetadata.cs +++ b/src/Microsoft.AspNet.Mvc.ModelBinding/Metadata/DefaultModelMetadata.cs @@ -255,6 +255,15 @@ namespace Microsoft.AspNet.Mvc.ModelBinding.Metadata } } + /// + public override bool IsBindingRequired + { + get + { + return BindingMetadata.IsBindingRequired; + } + } + /// public override bool IsEnum { @@ -305,9 +314,9 @@ namespace Microsoft.AspNet.Mvc.ModelBinding.Metadata { if (!_isRequired.HasValue) { - if (BindingMetadata.IsRequired.HasValue) + if (ValidationMetadata.IsRequired.HasValue) { - _isRequired = BindingMetadata.IsRequired; + _isRequired = ValidationMetadata.IsRequired; } else { diff --git a/src/Microsoft.AspNet.Mvc.ModelBinding/Metadata/ValidationMetadata.cs b/src/Microsoft.AspNet.Mvc.ModelBinding/Metadata/ValidationMetadata.cs index f6031272e6..6bab9aa27a 100644 --- a/src/Microsoft.AspNet.Mvc.ModelBinding/Metadata/ValidationMetadata.cs +++ b/src/Microsoft.AspNet.Mvc.ModelBinding/Metadata/ValidationMetadata.cs @@ -10,6 +10,14 @@ namespace Microsoft.AspNet.Mvc.ModelBinding.Metadata /// public class ValidationMetadata { + /// + /// Gets or sets a value indicating whether or not the model is a required value. Will be ignored + /// if the model metadata being created is not a property. If null then + /// will be computed based on the model . + /// See . + /// + public bool? IsRequired { get; set; } + /// /// Gets a list of metadata items for validators. /// diff --git a/src/Microsoft.AspNet.Mvc.ModelBinding/ModelMetadata.cs b/src/Microsoft.AspNet.Mvc.ModelBinding/ModelMetadata.cs index a23fc98ab2..e60d183ed1 100644 --- a/src/Microsoft.AspNet.Mvc.ModelBinding/ModelMetadata.cs +++ b/src/Microsoft.AspNet.Mvc.ModelBinding/ModelMetadata.cs @@ -166,6 +166,16 @@ namespace Microsoft.AspNet.Mvc.ModelBinding /// public abstract bool HideSurroundingHtml { get; } + /// + /// Gets a value indicating whether or not the model value is required by model binding. This is only + /// applicable when the current instance represents a property. + /// + /// + /// If true then the model value is considered required by model-binding and must have a value + /// supplied in the request to be considered valid. + /// + public abstract bool IsBindingRequired { get; } + /// /// Gets a value indicating whether or Nullable.GetUnderlyingType(ModelType) is /// for an . @@ -197,6 +207,15 @@ namespace Microsoft.AspNet.Mvc.ModelBinding /// Gets a value indicating whether or not the model value is required. This is only applicable when /// the current instance represents a property. /// + /// + /// + /// If true then the model value is considered required by validators. + /// + /// + /// By default an implicit will be added + /// if not present when true.. + /// + /// public abstract bool IsRequired { get; } /// diff --git a/test/Microsoft.AspNet.Mvc.FunctionalTests/ModelBindingBindingBehaviorTest.cs b/test/Microsoft.AspNet.Mvc.FunctionalTests/ModelBindingBindingBehaviorTest.cs new file mode 100644 index 0000000000..d58d9946f3 --- /dev/null +++ b/test/Microsoft.AspNet.Mvc.FunctionalTests/ModelBindingBindingBehaviorTest.cs @@ -0,0 +1,128 @@ +// Copyright (c) Microsoft Open Technologies, Inc. All rights reserved. +// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. + +using System; +using System.Collections.Generic; +using System.Net; +using System.Net.Http; +using System.Threading.Tasks; +using Microsoft.AspNet.Builder; +using Microsoft.Framework.DependencyInjection; +using ModelBindingWebSite; +using Newtonsoft.Json; +using Newtonsoft.Json.Linq; +using Xunit; + +namespace Microsoft.AspNet.Mvc.FunctionalTests +{ + public class ModelBindingBindingBehaviorTest + { + private const string SiteName = nameof(ModelBindingWebSite); + private readonly Action _app = new Startup().Configure; + private readonly Action _configureServices = new Startup().ConfigureServices; + + [Fact] + public async Task BindingBehavior_MissingRequiredProperties_ValidationErrors() + { + // Arrange + var server = TestHelper.CreateServer(_app, SiteName, _configureServices); + var client = server.CreateClient(); + + var url = "http://localhost/BindingBehavior/EchoModelValues"; + var request = new HttpRequestMessage(HttpMethod.Post, url); + var formData = new List> + { + new KeyValuePair("model.BehaviourOptionalProperty", "Hi"), + }; + + request.Content = new FormUrlEncodedContent(formData); + + // Act + var response = await client.SendAsync(request); + + // Assert + Assert.Equal(HttpStatusCode.BadRequest, response.StatusCode); + + var body = await response.Content.ReadAsStringAsync(); + var errors = JsonConvert.DeserializeObject(body); + + Assert.Equal(2, errors.Count); + + var error = Assert.Single(errors, kvp => kvp.Key == "model.BehaviourRequiredProperty"); + Assert.Equal("The 'BehaviourRequiredProperty' property is required.", ((JArray)error.Value)[0].Value()); + + error = Assert.Single(errors, kvp => kvp.Key == "model.BindRequiredProperty"); + Assert.Equal("The 'BindRequiredProperty' property is required.", ((JArray)error.Value)[0].Value()); + } + + [Fact] + public async Task BindingBehavior_OptionalIsOptional() + { + // Arrange + var server = TestHelper.CreateServer(_app, SiteName, _configureServices); + var client = server.CreateClient(); + + var url = "http://localhost/BindingBehavior/EchoModelValues"; + var request = new HttpRequestMessage(HttpMethod.Post, url); + var formData = new List> + { + new KeyValuePair("model.BehaviourRequiredProperty", "Hello"), + new KeyValuePair("model.BindRequiredProperty", "World!"), + }; + + request.Content = new FormUrlEncodedContent(formData); + + // Act + var response = await client.SendAsync(request); + + // Assert + Assert.Equal(HttpStatusCode.OK, response.StatusCode); + + var body = await response.Content.ReadAsStringAsync(); + var model = JsonConvert.DeserializeObject(body); + + Assert.Null(model.BehaviourNeverProperty); + Assert.Null(model.BehaviourOptionalProperty); + Assert.Equal("Hello", model.BehaviourRequiredProperty); + Assert.Equal("World!", model.BindRequiredProperty); + Assert.Null(model.BindNeverProperty); + } + + [Fact] + public async Task BindingBehavior_Never_IsNotBound() + { + // Arrange + var server = TestHelper.CreateServer(_app, SiteName, _configureServices); + var client = server.CreateClient(); + + var url = "http://localhost/BindingBehavior/EchoModelValues"; + var request = new HttpRequestMessage(HttpMethod.Post, url); + var formData = new List> + { + + new KeyValuePair("model.BehaviourNeverProperty", "Ignored"), + new KeyValuePair("model.BehaviourOptionalProperty", "Optional"), + new KeyValuePair("model.BehaviourRequiredProperty", "Hello"), + new KeyValuePair("model.BindRequiredProperty", "World!"), + new KeyValuePair("model.BindNeverProperty", "Ignored"), + }; + + request.Content = new FormUrlEncodedContent(formData); + + // Act + var response = await client.SendAsync(request); + + // Assert + Assert.Equal(HttpStatusCode.OK, response.StatusCode); + + var body = await response.Content.ReadAsStringAsync(); + var model = JsonConvert.DeserializeObject(body); + + Assert.Null(model.BehaviourNeverProperty); + Assert.Equal("Optional", model.BehaviourOptionalProperty); + Assert.Equal("Hello", model.BehaviourRequiredProperty); + Assert.Equal("World!", model.BindRequiredProperty); + Assert.Null(model.BindNeverProperty); + } + } +} \ No newline at end of file diff --git a/test/Microsoft.AspNet.Mvc.FunctionalTests/ModelBindingDataMemberRequiredTest.cs b/test/Microsoft.AspNet.Mvc.FunctionalTests/ModelBindingDataMemberRequiredTest.cs new file mode 100644 index 0000000000..ed04d5cf23 --- /dev/null +++ b/test/Microsoft.AspNet.Mvc.FunctionalTests/ModelBindingDataMemberRequiredTest.cs @@ -0,0 +1,87 @@ +// Copyright (c) Microsoft Open Technologies, Inc. All rights reserved. +// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. + +using System; +using System.Collections.Generic; +using System.Net; +using System.Net.Http; +using System.Threading.Tasks; +using Microsoft.AspNet.Builder; +using Microsoft.Framework.DependencyInjection; +using ModelBindingWebSite; +using Newtonsoft.Json; +using Newtonsoft.Json.Linq; +using Xunit; + +namespace Microsoft.AspNet.Mvc.FunctionalTests +{ + public class ModelBindingDataMemberRequiredTest + { + private const string SiteName = nameof(ModelBindingWebSite); + private readonly Action _app = new Startup().Configure; + private readonly Action _configureServices = new Startup().ConfigureServices; + + [Fact] + public async Task DataMember_MissingRequiredProperty_ValidationError() + { + // Arrange + var server = TestHelper.CreateServer(_app, SiteName, _configureServices); + var client = server.CreateClient(); + + var url = "http://localhost/DataMemberRequired/EchoModelValues"; + var request = new HttpRequestMessage(HttpMethod.Post, url); + var formData = new List> + { + new KeyValuePair("model.ExplicitlyOptionalProperty", "Hi"), + }; + + request.Content = new FormUrlEncodedContent(formData); + + // Act + var response = await client.SendAsync(request); + + // Assert + Assert.Equal(HttpStatusCode.BadRequest, response.StatusCode); + + var body = await response.Content.ReadAsStringAsync(); + var errors = JsonConvert.DeserializeObject(body); + + Assert.Equal(1, errors.Count); + + var error = Assert.Single(errors, kvp => kvp.Key == "model.RequiredProperty"); + Assert.Equal("The 'RequiredProperty' property is required.", ((JArray)error.Value)[0].Value()); + } + + [Fact] + public async Task DataMember_RequiredPropertyProvided_Success() + { + // Arrange + var server = TestHelper.CreateServer(_app, SiteName, _configureServices); + var client = server.CreateClient(); + + var url = "http://localhost/DataMemberRequired/EchoModelValues"; + var request = new HttpRequestMessage(HttpMethod.Post, url); + var formData = new List> + { + new KeyValuePair("model.ImplicitlyOptionalProperty", "Hello"), + new KeyValuePair("model.ExplicitlyOptionalProperty", "World!"), + new KeyValuePair("model.RequiredProperty", "Required!"), + }; + + request.Content = new FormUrlEncodedContent(formData); + + // Act + var response = await client.SendAsync(request); + + // Assert + Assert.Equal(HttpStatusCode.OK, response.StatusCode); + + var body = await response.Content.ReadAsStringAsync(); + var model = JsonConvert.DeserializeObject(body); + + Assert.Equal("Hello", model.ImplicitlyOptionalProperty); + Assert.Equal("World!", model.ExplicitlyOptionalProperty); + Assert.Equal("Required!", model.RequiredProperty); + } + } +} \ No newline at end of file diff --git a/test/Microsoft.AspNet.Mvc.ModelBinding.Test/Binders/MutableObjectModelBinderTest.cs b/test/Microsoft.AspNet.Mvc.ModelBinding.Test/Binders/MutableObjectModelBinderTest.cs index 20f6599def..f4bed4fa10 100644 --- a/test/Microsoft.AspNet.Mvc.ModelBinding.Test/Binders/MutableObjectModelBinderTest.cs +++ b/test/Microsoft.AspNet.Mvc.ModelBinding.Test/Binders/MutableObjectModelBinderTest.cs @@ -7,6 +7,7 @@ using System.Collections.Generic; using System.ComponentModel; using System.ComponentModel.DataAnnotations; using System.Linq; +using System.Runtime.Serialization; using System.Threading.Tasks; using Microsoft.AspNet.Http.Core; using Microsoft.AspNet.Mvc.ModelBinding.Validation; @@ -821,6 +822,56 @@ namespace Microsoft.AspNet.Mvc.ModelBinding Assert.Equal("The 'Age' property is required.", modelError.ErrorMessage); } + [Fact] + [ReplaceCulture] + public void ProcessDto_DataMemberIsRequiredFieldMissing_RaisesModelError() + { + // Arrange + var model = new ModelWithDataMemberIsRequired + { + Name = "original value", + Age = -20 + }; + + var containerMetadata = GetMetadataForType(model.GetType()); + var bindingContext = new ModelBindingContext + { + Model = model, + ModelMetadata = containerMetadata, + ModelName = "theModel", + OperationBindingContext = new OperationBindingContext + { + MetadataProvider = TestModelMetadataProvider.CreateDefaultProvider(), + ValidatorProvider = Mock.Of() + } + }; + var dto = new ComplexModelDto(containerMetadata, containerMetadata.Properties); + + var nameProperty = dto.PropertyMetadata.Single(o => o.PropertyName == "Name"); + dto.Results[nameProperty] = new ModelBindingResult( + "John Doe", + isModelSet: true, + key: ""); + + var testableBinder = new TestableMutableObjectModelBinder(); + + // Act + testableBinder.ProcessDto(bindingContext, dto); + + // Assert + var modelStateDictionary = bindingContext.ModelState; + Assert.False(modelStateDictionary.IsValid); + Assert.Single(modelStateDictionary); + + // Check Age error. + ModelState modelState; + Assert.True(modelStateDictionary.TryGetValue("theModel.Age", out modelState)); + var modelError = Assert.Single(modelState.Errors); + Assert.Null(modelError.Exception); + Assert.NotNull(modelError.ErrorMessage); + Assert.Equal("The 'Age' property is required.", modelError.ErrorMessage); + } + [Fact] [ReplaceCulture] public void ProcessDto_BindRequiredFieldNull_RaisesModelError() @@ -1638,6 +1689,15 @@ namespace Microsoft.AspNet.Mvc.ModelBinding public int Age { get; set; } } + [DataContract] + private class ModelWithDataMemberIsRequired + { + public string Name { get; set; } + + [DataMember(IsRequired = true)] + public int Age { get; set; } + } + [BindRequired] private class ModelWithMixedBindingBehaviors { diff --git a/test/Microsoft.AspNet.Mvc.ModelBinding.Test/Metadata/DataAnnotationsMetadataProviderTest.cs b/test/Microsoft.AspNet.Mvc.ModelBinding.Test/Metadata/DataAnnotationsMetadataProviderTest.cs index 96f0a510d8..836151bf59 100644 --- a/test/Microsoft.AspNet.Mvc.ModelBinding.Test/Metadata/DataAnnotationsMetadataProviderTest.cs +++ b/test/Microsoft.AspNet.Mvc.ModelBinding.Test/Metadata/DataAnnotationsMetadataProviderTest.cs @@ -48,7 +48,7 @@ namespace Microsoft.AspNet.Mvc.ModelBinding.Metadata [Theory] [MemberData(nameof(DisplayDetailsData))] - public void GetDisplayDetails_SimpleAttributes( + public void GetDisplayMetadata_SimpleAttributes( object attribute, Func accessor, object expected) @@ -68,7 +68,7 @@ namespace Microsoft.AspNet.Mvc.ModelBinding.Metadata } [Fact] - public void GetDisplayDetails_FindsDisplayFormat_FromDataType() + public void GetDisplayMetadata_FindsDisplayFormat_FromDataType() { // Arrange var provider = new DataAnnotationsMetadataProvider(); @@ -88,7 +88,7 @@ namespace Microsoft.AspNet.Mvc.ModelBinding.Metadata } [Fact] - public void GetDisplayDetails_FindsDisplayFormat_OverridingDataType() + public void GetDisplayMetadata_FindsDisplayFormat_OverridingDataType() { // Arrange var provider = new DataAnnotationsMetadataProvider(); @@ -111,7 +111,7 @@ namespace Microsoft.AspNet.Mvc.ModelBinding.Metadata } [Fact] - public void GetDisplayDetails_EditableAttributeFalse_SetsReadOnlyTrue() + public void GetBindingMetadata_EditableAttributeFalse_SetsReadOnlyTrue() { // Arrange var provider = new DataAnnotationsMetadataProvider(); @@ -130,7 +130,7 @@ namespace Microsoft.AspNet.Mvc.ModelBinding.Metadata } [Fact] - public void GetDisplayDetails_EditableAttributeTrue_SetsReadOnlyFalse() + public void GetBindingMetadata_EditableAttributeTrue_SetsReadOnlyFalse() { // Arrange var provider = new DataAnnotationsMetadataProvider(); @@ -148,28 +148,10 @@ namespace Microsoft.AspNet.Mvc.ModelBinding.Metadata Assert.False(context.BindingMetadata.IsReadOnly); } - [Fact] - public void GetDisplayDetails_RequiredAttribute_SetsRequired() - { - // Arrange - var provider = new DataAnnotationsMetadataProvider(); - - var required = new RequiredAttribute(); - - var attributes = new Attribute[] { required }; - var key = ModelMetadataIdentity.ForType(typeof(string)); - var context = new BindingMetadataProviderContext(key, attributes); - - // Act - provider.GetBindingMetadata(context); - - // Assert - Assert.Equal(true, context.BindingMetadata.IsRequired); - } // This is IMPORTANT. Product code needs to use GetName() instead of .Name. It's easy to regress. [Fact] - public void GetDisplayDetails_DisplayAttribute_NameFromResources() + public void GetDisplayMetadata_DisplayAttribute_NameFromResources() { // Arrange var provider = new DataAnnotationsMetadataProvider(); @@ -198,7 +180,7 @@ namespace Microsoft.AspNet.Mvc.ModelBinding.Metadata // This is IMPORTANT. Product code needs to use GetDescription() instead of .Description. It's easy to regress. [Fact] - public void GetDisplayDetails_DisplayAttribute_DescriptionFromResources() + public void GetDisplayMetadata_DisplayAttribute_DescriptionFromResources() { // Arrange var provider = new DataAnnotationsMetadataProvider(); @@ -243,7 +225,7 @@ namespace Microsoft.AspNet.Mvc.ModelBinding.Metadata [InlineData(typeof(StructWithFields), false)] [InlineData(typeof(StructWithFields?), false)] [InlineData(typeof(StructWithProperties), false)] - public void GetDisplayDetails_IsEnum_ReflectsModelType(Type type, bool expectedIsEnum) + public void GetDisplayMetadata_IsEnum_ReflectsModelType(Type type, bool expectedIsEnum) { // Arrange var provider = new DataAnnotationsMetadataProvider(); @@ -277,7 +259,7 @@ namespace Microsoft.AspNet.Mvc.ModelBinding.Metadata [InlineData(typeof(StructWithFields), false)] [InlineData(typeof(StructWithFields?), false)] [InlineData(typeof(StructWithProperties), false)] - public void GetDisplayDetails_IsFlagsEnum_ReflectsModelType(Type type, bool expectedIsFlagsEnum) + public void GetDisplayMetadata_IsFlagsEnum_ReflectsModelType(Type type, bool expectedIsFlagsEnum) { // Arrange var provider = new DataAnnotationsMetadataProvider(); @@ -407,7 +389,7 @@ namespace Microsoft.AspNet.Mvc.ModelBinding.Metadata [Theory] [MemberData(nameof(EnumNamesData))] - public void GetDisplayDetails_EnumNamesAndValues_ReflectsModelType( + public void GetDisplayMetadata_EnumNamesAndValues_ReflectsModelType( Type type, IReadOnlyDictionary expectedDictionary) { @@ -541,7 +523,7 @@ namespace Microsoft.AspNet.Mvc.ModelBinding.Metadata [Theory] [MemberData(nameof(EnumDisplayNamesData))] - public void GetDisplayDetails_EnumDisplayNamesAndValues_ReflectsModelType( + public void GetDisplayMetadata_EnumDisplayNamesAndValues_ReflectsModelType( Type type, IEnumerable> expectedKeyValuePairs) { @@ -560,7 +542,7 @@ namespace Microsoft.AspNet.Mvc.ModelBinding.Metadata } [Fact] - public void GetBindingDetails_RequiredAttribute_SetsIsRequiredToTrue() + public void GetValidationMetadata_RequiredAttribute_SetsIsRequiredToTrue() { // Arrange var provider = new DataAnnotationsMetadataProvider(); @@ -569,34 +551,55 @@ namespace Microsoft.AspNet.Mvc.ModelBinding.Metadata var attributes = new Attribute[] { required }; var key = ModelMetadataIdentity.ForProperty(typeof(int), "Length", typeof(string)); - var context = new BindingMetadataProviderContext(key, attributes); + var context = new ValidationMetadataProviderContext(key, attributes); // Act - provider.GetBindingMetadata(context); + provider.GetValidationMetadata(context); // Assert - Assert.True(context.BindingMetadata.IsRequired); + Assert.True(context.ValidationMetadata.IsRequired); } [Theory] [InlineData(true)] [InlineData(false)] [InlineData(null)] - public void GetBindingDetails_NoRequiredAttribute_IsRequiredLeftAlone(bool? initialValue) + public void GetValidationMetadata_NoRequiredAttribute_IsRequiredLeftAlone(bool? initialValue) { // Arrange var provider = new DataAnnotationsMetadataProvider(); var attributes = new Attribute[] { }; var key = ModelMetadataIdentity.ForProperty(typeof(int), "Length", typeof(string)); + var context = new ValidationMetadataProviderContext(key, attributes); + context.ValidationMetadata.IsRequired = initialValue; + + // Act + provider.GetValidationMetadata(context); + + // Assert + Assert.Equal(initialValue, context.ValidationMetadata.IsRequired); + } + + // [Required] has no effect on IsBindingRequired + [Theory] + [InlineData(true)] + [InlineData(false)] + public void GetBindingMetadata_RequiredAttribute_IsBindingRequiredLeftAlone(bool initialValue) + { + // Arrange + var provider = new DataAnnotationsMetadataProvider(); + + var attributes = new Attribute[] { new RequiredAttribute() }; + var key = ModelMetadataIdentity.ForProperty(typeof(int), "Length", typeof(string)); var context = new BindingMetadataProviderContext(key, attributes); - context.BindingMetadata.IsRequired = initialValue; + context.BindingMetadata.IsBindingRequired = initialValue; // Act provider.GetBindingMetadata(context); // Assert - Assert.Equal(initialValue, context.BindingMetadata.IsRequired); + Assert.Equal(initialValue, context.BindingMetadata.IsBindingRequired); } [Theory] diff --git a/test/Microsoft.AspNet.Mvc.ModelBinding.Test/Metadata/DataMemberRequiredBindingMetadataProviderTest.cs b/test/Microsoft.AspNet.Mvc.ModelBinding.Test/Metadata/DataMemberRequiredBindingMetadataProviderTest.cs index 9b26516bc5..15cdc1683e 100644 --- a/test/Microsoft.AspNet.Mvc.ModelBinding.Test/Metadata/DataMemberRequiredBindingMetadataProviderTest.cs +++ b/test/Microsoft.AspNet.Mvc.ModelBinding.Test/Metadata/DataMemberRequiredBindingMetadataProviderTest.cs @@ -9,7 +9,7 @@ namespace Microsoft.AspNet.Mvc.ModelBinding.Metadata public class DataMemberRequiredBindingMetadataProviderTest { [Fact] - public void IsRequired_SetToTrue_WithDataMemberIsRequiredTrue() + public void IsBindingRequired_SetToTrue_WithDataMemberIsRequiredTrue() { // Arrange var provider = new DataMemberRequiredBindingMetadataProvider(); @@ -29,14 +29,13 @@ namespace Microsoft.AspNet.Mvc.ModelBinding.Metadata provider.GetBindingMetadata(context); // Assert - Assert.True(context.BindingMetadata.IsRequired); + Assert.True(context.BindingMetadata.IsBindingRequired); } [Theory] [InlineData(true)] [InlineData(false)] - [InlineData(null)] - public void IsRequired_LeftAlone_DataMemberIsRequiredFalse(bool? initialValue) + public void IsBindingRequired_LeftAlone_DataMemberIsRequiredFalse(bool initialValue) { // Arrange var provider = new DataMemberRequiredBindingMetadataProvider(); @@ -52,20 +51,19 @@ namespace Microsoft.AspNet.Mvc.ModelBinding.Metadata typeof(ClassWithDataMemberIsRequiredFalse)); var context = new BindingMetadataProviderContext(key, attributes); - context.BindingMetadata.IsRequired = initialValue; + context.BindingMetadata.IsBindingRequired = initialValue; // Act provider.GetBindingMetadata(context); // Assert - Assert.Equal(initialValue, context.BindingMetadata.IsRequired); + Assert.Equal(initialValue, context.BindingMetadata.IsBindingRequired); } [Theory] [InlineData(true)] [InlineData(false)] - [InlineData(null)] - public void IsRequired_LeftAlone_ForNonPropertyMetadata(bool? initialValue) + public void IsBindingRequired_LeftAlone_ForNonPropertyMetadata(bool initialValue) { // Arrange var provider = new DataMemberRequiredBindingMetadataProvider(); @@ -78,20 +76,19 @@ namespace Microsoft.AspNet.Mvc.ModelBinding.Metadata var key = ModelMetadataIdentity.ForType(typeof(ClassWithDataMemberIsRequiredTrue)); var context = new BindingMetadataProviderContext(key, attributes); - context.BindingMetadata.IsRequired = initialValue; + context.BindingMetadata.IsBindingRequired = initialValue; // Act provider.GetBindingMetadata(context); // Assert - Assert.Equal(initialValue, context.BindingMetadata.IsRequired); + Assert.Equal(initialValue, context.BindingMetadata.IsBindingRequired); } [Theory] [InlineData(true)] [InlineData(false)] - [InlineData(null)] - public void IsRequired_LeftAlone_WithoutDataMemberAttribute(bool? initialValue) + public void IsBindingRequired_LeftAlone_WithoutDataMemberAttribute(bool initialValue) { // Arrange var provider = new DataMemberRequiredBindingMetadataProvider(); @@ -102,20 +99,19 @@ namespace Microsoft.AspNet.Mvc.ModelBinding.Metadata typeof(ClassWithoutAttributes)); var context = new BindingMetadataProviderContext(key, attributes: new object[0]); - context.BindingMetadata.IsRequired = initialValue; + context.BindingMetadata.IsBindingRequired = initialValue; // Act provider.GetBindingMetadata(context); // Assert - Assert.Equal(initialValue, context.BindingMetadata.IsRequired); + Assert.Equal(initialValue, context.BindingMetadata.IsBindingRequired); } [Theory] [InlineData(true)] [InlineData(false)] - [InlineData(null)] - public void IsRequired_LeftAlone_WithoutDataContractAttribute(bool? initialValue) + public void IsBindingRequired_LeftAlone_WithoutDataContractAttribute(bool initialValue) { // Arrange var provider = new DataMemberRequiredBindingMetadataProvider(); @@ -131,13 +127,13 @@ namespace Microsoft.AspNet.Mvc.ModelBinding.Metadata typeof(ClassWithDataMemberIsRequiredTrueWithoutDataContract)); var context = new BindingMetadataProviderContext(key, attributes); - context.BindingMetadata.IsRequired = initialValue; + context.BindingMetadata.IsBindingRequired = initialValue; // Act provider.GetBindingMetadata(context); // Assert - Assert.Equal(initialValue, context.BindingMetadata.IsRequired); + Assert.Equal(initialValue, context.BindingMetadata.IsBindingRequired); } [DataContract] diff --git a/test/Microsoft.AspNet.Mvc.ModelBinding.Test/Metadata/DefaultModelMetadataTest.cs b/test/Microsoft.AspNet.Mvc.ModelBinding.Test/Metadata/DefaultModelMetadataTest.cs index 7fbe56a6ba..79e43c72e7 100644 --- a/test/Microsoft.AspNet.Mvc.ModelBinding.Test/Metadata/DefaultModelMetadataTest.cs +++ b/test/Microsoft.AspNet.Mvc.ModelBinding.Test/Metadata/DefaultModelMetadataTest.cs @@ -36,6 +36,7 @@ namespace Microsoft.AspNet.Mvc.ModelBinding.Metadata Assert.False(metadata.HasNonDefaultEditFormat); Assert.False(metadata.HideSurroundingHtml); Assert.True(metadata.HtmlEncode); + Assert.False(metadata.IsBindingRequired); Assert.False(metadata.IsComplexType); Assert.False(metadata.IsCollectionType); Assert.False(metadata.IsEnum); diff --git a/test/Microsoft.AspNet.Mvc.ModelBinding.Test/Metadata/ModelMetadataProviderTest.cs b/test/Microsoft.AspNet.Mvc.ModelBinding.Test/Metadata/ModelMetadataProviderTest.cs index 84f3b60158..2c10466e94 100644 --- a/test/Microsoft.AspNet.Mvc.ModelBinding.Test/Metadata/ModelMetadataProviderTest.cs +++ b/test/Microsoft.AspNet.Mvc.ModelBinding.Test/Metadata/ModelMetadataProviderTest.cs @@ -637,7 +637,7 @@ namespace Microsoft.AspNet.Mvc.ModelBinding.Metadata } [Fact] - public void IsRequired_DataMemberIsRequiredTrue_SetsIsRequiredToTrue() + public void IsBindingRequired_DataMemberIsRequiredTrue_SetsIsBindingRequiredToTrue() { // Arrange var metadataProvider = TestModelMetadataProvider.CreateDefaultProvider(); @@ -648,11 +648,11 @@ namespace Microsoft.AspNet.Mvc.ModelBinding.Metadata nameof(ClassWithDataMemberIsRequiredTrue.StringProperty)); // Assert - Assert.True(metadata.IsRequired); + Assert.True(metadata.IsBindingRequired); } [Fact] - public void IsRequired_DataMemberIsRequiredFalse_FalseForReferenceType() + public void IsBindingRequired_DataMemberIsRequiredFalse_False() { // Arrange var metadataProvider = TestModelMetadataProvider.CreateDefaultProvider(); @@ -663,26 +663,11 @@ namespace Microsoft.AspNet.Mvc.ModelBinding.Metadata nameof(ClassWithDataMemberIsRequiredFalse.StringProperty)); // Assert - Assert.False(metadata.IsRequired); + Assert.False(metadata.IsBindingRequired); } [Fact] - public void IsRequired_DataMemberIsRequiredFalse_TrueForValueType() - { - // Arrange - var metadataProvider = TestModelMetadataProvider.CreateDefaultProvider(); - - // Act - var metadata = metadataProvider.GetMetadataForProperty( - typeof(ClassWithDataMemberIsRequiredFalse), - nameof(ClassWithDataMemberIsRequiredFalse.IntProperty)); - - // Assert - Assert.True(metadata.IsRequired); - } - - [Fact] - public void IsRequired_DataMemberIsRequiredTrueWithoutDataContract_False() + public void IsBindingRequiredDataMemberIsRequiredTrueWithoutDataContract_False() { // Arrange var metadataProvider = TestModelMetadataProvider.CreateDefaultProvider(); @@ -693,7 +678,7 @@ namespace Microsoft.AspNet.Mvc.ModelBinding.Metadata nameof(ClassWithDataMemberIsRequiredTrueWithoutDataContract.StringProperty)); // Assert - Assert.False(metadata.IsRequired); + Assert.False(metadata.IsBindingRequired); } [Fact] diff --git a/test/Microsoft.AspNet.Mvc.ModelBinding.Test/Metadata/ModelMetadataTest.cs b/test/Microsoft.AspNet.Mvc.ModelBinding.Test/Metadata/ModelMetadataTest.cs index 2ec98eea4b..26d6f281dd 100644 --- a/test/Microsoft.AspNet.Mvc.ModelBinding.Test/Metadata/ModelMetadataTest.cs +++ b/test/Microsoft.AspNet.Mvc.ModelBinding.Test/Metadata/ModelMetadataTest.cs @@ -309,6 +309,14 @@ namespace Microsoft.AspNet.Mvc.ModelBinding } } + public override bool IsBindingRequired + { + get + { + throw new NotImplementedException(); + } + } + public override bool IsEnum { get diff --git a/test/WebSites/ModelBindingWebSite/Controllers/BindingBehaviorController.cs b/test/WebSites/ModelBindingWebSite/Controllers/BindingBehaviorController.cs new file mode 100644 index 0000000000..fd2e0ff56b --- /dev/null +++ b/test/WebSites/ModelBindingWebSite/Controllers/BindingBehaviorController.cs @@ -0,0 +1,23 @@ +// Copyright (c) Microsoft Open Technologies, Inc. All rights reserved. +// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. + +using Microsoft.AspNet.Mvc; + +namespace ModelBindingWebSite +{ + [Route("BindingBehavior")] + public class BindingBehaviorController : Controller + { + // If the results are valid you get back the model, otherwise you get the ModelState errors. + [HttpPost("EchoModelValues")] + public object EchoModelValues(BindingBehaviorModel model) + { + if (!ModelState.IsValid) + { + return HttpBadRequest(ModelState); + } + + return model; + } + } +} diff --git a/test/WebSites/ModelBindingWebSite/Controllers/DataMemberRequiredController.cs b/test/WebSites/ModelBindingWebSite/Controllers/DataMemberRequiredController.cs new file mode 100644 index 0000000000..ab05f86d48 --- /dev/null +++ b/test/WebSites/ModelBindingWebSite/Controllers/DataMemberRequiredController.cs @@ -0,0 +1,23 @@ +// Copyright (c) Microsoft Open Technologies, Inc. All rights reserved. +// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. + +using Microsoft.AspNet.Mvc; + +namespace ModelBindingWebSite +{ + [Route("DataMemberRequired")] + public class DataMemberRequiredController : Controller + { + // If the results are valid you get back the model, otherwise you get the ModelState errors. + [HttpPost("EchoModelValues")] + public object EchoModelValues(DataMemberRequiredModel model) + { + if (!ModelState.IsValid) + { + return HttpBadRequest(ModelState); + } + + return model; + } + } +} diff --git a/test/WebSites/ModelBindingWebSite/Models/BindingBehaviorModel.cs b/test/WebSites/ModelBindingWebSite/Models/BindingBehaviorModel.cs new file mode 100644 index 0000000000..1a88cb9c55 --- /dev/null +++ b/test/WebSites/ModelBindingWebSite/Models/BindingBehaviorModel.cs @@ -0,0 +1,25 @@ +// Copyright (c) Microsoft Open Technologies, Inc. All rights reserved. +// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. + +using Microsoft.AspNet.Mvc.ModelBinding; + +namespace ModelBindingWebSite +{ + public class BindingBehaviorModel + { + [BindingBehavior(BindingBehavior.Never)] + public string BehaviourNeverProperty { get; set; } + + [BindingBehavior(BindingBehavior.Optional)] + public string BehaviourOptionalProperty { get; set; } + + [BindingBehavior(BindingBehavior.Required)] + public string BehaviourRequiredProperty { get; set; } + + [BindRequired] + public string BindRequiredProperty { get; set; } + + [BindNever] + public string BindNeverProperty { get; set; } + } +} diff --git a/test/WebSites/ModelBindingWebSite/Models/DataMemberRequiredModel.cs b/test/WebSites/ModelBindingWebSite/Models/DataMemberRequiredModel.cs new file mode 100644 index 0000000000..8f0d347cff --- /dev/null +++ b/test/WebSites/ModelBindingWebSite/Models/DataMemberRequiredModel.cs @@ -0,0 +1,20 @@ +// Copyright (c) Microsoft Open Technologies, Inc. All rights reserved. +// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. + +using System.Runtime.Serialization; + +namespace ModelBindingWebSite +{ + [DataContract] + public class DataMemberRequiredModel + { + [DataMember] + public string ImplicitlyOptionalProperty { get; set; } + + [DataMember(IsRequired = false)] + public string ExplicitlyOptionalProperty { get; set; } + + [DataMember(IsRequired = true)] + public string RequiredProperty { get; set; } + } +}