diff --git a/src/Microsoft.AspNet.Mvc.Common/Microsoft.AspNet.Mvc.Common.kproj b/src/Microsoft.AspNet.Mvc.Common/Microsoft.AspNet.Mvc.Common.kproj index 87d0bfd666..a82a179555 100644 --- a/src/Microsoft.AspNet.Mvc.Common/Microsoft.AspNet.Mvc.Common.kproj +++ b/src/Microsoft.AspNet.Mvc.Common/Microsoft.AspNet.Mvc.Common.kproj @@ -22,7 +22,8 @@ + - + \ No newline at end of file diff --git a/src/Microsoft.AspNet.Mvc.Core/Internal/PropertyHelper.cs b/src/Microsoft.AspNet.Mvc.Common/PropertyHelper.cs similarity index 91% rename from src/Microsoft.AspNet.Mvc.Core/Internal/PropertyHelper.cs rename to src/Microsoft.AspNet.Mvc.Common/PropertyHelper.cs index 893f54cbef..666ec7a3c2 100644 --- a/src/Microsoft.AspNet.Mvc.Core/Internal/PropertyHelper.cs +++ b/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. /// - 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 /// public static PropertyHelper[] GetProperties(object instance) { - return GetProperties(instance, CreateInstance, ReflectionCache); + return GetProperties(instance.GetType()); + } + + /// + /// Creates and caches fast property helpers that expose getters for every public get property on the + /// specified type. + /// + /// the type to extract property accessors for. + /// a cached array of all public property getters from the type of target instance. + /// + public static PropertyHelper[] GetProperties(Type type) + { + return GetProperties(type, CreateInstance, ReflectionCache); } /// @@ -181,19 +194,17 @@ namespace Microsoft.AspNet.Mvc } protected static PropertyHelper[] GetProperties( - object instance, + Type type, Func createPropertyHelper, ConcurrentDictionary 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 && diff --git a/src/Microsoft.AspNet.Mvc.Common/project.json b/src/Microsoft.AspNet.Mvc.Common/project.json index ad5b80a7e1..90bfb2c484 100644 --- a/src/Microsoft.AspNet.Mvc.Common/project.json +++ b/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": { diff --git a/src/Microsoft.AspNet.Mvc.Core/Microsoft.AspNet.Mvc.Core.kproj b/src/Microsoft.AspNet.Mvc.Core/Microsoft.AspNet.Mvc.Core.kproj index 0291a37558..fbaec86171 100644 --- a/src/Microsoft.AspNet.Mvc.Core/Microsoft.AspNet.Mvc.Core.kproj +++ b/src/Microsoft.AspNet.Mvc.Core/Microsoft.AspNet.Mvc.Core.kproj @@ -136,7 +136,6 @@ - diff --git a/src/Microsoft.AspNet.Mvc.Core/Rendering/HtmlAttributePropertyHelper.cs b/src/Microsoft.AspNet.Mvc.Core/Rendering/HtmlAttributePropertyHelper.cs index 08db8b6c18..bae504aa50 100644 --- a/src/Microsoft.AspNet.Mvc.Core/Rendering/HtmlAttributePropertyHelper.cs +++ b/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) diff --git a/src/Microsoft.AspNet.Mvc.ModelBinding/Metadata/AssociatedMetadataProvider.cs b/src/Microsoft.AspNet.Mvc.ModelBinding/Metadata/AssociatedMetadataProvider.cs index e2c5e2d4da..2e23e34724 100644 --- a/src/Microsoft.AspNet.Mvc.ModelBinding/Metadata/AssociatedMetadataProvider.cs +++ b/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 modelAccessor = null; if (container != null) { - var propertyGetter = propertyInfo.ValueAccessor; - modelAccessor = () => propertyGetter(container); + modelAccessor = () => propertyInfo.PropertyHelper.GetValue(container); } yield return CreatePropertyMetadata(modelAccessor, propertyInfo); } @@ -113,13 +110,13 @@ namespace Microsoft.AspNet.Mvc.ModelBinding propertyName: null) }; - var properties = new Dictionary(); - foreach (var property in type.GetProperties(BindingFlags.Public | BindingFlags.Instance)) + var properties = new Dictionary(StringComparer.Ordinal); + 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 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 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)dynamicMethod.CreateDelegate(typeof(Func)); - } - private sealed class TypeInformation { public TModelMetadata Prototype { get; set; } @@ -218,7 +146,7 @@ namespace Microsoft.AspNet.Mvc.ModelBinding private sealed class PropertyInformation { - public Func ValueAccessor { get; set; } + public PropertyHelper PropertyHelper { get; set; } public TModelMetadata Prototype { get; set; } public bool IsReadOnly { get; set; } } diff --git a/test/Microsoft.AspNet.Mvc.Core.Test/PropertyHelperTest.cs b/test/Microsoft.AspNet.Mvc.Core.Test/PropertyHelperTest.cs index 691296e5fe..a7b7765279 100644 --- a/test/Microsoft.AspNet.Mvc.Core.Test/PropertyHelperTest.cs +++ b/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; } + } } } diff --git a/test/Microsoft.AspNet.Mvc.ModelBinding.Test/Metadata/AssociatedMetadataProviderTest.cs b/test/Microsoft.AspNet.Mvc.ModelBinding.Test/Metadata/AssociatedMetadataProviderTest.cs index 8be9c1a284..a27909d305 100644 --- a/test/Microsoft.AspNet.Mvc.ModelBinding.Test/Metadata/AssociatedMetadataProviderTest.cs +++ b/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; @@ -41,7 +42,35 @@ namespace Microsoft.AspNet.Mvc.ModelBinding Assert.True(mixed.Attributes.Any(a => a is RequiredAttribute)); 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()); + + 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(); - ModelMetadata metadata = new ModelMetadata(provider, null, null, typeof(TypeModel), null); + var provider = new TestableAssociatedMetadataProvider(); + 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 { }