Modify AssociatedMetadataProvider to use PropertyHelper to create accessor

Additionally change it to use TypeExtensions.GetReadableProperties to get
property list. This causes it to ignore indexers which should not be
considered.

Fixes #595

src/Microsoft.AspNet.Mvc.Common/Microsoft.AspNet.Mvc.Common.kproj

@@ -22,6 +22,7 @@
   <ItemGroup>
     <Compile Include="NotNullArgument.cs" />
     <Compile Include="PlatformHelper.cs" />
+    <Compile Include="PropertyHelper.cs" />
     <Compile Include="TypeExtensions.cs" />
   </ItemGroup>
   <Import Project="$(VSToolsPath)\AspNet\Microsoft.Web.AspNet.targets" Condition="'$(VSToolsPath)' != ''" />

src/Microsoft.AspNet.Mvc.Common/PropertyHelper.cs

@@ -33,14 +33,15 @@ namespace Microsoft.AspNet.Mvc
         ///
         /// This constructor does not cache the helper. For caching, use GetProperties.
         /// </summary>
-        public PropertyHelper(PropertyInfo property)
+        public PropertyHelper([NotNull] PropertyInfo property)
         {
-            Contract.Assert(property != null);
+            Property = property;
-
             Name = property.Name;
             _valueGetter = MakeFastPropertyGetter(property);
         }
 
+        public PropertyInfo Property { get; private set; }
+
         public virtual string Name { get; protected set; }
 
         public object GetValue(object instance)
@@ -57,7 +58,19 @@ namespace Microsoft.AspNet.Mvc
         /// </returns>
         public static PropertyHelper[] GetProperties(object instance)
         {
-            return GetProperties(instance, CreateInstance, ReflectionCache);
+            return GetProperties(instance.GetType());
+        }
+
+        /// <summary>
+        /// Creates and caches fast property helpers that expose getters for every public get property on the
+        /// specified type.
+        /// </summary>
+        /// <param name="type">the type to extract property accessors for.</param>
+        /// <returns>a cached array of all public property getters from the type of target instance.
+        /// </returns>
+        public static PropertyHelper[] GetProperties(Type type)
+        {
+            return GetProperties(type, CreateInstance, ReflectionCache);
         }
 
         /// <summary>
