diff --git a/src/Microsoft.AspNet.Mvc.ModelBinding/Metadata/DefaultMetadataDetailsCache.cs b/src/Microsoft.AspNet.Mvc.ModelBinding/Metadata/DefaultMetadataDetails.cs similarity index 76% rename from src/Microsoft.AspNet.Mvc.ModelBinding/Metadata/DefaultMetadataDetailsCache.cs rename to src/Microsoft.AspNet.Mvc.ModelBinding/Metadata/DefaultMetadataDetails.cs index 1fff319426..f0766ed1bc 100644 --- a/src/Microsoft.AspNet.Mvc.ModelBinding/Metadata/DefaultMetadataDetailsCache.cs +++ b/src/Microsoft.AspNet.Mvc.ModelBinding/Metadata/DefaultMetadataDetails.cs @@ -7,21 +7,19 @@ using System.Collections.Generic; namespace Microsoft.AspNet.Mvc.ModelBinding.Metadata { /// - /// A cache of metadata objects for a . + /// Holds associated metadata objects for a . /// /// - /// These instances are shared by all instances representing - /// the same , property, or parameter. Any modifications to the data must be - /// thread-safe for multiple readers and writers. + /// Any modifications to the data must be thread-safe for multiple readers and writers. /// - public class DefaultMetadataDetailsCache + public class DefaultMetadataDetails { /// - /// Creates a new . + /// Creates a new . /// /// The . /// The set of model attributes. - public DefaultMetadataDetailsCache(ModelMetadataIdentity key, IReadOnlyList attributes) + public DefaultMetadataDetails(ModelMetadataIdentity key, IReadOnlyList attributes) { Key = key; Attributes = attributes; @@ -33,20 +31,25 @@ namespace Microsoft.AspNet.Mvc.ModelBinding.Metadata public IReadOnlyList Attributes { get; } /// - /// Gets or sets the + /// Gets or sets the . /// public BindingMetadata BindingMetadata { get; set; } /// - /// Gets or sets the + /// Gets or sets the . /// public DisplayMetadata DisplayMetadata { get; set; } /// - /// Gets or sets the + /// Gets or sets the . /// public ModelMetadataIdentity Key { get; } + /// + /// Gets or sets the entries for the model properties. + /// + public ModelMetadata[] Properties { get; set; } + /// /// Gets or sets a property accessor delegate to get the property value from a model object. /// diff --git a/src/Microsoft.AspNet.Mvc.ModelBinding/Metadata/DefaultModelMetadata.cs b/src/Microsoft.AspNet.Mvc.ModelBinding/Metadata/DefaultModelMetadata.cs index 66414b25c3..1195957bad 100644 --- a/src/Microsoft.AspNet.Mvc.ModelBinding/Metadata/DefaultModelMetadata.cs +++ b/src/Microsoft.AspNet.Mvc.ModelBinding/Metadata/DefaultModelMetadata.cs @@ -16,7 +16,7 @@ namespace Microsoft.AspNet.Mvc.ModelBinding.Metadata { private readonly IModelMetadataProvider _provider; private readonly ICompositeMetadataDetailsProvider _detailsProvider; - private readonly DefaultMetadataDetailsCache _cache; + private readonly DefaultMetadataDetails _details; private ReadOnlyDictionary _additionalValues; private bool? _isReadOnly; @@ -29,16 +29,16 @@ namespace Microsoft.AspNet.Mvc.ModelBinding.Metadata /// /// The . /// The . - /// The . + /// The . public DefaultModelMetadata( [NotNull] IModelMetadataProvider provider, [NotNull] ICompositeMetadataDetailsProvider detailsProvider, - [NotNull] DefaultMetadataDetailsCache cache) - : base(cache.Key) + [NotNull] DefaultMetadataDetails details) + : base(details.Key) { _provider = provider; _detailsProvider = detailsProvider; - _cache = cache; + _details = details; } /// @@ -48,7 +48,7 @@ namespace Microsoft.AspNet.Mvc.ModelBinding.Metadata { get { - return _cache.Attributes; + return _details.Attributes; } } @@ -62,14 +62,14 @@ namespace Microsoft.AspNet.Mvc.ModelBinding.Metadata { get { - if (_cache.BindingMetadata == null) + if (_details.BindingMetadata == null) { - var context = new BindingMetadataProviderContext(Identity, _cache.Attributes); + var context = new BindingMetadataProviderContext(Identity, _details.Attributes); _detailsProvider.GetBindingMetadata(context); - _cache.BindingMetadata = context.BindingMetadata; + _details.BindingMetadata = context.BindingMetadata; } - return _cache.BindingMetadata; + return _details.BindingMetadata; } } @@ -83,14 +83,14 @@ namespace Microsoft.AspNet.Mvc.ModelBinding.Metadata { get { - if (_cache.DisplayMetadata == null) + if (_details.DisplayMetadata == null) { - var context = new DisplayMetadataProviderContext(Identity, _cache.Attributes); + var context = new DisplayMetadataProviderContext(Identity, _details.Attributes); _detailsProvider.GetDisplayMetadata(context); - _cache.DisplayMetadata = context.DisplayMetadata; + _details.DisplayMetadata = context.DisplayMetadata; } - return _cache.DisplayMetadata; + return _details.DisplayMetadata; } } @@ -104,14 +104,14 @@ namespace Microsoft.AspNet.Mvc.ModelBinding.Metadata { get { - if (_cache.ValidationMetadata == null) + if (_details.ValidationMetadata == null) { - var context = new ValidationMetadataProviderContext(Identity, _cache.Attributes); + var context = new ValidationMetadataProviderContext(Identity, _details.Attributes); _detailsProvider.GetValidationMetadata(context); - _cache.ValidationMetadata = context.ValidationMetadata; + _details.ValidationMetadata = context.ValidationMetadata; } - return _cache.ValidationMetadata; + return _details.ValidationMetadata; } } @@ -286,7 +286,7 @@ namespace Microsoft.AspNet.Mvc.ModelBinding.Metadata } else { - _isReadOnly = _cache.PropertySetter != null; + _isReadOnly = _details.PropertySetter != null; } } diff --git a/src/Microsoft.AspNet.Mvc.ModelBinding/Metadata/DefaultModelMetadataProvider.cs b/src/Microsoft.AspNet.Mvc.ModelBinding/Metadata/DefaultModelMetadataProvider.cs index a27e563988..8ec2c57934 100644 --- a/src/Microsoft.AspNet.Mvc.ModelBinding/Metadata/DefaultModelMetadataProvider.cs +++ b/src/Microsoft.AspNet.Mvc.ModelBinding/Metadata/DefaultModelMetadataProvider.cs @@ -15,7 +15,7 @@ namespace Microsoft.AspNet.Mvc.ModelBinding.Metadata public class DefaultModelMetadataProvider : IModelMetadataProvider { private readonly TypeCache _typeCache = new TypeCache(); - private readonly PropertiesCache _propertiesCache = new PropertiesCache(); + private readonly Func _cacheEntryFactory; /// /// Creates a new . @@ -24,6 +24,8 @@ namespace Microsoft.AspNet.Mvc.ModelBinding.Metadata public DefaultModelMetadataProvider(ICompositeMetadataDetailsProvider detailsProvider) { DetailsProvider = detailsProvider; + + _cacheEntryFactory = CreateCacheEntry; } /// @@ -36,15 +38,24 @@ namespace Microsoft.AspNet.Mvc.ModelBinding.Metadata { var key = ModelMetadataIdentity.ForType(modelType); - var propertyEntries = _propertiesCache.GetOrAdd(key, CreatePropertyCacheEntries); + var cacheEntry = _typeCache.GetOrAdd(key, _cacheEntryFactory); - var properties = new ModelMetadata[propertyEntries.Length]; - for (var i = 0; i < properties.Length; i++) + // We're relying on a safe race-condition for Properties - take care only + // to set the value onces the properties are fully-initialized. + if (cacheEntry.Details.Properties == null) { - properties[i] = CreateModelMetadata(propertyEntries[i]); + var propertyDetails = CreatePropertyDetails(key); + + var properties = new ModelMetadata[propertyDetails.Length]; + for (var i = 0; i < properties.Length; i++) + { + properties[i] = CreateModelMetadata(propertyDetails[i]); + } + + cacheEntry.Details.Properties = properties; } - return properties; + return cacheEntry.Details.Properties; } /// @@ -52,85 +63,63 @@ namespace Microsoft.AspNet.Mvc.ModelBinding.Metadata { var key = ModelMetadataIdentity.ForType(modelType); - var entry = _typeCache.GetOrAdd(key, CreateTypeCacheEntry); - return CreateModelMetadata(entry); + var cacheEntry = _typeCache.GetOrAdd(key, _cacheEntryFactory); + return cacheEntry.Metadata; + } + + private ModelMetadataCacheEntry CreateCacheEntry(ModelMetadataIdentity key) + { + var details = CreateTypeDetails(key); + var metadata = CreateModelMetadata(details); + return new ModelMetadataCacheEntry(metadata, details); } /// - /// Creates a new from a . + /// Creates a new from a . /// - /// The entry with cached data. + /// The entry with cached data. /// A new instance. /// /// will always create instances of /// .Override this method to create a /// of a different concrete type. /// - protected virtual ModelMetadata CreateModelMetadata(DefaultMetadataDetailsCache entry) + protected virtual ModelMetadata CreateModelMetadata(DefaultMetadataDetails entry) { return new DefaultModelMetadata(this, DetailsProvider, entry); } /// - /// Creates the entries for the properties of a model + /// Creates the entries for the properties of a model /// . /// /// /// The identifying the model . /// - /// A cache object for each property of the model . + /// A details object for each property of the model . /// /// The results of this method will be cached and used to satisfy calls to /// . Override this method to provide a different /// set of property data. /// - protected virtual DefaultMetadataDetailsCache[] CreatePropertyCacheEntries([NotNull] ModelMetadataIdentity key) + protected virtual DefaultMetadataDetails[] CreatePropertyDetails([NotNull] ModelMetadataIdentity key) { - var propertyHelpers = PropertyHelper.GetProperties(key.ModelType); + var propertyHelpers = PropertyHelper.GetVisibleProperties(key.ModelType); - var propertyEntries = new List(propertyHelpers.Length); + var propertyEntries = new List(propertyHelpers.Length); for (var i = 0; i < propertyHelpers.Length; i++) { var propertyHelper = propertyHelpers[i]; - if (propertyHelper.Property.DeclaringType != key.ModelType) - { - // If this property was declared on a base type then look for the definition closest to the - // the model type to see if we should include it. - var ignoreProperty = false; - - // Walk up the hierarchy until we find the type that actally declares this - // PropertyInfo. - var currentType = key.ModelType.GetTypeInfo(); - while (currentType != propertyHelper.Property.DeclaringType.GetTypeInfo()) - { - // We've found a 'more proximal' public definition - var declaredProperty = currentType.GetDeclaredProperty(propertyHelper.Name); - if (declaredProperty != null) - { - ignoreProperty = true; - break; - } - - currentType = currentType.BaseType.GetTypeInfo(); - } - - if (ignoreProperty) - { - // There's a better definition, ignore this. - continue; - } - } - var propertyKey = ModelMetadataIdentity.ForProperty( propertyHelper.Property.PropertyType, propertyHelper.Name, key.ModelType); var attributes = new List(ModelAttributes.GetAttributesForProperty( - key.ModelType, + key.ModelType, propertyHelper.Property)); - var propertyEntry = new DefaultMetadataDetailsCache(propertyKey, attributes); + var propertyEntry = new DefaultMetadataDetails(propertyKey, attributes); if (propertyHelper.Property.CanRead && propertyHelper.Property.GetMethod?.IsPrivate == true) { propertyEntry.PropertyAccessor = PropertyHelper.MakeFastPropertyGetter(propertyHelper.Property); @@ -148,24 +137,24 @@ namespace Microsoft.AspNet.Mvc.ModelBinding.Metadata } /// - /// Creates the entry for a model . + /// Creates the entry for a model . /// /// /// The identifying the model . /// - /// A cache object for the model . + /// A details object for the model . /// /// The results of this method will be cached and used to satisfy calls to /// . Override this method to provide a different /// set of attributes. /// - protected virtual DefaultMetadataDetailsCache CreateTypeCacheEntry([NotNull] ModelMetadataIdentity key) + protected virtual DefaultMetadataDetails CreateTypeDetails([NotNull] ModelMetadataIdentity key) { var attributes = new List(ModelAttributes.GetAttributesForType(key.ModelType)); - return new DefaultMetadataDetailsCache(key, attributes); + return new DefaultMetadataDetails(key, attributes); } - private class TypeCache : ConcurrentDictionary + private class TypeCache : ConcurrentDictionary { public TypeCache() : base(ModelMetadataIdentityComparer.Instance) @@ -173,12 +162,17 @@ namespace Microsoft.AspNet.Mvc.ModelBinding.Metadata } } - private class PropertiesCache : ConcurrentDictionary + private struct ModelMetadataCacheEntry { - public PropertiesCache() - : base(ModelMetadataIdentityComparer.Instance) + public ModelMetadataCacheEntry(ModelMetadata metadata, DefaultMetadataDetails details) { + Metadata = metadata; + Details = details; } + + public ModelMetadata Metadata { get; private set; } + + public DefaultMetadataDetails Details { get; private set; } } private class ModelMetadataIdentityComparer : IEqualityComparer diff --git a/test/Microsoft.AspNet.Mvc.ModelBinding.Test/Metadata/DefaultModelMetadataProviderTest.cs b/test/Microsoft.AspNet.Mvc.ModelBinding.Test/Metadata/DefaultModelMetadataProviderTest.cs index 885a096849..f026a16879 100644 --- a/test/Microsoft.AspNet.Mvc.ModelBinding.Test/Metadata/DefaultModelMetadataProviderTest.cs +++ b/test/Microsoft.AspNet.Mvc.ModelBinding.Test/Metadata/DefaultModelMetadataProviderTest.cs @@ -26,7 +26,6 @@ namespace Microsoft.AspNet.Mvc.ModelBinding.Metadata Assert.Equal("OnType", attribute.Value); } - // The attributes and other 'details' are cached [Fact] public void GetMetadataForType_Cached() { @@ -38,6 +37,7 @@ namespace Microsoft.AspNet.Mvc.ModelBinding.Metadata var metadata2 = Assert.IsType(provider.GetMetadataForType(typeof(ModelType))); // Assert + Assert.Same(metadata1, metadata2); Assert.Same(metadata1.Attributes, metadata2.Attributes); Assert.Same(metadata1.BindingMetadata, metadata2.BindingMetadata); Assert.Same(metadata1.DisplayMetadata, metadata2.DisplayMetadata); @@ -80,19 +80,35 @@ namespace Microsoft.AspNet.Mvc.ModelBinding.Metadata var provider = CreateProvider(); // Act - var metadata1 = provider.GetMetadataForProperties(typeof(ModelType)).Cast().ToArray(); - var metadata2 = provider.GetMetadataForProperties(typeof(ModelType)).Cast().ToArray(); + var properties1 = provider.GetMetadataForProperties(typeof(ModelType)).Cast().ToArray(); + var properties2 = provider.GetMetadataForProperties(typeof(ModelType)).Cast().ToArray(); // Assert - for (var i = 0; i < metadata1.Length; i++) + Assert.Equal(properties1.Length, properties2.Length); + for (var i = 0; i < properties1.Length; i++) { - Assert.Same(metadata1[i].Attributes, metadata2[i].Attributes); - Assert.Same(metadata1[i].BindingMetadata, metadata2[i].BindingMetadata); - Assert.Same(metadata1[i].DisplayMetadata, metadata2[i].DisplayMetadata); - Assert.Same(metadata1[i].ValidationMetadata, metadata2[i].ValidationMetadata); + Assert.Same(properties1[i], properties2[i]); + Assert.Same(properties1[i].Attributes, properties2[i].Attributes); + Assert.Same(properties1[i].BindingMetadata, properties2[i].BindingMetadata); + Assert.Same(properties1[i].DisplayMetadata, properties2[i].DisplayMetadata); + Assert.Same(properties1[i].ValidationMetadata, properties2[i].ValidationMetadata); } } + [Fact] + public void GetMetadataForType_PropertiesCollection_Cached() + { + // Arrange + var provider = CreateProvider(); + + // Act + var metadata1 = Assert.IsType(provider.GetMetadataForType(typeof(ModelType))); + var metadata2 = Assert.IsType(provider.GetMetadataForType(typeof(ModelType))); + + // Assert + Assert.Same(metadata1.Properties, metadata2.Properties); + } + [Fact] public void GetMetadataForProperties_IncludesMergedAttributes() { diff --git a/test/Microsoft.AspNet.Mvc.ModelBinding.Test/Metadata/DefaultModelMetadataTest.cs b/test/Microsoft.AspNet.Mvc.ModelBinding.Test/Metadata/DefaultModelMetadataTest.cs index b1c0bba34f..3f05f2224b 100644 --- a/test/Microsoft.AspNet.Mvc.ModelBinding.Test/Metadata/DefaultModelMetadataTest.cs +++ b/test/Microsoft.AspNet.Mvc.ModelBinding.Test/Metadata/DefaultModelMetadataTest.cs @@ -23,7 +23,7 @@ namespace Microsoft.AspNet.Mvc.ModelBinding.Metadata Enumerable.Empty()); var key = ModelMetadataIdentity.ForType(typeof(string)); - var cache = new DefaultMetadataDetailsCache(key, new object[0]); + var cache = new DefaultMetadataDetails(key, new object[0]); // Act var metadata = new DefaultModelMetadata(provider, detailsProvider, cache); @@ -76,7 +76,7 @@ namespace Microsoft.AspNet.Mvc.ModelBinding.Metadata Enumerable.Empty()); var key = ModelMetadataIdentity.ForType(typeof(Exception)); - var cache = new DefaultMetadataDetailsCache(key, new object[0]); + var cache = new DefaultMetadataDetails(key, new object[0]); // Act var metadata = new DefaultModelMetadata(provider, detailsProvider, cache); @@ -95,7 +95,7 @@ namespace Microsoft.AspNet.Mvc.ModelBinding.Metadata var detailsProvider = new EmptyCompositeMetadataDetailsProvider(); var key = ModelMetadataIdentity.ForProperty(typeof(string), "Message", typeof(Exception)); - var cache = new DefaultMetadataDetailsCache(key, new object[0]); + var cache = new DefaultMetadataDetails(key, new object[0]); // Act var metadata = new DefaultModelMetadata(provider, detailsProvider, cache); @@ -117,7 +117,7 @@ namespace Microsoft.AspNet.Mvc.ModelBinding.Metadata var detailsProvider = new EmptyCompositeMetadataDetailsProvider(); var key = ModelMetadataIdentity.ForType(modelType); - var cache = new DefaultMetadataDetailsCache(key, new object[0]); + var cache = new DefaultMetadataDetails(key, new object[0]); var metadata = new DefaultModelMetadata(provider, detailsProvider, cache); @@ -138,7 +138,7 @@ namespace Microsoft.AspNet.Mvc.ModelBinding.Metadata var detailsProvider = new EmptyCompositeMetadataDetailsProvider(); var key = ModelMetadataIdentity.ForType(modelType); - var cache = new DefaultMetadataDetailsCache(key, new object[0]); + var cache = new DefaultMetadataDetails(key, new object[0]); var metadata = new DefaultModelMetadata(provider, detailsProvider, cache); @@ -162,13 +162,13 @@ namespace Microsoft.AspNet.Mvc.ModelBinding.Metadata new DefaultModelMetadata( provider.Object, detailsProvider, - new DefaultMetadataDetailsCache( + new DefaultMetadataDetails( ModelMetadataIdentity.ForProperty(typeof(int), "Prop1", typeof(string)), attributes: null)), new DefaultModelMetadata( provider.Object, detailsProvider, - new DefaultMetadataDetailsCache( + new DefaultMetadataDetails( ModelMetadataIdentity.ForProperty(typeof(int), "Prop2", typeof(string)), attributes: null)), }; @@ -178,7 +178,7 @@ namespace Microsoft.AspNet.Mvc.ModelBinding.Metadata .Returns(expectedProperties); var key = ModelMetadataIdentity.ForType(typeof(string)); - var cache = new DefaultMetadataDetailsCache(key, new object[0]); + var cache = new DefaultMetadataDetails(key, new object[0]); var metadata = new DefaultModelMetadata(provider.Object, detailsProvider, cache); @@ -238,7 +238,7 @@ namespace Microsoft.AspNet.Mvc.ModelBinding.Metadata expectedProperties.Add(new DefaultModelMetadata( provider.Object, detailsProvider, - new DefaultMetadataDetailsCache( + new DefaultMetadataDetails( ModelMetadataIdentity.ForProperty(typeof(int), originalName, typeof(string)), attributes: null))); } @@ -248,7 +248,7 @@ namespace Microsoft.AspNet.Mvc.ModelBinding.Metadata .Returns(expectedProperties); var key = ModelMetadataIdentity.ForType(typeof(string)); - var cache = new DefaultMetadataDetailsCache(key, new object[0]); + var cache = new DefaultMetadataDetails(key, new object[0]); var metadata = new DefaultModelMetadata(provider.Object, detailsProvider, cache); @@ -338,7 +338,7 @@ namespace Microsoft.AspNet.Mvc.ModelBinding.Metadata var expectedProperties = new List(); foreach (var kvp in originalNamesAndOrders) { - var propertyCache = new DefaultMetadataDetailsCache( + var propertyCache = new DefaultMetadataDetails( ModelMetadataIdentity.ForProperty(typeof(int), kvp.Key, typeof(string)), attributes: null); @@ -356,7 +356,7 @@ namespace Microsoft.AspNet.Mvc.ModelBinding.Metadata .Returns(expectedProperties); var key = ModelMetadataIdentity.ForType(typeof(string)); - var cache = new DefaultMetadataDetailsCache(key, new object[0]); + var cache = new DefaultMetadataDetails(key, new object[0]); var metadata = new DefaultModelMetadata(provider.Object, detailsProvider, cache); @@ -377,7 +377,7 @@ namespace Microsoft.AspNet.Mvc.ModelBinding.Metadata var detailsProvider = new EmptyCompositeMetadataDetailsProvider(); var key = ModelMetadataIdentity.ForType(typeof(string)); - var cache = new DefaultMetadataDetailsCache(key, new object[0]); + var cache = new DefaultMetadataDetails(key, new object[0]); var metadata = new DefaultModelMetadata(provider, detailsProvider, cache); @@ -398,7 +398,7 @@ namespace Microsoft.AspNet.Mvc.ModelBinding.Metadata var detailsProvider = new EmptyCompositeMetadataDetailsProvider(); var key = ModelMetadataIdentity.ForType(typeof(string)); - var cache = new DefaultMetadataDetailsCache(key, new object[0]); + var cache = new DefaultMetadataDetails(key, new object[0]); var metadata = new DefaultModelMetadata(provider, detailsProvider, cache); diff --git a/test/Microsoft.AspNet.Mvc.ModelBinding.Test/Metadata/ModelMetadataProviderTest.cs b/test/Microsoft.AspNet.Mvc.ModelBinding.Test/Metadata/ModelMetadataProviderTest.cs index 8b7f231bfb..ca86e01f86 100644 --- a/test/Microsoft.AspNet.Mvc.ModelBinding.Test/Metadata/ModelMetadataProviderTest.cs +++ b/test/Microsoft.AspNet.Mvc.ModelBinding.Test/Metadata/ModelMetadataProviderTest.cs @@ -838,10 +838,10 @@ namespace Microsoft.AspNet.Mvc.ModelBinding.Metadata _attributes = attributes; } - protected override DefaultMetadataDetailsCache CreateTypeCacheEntry(ModelMetadataIdentity key) + protected override DefaultMetadataDetails CreateTypeDetails([NotNull]ModelMetadataIdentity key) { - var entry = base.CreateTypeCacheEntry(key); - return new DefaultMetadataDetailsCache(key, _attributes.Concat(entry.Attributes).ToArray()); + var entry = base.CreateTypeDetails(key); + return new DefaultMetadataDetails(key, _attributes.Concat(entry.Attributes).ToArray()); } } }