huahaofeng
/
aspnetcore
1
0
Fork 0
Code Issues Pull Requests Projects Releases Wiki Activity

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.
This commit is contained in:
Ryan Nowak 2015-12-08 14:21:24 -08:00
parent c6f8ced9a2
commit cf6662d0c3
3 changed files with 99 additions and 93 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

115
src/Microsoft.AspNet.Mvc.Abstractions/ModelBinding/ModelMetadata.cs
View File

@ -18,8 +18,6 @@ namespace Microsoft.AspNet.Mvc.ModelBinding
[DebuggerDisplay("{DebuggerToString(),nq}")]
public abstract class ModelMetadata
{
private bool? _isComplexType;
/// <summary>
/// The default value of <see cref="ModelMetadata.Order"/>.
/// </summary>
@ -32,6 +30,8 @@ namespace Microsoft.AspNet.Mvc.ModelBinding
protected ModelMetadata(ModelMetadataIdentity identity)
{
Identity = identity;
InitializeTypeInformation();
}
/// <summary>
@ -294,6 +294,12 @@ namespace Microsoft.AspNet.Mvc.ModelBinding
/// </summary>
public abstract IReadOnlyList<object> ValidatorMetadata { get; }
/// <summary>
/// Gets the <see cref="Type"/> for elements of <see cref="ModelType"/> if that <see cref="Type"/>
/// implements <see cref="IEnumerable"/>.
/// </summary>
public Type ElementType { get; private set; }
/// <summary>
/// Gets a value indicating whether <see cref="ModelType"/> is a simple type.
/// </summary>
@ -301,29 +307,12 @@ namespace Microsoft.AspNet.Mvc.ModelBinding
/// A simple type is defined as a <see cref="Type"/> which has a
/// <see cref="System.ComponentModel.TypeConverter"/> that can convert from <see cref="string"/>.
/// </remarks>
public bool IsComplexType
{
get
{
if (_isComplexType == null)
{
_isComplexType = !TypeDescriptor.GetConverter(ModelType).CanConvertFrom(typeof(string));
}
return _isComplexType.Value;
}
}
public bool IsComplexType { get; private set; }
/// <summary>
/// Gets a value indicating whether or not <see cref="ModelType"/> is a <see cref="Nullable{T}"/>.
/// </summary>
public bool IsNullableValueType
{
get
{
return Nullable.GetUnderlyingType(ModelType) != null;
}
}
public bool IsNullableValueType { get; private set; }
/// <summary>
/// Gets a value indicating whether or not <see cref="ModelType"/> is a collection type.
@ -331,17 +320,7 @@ namespace Microsoft.AspNet.Mvc.ModelBinding
/// <remarks>
/// A collection type is defined as a <see cref="Type"/> which is assignable to <see cref="ICollection{T}"/>.
/// </remarks>
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; }
/// <summary>
/// Gets a value indicating whether or not <see cref="ModelType"/> is an enumerable type.
@ -350,32 +329,12 @@ namespace Microsoft.AspNet.Mvc.ModelBinding
/// An enumerable type is defined as a <see cref="Type"/> which is assignable to
/// <see cref="IEnumerable"/>, and is not a <see cref="string"/>.
/// </remarks>
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<T> extends it.
return typeof(IEnumerable).IsAssignableFrom(ModelType);
}
}
public bool IsEnumerableType { get; private set; }
/// <summary>
/// Gets a value indicating whether or not <see cref="ModelType"/> allows <c>null</c> values.
/// </summary>
public bool IsReferenceOrNullableType
{
get
{
return !ModelType.GetTypeInfo().IsValueType || IsNullableValueType;
}
}
public bool IsReferenceOrNullableType { get; private set; }
/// <summary>
/// Gets the underlying type argument if <see cref="ModelType"/> inherits from <see cref="Nullable{T}"/>.
@ -384,13 +343,7 @@ namespace Microsoft.AspNet.Mvc.ModelBinding
/// <remarks>
/// Identical to <see cref="ModelType"/> unless <see cref="IsNullableValueType"/> is <c>true</c>.
/// </remarks>
public Type UnderlyingOrModelType
{
get
{
return Nullable.GetUnderlyingType(ModelType) ?? ModelType;
}
}
public Type UnderlyingOrModelType { get; private set; }
/// <summary>
/// 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<T>.
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)

34
src/Microsoft.AspNet.Mvc.Core/ModelBinding/Metadata/DefaultModelMetadata.cs
View File

@ -25,7 +25,6 @@ namespace Microsoft.AspNet.Mvc.ModelBinding.Metadata
private ReadOnlyDictionary<object, object> _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<T>.
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;

43
test/Microsoft.AspNet.Mvc.Abstractions.Test/ModelBinding/ModelMetadataTest.cs
View File

@ -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<string>), typeof(string))]
[InlineData(typeof(DerivedList), typeof(int))]
[InlineData(typeof(IEnumerable), typeof(object))]
[InlineData(typeof(IEnumerable<string>), typeof(string))]
[InlineData(typeof(Collection<int>), typeof(int))]
[InlineData(typeof(Dictionary<object, object>), typeof(KeyValuePair<object, object>))]
[InlineData(typeof(DerivedDictionary), typeof(KeyValuePair<string, int>))]
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<string, int>
{
}
}
}