@@ -181,19 +194,17 @@ namespace Microsoft.AspNet.Mvc
         }
 
         protected static PropertyHelper[] GetProperties(
-            object instance,
+            Type type,
             Func<PropertyInfo, PropertyHelper> createPropertyHelper,
             ConcurrentDictionary<Type, PropertyHelper[]> cache)
         {
             // Using an array rather than IEnumerable, as target will be called on the hot path numerous times.
             PropertyHelper[] helpers;
 
-            var type = instance.GetType();
-
             if (!cache.TryGetValue(type, out helpers))
             {
                 // We avoid loading indexed properties using the where statement.
-                // Indexed properties are not useful (or valid) for grabbing properties off an anonymous object.
+                // Indexed properties are not useful (or valid) for grabbing properties off an object.
                 var properties = type.GetRuntimeProperties().Where(
                     prop => prop.GetIndexParameters().Length == 0 &&
                     prop.GetMethod != null &&

src/Microsoft.AspNet.Mvc.Common/project.json

@@ -4,6 +4,7 @@
   "dependencies": {
     "System.Linq": "4.0.0.0",
     "System.Reflection": "4.0.10.0",
+    "System.Reflection.Extensions": "4.0.0.0",
     "System.Runtime" : "4.0.20.0"
   },
   "configurations": {

src/Microsoft.AspNet.Mvc.Core/Microsoft.AspNet.Mvc.Core.kproj

@@ -136,7 +136,6 @@
     <Compile Include="DefaultControllerActivator.cs" />
     <Compile Include="IControllerFactory.cs" />
     <Compile Include="Injector.cs" />
-    <Compile Include="Internal\PropertyHelper.cs" />
     <Compile Include="Internal\TypeHelper.cs" />
     <Compile Include="Internal\UTF8EncodingWithoutBOM.cs" />
     <Compile Include="IUrlHelper.cs" />

src/Microsoft.AspNet.Mvc.Core/Rendering/HtmlAttributePropertyHelper.cs

@@ -14,7 +14,7 @@ namespace Microsoft.AspNet.Mvc.Rendering
 
         public static new PropertyHelper[] GetProperties(object instance)
         {
-            return GetProperties(instance, CreateInstance, ReflectionCache);
+            return GetProperties(instance.GetType(), CreateInstance, ReflectionCache);
         }
 
         private static PropertyHelper CreateInstance(PropertyInfo property)

src/Microsoft.AspNet.Mvc.ModelBinding/Metadata/AssociatedMetadataProvider.cs

@@ -4,10 +4,8 @@
 using System;
 using System.Collections.Concurrent;
 using System.Collections.Generic;
-using System.Diagnostics.Contracts;
 using System.Linq;
 using System.Reflection;
-using System.Reflection.Emit;
 
 namespace Microsoft.AspNet.Mvc.ModelBinding
 {
@@ -67,8 +65,7 @@ namespace Microsoft.AspNet.Mvc.ModelBinding
                 Func<object> modelAccessor = null;
                 if (container != null)
                 {
-                    var propertyGetter = propertyInfo.ValueAccessor;
+                    modelAccessor = () => propertyInfo.PropertyHelper.GetValue(container);
-                    modelAccessor = () => propertyGetter(container);
                 }
                 yield return CreatePropertyMetadata(modelAccessor, propertyInfo);
             }
@@ -113,13 +110,13 @@ namespace Microsoft.AspNet.Mvc.ModelBinding
                                                     propertyName: null)
             };
 
-            var properties = new Dictionary<string, PropertyInformation>();
+            var properties = new Dictionary<string, PropertyInformation>(StringComparer.Ordinal);
-            foreach (var property in type.GetProperties(BindingFlags.Public | BindingFlags.Instance))
+            foreach (var propertyHelper in PropertyHelper.GetProperties(type))
             {
                 // Avoid re-generating a property descriptor if one has already been generated for the property name
-                if (!properties.ContainsKey(property.Name))
+                if (!properties.ContainsKey(propertyHelper.Name))
                 {
-                    properties.Add(property.Name, CreatePropertyInformation(type, property));
+                    properties.Add(propertyHelper.Name, CreatePropertyInformation(type, propertyHelper));
                 }
             }
             info.Properties = properties;
@@ -127,11 +124,12 @@ namespace Microsoft.AspNet.Mvc.ModelBinding
             return info;
         }
 
-        private PropertyInformation CreatePropertyInformation(Type containerType, PropertyInfo property)
+        private PropertyInformation CreatePropertyInformation(Type containerType, PropertyHelper helper)
         {
+            var property = helper.Property;
             return new PropertyInformation
             {
-                ValueAccessor = CreatePropertyValueAccessor(property),
+                PropertyHelper = helper,
                 Prototype = CreateMetadataPrototype(property.GetCustomAttributes(),
                                                     containerType,
                                                     property.PropertyType,
@@ -140,76 +138,6 @@ namespace Microsoft.AspNet.Mvc.ModelBinding
             };
         }
 
-        private static Func<object, object> CreatePropertyValueAccessor(PropertyInfo property)
-        {
-            var declaringType = property.DeclaringType;
-            var declaringTypeInfo = declaringType.GetTypeInfo();
-            if (declaringTypeInfo.IsVisible)
-            {
-                if (property.CanRead)
-                {
-                    var getMethodInfo = property.GetMethod;
-                    if (getMethodInfo != null)
-                    {
-                        return CreateDynamicValueAccessor(getMethodInfo, declaringType, property.Name);
-                    }
-                }
-            }
-
-            // If either the type isn't public or we can't find a public getter, use the slow Reflection path
-            return container => property.GetValue(container);
-        }
-
-        // Uses Lightweight Code Gen to generate a tiny delegate that gets the property value
-        // This is an optimization to avoid having to go through the much slower System.Reflection APIs
-        // e.g. generates (object o) => (Person)o.Id
-        private static Func<object, object> CreateDynamicValueAccessor(MethodInfo getMethodInfo,
-                                                                       Type declaringType,
-                                                                       string propertyName)
-        {
-            Contract.Assert(getMethodInfo != null && getMethodInfo.IsPublic && !getMethodInfo.IsStatic);
-
-            var declaringTypeInfo = declaringType.GetTypeInfo();
-            var propertyType = getMethodInfo.ReturnType;
-            var dynamicMethod = new DynamicMethod("Get" + propertyName + "From" + declaringType.Name,
-                                                  typeof(object),
-                                                  new[] { typeof(object) });
-            var ilg = dynamicMethod.GetILGenerator();
-
-            // Load the container onto the stack, convert from object => declaring type for the property
-            ilg.Emit(OpCodes.Ldarg_0);
-            if (declaringTypeInfo.IsValueType)
-            {
-                ilg.Emit(OpCodes.Unbox, declaringType);
-            }
-            else
-            {
-                ilg.Emit(OpCodes.Castclass, declaringType);
-            }
-
-            // if declaring type is value type, we use Call : structs don't have inheritance
-            // if get method is sealed or isn't virtual, we use Call : it can't be overridden
-            if (declaringTypeInfo.IsValueType || !getMethodInfo.IsVirtual || getMethodInfo.IsFinal)
-            {
-                ilg.Emit(OpCodes.Call, getMethodInfo);
-            }
-            else
-            {
-                ilg.Emit(OpCodes.Callvirt, getMethodInfo);
-            }
-
-            // Box if the property type is a value type, so it can be returned as an object
-            if (propertyType.GetTypeInfo().IsValueType)
-            {
-                ilg.Emit(OpCodes.Box, propertyType);
-            }
-
-            // Return property value
-            ilg.Emit(OpCodes.Ret);
-
-            return (Func<object, object>)dynamicMethod.CreateDelegate(typeof(Func<object, object>));
-        }
-
         private sealed class TypeInformation
         {
             public TModelMetadata Prototype { get; set; }
@@ -218,7 +146,7 @@ namespace Microsoft.AspNet.Mvc.ModelBinding
 
         private sealed class PropertyInformation
         {
-            public Func<object, object> ValueAccessor { get; set; }
+            public PropertyHelper PropertyHelper { get; set; }
             public TModelMetadata Prototype { get; set; }
             public bool IsReadOnly { get; set; }
         }

test/Microsoft.AspNet.Mvc.Core.Test/PropertyHelperTest.cs

@@ -190,6 +190,23 @@ namespace Microsoft.AspNet.Mvc
             Assert.Equal("propBValue", propBHelper.GetValue(derived));
         }
 
+        [Fact]
+        public void GetProperties_ExcludesIndexersAndPropertiesWithoutPublicGetters()
+        {
+            // Arrange
+            var type = typeof(DerivedClassWithNonReadableProperties);
+
+
+            // Act
+            var result = PropertyHelper.GetProperties(type).ToArray();
+
+            // Assert
+            Assert.Equal(3, result.Length);
+            Assert.Equal("Visible", result[0].Name);
+            Assert.Equal("PropA", result[1].Name);
+            Assert.Equal("PropB", result[2].Name);
+        }
+
         [Fact]
         public void MakeFastPropertySetter_SetsPropertyValues_ForPublicAndNobPublicProperties()
         {
@@ -311,5 +328,27 @@ namespace Microsoft.AspNet.Mvc
                 set { _value = "Overriden" + value; }
             }
         }
+
+        private class DerivedClassWithNonReadableProperties : BaseClassWithVirtual
+        {
+            public string this[int index]
+            {
+                get { return string.Empty; }
+                set { }
+            }
+
+            public int Visible { get; set; }
+
+            private string NotVisible { get; set; }
+
+            public string NotVisible2 { private get; set; }
+
+            public string NotVisible3
+            {
+                set { }
+            }
+
+            public static string NotVisible4 { get; set; }
+        }
     }
 }

