From 3dd4c3c8b1d1f5d0c29f2c096c8ac5459d38efc8 Mon Sep 17 00:00:00 2001 From: ajcvickers Date: Wed, 26 Aug 2020 09:14:28 -0700 Subject: [PATCH 1/3] Support overriding virtual indexer property in proxies Fixes #21985 --- .../Internal/PropertyChangeInterceptorBase.cs | 77 +++++++++++++++++++ .../Internal/PropertyChangedInterceptor.cs | 20 ++--- .../Internal/PropertyChangingInterceptor.cs | 20 ++--- .../ManyToManyTrackingTestBase.cs | 46 +++++++++++ .../Query/ManyToManyQueryFixtureBase.cs | 7 ++ .../ManyToManyModel/ProxyableSharedType.cs | 18 +++++ 6 files changed, 162 insertions(+), 26 deletions(-) create mode 100644 src/EFCore.Proxies/Proxies/Internal/PropertyChangeInterceptorBase.cs create mode 100644 test/EFCore.Specification.Tests/TestModels/ManyToManyModel/ProxyableSharedType.cs diff --git a/src/EFCore.Proxies/Proxies/Internal/PropertyChangeInterceptorBase.cs b/src/EFCore.Proxies/Proxies/Internal/PropertyChangeInterceptorBase.cs new file mode 100644 index 00000000000..64ce7532320 --- /dev/null +++ b/src/EFCore.Proxies/Proxies/Internal/PropertyChangeInterceptorBase.cs @@ -0,0 +1,77 @@ +// 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 Castle.DynamicProxy; +using JetBrains.Annotations; +using Microsoft.EntityFrameworkCore.ChangeTracking; +using Microsoft.EntityFrameworkCore.Metadata; +using Microsoft.EntityFrameworkCore.Metadata.Internal; + +namespace Microsoft.EntityFrameworkCore.Proxies.Internal +{ + /// + /// This is an internal API that supports the Entity Framework Core infrastructure and not subject to + /// the same compatibility standards as public APIs. It may be changed or removed without notice in + /// any release. You should only use it directly in your code with extreme caution and knowing that + /// doing so can result in application failures when updating to a new Entity Framework Core release. + /// + public abstract class PropertyChangeInterceptorBase + { + /// + /// This is an internal API that supports the Entity Framework Core infrastructure and not subject to + /// the same compatibility standards as public APIs. It may be changed or removed without notice in + /// any release. You should only use it directly in your code with extreme caution and knowing that + /// doing so can result in application failures when updating to a new Entity Framework Core release. + /// + protected PropertyChangeInterceptorBase([NotNull] IEntityType entityType) + { + EntityType = entityType; + } + + /// + /// This is an internal API that supports the Entity Framework Core infrastructure and not subject to + /// the same compatibility standards as public APIs. It may be changed or removed without notice in + /// any release. You should only use it directly in your code with extreme caution and knowing that + /// doing so can result in application failures when updating to a new Entity Framework Core release. + /// + protected virtual IEntityType EntityType { get; } + + /// + /// This is an internal API that supports the Entity Framework Core infrastructure and not subject to + /// the same compatibility standards as public APIs. It may be changed or removed without notice in + /// any release. You should only use it directly in your code with extreme caution and knowing that + /// doing so can result in application failures when updating to a new Entity Framework Core release. + /// + protected virtual ValueComparer GetValueComparer([NotNull] IProperty property) + => property.IsKey() + || property.IsForeignKey() + ? property.GetKeyValueComparer() + : property.GetValueComparer(); + + /// + /// This is an internal API that supports the Entity Framework Core infrastructure and not subject to + /// the same compatibility standards as public APIs. It may be changed or removed without notice in + /// any release. You should only use it directly in your code with extreme caution and knowing that + /// doing so can result in application failures when updating to a new Entity Framework Core release. + /// + protected virtual string FindPropertyName([NotNull] IInvocation invocation) + { + var methodName = invocation.Method.Name; + if (methodName.Equals("set_Item", StringComparison.Ordinal)) + { +#pragma warning disable EF1001 // Internal EF Core API usage. + var indexerPropertyInfo = EntityType.FindIndexerPropertyInfo(); +#pragma warning restore EF1001 // Internal EF Core API usage. + + if (indexerPropertyInfo != null + && indexerPropertyInfo.GetSetMethod(nonPublic: true) == invocation.Method) + { + return (string)invocation.Arguments[0]; + } + } + + return methodName.Substring(4); + } + } +} diff --git a/src/EFCore.Proxies/Proxies/Internal/PropertyChangedInterceptor.cs b/src/EFCore.Proxies/Proxies/Internal/PropertyChangedInterceptor.cs index 511ec1a39ab..2e6cca77ab4 100644 --- a/src/EFCore.Proxies/Proxies/Internal/PropertyChangedInterceptor.cs +++ b/src/EFCore.Proxies/Proxies/Internal/PropertyChangedInterceptor.cs @@ -17,11 +17,10 @@ namespace Microsoft.EntityFrameworkCore.Proxies.Internal /// any release. You should only use it directly in your code with extreme caution and knowing that /// doing so can result in application failures when updating to a new Entity Framework Core release. /// - public class PropertyChangedInterceptor : IInterceptor + public class PropertyChangedInterceptor : PropertyChangeInterceptorBase, IInterceptor { private static readonly Type _notifyChangedInterface = typeof(INotifyPropertyChanged); - private readonly IEntityType _entityType; private readonly bool _checkEquality; private PropertyChangedEventHandler _handler; @@ -34,8 +33,8 @@ public class PropertyChangedInterceptor : IInterceptor public PropertyChangedInterceptor( [NotNull] IEntityType entityType, bool checkEquality) + : base(entityType) { - _entityType = entityType; _checkEquality = checkEquality; } @@ -64,22 +63,17 @@ public virtual void Intercept(IInvocation invocation) } else if (methodName.StartsWith("set_", StringComparison.Ordinal)) { - var propertyName = methodName.Substring(4); + var propertyName = FindPropertyName(invocation); - var property = _entityType.FindProperty(propertyName); + var property = EntityType.FindProperty(propertyName); if (property != null) { - var comparer = property.IsKey() - || property.IsForeignKey() - ? property.GetKeyValueComparer() - : property.GetValueComparer(); - - HandleChanged(invocation, property, comparer); + HandleChanged(invocation, property, GetValueComparer(property)); } else { - var navigation = _entityType.FindNavigation(propertyName) - ?? (INavigationBase)_entityType.FindSkipNavigation(propertyName); + var navigation = EntityType.FindNavigation(propertyName) + ?? (INavigationBase)EntityType.FindSkipNavigation(propertyName); if (navigation != null) { diff --git a/src/EFCore.Proxies/Proxies/Internal/PropertyChangingInterceptor.cs b/src/EFCore.Proxies/Proxies/Internal/PropertyChangingInterceptor.cs index 71c510924e3..1fbfd1b9989 100644 --- a/src/EFCore.Proxies/Proxies/Internal/PropertyChangingInterceptor.cs +++ b/src/EFCore.Proxies/Proxies/Internal/PropertyChangingInterceptor.cs @@ -17,11 +17,10 @@ namespace Microsoft.EntityFrameworkCore.Proxies.Internal /// any release. You should only use it directly in your code with extreme caution and knowing that /// doing so can result in application failures when updating to a new Entity Framework Core release. /// - public class PropertyChangingInterceptor : IInterceptor + public class PropertyChangingInterceptor : PropertyChangeInterceptorBase, IInterceptor { private static readonly Type _notifyChangingInterface = typeof(INotifyPropertyChanging); - private readonly IEntityType _entityType; private readonly bool _checkEquality; private PropertyChangingEventHandler _handler; @@ -34,8 +33,8 @@ public class PropertyChangingInterceptor : IInterceptor public PropertyChangingInterceptor( [NotNull] IEntityType entityType, bool checkEquality) + : base(entityType) { - _entityType = entityType; _checkEquality = checkEquality; } @@ -64,22 +63,17 @@ public virtual void Intercept(IInvocation invocation) } else if (methodName.StartsWith("set_", StringComparison.Ordinal)) { - var propertyName = methodName.Substring(4); + var propertyName = FindPropertyName(invocation); - var property = _entityType.FindProperty(propertyName); + var property = EntityType.FindProperty(propertyName); if (property != null) { - var comparer = property.IsKey() - || property.IsForeignKey() - ? property.GetKeyValueComparer() - : property.GetValueComparer(); - - HandleChanging(invocation, property, comparer); + HandleChanging(invocation, property, GetValueComparer(property)); } else { - var navigation = _entityType.FindNavigation(propertyName) - ?? (INavigationBase)_entityType.FindSkipNavigation(propertyName); + var navigation = EntityType.FindNavigation(propertyName) + ?? (INavigationBase)EntityType.FindSkipNavigation(propertyName); if (navigation != null) { diff --git a/test/EFCore.Specification.Tests/ManyToManyTrackingTestBase.cs b/test/EFCore.Specification.Tests/ManyToManyTrackingTestBase.cs index d81c8e86e02..2820e5dc9c8 100644 --- a/test/EFCore.Specification.Tests/ManyToManyTrackingTestBase.cs +++ b/test/EFCore.Specification.Tests/ManyToManyTrackingTestBase.cs @@ -3037,6 +3037,52 @@ public void Can_insert_update_delete_shared_type_entity_type() }); } + [ConditionalFact] + public void Can_insert_update_delete_proxyable_shared_type_entity_type() + { + ExecuteWithStrategyInTransaction( + context => + { + var entity = context.Set("PST").CreateInstance( + c => + { + c["Id"] = 1; + c["Payload"] = "NewlyAdded"; + }); + + context.Set("PST").Add(entity); + + context.SaveChanges(); + }, + context => + { + var entity = context.Set("PST").Single(e => (int)e["Id"] == 1); + + Assert.Equal("NewlyAdded", (string)entity["Payload"]); + + entity["Payload"] = "AlreadyUpdated"; + + if (RequiresDetectChanges) + { + context.ChangeTracker.DetectChanges(); + } + + context.SaveChanges(); + }, + context => + { + var entity = context.Set("PST").Single(e => (int)e["Id"] == 1); + + Assert.Equal("AlreadyUpdated", (string)entity["Payload"]); + + context.Set("PST").Remove(entity); + + context.SaveChanges(); + + Assert.False(context.Set("PST").Any(e => (int)e["Id"] == 1)); + }); + } + protected ManyToManyTrackingTestBase(TFixture fixture) => Fixture = fixture; diff --git a/test/EFCore.Specification.Tests/Query/ManyToManyQueryFixtureBase.cs b/test/EFCore.Specification.Tests/Query/ManyToManyQueryFixtureBase.cs index c62fb5f1dc9..d2f1e2044ee 100644 --- a/test/EFCore.Specification.Tests/Query/ManyToManyQueryFixtureBase.cs +++ b/test/EFCore.Specification.Tests/Query/ManyToManyQueryFixtureBase.cs @@ -320,6 +320,13 @@ protected override void OnModelCreating(ModelBuilder modelBuilder, DbContext con e.CompositeId3, e.LeafId }); + + modelBuilder.SharedTypeEntity( + "PST", b => + { + b.IndexerProperty("Id").ValueGeneratedNever(); // This is nullable due to #22009 + b.IndexerProperty("Payload"); + }); } protected override void Seed(ManyToManyContext context) diff --git a/test/EFCore.Specification.Tests/TestModels/ManyToManyModel/ProxyableSharedType.cs b/test/EFCore.Specification.Tests/TestModels/ManyToManyModel/ProxyableSharedType.cs new file mode 100644 index 00000000000..a523151f246 --- /dev/null +++ b/test/EFCore.Specification.Tests/TestModels/ManyToManyModel/ProxyableSharedType.cs @@ -0,0 +1,18 @@ +// 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.Collections.Generic; + +namespace Microsoft.EntityFrameworkCore.TestModels.ManyToManyModel +{ + public class ProxyableSharedType + { + private readonly Dictionary _keyValueStore = new Dictionary(); + + public virtual object this[string key] + { + get => _keyValueStore.TryGetValue(key, out var value) ? value : default; + set => _keyValueStore[key] = value; + } + } +} From 5b398ceffd4e938985ad64804d0a1d94a0b72598 Mon Sep 17 00:00:00 2001 From: ajcvickers Date: Thu, 27 Aug 2020 14:16:23 -0700 Subject: [PATCH 2/3] Treat indexer properties like nullable backing fields when handling default values --- .../ChangeTracking/Internal/SnapshotFactoryFactory.cs | 7 ++++++- src/EFCore/Extensions/Internal/ExpressionExtensions.cs | 9 +++++---- src/EFCore/Metadata/Internal/ClrPropertyGetterFactory.cs | 2 +- src/EFCore/Metadata/Internal/PropertyAccessorsFactory.cs | 2 +- src/EFCore/Metadata/Internal/PropertyBase.cs | 9 +-------- .../Query/ManyToManyQueryFixtureBase.cs | 2 +- 6 files changed, 15 insertions(+), 16 deletions(-) diff --git a/src/EFCore/ChangeTracking/Internal/SnapshotFactoryFactory.cs b/src/EFCore/ChangeTracking/Internal/SnapshotFactoryFactory.cs index ceff009bb1f..2ed800fd9c7 100644 --- a/src/EFCore/ChangeTracking/Internal/SnapshotFactoryFactory.cs +++ b/src/EFCore/ChangeTracking/Internal/SnapshotFactoryFactory.cs @@ -145,7 +145,12 @@ protected virtual Expression CreateSnapshotExpression( if (memberAccess.Type != propertyBase.ClrType) { - memberAccess = Expression.Convert(memberAccess, propertyBase.ClrType); + var hasDefaultValueExpression = memberAccess.MakeHasDefaultValue(propertyBase); + + memberAccess = Expression.Condition( + hasDefaultValueExpression, + propertyBase.ClrType.GetDefaultValueConstant(), + Expression.Convert(memberAccess, propertyBase.ClrType)); } arguments[i] = (propertyBase as INavigation)?.IsCollection ?? false diff --git a/src/EFCore/Extensions/Internal/ExpressionExtensions.cs b/src/EFCore/Extensions/Internal/ExpressionExtensions.cs index 541de45367b..9730c1002c1 100644 --- a/src/EFCore/Extensions/Internal/ExpressionExtensions.cs +++ b/src/EFCore/Extensions/Internal/ExpressionExtensions.cs @@ -29,9 +29,9 @@ public static class ExpressionExtensions /// any release. You should only use it directly in your code with extreme caution and knowing that /// doing so can result in application failures when updating to a new Entity Framework Core release. /// - public static Expression MakeHasDefaultValue( + public static Expression MakeHasDefaultValue( [NotNull] this Expression currentValueExpression, - [NotNull] IPropertyBase propertyBase) + [CanBeNull] IPropertyBase propertyBase) { if (!currentValueExpression.Type.IsValueType) { @@ -50,11 +50,12 @@ public static Expression MakeHasDefaultValue( } var property = propertyBase as IProperty; + var clrType = propertyBase?.ClrType ?? currentValueExpression.Type; var comparer = property?.GetValueComparer() - ?? ValueComparer.CreateDefault(typeof(TProperty), favorStructuralComparisons: false); + ?? ValueComparer.CreateDefault(clrType, favorStructuralComparisons: false); return comparer.ExtractEqualsBody( - comparer.Type != typeof(TProperty) + comparer.Type != clrType ? Expression.Convert(currentValueExpression, comparer.Type) : currentValueExpression, Expression.Default(comparer.Type)); diff --git a/src/EFCore/Metadata/Internal/ClrPropertyGetterFactory.cs b/src/EFCore/Metadata/Internal/ClrPropertyGetterFactory.cs index 9b6b0de5353..58c78e4cbbf 100644 --- a/src/EFCore/Metadata/Internal/ClrPropertyGetterFactory.cs +++ b/src/EFCore/Metadata/Internal/ClrPropertyGetterFactory.cs @@ -62,7 +62,7 @@ protected override IClrPropertyGetter CreateGeneric(propertyBase); + var hasDefaultValueExpression = readExpression.MakeHasDefaultValue(propertyBase); if (readExpression.Type != typeof(TValue)) { diff --git a/src/EFCore/Metadata/Internal/PropertyAccessorsFactory.cs b/src/EFCore/Metadata/Internal/PropertyAccessorsFactory.cs index 2942b8bf94c..5d9294020aa 100644 --- a/src/EFCore/Metadata/Internal/PropertyAccessorsFactory.cs +++ b/src/EFCore/Metadata/Internal/PropertyAccessorsFactory.cs @@ -77,7 +77,7 @@ private static Func CreateCurrentValueGetter if (currentValueExpression.Type != typeof(TProperty)) { currentValueExpression = Expression.Condition( - currentValueExpression.MakeHasDefaultValue(propertyBase), + currentValueExpression.MakeHasDefaultValue(propertyBase), Expression.Constant(default(TProperty), typeof(TProperty)), Expression.Convert(currentValueExpression, typeof(TProperty))); } diff --git a/src/EFCore/Metadata/Internal/PropertyBase.cs b/src/EFCore/Metadata/Internal/PropertyBase.cs index b3d83b4db52..d108560a524 100644 --- a/src/EFCore/Metadata/Internal/PropertyBase.cs +++ b/src/EFCore/Metadata/Internal/PropertyBase.cs @@ -410,20 +410,13 @@ public static Expression CreateMemberAccess( Expression expression = Expression.MakeIndex( instanceExpression, (PropertyInfo)memberInfo, new List { Expression.Constant(property.Name) }); - if (expression.Type != property.ClrType) - { - expression = Expression.Convert(expression, property.ClrType); - } - if (property.DeclaringType.IsPropertyBag) { - var defaultValueConstant = property.ClrType.GetDefaultValueConstant(); - expression = Expression.Condition( Expression.Call( instanceExpression, _containsKeyMethod, new List { Expression.Constant(property.Name) }), expression, - defaultValueConstant); + expression.Type.GetDefaultValueConstant()); } return expression; diff --git a/test/EFCore.Specification.Tests/Query/ManyToManyQueryFixtureBase.cs b/test/EFCore.Specification.Tests/Query/ManyToManyQueryFixtureBase.cs index d2f1e2044ee..aac3a89a346 100644 --- a/test/EFCore.Specification.Tests/Query/ManyToManyQueryFixtureBase.cs +++ b/test/EFCore.Specification.Tests/Query/ManyToManyQueryFixtureBase.cs @@ -324,7 +324,7 @@ protected override void OnModelCreating(ModelBuilder modelBuilder, DbContext con modelBuilder.SharedTypeEntity( "PST", b => { - b.IndexerProperty("Id").ValueGeneratedNever(); // This is nullable due to #22009 + b.IndexerProperty("Id").ValueGeneratedNever(); b.IndexerProperty("Payload"); }); } From 59f0f2c241502465bed42fc0c78105b355d69d9b Mon Sep 17 00:00:00 2001 From: ajcvickers Date: Thu, 27 Aug 2020 15:15:50 -0700 Subject: [PATCH 3/3] Cleanup --- .../ChangeTracking/Internal/SnapshotFactoryFactory.cs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/EFCore/ChangeTracking/Internal/SnapshotFactoryFactory.cs b/src/EFCore/ChangeTracking/Internal/SnapshotFactoryFactory.cs index 2ed800fd9c7..24fbbc48521 100644 --- a/src/EFCore/ChangeTracking/Internal/SnapshotFactoryFactory.cs +++ b/src/EFCore/ChangeTracking/Internal/SnapshotFactoryFactory.cs @@ -148,9 +148,9 @@ protected virtual Expression CreateSnapshotExpression( var hasDefaultValueExpression = memberAccess.MakeHasDefaultValue(propertyBase); memberAccess = Expression.Condition( - hasDefaultValueExpression, - propertyBase.ClrType.GetDefaultValueConstant(), - Expression.Convert(memberAccess, propertyBase.ClrType)); + hasDefaultValueExpression, + propertyBase.ClrType.GetDefaultValueConstant(), + Expression.Convert(memberAccess, propertyBase.ClrType)); } arguments[i] = (propertyBase as INavigation)?.IsCollection ?? false