From 3fd4991959ee3d150e63fffe2e28d3a97c52b21b Mon Sep 17 00:00:00 2001 From: Doug Bunting Date: Thu, 23 Apr 2015 20:11:09 -0700 Subject: [PATCH] Bind to readonly non-`null` collections - part 1/2 of #2294 - handle readonly non-`null` collections in relevant binders - `CollectionModelBinder.CopyToModel()` and `MutableObjectModelBinder.AddToProperty()` methods - handle read-only controller properties in `DefaultControllerActionArgumentBinder` - do not copy into arrays e.g. add `CopyToModel()` override in `ArrayModelBinder` - remove ability to set a private controller property - confirm `SetMethod.IsPublic` in `DefaultControllerActionArgumentBinder` - avoid NREs in `GetModel()` overrides Test handling of readonly collections - previous tests barely touched this scenario - also add more tests setting controller properties nits: - add missing `[NotNull]` attributes - add missing doc comments - consolidate a few `[Fact]`s into `[Theory]`s - simplify some wrapping; shorten a few lines - remove dead code in `DefaultControllerActionArgumentBinder` and `ControllerActionArgumentBinderTests` --- .../DefaultControllerActionArgumentBinder.cs | 70 ++++- .../Binders/ArrayModelBinder.cs | 18 +- .../Binders/CollectionModelBinder.cs | 99 +++++- .../Binders/DictionaryModelBinder.cs | 9 +- .../Binders/MutableObjectModelBinder.cs | 165 ++++++++-- .../ControllerActionArgumentBinderTests.cs | 289 +++++++++++++----- .../Binders/ArrayModelBinderTest.cs | 79 ++++- .../Binders/CollectionModelBinderTest.cs | 109 ++++++- .../Binders/DictionaryModelBinderTest.cs | 84 +++-- .../Binders/MutableObjectModelBinderTest.cs | 240 +++++++++++---- 10 files changed, 925 insertions(+), 237 deletions(-) diff --git a/src/Microsoft.AspNet.Mvc.Core/DefaultControllerActionArgumentBinder.cs b/src/Microsoft.AspNet.Mvc.Core/DefaultControllerActionArgumentBinder.cs index 4f829bf454..183b415d29 100644 --- a/src/Microsoft.AspNet.Mvc.Core/DefaultControllerActionArgumentBinder.cs +++ b/src/Microsoft.AspNet.Mvc.Core/DefaultControllerActionArgumentBinder.cs @@ -4,12 +4,12 @@ using System; using System.Collections.Generic; using System.Linq; +using System.Reflection; using System.Threading.Tasks; using Microsoft.AspNet.Mvc.Core; using Microsoft.AspNet.Mvc.ModelBinding; using Microsoft.AspNet.Mvc.ModelBinding.Validation; using Microsoft.Framework.Internal; -using Microsoft.Framework.OptionsModel; namespace Microsoft.AspNet.Mvc { @@ -19,6 +19,10 @@ namespace Microsoft.AspNet.Mvc /// public class DefaultControllerActionArgumentBinder : IControllerActionArgumentBinder { + private static readonly MethodInfo CallPropertyAddRangeOpenGenericMethod = + typeof(DefaultControllerActionArgumentBinder).GetTypeInfo().GetDeclaredMethod( + nameof(CallPropertyAddRange)); + private readonly IModelMetadataProvider _modelMetadataProvider; private readonly IObjectModelValidator _validator; @@ -64,12 +68,11 @@ namespace Microsoft.AspNet.Mvc } public async Task BindModelAsync( - ParameterDescriptor parameter, - ModelStateDictionary modelState, - OperationBindingContext operationContext) + [NotNull] ParameterDescriptor parameter, + [NotNull] ModelStateDictionary modelState, + [NotNull] OperationBindingContext operationContext) { var metadata = _modelMetadataProvider.GetMetadataForType(parameter.ParameterType); - var parameterType = parameter.ParameterType; var modelBindingContext = GetModelBindingContext( parameter.Name, metadata, @@ -98,6 +101,21 @@ namespace Microsoft.AspNet.Mvc return modelBindingResult; } + // Called via reflection. + private static void CallPropertyAddRange(object target, object source) + { + var targetCollection = (ICollection)target; + var sourceCollection = source as IEnumerable; + if (sourceCollection != null && !targetCollection.IsReadOnly) + { + targetCollection.Clear(); + foreach (var item in sourceCollection) + { + targetCollection.Add(item); + } + } + } + private void ActivateProperties(object controller, Type containerType, Dictionary properties) { var propertyHelpers = PropertyHelper.GetProperties(controller); @@ -105,17 +123,47 @@ namespace Microsoft.AspNet.Mvc { var propertyHelper = propertyHelpers.First(helper => string.Equals(helper.Name, property.Key, StringComparison.Ordinal)); - if (propertyHelper.Property == null || !propertyHelper.Property.CanWrite) + var propertyType = propertyHelper.Property.PropertyType; + var source = property.Value; + if (propertyHelper.Property.CanWrite && propertyHelper.Property.SetMethod?.IsPublic == true) { - // nothing to do - return; + // Handle settable property. Do not set the property if the type is a non-nullable type. + if (source != null || propertyType.AllowsNullValue()) + { + propertyHelper.SetValue(controller, source); + } + + continue; } - // Do not set the property if the type is a non nullable type. - if (property.Value != null || propertyHelper.Property.PropertyType.AllowsNullValue()) + if (propertyType.IsArray) { - propertyHelper.SetValue(controller, property.Value); + // Do not attempt to copy values into an array because an array's length is immutable. This choice + // is also consistent with MutableObjectModelBinder's handling of a read-only array property. + continue; } + + var target = propertyHelper.GetValue(controller); + if (source == null || target == null) + { + // Nothing to do when source or target is null. + continue; + } + + // Determine T if this is an ICollection property. + var collectionTypeArguments = propertyType + .ExtractGenericInterface(typeof(ICollection<>)) + ?.GenericTypeArguments; + if (collectionTypeArguments == null) + { + // Not a collection model. + continue; + } + + // Handle a read-only collection property. + var propertyAddRange = CallPropertyAddRangeOpenGenericMethod.MakeGenericMethod( + collectionTypeArguments); + propertyAddRange.Invoke(obj: null, parameters: new[] { target, source }); } } diff --git a/src/Microsoft.AspNet.Mvc.ModelBinding/Binders/ArrayModelBinder.cs b/src/Microsoft.AspNet.Mvc.ModelBinding/Binders/ArrayModelBinder.cs index 414903b27c..bfeab3a657 100644 --- a/src/Microsoft.AspNet.Mvc.ModelBinding/Binders/ArrayModelBinder.cs +++ b/src/Microsoft.AspNet.Mvc.ModelBinding/Binders/ArrayModelBinder.cs @@ -4,12 +4,18 @@ using System.Collections.Generic; using System.Linq; using System.Threading.Tasks; +using Microsoft.Framework.Internal; namespace Microsoft.AspNet.Mvc.ModelBinding { + /// + /// implementation for binding array values. + /// + /// Type of elements in the array. public class ArrayModelBinder : CollectionModelBinder { - public override Task BindModelAsync(ModelBindingContext bindingContext) + /// + public override Task BindModelAsync([NotNull] ModelBindingContext bindingContext) { if (bindingContext.ModelMetadata.IsReadOnly) { @@ -19,9 +25,17 @@ namespace Microsoft.AspNet.Mvc.ModelBinding return base.BindModelAsync(bindingContext); } + /// protected override object GetModel(IEnumerable newCollection) { - return newCollection.ToArray(); + return newCollection?.ToArray(); + } + + /// + protected override void CopyToModel([NotNull] object target, IEnumerable sourceCollection) + { + // Do not attempt to copy values into an array because an array's length is immutable. This choice is also + // consistent with MutableObjectModelBinder's handling of a read-only array property. } } } diff --git a/src/Microsoft.AspNet.Mvc.ModelBinding/Binders/CollectionModelBinder.cs b/src/Microsoft.AspNet.Mvc.ModelBinding/Binders/CollectionModelBinder.cs index 54cf34a56b..7f8e6877c3 100644 --- a/src/Microsoft.AspNet.Mvc.ModelBinding/Binders/CollectionModelBinder.cs +++ b/src/Microsoft.AspNet.Mvc.ModelBinding/Binders/CollectionModelBinder.cs @@ -4,16 +4,23 @@ using System; using System.Collections; using System.Collections.Generic; +using System.Diagnostics; using System.Globalization; using System.Linq; using System.Threading.Tasks; using Microsoft.AspNet.Mvc.ModelBinding.Internal; +using Microsoft.Framework.Internal; namespace Microsoft.AspNet.Mvc.ModelBinding { + /// + /// implementation for binding collection values. + /// + /// Type of elements in the collection. public class CollectionModelBinder : IModelBinder { - public virtual async Task BindModelAsync(ModelBindingContext bindingContext) + /// + public virtual async Task BindModelAsync([NotNull] ModelBindingContext bindingContext) { ModelBindingHelper.ValidateBindingContext(bindingContext); @@ -23,19 +30,40 @@ namespace Microsoft.AspNet.Mvc.ModelBinding } var valueProviderResult = await bindingContext.ValueProvider.GetValueAsync(bindingContext.ModelName); - var bindCollectionTask = valueProviderResult != null ? - BindSimpleCollection(bindingContext, valueProviderResult.RawValue, valueProviderResult.Culture) : - BindComplexCollection(bindingContext); - var boundCollection = await bindCollectionTask; - var model = GetModel(boundCollection); + + IEnumerable boundCollection; + if (valueProviderResult == null) + { + boundCollection = await BindComplexCollection(bindingContext); + } + else + { + boundCollection = await BindSimpleCollection( + bindingContext, + valueProviderResult.RawValue, + valueProviderResult.Culture); + } + + var model = bindingContext.Model; + if (model == null) + { + model = GetModel(boundCollection); + } + else + { + // Special case for TryUpdateModelAsync(collection, ...) scenarios. Model is null in all other cases. + CopyToModel(model, boundCollection); + } + return new ModelBindingResult(model, bindingContext.ModelName, isModelSet: true); } // Used when the ValueProvider contains the collection to be bound as a single element, e.g. the raw value // is [ "1", "2" ] and needs to be converted to an int[]. - internal async Task> BindSimpleCollection(ModelBindingContext bindingContext, - object rawValue, - CultureInfo culture) + internal async Task> BindSimpleCollection( + ModelBindingContext bindingContext, + object rawValue, + CultureInfo culture) { if (rawValue == null) { @@ -62,7 +90,8 @@ namespace Microsoft.AspNet.Mvc.ModelBinding }; object boundValue = null; - var result = await bindingContext.OperationBindingContext.ModelBinder.BindModelAsync(innerBindingContext); + var result = + await bindingContext.OperationBindingContext.ModelBinder.BindModelAsync(innerBindingContext); if (result != null) { boundValue = result.Model; @@ -79,11 +108,13 @@ namespace Microsoft.AspNet.Mvc.ModelBinding var indexPropertyName = ModelBindingHelper.CreatePropertyModelName(bindingContext.ModelName, "index"); var valueProviderResultIndex = await bindingContext.ValueProvider.GetValueAsync(indexPropertyName); var indexNames = CollectionModelBinderUtil.GetIndexNamesFromValueProviderResult(valueProviderResultIndex); + return await BindComplexCollectionFromIndexes(bindingContext, indexNames); } - internal async Task> BindComplexCollectionFromIndexes(ModelBindingContext bindingContext, - IEnumerable indexNames) + internal async Task> BindComplexCollectionFromIndexes( + ModelBindingContext bindingContext, + IEnumerable indexNames) { bool indexNamesIsFinite; if (indexNames != null) @@ -114,7 +145,8 @@ namespace Microsoft.AspNet.Mvc.ModelBinding var modelType = bindingContext.ModelType; - var result = await bindingContext.OperationBindingContext.ModelBinder.BindModelAsync(childBindingContext); + var result = + await bindingContext.OperationBindingContext.ModelBinder.BindModelAsync(childBindingContext); if (result != null) { didBind = true; @@ -133,13 +165,50 @@ namespace Microsoft.AspNet.Mvc.ModelBinding return boundCollection; } - // Extensibility point that allows the bound collection to be manipulated or transformed before - // being returned from the binder. + /// + /// Gets an assignable to the collection property. + /// + /// + /// Collection of values retrieved from value providers. Or null if nothing was bound. + /// + /// + /// assignable to the collection property. Or null if nothing was bound. + /// + /// + /// Extensibility point that allows the bound collection to be manipulated or transformed before being + /// returned from the binder. + /// protected virtual object GetModel(IEnumerable newCollection) { + // Depends on fact BindSimpleCollection() and BindComplexCollection() always return a List + // instance or null. In addition GenericModelBinder confirms a List is assignable to the + // property prior to instantiating this binder and subclass binders do not call this method. return newCollection; } + /// + /// Adds values from to given . + /// + /// into which values are copied. + /// + /// Collection of values retrieved from value providers. Or null if nothing was bound. + /// + /// Called only in TryUpdateModelAsync(collection, ...) scenarios. + protected virtual void CopyToModel([NotNull] object target, IEnumerable sourceCollection) + { + var targetCollection = target as ICollection; + Debug.Assert(targetCollection != null); // This binder is instantiated only for ICollection model types. + + if (sourceCollection != null && targetCollection != null && !targetCollection.IsReadOnly) + { + targetCollection.Clear(); + foreach (var element in sourceCollection) + { + targetCollection.Add(element); + } + } + } + internal static object[] RawValueToObjectArray(object rawValue) { // precondition: rawValue is not null diff --git a/src/Microsoft.AspNet.Mvc.ModelBinding/Binders/DictionaryModelBinder.cs b/src/Microsoft.AspNet.Mvc.ModelBinding/Binders/DictionaryModelBinder.cs index 328670a8f4..f8fc5389bd 100644 --- a/src/Microsoft.AspNet.Mvc.ModelBinding/Binders/DictionaryModelBinder.cs +++ b/src/Microsoft.AspNet.Mvc.ModelBinding/Binders/DictionaryModelBinder.cs @@ -1,17 +1,22 @@ // 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.Linq; namespace Microsoft.AspNet.Mvc.ModelBinding { + /// + /// implementation for binding dictionary values. + /// + /// Type of keys in the dictionary. + /// Type of values in the dictionary. public class DictionaryModelBinder : CollectionModelBinder> { + /// protected override object GetModel(IEnumerable> newCollection) { - return newCollection.ToDictionary(kvp => kvp.Key, kvp => kvp.Value); + return newCollection?.ToDictionary(kvp => kvp.Key, kvp => kvp.Value); } } } diff --git a/src/Microsoft.AspNet.Mvc.ModelBinding/Binders/MutableObjectModelBinder.cs b/src/Microsoft.AspNet.Mvc.ModelBinding/Binders/MutableObjectModelBinder.cs index 7d7d25bcaf..92dcd1597f 100644 --- a/src/Microsoft.AspNet.Mvc.ModelBinding/Binders/MutableObjectModelBinder.cs +++ b/src/Microsoft.AspNet.Mvc.ModelBinding/Binders/MutableObjectModelBinder.cs @@ -9,12 +9,20 @@ using System.Reflection; using System.Threading.Tasks; using Microsoft.AspNet.Mvc.ModelBinding.Internal; using Microsoft.AspNet.Mvc.ModelBinding.Validation; +using Microsoft.Framework.Internal; namespace Microsoft.AspNet.Mvc.ModelBinding { + /// + /// implementation for binding complex values. + /// public class MutableObjectModelBinder : IModelBinder { - public virtual async Task BindModelAsync(ModelBindingContext bindingContext) + private static readonly MethodInfo CallPropertyAddRangeOpenGenericMethod = + typeof(MutableObjectModelBinder).GetTypeInfo().GetDeclaredMethod(nameof(CallPropertyAddRange)); + + /// + public virtual async Task BindModelAsync([NotNull] ModelBindingContext bindingContext) { ModelBindingHelper.ValidateBindingContext(bindingContext); if (!CanBindType(bindingContext.ModelMetadata)) @@ -44,7 +52,13 @@ namespace Microsoft.AspNet.Mvc.ModelBinding isModelSet: true); } - protected virtual bool CanUpdateProperty(ModelMetadata propertyMetadata) + /// + /// Gets an indication whether a property with the given can be updated. + /// + /// for the property of interest. + /// true if the property can be updated; false otherwise. + /// Should return true only for properties can update. + protected virtual bool CanUpdateProperty([NotNull] ModelMetadata propertyMetadata) { return CanUpdatePropertyInternal(propertyMetadata); } @@ -255,14 +269,24 @@ namespace Microsoft.AspNet.Mvc.ModelBinding return await bindingContext.OperationBindingContext.ModelBinder.BindModelAsync(childContext); } - protected virtual object CreateModel(ModelBindingContext bindingContext) + /// + /// Creates suitable for given . + /// + /// The . + /// An compatible with . + protected virtual object CreateModel([NotNull] ModelBindingContext bindingContext) { // If the Activator throws an exception, we want to propagate it back up the call stack, since the // application developer should know that this was an invalid type to try to bind to. return Activator.CreateInstance(bindingContext.ModelType); } - protected virtual void EnsureModel(ModelBindingContext bindingContext) + /// + /// Ensures is not null in given + /// . + /// + /// The . + protected virtual void EnsureModel([NotNull] ModelBindingContext bindingContext) { if (bindingContext.Model == null) { @@ -270,7 +294,13 @@ namespace Microsoft.AspNet.Mvc.ModelBinding } } - protected virtual IEnumerable GetMetadataForProperties(ModelBindingContext bindingContext) + /// + /// Gets the collection of for properties this binder should update. + /// + /// The . + /// Collection of for properties this binder should update. + protected virtual IEnumerable GetMetadataForProperties( + [NotNull] ModelBindingContext bindingContext) { var validationInfo = GetPropertyValidationInfo(bindingContext); var newPropertyFilter = GetPropertyFilter(); @@ -404,11 +434,25 @@ namespace Microsoft.AspNet.Mvc.ModelBinding } } + /// + /// Updates a property in the current . + /// + /// The . + /// + /// The for the model containing property to set. + /// + /// The for the property to set. + /// The for the property's new value. + /// + /// The which ensures the value is not null. Or null if no such + /// requirement exists. + /// + /// Should succeed in all cases that returns true. protected virtual void SetProperty( - ModelBindingContext bindingContext, - ModelExplorer modelExplorer, - ModelMetadata propertyMetadata, - ModelBindingResult dtoResult, + [NotNull] ModelBindingContext bindingContext, + [NotNull] ModelExplorer modelExplorer, + [NotNull] ModelMetadata propertyMetadata, + [NotNull] ModelBindingResult dtoResult, IModelValidator requiredValidator) { var bindingFlags = BindingFlags.Instance | BindingFlags.Public | BindingFlags.IgnoreCase; @@ -416,9 +460,16 @@ namespace Microsoft.AspNet.Mvc.ModelBinding propertyMetadata.PropertyName, bindingFlags); - if (property == null || !property.CanWrite) + if (property == null) { - // nothing to do + // Nothing to do if property does not exist. + return; + } + + if (!property.CanWrite) + { + // Try to handle as a collection if property exists but is not settable. + AddToProperty(bindingContext, modelExplorer, property, dtoResult); return; } @@ -466,21 +517,9 @@ namespace Microsoft.AspNet.Mvc.ModelBinding { propertyMetadata.PropertySetter(bindingContext.Model, value); } - catch (Exception ex) + catch (Exception exception) { - // don't display a duplicate error message if a binding error has already occurred for this field - var targetInvocationException = ex as TargetInvocationException; - if (targetInvocationException != null && - targetInvocationException.InnerException != null) - { - ex = targetInvocationException.InnerException; - } - var modelStateKey = dtoResult.Key; - var validationState = bindingContext.ModelState.GetFieldValidationState(modelStateKey); - if (validationState == ModelValidationState.Unvalidated) - { - bindingContext.ModelState.AddModelError(modelStateKey, ex); - } + AddModelError(exception, bindingContext, dtoResult); } } else @@ -496,6 +535,82 @@ namespace Microsoft.AspNet.Mvc.ModelBinding } } + // Neither [DefaultValue] nor [Required] is relevant for a non-settable collection. + private void AddToProperty( + ModelBindingContext bindingContext, + ModelExplorer modelExplorer, + PropertyInfo property, + ModelBindingResult dtoResult) + { + var propertyExplorer = modelExplorer.GetExplorerForProperty(property.Name); + + var target = propertyExplorer.Model; + var source = dtoResult.Model; + if (!dtoResult.IsModelSet || target == null || source == null) + { + // Cannot copy to or from a null collection. + return; + } + + // Determine T if this is an ICollection property. No need for a T[] case because CanUpdateProperty() + // ensures property is either settable or not an array. Underlying assumption is that CanUpdateProperty() + // and SetProperty() are overridden together. + var collectionTypeArguments = propertyExplorer.ModelType + .ExtractGenericInterface(typeof(ICollection<>)) + ?.GenericTypeArguments; + if (collectionTypeArguments == null) + { + // Not a collection model. + return; + } + + var propertyAddRange = CallPropertyAddRangeOpenGenericMethod.MakeGenericMethod(collectionTypeArguments); + try + { + propertyAddRange.Invoke(obj: null, parameters: new[] { target, source }); + } + catch (Exception exception) + { + AddModelError(exception, bindingContext, dtoResult); + } + } + + // Called via reflection. + private static void CallPropertyAddRange(object target, object source) + { + var targetCollection = (ICollection)target; + var sourceCollection = source as IEnumerable; + if (sourceCollection != null && !targetCollection.IsReadOnly) + { + targetCollection.Clear(); + foreach (var item in sourceCollection) + { + targetCollection.Add(item); + } + } + } + + private static void AddModelError( + Exception exception, + ModelBindingContext bindingContext, + ModelBindingResult dtoResult) + { + var targetInvocationException = exception as TargetInvocationException; + if (targetInvocationException != null && targetInvocationException.InnerException != null) + { + exception = targetInvocationException.InnerException; + } + + // Do not add an error message if a binding error has already occurred for this property. + var modelState = bindingContext.ModelState; + var modelStateKey = dtoResult.Key; + var validationState = modelState.GetFieldValidationState(modelStateKey); + if (validationState == ModelValidationState.Unvalidated) + { + modelState.AddModelError(modelStateKey, exception); + } + } + // Returns true if validator execution adds a model error. private static bool RunValidator( IModelValidator validator, diff --git a/test/Microsoft.AspNet.Mvc.Core.Test/ParameterBinding/ControllerActionArgumentBinderTests.cs b/test/Microsoft.AspNet.Mvc.Core.Test/ParameterBinding/ControllerActionArgumentBinderTests.cs index 9abc66e547..3032acf3a8 100644 --- a/test/Microsoft.AspNet.Mvc.Core.Test/ParameterBinding/ControllerActionArgumentBinderTests.cs +++ b/test/Microsoft.AspNet.Mvc.Core.Test/ParameterBinding/ControllerActionArgumentBinderTests.cs @@ -3,12 +3,10 @@ using System; using System.Collections.Generic; -using System.Linq; using System.Reflection; using System.Threading.Tasks; using Microsoft.AspNet.Http; using Microsoft.AspNet.Mvc.ModelBinding; -using Microsoft.AspNet.Mvc.ModelBinding.Metadata; using Microsoft.AspNet.Mvc.ModelBinding.Validation; using Microsoft.AspNet.Routing; using Moq; @@ -18,39 +16,6 @@ namespace Microsoft.AspNet.Mvc.Core.Test { public class ControllerActionArgumentBinderTests { - public class MySimpleModel - { - } - - [Bind(Prefix = "TypePrefix")] - public class MySimpleModelWithTypeBasedBind - { - } - - public void ParameterWithNoBindAttribute(MySimpleModelWithTypeBasedBind parameter) - { - } - - public void ParameterHasFieldPrefix([Bind(Prefix = "simpleModelPrefix")] string parameter) - { - } - - public void ParameterHasEmptyFieldPrefix([Bind(Prefix = "")] MySimpleModel parameter, - [Bind(Prefix = "")] MySimpleModelWithTypeBasedBind parameter1) - { - } - - public void ParameterHasPrefixAndComplexType( - [Bind(Prefix = "simpleModelPrefix")] MySimpleModel parameter, - [Bind(Prefix = "simpleModelPrefix")] MySimpleModelWithTypeBasedBind parameter1) - { - } - - public void ParameterHasEmptyBindAttribute([Bind] MySimpleModel parameter, - [Bind] MySimpleModelWithTypeBasedBind parameter1) - { - } - [Fact] public async Task BindActionArgumentsAsync_DoesNotAddActionArguments_IfBinderReturnsFalse() { @@ -76,7 +41,6 @@ namespace Microsoft.AspNet.Mvc.Core.Test }; var modelMetadataProvider = TestModelMetadataProvider.CreateDefaultProvider(); - var argumentBinder = GetArgumentBinder(); // Act @@ -253,7 +217,7 @@ namespace Microsoft.AspNet.Mvc.Core.Test actionDescriptor.BoundProperties.Add( new ParameterDescriptor { - Name = "ValueBinderMarkedProperty", + Name = nameof(TestController.StringProperty), ParameterType = typeof(string), }); @@ -284,7 +248,7 @@ namespace Microsoft.AspNet.Mvc.Core.Test actionDescriptor.BoundProperties.Add( new ParameterDescriptor { - Name = "ValueBinderMarkedProperty", + Name = nameof(TestController.StringProperty), ParameterType = typeof(string), }); @@ -325,8 +289,8 @@ namespace Microsoft.AspNet.Mvc.Core.Test actionDescriptor.BoundProperties.Add( new ParameterDescriptor { - Name = "ValueBinderMarkedProperty", - BindingInfo = new BindingInfo(), + Name = nameof(TestController.StringProperty), + BindingInfo = new BindingInfo(), ParameterType = typeof(string) }); @@ -339,8 +303,37 @@ namespace Microsoft.AspNet.Mvc.Core.Test var result = await argumentBinder.BindActionArgumentsAsync(actionContext, actionBindingContext, controller); // Assert - Assert.Equal("Hello", controller.ValueBinderMarkedProperty); - Assert.Null(controller.UnmarkedProperty); + Assert.Equal("Hello", controller.StringProperty); + Assert.Equal(new List { "goodbye" }, controller.CollectionProperty); + Assert.Null(controller.UntouchedProperty); + } + + [Fact] + public async Task BindActionArgumentsAsync_AddsToCollectionControllerProperties() + { + // Arrange + var actionDescriptor = GetActionDescriptor(); + actionDescriptor.BoundProperties.Add( + new ParameterDescriptor + { + Name = nameof(TestController.CollectionProperty), + BindingInfo = new BindingInfo(), + ParameterType = typeof(ICollection), + }); + + var expected = new List { "Hello", "World", "!!" }; + var actionContext = GetActionContext(actionDescriptor); + var actionBindingContext = GetActionBindingContext(model: expected); + var argumentBinder = GetArgumentBinder(); + var controller = new TestController(); + + // Act + var result = await argumentBinder.BindActionArgumentsAsync(actionContext, actionBindingContext, controller); + + // Assert + Assert.Equal(expected, controller.CollectionProperty); + Assert.Null(controller.StringProperty); + Assert.Null(controller.UntouchedProperty); } [Fact] @@ -351,7 +344,7 @@ namespace Microsoft.AspNet.Mvc.Core.Test actionDescriptor.BoundProperties.Add( new ParameterDescriptor { - Name = "NotNullableProperty", + Name = nameof(TestController.NonNullableProperty), BindingInfo = new BindingInfo() { BindingSource = BindingSource.Custom }, ParameterType = typeof(int) }); @@ -372,13 +365,13 @@ namespace Microsoft.AspNet.Mvc.Core.Test var controller = new TestController(); // Some non default value. - controller.NotNullableProperty = -1; + controller.NonNullableProperty = -1; // Act var result = await argumentBinder.BindActionArgumentsAsync(actionContext, actionBindingContext, controller); // Assert - Assert.Equal(-1, controller.NotNullableProperty); + Assert.Equal(-1, controller.NonNullableProperty); } [Fact] @@ -419,6 +412,148 @@ namespace Microsoft.AspNet.Mvc.Core.Test Assert.Null(controller.NullableProperty); } + // property name, property type, property accessor, input value, expected value + public static TheoryData, object, object> SkippedPropertyData + { + get + { + return new TheoryData, object, object> + { + { + nameof(TestController.ArrayProperty), + typeof(string[]), + controller => ((TestController)controller).ArrayProperty, + new string[] { "hello", "world" }, + new string[] { "goodbye" } + }, + { + nameof(TestController.CollectionProperty), + typeof(ICollection), + controller => ((TestController)controller).CollectionProperty, + null, + new List { "goodbye" } + }, + { + nameof(TestController.NonCollectionProperty), + typeof(Person), + controller => ((TestController)controller).NonCollectionProperty, + new Person { Name = "Fred" }, + new Person { Name = "Ginger" } + }, + { + nameof(TestController.NullCollectionProperty), + typeof(ICollection), + controller => ((TestController)controller).NullCollectionProperty, + new List { "hello", "world" }, + null + }, + }; + } + } + + [Theory] + [MemberData(nameof(SkippedPropertyData))] + public async Task BindActionArgumentsAsync_SkipsReadOnlyControllerProperties( + string propertyName, + Type propertyType, + Func propertyAccessor, + object inputValue, + object expectedValue) + { + // Arrange + var actionDescriptor = GetActionDescriptor(); + actionDescriptor.BoundProperties.Add( + new ParameterDescriptor + { + Name = propertyName, + BindingInfo = new BindingInfo(), + ParameterType = propertyType, + }); + + var actionContext = GetActionContext(actionDescriptor); + var actionBindingContext = GetActionBindingContext(model: inputValue); + var argumentBinder = GetArgumentBinder(); + var controller = new TestController(); + + // Act + var result = await argumentBinder.BindActionArgumentsAsync(actionContext, actionBindingContext, controller); + + // Assert + Assert.Equal(expectedValue, propertyAccessor(controller)); + Assert.Null(controller.StringProperty); + Assert.Null(controller.UntouchedProperty); + } + + [Fact] + public async Task BindActionArgumentsAsync_SetsMultipleControllerProperties() + { + // Arrange + var boundPropertyTypes = new Dictionary + { + { nameof(TestController.ArrayProperty), typeof(string[]) }, // Skipped + { nameof(TestController.CollectionProperty), typeof(List) }, + { nameof(TestController.NonCollectionProperty), typeof(Person) }, // Skipped + { nameof(TestController.NullCollectionProperty), typeof(List) }, // Skipped + { nameof(TestController.StringProperty), typeof(string) }, + }; + var inputPropertyValues = new Dictionary + { + { nameof(TestController.ArrayProperty), new string[] { "hello", "world" } }, + { nameof(TestController.CollectionProperty), new List { "hello", "world" } }, + { nameof(TestController.NonCollectionProperty), new Person { Name = "Fred" } }, + { nameof(TestController.NullCollectionProperty), new List { "hello", "world" } }, + { nameof(TestController.StringProperty), "Hello" }, + }; + var expectedPropertyValues = new Dictionary + { + { nameof(TestController.ArrayProperty), new string[] { "goodbye" } }, + { nameof(TestController.CollectionProperty), new List { "hello", "world" } }, + { nameof(TestController.NonCollectionProperty), new Person { Name = "Ginger" } }, + { nameof(TestController.NullCollectionProperty), null }, + { nameof(TestController.StringProperty), "Hello" }, + }; + + var actionDescriptor = GetActionDescriptor(); + foreach (var keyValuePair in boundPropertyTypes) + { + actionDescriptor.BoundProperties.Add( + new ParameterDescriptor + { + Name = keyValuePair.Key, + BindingInfo = new BindingInfo(), + ParameterType = keyValuePair.Value, + }); + } + + var actionContext = GetActionContext(actionDescriptor); + var argumentBinder = GetArgumentBinder(); + var controller = new TestController(); + var binder = new Mock(); + binder + .Setup(b => b.BindModelAsync(It.IsAny())) + .Returns(bindingContext => + { + object model; + var isModelSet = inputPropertyValues.TryGetValue(bindingContext.ModelName, out model); + return Task.FromResult(new ModelBindingResult(model, bindingContext.ModelName, isModelSet)); + }); + var actionBindingContext = new ActionBindingContext + { + ModelBinder = binder.Object, + }; + + // Act + var result = await argumentBinder.BindActionArgumentsAsync(actionContext, actionBindingContext, controller); + + // Assert + Assert.Equal(new string[] { "goodbye" }, controller.ArrayProperty); // Skipped + Assert.Equal(new List { "hello", "world" }, controller.CollectionProperty); + Assert.Equal(new Person { Name = "Ginger" }, controller.NonCollectionProperty); // Skipped + Assert.Null(controller.NullCollectionProperty); // Skipped + Assert.Null(controller.UntouchedProperty); // Not bound + Assert.Equal("Hello", controller.StringProperty); + } + private static ActionContext GetActionContext(ActionDescriptor descriptor = null) { return new ActionContext( @@ -440,12 +575,17 @@ namespace Microsoft.AspNet.Mvc.Core.Test } private static ActionBindingContext GetActionBindingContext() + { + return GetActionBindingContext("Hello"); + } + + private static ActionBindingContext GetActionBindingContext(object model) { var binder = new Mock(); binder .Setup(b => b.BindModelAsync(It.IsAny())) .Returns(Task.FromResult( - result: new ModelBindingResult(model: "Hello", key: string.Empty, isModelSet: true))); + result: new ModelBindingResult(model: model, key: string.Empty, isModelSet: true))); return new ActionBindingContext() { ModelBinder = binder.Object, @@ -466,41 +606,40 @@ namespace Microsoft.AspNet.Mvc.Core.Test validator); } + // No need for bind-related attributes on properties in this controller class. Properties are added directly + // to the BoundProperties collection, bypassing usual requirements. private class TestController { - public string UnmarkedProperty { get; set; } + public string UntouchedProperty { get; set; } - [NonValueProviderBinderMetadata] - public string NonValueBinderMarkedProperty { get; set; } + public string[] ArrayProperty { get; } = new string[] { "goodbye" }; - [ValueProviderMetadata] - public string ValueBinderMarkedProperty { get; set; } + public ICollection CollectionProperty { get; } = new List { "goodbye" }; - [CustomBindingSource] - public int NotNullableProperty { get; set; } + public Person NonCollectionProperty { get; } = new Person { Name = "Ginger" }; + + public ICollection NullCollectionProperty { get; private set; } + + public string StringProperty { get; set; } + + public int NonNullableProperty { get; set; } - [CustomBindingSource] public int? NullableProperty { get; set; } - - public Person ActionWithBodyParam([FromBody] Person bodyParam) - { - return bodyParam; - } - - public Person ActionWithTwoBodyParam([FromBody] Person bodyParam, [FromBody] Person bodyParam1) - { - return bodyParam; - } } - private class Person + private class Person : IEquatable, IEquatable { public string Name { get; set; } - } - private class NonValueProviderBinderMetadataAttribute : Attribute, IBindingSourceMetadata - { - public BindingSource BindingSource { get { return BindingSource.Body; } } + public bool Equals(Person other) + { + return other != null && string.Equals(Name, other.Name, StringComparison.Ordinal); + } + + bool IEquatable.Equals(object obj) + { + return Equals(obj as Person); + } } private class CustomBindingSourceAttribute : Attribute, IBindingSourceMetadata @@ -512,17 +651,5 @@ namespace Microsoft.AspNet.Mvc.Core.Test { public BindingSource BindingSource { get { return BindingSource.Query; } } } - - [Bind(new string[] { nameof(IncludedExplicitly1), nameof(IncludedExplicitly2) })] - private class TypeWithIncludedPropertiesUsingBindAttribute - { - public int ExcludedByDefault1 { get; set; } - - public int ExcludedByDefault2 { get; set; } - - public int IncludedExplicitly1 { get; set; } - - public int IncludedExplicitly2 { get; set; } - } } } diff --git a/test/Microsoft.AspNet.Mvc.ModelBinding.Test/Binders/ArrayModelBinderTest.cs b/test/Microsoft.AspNet.Mvc.ModelBinding.Test/Binders/ArrayModelBinderTest.cs index 182b4c0303..de046fc014 100644 --- a/test/Microsoft.AspNet.Mvc.ModelBinding.Test/Binders/ArrayModelBinderTest.cs +++ b/test/Microsoft.AspNet.Mvc.ModelBinding.Test/Binders/ArrayModelBinderTest.cs @@ -11,57 +11,110 @@ namespace Microsoft.AspNet.Mvc.ModelBinding.Test public class ArrayModelBinderTest { [Fact] - public async Task BindModel() + public async Task BindModelAsync_ValueProviderContainPrefix_Succeeds() { // Arrange var valueProvider = new SimpleHttpValueProvider { { "someName[0]", "42" }, - { "someName[1]", "84" } + { "someName[1]", "84" }, }; var bindingContext = GetBindingContext(valueProvider); + var modelState = bindingContext.ModelState; var binder = new ArrayModelBinder(); // Act - var retVal = await binder.BindModelAsync(bindingContext); + var result = await binder.BindModelAsync(bindingContext); // Assert - Assert.NotNull(retVal); + Assert.NotNull(result); - int[] array = retVal.Model as int[]; + var array = Assert.IsType(result.Model); Assert.Equal(new[] { 42, 84 }, array); + Assert.True(modelState.IsValid); } [Fact] - public async Task GetBinder_ValueProviderDoesNotContainPrefix_ReturnsNull() + public async Task BindModelAsync_ValueProviderDoesNotContainPrefix_ReturnsNull() { // Arrange var bindingContext = GetBindingContext(new SimpleHttpValueProvider()); var binder = new ArrayModelBinder(); // Act - var bound = await binder.BindModelAsync(bindingContext); + var result = await binder.BindModelAsync(bindingContext); // Assert - Assert.Null(bound); + Assert.Null(result); } - [Fact] - public async Task GetBinder_ModelMetadataReturnsReadOnly_ReturnsNull() + public static TheoryData ArrayModelData + { + get + { + return new TheoryData + { + new int[0], + new [] { 357 }, + new [] { 357, 357 }, + }; + } + } + + [Theory] + [InlineData(null)] + [MemberData(nameof(ArrayModelData))] + public async Task BindModelAsync_ModelMetadataReadOnly_ReturnsNull(int[] model) { // Arrange var valueProvider = new SimpleHttpValueProvider { - { "foo[0]", "42" }, + { "someName[0]", "42" }, + { "someName[1]", "84" }, }; var bindingContext = GetBindingContext(valueProvider, isReadOnly: true); + bindingContext.Model = model; var binder = new ArrayModelBinder(); // Act - var bound = await binder.BindModelAsync(bindingContext); + var result = await binder.BindModelAsync(bindingContext); // Assert - Assert.Null(bound); + Assert.Null(result); + } + + // Here "fails silently" means the call does not update the array but also does not throw or set an error. + [Theory] + [MemberData(nameof(ArrayModelData))] + public async Task BindModelAsync_ModelMetadataNotReadOnly_ModelNonNull_FailsSilently(int[] model) + { + // Arrange + var arrayLength = model.Length; + var valueProvider = new SimpleHttpValueProvider + { + { "someName[0]", "42" }, + { "someName[1]", "84" }, + }; + + var bindingContext = GetBindingContext(valueProvider, isReadOnly: false); + var modelState = bindingContext.ModelState; + bindingContext.Model = model; + var binder = new ArrayModelBinder(); + + // Act + var result = await binder.BindModelAsync(bindingContext); + + // Assert + Assert.NotNull(result); + Assert.True(result.IsModelSet); + Assert.Same(model, result.Model); + + Assert.True(modelState.IsValid); + for (var i = 0; i < arrayLength; i++) + { + // Array should be unchanged. + Assert.Equal(357, model[i]); + } } private static IModelBinder CreateIntBinder() diff --git a/test/Microsoft.AspNet.Mvc.ModelBinding.Test/Binders/CollectionModelBinderTest.cs b/test/Microsoft.AspNet.Mvc.ModelBinding.Test/Binders/CollectionModelBinderTest.cs index 49a0943989..200795fc02 100644 --- a/test/Microsoft.AspNet.Mvc.ModelBinding.Test/Binders/CollectionModelBinderTest.cs +++ b/test/Microsoft.AspNet.Mvc.ModelBinding.Test/Binders/CollectionModelBinderTest.cs @@ -56,8 +56,10 @@ namespace Microsoft.AspNet.Mvc.ModelBinding.Test Assert.Equal(new[] { 42, 100 }, boundCollection.ToArray()); } - [Fact] - public async Task BindModel_ComplexCollection() + [Theory] + [InlineData(false)] + [InlineData(true)] + public async Task BindModel_ComplexCollection_Succeeds(bool isReadOnly) { // Arrange var valueProvider = new SimpleHttpValueProvider @@ -67,33 +69,109 @@ namespace Microsoft.AspNet.Mvc.ModelBinding.Test { "someName[bar]", "100" }, { "someName[baz]", "200" } }; - var bindingContext = GetModelBindingContext(valueProvider); + var bindingContext = GetModelBindingContext(valueProvider, isReadOnly); + var modelState = bindingContext.ModelState; var binder = new CollectionModelBinder(); // Act - var retVal = await binder.BindModelAsync(bindingContext); + var result = await binder.BindModelAsync(bindingContext); // Assert - Assert.Equal(new[] { 42, 100, 200 }, ((List)retVal.Model).ToArray()); + Assert.NotNull(result); + Assert.True(result.IsModelSet); + + var list = Assert.IsAssignableFrom>(result.Model); + Assert.Equal(new[] { 42, 100, 200 }, list.ToArray()); + + Assert.True(modelState.IsValid); } - [Fact] - public async Task BindModel_SimpleCollection() + [Theory] + [InlineData(false)] + [InlineData(true)] + public async Task BindModel_ComplexCollection_BindingContextModelNonNull_Succeeds(bool isReadOnly) + { + // Arrange + var valueProvider = new SimpleHttpValueProvider + { + { "someName.index", new[] { "foo", "bar", "baz" } }, + { "someName[foo]", "42" }, + { "someName[bar]", "100" }, + { "someName[baz]", "200" } + }; + var bindingContext = GetModelBindingContext(valueProvider, isReadOnly); + var modelState = bindingContext.ModelState; + var list = new List(); + bindingContext.Model = list; + var binder = new CollectionModelBinder(); + + // Act + var result = await binder.BindModelAsync(bindingContext); + + // Assert + Assert.NotNull(result); + Assert.True(result.IsModelSet); + + Assert.Same(list, result.Model); + Assert.Equal(new[] { 42, 100, 200 }, list.ToArray()); + + Assert.True(modelState.IsValid); + } + + [Theory] + [InlineData(false)] + [InlineData(true)] + public async Task BindModel_SimpleCollection_Succeeds(bool isReadOnly) { // Arrange var valueProvider = new SimpleHttpValueProvider { { "someName", new[] { "42", "100", "200" } } }; - var bindingContext = GetModelBindingContext(valueProvider); + var bindingContext = GetModelBindingContext(valueProvider, isReadOnly); + var modelState = bindingContext.ModelState; var binder = new CollectionModelBinder(); // Act - var retVal = await binder.BindModelAsync(bindingContext); + var result = await binder.BindModelAsync(bindingContext); // Assert - Assert.NotNull(retVal); - Assert.Equal(new[] { 42, 100, 200 }, ((List)retVal.Model).ToArray()); + Assert.NotNull(result); + Assert.True(result.IsModelSet); + + var list = Assert.IsAssignableFrom>(result.Model); + Assert.Equal(new[] { 42, 100, 200 }, list.ToArray()); + + Assert.True(modelState.IsValid); + } + + [Theory] + [InlineData(false)] + [InlineData(true)] + public async Task BindModel_SimpleCollection_BindingContextModelNonNull_Succeeds(bool isReadOnly) + { + // Arrange + var valueProvider = new SimpleHttpValueProvider + { + { "someName", new[] { "42", "100", "200" } } + }; + var bindingContext = GetModelBindingContext(valueProvider, isReadOnly); + var modelState = bindingContext.ModelState; + var list = new List(); + bindingContext.Model = list; + var binder = new CollectionModelBinder(); + + // Act + var result = await binder.BindModelAsync(bindingContext); + + // Assert + Assert.NotNull(result); + Assert.True(result.IsModelSet); + + Assert.Same(list, result.Model); + Assert.Equal(new[] { 42, 100, 200 }, list.ToArray()); + + Assert.True(modelState.IsValid); } #endif @@ -156,9 +234,13 @@ namespace Microsoft.AspNet.Mvc.ModelBinding.Test Assert.Equal(new[] { 42 }, boundCollection.ToArray()); } - private static ModelBindingContext GetModelBindingContext(IValueProvider valueProvider) + private static ModelBindingContext GetModelBindingContext( + IValueProvider valueProvider, + bool isReadOnly = false) { - var metadataProvider = new EmptyModelMetadataProvider(); + var metadataProvider = new TestModelMetadataProvider(); + metadataProvider.ForType>().BindingDetails(bd => bd.IsReadOnly = isReadOnly); + var bindingContext = new ModelBindingContext { ModelMetadata = metadataProvider.GetMetadataForType(typeof(int)), @@ -170,6 +252,7 @@ namespace Microsoft.AspNet.Mvc.ModelBinding.Test MetadataProvider = metadataProvider } }; + return bindingContext; } diff --git a/test/Microsoft.AspNet.Mvc.ModelBinding.Test/Binders/DictionaryModelBinderTest.cs b/test/Microsoft.AspNet.Mvc.ModelBinding.Test/Binders/DictionaryModelBinderTest.cs index e6aee0d8c7..167b2341ce 100644 --- a/test/Microsoft.AspNet.Mvc.ModelBinding.Test/Binders/DictionaryModelBinderTest.cs +++ b/test/Microsoft.AspNet.Mvc.ModelBinding.Test/Binders/DictionaryModelBinderTest.cs @@ -11,39 +11,81 @@ namespace Microsoft.AspNet.Mvc.ModelBinding.Test { public class DictionaryModelBinderTest { - [Fact] - public async Task BindModel() + [Theory] + [InlineData(false)] + [InlineData(true)] + public async Task BindModel_Succeeds(bool isReadOnly) { // Arrange - var metadataProvider = new EmptyModelMetadataProvider(); - ModelBindingContext bindingContext = new ModelBindingContext + var bindingContext = GetModelBindingContext(isReadOnly); + var modelState = bindingContext.ModelState; + var binder = new DictionaryModelBinder(); + + // Act + var result = await binder.BindModelAsync(bindingContext); + + // Assert + Assert.NotNull(result); + Assert.True(result.IsModelSet); + var dictionary = Assert.IsAssignableFrom>(result.Model); + Assert.True(modelState.IsValid); + + Assert.NotNull(dictionary); + Assert.Equal(2, dictionary.Count); + Assert.Equal("forty-two", dictionary[42]); + Assert.Equal("eighty-four", dictionary[84]); + } + + [Theory] + [InlineData(false)] + [InlineData(true)] + public async Task BindModel_BindingContextModelNonNull_Succeeds(bool isReadOnly) + { + // Arrange + var bindingContext = GetModelBindingContext(isReadOnly); + var modelState = bindingContext.ModelState; + var dictionary = new Dictionary(); + bindingContext.Model = dictionary; + var binder = new DictionaryModelBinder(); + + // Act + var result = await binder.BindModelAsync(bindingContext); + + // Assert + Assert.NotNull(result); + Assert.True(result.IsModelSet); + Assert.Same(dictionary, result.Model); + Assert.True(modelState.IsValid); + + Assert.NotNull(dictionary); + Assert.Equal(2, dictionary.Count); + Assert.Equal("forty-two", dictionary[42]); + Assert.Equal("eighty-four", dictionary[84]); + } + + private static ModelBindingContext GetModelBindingContext(bool isReadOnly) + { + var metadataProvider = new TestModelMetadataProvider(); + metadataProvider.ForType>().BindingDetails(bd => bd.IsReadOnly = isReadOnly); + var valueProvider = new SimpleHttpValueProvider + { + { "someName[0]", new KeyValuePair(42, "forty-two") }, + { "someName[1]", new KeyValuePair(84, "eighty-four") }, + }; + + var bindingContext = new ModelBindingContext { ModelMetadata = metadataProvider.GetMetadataForType(typeof(IDictionary)), ModelName = "someName", - ValueProvider = new SimpleHttpValueProvider - { - { "someName[0]", new KeyValuePair(42, "forty-two") }, - { "someName[1]", new KeyValuePair(84, "eighty-four") } - }, + ValueProvider = valueProvider, OperationBindingContext = new OperationBindingContext { ModelBinder = CreateKvpBinder(), MetadataProvider = metadataProvider } }; - var binder = new DictionaryModelBinder(); - // Act - var retVal = await binder.BindModelAsync(bindingContext); - - // Assert - Assert.NotNull(retVal); - - var dictionary = Assert.IsAssignableFrom>(retVal.Model); - Assert.NotNull(dictionary); - Assert.Equal(2, dictionary.Count); - Assert.Equal("forty-two", dictionary[42]); - Assert.Equal("eighty-four", dictionary[84]); + return bindingContext; } private static IModelBinder CreateKvpBinder() diff --git a/test/Microsoft.AspNet.Mvc.ModelBinding.Test/Binders/MutableObjectModelBinderTest.cs b/test/Microsoft.AspNet.Mvc.ModelBinding.Test/Binders/MutableObjectModelBinderTest.cs index 1b30ee21df..a90144202f 100644 --- a/test/Microsoft.AspNet.Mvc.ModelBinding.Test/Binders/MutableObjectModelBinderTest.cs +++ b/test/Microsoft.AspNet.Mvc.ModelBinding.Test/Binders/MutableObjectModelBinderTest.cs @@ -305,7 +305,7 @@ namespace Microsoft.AspNet.Mvc.ModelBinding return null; }); - + var modelMetadata = GetMetadataForType(modelType); var bindingContext = new MutableObjectBinderContext { @@ -486,69 +486,43 @@ namespace Microsoft.AspNet.Mvc.ModelBinding testableBinder.Verify(); } - [Fact] - public void CanUpdateProperty_HasPublicSetter_ReturnsTrue() + [Theory] + [InlineData(nameof(MyModelTestingCanUpdateProperty.ReadOnlyArray), false)] + [InlineData(nameof(MyModelTestingCanUpdateProperty.ReadOnlyInt), false)] // read-only value type + [InlineData(nameof(MyModelTestingCanUpdateProperty.ReadOnlyObject), true)] + [InlineData(nameof(MyModelTestingCanUpdateProperty.ReadOnlySimple), true)] + [InlineData(nameof(MyModelTestingCanUpdateProperty.ReadOnlyString), false)] + [InlineData(nameof(MyModelTestingCanUpdateProperty.ReadWriteString), true)] + public void CanUpdateProperty_ReturnsExpectedValue(string propertyName, bool expected) { // Arrange - var propertyMetadata = GetMetadataForCanUpdateProperty("ReadWriteString"); + var propertyMetadata = GetMetadataForCanUpdateProperty(propertyName); // Act var canUpdate = MutableObjectModelBinder.CanUpdatePropertyInternal(propertyMetadata); // Assert - Assert.True(canUpdate); + Assert.Equal(expected, canUpdate); } - [Fact] - public void CanUpdateProperty_ReadOnlyArray_ReturnsFalse() + [Theory] + [InlineData(nameof(CollectionContainer.ReadOnlyArray), false)] + [InlineData(nameof(CollectionContainer.ReadOnlyDictionary), true)] + [InlineData(nameof(CollectionContainer.ReadOnlyList), true)] + [InlineData(nameof(CollectionContainer.SettableArray), true)] + [InlineData(nameof(CollectionContainer.SettableDictionary), true)] + [InlineData(nameof(CollectionContainer.SettableList), true)] + public void CanUpdateProperty_CollectionProperty_FalseOnlyForArray(string propertyName, bool expected) { // Arrange - var propertyMetadata = GetMetadataForCanUpdateProperty("ReadOnlyArray"); + var metadataProvider = TestModelMetadataProvider.CreateDefaultProvider(); + var metadata = metadataProvider.GetMetadataForProperty(typeof(CollectionContainer), propertyName); // Act - var canUpdate = MutableObjectModelBinder.CanUpdatePropertyInternal(propertyMetadata); + var canUpdate = MutableObjectModelBinder.CanUpdatePropertyInternal(metadata); // Assert - Assert.False(canUpdate); - } - - [Fact] - public void CanUpdateProperty_ReadOnlyReferenceTypeNotBlacklisted_ReturnsTrue() - { - // Arrange - var propertyMetadata = GetMetadataForCanUpdateProperty("ReadOnlyObject"); - - // Act - var canUpdate = MutableObjectModelBinder.CanUpdatePropertyInternal(propertyMetadata); - - // Assert - Assert.True(canUpdate); - } - - [Fact] - public void CanUpdateProperty_ReadOnlyString_ReturnsFalse() - { - // Arrange - var propertyMetadata = GetMetadataForCanUpdateProperty("ReadOnlyString"); - - // Act - var canUpdate = MutableObjectModelBinder.CanUpdatePropertyInternal(propertyMetadata); - - // Assert - Assert.False(canUpdate); - } - - [Fact] - public void CanUpdateProperty_ReadOnlyValueType_ReturnsFalse() - { - // Arrange - var propertyMetadata = GetMetadataForCanUpdateProperty("ReadOnlyInt"); - - // Act - var canUpdate = MutableObjectModelBinder.CanUpdatePropertyInternal(propertyMetadata); - - // Assert - Assert.False(canUpdate); + Assert.Equal(expected, canUpdate); } [Fact] @@ -1364,6 +1338,142 @@ namespace Microsoft.AspNet.Mvc.ModelBinding // If didn't throw, success! } + // Property name, property accessor + public static TheoryData> MyCanUpdateButCannotSetPropertyData + { + get + { + return new TheoryData> + { + { + nameof(MyModelTestingCanUpdateProperty.ReadOnlyObject), + model => ((Simple)((MyModelTestingCanUpdateProperty)model).ReadOnlyObject).Name + }, + { + nameof(MyModelTestingCanUpdateProperty.ReadOnlySimple), + model => ((MyModelTestingCanUpdateProperty)model).ReadOnlySimple.Name + }, + }; + } + } + + // Reviewers: Is this inconsistency with CanUpdateProperty() an issue we should be tracking? + [Theory] + [MemberData(nameof(MyCanUpdateButCannotSetPropertyData))] + public void SetProperty_ValueProvidedAndCanUpdatePropertyTrue_DoesNothing( + string propertyName, + Func propertAccessor) + { + // Arrange + var model = new MyModelTestingCanUpdateProperty(); + var type = model.GetType(); + var bindingContext = CreateContext(GetMetadataForType(type), model); + var modelState = bindingContext.ModelState; + var metadataProvider = bindingContext.OperationBindingContext.MetadataProvider; + var modelExplorer = metadataProvider.GetModelExplorerForType(type, model); + + var propertyMetadata = bindingContext.ModelMetadata.Properties[propertyName]; + var dtoResult = new ModelBindingResult( + model: new Simple { Name = "Hanna" }, + isModelSet: true, + key: propertyName); + + var testableBinder = new TestableMutableObjectModelBinder(); + + // Act + testableBinder.SetProperty( + bindingContext, + modelExplorer, + propertyMetadata, + dtoResult, + requiredValidator: null); + + // Assert + Assert.Equal("Joe", propertAccessor(model)); + Assert.True(modelState.IsValid); + Assert.Empty(modelState); + } + + // Property name, property accessor, collection. + public static TheoryData, object> CollectionPropertyData + { + get + { + return new TheoryData, object> + { + { + nameof(CollectionContainer.ReadOnlyDictionary), + model => ((CollectionContainer)model).ReadOnlyDictionary, + new Dictionary + { + { 1, "one" }, + { 2, "two" }, + { 3, "three" }, + } + }, + { + nameof(CollectionContainer.ReadOnlyList), + model => ((CollectionContainer)model).ReadOnlyList, + new List { 1, 2, 3, 4 } + }, + { + nameof(CollectionContainer.SettableArray), + model => ((CollectionContainer)model).SettableArray, + new int[] { 1, 2, 3, 4 } + }, + { + nameof(CollectionContainer.SettableDictionary), + model => ((CollectionContainer)model).SettableDictionary, + new Dictionary + { + { 1, "one" }, + { 2, "two" }, + { 3, "three" }, + } + }, + { + nameof(CollectionContainer.SettableList), + model => ((CollectionContainer)model).SettableList, + new List { 1, 2, 3, 4 } + }, + }; + } + } + + [Theory] + [MemberData(nameof(CollectionPropertyData))] + public void SetProperty_CollectionProperty_UpdatesModel( + string propertyName, + Func propertyAccessor, + object collection) + { + // Arrange + var model = new CollectionContainer(); + var type = model.GetType(); + var bindingContext = CreateContext(GetMetadataForType(type), model); + var modelState = bindingContext.ModelState; + var metadataProvider = bindingContext.OperationBindingContext.MetadataProvider; + var modelExplorer = metadataProvider.GetModelExplorerForType(type, model); + + var propertyMetadata = bindingContext.ModelMetadata.Properties[propertyName]; + var dtoResult = new ModelBindingResult(model: collection, isModelSet: true, key: propertyName); + + var testableBinder = new TestableMutableObjectModelBinder(); + + // Act + testableBinder.SetProperty( + bindingContext, + modelExplorer, + propertyMetadata, + dtoResult, + requiredValidator: null); + + // Assert + Assert.Equal(collection, propertyAccessor(model)); + Assert.True(modelState.IsValid); + Assert.Empty(modelState); + } + [Fact] public void SetProperty_PropertyIsSettable_CallsSetter() { @@ -1715,8 +1825,9 @@ namespace Microsoft.AspNet.Mvc.ModelBinding public int ReadOnlyInt { get; private set; } public string ReadOnlyString { get; private set; } public string[] ReadOnlyArray { get; private set; } - public object ReadOnlyObject { get; private set; } + public object ReadOnlyObject { get; } = new Simple { Name = "Joe" }; public string ReadWriteString { get; set; } + public Simple ReadOnlySimple { get; } = new Simple { Name = "Joe" }; } private sealed class ModelWhosePropertySetterThrows @@ -1788,7 +1899,7 @@ namespace Microsoft.AspNet.Mvc.ModelBinding public int IncludedByDefault2 { get; set; } } - public class Document + private class Document { [NonValueBinderMetadata] public string Version { get; set; } @@ -1807,7 +1918,7 @@ namespace Microsoft.AspNet.Mvc.ModelBinding public BindingSource BindingSource { get { return BindingSource.Query; } } } - public class ExcludedProvider : IPropertyBindingPredicateProvider + private class ExcludedProvider : IPropertyBindingPredicateProvider { public Func PropertyFilter { @@ -1820,16 +1931,37 @@ namespace Microsoft.AspNet.Mvc.ModelBinding } } - public class SimpleContainer + private class SimpleContainer { public Simple Simple { get; set; } } - public class Simple + private class Simple { public string Name { get; set; } } + private class CollectionContainer + { + public int[] ReadOnlyArray { get; } = new int[4]; + + // Read-only collections get added values. + public IDictionary ReadOnlyDictionary { get; } = new Dictionary(); + + public IList ReadOnlyList { get; } = new List(); + + // Settable values are overwritten. + public int[] SettableArray { get; set; } = new int[] { 0, 1 }; + + public IDictionary SettableDictionary { get; set; } = new Dictionary + { + { 0, "zero" }, + { 25, "twenty-five" }, + }; + + public IList SettableList { get; set; } = new List { 3, 9, 0 }; + } + private IServiceProvider CreateServices() { var services = new Mock(MockBehavior.Strict);