test/Microsoft.AspNet.Mvc.ModelBinding.Test/Metadata/AssociatedMetadataProviderTest.cs

@@ -4,6 +4,7 @@
 using System;
 using System.Collections.Generic;
 using System.ComponentModel;
+using System.ComponentModel.DataAnnotations;
 using System.Linq;
 using Microsoft.AspNet.Testing;
 using Xunit;
@@ -42,6 +43,34 @@ namespace Microsoft.AspNet.Mvc.ModelBinding
             Assert.True(mixed.Attributes.Any(a => a is RangeAttribute));
         }
         
+        [Fact]
+        public void GetMetadataForProperties_ExcludesIndexers()
+        {
+            // Arrange
+            var value = "some value";
+            var model = new ModelWithIndexer { Value = value };
+            var provider = new TestableAssociatedMetadataProvider();
+            var modelType = model.GetType();
+
+            // Act
+            provider.GetMetadataForProperties(model, modelType).ToList();
+
+            // Assert
+            Assert.Equal(2, provider.CreateMetadataFromPrototypeLog.Count);
+            Assert.Equal(value, provider.CreateMetadataFromPrototypeLog[0].Model);
+            Assert.Null(provider.CreateMetadataFromPrototypeLog[1].Model);
+
+            var valueMetadata = provider.CreateMetadataPrototypeLog.Single(m => m.ContainerType == modelType &&
+                                                                        m.PropertyName == "Value");
+            Assert.Equal(typeof(string), valueMetadata.ModelType);
+            Assert.Single(valueMetadata.Attributes.OfType<MinLengthAttribute>());
+
+            var testPropertyMetadata = provider.CreateMetadataPrototypeLog.Single(m => m.ContainerType == modelType &&
+                                                                                     m.PropertyName == "TestProperty");
+            Assert.Equal(typeof(string), testPropertyMetadata.ModelType);
+        }
+
+        // GetMetadataForProperties
         [Fact]
         public void GetMetadataForPropertyWithNullContainerReturnsMetadataWithNullValuesForProperties()
         {
@@ -150,16 +179,16 @@ namespace Microsoft.AspNet.Mvc.ModelBinding
         [Fact]
         public void GetMetadataForTypeIncludesAttributesOnType()
         {
-            TestableAssociatedMetadataProvider provider = new TestableAssociatedMetadataProvider();
+            var provider = new TestableAssociatedMetadataProvider();
-            ModelMetadata metadata = new ModelMetadata(provider, null, null, typeof(TypeModel), null);
+            var metadata = new ModelMetadata(provider, null, null, typeof(TypeModel), null);
             provider.CreateMetadataFromPrototypeReturnValue = metadata;
 
             // Act
-            ModelMetadata result = provider.GetMetadataForType(null, typeof(TypeModel));
+            var result = provider.GetMetadataForType(null, typeof(TypeModel));
 
             // Assert
             Assert.Same(metadata, result);
-            CreateMetadataPrototypeParams parms = provider.CreateMetadataPrototypeLog.Single(p => p.ModelType == typeof(TypeModel));
+            var parms = provider.CreateMetadataPrototypeLog.Single(p => p.ModelType == typeof(TypeModel));
             Assert.True(parms.Attributes.Any(a => a is ReadOnlyAttribute));
         }
 #endif
@@ -181,6 +210,23 @@ namespace Microsoft.AspNet.Mvc.ModelBinding
             public double MixedAttributes { get; set; }
         }
 
+        private class BaseType
+        {
+            public string TestProperty { get; set; }
+        }
+
+        private class ModelWithIndexer : BaseType
+        {
+            public string this[string x]
+            {
+                get { return string.Empty; }
+                set { }
+            }
+
+            [MinLength(4)]
+            public string Value { get; set; }
+        }
+
         private sealed class RequiredAttribute : Attribute
         {
         }