From 289c6a70ba7e527486e3c2329aba7447a5f86bfc Mon Sep 17 00:00:00 2001 From: Ryan Nowak Date: Tue, 19 May 2015 14:32:11 -0700 Subject: [PATCH 1/3] 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. --- .../ModelBinding/GenericModelBinder.cs | 125 ++++++++++----- .../ModelBinding/GenericModelBinderTest.cs | 149 ++++++++++++++++++ .../ArrayModelBinderIntegrationTest.cs | 12 +- .../ByteArrayModelBinderIntegrationTest.cs | 8 +- .../CollectionModelBinderIntegrationTest.cs | 18 +-- .../DictionaryModelBinderIntegrationTest.cs | 12 +- .../GenericModelBinderIntegrationTest.cs | 24 +-- .../KeyValuePairModelBinderIntegrationTest.cs | 14 +- .../ValidationIntegrationTests.cs | 6 +- 9 files changed, 285 insertions(+), 83 deletions(-) create mode 100644 test/Microsoft.AspNet.Mvc.Core.Test/ModelBinding/GenericModelBinderTest.cs diff --git a/src/Microsoft.AspNet.Mvc.Core/ModelBinding/GenericModelBinder.cs b/src/Microsoft.AspNet.Mvc.Core/ModelBinding/GenericModelBinder.cs index 2997e7f8b1..2c7599283e 100644 --- a/src/Microsoft.AspNet.Mvc.Core/ModelBinding/GenericModelBinder.cs +++ b/src/Microsoft.AspNet.Mvc.Core/ModelBinding/GenericModelBinder.cs @@ -13,25 +13,54 @@ public class GenericModelBinder : IModelBinder { public async Task BindModelAsync(ModelBindingContext bindingContext) { - var binderType = ResolveBinderType(bindingContext.ModelType); - if (binderType != null) + var bindingInfo = ResolveGenericBindingInfo(bindingContext.ModelType); + if (bindingInfo != null) { - var binder = (IModelBinder)Activator.CreateInstance(binderType); + var binder = (IModelBinder)Activator.CreateInstance(bindingInfo.ModelBinderType); var result = await binder.BindModelAsync(bindingContext); - var modelBindingResult = result != null ? - new ModelBindingResult(result.Model, result.Key, result.IsModelSet, result.ValidationNode) : - new ModelBindingResult(model: null, key: bindingContext.ModelName, isModelSet: false); - - // Were able to resolve a binder type. + if (result != null && result.IsModelSet) + { + // Success - propegate the values returned by the model binder. + return new ModelBindingResult(result.Model, result.Key, result.IsModelSet, result.ValidationNode); + } + + // If this is the fallback case, and we didn't bind 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 = result?.Model; + if (model == null && bindingInfo.UnderlyingModelType.IsArray) + { + model = Array.CreateInstance(bindingInfo.UnderlyingModelType.GetElementType(), 0); + } + else if (model == null) + { + model = Activator.CreateInstance(bindingInfo.UnderlyingModelType); + } + + var validationNode = new ModelValidationNode( + bindingContext.ModelName, + bindingContext.ModelMetadata, + model); + + return new ModelBindingResult(model, bindingContext.ModelName, true, validationNode); + } + + // We always want to return a result for model-types that we handle. + // // Always tell the model binding system to skip other model binders i.e. return non-null. - return modelBindingResult; + return new ModelBindingResult(model: null, key: bindingContext.ModelName, isModelSet: false); } return null; } - private static Type ResolveBinderType(Type modelType) + private static GenericModelBindingInfo ResolveGenericBindingInfo(Type modelType) { return GetArrayBinder(modelType) ?? GetCollectionBinder(modelType) ?? @@ -39,57 +68,65 @@ private static Type ResolveBinderType(Type modelType) GetKeyValuePairBinder(modelType); } - private static Type GetArrayBinder(Type modelType) + private static GenericModelBindingInfo GetArrayBinder(Type modelType) { if (modelType.IsArray) { var elementType = modelType.GetElementType(); - return typeof(ArrayModelBinder<>).MakeGenericType(elementType); + var modelBinderType = typeof(ArrayModelBinder<>).MakeGenericType(elementType); + + return new GenericModelBindingInfo() + { + ModelBinderType = modelBinderType, + UnderlyingModelType = modelType, + }; } + return null; } - private static Type GetCollectionBinder(Type modelType) + private static GenericModelBindingInfo GetCollectionBinder(Type modelType) { - return GetGenericBinderType( - typeof(ICollection<>), - typeof(List<>), - typeof(CollectionModelBinder<>), - modelType); + return GetGenericModelBindingInfo( + typeof(ICollection<>), + typeof(List<>), + typeof(CollectionModelBinder<>), + modelType); } - private static Type GetDictionaryBinder(Type modelType) + private static GenericModelBindingInfo GetDictionaryBinder(Type modelType) { - return GetGenericBinderType( - typeof(IDictionary<,>), - typeof(Dictionary<,>), - typeof(DictionaryModelBinder<,>), - modelType); + return GetGenericModelBindingInfo( + typeof(IDictionary<,>), + typeof(Dictionary<,>), + typeof(DictionaryModelBinder<,>), + modelType); } - private static Type GetKeyValuePairBinder(Type modelType) + private static GenericModelBindingInfo GetKeyValuePairBinder(Type modelType) { var modelTypeInfo = modelType.GetTypeInfo(); if (modelTypeInfo.IsGenericType && modelTypeInfo.GetGenericTypeDefinition() == typeof(KeyValuePair<,>)) { - return typeof(KeyValuePairModelBinder<,>).MakeGenericType(modelTypeInfo.GenericTypeArguments); + var modelBinderType = typeof(KeyValuePairModelBinder<,>) + .MakeGenericType(modelTypeInfo.GenericTypeArguments); + + return new GenericModelBindingInfo() + { + ModelBinderType = modelBinderType, + UnderlyingModelType = modelType, + }; } 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) + private static GenericModelBindingInfo GetGenericModelBindingInfo( + Type supportedInterfaceType, + Type newInstanceType, + Type openBinderType, + Type modelType) { Debug.Assert(supportedInterfaceType != null); Debug.Assert(openBinderType != null); @@ -108,7 +145,11 @@ private static Type GetGenericBinderType(Type supportedInterfaceType, return null; } - return openBinderType.MakeGenericType(modelTypeArguments); + return new GenericModelBindingInfo() + { + ModelBinderType = openBinderType.MakeGenericType(modelTypeArguments), + UnderlyingModelType = closedNewInstanceType, + }; } // Get the generic arguments for the binder, based on the model type. Or null if not compatible. @@ -130,5 +171,13 @@ private static Type[] GetGenericBinderTypeArgs(Type supportedInterfaceType, Type return modelTypeArguments; } + + private class GenericModelBindingInfo + { + public Type ModelBinderType { get; set; } + + // The concrete type that should be created if needed + public Type UnderlyingModelType { get; set; } + } } } diff --git a/test/Microsoft.AspNet.Mvc.Core.Test/ModelBinding/GenericModelBinderTest.cs b/test/Microsoft.AspNet.Mvc.Core.Test/ModelBinding/GenericModelBinderTest.cs new file mode 100644 index 0000000000..9e5f6f1c9d --- /dev/null +++ b/test/Microsoft.AspNet.Mvc.Core.Test/ModelBinding/GenericModelBinderTest.cs @@ -0,0 +1,149 @@ +// 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.Threading.Tasks; +using Microsoft.AspNet.Http.Internal; +using Microsoft.AspNet.Mvc.ModelBinding; +using Xunit; + +namespace Microsoft.AspNet.Mvc +{ + public class GenericModelBinderTest + { + [Fact] + public async Task GenericModelBinder_DoesNotCreateCollection_ForTopLevelModel_OnFirstPass() + { + // Arrange + var binder = new GenericModelBinder(); + + var context = CreateContext(); + context.ModelName = "param"; + + 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.Null(result.Model); + Assert.Equal("param", result.Key); + Assert.False(result.IsModelSet); + Assert.Null(result.ValidationNode); + } + + [Fact] + public async Task GenericModelBinder_CreatesEmptyCollection_ForTopLevelModel_OnFallback() + { + // Arrange + var binder = new GenericModelBinder(); + + var context = CreateContext(); + context.ModelName = string.Empty; + + 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.Empty(Assert.IsType>(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 GenericModelBinder_CreatesEmptyCollection_ForTopLevelModel_WithExplicitPrefix() + { + // Arrange + var binder = new GenericModelBinder(); + + var context = CreateContext(); + context.ModelName = "prefix"; + context.BinderModelName = "prefix"; + + 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.Empty(Assert.IsType>(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 GenericModelBinder_DoesNotCreateCollection_ForNonTopLevelModel(string prefix) + { + // Arrange + var binder = new GenericModelBinder(); + + var context = CreateContext(); + context.ModelName = ModelNames.CreatePropertyModelName(prefix, "ListProperty"); + + var metadataProvider = context.OperationBindingContext.MetadataProvider; + context.ModelMetadata = metadataProvider.GetMetadataForProperty( + typeof(ModelWithListProperty), + nameof(ModelWithListProperty.ListProperty)); + + context.ValueProvider = new TestValueProvider(new Dictionary()); + + // Act + var result = await binder.BindModelAsync(context); + + // Assert + Assert.NotNull(result); + + Assert.Null(result.Model); + Assert.Same(context.ModelName, result.Key); + Assert.False(result.IsModelSet); + Assert.Null(result.ValidationNode); + } + + private class ModelWithListProperty + { + public List ListProperty { get; set; } + } + + private static ModelBindingContext CreateContext() + { + var modelBindingContext = new ModelBindingContext() + { + OperationBindingContext = new OperationBindingContext() + { + HttpContext = new DefaultHttpContext(), + MetadataProvider = new TestModelMetadataProvider(), + } + }; + + return modelBindingContext; + } + } +} diff --git a/test/Microsoft.AspNet.Mvc.IntegrationTests/ArrayModelBinderIntegrationTest.cs b/test/Microsoft.AspNet.Mvc.IntegrationTests/ArrayModelBinderIntegrationTest.cs index 94e59a69ff..0f810fffe0 100644 --- a/test/Microsoft.AspNet.Mvc.IntegrationTests/ArrayModelBinderIntegrationTest.cs +++ b/test/Microsoft.AspNet.Mvc.IntegrationTests/ArrayModelBinderIntegrationTest.cs @@ -138,7 +138,7 @@ public async Task ArrayModelBinder_BindsArrayOfSimpleType_EmptyPrefix_Success() Assert.Equal("11", entry.Value.RawValue); } - [Fact(Skip = "Empty collection should be created by the collection model binder #1579")] + [Fact] public async Task ArrayModelBinder_BindsArrayOfSimpleType_NoData() { // Arrange @@ -160,8 +160,8 @@ public async Task ArrayModelBinder_BindsArrayOfSimpleType_NoData() var modelBindingResult = await argumentBinder.BindModelAsync(parameter, modelState, operationContext); // Assert - Assert.NotNull(modelBindingResult); // This fails due to #1579 - Assert.False(modelBindingResult.IsModelSet); + Assert.NotNull(modelBindingResult); + Assert.True(modelBindingResult.IsModelSet); Assert.Empty(Assert.IsType(modelBindingResult.Model)); Assert.Equal(0, modelState.Count); @@ -304,7 +304,7 @@ public async Task ArrayModelBinder_BindsArrayOfComplexType_EmptyPrefix_Success() Assert.Equal("lang", entry.Value.RawValue); } - [Fact(Skip = "Empty collection should be created by the collection model binder #1579")] + [Fact] public async Task ArrayModelBinder_BindsArrayOfComplexType_NoData() { // Arrange @@ -326,8 +326,8 @@ public async Task ArrayModelBinder_BindsArrayOfComplexType_NoData() var modelBindingResult = await argumentBinder.BindModelAsync(parameter, modelState, operationContext); // Assert - Assert.NotNull(modelBindingResult); // This fails due to #1579 - Assert.False(modelBindingResult.IsModelSet); + Assert.NotNull(modelBindingResult); + Assert.True(modelBindingResult.IsModelSet); Assert.Empty(Assert.IsType(modelBindingResult.Model)); Assert.Equal(0, modelState.Count); diff --git a/test/Microsoft.AspNet.Mvc.IntegrationTests/ByteArrayModelBinderIntegrationTest.cs b/test/Microsoft.AspNet.Mvc.IntegrationTests/ByteArrayModelBinderIntegrationTest.cs index 4203930dea..0087f5dbc4 100644 --- a/test/Microsoft.AspNet.Mvc.IntegrationTests/ByteArrayModelBinderIntegrationTest.cs +++ b/test/Microsoft.AspNet.Mvc.IntegrationTests/ByteArrayModelBinderIntegrationTest.cs @@ -76,7 +76,7 @@ public async Task BindProperty_WithData_GetsBound(bool fallBackScenario) Assert.Equal(ModelValidationState.Valid, modelState[key].ValidationState); // Should be skipped. bug#2447 } - [Fact] + [Fact(Skip = "ByteArrayModelBinder should return a non-null result #2456")] public async Task BindParameter_NoData_DoesNotGetBound() { // Arrange @@ -102,11 +102,15 @@ public async Task BindParameter_NoData_DoesNotGetBound() // Assert // ModelBindingResult - Assert.Null(modelBindingResult); + Assert.NotNull(modelBindingResult); // ModelState Assert.True(modelState.IsValid); Assert.Empty(modelState.Keys); + + Assert.Equal("CustomParameter", modelBindingResult.Key); + Assert.True(modelBindingResult.IsModelSet); + Assert.Equal(new byte[0], modelBindingResult.Model); } [Fact(Skip = "ModelState.Value not set due to #2445")] diff --git a/test/Microsoft.AspNet.Mvc.IntegrationTests/CollectionModelBinderIntegrationTest.cs b/test/Microsoft.AspNet.Mvc.IntegrationTests/CollectionModelBinderIntegrationTest.cs index 30a25e1dd0..5eca2f79f7 100644 --- a/test/Microsoft.AspNet.Mvc.IntegrationTests/CollectionModelBinderIntegrationTest.cs +++ b/test/Microsoft.AspNet.Mvc.IntegrationTests/CollectionModelBinderIntegrationTest.cs @@ -145,7 +145,7 @@ public async Task CollectionModelBinder_BindsCollectionOfSimpleType_EmptyPrefix_ Assert.Equal("11", entry.Value.RawValue); } - [Fact(Skip = "Empty collection should be created by the collection model binder #1579")] + [Fact] public async Task CollectionModelBinder_BindsListOfSimpleType_NoData() { // Arrange @@ -167,8 +167,8 @@ public async Task CollectionModelBinder_BindsListOfSimpleType_NoData() var modelBindingResult = await argumentBinder.BindModelAsync(parameter, modelState, operationContext); // Assert - Assert.NotNull(modelBindingResult); // This fails due to #1579 - Assert.False(modelBindingResult.IsModelSet); + Assert.NotNull(modelBindingResult); + Assert.True(modelBindingResult.IsModelSet); Assert.Empty(Assert.IsType>(modelBindingResult.Model)); Assert.Equal(0, modelState.Count); @@ -311,7 +311,7 @@ public async Task CollectionModelBinder_BindsCollectionOfComplexType_EmptyPrefix Assert.Equal("11", entry.Value.RawValue); } - [Fact(Skip = "Empty collection should be created by the collection model binder #1579")] + [Fact] public async Task CollectionModelBinder_BindsListOfComplexType_NoData() { // Arrange @@ -333,8 +333,8 @@ public async Task CollectionModelBinder_BindsListOfComplexType_NoData() var modelBindingResult = await argumentBinder.BindModelAsync(parameter, modelState, operationContext); // Assert - Assert.NotNull(modelBindingResult); // This fails due to #1579 - Assert.False(modelBindingResult.IsModelSet); + Assert.NotNull(modelBindingResult); + Assert.True(modelBindingResult.IsModelSet); Assert.Empty(Assert.IsType>(modelBindingResult.Model)); Assert.Equal(0, modelState.Count); @@ -510,7 +510,7 @@ public async Task CollectionModelBinder_BindsCollectionOfComplexType_WithRequire Assert.Equal(ModelValidationState.Invalid, entry.ValidationState); } - [Fact(Skip = "Empty collection should be created by the collection model binder #1579")] + [Fact] public async Task CollectionModelBinder_BindsListOfComplexType_WithRequiredProperty_NoData() { // Arrange @@ -532,8 +532,8 @@ public async Task CollectionModelBinder_BindsListOfComplexType_WithRequiredPrope var modelBindingResult = await argumentBinder.BindModelAsync(parameter, modelState, operationContext); // Assert - Assert.NotNull(modelBindingResult); // This fails due to #1579 - Assert.False(modelBindingResult.IsModelSet); + Assert.NotNull(modelBindingResult); + Assert.True(modelBindingResult.IsModelSet); Assert.Empty(Assert.IsType>(modelBindingResult.Model)); Assert.Equal(0, modelState.Count); diff --git a/test/Microsoft.AspNet.Mvc.IntegrationTests/DictionaryModelBinderIntegrationTest.cs b/test/Microsoft.AspNet.Mvc.IntegrationTests/DictionaryModelBinderIntegrationTest.cs index 792e0b7157..7b72180e14 100644 --- a/test/Microsoft.AspNet.Mvc.IntegrationTests/DictionaryModelBinderIntegrationTest.cs +++ b/test/Microsoft.AspNet.Mvc.IntegrationTests/DictionaryModelBinderIntegrationTest.cs @@ -140,7 +140,7 @@ public async Task DictionaryModelBinder_BindsDictionaryOfSimpleType_EmptyPrefix_ Assert.Equal("10", entry.Value.RawValue); } - [Fact(Skip = "Empty collection should be created by the collection model binder #1579")] + [Fact] public async Task DictionaryModelBinder_BindsDictionaryOfSimpleType_NoData() { // Arrange @@ -162,8 +162,8 @@ public async Task DictionaryModelBinder_BindsDictionaryOfSimpleType_NoData() var modelBindingResult = await argumentBinder.BindModelAsync(parameter, modelState, operationContext); // Assert - Assert.NotNull(modelBindingResult); // This fails due to #1579 - Assert.False(modelBindingResult.IsModelSet); + Assert.NotNull(modelBindingResult); + Assert.True(modelBindingResult.IsModelSet); Assert.Empty(Assert.IsType>(modelBindingResult.Model)); Assert.Equal(0, modelState.Count); @@ -309,7 +309,7 @@ public async Task DictionaryModelBinder_BindsDictionaryOfComplexType_EmptyPrefix Assert.Equal("10", entry.Value.RawValue); } - [Fact(Skip = "Empty collection should be created by the collection model binder #1579")] + [Fact] public async Task DictionaryModelBinder_BindsDictionaryOfComplexType_NoData() { // Arrange @@ -331,8 +331,8 @@ public async Task DictionaryModelBinder_BindsDictionaryOfComplexType_NoData() var modelBindingResult = await argumentBinder.BindModelAsync(parameter, modelState, operationContext); // Assert - Assert.NotNull(modelBindingResult); // This fails due to #1579 - Assert.False(modelBindingResult.IsModelSet); + Assert.NotNull(modelBindingResult); + Assert.True(modelBindingResult.IsModelSet); Assert.Empty(Assert.IsType>(modelBindingResult.Model)); Assert.Equal(0, modelState.Count); diff --git a/test/Microsoft.AspNet.Mvc.IntegrationTests/GenericModelBinderIntegrationTest.cs b/test/Microsoft.AspNet.Mvc.IntegrationTests/GenericModelBinderIntegrationTest.cs index 4a32af6ddc..b1a026d06e 100644 --- a/test/Microsoft.AspNet.Mvc.IntegrationTests/GenericModelBinderIntegrationTest.cs +++ b/test/Microsoft.AspNet.Mvc.IntegrationTests/GenericModelBinderIntegrationTest.cs @@ -99,7 +99,7 @@ public async Task GenericModelBinder_BindsCollection_ElementTypeFromGreedyModelB // // In this example we choose IFormCollection - because IFormCollection has a dedicated // model binder. - [Fact(Skip = "Empty collection should be created by the collection model binder #1579")] + [Fact] public async Task GenericModelBinder_BindsCollection_ElementTypeFromGreedyModelBinder_NoData() { // Arrange @@ -122,8 +122,8 @@ public async Task GenericModelBinder_BindsCollection_ElementTypeFromGreedyModelB var modelBindingResult = await argumentBinder.BindModelAsync(parameter, modelState, operationContext); // Assert - Assert.NotNull(modelBindingResult); // This fails due to #1579 - Assert.False(modelBindingResult.IsModelSet); + Assert.NotNull(modelBindingResult); + Assert.True(modelBindingResult.IsModelSet); var model = Assert.IsType>(modelBindingResult.Model); Assert.Empty(model); @@ -298,7 +298,7 @@ public async Task GenericModelBinder_BindsArrayOfDictionary_EmptyPrefix_Success( // This is part of a random sampling of scenarios where a GenericModelBinder is used // recursively. - [Fact(Skip = "Empty collection should be created by the collection model binder #1579")] + [Fact] public async Task GenericModelBinder_BindsArrayOfDictionary_NoData() { // Arrange @@ -320,8 +320,8 @@ public async Task GenericModelBinder_BindsArrayOfDictionary_NoData() var modelBindingResult = await argumentBinder.BindModelAsync(parameter, modelState, operationContext); // Assert - Assert.NotNull(modelBindingResult); // This fails due to #1579 - Assert.False(modelBindingResult.IsModelSet); + Assert.NotNull(modelBindingResult); + Assert.True(modelBindingResult.IsModelSet); var model = Assert.IsType[]>(modelBindingResult.Model); Assert.NotNull(model); @@ -424,7 +424,7 @@ public async Task GenericModelBinder_BindsCollectionOfKeyValuePair_EmptyPrefix_S // This is part of a random sampling of scenarios where a GenericModelBinder is used // recursively. - [Fact(Skip = "Empty collection should be created by the collection model binder #1579")] + [Fact] public async Task GenericModelBinder_BindsCollectionOfKeyValuePair_NoData() { // Arrange @@ -446,8 +446,8 @@ public async Task GenericModelBinder_BindsCollectionOfKeyValuePair_NoData() var modelBindingResult = await argumentBinder.BindModelAsync(parameter, modelState, operationContext); // Assert - Assert.NotNull(modelBindingResult); // Fails due to #1579 - Assert.False(modelBindingResult.IsModelSet); + Assert.NotNull(modelBindingResult); + Assert.True(modelBindingResult.IsModelSet); var model = Assert.IsType>>(modelBindingResult.Model); Assert.NotNull(model); @@ -559,7 +559,7 @@ public async Task GenericModelBinder_BindsDictionaryOfList_EmptyPrefix_Success() // This is part of a random sampling of scenarios where a GenericModelBinder is used // recursively. - [Fact(Skip = "Empty collection should be created by the collection model binder #1579")] + [Fact] public async Task GenericModelBinder_BindsDictionaryOfList_NoData() { // Arrange @@ -581,8 +581,8 @@ public async Task GenericModelBinder_BindsDictionaryOfList_NoData() var modelBindingResult = await argumentBinder.BindModelAsync(parameter, modelState, operationContext); // Assert - Assert.NotNull(modelBindingResult); // Fails due to #1579 - Assert.False(modelBindingResult.IsModelSet); + Assert.NotNull(modelBindingResult); + Assert.True(modelBindingResult.IsModelSet); var model = Assert.IsType>>(modelBindingResult.Model); Assert.NotNull(model); diff --git a/test/Microsoft.AspNet.Mvc.IntegrationTests/KeyValuePairModelBinderIntegrationTest.cs b/test/Microsoft.AspNet.Mvc.IntegrationTests/KeyValuePairModelBinderIntegrationTest.cs index 230a1b34c8..58071797f7 100644 --- a/test/Microsoft.AspNet.Mvc.IntegrationTests/KeyValuePairModelBinderIntegrationTest.cs +++ b/test/Microsoft.AspNet.Mvc.IntegrationTests/KeyValuePairModelBinderIntegrationTest.cs @@ -139,7 +139,7 @@ public async Task KeyValuePairModelBinder_BindsKeyValuePairOfSimpleType_EmptyPre Assert.Equal("10", entry.Value.RawValue); } - [Fact(Skip = "Empty collection should be created by the collection model binder #1579")] + [Fact] public async Task KeyValuePairModelBinder_BindsKeyValuePairOfSimpleType_NoData() { // Arrange @@ -161,10 +161,10 @@ public async Task KeyValuePairModelBinder_BindsKeyValuePairOfSimpleType_NoData() var modelBindingResult = await argumentBinder.BindModelAsync(parameter, modelState, operationContext); // Assert - Assert.NotNull(modelBindingResult); // This fails due to #1579 - Assert.False(modelBindingResult.IsModelSet); + Assert.NotNull(modelBindingResult); + Assert.True(modelBindingResult.IsModelSet); - Assert.Equal(new int[0], modelBindingResult.Model); + Assert.Equal(new KeyValuePair(), modelBindingResult.Model); Assert.Equal(0, modelState.Count); Assert.Equal(0, modelState.ErrorCount); @@ -306,7 +306,7 @@ public async Task KeyValuePairModelBinder_BindsKeyValuePairOfComplexType_EmptyPr Assert.Equal("10", entry.Value.RawValue); } - [Fact(Skip = "Empty collection should be created by the collection model binder #1579")] + [Fact] public async Task KeyValuePairModelBinder_BindsKeyValuePairOfComplexType_NoData() { // Arrange @@ -328,8 +328,8 @@ public async Task KeyValuePairModelBinder_BindsKeyValuePairOfComplexType_NoData( var modelBindingResult = await argumentBinder.BindModelAsync(parameter, modelState, operationContext); // Assert - Assert.NotNull(modelBindingResult); // This fails due to #1579 - Assert.False(modelBindingResult.IsModelSet); + Assert.NotNull(modelBindingResult); + Assert.True(modelBindingResult.IsModelSet); Assert.Equal(new KeyValuePair(), modelBindingResult.Model); diff --git a/test/Microsoft.AspNet.Mvc.IntegrationTests/ValidationIntegrationTests.cs b/test/Microsoft.AspNet.Mvc.IntegrationTests/ValidationIntegrationTests.cs index 6b832ca281..dfccd8d93b 100644 --- a/test/Microsoft.AspNet.Mvc.IntegrationTests/ValidationIntegrationTests.cs +++ b/test/Microsoft.AspNet.Mvc.IntegrationTests/ValidationIntegrationTests.cs @@ -974,7 +974,7 @@ public async Task Validation_StringLengthAttribute_OnProperyOfCollectionElement_ Assert.Null(error.Exception); } - [Fact(Skip = "Empty collection should be created by the collection model binder #1579")] + [Fact] public async Task Validation_StringLengthAttribute_OnProperyOfCollectionElement_NoData() { // Arrange @@ -997,14 +997,14 @@ public async Task Validation_StringLengthAttribute_OnProperyOfCollectionElement_ // Assert Assert.NotNull(modelBindingResult); - Assert.False(modelBindingResult.IsModelSet); + Assert.True(modelBindingResult.IsModelSet); var model = Assert.IsType>(modelBindingResult.Model); Assert.Empty(model); Assert.Equal(0, modelState.Count); Assert.Equal(0, modelState.ErrorCount); - Assert.False(modelState.IsValid); + Assert.True(modelState.IsValid); } private class Order11 From cbadc7555a784f6f3b7ed1fca9310a9afd075ca3 Mon Sep 17 00:00:00 2001 From: Ryan Nowak Date: Thu, 21 May 2015 00:56:08 -0700 Subject: [PATCH 2/3] cr feedback --- .../ModelBinding/GenericModelBinder.cs | 43 +++++++++++-------- 1 file changed, 26 insertions(+), 17 deletions(-) diff --git a/src/Microsoft.AspNet.Mvc.Core/ModelBinding/GenericModelBinder.cs b/src/Microsoft.AspNet.Mvc.Core/ModelBinding/GenericModelBinder.cs index 2c7599283e..57372768bb 100644 --- a/src/Microsoft.AspNet.Mvc.Core/ModelBinding/GenericModelBinder.cs +++ b/src/Microsoft.AspNet.Mvc.Core/ModelBinding/GenericModelBinder.cs @@ -21,8 +21,8 @@ public async Task BindModelAsync(ModelBindingContext binding if (result != null && result.IsModelSet) { - // Success - propegate the values returned by the model binder. - return new ModelBindingResult(result.Model, result.Key, result.IsModelSet, result.ValidationNode); + // Success - propagate the values returned by the model binder. + return result; } // If this is the fallback case, and we didn't bind as a top-level model, then generate a @@ -32,13 +32,12 @@ public async Task BindModelAsync(ModelBindingContext binding if (isTopLevelObject && (hasExplicitAlias || bindingContext.ModelName == string.Empty)) { - - var model = result?.Model; - if (model == null && bindingInfo.UnderlyingModelType.IsArray) + object model; + if (bindingInfo.UnderlyingModelType.IsArray) { model = Array.CreateInstance(bindingInfo.UnderlyingModelType.GetElementType(), 0); } - else if (model == null) + else { model = Activator.CreateInstance(bindingInfo.UnderlyingModelType); } @@ -51,9 +50,8 @@ public async Task BindModelAsync(ModelBindingContext binding return new ModelBindingResult(model, bindingContext.ModelName, true, validationNode); } - // We always want to return a result for model-types that we handle. - // - // Always tell the model binding system to skip other model binders i.e. return non-null. + // We always want to return a result for model types that we handle; tell the model binding + // system to skip other model binders by returning non-null. return new ModelBindingResult(model: null, key: bindingContext.ModelName, isModelSet: false); } @@ -62,13 +60,13 @@ public async Task BindModelAsync(ModelBindingContext binding private static GenericModelBindingInfo ResolveGenericBindingInfo(Type modelType) { - return GetArrayBinder(modelType) ?? - GetCollectionBinder(modelType) ?? - GetDictionaryBinder(modelType) ?? - GetKeyValuePairBinder(modelType); + return GetArrayBinderInfo(modelType) ?? + GetCollectionBinderInfo(modelType) ?? + GetDictionaryBinderInfo(modelType) ?? + GetKeyValuePairBinderInfo(modelType); } - private static GenericModelBindingInfo GetArrayBinder(Type modelType) + private static GenericModelBindingInfo GetArrayBinderInfo(Type modelType) { if (modelType.IsArray) { @@ -85,7 +83,7 @@ private static GenericModelBindingInfo GetArrayBinder(Type modelType) return null; } - private static GenericModelBindingInfo GetCollectionBinder(Type modelType) + private static GenericModelBindingInfo GetCollectionBinderInfo(Type modelType) { return GetGenericModelBindingInfo( typeof(ICollection<>), @@ -94,7 +92,7 @@ private static GenericModelBindingInfo GetCollectionBinder(Type modelType) modelType); } - private static GenericModelBindingInfo GetDictionaryBinder(Type modelType) + private static GenericModelBindingInfo GetDictionaryBinderInfo(Type modelType) { return GetGenericModelBindingInfo( typeof(IDictionary<,>), @@ -103,7 +101,7 @@ private static GenericModelBindingInfo GetDictionaryBinder(Type modelType) modelType); } - private static GenericModelBindingInfo GetKeyValuePairBinder(Type modelType) + private static GenericModelBindingInfo GetKeyValuePairBinderInfo(Type modelType) { var modelTypeInfo = modelType.GetTypeInfo(); if (modelTypeInfo.IsGenericType && @@ -122,6 +120,17 @@ private static GenericModelBindingInfo GetKeyValuePairBinder(Type modelType) return null; } + // + // Example: + // GetGenericBinderType(typeof(IList), typeof(List), typeof(ListBinder), ...) + // + // This means that the ListBinder type can work with models that implement IList, and if there is no + // existing model instance, the binder will create a List{T}. + // + // This method will return null if the given model type isn't compatible with the combination of + // supportedInterfaceType and modelType. If supportedInterfaceType and modelType are compatible, then + // it will return the closed-generic newInstanceType and closed-generic openBinderType. + // private static GenericModelBindingInfo GetGenericModelBindingInfo( Type supportedInterfaceType, Type newInstanceType, From 9f23ad7cbe38c47e40838761a481f81affe5cb3c Mon Sep 17 00:00:00 2001 From: Ryan Nowak Date: Thu, 21 May 2015 21:02:40 -0700 Subject: [PATCH 3/3] CR feedback --- .../ModelBinding/ArrayModelBinder.cs | 5 + .../ModelBinding/CollectionModelBinder.cs | 33 +++- .../ModelBinding/DictionaryModelBinder.cs | 5 + .../ModelBinding/GenericModelBinder.cs | 144 ++++++------------ .../ModelBinding/KeyValuePairModelBinder.cs | 23 ++- .../ModelBinding/ArrayModelBinderTest.cs | 120 ++++++++++++++- .../ModelBinding/CollectionModelBinderTest.cs | 125 +++++++++++++++ .../ModelBinding/DictionaryModelBinderTest.cs | 125 +++++++++++++++ .../ModelBinding/GenericModelBinderTest.cs | 132 ---------------- .../KeyValuePairModelBinderTest.cs | 126 +++++++++++++++ 10 files changed, 596 insertions(+), 242 deletions(-) diff --git a/src/Microsoft.AspNet.Mvc.Core/ModelBinding/ArrayModelBinder.cs b/src/Microsoft.AspNet.Mvc.Core/ModelBinding/ArrayModelBinder.cs index 2d68d0e5a2..f7df7ae430 100644 --- 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 BindModelAsync([NotNull] ModelBindingCo return base.BindModelAsync(bindingContext); } + protected override object CreateEmptyCollection() + { + return new TElement[0]; + } + /// protected override object GetModel(IEnumerable newCollection) { diff --git a/src/Microsoft.AspNet.Mvc.Core/ModelBinding/CollectionModelBinder.cs b/src/Microsoft.AspNet.Mvc.Core/ModelBinding/CollectionModelBinder.cs index 040f389dd4..fe4ffca3ac 100644 --- 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 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 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 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(); + } + // Used when the ValueProvider contains the collection to be bound as a single element, e.g. the raw value // is [ "1", "2" ] and needs to be converted to an int[]. internal async Task BindSimpleCollection( @@ -165,8 +194,6 @@ internal async Task 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 index f8bafd1532..24384b1ec2 100644 --- 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> newCo { return newCollection?.ToDictionary(kvp => kvp.Key, kvp => kvp.Value); } + + protected override object CreateEmptyCollection() + { + return new Dictionary(); + } } } diff --git a/src/Microsoft.AspNet.Mvc.Core/ModelBinding/GenericModelBinder.cs b/src/Microsoft.AspNet.Mvc.Core/ModelBinding/GenericModelBinder.cs index 57372768bb..2997e7f8b1 100644 --- a/src/Microsoft.AspNet.Mvc.Core/ModelBinding/GenericModelBinder.cs +++ b/src/Microsoft.AspNet.Mvc.Core/ModelBinding/GenericModelBinder.cs @@ -13,129 +13,83 @@ public class GenericModelBinder : IModelBinder { public async Task BindModelAsync(ModelBindingContext bindingContext) { - var bindingInfo = ResolveGenericBindingInfo(bindingContext.ModelType); - if (bindingInfo != null) + var binderType = ResolveBinderType(bindingContext.ModelType); + if (binderType != null) { - var binder = (IModelBinder)Activator.CreateInstance(bindingInfo.ModelBinderType); + var binder = (IModelBinder)Activator.CreateInstance(binderType); var result = await binder.BindModelAsync(bindingContext); - if (result != null && result.IsModelSet) - { - // Success - propagate the values returned by the model binder. - return result; - } - - // If this is the fallback case, and we didn't bind 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)) - { - object model; - if (bindingInfo.UnderlyingModelType.IsArray) - { - model = Array.CreateInstance(bindingInfo.UnderlyingModelType.GetElementType(), 0); - } - else - { - model = Activator.CreateInstance(bindingInfo.UnderlyingModelType); - } - - var validationNode = new ModelValidationNode( - bindingContext.ModelName, - bindingContext.ModelMetadata, - model); - - return new ModelBindingResult(model, bindingContext.ModelName, true, validationNode); - } - - // We always want to return a result for model types that we handle; tell the model binding - // system to skip other model binders by returning non-null. - return new ModelBindingResult(model: null, key: bindingContext.ModelName, isModelSet: false); + var modelBindingResult = result != null ? + new ModelBindingResult(result.Model, result.Key, result.IsModelSet, result.ValidationNode) : + new ModelBindingResult(model: null, key: bindingContext.ModelName, isModelSet: false); + + // Were able to resolve a binder type. + // Always tell the model binding system to skip other model binders i.e. return non-null. + return modelBindingResult; } return null; } - private static GenericModelBindingInfo ResolveGenericBindingInfo(Type modelType) + private static Type ResolveBinderType(Type modelType) { - return GetArrayBinderInfo(modelType) ?? - GetCollectionBinderInfo(modelType) ?? - GetDictionaryBinderInfo(modelType) ?? - GetKeyValuePairBinderInfo(modelType); + return GetArrayBinder(modelType) ?? + GetCollectionBinder(modelType) ?? + GetDictionaryBinder(modelType) ?? + GetKeyValuePairBinder(modelType); } - private static GenericModelBindingInfo GetArrayBinderInfo(Type modelType) + private static Type GetArrayBinder(Type modelType) { if (modelType.IsArray) { var elementType = modelType.GetElementType(); - var modelBinderType = typeof(ArrayModelBinder<>).MakeGenericType(elementType); - - return new GenericModelBindingInfo() - { - ModelBinderType = modelBinderType, - UnderlyingModelType = modelType, - }; + return typeof(ArrayModelBinder<>).MakeGenericType(elementType); } - return null; } - private static GenericModelBindingInfo GetCollectionBinderInfo(Type modelType) + private static Type GetCollectionBinder(Type modelType) { - return GetGenericModelBindingInfo( - typeof(ICollection<>), - typeof(List<>), - typeof(CollectionModelBinder<>), - modelType); + return GetGenericBinderType( + typeof(ICollection<>), + typeof(List<>), + typeof(CollectionModelBinder<>), + modelType); } - private static GenericModelBindingInfo GetDictionaryBinderInfo(Type modelType) + private static Type GetDictionaryBinder(Type modelType) { - return GetGenericModelBindingInfo( - typeof(IDictionary<,>), - typeof(Dictionary<,>), - typeof(DictionaryModelBinder<,>), - modelType); + return GetGenericBinderType( + typeof(IDictionary<,>), + typeof(Dictionary<,>), + typeof(DictionaryModelBinder<,>), + modelType); } - private static GenericModelBindingInfo GetKeyValuePairBinderInfo(Type modelType) + private static Type GetKeyValuePairBinder(Type modelType) { var modelTypeInfo = modelType.GetTypeInfo(); if (modelTypeInfo.IsGenericType && modelTypeInfo.GetGenericTypeDefinition() == typeof(KeyValuePair<,>)) { - var modelBinderType = typeof(KeyValuePairModelBinder<,>) - .MakeGenericType(modelTypeInfo.GenericTypeArguments); - - return new GenericModelBindingInfo() - { - ModelBinderType = modelBinderType, - UnderlyingModelType = modelType, - }; + return typeof(KeyValuePairModelBinder<,>).MakeGenericType(modelTypeInfo.GenericTypeArguments); } return null; } - // - // Example: - // GetGenericBinderType(typeof(IList), typeof(List), typeof(ListBinder), ...) - // - // This means that the ListBinder type can work with models that implement IList, and if there is no - // existing model instance, the binder will create a List{T}. - // - // This method will return null if the given model type isn't compatible with the combination of - // supportedInterfaceType and modelType. If supportedInterfaceType and modelType are compatible, then - // it will return the closed-generic newInstanceType and closed-generic openBinderType. - // - private static GenericModelBindingInfo GetGenericModelBindingInfo( - Type supportedInterfaceType, - Type newInstanceType, - Type openBinderType, - Type modelType) + /// + /// 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) { Debug.Assert(supportedInterfaceType != null); Debug.Assert(openBinderType != null); @@ -154,11 +108,7 @@ private static GenericModelBindingInfo GetGenericModelBindingInfo( return null; } - return new GenericModelBindingInfo() - { - ModelBinderType = openBinderType.MakeGenericType(modelTypeArguments), - UnderlyingModelType = closedNewInstanceType, - }; + return openBinderType.MakeGenericType(modelTypeArguments); } // Get the generic arguments for the binder, based on the model type. Or null if not compatible. @@ -180,13 +130,5 @@ private static Type[] GetGenericBinderTypeArgs(Type supportedInterfaceType, Type return modelTypeArguments; } - - private class GenericModelBindingInfo - { - public Type ModelBinderType { get; set; } - - // The concrete type that should be created if needed - public Type UnderlyingModelType { get; set; } - } } } diff --git a/src/Microsoft.AspNet.Mvc.Core/ModelBinding/KeyValuePairModelBinder.cs b/src/Microsoft.AspNet.Mvc.Core/ModelBinding/KeyValuePairModelBinder.cs index 12e285e6b4..62ebdf047f 100644 --- 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 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(); + + 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 index 5161ec3893..86c7752e51 100644 --- 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(); + var binder = new ArrayModelBinder(); + + var context = CreateContext(); + context.ModelName = "param"; + + var metadataProvider = context.OperationBindingContext.MetadataProvider; + context.ModelMetadata = metadataProvider.GetMetadataForType(typeof(string[])); + + context.ValueProvider = new TestValueProvider(new Dictionary()); // 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(); + + var context = CreateContext(); + context.ModelName = string.Empty; + + var metadataProvider = context.OperationBindingContext.MetadataProvider; + context.ModelMetadata = metadataProvider.GetMetadataForType(typeof(string[])); + + context.ValueProvider = new TestValueProvider(new Dictionary()); + + // Act + var result = await binder.BindModelAsync(context); + + // Assert + Assert.NotNull(result); + + Assert.Empty(Assert.IsType(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(); + + 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()); + + // Act + var result = await binder.BindModelAsync(context); + + // Assert + Assert.NotNull(result); + + Assert.Empty(Assert.IsType(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(); + + 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()); + + // 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 diff --git a/test/Microsoft.AspNet.Mvc.Core.Test/ModelBinding/CollectionModelBinderTest.cs b/test/Microsoft.AspNet.Mvc.Core.Test/ModelBinding/CollectionModelBinderTest.cs index c0f4af2331..dbde65ee76 100644 --- a/test/Microsoft.AspNet.Mvc.Core.Test/ModelBinding/CollectionModelBinderTest.cs +++ b/test/Microsoft.AspNet.Mvc.Core.Test/ModelBinding/CollectionModelBinderTest.cs @@ -7,6 +7,7 @@ using System.Linq; #endif using System.Threading.Tasks; +using Microsoft.AspNet.Http.Internal; #if DNX451 using Moq; #endif @@ -216,6 +217,111 @@ public async Task BindSimpleCollection_RawValueIsNull_ReturnsNull() Assert.Null(boundCollection); } + [Fact] + public async Task CollectionModelBinder_DoesNotCreateCollection_ForTopLevelModel_OnFirstPass() + { + // Arrange + var binder = new CollectionModelBinder(); + + var context = CreateContext(); + context.ModelName = "param"; + + 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.Null(result); + } + + [Fact] + public async Task CollectionModelBinder_CreatesEmptyCollection_ForTopLevelModel_OnFallback() + { + // Arrange + var binder = new CollectionModelBinder(); + + var context = CreateContext(); + context.ModelName = string.Empty; + + 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.Empty(Assert.IsType>(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 CollectionModelBinder_CreatesEmptyCollection_ForTopLevelModel_WithExplicitPrefix() + { + // Arrange + var binder = new CollectionModelBinder(); + + var context = CreateContext(); + context.ModelName = "prefix"; + context.BinderModelName = "prefix"; + + 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.Empty(Assert.IsType>(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 CollectionModelBinder_DoesNotCreateCollection_ForNonTopLevelModel(string prefix) + { + // Arrange + var binder = new CollectionModelBinder(); + + var context = CreateContext(); + context.ModelName = ModelNames.CreatePropertyModelName(prefix, "ListProperty"); + + var metadataProvider = context.OperationBindingContext.MetadataProvider; + context.ModelMetadata = metadataProvider.GetMetadataForProperty( + typeof(ModelWithListProperty), + nameof(ModelWithListProperty.ListProperty)); + + context.ValueProvider = new TestValueProvider(new Dictionary()); + + // Act + var result = await binder.BindModelAsync(context); + + // Assert + Assert.Null(result); + } + #if DNX451 [Fact] public async Task BindSimpleCollection_SubBindingSucceeds() @@ -284,5 +390,24 @@ private static IModelBinder CreateIntBinder() return mockIntBinder.Object; } #endif + + private static ModelBindingContext CreateContext() + { + var modelBindingContext = new ModelBindingContext() + { + OperationBindingContext = new OperationBindingContext() + { + HttpContext = new DefaultHttpContext(), + MetadataProvider = new TestModelMetadataProvider(), + } + }; + + return modelBindingContext; + } + + private class ModelWithListProperty + { + public List ListProperty { get; set; } + } } } diff --git a/test/Microsoft.AspNet.Mvc.Core.Test/ModelBinding/DictionaryModelBinderTest.cs b/test/Microsoft.AspNet.Mvc.Core.Test/ModelBinding/DictionaryModelBinderTest.cs index 34b0318aea..4ad8d02f32 100644 --- a/test/Microsoft.AspNet.Mvc.Core.Test/ModelBinding/DictionaryModelBinderTest.cs +++ b/test/Microsoft.AspNet.Mvc.Core.Test/ModelBinding/DictionaryModelBinderTest.cs @@ -4,6 +4,7 @@ #if DNX451 using System.Collections.Generic; using System.Threading.Tasks; +using Microsoft.AspNet.Http.Internal; using Moq; using Xunit; @@ -63,6 +64,125 @@ public async Task BindModel_BindingContextModelNonNull_Succeeds(bool isReadOnly) Assert.Equal("eighty-four", dictionary[84]); } + [Fact] + public async Task DictionaryModelBinder_DoesNotCreateCollection_ForTopLevelModel_OnFirstPass() + { + // Arrange + var binder = new DictionaryModelBinder(); + + var context = CreateContext(); + context.ModelName = "param"; + + var metadataProvider = context.OperationBindingContext.MetadataProvider; + context.ModelMetadata = metadataProvider.GetMetadataForType(typeof(Dictionary)); + + context.ValueProvider = new TestValueProvider(new Dictionary()); + + // Act + var result = await binder.BindModelAsync(context); + + // Assert + Assert.Null(result); + } + + [Fact] + public async Task DictionaryModelBinder_CreatesEmptyCollection_ForTopLevelModel_OnFallback() + { + // Arrange + var binder = new DictionaryModelBinder(); + + var context = CreateContext(); + context.ModelName = string.Empty; + + var metadataProvider = context.OperationBindingContext.MetadataProvider; + context.ModelMetadata = metadataProvider.GetMetadataForType(typeof(Dictionary)); + + context.ValueProvider = new TestValueProvider(new Dictionary()); + + // Act + var result = await binder.BindModelAsync(context); + + // Assert + Assert.NotNull(result); + + Assert.Empty(Assert.IsType>(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 DictionaryModelBinder_CreatesEmptyCollection_ForTopLevelModel_WithExplicitPrefix() + { + // Arrange + var binder = new DictionaryModelBinder(); + + var context = CreateContext(); + context.ModelName = "prefix"; + context.BinderModelName = "prefix"; + + var metadataProvider = context.OperationBindingContext.MetadataProvider; + context.ModelMetadata = metadataProvider.GetMetadataForType(typeof(Dictionary)); + + context.ValueProvider = new TestValueProvider(new Dictionary()); + + // Act + var result = await binder.BindModelAsync(context); + + // Assert + Assert.NotNull(result); + + Assert.Empty(Assert.IsType>(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 DictionaryModelBinder_DoesNotCreateCollection_ForNonTopLevelModel(string prefix) + { + // Arrange + var binder = new DictionaryModelBinder(); + + var context = CreateContext(); + context.ModelName = ModelNames.CreatePropertyModelName(prefix, "ListProperty"); + + var metadataProvider = context.OperationBindingContext.MetadataProvider; + context.ModelMetadata = metadataProvider.GetMetadataForProperty( + typeof(ModelWithDictionaryProperty), + nameof(ModelWithDictionaryProperty.DictionaryProperty)); + + context.ValueProvider = new TestValueProvider(new Dictionary()); + + // Act + var result = await binder.BindModelAsync(context); + + // Assert + Assert.Null(result); + } + + private static ModelBindingContext CreateContext() + { + var modelBindingContext = new ModelBindingContext() + { + OperationBindingContext = new OperationBindingContext() + { + HttpContext = new DefaultHttpContext(), + MetadataProvider = new TestModelMetadataProvider(), + } + }; + + return modelBindingContext; + } + private static ModelBindingContext GetModelBindingContext(bool isReadOnly) { var metadataProvider = new TestModelMetadataProvider(); @@ -105,6 +225,11 @@ private static IModelBinder CreateKvpBinder() }); return mockKvpBinder.Object; } + + private class ModelWithDictionaryProperty + { + public Dictionary DictionaryProperty { get; set; } + } } } #endif diff --git a/test/Microsoft.AspNet.Mvc.Core.Test/ModelBinding/GenericModelBinderTest.cs b/test/Microsoft.AspNet.Mvc.Core.Test/ModelBinding/GenericModelBinderTest.cs index 9e5f6f1c9d..be672dfd8b 100644 --- a/test/Microsoft.AspNet.Mvc.Core.Test/ModelBinding/GenericModelBinderTest.cs +++ b/test/Microsoft.AspNet.Mvc.Core.Test/ModelBinding/GenericModelBinderTest.cs @@ -12,138 +12,6 @@ namespace Microsoft.AspNet.Mvc { public class GenericModelBinderTest { - [Fact] - public async Task GenericModelBinder_DoesNotCreateCollection_ForTopLevelModel_OnFirstPass() - { - // Arrange - var binder = new GenericModelBinder(); - var context = CreateContext(); - context.ModelName = "param"; - - 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.Null(result.Model); - Assert.Equal("param", result.Key); - Assert.False(result.IsModelSet); - Assert.Null(result.ValidationNode); - } - - [Fact] - public async Task GenericModelBinder_CreatesEmptyCollection_ForTopLevelModel_OnFallback() - { - // Arrange - var binder = new GenericModelBinder(); - - var context = CreateContext(); - context.ModelName = string.Empty; - - 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.Empty(Assert.IsType>(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 GenericModelBinder_CreatesEmptyCollection_ForTopLevelModel_WithExplicitPrefix() - { - // Arrange - var binder = new GenericModelBinder(); - - var context = CreateContext(); - context.ModelName = "prefix"; - context.BinderModelName = "prefix"; - - 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.Empty(Assert.IsType>(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 GenericModelBinder_DoesNotCreateCollection_ForNonTopLevelModel(string prefix) - { - // Arrange - var binder = new GenericModelBinder(); - - var context = CreateContext(); - context.ModelName = ModelNames.CreatePropertyModelName(prefix, "ListProperty"); - - var metadataProvider = context.OperationBindingContext.MetadataProvider; - context.ModelMetadata = metadataProvider.GetMetadataForProperty( - typeof(ModelWithListProperty), - nameof(ModelWithListProperty.ListProperty)); - - context.ValueProvider = new TestValueProvider(new Dictionary()); - - // Act - var result = await binder.BindModelAsync(context); - - // Assert - Assert.NotNull(result); - - Assert.Null(result.Model); - Assert.Same(context.ModelName, result.Key); - Assert.False(result.IsModelSet); - Assert.Null(result.ValidationNode); - } - - private class ModelWithListProperty - { - public List ListProperty { get; set; } - } - - private static ModelBindingContext CreateContext() - { - var modelBindingContext = new ModelBindingContext() - { - OperationBindingContext = new OperationBindingContext() - { - HttpContext = new DefaultHttpContext(), - MetadataProvider = new TestModelMetadataProvider(), - } - }; - - return modelBindingContext; - } } } diff --git a/test/Microsoft.AspNet.Mvc.Core.Test/ModelBinding/KeyValuePairModelBinderTest.cs b/test/Microsoft.AspNet.Mvc.Core.Test/ModelBinding/KeyValuePairModelBinderTest.cs index 1b9b463e3e..257eda9b21 100644 --- a/test/Microsoft.AspNet.Mvc.Core.Test/ModelBinding/KeyValuePairModelBinderTest.cs +++ b/test/Microsoft.AspNet.Mvc.Core.Test/ModelBinding/KeyValuePairModelBinderTest.cs @@ -6,6 +6,7 @@ using System.Collections.Generic; using System.Linq; using System.Threading.Tasks; +using Microsoft.AspNet.Http.Internal; using Microsoft.AspNet.Mvc.ModelBinding.Validation; using Moq; using Xunit; @@ -158,6 +159,126 @@ public async Task TryBindStrongModel_BinderExists_BinderReturnsIncorrectlyTypedO Assert.Empty(bindingContext.ModelState); } + [Fact] + public async Task KeyValuePairModelBinder_DoesNotCreateCollection_ForTopLevelModel_OnFirstPass() + { + // Arrange + var binder = new KeyValuePairModelBinder(); + + var context = CreateContext(); + context.ModelName = "param"; + + var metadataProvider = context.OperationBindingContext.MetadataProvider; + context.ModelMetadata = metadataProvider.GetMetadataForType(typeof(KeyValuePair)); + + context.ValueProvider = new TestValueProvider(new Dictionary()); + + // Act + var result = await binder.BindModelAsync(context); + + // Assert + Assert.Null(result); + } + + [Fact] + public async Task KeyValuePairModelBinder_CreatesEmptyCollection_ForTopLevelModel_OnFallback() + { + // Arrange + var binder = new KeyValuePairModelBinder(); + + var context = CreateContext(); + context.ModelName = string.Empty; + + var metadataProvider = context.OperationBindingContext.MetadataProvider; + context.ModelMetadata = metadataProvider.GetMetadataForType(typeof(KeyValuePair)); + + context.ValueProvider = new TestValueProvider(new Dictionary()); + + // Act + var result = await binder.BindModelAsync(context); + + // Assert + Assert.NotNull(result); + + Assert.Equal(default(KeyValuePair), Assert.IsType>(result.Model)); + Assert.Equal(string.Empty, result.Key); + Assert.True(result.IsModelSet); + + Assert.Equal(result.ValidationNode.Model, result.Model); + Assert.Same(result.ValidationNode.Key, result.Key); + Assert.Same(result.ValidationNode.ModelMetadata, context.ModelMetadata); + } + + [Fact] + public async Task KeyValuePairModelBinder_CreatesEmptyCollection_ForTopLevelModel_WithExplicitPrefix() + { + // Arrange + var binder = new KeyValuePairModelBinder(); + + var context = CreateContext(); + context.ModelName = "prefix"; + context.BinderModelName = "prefix"; + + var metadataProvider = context.OperationBindingContext.MetadataProvider; + context.ModelMetadata = metadataProvider.GetMetadataForType(typeof(KeyValuePair)); + + context.ValueProvider = new TestValueProvider(new Dictionary()); + + // Act + var result = await binder.BindModelAsync(context); + + // Assert + Assert.NotNull(result); + + Assert.Equal(default(KeyValuePair), Assert.IsType>(result.Model)); + Assert.Equal("prefix", result.Key); + Assert.True(result.IsModelSet); + + Assert.Equal(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 KeyValuePairModelBinder_DoesNotCreateCollection_ForNonTopLevelModel(string prefix) + { + // Arrange + var binder = new KeyValuePairModelBinder(); + + var context = CreateContext(); + context.ModelName = ModelNames.CreatePropertyModelName(prefix, "KeyValuePairProperty"); + + var metadataProvider = context.OperationBindingContext.MetadataProvider; + context.ModelMetadata = metadataProvider.GetMetadataForProperty( + typeof(ModelWithKeyValuePairProperty), + nameof(ModelWithKeyValuePairProperty.KeyValuePairProperty)); + + context.ValueProvider = new TestValueProvider(new Dictionary()); + + // Act + var result = await binder.BindModelAsync(context); + + // Assert + Assert.Null(result); + } + + private static ModelBindingContext CreateContext() + { + var modelBindingContext = new ModelBindingContext() + { + OperationBindingContext = new OperationBindingContext() + { + HttpContext = new DefaultHttpContext(), + MetadataProvider = new TestModelMetadataProvider(), + ModelBinder = new TypeMatchModelBinder(), + } + }; + + return modelBindingContext; + } + private static ModelBindingContext GetBindingContext( IValueProvider valueProvider, IModelBinder innerBinder = null, @@ -210,6 +331,11 @@ private static IModelBinder CreateStringBinder() }); return mockStringBinder.Object; } + + private class ModelWithKeyValuePairProperty + { + public KeyValuePair KeyValuePairProperty { get; set; } + } } } #endif