Skip to content

Commit

Permalink
Stop using Property to cache type mappings
Browse files Browse the repository at this point in the history 
Also, stop providers from using Property directly when determining a type mapping so that providers build type mappings with the same information that is used to cache them.

Fixes #11358
  • Loading branch information
@ajcvickers
ajcvickers committed Mar 26, 2018
1 parent bc7cc6a commit c7d8f26
Show file tree
Hide file tree
Showing 5 changed files with 99 additions and 68 deletions.
21 changes: 19 additions & 2 deletions src/EFCore.Relational/Storage/Internal/FallbackRelationalTypeMappingSource.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@

using System;
using JetBrains.Annotations;
using Microsoft.EntityFrameworkCore.Metadata;
using Microsoft.EntityFrameworkCore.Utilities;

namespace Microsoft.EntityFrameworkCore.Storage.Internal
Expand All @@ -17,6 +18,9 @@ public class FallbackRelationalTypeMappingSource : RelationalTypeMappingSource
private readonly IRelationalTypeMapper _relationalTypeMapper;
#pragma warning restore 618

[ThreadStatic]
private static IProperty _property;

/// <summary>
/// This API supports the Entity Framework Core infrastructure and is not intended to be used
/// directly from your code. This API may change or be removed in future releases.
Expand All @@ -32,6 +36,19 @@ public FallbackRelationalTypeMappingSource(
_relationalTypeMapper = typeMapper;
}

/// <summary>
/// This API supports the Entity Framework Core infrastructure and is not intended to be used
/// directly from your code. This API may change or be removed in future releases.
/// </summary>
protected override CoreTypeMapping FindMappingWithConversion(
TypeMappingInfo mappingInfo,
IProperty property)
{
_property = property;

return base.FindMappingWithConversion(mappingInfo, property);
}

/// <summary>
/// This API supports the Entity Framework Core infrastructure and is not intended to be used
/// directly from your code. This API may change or be removed in future releases.
Expand Down Expand Up @@ -67,8 +84,8 @@ protected override RelationalTypeMapping FindMapping(RelationalTypeMappingInfo m
}

private RelationalTypeMapping FindMappingForProperty(RelationalTypeMappingInfo mappingInfo)
=> mappingInfo.Property != null
? _relationalTypeMapper.FindMapping(mappingInfo.Property)
=> _property != null
? _relationalTypeMapper.FindMapping(_property)
: null;

private RelationalTypeMapping FindMappingForClrType(RelationalTypeMappingInfo mappingInfo)
Expand Down
10 changes: 5 additions & 5 deletions src/EFCore.Relational/Storage/RelationalTypeMappingSource.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,7 @@ protected override CoreTypeMapping FindMapping(TypeMappingInfo mappingInfo)
/// <returns> The type mapping, or <c>null</c> if none was found. </returns>
public override CoreTypeMapping FindMapping(IProperty property)
=> property.FindRelationalMapping()
?? FindMappingWithConversion(new ConcreteRelationalTypeMappingInfo(property));
?? FindMappingWithConversion(new ConcreteRelationalTypeMappingInfo(property), property);

/// <summary>
/// <para>
Expand All @@ -95,7 +95,7 @@ public override CoreTypeMapping FindMapping(IProperty property)
/// <param name="type"> The CLR type. </param>
/// <returns> The type mapping, or <c>null</c> if none was found. </returns>
public override CoreTypeMapping FindMapping(Type type)
=> FindMappingWithConversion(new ConcreteRelationalTypeMappingInfo(type));
=> FindMappingWithConversion(new ConcreteRelationalTypeMappingInfo(type), null);

/// <summary>
/// <para>
Expand All @@ -113,7 +113,7 @@ public override CoreTypeMapping FindMapping(Type type)
/// <param name="member"> The field or property. </param>
/// <returns> The type mapping, or <c>null</c> if none was found. </returns>
public override CoreTypeMapping FindMapping(MemberInfo member)
=> FindMappingWithConversion(new ConcreteRelationalTypeMappingInfo(member));
=> FindMappingWithConversion(new ConcreteRelationalTypeMappingInfo(member), null);

