From ed2ebbd66935b70b6af74f55882dd2cf8f1cb6be Mon Sep 17 00:00:00 2001 From: lajones Date: Wed, 7 Nov 2018 13:16:51 -0800 Subject: [PATCH] Updating to pass the PropertyInfo for the indexer when the PropertyBase is created and thus make indexed properties no longer shadow properties. --- .../Internal/InternalEntityEntry.cs | 2 +- .../Internal/InternalMixedEntityEntry.cs | 2 +- .../Internal/SnapshotFactoryFactory.cs | 15 ++++++++++ .../Metadata/Internal/ClrAccessorFactory.cs | 2 +- .../Internal/EntityMaterializerSource.cs | 2 +- src/EFCore/Metadata/Internal/EntityType.cs | 19 ++++++++---- .../Internal/IndexedPropertyGetterFactory.cs | 6 ++-- .../Internal/IndexedPropertySetterFactory.cs | 9 +++--- src/EFCore/Metadata/Internal/Property.cs | 5 ++-- src/EFCore/Metadata/Internal/PropertyBase.cs | 8 ++--- .../Metadata/Internal/PropertyExtensions.cs | 5 ++++ .../Metadata/Internal/TypeBaseExtensions.cs | 6 +--- src/Shared/PropertyInfoExtensions.cs | 15 ++++++++++ .../IndexedPropertyGetterFactoryTest.cs | 8 ++--- .../IndexedPropertySetterFactoryTest.cs | 29 ------------------- 15 files changed, 69 insertions(+), 64 deletions(-) diff --git a/src/EFCore/ChangeTracking/Internal/InternalEntityEntry.cs b/src/EFCore/ChangeTracking/Internal/InternalEntityEntry.cs index adff4201aba..4c3c5559226 100644 --- a/src/EFCore/ChangeTracking/Internal/InternalEntityEntry.cs +++ b/src/EFCore/ChangeTracking/Internal/InternalEntityEntry.cs @@ -589,7 +589,7 @@ public virtual TProperty GetRelationshipSnapshotValue([NotNull] IProp /// protected virtual object ReadPropertyValue([NotNull] IPropertyBase propertyBase) { - Debug.Assert(propertyBase.IsIndexedProperty || !propertyBase.IsShadowProperty); + Debug.Assert(!propertyBase.IsShadowProperty); return ((PropertyBase)propertyBase).Getter.GetClrValue(Entity); } diff --git a/src/EFCore/ChangeTracking/Internal/InternalMixedEntityEntry.cs b/src/EFCore/ChangeTracking/Internal/InternalMixedEntityEntry.cs index 33ea2b25911..04c9333301f 100644 --- a/src/EFCore/ChangeTracking/Internal/InternalMixedEntityEntry.cs +++ b/src/EFCore/ChangeTracking/Internal/InternalMixedEntityEntry.cs @@ -68,7 +68,7 @@ protected override T ReadShadowValue(int shadowIndex) /// directly from your code. This API may change or be removed in future releases. /// protected override object ReadPropertyValue(IPropertyBase propertyBase) - => propertyBase.IsIndexedProperty || !propertyBase.IsShadowProperty + => !propertyBase.IsShadowProperty ? base.ReadPropertyValue(propertyBase) : _shadowValues[propertyBase.GetShadowIndex()]; diff --git a/src/EFCore/ChangeTracking/Internal/SnapshotFactoryFactory.cs b/src/EFCore/ChangeTracking/Internal/SnapshotFactoryFactory.cs index 938f627654f..97eeddcfdd0 100644 --- a/src/EFCore/ChangeTracking/Internal/SnapshotFactoryFactory.cs +++ b/src/EFCore/ChangeTracking/Internal/SnapshotFactoryFactory.cs @@ -114,6 +114,21 @@ private Expression CreateSnapshotExpression( CreateReadShadowValueExpression(parameter, propertyBase), propertyBase); } + else if (propertyBase.IsIndexedProperty) + { + var indexerAccessExpression = (Expression)Expression.MakeIndex( + entityVariable, + propertyBase.PropertyInfo, + new List() { Expression.Constant(propertyBase.Name) }); + if (propertyBase.PropertyInfo.PropertyType != propertyBase.ClrType) + { + indexerAccessExpression = + Expression.Convert(indexerAccessExpression, propertyBase.ClrType); + } + + arguments[i] = + CreateSnapshotValueExpression(indexerAccessExpression, propertyBase); + } else { var memberAccess = (Expression)Expression.MakeMemberAccess( diff --git a/src/EFCore/Metadata/Internal/ClrAccessorFactory.cs b/src/EFCore/Metadata/Internal/ClrAccessorFactory.cs index 03a001d43b0..1e70b03c4ca 100644 --- a/src/EFCore/Metadata/Internal/ClrAccessorFactory.cs +++ b/src/EFCore/Metadata/Internal/ClrAccessorFactory.cs @@ -23,7 +23,7 @@ private static readonly MethodInfo _genericCreate /// directly from your code. This API may change or be removed in future releases. /// public virtual TAccessor Create([NotNull] IPropertyBase property) - => property as TAccessor ?? Create(null, property); + => property as TAccessor ?? Create(property.PropertyInfo, property); /// /// This API supports the Entity Framework Core infrastructure and is not intended to be used diff --git a/src/EFCore/Metadata/Internal/EntityMaterializerSource.cs b/src/EFCore/Metadata/Internal/EntityMaterializerSource.cs index 794f455ac63..6d7a64813dc 100644 --- a/src/EFCore/Metadata/Internal/EntityMaterializerSource.cs +++ b/src/EFCore/Metadata/Internal/EntityMaterializerSource.cs @@ -112,7 +112,7 @@ public virtual Expression CreateMaterializeExpression( .Concat( entityType .GetProperties() - .Where(p => !p.IsShadowProperty || p.IsIndexedProperty))); + .Where(p => !p.IsShadowProperty))); foreach (var consumedProperty in constructorBinding .ParameterBindings diff --git a/src/EFCore/Metadata/Internal/EntityType.cs b/src/EFCore/Metadata/Internal/EntityType.cs index bda2f422ee4..3591a638f5a 100644 --- a/src/EFCore/Metadata/Internal/EntityType.cs +++ b/src/EFCore/Metadata/Internal/EntityType.cs @@ -1597,7 +1597,7 @@ public virtual Property AddProperty( ClrType?.GetMembersInHierarchy(name).FirstOrDefault(), configurationSource, typeConfigurationSource, - isIndexProperty: false); + isIndexedProperty: false); } /// @@ -1628,7 +1628,7 @@ public virtual Property AddProperty( memberInfo, configurationSource, configurationSource, - isIndexProperty: false); + isIndexedProperty: false); } @@ -1646,13 +1646,19 @@ public virtual Property AddIndexedProperty( Check.NotNull(name, nameof(name)); Check.NotNull(propertyType, nameof(propertyType)); + var indexerProperty = this.EFIndexerProperty(); + if (indexerProperty == null) + { + throw new InvalidOperationException(CoreStrings.NoIndexer(name, this.DisplayName())); + } + return AddProperty( name, propertyType, - null, + indexerProperty, configurationSource, typeConfigurationSource, - isIndexProperty: true); + isIndexedProperty: true); } private Property AddProperty( @@ -1661,7 +1667,7 @@ private Property AddProperty( MemberInfo memberInfo, ConfigurationSource configurationSource, ConfigurationSource? typeConfigurationSource, - bool isIndexProperty) + bool isIndexedProperty) { var conflictingMember = FindMembersInHierarchy(name).FirstOrDefault(); if (conflictingMember != null) @@ -1685,6 +1691,7 @@ private Property AddProperty( else { if (memberInfo != null + && !isIndexedProperty && propertyType != memberInfo.GetMemberType()) { throw new InvalidOperationException( @@ -1698,7 +1705,7 @@ private Property AddProperty( var property = new Property( name, propertyType, memberInfo as PropertyInfo, memberInfo as FieldInfo, this, - configurationSource, typeConfigurationSource, isIndexProperty); + configurationSource, typeConfigurationSource); _properties.Add(property.Name, property); PropertyMetadataChanged(); diff --git a/src/EFCore/Metadata/Internal/IndexedPropertyGetterFactory.cs b/src/EFCore/Metadata/Internal/IndexedPropertyGetterFactory.cs index 843e037f519..5dffd2a82c3 100644 --- a/src/EFCore/Metadata/Internal/IndexedPropertyGetterFactory.cs +++ b/src/EFCore/Metadata/Internal/IndexedPropertyGetterFactory.cs @@ -25,10 +25,10 @@ public class IndexedPropertyGetterFactory : ClrAccessorFactory( PropertyInfo propertyInfo, IPropertyBase propertyBase) { + Debug.Assert(propertyInfo != null); Debug.Assert(propertyBase != null); - var indexerPropertyInfo = propertyBase.DeclaringType.EFIndexerProperty(); - if (indexerPropertyInfo == null) + if (!propertyInfo.IsEFIndexerProperty()) { throw new InvalidOperationException( CoreStrings.NoIndexer(propertyBase.Name, propertyBase.DeclaringType.DisplayName())); @@ -37,7 +37,7 @@ protected override IClrPropertyGetter CreateGeneric() { Expression.Constant(propertyBase.Name) }; Expression readExpression = Expression.MakeIndex( - entityParameter, indexerPropertyInfo, indexerParameterList); + entityParameter, propertyInfo, indexerParameterList); if (readExpression.Type != typeof(TValue)) { diff --git a/src/EFCore/Metadata/Internal/IndexedPropertySetterFactory.cs b/src/EFCore/Metadata/Internal/IndexedPropertySetterFactory.cs index 98a555a1f9e..f29370a8e93 100644 --- a/src/EFCore/Metadata/Internal/IndexedPropertySetterFactory.cs +++ b/src/EFCore/Metadata/Internal/IndexedPropertySetterFactory.cs @@ -24,10 +24,10 @@ public class IndexedPropertySetterFactory : ClrAccessorFactory( PropertyInfo propertyInfo, IPropertyBase propertyBase) { + Debug.Assert(propertyInfo != null); Debug.Assert(propertyBase != null); - var indexerPropertyInfo = propertyBase.DeclaringType.EFIndexerProperty(); - if (indexerPropertyInfo == null) + if (!propertyInfo.IsEFIndexerProperty()) { throw new InvalidOperationException( CoreStrings.NoIndexer(propertyBase.Name, propertyBase.DeclaringType.DisplayName())); @@ -37,14 +37,15 @@ protected override IClrPropertySetter CreateGeneric() { Expression.Constant(propertyBase.Name) }; - // the indexer expects the value to be an object, so cast it to that if necessary + // the indexer expects the value to be an object, but the indexed property + // can have been declared as a different type so cast it to that if necessary var propertyType = propertyBase.ClrType; var convertedParameter = propertyType == typeof(object) ? (Expression)valueParameter : Expression.TypeAs(valueParameter, typeof(object)); Expression writeExpression = Expression.Assign( - Expression.MakeIndex(entityParameter, indexerPropertyInfo, indexerParameterList), + Expression.MakeIndex(entityParameter, propertyInfo, indexerParameterList), convertedParameter); var setter = Expression.Lambda>( diff --git a/src/EFCore/Metadata/Internal/Property.cs b/src/EFCore/Metadata/Internal/Property.cs index 70b20754a0e..6ec56ac7148 100644 --- a/src/EFCore/Metadata/Internal/Property.cs +++ b/src/EFCore/Metadata/Internal/Property.cs @@ -45,9 +45,8 @@ public Property( [CanBeNull] FieldInfo fieldInfo, [NotNull] EntityType declaringEntityType, ConfigurationSource configurationSource, - ConfigurationSource? typeConfigurationSource, - bool isIndexProperty = false) - : base(name, propertyInfo, fieldInfo, isIndexProperty) + ConfigurationSource? typeConfigurationSource) + : base(name, propertyInfo, fieldInfo) { Check.NotNull(clrType, nameof(clrType)); Check.NotNull(declaringEntityType, nameof(declaringEntityType)); diff --git a/src/EFCore/Metadata/Internal/PropertyBase.cs b/src/EFCore/Metadata/Internal/PropertyBase.cs index 2590e3105e1..8e3d986b725 100644 --- a/src/EFCore/Metadata/Internal/PropertyBase.cs +++ b/src/EFCore/Metadata/Internal/PropertyBase.cs @@ -18,13 +18,13 @@ public abstract class PropertyBase : ConventionalAnnotatable, IMutablePropertyBa { private FieldInfo _fieldInfo; private ConfigurationSource? _fieldInfoConfigurationSource; + private bool _isIndexedProperty; // Warning: Never access these fields directly as access needs to be thread-safe private IClrPropertyGetter _getter; private IClrPropertySetter _setter; private PropertyAccessors _accessors; private PropertyIndexes _indexes; - private bool _isIndexedProperty; /// /// This API supports the Entity Framework Core infrastructure and is not intended to be used @@ -33,15 +33,15 @@ public abstract class PropertyBase : ConventionalAnnotatable, IMutablePropertyBa protected PropertyBase( [NotNull] string name, [CanBeNull] PropertyInfo propertyInfo, - [CanBeNull] FieldInfo fieldInfo, - bool isIndexProperty = false) + [CanBeNull] FieldInfo fieldInfo) { Check.NotEmpty(name, nameof(name)); Name = name; PropertyInfo = propertyInfo; _fieldInfo = fieldInfo; - _isIndexedProperty = isIndexProperty; + _isIndexedProperty = propertyInfo != null + && propertyInfo.IsEFIndexerProperty(); } /// diff --git a/src/EFCore/Metadata/Internal/PropertyExtensions.cs b/src/EFCore/Metadata/Internal/PropertyExtensions.cs index f26669d12b3..caded3672ae 100644 --- a/src/EFCore/Metadata/Internal/PropertyExtensions.cs +++ b/src/EFCore/Metadata/Internal/PropertyExtensions.cs @@ -244,6 +244,11 @@ public static string ToDebugString([NotNull] this IProperty property, bool singl builder.Append(" Shadow"); } + if (property.IsIndexedProperty) + { + builder.Append(" Indexed"); + } + if (!property.IsNullable) { builder.Append(" Required"); diff --git a/src/EFCore/Metadata/Internal/TypeBaseExtensions.cs b/src/EFCore/Metadata/Internal/TypeBaseExtensions.cs index af4aec8e015..1fe72b0d2b2 100644 --- a/src/EFCore/Metadata/Internal/TypeBaseExtensions.cs +++ b/src/EFCore/Metadata/Internal/TypeBaseExtensions.cs @@ -52,11 +52,7 @@ public static PropertyInfo EFIndexerProperty([NotNull] this ITypeBase type) : type.ClrType.GetRuntimeProperties(); // find the indexer with single argument of type string which returns an object - return (from p in runtimeProperties - where p.PropertyType == typeof(object) - let q = p.GetIndexParameters() - where q.Length == 1 && q[0].ParameterType == typeof(string) - select p).FirstOrDefault(); + return runtimeProperties.FirstOrDefault(p => p.IsEFIndexerProperty()); } } } diff --git a/src/Shared/PropertyInfoExtensions.cs b/src/Shared/PropertyInfoExtensions.cs index 64d3d59623e..d135a3b803b 100644 --- a/src/Shared/PropertyInfoExtensions.cs +++ b/src/Shared/PropertyInfoExtensions.cs @@ -21,6 +21,21 @@ public static bool IsCandidateProperty(this PropertyInfo propertyInfo, bool need && propertyInfo.GetMethod != null && (!publicOnly || propertyInfo.GetMethod.IsPublic) && propertyInfo.GetIndexParameters().Length == 0; + public static bool IsEFIndexerProperty([NotNull] this PropertyInfo propertyInfo) + { + if (propertyInfo.PropertyType == typeof(object)) + { + var indexParams = propertyInfo.GetIndexParameters(); + if (indexParams.Length == 1 + && indexParams[0].ParameterType == typeof(string)) + { + return true; + } + } + + return false; + } + public static PropertyInfo FindGetterProperty([NotNull] this PropertyInfo propertyInfo) => propertyInfo.DeclaringType .GetPropertiesInHierarchy(propertyInfo.GetSimpleMemberName()) diff --git a/test/EFCore.Tests/Metadata/Internal/IndexedPropertyGetterFactoryTest.cs b/test/EFCore.Tests/Metadata/Internal/IndexedPropertyGetterFactoryTest.cs index 5a3edd876e6..9ca89facaff 100644 --- a/test/EFCore.Tests/Metadata/Internal/IndexedPropertyGetterFactoryTest.cs +++ b/test/EFCore.Tests/Metadata/Internal/IndexedPropertyGetterFactoryTest.cs @@ -30,14 +30,10 @@ public void Exception_is_returned_for_indexed_property_without_indexer() { var entityType = new Model().AddEntityType(typeof(NonIndexedClass)); var idProperty = entityType.AddProperty("Id", typeof(int)); - var propertyA = entityType.AddIndexedProperty("PropertyA", typeof(string)); - var propertyB = entityType.AddIndexedProperty("PropertyB", typeof(int)); - - var nonIndexedClass = new NonIndexedClass(); Assert.Throws( - () => new IndexedPropertyGetterFactory().Create(propertyA).GetClrValue(nonIndexedClass)); + () => entityType.AddIndexedProperty("PropA", typeof(string))); Assert.Throws( - () => new IndexedPropertyGetterFactory().Create(propertyB).GetClrValue(nonIndexedClass)); + () => entityType.AddIndexedProperty("PropB", typeof(int))); } private class IndexedClass diff --git a/test/EFCore.Tests/Metadata/Internal/IndexedPropertySetterFactoryTest.cs b/test/EFCore.Tests/Metadata/Internal/IndexedPropertySetterFactoryTest.cs index 4caa77a7248..23012ea1508 100644 --- a/test/EFCore.Tests/Metadata/Internal/IndexedPropertySetterFactoryTest.cs +++ b/test/EFCore.Tests/Metadata/Internal/IndexedPropertySetterFactoryTest.cs @@ -1,7 +1,6 @@ // Copyright (c) .NET Foundation. All rights reserved. // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. -using System; using System.Collections.Generic; using Xunit; @@ -29,27 +28,6 @@ public void Delegate_setter_is_returned_for_indexed_property() Assert.Equal(456, indexedClass["PropertyB"]); } - [Fact] - public void Exception_is_returned_when_setting_indexed_property_without_indexer() - { - var entityType = new Model().AddEntityType(typeof(NonIndexedClass)); - var idProperty = entityType.AddProperty("Id", typeof(int)); - var propertyA = entityType.AddIndexedProperty("PropertyA", typeof(string)); - var propertyB = entityType.AddIndexedProperty("PropertyB", typeof(int)); - - var indexedClass = new NonIndexedClass - { - Id = 1, - PropA = "PropAValue", - PropB = 123 - }; - - Assert.Throws( - () => new IndexedPropertySetterFactory().Create(propertyA).SetClrValue(indexedClass, "UpdatedValueA")); - Assert.Throws( - () => new IndexedPropertySetterFactory().Create(propertyB).SetClrValue(indexedClass, 456)); - } - private class IndexedClass { private Dictionary _internalValues = new Dictionary() @@ -66,12 +44,5 @@ public object this[string name] set => _internalValues[name] = value; } } - - private class NonIndexedClass - { - internal int Id { get; set; } - public string PropA { get; set; } - public int PropB { get; set; } - } } }