From cf6662d0c3f7d764089a3938f9fb3a21bf25f55d Mon Sep 17 00:00:00 2001 From: Ryan Nowak Date: Tue, 8 Dec 2015 14:21:24 -0800 Subject: [PATCH] Partial fix for #3676 - fix race in ElementMetadata The accessor for ElementMetadata can sometimes return null when concurrently accessed. This change solves this issue and simplified a bunch of lazy-computed properties, by precomputing all type-related facets of ModelMetadata. This also adds a ElementType property to ModelMetadata to maintain the current separation of concerns as ModelMetadata is unaware of the metadata provider. --- .../ModelBinding/ModelMetadata.cs | 115 ++++++++---------- .../Metadata/DefaultModelMetadata.cs | 34 +----- .../ModelBinding/ModelMetadataTest.cs | 43 +++++++ 3 files changed, 99 insertions(+), 93 deletions(-) diff --git a/src/Microsoft.AspNet.Mvc.Abstractions/ModelBinding/ModelMetadata.cs b/src/Microsoft.AspNet.Mvc.Abstractions/ModelBinding/ModelMetadata.cs index 6da18ea735..3dd1e7940d 100644 --- a/src/Microsoft.AspNet.Mvc.Abstractions/ModelBinding/ModelMetadata.cs +++ b/src/Microsoft.AspNet.Mvc.Abstractions/ModelBinding/ModelMetadata.cs @@ -18,8 +18,6 @@ namespace Microsoft.AspNet.Mvc.ModelBinding [DebuggerDisplay("{DebuggerToString(),nq}")] public abstract class ModelMetadata { - private bool? _isComplexType; - /// /// The default value of . /// @@ -32,6 +30,8 @@ namespace Microsoft.AspNet.Mvc.ModelBinding protected ModelMetadata(ModelMetadataIdentity identity) { Identity = identity; + + InitializeTypeInformation(); } /// @@ -294,6 +294,12 @@ namespace Microsoft.AspNet.Mvc.ModelBinding /// public abstract IReadOnlyList ValidatorMetadata { get; } + /// + /// Gets the for elements of if that + /// implements . + /// + public Type ElementType { get; private set; } + /// /// Gets a value indicating whether is a simple type. /// @@ -301,29 +307,12 @@ namespace Microsoft.AspNet.Mvc.ModelBinding /// A simple type is defined as a which has a /// that can convert from . /// - public bool IsComplexType - { - get - { - if (_isComplexType == null) - { - _isComplexType = !TypeDescriptor.GetConverter(ModelType).CanConvertFrom(typeof(string)); - } - - return _isComplexType.Value; - } - } + public bool IsComplexType { get; private set; } /// /// Gets a value indicating whether or not is a . /// - public bool IsNullableValueType - { - get - { - return Nullable.GetUnderlyingType(ModelType) != null; - } - } + public bool IsNullableValueType { get; private set; } /// /// Gets a value indicating whether or not is a collection type. @@ -331,17 +320,7 @@ namespace Microsoft.AspNet.Mvc.ModelBinding /// /// 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; - } - } + public bool IsCollectionType { get; private set; } /// /// Gets a value indicating whether or not is an enumerable type. @@ -350,32 +329,12 @@ namespace Microsoft.AspNet.Mvc.ModelBinding /// An enumerable type is defined as a which is assignable to /// , and is not a . /// - public bool IsEnumerableType - { - get - { - if (ModelType == typeof(string)) - { - // Even though string implements IEnumerable, we don't really think of it - // as a collection for the purposes of model binding. - return false; - } - - // We only need to look for IEnumerable, because IEnumerable extends it. - return typeof(IEnumerable).IsAssignableFrom(ModelType); - } - } + public bool IsEnumerableType { get; private set; } /// /// Gets a value indicating whether or not allows null values. /// - public bool IsReferenceOrNullableType - { - get - { - return !ModelType.GetTypeInfo().IsValueType || IsNullableValueType; - } - } + public bool IsReferenceOrNullableType { get; private set; } /// /// Gets the underlying type argument if inherits from . @@ -384,13 +343,7 @@ namespace Microsoft.AspNet.Mvc.ModelBinding /// /// Identical to unless is true. /// - public Type UnderlyingOrModelType - { - get - { - return Nullable.GetUnderlyingType(ModelType) ?? ModelType; - } - } + public Type UnderlyingOrModelType { get; private set; } /// /// Gets a property getter delegate to get the property value from a model object. @@ -415,6 +368,46 @@ namespace Microsoft.AspNet.Mvc.ModelBinding return DisplayName ?? PropertyName ?? ModelType.Name; } + private void InitializeTypeInformation() + { + Debug.Assert(ModelType != null); + + IsComplexType = !TypeDescriptor.GetConverter(ModelType).CanConvertFrom(typeof(string)); + IsNullableValueType = Nullable.GetUnderlyingType(ModelType) != null; + IsReferenceOrNullableType = !ModelType.GetTypeInfo().IsValueType || IsNullableValueType; + UnderlyingOrModelType = Nullable.GetUnderlyingType(ModelType) ?? ModelType; + + var collectionType = ClosedGenericMatcher.ExtractGenericInterface(ModelType, typeof(ICollection<>)); + IsCollectionType = collectionType != null; + + if (ModelType == typeof(string) || !typeof(IEnumerable).IsAssignableFrom(ModelType)) + { + // Do nothing, not Enumerable. + } + else if (ModelType.IsArray) + { + IsEnumerableType = true; + ElementType = ModelType.GetElementType(); + } + else + { + IsEnumerableType = true; + + var enumerableType = ClosedGenericMatcher.ExtractGenericInterface(ModelType, typeof(IEnumerable<>)); + ElementType = enumerableType?.GenericTypeArguments[0]; + + if (ElementType == null && typeof(IEnumerable).IsAssignableFrom(ModelType)) + { + // ModelType implements IEnumerable but not IEnumerable. + ElementType = typeof(object); + } + + Debug.Assert( + ElementType != null, + $"Unable to find element type for '{ModelType.FullName}' though IsEnumerableType is true."); + } + } + private string DebuggerToString() { if (Identity.MetadataKind == ModelMetadataKind.Type) diff --git a/src/Microsoft.AspNet.Mvc.Core/ModelBinding/Metadata/DefaultModelMetadata.cs b/src/Microsoft.AspNet.Mvc.Core/ModelBinding/Metadata/DefaultModelMetadata.cs index 091990a83b..57de7ad847 100644 --- a/src/Microsoft.AspNet.Mvc.Core/ModelBinding/Metadata/DefaultModelMetadata.cs +++ b/src/Microsoft.AspNet.Mvc.Core/ModelBinding/Metadata/DefaultModelMetadata.cs @@ -25,7 +25,6 @@ namespace Microsoft.AspNet.Mvc.ModelBinding.Metadata private ReadOnlyDictionary _additionalValues; private ModelMetadata _elementMetadata; - private bool _haveCalculatedElementMetadata; private bool? _isBindingRequired; private bool? _isReadOnly; private bool? _isRequired; @@ -248,38 +247,9 @@ namespace Microsoft.AspNet.Mvc.ModelBinding.Metadata { get { - if (!_haveCalculatedElementMetadata) + if (_elementMetadata == null && ElementType != null) { - _haveCalculatedElementMetadata = true; - if (!IsEnumerableType) - { - // Short-circuit checks below. If not IsEnumerableType, ElementMetadata is null. - // For example, as in IsEnumerableType, do not consider strings collections. - return null; - } - - Type elementType = null; - if (ModelType.IsArray) - { - elementType = ModelType.GetElementType(); - } - else - { - elementType = ClosedGenericMatcher.ExtractGenericInterface(ModelType, typeof(IEnumerable<>)) - ?.GenericTypeArguments[0]; - if (elementType == null && typeof(IEnumerable).IsAssignableFrom(ModelType)) - { - // ModelType implements IEnumerable but not IEnumerable. - elementType = typeof(object); - } - } - - Debug.Assert( - elementType != null, - $"Unable to find element type for '{ ModelType.FullName }' though IsEnumerableType is true."); - - // Success - _elementMetadata = _provider.GetMetadataForType(elementType); + _elementMetadata = _provider.GetMetadataForType(ElementType); } return _elementMetadata; diff --git a/test/Microsoft.AspNet.Mvc.Abstractions.Test/ModelBinding/ModelMetadataTest.cs b/test/Microsoft.AspNet.Mvc.Abstractions.Test/ModelBinding/ModelMetadataTest.cs index 8e6b1d8ed2..60cca2c443 100644 --- a/test/Microsoft.AspNet.Mvc.Abstractions.Test/ModelBinding/ModelMetadataTest.cs +++ b/test/Microsoft.AspNet.Mvc.Abstractions.Test/ModelBinding/ModelMetadataTest.cs @@ -217,6 +217,45 @@ namespace Microsoft.AspNet.Mvc.ModelBinding Assert.Equal(expected, modelMetadata.UnderlyingOrModelType); } + [Theory] + [InlineData(typeof(object))] + [InlineData(typeof(int))] + [InlineData(typeof(NonCollectionType))] + [InlineData(typeof(string))] + public void ElementType_ReturnsNull_ForNonCollections(Type modelType) + { + // Arrange + var metadata = new TestModelMetadata(modelType); + + // Act + var elementType = metadata.ElementType; + + // Assert + Assert.Null(elementType); + } + + [Theory] + [InlineData(typeof(int[]), typeof(int))] + [InlineData(typeof(List), typeof(string))] + [InlineData(typeof(DerivedList), typeof(int))] + [InlineData(typeof(IEnumerable), typeof(object))] + [InlineData(typeof(IEnumerable), typeof(string))] + [InlineData(typeof(Collection), typeof(int))] + [InlineData(typeof(Dictionary), typeof(KeyValuePair))] + [InlineData(typeof(DerivedDictionary), typeof(KeyValuePair))] + public void ElementType_ReturnsExpectedMetadata(Type modelType, Type expected) + { + // Arrange + var metadata = new TestModelMetadata(modelType); + + // Act + var elementType = metadata.ElementType; + + // Assert + Assert.NotNull(elementType); + Assert.Equal(expected, elementType); + } + // GetDisplayName() [Fact] @@ -607,5 +646,9 @@ namespace Microsoft.AspNet.Mvc.ModelBinding throw new NotImplementedException(); } } + + private class DerivedDictionary : Dictionary + { + } } }