/// <summary>
/// <para>
Expand All @@ -130,7 +130,7 @@ public override CoreTypeMapping FindMapping(MemberInfo member)
/// <param name="storeTypeName"> The database type name. </param>
/// <returns> The type mapping, or <c>null</c> if none was found. </returns>
public virtual RelationalTypeMapping FindMapping(string storeTypeName)
=> (RelationalTypeMapping)FindMappingWithConversion(new ConcreteRelationalTypeMappingInfo(storeTypeName));
=> (RelationalTypeMapping)FindMappingWithConversion(new ConcreteRelationalTypeMappingInfo(storeTypeName), null);

/// <summary>
/// <para>
Expand Down Expand Up @@ -164,7 +164,7 @@ public virtual RelationalTypeMapping FindMapping(
int? scale = null)
=> (RelationalTypeMapping)FindMappingWithConversion(
new ConcreteRelationalTypeMappingInfo(
type, keyOrIndex, unicode, size, rowVersion, fixedLength, precision, scale));
type, keyOrIndex, unicode, size, rowVersion, fixedLength, precision, scale), null);

RelationalTypeMapping IRelationalTypeMappingSource.FindMapping(IProperty property)
=> (RelationalTypeMapping)FindMapping(property);
Expand Down
25 changes: 14 additions & 11 deletions src/EFCore.SqlServer/Storage/Internal/SqlServerTypeMappingSource.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
using System.Linq;
using JetBrains.Annotations;
using Microsoft.EntityFrameworkCore.ChangeTracking;
using Microsoft.EntityFrameworkCore.Metadata;
using Microsoft.EntityFrameworkCore.SqlServer.Internal;
using Microsoft.EntityFrameworkCore.Storage;

