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

src/Microsoft.AspNet.Mvc.ModelBinding/Binders/ArrayModelBinder.cs

@@ -2,26 +2,66 @@
 // 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
 {
+    /// <summary>
+    /// <see cref="IModelBinder"/> implementation for binding array values.
+    /// </summary>
+    /// <typeparam name="TElement">Type of elements in the array.</typeparam>
     public class ArrayModelBinder<TElement> : CollectionModelBinder<TElement>
     {
-        public override Task<ModelBindingResult> BindModelAsync(ModelBindingContext bindingContext)
+        /// <inheritdoc />
+        public override Task<ModelBindingResult> BindModelAsync([NotNull] ModelBindingContext bindingContext)
         {
-            if (bindingContext.ModelMetadata.IsReadOnly)
+            if (bindingContext.Model == null && bindingContext.ModelMetadata.IsReadOnly)
             {
                 return Task.FromResult<ModelBindingResult>(null);
             }
 
             return base.BindModelAsync(bindingContext);
         }
 
+        /// <inheritdoc />
         protected override object GetModel(IEnumerable<TElement> newCollection)
         {
+            if (newCollection == null)
+            {
+                return null;
+            }
+
             return newCollection.ToArray();
         }
+
+        /// <inheritdoc />
+        protected override void CopyToModel([NotNull] object target, IEnumerable<TElement> 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);
+            }
+        }
     }
 }

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
 {
+    /// <summary>
+    /// <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 virtual async Task<ModelBindingResult> BindModelAsync(ModelBindingContext bindingContext)
+        /// <inheritdoc />
+        public virtual async Task<ModelBindingResult> BindModelAsync([NotNull] ModelBindingContext bindingContext)
         {
             ModelBindingHelper.ValidateBindingContext(bindingContext);
 
@@ -23,19 +30,40 @@ public virtual async Task<ModelBindingResult> 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<TElement> 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<IEnumerable<TElement>> BindSimpleCollection(ModelBindingContext bindingContext,
-                                                                 object rawValue,
-                                                                 CultureInfo culture)
+        internal async Task<IEnumerable<TElement>> BindSimpleCollection(
+            ModelBindingContext bindingContext,
+            object rawValue,
+            CultureInfo culture)
         {
             if (rawValue == null)
             {
@@ -62,7 +90,8 @@ internal async Task<IEnumerable<TElement>> 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<IEnumerable<TElement>> 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<IEnumerable<TElement>> BindComplexCollectionFromIndexes(ModelBindingContext bindingContext,
-                                                                             IEnumerable<string> indexNames)
+        internal async Task<IEnumerable<TElement>> BindComplexCollectionFromIndexes(
+            ModelBindingContext bindingContext,
+            IEnumerable<string> indexNames)
         {
             bool indexNamesIsFinite;
             if (indexNames != null)
@@ -114,7 +145,8 @@ internal async Task<IEnumerable<TElement>> 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<IEnumerable<TElement>> BindComplexCollectionFromIndexes(Mode
             return boundCollection;
         }
 
-        // Extensibility point that allows the bound collection to be manipulated or transformed before
-        // being returned from the binder.
+        /// <summary>
+        /// Gets an <see cref="object"/> suitable for the associated property.
+        /// </summary>
+        /// <param name="newCollection">
+        /// Collection of values retrieved from value providers. Or <c>null</c> if nothing was bound.
+        /// </param>
+        /// <returns>
+        /// <see cref="object"/> suitable for the associated property. 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)
         {
+            // Depends on fact BindSimpleCollection() and BindComplexCollection() always return a List<TElement>
+            // 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 newCollection;
         }
 
+        /// <summary>
+        /// Adds values from <paramref name="sourceCollection"/> to given <paramref name="target"/>.
+        /// </summary>
+        /// <param name="target"><see cref="object"/> into which values are copied.</param>
+        /// <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.
+
+            if (sourceCollection != null && targetCollection != null)
+            {
+                foreach (var element in sourceCollection)
+                {
+                    targetCollection.Add(element);
+                }
+            }
+        }
+
         internal static object[] RawValueToObjectArray(object rawValue)
         {
             // precondition: rawValue is not null

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
 {
+    /// <summary>
+    /// <see cref="IModelBinder"/> implementation for binding dictionary values.
+    /// </summary>
+    /// <typeparam name="TKey">Type of keys in the dictionary.</typeparam>
+    /// <typeparam name="TValue">Type of values in the dictionary.</typeparam>
     public class DictionaryModelBinder<TKey, TValue> : CollectionModelBinder<KeyValuePair<TKey, TValue>>
     {
+        /// <inheritdoc />
         protected override object GetModel(IEnumerable<KeyValuePair<TKey, TValue>> newCollection)
         {
+            if (newCollection == null)
+            {
+                return null;
+            }
+
             return newCollection.ToDictionary(kvp => kvp.Key, kvp => kvp.Value);
         }
     }