diff --git a/src/Microsoft.AspNet.Mvc.Abstractions/ModelBinding/ModelMetadata.cs b/src/Microsoft.AspNet.Mvc.Abstractions/ModelBinding/ModelMetadata.cs index 6eeb98510b..6ab109e863 100644 --- a/src/Microsoft.AspNet.Mvc.Abstractions/ModelBinding/ModelMetadata.cs +++ b/src/Microsoft.AspNet.Mvc.Abstractions/ModelBinding/ModelMetadata.cs @@ -132,7 +132,7 @@ namespace Microsoft.AspNet.Mvc.ModelBinding /// for T if implements /// . for object if /// implements but not . null otherwise i.e. when - /// is false. + /// is false. /// public abstract ModelMetadata ElementMetadata { get; } @@ -322,10 +322,28 @@ namespace Microsoft.AspNet.Mvc.ModelBinding /// Gets a value indicating whether or not is a collection type. /// /// - /// A collection type is defined as a which is assignable to - /// , and is not a . + /// A collection type is defined as a which is assignable to . /// public bool IsCollectionType + { + get + { + // Ignore non-generic ICollection type. Would require an additional check and that interface is not + // used in MVC. + var collectionType = ClosedGenericMatcher.ExtractGenericInterface(ModelType, typeof(ICollection<>)); + + return collectionType != null; + } + } + + /// + /// Gets a value indicating whether or not is an enumerable type. + /// + /// + /// An enumerable type is defined as a which is assignable to + /// , and is not a . + /// + public bool IsEnumerableType { get { diff --git a/src/Microsoft.AspNet.Mvc.Abstractions/project.json b/src/Microsoft.AspNet.Mvc.Abstractions/project.json index 8fb6eeb58f..f1d0a721ef 100644 --- a/src/Microsoft.AspNet.Mvc.Abstractions/project.json +++ b/src/Microsoft.AspNet.Mvc.Abstractions/project.json @@ -10,6 +10,10 @@ }, "dependencies": { "Microsoft.AspNet.Routing": "1.0.0-*", + "Microsoft.Framework.ClosedGenericMatcher.Sources": { + "version": "1.0.0-*", + "type": "build" + }, "Microsoft.Framework.CopyOnWriteDictionary.Sources": { "version": "1.0.0-*", "type": "build" diff --git a/src/Microsoft.AspNet.Mvc.ApiExplorer/DefaultApiDescriptionProvider.cs b/src/Microsoft.AspNet.Mvc.ApiExplorer/DefaultApiDescriptionProvider.cs index 45395cc25c..dcb03587bf 100644 --- a/src/Microsoft.AspNet.Mvc.ApiExplorer/DefaultApiDescriptionProvider.cs +++ b/src/Microsoft.AspNet.Mvc.ApiExplorer/DefaultApiDescriptionProvider.cs @@ -544,7 +544,7 @@ namespace Microsoft.AspNet.Mvc.ApiExplorer // // 3) Types with no properties. Obviously nothing to explore there. // - if (modelMetadata.IsCollectionType || + if (modelMetadata.IsEnumerableType || !modelMetadata.IsComplexType || !modelMetadata.Properties.Any()) { diff --git a/src/Microsoft.AspNet.Mvc.Core/Controllers/DefaultControllerActionArgumentBinder.cs b/src/Microsoft.AspNet.Mvc.Core/Controllers/DefaultControllerActionArgumentBinder.cs index 4449f19901..729f1a1780 100644 --- a/src/Microsoft.AspNet.Mvc.Core/Controllers/DefaultControllerActionArgumentBinder.cs +++ b/src/Microsoft.AspNet.Mvc.Core/Controllers/DefaultControllerActionArgumentBinder.cs @@ -124,13 +124,11 @@ namespace Microsoft.AspNet.Mvc.Controllers var propertyHelper = propertyHelpers.First(helper => string.Equals(helper.Name, property.Key, StringComparison.Ordinal)); var propertyType = propertyHelper.Property.PropertyType; + var metadata = _modelMetadataProvider.GetMetadataForType(propertyType); var source = property.Value; if (propertyHelper.Property.CanWrite && propertyHelper.Property.SetMethod?.IsPublic == true) { - // Handle settable property. - var metadata = _modelMetadataProvider.GetMetadataForType(propertyType); - - // Do not set the property to null if the type is a non-nullable type. + // Handle settable property. Do not set the property to null if the type is a non-nullable type. if (source != null || metadata.IsReferenceOrNullableType) { propertyHelper.SetValue(controller, source); @@ -153,12 +151,7 @@ namespace Microsoft.AspNet.Mvc.Controllers continue; } - // Determine T if this is an ICollection property. - var collectionTypeArguments = ClosedGenericMatcher.ExtractGenericInterface( - propertyType, - typeof(ICollection<>)) - ?.GenericTypeArguments; - if (collectionTypeArguments == null) + if (!metadata.IsCollectionType) { // Not a collection model. continue; @@ -166,7 +159,7 @@ namespace Microsoft.AspNet.Mvc.Controllers // Handle a read-only collection property. var propertyAddRange = CallPropertyAddRangeOpenGenericMethod.MakeGenericMethod( - collectionTypeArguments); + metadata.ElementMetadata.ModelType); propertyAddRange.Invoke(obj: null, parameters: new[] { target, source }); } } diff --git a/src/Microsoft.AspNet.Mvc.Core/ModelBinding/GenericModelBinder.cs b/src/Microsoft.AspNet.Mvc.Core/ModelBinding/GenericModelBinder.cs index cfad6553aa..6d5ca57fa2 100644 --- a/src/Microsoft.AspNet.Mvc.Core/ModelBinding/GenericModelBinder.cs +++ b/src/Microsoft.AspNet.Mvc.Core/ModelBinding/GenericModelBinder.cs @@ -117,9 +117,7 @@ namespace Microsoft.AspNet.Mvc.ModelBinding // ICollection. For example an IEnumerable property may have a List default value. Do not use // IsAssignableFrom() because that does not handle explicit interface implementations and binders all // perform explicit casts. - var closedGenericInterface = - ClosedGenericMatcher.ExtractGenericInterface(context.Model.GetType(), typeof(ICollection<>)); - if (closedGenericInterface == null) + if (!context.ModelMetadata.IsCollectionType) { return null; } diff --git a/src/Microsoft.AspNet.Mvc.Core/ModelBinding/Metadata/DefaultModelMetadata.cs b/src/Microsoft.AspNet.Mvc.Core/ModelBinding/Metadata/DefaultModelMetadata.cs index 9d9723315c..a89c04260a 100644 --- a/src/Microsoft.AspNet.Mvc.Core/ModelBinding/Metadata/DefaultModelMetadata.cs +++ b/src/Microsoft.AspNet.Mvc.Core/ModelBinding/Metadata/DefaultModelMetadata.cs @@ -236,10 +236,10 @@ namespace Microsoft.AspNet.Mvc.ModelBinding.Metadata if (!_haveCalculatedElementMetadata) { _haveCalculatedElementMetadata = true; - if (!IsCollectionType) + if (!IsEnumerableType) { - // Short-circuit checks below. If not IsCollectionType, ElementMetadata is null. - // For example, as in IsCollectionType, do not consider strings collections. + // Short-circuit checks below. If not IsEnumerableType, ElementMetadata is null. + // For example, as in IsEnumerableType, do not consider strings collections. return null; } @@ -261,7 +261,7 @@ namespace Microsoft.AspNet.Mvc.ModelBinding.Metadata Debug.Assert( elementType != null, - $"Unable to find element type for '{ ModelType.FullName }' though IsCollectionType is true."); + $"Unable to find element type for '{ ModelType.FullName }' though IsEnumerableType is true."); // Success _elementMetadata = _provider.GetMetadataForType(elementType); diff --git a/src/Microsoft.AspNet.Mvc.Core/ModelBinding/MutableObjectModelBinder.cs b/src/Microsoft.AspNet.Mvc.Core/ModelBinding/MutableObjectModelBinder.cs index f6f0ea5e68..4491225b5c 100644 --- a/src/Microsoft.AspNet.Mvc.Core/ModelBinding/MutableObjectModelBinder.cs +++ b/src/Microsoft.AspNet.Mvc.Core/ModelBinding/MutableObjectModelBinder.cs @@ -231,7 +231,7 @@ namespace Microsoft.AspNet.Mvc.ModelBinding return false; } - if (modelMetadata.IsCollectionType) + if (modelMetadata.IsEnumerableType) { return false; } @@ -522,17 +522,14 @@ namespace Microsoft.AspNet.Mvc.ModelBinding // Determine T if this is an ICollection property. No need for a T[] case because CanUpdateProperty() // ensures property is either settable or not an array. Underlying assumption is that CanUpdateProperty() // and SetProperty() are overridden together. - var collectionTypeArguments = ClosedGenericMatcher.ExtractGenericInterface( - propertyExplorer.ModelType, - typeof(ICollection<>)) - ?.GenericTypeArguments; - if (collectionTypeArguments == null) + if (!propertyExplorer.Metadata.IsCollectionType) { // Not a collection model. return; } - var propertyAddRange = CallPropertyAddRangeOpenGenericMethod.MakeGenericMethod(collectionTypeArguments); + var propertyAddRange = CallPropertyAddRangeOpenGenericMethod.MakeGenericMethod( + propertyExplorer.Metadata.ElementMetadata.ModelType); try { propertyAddRange.Invoke(obj: null, parameters: new[] { target, source }); diff --git a/src/Microsoft.AspNet.Mvc.TagHelpers/SelectTagHelper.cs b/src/Microsoft.AspNet.Mvc.TagHelpers/SelectTagHelper.cs index 5a858991e2..48ab9263ca 100644 --- a/src/Microsoft.AspNet.Mvc.TagHelpers/SelectTagHelper.cs +++ b/src/Microsoft.AspNet.Mvc.TagHelpers/SelectTagHelper.cs @@ -90,7 +90,7 @@ namespace Microsoft.AspNet.Mvc.TagHelpers // Base allowMultiple on the instance or declared type of the expression to avoid a // "SelectExpressionNotEnumerable" InvalidOperationException during generation. - // Metadata.IsCollectionType() is similar but does not take runtime type into account. + // Metadata.IsEnumerableType is similar but does not take runtime type into account. var realModelType = For.ModelExplorer.ModelType; var allowMultiple = typeof(string) != realModelType && typeof(IEnumerable).GetTypeInfo().IsAssignableFrom(realModelType.GetTypeInfo()); diff --git a/src/Microsoft.AspNet.Mvc.ViewFeatures/ViewFeatures/DefaultHtmlGenerator.cs b/src/Microsoft.AspNet.Mvc.ViewFeatures/ViewFeatures/DefaultHtmlGenerator.cs index 85c385d7a4..ff4855a00e 100644 --- a/src/Microsoft.AspNet.Mvc.ViewFeatures/ViewFeatures/DefaultHtmlGenerator.cs +++ b/src/Microsoft.AspNet.Mvc.ViewFeatures/ViewFeatures/DefaultHtmlGenerator.cs @@ -818,7 +818,7 @@ namespace Microsoft.AspNet.Mvc.ViewFeatures modelExplorer = modelExplorer ?? ExpressionMetadataProvider.FromStringExpression(expression, viewContext.ViewData, _metadataProvider); var metadata = modelExplorer.Metadata; - if (allowMultiple && metadata.IsCollectionType) + if (allowMultiple && metadata.IsEnumerableType) { metadata = metadata.ElementMetadata; } diff --git a/test/Microsoft.AspNet.Mvc.Abstractions.Test/ModelBinding/ModelMetadataTest.cs b/test/Microsoft.AspNet.Mvc.Abstractions.Test/ModelBinding/ModelMetadataTest.cs index 4f028d9e6a..17dcce2206 100644 --- a/test/Microsoft.AspNet.Mvc.Abstractions.Test/ModelBinding/ModelMetadataTest.cs +++ b/test/Microsoft.AspNet.Mvc.Abstractions.Test/ModelBinding/ModelMetadataTest.cs @@ -50,7 +50,8 @@ namespace Microsoft.AspNet.Mvc.ModelBinding Assert.True(modelMetadata.IsComplexType); } - // IsCollectionType + // IsCollectionType / IsEnumerableType + private class NonCollectionType { } @@ -59,11 +60,49 @@ namespace Microsoft.AspNet.Mvc.ModelBinding { } + private class JustEnumerable : IEnumerable + { + public IEnumerator GetEnumerator() + { + throw new NotImplementedException(); + } + } + + public static TheoryData NonCollectionNonEnumerableData + { + get + { + return new TheoryData + { + typeof(object), + typeof(int), + typeof(NonCollectionType), + typeof(string), + }; + } + } + + public static TheoryData CollectionAndEnumerableData + { + get + { + return new TheoryData + { + typeof(int[]), + typeof(List), + typeof(DerivedList), + typeof(Collection), + typeof(Dictionary), + typeof(CollectionImplementation), + }; + } + } + [Theory] - [InlineData(typeof(object))] - [InlineData(typeof(int))] - [InlineData(typeof(NonCollectionType))] - [InlineData(typeof(string))] + [MemberData(nameof(NonCollectionNonEnumerableData))] + [InlineData(typeof(IEnumerable))] + [InlineData(typeof(IEnumerable))] + [InlineData(typeof(JustEnumerable))] public void IsCollectionType_ReturnsFalseForNonCollectionTypes(Type type) { // Arrange @@ -77,13 +116,7 @@ namespace Microsoft.AspNet.Mvc.ModelBinding } [Theory] - [InlineData(typeof(int[]))] - [InlineData(typeof(List))] - [InlineData(typeof(DerivedList))] - [InlineData(typeof(IEnumerable))] - [InlineData(typeof(IEnumerable))] - [InlineData(typeof(Collection))] - [InlineData(typeof(Dictionary))] + [MemberData(nameof(CollectionAndEnumerableData))] public void IsCollectionType_ReturnsTrueForCollectionTypes(Type type) { // Arrange @@ -96,6 +129,37 @@ namespace Microsoft.AspNet.Mvc.ModelBinding Assert.True(modelMetadata.IsCollectionType); } + [Theory] + [MemberData(nameof(NonCollectionNonEnumerableData))] + public void IsEnumerableType_ReturnsFalseForNonEnumerableTypes(Type type) + { + // Arrange + var provider = new EmptyModelMetadataProvider(); + + // Act + var modelMetadata = new TestModelMetadata(type); + + // Assert + Assert.False(modelMetadata.IsEnumerableType); + } + + [Theory] + [MemberData(nameof(CollectionAndEnumerableData))] + [InlineData(typeof(IEnumerable))] + [InlineData(typeof(IEnumerable))] + [InlineData(typeof(JustEnumerable))] + public void IsEnumerableType_ReturnsTrueForEnumerableTypes(Type type) + { + // Arrange + var provider = new EmptyModelMetadataProvider(); + + // Act + var modelMetadata = new TestModelMetadata(type); + + // Assert + Assert.True(modelMetadata.IsEnumerableType); + } + // IsNullableValueType [Theory] @@ -481,5 +545,59 @@ namespace Microsoft.AspNet.Mvc.ModelBinding } } } + + private class CollectionImplementation : ICollection + { + public int Count + { + get + { + throw new NotImplementedException(); + } + } + + public bool IsReadOnly + { + get + { + throw new NotImplementedException(); + } + } + + public void Add(string item) + { + throw new NotImplementedException(); + } + + public void Clear() + { + throw new NotImplementedException(); + } + + public bool Contains(string item) + { + throw new NotImplementedException(); + } + + public void CopyTo(string[] array, int arrayIndex) + { + throw new NotImplementedException(); + } + + public IEnumerator GetEnumerator() + { + throw new NotImplementedException(); + } + + public bool Remove(string item) + { + throw new NotImplementedException(); + } + + IEnumerator IEnumerable.GetEnumerator() + { + throw new NotImplementedException(); + } + } } } diff --git a/test/Microsoft.AspNet.Mvc.Core.Test/ModelBinding/Metadata/DefaultModelMetadataTest.cs b/test/Microsoft.AspNet.Mvc.Core.Test/ModelBinding/Metadata/DefaultModelMetadataTest.cs index 5939d875fb..9f74f3ad99 100644 --- a/test/Microsoft.AspNet.Mvc.Core.Test/ModelBinding/Metadata/DefaultModelMetadataTest.cs +++ b/test/Microsoft.AspNet.Mvc.Core.Test/ModelBinding/Metadata/DefaultModelMetadataTest.cs @@ -40,8 +40,9 @@ namespace Microsoft.AspNet.Mvc.ModelBinding.Metadata Assert.True(metadata.HtmlEncode); Assert.True(metadata.IsBindingAllowed); Assert.False(metadata.IsBindingRequired); - Assert.False(metadata.IsComplexType); Assert.False(metadata.IsCollectionType); + Assert.False(metadata.IsComplexType); + Assert.False(metadata.IsEnumerableType); Assert.False(metadata.IsEnum); Assert.False(metadata.IsFlagsEnum); Assert.False(metadata.IsNullableValueType); @@ -218,7 +219,7 @@ namespace Microsoft.AspNet.Mvc.ModelBinding.Metadata // Act var isBindingRequired = metadata.IsBindingRequired; - + // Assert Assert.False(isBindingRequired); }