Fix #1579 - Bind top-level collections as an empty collection

This change treats 'top-level' collection-type models similarly to top-level POCO model - namely that they will always be instantiated even if there's no data to put inside.
aspnet · May 22, 2015 · 8f38650 · davidfowl · May 22, 2015 · rynowak
1 parent b64fd7a
commit 8f38650
Show file tree

Hide file tree

Showing 15 changed files with 602 additions and 54 deletions.
diff --git a/src/Microsoft.AspNet.Mvc.Core/ModelBinding/ArrayModelBinder.cs b/src/Microsoft.AspNet.Mvc.Core/ModelBinding/ArrayModelBinder.cs
@@ -25,6 +25,11 @@ public override Task<ModelBindingResult> BindModelAsync([NotNull] ModelBindingCo
             return base.BindModelAsync(bindingContext);
         }
 
+        protected override object CreateEmptyCollection()
+        {
+            return new TElement[0];
+        }
+
         /// <inheritdoc />
         protected override object GetModel(IEnumerable<TElement> newCollection)
         {

diff --git a/src/Microsoft.AspNet.Mvc.Core/ModelBinding/CollectionModelBinder.cs b/src/Microsoft.AspNet.Mvc.Core/ModelBinding/CollectionModelBinder.cs
@@ -23,8 +23,31 @@ public virtual async Task<ModelBindingResult> BindModelAsync([NotNull] ModelBind
         {
             ModelBindingHelper.ValidateBindingContext(bindingContext);
 
+            object model;
+
             if (!await bindingContext.ValueProvider.ContainsPrefixAsync(bindingContext.ModelName))
             {
+                // If this is the fallback case, and we failed to find data as a top-level model, then generate a
+                // default 'empty' model and return it.
+                var isTopLevelObject = bindingContext.ModelMetadata.ContainerType == null;
+                var hasExplicitAlias = bindingContext.BinderModelName != null;
+
+                if (isTopLevelObject && (hasExplicitAlias || bindingContext.ModelName == string.Empty))
+                {
+                    model = CreateEmptyCollection();
+
+                    var validationNode = new ModelValidationNode(
+                        bindingContext.ModelName,
+                        bindingContext.ModelMetadata,
+                        model);
+
+                    return new ModelBindingResult(
+                        model,
+                        bindingContext.ModelName,
+                        isModelSet: true,
+                        validationNode: validationNode);
+                }
+
                 return null;
             }
 
@@ -46,7 +69,7 @@ public virtual async Task<ModelBindingResult> BindModelAsync([NotNull] ModelBind
                 boundCollection = result.Model;
             }
 
-            var model = bindingContext.Model;
+            model = bindingContext.Model;
             if (model == null)
             {
                 model = GetModel(boundCollection);
@@ -64,6 +87,12 @@ public virtual async Task<ModelBindingResult> BindModelAsync([NotNull] ModelBind
                 validationNode: result?.ValidationNode);
         }
 
+        // Called when we're creating a default 'empty' model for a top level bind.
+        protected virtual object CreateEmptyCollection()
+        {
+            return new List<TElement>();
+        }
+
         // 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<CollectionResult> BindSimpleCollection(
@@ -165,8 +194,6 @@ internal async Task<CollectionResult> BindComplexCollectionFromIndexes(
                 var didBind = false;
                 object boundValue = null;
 
-                var modelType = bindingContext.ModelType;
-
                 var result =
                     await bindingContext.OperationBindingContext.ModelBinder.BindModelAsync(childBindingContext);
                 if (result != null && result.IsModelSet)

diff --git a/src/Microsoft.AspNet.Mvc.Core/ModelBinding/DictionaryModelBinder.cs b/src/Microsoft.AspNet.Mvc.Core/ModelBinding/DictionaryModelBinder.cs
@@ -18,5 +18,10 @@ protected override object GetModel(IEnumerable<KeyValuePair<TKey, TValue>> newCo
         {
             return newCollection?.ToDictionary(kvp => kvp.Key, kvp => kvp.Value);
         }
+
+        protected override object CreateEmptyCollection()
+        {
+            return new Dictionary<TKey, TValue>();
+        }
     }
 }
diff --git a/src/Microsoft.AspNet.Mvc.Core/ModelBinding/KeyValuePairModelBinder.cs b/src/Microsoft.AspNet.Mvc.Core/ModelBinding/KeyValuePairModelBinder.cs
@@ -62,8 +62,27 @@ public async Task<ModelBindingResult> BindModelAsync(ModelBindingContext binding
             }
             else
             {
-                // Caller (GenericModelBinder) was able to resolve a binder type and will create a ModelBindingResult
-                // that exits current ModelBinding loop.
+                // If this is the fallback case, and we failed to find data as a top-level model, then generate a
+                // default 'empty' model and return it.
+                var isTopLevelObject = bindingContext.ModelMetadata.ContainerType == null;
+                var hasExplicitAlias = bindingContext.BinderModelName != null;
+
+                if (isTopLevelObject && (hasExplicitAlias || bindingContext.ModelName == string.Empty))
+                {
+                    var model = new KeyValuePair<TKey, TValue>();
+
+                    var validationNode = new ModelValidationNode(
+                        bindingContext.ModelName,
+                        bindingContext.ModelMetadata,
+                        model);
+
+                    return new ModelBindingResult(
+                        model,
+                        bindingContext.ModelName,
+                        isModelSet: true,
+                        validationNode: validationNode);
+                }
+
                 return null;
             }
         }

diff --git a/test/Microsoft.AspNet.Mvc.Core.Test/ModelBinding/ArrayModelBinderTest.cs b/test/Microsoft.AspNet.Mvc.Core.Test/ModelBinding/ArrayModelBinderTest.cs
@@ -2,7 +2,9 @@
 // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.
 
 #if DNX451
+using System.Collections.Generic;
 using System.Threading.Tasks;
+using Microsoft.AspNet.Http.Internal;
 using Moq;
 using Xunit;
 
@@ -35,14 +37,105 @@ public async Task BindModelAsync_ValueProviderContainPrefix_Succeeds()
         }
 
         [Fact]
-        public async Task BindModelAsync_ValueProviderDoesNotContainPrefix_ReturnsNull()
+        public async Task ArrayModelBinder_DoesNotCreateCollection_ForTopLevelModel_OnFirstPass()
         {
             // Arrange
-            var bindingContext = GetBindingContext(new SimpleHttpValueProvider());
-            var binder = new ArrayModelBinder<int>();
+            var binder = new ArrayModelBinder<string>();
+
+            var context = CreateContext();
+            context.ModelName = "param";
+
+            var metadataProvider = context.OperationBindingContext.MetadataProvider;
+            context.ModelMetadata = metadataProvider.GetMetadataForType(typeof(string[]));
+
+            context.ValueProvider = new TestValueProvider(new Dictionary<string, object>());
 
             // Act
-            var result = await binder.BindModelAsync(bindingContext);
+            var result = await binder.BindModelAsync(context);
+
+            // Assert
+            Assert.Null(result);
+        }
+
+        [Fact]
+        public async Task ArrayModelBinder_CreatesEmptyCollection_ForTopLevelModel_OnFallback()
+        {
+            // Arrange
+            var binder = new ArrayModelBinder<string>();
+
+            var context = CreateContext();
+            context.ModelName = string.Empty;
+
+            var metadataProvider = context.OperationBindingContext.MetadataProvider;
+            context.ModelMetadata = metadataProvider.GetMetadataForType(typeof(string[]));
+
+            context.ValueProvider = new TestValueProvider(new Dictionary<string, object>());
+
+            // Act
+            var result = await binder.BindModelAsync(context);
+
+            // Assert
+            Assert.NotNull(result);
+
+            Assert.Empty(Assert.IsType<string[]>(result.Model));
+            Assert.Equal(string.Empty, 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);
+        }
+
+        [Fact]
+        public async Task ArrayModelBinder_CreatesEmptyCollection_ForTopLevelModel_WithExplicitPrefix()
+        {
+            // Arrange
+            var binder = new ArrayModelBinder<string>();
+
+            var context = CreateContext();
+            context.ModelName = "prefix";
+            context.BinderModelName = "prefix";
+
+            var metadataProvider = context.OperationBindingContext.MetadataProvider;
+            context.ModelMetadata = metadataProvider.GetMetadataForType(typeof(string[]));
+
+            context.ValueProvider = new TestValueProvider(new Dictionary<string, object>());
+
+            // Act
+            var result = await binder.BindModelAsync(context);
+
+            // Assert
+            Assert.NotNull(result);
+
+            Assert.Empty(Assert.IsType<string[]>(result.Model));
+            Assert.Equal("prefix", 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")]
+        public async Task ArrayModelBinder_DoesNotCreateCollection_ForNonTopLevelModel(string prefix)
+        {
+            // Arrange
+            var binder = new ArrayModelBinder<string>();
+
+            var context = CreateContext();
+            context.ModelName = ModelNames.CreatePropertyModelName(prefix, "ArrayProperty");
+
+            var metadataProvider = context.OperationBindingContext.MetadataProvider;
+            context.ModelMetadata = metadataProvider.GetMetadataForProperty(
+                typeof(ModelWithArrayProperty),
+                nameof(ModelWithArrayProperty.ArrayProperty));
+
+            context.ValueProvider = new TestValueProvider(new Dictionary<string, object>());
+
+            // Act
+            var result = await binder.BindModelAsync(context);
 
             // Assert
             Assert.Null(result);
@@ -155,6 +248,25 @@ private static ModelBindingContext GetBindingContext(
             };
             return bindingContext;
         }
+
+        private static ModelBindingContext CreateContext()
+        {
+            var modelBindingContext = new ModelBindingContext()
+            {
+                OperationBindingContext = new OperationBindingContext()
+                {
+                    HttpContext = new DefaultHttpContext(),
+                    MetadataProvider = new TestModelMetadataProvider(),
+                }
+            };
+
+            return modelBindingContext;
+        }
+
+        private class ModelWithArrayProperty
+        {
+            public string[] ArrayProperty { get; set; }
+        }
     }
 }
 #endif