From 78102c0ed6c7172154c78a117fb000df1cf661a8 Mon Sep 17 00:00:00 2001 From: Doug Bunting Date: Sun, 19 Apr 2015 11:25:01 -0700 Subject: [PATCH] Bind to readonly non-`null` collections - parts 1/3 and 2/3 of #2294 - fill readonly gaps in `CollectionModelBinder` and `MutableObjectModelBinder` - `CollectionModelBinder.CopyToModel()` and `MutableObjectModelBinder.AddToProperty()` methods - add `CopyToModel()` override in `ArrayModelBinder` - avoid NREs in `GetModel()` overrides Test handling of readonly collections - previous tests barely touched this scenario nits: - add missing `[NotNull]` attributes - add missing doc comments - consolidate a few `[Fact]`s into `[Theory]`s - simplify some wrapping; shorten a few lines --- .../Binders/ArrayModelBinder.cs | 44 +++- .../Binders/CollectionModelBinder.cs | 98 ++++++-- .../Binders/DictionaryModelBinder.cs | 12 +- .../Binders/MutableObjectModelBinder.cs | 172 ++++++++++--- .../Binders/ArrayModelBinderTest.cs | 65 ++++- .../Binders/CollectionModelBinderTest.cs | 101 +++++++- .../Binders/DictionaryModelBinderTest.cs | 84 +++++-- .../Binders/MutableObjectModelBinderTest.cs | 238 ++++++++++++++---- 8 files changed, 666 insertions(+), 148 deletions(-) diff --git a/src/Microsoft.AspNet.Mvc.ModelBinding/Binders/ArrayModelBinder.cs b/src/Microsoft.AspNet.Mvc.ModelBinding/Binders/ArrayModelBinder.cs index 414903b27c..89269cf002 100644 --- a/src/Microsoft.AspNet.Mvc.ModelBinding/Binders/ArrayModelBinder.cs +++ b/src/Microsoft.AspNet.Mvc.ModelBinding/Binders/ArrayModelBinder.cs @@ -2,16 +2,23 @@ // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. using System.Collections.Generic; +using System.Diagnostics; 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) + if (bindingContext.Model == null && bindingContext.ModelMetadata.IsReadOnly) { return Task.FromResult(null); } @@ -19,9 +26,42 @@ public override Task BindModelAsync(ModelBindingContext bind return base.BindModelAsync(bindingContext); } + /// protected override object GetModel(IEnumerable newCollection) { + if (newCollection == null) + { + return null; + } + return newCollection.ToArray(); } + + /// + protected override void CopyToModel([NotNull] object target, IEnumerable sourceCollection) + { + TElement[] targetArray = target as TElement[]; + Debug.Assert(targetArray != null); // This binder is instantiated only for array model types. + + if (sourceCollection != null && targetArray != null) + { + int maxIndex = targetArray.Length - 1; + int index = 0; + foreach (var element in sourceCollection) + { + if (index > maxIndex) + { + break; + } + + targetArray[index++] = element; + } + } + else + { + // Do not expect base implementation will succeed but just in case... + base.CopyToModel(target, sourceCollection); + } + } } } diff --git a/src/Microsoft.AspNet.Mvc.ModelBinding/Binders/CollectionModelBinder.cs b/src/Microsoft.AspNet.Mvc.ModelBinding/Binders/CollectionModelBinder.cs index 54cf34a56b..957b6d1ded 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 @@ public virtual async Task BindModelAsync(ModelBindingContext } 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 @@ internal async Task> BindSimpleCollection(ModelBindingCont }; 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 @@ private async Task> BindComplexCollection(ModelBindingCont 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 @@ internal async Task> BindComplexCollectionFromIndexes(Mode 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,49 @@ internal async Task> BindComplexCollectionFromIndexes(Mode return boundCollection; } - // Extensibility point that allows the bound collection to be manipulated or transformed before - // being returned from the binder. + /// + /// Gets an suitable for the associated property. + /// + /// + /// Collection of values retrieved from value providers. Or null if nothing was bound. + /// + /// + /// suitable for the associated 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) + { + 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..0f1f75c756 100644 --- a/src/Microsoft.AspNet.Mvc.ModelBinding/Binders/DictionaryModelBinder.cs +++ b/src/Microsoft.AspNet.Mvc.ModelBinding/Binders/DictionaryModelBinder.cs @@ -1,16 +1,26 @@ // 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) { + if (newCollection == null) + { + return null; + } + 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..5107cd2442 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.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 @@ public virtual async Task BindModelAsync(ModelBindingContext 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 @@ private async Task CreateAndPopulateDto( 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 @@ protected virtual void EnsureModel(ModelBindingContext bindingContext) } } - 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 @@ internal void ProcessDto(ModelBindingContext bindingContext, ComplexModelDto dto } } + /// + /// 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 @@ protected virtual void SetProperty( propertyMetadata.PropertyName, bindingFlags); - if (property == null || !property.CanWrite) + if (property == null) + { + // Nothing to do if property does not exist. + return; + } + + if (!property.CanWrite) { - // nothing to do + // Try to handle as a collection if property exists but is not settable. + AddToProperty(bindingContext, modelExplorer, property, dtoResult); return; } @@ -462,26 +513,10 @@ protected virtual void SetProperty( if (value != null || property.PropertyType.AllowsNullValue()) { - try - { - propertyMetadata.PropertySetter(bindingContext.Model, value); - } - catch (Exception ex) - { - // 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); - } - } + TryAction( + bindingContext, + dtoResult, + () => propertyMetadata.PropertySetter(bindingContext.Model, value)); } else { @@ -496,6 +531,81 @@ protected virtual void SetProperty( } } + // 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 elementTypeArray = propertyExplorer.ModelType + .ExtractGenericInterface(typeof(ICollection<>)) + ?.GenericTypeArguments; + if (elementTypeArray == null) + { + // Not a collection model. + return; + } + + var propertyAddRange = CallPropertyAddRangeOpenGenericMethod.MakeGenericMethod(elementTypeArray); + TryAction( + bindingContext, + dtoResult, + () => propertyAddRange.Invoke(obj: null, parameters: new[] { target, source })); + } + + // Called via reflection. + private static void CallPropertyAddRange(object target, object source) + { + var targetCollection = (ICollection)target; + var sourceCollection = source as IEnumerable; + if (sourceCollection != null) + { + foreach (var item in sourceCollection) + { + targetCollection.Add(item); + } + } + } + + private static void TryAction(ModelBindingContext bindingContext, ModelBindingResult dtoResult, Action action) + { + try + { + action(); + } + catch (Exception exception) + { + 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 modelStateKey = dtoResult.Key; + var validationState = bindingContext.ModelState.GetFieldValidationState(modelStateKey); + if (validationState == ModelValidationState.Unvalidated) + { + bindingContext.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.ModelBinding.Test/Binders/ArrayModelBinderTest.cs b/test/Microsoft.AspNet.Mvc.ModelBinding.Test/Binders/ArrayModelBinderTest.cs index 182b4c0303..71b3a654dc 100644 --- a/test/Microsoft.AspNet.Mvc.ModelBinding.Test/Binders/ArrayModelBinderTest.cs +++ b/test/Microsoft.AspNet.Mvc.ModelBinding.Test/Binders/ArrayModelBinderTest.cs @@ -11,57 +11,98 @@ 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[]; + int[] array = result.Model as int[]; 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 async Task BindModelAsync_ModelMetadataReturnsReadOnly_ReturnsNull() { // Arrange var valueProvider = new SimpleHttpValueProvider { - { "foo[0]", "42" }, + { "someName[0]", "42" }, + { "someName[1]", "84" }, }; var bindingContext = GetBindingContext(valueProvider, isReadOnly: true); var binder = new ArrayModelBinder(); // Act - var bound = await binder.BindModelAsync(bindingContext); + var result = await binder.BindModelAsync(bindingContext); // Assert - Assert.Null(bound); + Assert.Null(result); + } + + [Theory] + [InlineData(false, 0)] + [InlineData(false, 1)] + [InlineData(false, 2)] + [InlineData(true, 0)] + [InlineData(true, 1)] + [InlineData(true, 2)] + public async Task BindModelAsync_BindingContextModelNonNull_Succeeds(bool isReadOnly, int arrayLength) + { + // Arrange + var valueProvider = new SimpleHttpValueProvider + { + { "someName[0]", "42" }, + { "someName[1]", "84" }, + }; + var expected = new[] { 42, 84 }; + + var bindingContext = GetBindingContext(valueProvider, isReadOnly); + var modelState = bindingContext.ModelState; + var array = new int[arrayLength]; + bindingContext.Model = array; + var binder = new ArrayModelBinder(); + + // Act + var result = await binder.BindModelAsync(bindingContext); + + // Assert + Assert.NotNull(result); + Assert.True(result.IsModelSet); + Assert.Same(array, result.Model); + + Assert.True(modelState.IsValid); + for (int i = 0; i < arrayLength; i++) + { + Assert.Equal(expected[i], array[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..31fd76693e 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 @@ public async Task BindComplexCollectionFromIndexes_InfiniteIndexes() 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,101 @@ public async Task BindModel_ComplexCollection() { "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 +226,13 @@ public async Task BindSimpleCollection_SubBindingSucceeds() 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 +244,7 @@ private static ModelBindingContext GetModelBindingContext(IValueProvider valuePr 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,41 +11,83 @@ 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 - { - ModelMetadata = metadataProvider.GetMetadataForType(typeof(IDictionary)), - ModelName = "someName", - ValueProvider = new SimpleHttpValueProvider - { - { "someName[0]", new KeyValuePair(42, "forty-two") }, - { "someName[1]", new KeyValuePair(84, "eighty-four") } - }, - OperationBindingContext = new OperationBindingContext - { - ModelBinder = CreateKvpBinder(), - MetadataProvider = metadataProvider - } - }; + 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 retVal = await binder.BindModelAsync(bindingContext); + var result = await binder.BindModelAsync(bindingContext); // Assert - Assert.NotNull(retVal); + Assert.NotNull(result); + Assert.True(result.IsModelSet); + Assert.Same(dictionary, result.Model); + Assert.True(modelState.IsValid); - 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]); } + 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 = valueProvider, + OperationBindingContext = new OperationBindingContext + { + ModelBinder = CreateKvpBinder(), + MetadataProvider = metadataProvider + } + }; + + return bindingContext; + } + private static IModelBinder CreateKvpBinder() { Mock mockKvpBinder = new Mock(); diff --git a/test/Microsoft.AspNet.Mvc.ModelBinding.Test/Binders/MutableObjectModelBinderTest.cs b/test/Microsoft.AspNet.Mvc.ModelBinding.Test/Binders/MutableObjectModelBinderTest.cs index 1b30ee21df..6515f53715 100644 --- a/test/Microsoft.AspNet.Mvc.ModelBinding.Test/Binders/MutableObjectModelBinderTest.cs +++ b/test/Microsoft.AspNet.Mvc.ModelBinding.Test/Binders/MutableObjectModelBinderTest.cs @@ -486,69 +486,43 @@ public async Task BindModel_InitsInstance_ForEmptyModelName() testableBinder.Verify(); } - [Fact] - public void CanUpdateProperty_HasPublicSetter_ReturnsTrue() - { - // Arrange - var propertyMetadata = GetMetadataForCanUpdateProperty("ReadWriteString"); - - // Act - var canUpdate = MutableObjectModelBinder.CanUpdatePropertyInternal(propertyMetadata); - - // Assert - Assert.True(canUpdate); - } - - [Fact] - public void CanUpdateProperty_ReadOnlyArray_ReturnsFalse() - { - // Arrange - var propertyMetadata = GetMetadataForCanUpdateProperty("ReadOnlyArray"); - - // Act - var canUpdate = MutableObjectModelBinder.CanUpdatePropertyInternal(propertyMetadata); - - // 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() + [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("ReadOnlyString"); + var propertyMetadata = GetMetadataForCanUpdateProperty(propertyName); // Act var canUpdate = MutableObjectModelBinder.CanUpdatePropertyInternal(propertyMetadata); // Assert - Assert.False(canUpdate); + Assert.Equal(expected, canUpdate); } - [Fact] - public void CanUpdateProperty_ReadOnlyValueType_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("ReadOnlyInt"); + 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); + Assert.Equal(expected, canUpdate); } [Fact] @@ -1364,6 +1338,142 @@ public void SetProperty_PropertyIsReadOnly_DoesNothing() // 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 @@ private sealed class MyModelTestingCanUpdateProperty 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 @@ private class TypeWithExcludedPropertiesUsingBindAttribute public int IncludedByDefault2 { get; set; } } - public class Document + private class Document { [NonValueBinderMetadata] public string Version { get; set; } @@ -1807,7 +1918,7 @@ private class ValueBinderMetadataAttribute : Attribute, IBindingSourceMetadata public BindingSource BindingSource { get { return BindingSource.Query; } } } - public class ExcludedProvider : IPropertyBindingPredicateProvider + private class ExcludedProvider : IPropertyBindingPredicateProvider { public Func PropertyFilter { @@ -1820,16 +1931,37 @@ public Func PropertyFilter } } - 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);