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<T>`
- correct `ModelMetadata` of a few tests that previously did not need that information

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()
+        /// <inheritdoc />
+        public override bool CanCreateInstance(Type targetType)
         {
+            return targetType == typeof(TElement[]);
+        }
+
+        /// <inheritdoc />
+        protected override object CreateEmptyCollection(Type targetType)
+        {
+            Debug.Assert(targetType == typeof(TElement[]), "GenericModelBinder only creates this binder for arrays.");
+
             return new TElement[0];
         }
 
         /// <inheritdoc />
-        protected override object GetModel(IEnumerable<TElement> newCollection)
+        protected override object ConvertToCollectionType(Type targetType, IEnumerable<TElement> collection)
         {
-            return newCollection?.ToArray();
+            Debug.Assert(targetType == typeof(TElement[]), "GenericModelBinder only creates this binder for arrays.");
+
+            // If non-null, collection is a List<TElement>, never already a TElement[].
+            return collection?.ToArray();
         }
 
         /// <inheritdoc />

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
     /// <see cref="IModelBinder"/> implementation for binding collection values.
     /// </summary>
     /// <typeparam name="TElement">Type of elements in the collection.</typeparam>
-    public class CollectionModelBinder<TElement> : IModelBinder
+    public class CollectionModelBinder<TElement> : ICollectionModelBinder
     {
         /// <inheritdoc />
         public virtual async Task<ModelBindingResult> 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<TElement> 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.
+        /// <inheritdoc />
-        protected virtual object CreateEmptyCollection()
+        public virtual bool CanCreateInstance(Type targetType)
         {
-            return new List<TElement>();
+            return CreateEmptyCollection(targetType) != null;
+        }
+
+        /// <summary>
+        /// Create an <see cref="object"/> assignable to <paramref name="targetType"/>.
+        /// </summary>
+        /// <param name="targetType"><see cref="Type"/> of the model.</param>
+        /// <returns>An <see cref="object"/> assignable to <paramref name="targetType"/>.</returns>
+        /// <remarks>Called when creating a default 'empty' model for a top level bind.</remarks>
+        protected virtual object CreateEmptyCollection(Type targetType)
+        {
+            if (targetType.IsAssignableFrom(typeof(List<TElement>)))
+            {
+                // Simple case such as ICollection<TElement>, IEnumerable<TElement> and IList<TElement>.
+                return new List<TElement>();
+            }
+
+            return CreateInstance(targetType);
+        }
+
+        /// <summary>
+        /// Create an instance of <paramref name="targetType"/>.
+        /// </summary>
+        /// <param name="targetType"><see cref="Type"/> of the model.</param>
+        /// <returns>An instance of <paramref name="targetType"/>.</returns>
+        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<CollectionResult> 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<CollectionResult> BindComplexCollectionFromIndexes(
             ModelBindingContext bindingContext,
             IEnumerable<string> 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
         }
 
         /// <summary>
-        /// Gets an <see cref="object"/> assignable to the collection property.
+        /// Gets an <see cref="object"/> assignable to <paramref name="targetType"/> that contains members from
+        /// <paramref name="collection"/>.
         /// </summary>
-        /// <param name="newCollection">
+        /// <param name="targetType"><see cref="Type"/> of the model.</param>
+        /// <param name="collection">
         /// Collection of values retrieved from value providers. Or <c>null</c> if nothing was bound.
         /// </param>
         /// <returns>
-        /// <see cref="object"/> assignable to the collection property. Or <c>null</c> if nothing was bound.
+        /// An <see cref="object"/> assignable to <paramref name="targetType"/>. Or <c>null</c> if nothing was bound.
         /// </returns>
         /// <remarks>
         /// Extensibility point that allows the bound collection to be manipulated or transformed before being
         /// returned from the binder.
         /// </remarks>
-        protected virtual object GetModel(IEnumerable<TElement> newCollection)
+        protected virtual object ConvertToCollectionType(Type targetType, IEnumerable<TElement> collection)
         {
-            // Depends on fact BindSimpleCollection() and BindComplexCollection() always return a List<TElement>
+            if (collection == null)
-            // instance or null. In addition GenericModelBinder confirms a List<TElement> is assignable to the
+            {
-            // property prior to instantiating this binder and subclass binders do not call this method.
+                return null;
+            }
+
+            if (targetType.IsAssignableFrom(typeof(List<TElement>)))
+            {
+                // Depends on fact BindSimpleCollection() and BindComplexCollection() always return a List<TElement>
+                // instance or null.
+                return collection;
+            }
+
+            var newCollection = CreateInstance(targetType);
+            CopyToModel(newCollection, collection);
+
             return newCollection;
         }
 
@@ -254,11 +308,10 @@ namespace Microsoft.AspNet.Mvc.ModelBinding
         /// <param name="sourceCollection">
         /// Collection of values retrieved from value providers. Or <c>null</c> if nothing was bound.
         /// </param>
-        /// <remarks>Called only in TryUpdateModelAsync(collection, ...) scenarios.</remarks>
         protected virtual void CopyToModel([NotNull] object target, IEnumerable<TElement> sourceCollection)
         {
             var targetCollection = target as ICollection<TElement>;
-            Debug.Assert(targetCollection != null); // This binder is instantiated only for ICollection model types.
+            Debug.Assert(targetCollection != null, "This binder is instantiated only for ICollection<T> 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
 

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<TKey, TValue>)result.Model;
+            var model = (IDictionary<TKey, TValue>)result.Model;
             if (model.Count != 0)
             {
                 // ICollection<KeyValuePair<TKey, TValue>> approach was successful.
@@ -80,15 +84,37 @@ namespace Microsoft.AspNet.Mvc.ModelBinding
         }
 
         /// <inheritdoc />
-        protected override object GetModel(IEnumerable<KeyValuePair<TKey, TValue>> newCollection)
+        protected override object ConvertToCollectionType(
+            Type targetType,
+            IEnumerable<KeyValuePair<TKey, TValue>> collection)
         {
-            return newCollection?.ToDictionary(kvp => kvp.Key, kvp => kvp.Value);
+            if (collection == null)
+            {
+                return null;
+            }
+
+            if (targetType.IsAssignableFrom(typeof(Dictionary<TKey, TValue>)))
+            {
+                // Collection is a List<KeyValuePair<TKey, TValue>>, never already a Dictionary<TKey, TValue>.
+                return collection.ToDictionary(kvp => kvp.Key, kvp => kvp.Value);
+            }
+
+            var newCollection = CreateInstance(targetType);
+            CopyToModel(newCollection, collection);
+
+            return newCollection;
         }
 
         /// <inheritdoc />
-        protected override object CreateEmptyCollection()
+        protected override object CreateEmptyCollection(Type targetType)
         {
-            return new Dictionary<TKey, TValue>();
+            if (targetType.IsAssignableFrom(typeof(Dictionary<TKey, TValue>)))
+            {
+                // Simple case such as IDictionary<TKey, TValue>.
+                return new Dictionary<TKey, TValue>();
+            }
+
+            return CreateInstance(targetType);
         }
 
         private static TKey ConvertFromString(string keyString)

src/Microsoft.AspNet.Mvc.Core/ModelBinding/GenericModelBinder.cs

@@ -13,12 +13,21 @@ namespace Microsoft.AspNet.Mvc.ModelBinding
     {
         public async Task<ModelBindingResult> 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) ??
+                GetCollectionBinder(modelType) ??
-                   GetDictionaryBinder(modelType) ??
+                GetDictionaryBinder(modelType) ??
-                   GetKeyValuePairBinder(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(ICollection<>),
-                        typeof(List<>),
+                typeof(CollectionModelBinder<>),
-                        typeof(CollectionModelBinder<>),
+                modelType);
-                        modelType);
         }
 
         private static Type GetDictionaryBinder(Type modelType)
         {
             return GetGenericBinderType(
-                        typeof(IDictionary<,>),
+                typeof(IDictionary<,>),
-                        typeof(Dictionary<,>),
+                typeof(DictionaryModelBinder<,>),
-                        typeof(DictionaryModelBinder<,>),
+                modelType);
-                        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<T>. Can a
+                // List<T> (the default CollectionModelBinder type) instance be used instead of that exact type?
+                // Likely this will succeed only if the property type is exactly IEnumerable<T>.
+                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<T>. For example an IEnumerable<T> property may have a List<T> 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;
         }
 
-        /// <remarks>
+        // Example: GetGenericBinderType(typeof(IList<T>), typeof(ListBinder<T>), ...) means that the ListBinder<T>
-        /// Example: <c>GetGenericBinderType(typeof(IList&lt;T&gt;), typeof(List&lt;T&gt;),
+        // type can update models that implement IList<T>. This method will return
-        /// typeof(ListBinder&lt;T&gt;), ...)</c> means that the <c>ListBinder&lt;T&gt;</c> type can update models that
+        // ListBinder<T> or null, depending on whether the type checks succeed.
-        /// implement <see cref="IList{T}"/>, and if for some reason the existing model instance is not updatable the
+        private static Type GetGenericBinderType(Type supportedInterfaceType, Type openBinderType, Type modelType)
-        /// binder will create a <see cref="List{T}"/> object and bind to that instead. This method will return
-        /// <c>ListBinder&lt;T&gt;</c> or <c>null</c>, depending on whether the type and updatability checks succeed.
-        /// </remarks>
-        private static Type GetGenericBinderType(Type supportedInterfaceType,
-                                                 Type newInstanceType,
-                                                 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;
             }
 

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
+{
+    /// <summary>
+    /// Interface for model binding collections.
+    /// </summary>
+    public interface ICollectionModelBinder : IModelBinder
+    {
+        /// <summary>
+        /// Gets an indication whether or not this <see cref="ICollectionModelBinder"/> implementation can create
+        /// an <see cref="object"/> assignable to <paramref name="targetType"/>.
+        /// </summary>
+        /// <param name="targetType"><see cref="Type"/> of the model.</param>
+        /// <returns>
+        /// <c>true</c> if this <see cref="ICollectionModelBinder"/> implementation can create an <see cref="object"/>
+        /// assignable to <paramref name="targetType"/>; <c>false</c> otherwise.
+        /// </returns>
+        /// <remarks>
+        /// A <c>true</c> return value is necessary for successful model binding if model is initially <c>null</c>.
+        /// </remarks>
+        bool CanCreateInstance(Type targetType);
+    }
+}

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<string>();
+
+            var context = CreateContext();
+            context.IsTopLevelObject = true;
+
+            var list = new List<string>();
+            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<string>));
+
+            context.ValueProvider = new TestValueProvider(new Dictionary<string, object>());
+
+            // 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<Type, bool> CanCreateInstanceData
+        {
+            get
+            {
+                return new TheoryData<Type, bool>
+                {
+                    { typeof(IEnumerable<int>), true },
+                    { typeof(ICollection<int>), true },
+                    { typeof(IList<int>), true },
+                    { typeof(List<int>), true },
+                    { typeof(LinkedList<int>), true },
+                    { typeof(ISet<int>), false },
+                    { typeof(ListWithInternalConstructor<int>), false },
+                    { typeof(ListWithThrowingConstructor<int>), false },
+                };
+            }
+        }
+
+        [Theory]
+        [MemberData(nameof(CanCreateInstanceData))]
+        public void CanCreateInstance_ReturnsExpectedValue(Type modelType, bool expectedResult)
+        {
+            // Arrange
+            var binder = new CollectionModelBinder<int>();
+
+            // 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<int>)),
                 ModelName = "someName",
                 ValueProvider = valueProvider,
                 OperationBindingContext = new OperationBindingContext
@@ -387,5 +459,29 @@ namespace Microsoft.AspNet.Mvc.ModelBinding.Test
         {
             public List<string> ListProperty { get; set; }
         }
+
+        private class ModelWithSimpleProperties
+        {
+            public int Id { get; set; }
+
+            public string Name { get; set; }
+        }
+
+        private class ListWithInternalConstructor<T> : List<T>
+        {
+            internal ListWithInternalConstructor()
+                : base()
+            {
+            }
+        }
+
+        private class ListWithThrowingConstructor<T> : List<T>
+        {
+            public ListWithThrowingConstructor()
+                : base()
+            {
+                throw new ApplicationException("No, don't do this.");
+            }
+        }
     }
 }

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),
+                typeof(ModelWithDictionaryProperties),
-                nameof(ModelWithDictionaryProperty.DictionaryProperty));
+                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),
+                typeof(ModelWithDictionaryProperties),
-                nameof(ModelWithDictionaryProperty.DictionaryProperty));
+                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),
+                typeof(ModelWithDictionaryProperties),
-                nameof(ModelWithDictionaryProperty.DictionaryProperty));
+                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),
+                typeof(ModelWithDictionaryProperties),
-                nameof(ModelWithDictionaryProperty.DictionaryProperty));
+                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<string, string> dictionary)
+        {
+            // Arrange
+            var expectedDictionary = new SortedDictionary<string, string>(dictionary);
+            var binder = new DictionaryModelBinder<string, string>();
+            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<SortedDictionary<string, string>>(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),
+                typeof(ModelWithDictionaryProperties),
-                nameof(ModelWithDictionaryProperty.DictionaryProperty));
+                nameof(ModelWithDictionaryProperties.DictionaryProperty));
 
             context.ValueProvider = new TestValueProvider(new Dictionary<string, object>());
 
@@ -344,6 +379,39 @@ namespace Microsoft.AspNet.Mvc.ModelBinding.Test
             Assert.Null(result);
         }
 
+        // Model type -> can create instance.
+        public static TheoryData<Type, bool> CanCreateInstanceData
+        {
+            get
+            {
+                return new TheoryData<Type, bool>
+                {
+                    { typeof(IEnumerable<KeyValuePair<int, int>>), true },
+                    { typeof(ICollection<KeyValuePair<int, int>>), true },
+                    { typeof(IDictionary<int, int>), true },
+                    { typeof(Dictionary<int, int>), true },
+                    { typeof(SortedDictionary<int, int>), true },
+                    { typeof(IList<KeyValuePair<int, int>>), false },
+                    { typeof(DictionaryWithInternalConstructor<int, int>), false },
+                    { typeof(DictionaryWithThrowingConstructor<int, int>), false },
+                };
+            }
+        }
+
+        [Theory]
+        [MemberData(nameof(CanCreateInstanceData))]
+        public void CanCreateInstance_ReturnsExpectedValue(Type modelType, bool expectedResult)
+        {
+            // Arrange
+            var binder = new DictionaryModelBinder<int, int>();
+
+            // 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<List<int>>().BindingDetails(bd => bd.IsReadOnly = isReadOnly);
+            metadataProvider.ForType<IDictionary<int, string>>().BindingDetails(bd => bd.IsReadOnly = isReadOnly);
             var valueProvider = new SimpleHttpValueProvider
             {
                 { "someName[0]", new KeyValuePair<int, string>(42, "forty-two") },
@@ -441,9 +509,16 @@ namespace Microsoft.AspNet.Mvc.ModelBinding.Test
             return mockKvpBinder.Object;
         }
 
-        private class ModelWithDictionaryProperty
+        private class ModelWithDictionaryProperties
         {
+            // A Dictionary<string, string> instance cannot be assigned to this property.
+            public SortedDictionary<string, string> CustomDictionaryProperty { get; set; }
+
             public Dictionary<string, string> DictionaryProperty { get; set; }
+
+            public Dictionary<int, ModelWithProperties> DictionaryWithComplexValuesProperty { get; set; }
+
+            public Dictionary<long, int> DictionaryWithValueTypesProperty { get; set; }
         }
 
         private class ModelWithProperties
@@ -471,6 +546,23 @@ namespace Microsoft.AspNet.Mvc.ModelBinding.Test
                 return $"{{{ Id }, '{ Name }'}}";
             }
         }
+
+        private class DictionaryWithInternalConstructor<TKey, TValue> : Dictionary<TKey, TValue>
+        {
+            internal DictionaryWithInternalConstructor()
+                : base()
+            {
+            }
+        }
+
+        private class DictionaryWithThrowingConstructor<TKey, TValue> : Dictionary<TKey, TValue>
+        {
+            public DictionaryWithThrowingConstructor()
+                : base()
+            {
+                throw new ApplicationException("No, don't do this.");
+            }
+        }
     }
 }
 #endif

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 =
+            var url = "http://localhost/FromQueryAttribute_Company/CreateDepartment?TestEmployees[0].EmployeeId=1234";
-                "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);
 

test/Microsoft.AspNet.Mvc.IntegrationTests/ActionParametersIntegrationTest.cs

@@ -77,7 +77,7 @@ namespace Microsoft.AspNet.Mvc.IntegrationTests
             public CustomReadOnlyCollection<Address> 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.
+            // Read-only collection should not be updated.
-            Assert.Equal(0, boundModel.Address.Count());
+            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.
+            // Read-only collection should not be updated.
-            Assert.Equal(0, boundModel.Address.Count());
+            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<T> : ICollection<T>, IReadOnlyCollection<T>
+        private class CustomReadOnlyCollection<T> : ICollection<T>
         {
             private ICollection<T> _original;
 
-            public CustomReadOnlyCollection() : this(new List<T>())
+            public CustomReadOnlyCollection()
+                : this(new List<T>())
             {
             }
 

test/Microsoft.AspNet.Mvc.IntegrationTests/TryUpdateModelIntegrationTest.cs

@@ -173,7 +173,7 @@ namespace Microsoft.AspNet.Mvc.IntegrationTests
             public CustomReadOnlyCollection<Address> 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.
+            // Read-only collection should not be updated.
-            Assert.Equal(0, model.Address.Count());
+            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.
+            // Read-only collection should not be updated.
-            Assert.Equal(0, model.Address.Count());
+            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<T> : ICollection<T>, IReadOnlyCollection<T>
+        private class CustomReadOnlyCollection<T> : ICollection<T>
         {
             private ICollection<T> _original;
 
-            public CustomReadOnlyCollection() : this(new List<T>())
+            public CustomReadOnlyCollection()
+                : this(new List<T>())
             {
             }