From d45e2ee3f53f50218dc875b92178dcf3e742e706 Mon Sep 17 00:00:00 2001 From: Doug Bunting Date: Mon, 10 Aug 2015 09:50:01 -0700 Subject: [PATCH] Handle broader range of collection types in model binding - #2793 - add `ICollectionModelBinder`, allowing `GenericModelBinder` to call `CreateEmptyCollection()` - adjust `CollectionModelBinder` and `DictionaryModelBinder` to activate model if default types are incompatible - do not create default (empty) top-level collection in fallback case if Model already non-`null` - change type checks in `GenericModelBinder` to align with `CollectionModelBinder` capabilities - add special case for `IEnumerable` - correct `ModelMetadata` of a few tests that previously did not need that information --- .../ModelBinding/ArrayModelBinder.cs | 20 ++- .../ModelBinding/CollectionModelBinder.cs | 99 +++++++++++---- .../ModelBinding/DictionaryModelBinder.cs | 36 +++++- .../ModelBinding/GenericModelBinder.cs | 109 ++++++++++------ .../ModelBinding/ICollectionModelBinder.cs | 27 ++++ .../ModelBinding/CollectionModelBinderTest.cs | 100 ++++++++++++++- .../ModelBinding/DictionaryModelBinderTest.cs | 116 ++++++++++++++++-- .../ModelBindingFromQueryTest.cs | 5 +- .../ActionParametersIntegrationTest.cs | 35 ++++-- .../TryUpdateModelIntegrationTest.cs | 37 ++++-- 10 files changed, 475 insertions(+), 109 deletions(-) create mode 100644 src/Microsoft.AspNet.Mvc.Core/ModelBinding/ICollectionModelBinder.cs diff --git a/src/Microsoft.AspNet.Mvc.Core/ModelBinding/ArrayModelBinder.cs b/src/Microsoft.AspNet.Mvc.Core/ModelBinding/ArrayModelBinder.cs index f7df7ae430..4843261a2a 100644 --- a/src/Microsoft.AspNet.Mvc.Core/ModelBinding/ArrayModelBinder.cs +++ b/src/Microsoft.AspNet.Mvc.Core/ModelBinding/ArrayModelBinder.cs @@ -1,7 +1,9 @@ // Copyright (c) .NET Foundation. 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.Diagnostics; using System.Linq; using System.Threading.Tasks; using Microsoft.Framework.Internal; @@ -25,15 +27,27 @@ namespace Microsoft.AspNet.Mvc.ModelBinding return base.BindModelAsync(bindingContext); } - protected override object CreateEmptyCollection() + /// + public override bool CanCreateInstance(Type targetType) { + return targetType == typeof(TElement[]); + } + + /// + protected override object CreateEmptyCollection(Type targetType) + { + Debug.Assert(targetType == typeof(TElement[]), "GenericModelBinder only creates this binder for arrays."); + return new TElement[0]; } /// - protected override object GetModel(IEnumerable newCollection) + protected override object ConvertToCollectionType(Type targetType, IEnumerable collection) { - return newCollection?.ToArray(); + Debug.Assert(targetType == typeof(TElement[]), "GenericModelBinder only creates this binder for arrays."); + + // If non-null, collection is a List, never already a TElement[]. + return collection?.ToArray(); } /// diff --git a/src/Microsoft.AspNet.Mvc.Core/ModelBinding/CollectionModelBinder.cs b/src/Microsoft.AspNet.Mvc.Core/ModelBinding/CollectionModelBinder.cs index f207b25c4d..bb7993345f 100644 --- a/src/Microsoft.AspNet.Mvc.Core/ModelBinding/CollectionModelBinder.cs +++ b/src/Microsoft.AspNet.Mvc.Core/ModelBinding/CollectionModelBinder.cs @@ -7,6 +7,9 @@ using System.Collections.Generic; using System.Diagnostics; using System.Globalization; using System.Linq; +#if DNXCORE50 +using System.Reflection; +#endif using System.Threading.Tasks; using Microsoft.Framework.Internal; @@ -16,22 +19,24 @@ namespace Microsoft.AspNet.Mvc.ModelBinding /// implementation for binding collection values. /// /// Type of elements in the collection. - public class CollectionModelBinder : IModelBinder + public class CollectionModelBinder : ICollectionModelBinder { /// public virtual async Task BindModelAsync([NotNull] ModelBindingContext bindingContext) { ModelBindingHelper.ValidateBindingContext(bindingContext); - object model; - + var model = bindingContext.Model; if (!await bindingContext.ValueProvider.ContainsPrefixAsync(bindingContext.ModelName)) { // If this is the fallback case and we failed to find data for a top-level model, then generate a - // default 'empty' model and return it. + // default 'empty' model (or use existing Model) and return it. if (!bindingContext.IsFirstChanceBinding && bindingContext.IsTopLevelObject) { - model = CreateEmptyCollection(); + if (model == null) + { + model = CreateEmptyCollection(bindingContext.ModelType); + } var validationNode = new ModelValidationNode( bindingContext.ModelName, @@ -50,12 +55,10 @@ namespace Microsoft.AspNet.Mvc.ModelBinding var valueProviderResult = await bindingContext.ValueProvider.GetValueAsync(bindingContext.ModelName); - IEnumerable boundCollection; CollectionResult result; if (valueProviderResult == null) { result = await BindComplexCollection(bindingContext); - boundCollection = result.Model; } else { @@ -63,13 +66,12 @@ namespace Microsoft.AspNet.Mvc.ModelBinding bindingContext, valueProviderResult.RawValue, valueProviderResult.Culture); - boundCollection = result.Model; } - model = bindingContext.Model; + var boundCollection = result.Model; if (model == null) { - model = GetModel(boundCollection); + model = ConvertToCollectionType(bindingContext.ModelType, boundCollection); } else { @@ -84,14 +86,50 @@ namespace Microsoft.AspNet.Mvc.ModelBinding validationNode: result?.ValidationNode); } - // Called when we're creating a default 'empty' model for a top level bind. - protected virtual object CreateEmptyCollection() + /// + public virtual bool CanCreateInstance(Type targetType) { - return new List(); + return CreateEmptyCollection(targetType) != null; + } + + /// + /// Create an assignable to . + /// + /// of the model. + /// An assignable to . + /// Called when creating a default 'empty' model for a top level bind. + protected virtual object CreateEmptyCollection(Type targetType) + { + if (targetType.IsAssignableFrom(typeof(List))) + { + // Simple case such as ICollection, IEnumerable and IList. + return new List(); + } + + return CreateInstance(targetType); + } + + /// + /// Create an instance of . + /// + /// of the model. + /// An instance of . + protected object CreateInstance(Type targetType) + { + try + { + return Activator.CreateInstance(targetType); + } + catch (Exception) + { + // Details of exception are not important. + return null; + } } // 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 for testing. internal async Task BindSimpleCollection( ModelBindingContext bindingContext, object rawValue, @@ -156,6 +194,7 @@ namespace Microsoft.AspNet.Mvc.ModelBinding return await BindComplexCollectionFromIndexes(bindingContext, indexNames); } + // Internal for testing. internal async Task BindComplexCollectionFromIndexes( ModelBindingContext bindingContext, IEnumerable indexNames) @@ -219,6 +258,7 @@ namespace Microsoft.AspNet.Mvc.ModelBinding }; } + // Internal for testing. internal class CollectionResult { public ModelValidationNode ValidationNode { get; set; } @@ -227,23 +267,37 @@ namespace Microsoft.AspNet.Mvc.ModelBinding } /// - /// Gets an assignable to the collection property. + /// Gets an assignable to that contains members from + /// . /// - /// + /// of the model. + /// /// Collection of values retrieved from value providers. Or null if nothing was bound. /// /// - /// assignable to the collection property. Or null if nothing was bound. + /// An assignable to . 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) + protected virtual object ConvertToCollectionType(Type targetType, IEnumerable collection) { - // 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. + if (collection == null) + { + return null; + } + + if (targetType.IsAssignableFrom(typeof(List))) + { + // Depends on fact BindSimpleCollection() and BindComplexCollection() always return a List + // instance or null. + return collection; + } + + var newCollection = CreateInstance(targetType); + CopyToModel(newCollection, collection); + return newCollection; } @@ -254,11 +308,10 @@ namespace Microsoft.AspNet.Mvc.ModelBinding /// /// 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. + Debug.Assert(targetCollection != null, "This binder is instantiated only for ICollection model types."); if (sourceCollection != null && targetCollection != null && !targetCollection.IsReadOnly) { @@ -270,7 +323,7 @@ namespace Microsoft.AspNet.Mvc.ModelBinding } } - internal static object[] RawValueToObjectArray(object rawValue) + private static object[] RawValueToObjectArray(object rawValue) { // precondition: rawValue is not null diff --git a/src/Microsoft.AspNet.Mvc.Core/ModelBinding/DictionaryModelBinder.cs b/src/Microsoft.AspNet.Mvc.Core/ModelBinding/DictionaryModelBinder.cs index 26c3dab010..3990f3255f 100644 --- a/src/Microsoft.AspNet.Mvc.Core/ModelBinding/DictionaryModelBinder.cs +++ b/src/Microsoft.AspNet.Mvc.Core/ModelBinding/DictionaryModelBinder.cs @@ -1,9 +1,13 @@ // Copyright (c) .NET Foundation. 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.Diagnostics; using System.Linq; +#if DNXCORE50 +using System.Reflection; +#endif using System.Threading.Tasks; using Microsoft.Framework.Internal; @@ -27,7 +31,7 @@ namespace Microsoft.AspNet.Mvc.ModelBinding } Debug.Assert(result.Model != null); - var model = (Dictionary)result.Model; + var model = (IDictionary)result.Model; if (model.Count != 0) { // ICollection> approach was successful. @@ -80,15 +84,37 @@ namespace Microsoft.AspNet.Mvc.ModelBinding } /// - protected override object GetModel(IEnumerable> newCollection) + protected override object ConvertToCollectionType( + Type targetType, + IEnumerable> collection) { - return newCollection?.ToDictionary(kvp => kvp.Key, kvp => kvp.Value); + if (collection == null) + { + return null; + } + + if (targetType.IsAssignableFrom(typeof(Dictionary))) + { + // Collection is a List>, never already a Dictionary. + return collection.ToDictionary(kvp => kvp.Key, kvp => kvp.Value); + } + + var newCollection = CreateInstance(targetType); + CopyToModel(newCollection, collection); + + return newCollection; } /// - protected override object CreateEmptyCollection() + protected override object CreateEmptyCollection(Type targetType) { - return new Dictionary(); + if (targetType.IsAssignableFrom(typeof(Dictionary))) + { + // Simple case such as IDictionary. + return new Dictionary(); + } + + return CreateInstance(targetType); } private static TKey ConvertFromString(string keyString) diff --git a/src/Microsoft.AspNet.Mvc.Core/ModelBinding/GenericModelBinder.cs b/src/Microsoft.AspNet.Mvc.Core/ModelBinding/GenericModelBinder.cs index 83820ea56f..f325a5f558 100644 --- a/src/Microsoft.AspNet.Mvc.Core/ModelBinding/GenericModelBinder.cs +++ b/src/Microsoft.AspNet.Mvc.Core/ModelBinding/GenericModelBinder.cs @@ -13,12 +13,21 @@ namespace Microsoft.AspNet.Mvc.ModelBinding { public async Task BindModelAsync(ModelBindingContext bindingContext) { - var binderType = ResolveBinderType(bindingContext.ModelType); + var binderType = ResolveBinderType(bindingContext); if (binderType != null) { var binder = (IModelBinder)Activator.CreateInstance(binderType); - var result = await binder.BindModelAsync(bindingContext); + var collectionBinder = binder as ICollectionModelBinder; + if (collectionBinder != null && + bindingContext.Model == null && + !collectionBinder.CanCreateInstance(bindingContext.ModelType)) + { + // Able to resolve a binder type but need a new model instance and that binder cannot create it. + return null; + } + + var result = await binder.BindModelAsync(bindingContext); var modelBindingResult = result != null ? result : new ModelBindingResult(model: null, key: bindingContext.ModelName, isModelSet: false); @@ -31,12 +40,15 @@ namespace Microsoft.AspNet.Mvc.ModelBinding return null; } - private static Type ResolveBinderType(Type modelType) + private static Type ResolveBinderType(ModelBindingContext context) { + var modelType = context.ModelType; + return GetArrayBinder(modelType) ?? - GetCollectionBinder(modelType) ?? - GetDictionaryBinder(modelType) ?? - GetKeyValuePairBinder(modelType); + GetCollectionBinder(modelType) ?? + GetDictionaryBinder(modelType) ?? + GetEnumerableBinder(context) ?? + GetKeyValuePairBinder(modelType); } private static Type GetArrayBinder(Type modelType) @@ -52,19 +64,50 @@ namespace Microsoft.AspNet.Mvc.ModelBinding private static Type GetCollectionBinder(Type modelType) { return GetGenericBinderType( - typeof(ICollection<>), - typeof(List<>), - typeof(CollectionModelBinder<>), - modelType); + typeof(ICollection<>), + typeof(CollectionModelBinder<>), + modelType); } private static Type GetDictionaryBinder(Type modelType) { return GetGenericBinderType( - typeof(IDictionary<,>), - typeof(Dictionary<,>), - typeof(DictionaryModelBinder<,>), - modelType); + typeof(IDictionary<,>), + typeof(DictionaryModelBinder<,>), + modelType); + } + + private static Type GetEnumerableBinder(ModelBindingContext context) + { + var modelTypeArguments = GetGenericBinderTypeArgs(typeof(IEnumerable<>), context.ModelType); + if (modelTypeArguments == null) + { + return null; + } + + if (context.Model == null) + { + // GetCollectionBinder has already confirmed modelType is not compatible with ICollection. Can a + // List (the default CollectionModelBinder type) instance be used instead of that exact type? + // Likely this will succeed only if the property type is exactly IEnumerable. + var closedListType = typeof(List<>).MakeGenericType(modelTypeArguments); + if (!context.ModelType.IsAssignableFrom(closedListType)) + { + return null; + } + } + else + { + // A non-null instance must be updated in-place. For that the instance must also implement + // ICollection. For example an IEnumerable property may have a List default value. + var closedCollectionType = typeof(ICollection<>).MakeGenericType(modelTypeArguments); + if (!closedCollectionType.IsAssignableFrom(context.Model.GetType())) + { + return null; + } + } + + return typeof(CollectionModelBinder<>).MakeGenericType(modelTypeArguments); } private static Type GetKeyValuePairBinder(Type modelType) @@ -79,52 +122,46 @@ namespace Microsoft.AspNet.Mvc.ModelBinding return null; } - /// - /// Example: GetGenericBinderType(typeof(IList<T>), typeof(List<T>), - /// typeof(ListBinder<T>), ...) means that the ListBinder<T> type can update models that - /// implement , and if for some reason the existing model instance is not updatable the - /// binder will create a object and bind to that instead. This method will return - /// ListBinder<T> or null, depending on whether the type and updatability checks succeed. - /// - private static Type GetGenericBinderType(Type supportedInterfaceType, - Type newInstanceType, - Type openBinderType, - Type modelType) + // Example: GetGenericBinderType(typeof(IList), typeof(ListBinder), ...) means that the ListBinder + // type can update models that implement IList. This method will return + // ListBinder or null, depending on whether the type checks succeed. + private static Type GetGenericBinderType(Type supportedInterfaceType, Type openBinderType, Type modelType) { - Debug.Assert(supportedInterfaceType != null); Debug.Assert(openBinderType != null); - Debug.Assert(modelType != null); var modelTypeArguments = GetGenericBinderTypeArgs(supportedInterfaceType, modelType); - if (modelTypeArguments == null) { return null; } - var closedNewInstanceType = newInstanceType.MakeGenericType(modelTypeArguments); - if (!modelType.GetTypeInfo().IsAssignableFrom(closedNewInstanceType.GetTypeInfo())) - { - return null; - } - return openBinderType.MakeGenericType(modelTypeArguments); } // Get the generic arguments for the binder, based on the model type. Or null if not compatible. private static Type[] GetGenericBinderTypeArgs(Type supportedInterfaceType, Type modelType) { + Debug.Assert(supportedInterfaceType != null); + Debug.Assert(modelType != null); + var modelTypeInfo = modelType.GetTypeInfo(); if (!modelTypeInfo.IsGenericType || modelTypeInfo.IsGenericTypeDefinition) { - // not a closed generic type + // modelType is not a closed generic type. return null; } var modelTypeArguments = modelTypeInfo.GenericTypeArguments; if (modelTypeArguments.Length != supportedInterfaceType.GetTypeInfo().GenericTypeParameters.Length) { - // wrong number of generic type arguments + // Wrong number of generic type arguments. + return null; + } + + var closedInstanceType = supportedInterfaceType.MakeGenericType(modelTypeArguments); + if (!closedInstanceType.IsAssignableFrom(modelType)) + { + // modelType is not compatible with supportedInterfaceType. return null; } diff --git a/src/Microsoft.AspNet.Mvc.Core/ModelBinding/ICollectionModelBinder.cs b/src/Microsoft.AspNet.Mvc.Core/ModelBinding/ICollectionModelBinder.cs new file mode 100644 index 0000000000..13c4be6091 --- /dev/null +++ b/src/Microsoft.AspNet.Mvc.Core/ModelBinding/ICollectionModelBinder.cs @@ -0,0 +1,27 @@ +// Copyright (c) .NET Foundation. All rights reserved. +// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. + +using System; + +namespace Microsoft.AspNet.Mvc.ModelBinding +{ + /// + /// Interface for model binding collections. + /// + public interface ICollectionModelBinder : IModelBinder + { + /// + /// Gets an indication whether or not this implementation can create + /// an assignable to . + /// + /// of the model. + /// + /// true if this implementation can create an + /// assignable to ; false otherwise. + /// + /// + /// A true return value is necessary for successful model binding if model is initially null. + /// + bool CanCreateInstance(Type targetType); + } +} diff --git a/test/Microsoft.AspNet.Mvc.Core.Test/ModelBinding/CollectionModelBinderTest.cs b/test/Microsoft.AspNet.Mvc.Core.Test/ModelBinding/CollectionModelBinderTest.cs index 8b80636b7c..413b6687a4 100644 --- a/test/Microsoft.AspNet.Mvc.Core.Test/ModelBinding/CollectionModelBinderTest.cs +++ b/test/Microsoft.AspNet.Mvc.Core.Test/ModelBinding/CollectionModelBinderTest.cs @@ -1,8 +1,9 @@ // Copyright (c) .NET Foundation. All rights reserved. // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. -#if DNX451 +using System; using System.Collections.Generic; +#if DNX451 using System.Globalization; using System.Linq; #endif @@ -275,6 +276,44 @@ namespace Microsoft.AspNet.Mvc.ModelBinding.Test Assert.Same(result.ValidationNode.ModelMetadata, context.ModelMetadata); } + // Setup like CollectionModelBinder_CreatesEmptyCollection_IfIsTopLevelObjectAndNotIsFirstChanceBinding except + // Model already has a value. + [Fact] + public async Task CollectionModelBinder_DoesNotCreateEmptyCollection_IfModelNonNull() + { + // Arrange + var binder = new CollectionModelBinder(); + + var context = CreateContext(); + context.IsTopLevelObject = true; + + var list = new List(); + context.Model = list; + + // Lack of prefix and non-empty model name both ignored. + context.ModelName = "modelName"; + + var metadataProvider = context.OperationBindingContext.MetadataProvider; + context.ModelMetadata = metadataProvider.GetMetadataForType(typeof(List)); + + context.ValueProvider = new TestValueProvider(new Dictionary()); + + // Act + var result = await binder.BindModelAsync(context); + + // Assert + Assert.NotNull(result); + + Assert.Same(list, result.Model); + Assert.Empty(list); + Assert.Equal("modelName", result.Key); + Assert.True(result.IsModelSet); + + Assert.Same(result.ValidationNode.Model, result.Model); + Assert.Same(result.ValidationNode.Key, result.Key); + Assert.Same(result.ValidationNode.ModelMetadata, context.ModelMetadata); + } + [Theory] [InlineData("")] [InlineData("param")] @@ -300,6 +339,39 @@ namespace Microsoft.AspNet.Mvc.ModelBinding.Test Assert.Null(result); } + // Model type -> can create instance. + public static TheoryData CanCreateInstanceData + { + get + { + return new TheoryData + { + { typeof(IEnumerable), true }, + { typeof(ICollection), true }, + { typeof(IList), true }, + { typeof(List), true }, + { typeof(LinkedList), true }, + { typeof(ISet), false }, + { typeof(ListWithInternalConstructor), false }, + { typeof(ListWithThrowingConstructor), false }, + }; + } + } + + [Theory] + [MemberData(nameof(CanCreateInstanceData))] + public void CanCreateInstance_ReturnsExpectedValue(Type modelType, bool expectedResult) + { + // Arrange + var binder = new CollectionModelBinder(); + + // Act + var result = binder.CanCreateInstance(modelType); + + // Assert + Assert.Equal(expectedResult, result); + } + #if DNX451 [Fact] public async Task BindSimpleCollection_SubBindingSucceeds() @@ -335,7 +407,7 @@ namespace Microsoft.AspNet.Mvc.ModelBinding.Test var bindingContext = new ModelBindingContext { - ModelMetadata = metadataProvider.GetMetadataForType(typeof(int)), + ModelMetadata = metadataProvider.GetMetadataForType(typeof(IList)), ModelName = "someName", ValueProvider = valueProvider, OperationBindingContext = new OperationBindingContext @@ -387,5 +459,29 @@ namespace Microsoft.AspNet.Mvc.ModelBinding.Test { public List ListProperty { get; set; } } + + private class ModelWithSimpleProperties + { + public int Id { get; set; } + + public string Name { get; set; } + } + + private class ListWithInternalConstructor : List + { + internal ListWithInternalConstructor() + : base() + { + } + } + + private class ListWithThrowingConstructor : List + { + public ListWithThrowingConstructor() + : base() + { + throw new ApplicationException("No, don't do this."); + } + } } } diff --git a/test/Microsoft.AspNet.Mvc.Core.Test/ModelBinding/DictionaryModelBinderTest.cs b/test/Microsoft.AspNet.Mvc.Core.Test/ModelBinding/DictionaryModelBinderTest.cs index f7d13df7ce..ec67e53eff 100644 --- a/test/Microsoft.AspNet.Mvc.Core.Test/ModelBinding/DictionaryModelBinderTest.cs +++ b/test/Microsoft.AspNet.Mvc.Core.Test/ModelBinding/DictionaryModelBinderTest.cs @@ -112,8 +112,8 @@ namespace Microsoft.AspNet.Mvc.ModelBinding.Test var metadataProvider = context.OperationBindingContext.MetadataProvider; context.ModelMetadata = metadataProvider.GetMetadataForProperty( - typeof(ModelWithDictionaryProperty), - nameof(ModelWithDictionaryProperty.DictionaryProperty)); + typeof(ModelWithDictionaryProperties), + nameof(ModelWithDictionaryProperties.DictionaryProperty)); // Act var result = await binder.BindModelAsync(context); @@ -150,8 +150,8 @@ namespace Microsoft.AspNet.Mvc.ModelBinding.Test var metadataProvider = context.OperationBindingContext.MetadataProvider; context.ModelMetadata = metadataProvider.GetMetadataForProperty( - typeof(ModelWithDictionaryProperty), - nameof(ModelWithDictionaryProperty.DictionaryProperty)); + typeof(ModelWithDictionaryProperties), + nameof(ModelWithDictionaryProperties.DictionaryProperty)); // Act var result = await binder.BindModelAsync(context); @@ -202,8 +202,8 @@ namespace Microsoft.AspNet.Mvc.ModelBinding.Test var metadataProvider = context.OperationBindingContext.MetadataProvider; context.ModelMetadata = metadataProvider.GetMetadataForProperty( - typeof(ModelWithDictionaryProperty), - nameof(ModelWithDictionaryProperty.DictionaryProperty)); + typeof(ModelWithDictionaryProperties), + nameof(ModelWithDictionaryProperties.DictionaryWithValueTypesProperty)); // Act var result = await binder.BindModelAsync(context); @@ -244,8 +244,8 @@ namespace Microsoft.AspNet.Mvc.ModelBinding.Test var metadataProvider = context.OperationBindingContext.MetadataProvider; context.ModelMetadata = metadataProvider.GetMetadataForProperty( - typeof(ModelWithDictionaryProperty), - nameof(ModelWithDictionaryProperty.DictionaryProperty)); + typeof(ModelWithDictionaryProperties), + nameof(ModelWithDictionaryProperties.DictionaryWithComplexValuesProperty)); // Act var result = await binder.BindModelAsync(context); @@ -261,6 +261,41 @@ namespace Microsoft.AspNet.Mvc.ModelBinding.Test Assert.Equal(dictionary, resultDictionary); } + [Theory] + [MemberData(nameof(StringToStringData))] + public async Task BindModel_FallsBackToBindingValues_WithCustomDictionary( + string modelName, + string keyFormat, + IDictionary dictionary) + { + // Arrange + var expectedDictionary = new SortedDictionary(dictionary); + var binder = new DictionaryModelBinder(); + var context = CreateContext(); + context.ModelName = modelName; + context.OperationBindingContext.ModelBinder = CreateCompositeBinder(); + context.OperationBindingContext.ValueProvider = CreateEnumerableValueProvider(keyFormat, dictionary); + context.ValueProvider = context.OperationBindingContext.ValueProvider; + + var metadataProvider = context.OperationBindingContext.MetadataProvider; + context.ModelMetadata = metadataProvider.GetMetadataForProperty( + typeof(ModelWithDictionaryProperties), + nameof(ModelWithDictionaryProperties.CustomDictionaryProperty)); + + // Act + var result = await binder.BindModelAsync(context); + + // Assert + Assert.NotNull(result); + Assert.False(result.IsFatalError); + Assert.True(result.IsModelSet); + Assert.Equal(modelName, result.Key); + Assert.NotNull(result.ValidationNode); + + var resultDictionary = Assert.IsAssignableFrom>(result.Model); + Assert.Equal(expectedDictionary, resultDictionary); + } + [Fact] public async Task DictionaryModelBinder_DoesNotCreateCollection_IfIsTopLevelObjectAndIsFirstChanceBinding() { @@ -332,8 +367,8 @@ namespace Microsoft.AspNet.Mvc.ModelBinding.Test var metadataProvider = context.OperationBindingContext.MetadataProvider; context.ModelMetadata = metadataProvider.GetMetadataForProperty( - typeof(ModelWithDictionaryProperty), - nameof(ModelWithDictionaryProperty.DictionaryProperty)); + typeof(ModelWithDictionaryProperties), + nameof(ModelWithDictionaryProperties.DictionaryProperty)); context.ValueProvider = new TestValueProvider(new Dictionary()); @@ -344,6 +379,39 @@ namespace Microsoft.AspNet.Mvc.ModelBinding.Test Assert.Null(result); } + // Model type -> can create instance. + public static TheoryData CanCreateInstanceData + { + get + { + return new TheoryData + { + { typeof(IEnumerable>), true }, + { typeof(ICollection>), true }, + { typeof(IDictionary), true }, + { typeof(Dictionary), true }, + { typeof(SortedDictionary), true }, + { typeof(IList>), false }, + { typeof(DictionaryWithInternalConstructor), false }, + { typeof(DictionaryWithThrowingConstructor), false }, + }; + } + } + + [Theory] + [MemberData(nameof(CanCreateInstanceData))] + public void CanCreateInstance_ReturnsExpectedValue(Type modelType, bool expectedResult) + { + // Arrange + var binder = new DictionaryModelBinder(); + + // Act + var result = binder.CanCreateInstance(modelType); + + // Assert + Assert.Equal(expectedResult, result); + } + private static ModelBindingContext CreateContext() { var modelBindingContext = new ModelBindingContext() @@ -401,7 +469,7 @@ namespace Microsoft.AspNet.Mvc.ModelBinding.Test private static ModelBindingContext GetModelBindingContext(bool isReadOnly) { var metadataProvider = new TestModelMetadataProvider(); - metadataProvider.ForType>().BindingDetails(bd => bd.IsReadOnly = isReadOnly); + metadataProvider.ForType>().BindingDetails(bd => bd.IsReadOnly = isReadOnly); var valueProvider = new SimpleHttpValueProvider { { "someName[0]", new KeyValuePair(42, "forty-two") }, @@ -441,9 +509,16 @@ namespace Microsoft.AspNet.Mvc.ModelBinding.Test return mockKvpBinder.Object; } - private class ModelWithDictionaryProperty + private class ModelWithDictionaryProperties { + // A Dictionary instance cannot be assigned to this property. + public SortedDictionary CustomDictionaryProperty { get; set; } + public Dictionary DictionaryProperty { get; set; } + + public Dictionary DictionaryWithComplexValuesProperty { get; set; } + + public Dictionary DictionaryWithValueTypesProperty { get; set; } } private class ModelWithProperties @@ -471,6 +546,23 @@ namespace Microsoft.AspNet.Mvc.ModelBinding.Test return $"{{{ Id }, '{ Name }'}}"; } } + + private class DictionaryWithInternalConstructor : Dictionary + { + internal DictionaryWithInternalConstructor() + : base() + { + } + } + + private class DictionaryWithThrowingConstructor : Dictionary + { + public DictionaryWithThrowingConstructor() + : base() + { + throw new ApplicationException("No, don't do this."); + } + } } } #endif diff --git a/test/Microsoft.AspNet.Mvc.FunctionalTests/ModelBindingFromQueryTest.cs b/test/Microsoft.AspNet.Mvc.FunctionalTests/ModelBindingFromQueryTest.cs index 3955eebb3c..dd1496b377 100644 --- a/test/Microsoft.AspNet.Mvc.FunctionalTests/ModelBindingFromQueryTest.cs +++ b/test/Microsoft.AspNet.Mvc.FunctionalTests/ModelBindingFromQueryTest.cs @@ -92,9 +92,7 @@ namespace Microsoft.AspNet.Mvc.FunctionalTests var server = TestHelper.CreateServer(_app, SiteName, _configureServices); var client = server.CreateClient(); - var url = - "http://localhost/FromQueryAttribute_Company/CreateDepartment?TestEmployees[0].EmployeeId=1234"; - + var url = "http://localhost/FromQueryAttribute_Company/CreateDepartment?TestEmployees[0].EmployeeId=1234"; // Act var response = await client.GetAsync(url); @@ -118,7 +116,6 @@ namespace Microsoft.AspNet.Mvc.FunctionalTests var url = "http://localhost/FromQueryAttribute_Company/ValidateDepartment?TestEmployees[0].Department=contoso"; - // Act var response = await client.GetAsync(url); diff --git a/test/Microsoft.AspNet.Mvc.IntegrationTests/ActionParametersIntegrationTest.cs b/test/Microsoft.AspNet.Mvc.IntegrationTests/ActionParametersIntegrationTest.cs index 6d9af63990..a691f6fca7 100644 --- a/test/Microsoft.AspNet.Mvc.IntegrationTests/ActionParametersIntegrationTest.cs +++ b/test/Microsoft.AspNet.Mvc.IntegrationTests/ActionParametersIntegrationTest.cs @@ -77,7 +77,7 @@ namespace Microsoft.AspNet.Mvc.IntegrationTests public CustomReadOnlyCollection
Address { get; set; } } - [Fact(Skip = "Concrete Collection types don't work with GenericModelBinder #2793")] + [Fact] public async Task ActionParameter_ReadOnlyCollectionModel_EmptyPrefix_DoesNotGetBound() { // Arrange @@ -106,12 +106,17 @@ namespace Microsoft.AspNet.Mvc.IntegrationTests Assert.NotNull(boundModel); Assert.NotNull(boundModel.Address); - // Arrays should not be updated. - Assert.Equal(0, boundModel.Address.Count()); + // Read-only collection should not be updated. + Assert.Empty(boundModel.Address); - // ModelState + // ModelState (data is valid but is not copied into Address). Assert.True(modelState.IsValid); - Assert.Empty(modelState.Keys); + var entry = Assert.Single(modelState); + Assert.Equal("Address[0].Street", entry.Key); + var state = entry.Value; + Assert.NotNull(state); + Assert.Equal(ModelValidationState.Valid, state.ValidationState); + Assert.Equal("SomeStreet", state.Value.RawValue); } private class Person4 @@ -252,7 +257,7 @@ namespace Microsoft.AspNet.Mvc.IntegrationTests Assert.Equal(ModelValidationState.Valid, modelState[key].ValidationState); } - [Fact(Skip = "Concrete Collection types don't work with GenericModelBinder #2793")] + [Fact] public async Task ActionParameter_ReadOnlyCollectionModel_WithPrefix_DoesNotGetBound() { // Arrange @@ -285,12 +290,17 @@ namespace Microsoft.AspNet.Mvc.IntegrationTests Assert.NotNull(boundModel); Assert.NotNull(boundModel.Address); - // Arrays should not be updated. - Assert.Equal(0, boundModel.Address.Count()); + // Read-only collection should not be updated. + Assert.Empty(boundModel.Address); - // ModelState + // ModelState (data is valid but is not copied into Address). Assert.True(modelState.IsValid); - Assert.Empty(modelState.Keys); + var entry = Assert.Single(modelState); + Assert.Equal("prefix.Address[0].Street", entry.Key); + var state = entry.Value; + Assert.NotNull(state); + Assert.Equal(ModelValidationState.Valid, state.ValidationState); + Assert.Equal("SomeStreet", state.Value.RawValue); } [Fact] @@ -380,11 +390,12 @@ namespace Microsoft.AspNet.Mvc.IntegrationTests Assert.Empty(modelState.Keys); } - private class CustomReadOnlyCollection : ICollection, IReadOnlyCollection + private class CustomReadOnlyCollection : ICollection { private ICollection _original; - public CustomReadOnlyCollection() : this(new List()) + public CustomReadOnlyCollection() + : this(new List()) { } diff --git a/test/Microsoft.AspNet.Mvc.IntegrationTests/TryUpdateModelIntegrationTest.cs b/test/Microsoft.AspNet.Mvc.IntegrationTests/TryUpdateModelIntegrationTest.cs index 2b5882c895..782cac6fea 100644 --- a/test/Microsoft.AspNet.Mvc.IntegrationTests/TryUpdateModelIntegrationTest.cs +++ b/test/Microsoft.AspNet.Mvc.IntegrationTests/TryUpdateModelIntegrationTest.cs @@ -173,7 +173,7 @@ namespace Microsoft.AspNet.Mvc.IntegrationTests public CustomReadOnlyCollection
Address { get; set; } } - [Fact(Skip = "Concrete Collection types don't work with GenericModelBinder #2793")] + [Fact(Skip = "Validation incorrect for collections when using TryUpdateModel, #2941")] public async Task TryUpdateModel_ReadOnlyCollectionModel_EmptyPrefix_DoesNotGetBound() { // Arrange @@ -184,6 +184,7 @@ namespace Microsoft.AspNet.Mvc.IntegrationTests var modelState = new ModelStateDictionary(); var model = new Person6(); + // Act var result = await TryUpdateModel(model, string.Empty, operationContext, modelState); @@ -193,12 +194,17 @@ namespace Microsoft.AspNet.Mvc.IntegrationTests // Model Assert.NotNull(model.Address); - // Arrays should not be updated. - Assert.Equal(0, model.Address.Count()); + // Read-only collection should not be updated. + Assert.Empty(model.Address); - // ModelState + // ModelState (data is valid but is not copied into Address). Assert.True(modelState.IsValid); - Assert.Empty(modelState.Keys); + var entry = Assert.Single(modelState); + Assert.Equal("Address[0].Street", entry.Key); + var state = entry.Value; + Assert.NotNull(state); + Assert.Equal(ModelValidationState.Valid, state.ValidationState); + Assert.Equal("SomeStreet", state.Value.RawValue); } private class Person4 @@ -409,7 +415,7 @@ namespace Microsoft.AspNet.Mvc.IntegrationTests Assert.Equal(ModelValidationState.Valid, modelState[key].ValidationState); } - [Fact(Skip = "Concrete Collection types don't work with GenericModelBinder #2793")] + [Fact(Skip = "Validation incorrect for collections when using TryUpdateModel, #2941")] public async Task TryUpdateModel_ReadOnlyCollectionModel_WithPrefix_DoesNotGetBound() { // Arrange @@ -420,6 +426,7 @@ namespace Microsoft.AspNet.Mvc.IntegrationTests var modelState = new ModelStateDictionary(); var model = new Person6(); + // Act var result = await TryUpdateModel(model, "prefix", operationContext, modelState); @@ -429,12 +436,17 @@ namespace Microsoft.AspNet.Mvc.IntegrationTests // Model Assert.NotNull(model.Address); - // Arrays should not be updated. - Assert.Equal(0, model.Address.Count()); + // Read-only collection should not be updated. + Assert.Empty(model.Address); - // ModelState + // ModelState (data is valid but is not copied into Address). Assert.True(modelState.IsValid); - Assert.Empty(modelState.Keys); + var entry = Assert.Single(modelState); + Assert.Equal("prefix.Address[0].Street", entry.Key); + var state = entry.Value; + Assert.NotNull(state); + Assert.Equal(ModelValidationState.Valid, state.ValidationState); + Assert.Equal("SomeStreet", state.Value.RawValue); } [Fact] @@ -499,11 +511,12 @@ namespace Microsoft.AspNet.Mvc.IntegrationTests Assert.Empty(modelState.Keys); } - private class CustomReadOnlyCollection : ICollection, IReadOnlyCollection + private class CustomReadOnlyCollection : ICollection { private ICollection _original; - public CustomReadOnlyCollection() : this(new List()) + public CustomReadOnlyCollection() + : this(new List()) { }