Expand Down Expand Up @@ -204,26 +205,28 @@ public SqlServerTypeMappingSource(
/// This API supports the Entity Framework Core infrastructure and is not intended to be used
/// directly from your code. This API may change or be removed in future releases.
/// </summary>
protected override RelationalTypeMapping FindMapping(RelationalTypeMappingInfo mappingInfo)
protected override void ValidateMapping(CoreTypeMapping mapping, IProperty property)
{
var mapping = FindRawMapping(mappingInfo)?.Clone(mappingInfo);
var relationalMapping = mapping as RelationalTypeMapping;

if (_disallowedMappings.Contains(mapping?.StoreType))
if (_disallowedMappings.Contains(relationalMapping?.StoreType))
{
var propertyName = mappingInfo.Property?.Name
?? mappingInfo.MemberInfo?.Name;

if (propertyName == null)
if (property== null)
{
throw new ArgumentException(SqlServerStrings.UnqualifiedDataType(mapping.StoreType));
throw new ArgumentException(SqlServerStrings.UnqualifiedDataType(relationalMapping.StoreType));
}

throw new ArgumentException(SqlServerStrings.UnqualifiedDataTypeOnProperty(mapping.StoreType, propertyName));
throw new ArgumentException(SqlServerStrings.UnqualifiedDataTypeOnProperty(relationalMapping.StoreType, property.Name));
}

return mapping;
}

/// <summary>
/// This API supports the Entity Framework Core infrastructure and is not intended to be used
/// directly from your code. This API may change or be removed in future releases.
/// </summary>
protected override RelationalTypeMapping FindMapping(RelationalTypeMappingInfo mappingInfo)
=> FindRawMapping(mappingInfo)?.Clone(mappingInfo);

private RelationalTypeMapping FindRawMapping(RelationalTypeMappingInfo mappingInfo)
{
var clrType = mappingInfo.ClrType;
Expand Down
51 changes: 27 additions & 24 deletions src/EFCore/Storage/TypeMappingInfo.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,9 @@ namespace Microsoft.EntityFrameworkCore.Storage
/// </summary>
public abstract class TypeMappingInfo
{
private readonly Type _providerClrType;
private readonly ValueConverter _customConverter;

/// <summary>
/// Creates a new instance of <see cref="TypeMappingInfo" />.
/// </summary>
Expand All @@ -34,13 +37,24 @@ protected TypeMappingInfo([NotNull] IProperty property)

var principals = property.FindPrincipals().ToList();

Property = property;
MemberInfo = property.GetIdentifyingMemberInfo();
_providerClrType = principals
?.Select(p => p.GetProviderClrType())
.FirstOrDefault(t => t != null)
?.UnwrapNullableType();

_customConverter = principals
?.Select(p => p.GetValueConverter())
.FirstOrDefault(c => c != null);

var mappingHints = _customConverter?.MappingHints;

IsKeyOrIndex = property.IsKeyOrForeignKey() || property.IsIndex();
Size = principals.Select(p => p.GetMaxLength()).FirstOrDefault(t => t != null);
IsUnicode = principals.Select(p => p.IsUnicode()).FirstOrDefault(t => t != null);
Size = principals.Select(p => p.GetMaxLength()).FirstOrDefault(t => t != null) ?? mappingHints?.Size;
IsUnicode = principals.Select(p => p.IsUnicode()).FirstOrDefault(t => t != null) ?? mappingHints?.IsUnicode;
IsRowVersion = property.IsConcurrencyToken && property.ValueGenerated == ValueGenerated.OnAddOrUpdate;
ClrType = property.ClrType.UnwrapNullableType();
ClrType = (_customConverter?.ProviderClrType ?? property.ClrType).UnwrapNullableType();
Scale = mappingHints?.Scale;
Precision = mappingHints?.Precision;
}

/// <summary>
Expand All @@ -63,7 +77,6 @@ protected TypeMappingInfo([NotNull] MemberInfo member)
Check.NotNull(member, nameof(member));

ClrType = member.GetMemberType().UnwrapNullableType();
MemberInfo = member;
}

/// <summary>
Expand Down Expand Up @@ -105,10 +118,10 @@ protected TypeMappingInfo(
{
Check.NotNull(source, nameof(source));

Property = source.Property;
IsRowVersion = source.IsRowVersion;
IsKeyOrIndex = source.IsKeyOrIndex;
MemberInfo = source.MemberInfo;
_providerClrType = source._providerClrType;
_customConverter = source._customConverter;

var mappingHints = converter.MappingHints;

Expand All @@ -127,11 +140,6 @@ protected TypeMappingInfo(
/// <returns> The new mapping info. </returns>
public abstract TypeMappingInfo WithConverter(ValueConverterInfo converterInfo);

/// <summary>
/// The property for which mapping is needed.
/// </summary>
public virtual IProperty Property { get; }

/// <summary>
/// Indicates whether or not the mapping is part of a key or index.
/// </summary>
Expand Down Expand Up @@ -162,11 +170,6 @@ protected TypeMappingInfo(
/// </summary>
public virtual int? Scale { get; }

/// <summary>
/// The field or property info for the property.
/// </summary>
public virtual MemberInfo MemberInfo { get; }

/// <summary>
/// The CLR type in the model.
/// </summary>
Expand All @@ -178,9 +181,9 @@ protected TypeMappingInfo(
/// <param name="other"> The other object. </param>
/// <returns> <c>True</c> if they represent the same mapping; <c>false</c> otherwise. </returns>
protected virtual bool Equals([NotNull] TypeMappingInfo other)
=> Property == other.Property
&& ClrType == other.ClrType
&& MemberInfo == other.MemberInfo
=> ClrType == other.ClrType
&& _providerClrType == other._providerClrType
&& _customConverter == other._customConverter
&& IsKeyOrIndex == other.IsKeyOrIndex
&& Size == other.Size
&& IsUnicode == other.IsUnicode
Expand All @@ -205,11 +208,11 @@ public override bool Equals(object obj)
/// <returns> The hash code. </returns>
public override int GetHashCode()
{
var hashCode = Property?.GetHashCode() ?? 0;
hashCode = (hashCode * 397) ^ ClrType?.GetHashCode() ?? 0;
var hashCode = ClrType?.GetHashCode() ?? 0;
hashCode = (hashCode * 397) ^ (_providerClrType?.GetHashCode() ?? 0);
hashCode = (hashCode * 397) ^ (_customConverter?.GetHashCode() ?? 0);
hashCode = (hashCode * 397) ^ IsKeyOrIndex.GetHashCode();
hashCode = (hashCode * 397) ^ (Size?.GetHashCode() ?? 0);
hashCode = (hashCode * 397) ^ (MemberInfo?.GetHashCode() ?? 0);
hashCode = (hashCode * 397) ^ (IsUnicode?.GetHashCode() ?? 0);
hashCode = (hashCode * 397) ^ (IsRowVersion?.GetHashCode() ?? 0);
hashCode = (hashCode * 397) ^ (Scale?.GetHashCode() ?? 0);
Expand Down
60 changes: 34 additions & 26 deletions src/EFCore/Storage/TypeMappingSource.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,17 @@ protected TypeMappingSource([NotNull] TypeMappingSourceDependencies dependencies
/// <returns> The type mapping, or <c>null</c> if none could be found. </returns>
protected abstract CoreTypeMapping FindMapping([NotNull] TypeMappingInfo mappingInfo);

/// <summary>
/// Called after a mapping has been found so that it can be validated for the given property.
/// </summary>
/// <param name="mapping"> The mapping, if any. </param>
/// <param name="property"> The property, if any. </param>
protected virtual void ValidateMapping(
[CanBeNull] CoreTypeMapping mapping,
[CanBeNull] IProperty property)
{
}

/// <summary>
/// <para>
/// Uses any available <see cref="ValueConverter" /> to help find a mapping that works.
Expand All @@ -67,36 +78,29 @@ protected TypeMappingSource([NotNull] TypeMappingSourceDependencies dependencies
/// </para>
/// </summary>
/// <param name="mappingInfo"> The mapping info. </param>
/// <param name="property"> The property for which the type mapping is needed, if any. </param>
/// <returns> The type mapping with conversions applied, or <c>null</c> if none could be found. </returns>
protected virtual CoreTypeMapping FindMappingWithConversion([NotNull] TypeMappingInfo mappingInfo)
protected virtual CoreTypeMapping FindMappingWithConversion(
[NotNull] TypeMappingInfo mappingInfo,
[CanBeNull] IProperty property)
{
Check.NotNull(mappingInfo, nameof(mappingInfo));

return _explicitMappings.GetOrAdd(
mappingInfo,
k =>
{
var principals = mappingInfo.Property?.FindPrincipals().ToList();

var customConverter = principals
?.Select(p => p.GetValueConverter())
.FirstOrDefault(c => c != null);
var principals = property?.FindPrincipals().ToList();

if (customConverter != null)
{
mappingInfo = mappingInfo.WithConverter(
new ValueConverterInfo(
customConverter.ModelClrType,
customConverter.ProviderClrType,
i => customConverter,
customConverter.MappingHints));
}
var customConverter = principals
?.Select(p => p.GetValueConverter())
.FirstOrDefault(c => c != null);

var providerClrType = principals
?.Select(p => p.GetProviderClrType())
.FirstOrDefault(t => t != null)
?.UnwrapNullableType();
var providerClrType = principals
?.Select(p => p.GetProviderClrType())
.FirstOrDefault(t => t != null)
?.UnwrapNullableType();

var resolvedMapping = _explicitMappings.GetOrAdd(
mappingInfo,
k =>
{
var mapping = providerClrType == null
|| providerClrType == mappingInfo.ClrType
? FindMapping(mappingInfo)
Expand Down Expand Up @@ -149,6 +153,10 @@ protected virtual CoreTypeMapping FindMappingWithConversion([NotNull] TypeMappin

return mapping;
});

ValidateMapping(resolvedMapping, property);

return resolvedMapping;
}

/// <summary>
Expand All @@ -163,7 +171,7 @@ protected virtual CoreTypeMapping FindMappingWithConversion([NotNull] TypeMappin
/// <returns> The type mapping, or <c>null</c> if none was found. </returns>
public virtual CoreTypeMapping FindMapping(IProperty property)
=> property.FindMapping()
?? FindMappingWithConversion(new ConcreteTypeMappingInfo(property));
?? FindMappingWithConversion(new ConcreteTypeMappingInfo(property), property);

/// <summary>
/// <para>
Expand All @@ -181,7 +189,7 @@ public virtual CoreTypeMapping FindMapping(IProperty property)
/// <param name="type"> The CLR type. </param>
/// <returns> The type mapping, or <c>null</c> if none was found. </returns>
public virtual CoreTypeMapping FindMapping(Type type)
=> FindMappingWithConversion(new ConcreteTypeMappingInfo(type));
=> FindMappingWithConversion(new ConcreteTypeMappingInfo(type), null);

/// <summary>
/// <para>
Expand All @@ -199,7 +207,7 @@ public virtual CoreTypeMapping FindMapping(Type type)
/// <param name="member"> The field or property. </param>
/// <returns> The type mapping, or <c>null</c> if none was found. </returns>
public virtual CoreTypeMapping FindMapping(MemberInfo member)
=> FindMappingWithConversion(new ConcreteTypeMappingInfo(member));
=> FindMappingWithConversion(new ConcreteTypeMappingInfo(member), null);

private sealed class ConcreteTypeMappingInfo : TypeMappingInfo
{
Expand Down

0 comments on commit c7d8f26

Please sign in to comment.