From c072747f6d5120c7412ad3abe0bfd9e917d753ce Mon Sep 17 00:00:00 2001 From: ajcvickers Date: Thu, 27 Aug 2020 08:10:11 -0700 Subject: [PATCH] Limit change tracking proxies to only changing _and_ changed strategies Fixes #21920 --- .../Proxies/Internal/ProxyFactory.cs | 58 +++-------- src/EFCore/Infrastructure/ModelValidator.cs | 14 +-- src/EFCore/Metadata/Internal/EntityType.cs | 34 +++++-- src/EFCore/Properties/CoreStrings.Designer.cs | 10 +- src/EFCore/Properties/CoreStrings.resx | 57 +++++------ .../ChangeDetectionProxyTests.cs | 95 ++++++++++++++----- 6 files changed, 151 insertions(+), 117 deletions(-) diff --git a/src/EFCore.Proxies/Proxies/Internal/ProxyFactory.cs b/src/EFCore.Proxies/Proxies/Internal/ProxyFactory.cs index 00b9a6954d8..94dfc9a58d0 100644 --- a/src/EFCore.Proxies/Proxies/Internal/ProxyFactory.cs +++ b/src/EFCore.Proxies/Proxies/Internal/ProxyFactory.cs @@ -154,31 +154,14 @@ private Type[] GetInterfacesToProxy( if (options.UseChangeTrackingProxies) { - var changeTrackingStrategy = entityType.GetChangeTrackingStrategy(); - switch (changeTrackingStrategy) + if (!_notifyPropertyChangedInterface.IsAssignableFrom(entityType.ClrType)) { - case ChangeTrackingStrategy.ChangedNotifications: - - if (!_notifyPropertyChangedInterface.IsAssignableFrom(entityType.ClrType)) - { - interfacesToProxy.Add(_notifyPropertyChangedInterface); - } - - break; - case ChangeTrackingStrategy.ChangingAndChangedNotifications: - case ChangeTrackingStrategy.ChangingAndChangedNotificationsWithOriginalValues: - - if (!_notifyPropertyChangedInterface.IsAssignableFrom(entityType.ClrType)) - { - interfacesToProxy.Add(_notifyPropertyChangedInterface); - } - - if (!_notifyPropertyChangingInterface.IsAssignableFrom(entityType.ClrType)) - { - interfacesToProxy.Add(_notifyPropertyChangingInterface); - } + interfacesToProxy.Add(_notifyPropertyChangedInterface); + } - break; + if (!_notifyPropertyChangingInterface.IsAssignableFrom(entityType.ClrType)) + { + interfacesToProxy.Add(_notifyPropertyChangingInterface); } } @@ -199,31 +182,14 @@ private IInterceptor[] GetNotifyChangeInterceptors( if (options.UseChangeTrackingProxies) { - var changeTrackingStrategy = entityType.GetChangeTrackingStrategy(); - switch (changeTrackingStrategy) + if (!_notifyPropertyChangedInterface.IsAssignableFrom(entityType.ClrType)) { - case ChangeTrackingStrategy.ChangedNotifications: - - if (!_notifyPropertyChangedInterface.IsAssignableFrom(entityType.ClrType)) - { - interceptors.Add(new PropertyChangedInterceptor(entityType, options.CheckEquality)); - } - - break; - case ChangeTrackingStrategy.ChangingAndChangedNotifications: - case ChangeTrackingStrategy.ChangingAndChangedNotificationsWithOriginalValues: - - if (!_notifyPropertyChangedInterface.IsAssignableFrom(entityType.ClrType)) - { - interceptors.Add(new PropertyChangedInterceptor(entityType, options.CheckEquality)); - } - - if (!_notifyPropertyChangingInterface.IsAssignableFrom(entityType.ClrType)) - { - interceptors.Add(new PropertyChangingInterceptor(entityType, options.CheckEquality)); - } + interceptors.Add(new PropertyChangedInterceptor(entityType, options.CheckEquality)); + } - break; + if (!_notifyPropertyChangingInterface.IsAssignableFrom(entityType.ClrType)) + { + interceptors.Add(new PropertyChangingInterceptor(entityType, options.CheckEquality)); } } diff --git a/src/EFCore/Infrastructure/ModelValidator.cs b/src/EFCore/Infrastructure/ModelValidator.cs index 4d7bb871083..f021e1665ab 100644 --- a/src/EFCore/Infrastructure/ModelValidator.cs +++ b/src/EFCore/Infrastructure/ModelValidator.cs @@ -687,15 +687,15 @@ protected virtual void ValidateChangeTrackingStrategy( { Check.NotNull(model, nameof(model)); - if ((string)model[CoreAnnotationNames.SkipChangeTrackingStrategyValidationAnnotation] != "true") + var requireFullNotifications = (string)model[CoreAnnotationNames.SkipChangeTrackingStrategyValidationAnnotation] == "true"; + foreach (var entityType in model.GetEntityTypes()) { - foreach (var entityType in model.GetEntityTypes()) + var errorMessage = entityType.AsEntityType().CheckChangeTrackingStrategy( + entityType.GetChangeTrackingStrategy(), requireFullNotifications); + + if (errorMessage != null) { - var errorMessage = entityType.AsEntityType().CheckChangeTrackingStrategy(entityType.GetChangeTrackingStrategy()); - if (errorMessage != null) - { - throw new InvalidOperationException(errorMessage); - } + throw new InvalidOperationException(errorMessage); } } } diff --git a/src/EFCore/Metadata/Internal/EntityType.cs b/src/EFCore/Metadata/Internal/EntityType.cs index e9f52b57ae9..58e9c32e9b5 100644 --- a/src/EFCore/Metadata/Internal/EntityType.cs +++ b/src/EFCore/Metadata/Internal/EntityType.cs @@ -3069,7 +3069,8 @@ public virtual void AddData([NotNull] IEnumerable data) { if (changeTrackingStrategy != null) { - var errorMessage = CheckChangeTrackingStrategy(changeTrackingStrategy.Value); + var requireFullNotifications = (string)Model[CoreAnnotationNames.SkipChangeTrackingStrategyValidationAnnotation] == "true"; + var errorMessage = CheckChangeTrackingStrategy(changeTrackingStrategy.Value, requireFullNotifications); if (errorMessage != null) { throw new InvalidOperationException(errorMessage); @@ -3087,21 +3088,34 @@ public virtual void AddData([NotNull] IEnumerable data) /// 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 virtual string CheckChangeTrackingStrategy(ChangeTrackingStrategy value) + public virtual string CheckChangeTrackingStrategy(ChangeTrackingStrategy value, bool requireFullNotifications) { if (ClrType != null) { - if (value != ChangeTrackingStrategy.Snapshot - && !typeof(INotifyPropertyChanged).IsAssignableFrom(ClrType)) + if (requireFullNotifications) { - return CoreStrings.ChangeTrackingInterfaceMissing(this.DisplayName(), value, nameof(INotifyPropertyChanged)); + if (value != ChangeTrackingStrategy.ChangingAndChangedNotifications + && value != ChangeTrackingStrategy.ChangingAndChangedNotificationsWithOriginalValues) + { + return CoreStrings.FullChangeTrackingRequired( + this.DisplayName(), value, nameof(ChangeTrackingStrategy.ChangingAndChangedNotifications), + nameof(ChangeTrackingStrategy.ChangingAndChangedNotificationsWithOriginalValues)); + } } - - if ((value == ChangeTrackingStrategy.ChangingAndChangedNotifications - || value == ChangeTrackingStrategy.ChangingAndChangedNotificationsWithOriginalValues) - && !typeof(INotifyPropertyChanging).IsAssignableFrom(ClrType)) + else { - return CoreStrings.ChangeTrackingInterfaceMissing(this.DisplayName(), value, nameof(INotifyPropertyChanging)); + if (value != ChangeTrackingStrategy.Snapshot + && !typeof(INotifyPropertyChanged).IsAssignableFrom(ClrType)) + { + return CoreStrings.ChangeTrackingInterfaceMissing(this.DisplayName(), value, nameof(INotifyPropertyChanged)); + } + + if ((value == ChangeTrackingStrategy.ChangingAndChangedNotifications + || value == ChangeTrackingStrategy.ChangingAndChangedNotificationsWithOriginalValues) + && !typeof(INotifyPropertyChanging).IsAssignableFrom(ClrType)) + { + return CoreStrings.ChangeTrackingInterfaceMissing(this.DisplayName(), value, nameof(INotifyPropertyChanging)); + } } } diff --git a/src/EFCore/Properties/CoreStrings.Designer.cs b/src/EFCore/Properties/CoreStrings.Designer.cs index bd7e0997f59..cac167f057b 100644 --- a/src/EFCore/Properties/CoreStrings.Designer.cs +++ b/src/EFCore/Properties/CoreStrings.Designer.cs @@ -1,4 +1,4 @@ -// +// using System; using System.Reflection; @@ -980,6 +980,14 @@ public static string ForeignKeyWrongType([CanBeNull] object foreignKey, [CanBeNu GetString("ForeignKeyWrongType", nameof(foreignKey), nameof(key), nameof(principalType), nameof(entityType), nameof(otherEntityType)), foreignKey, key, principalType, entityType, otherEntityType); + /// + /// The entity type '{entityType}' is configured to use the '{changeTrackingStrategy}' change tracking strategy when full change tracking notifications are required. Configure all entity types in the model to use the '{fullStrategy}' or '{fullPlusStrategy}' strategy. + /// + public static string FullChangeTrackingRequired([CanBeNull] object entityType, [CanBeNull] object changeTrackingStrategy, [CanBeNull] object fullStrategy, [CanBeNull] object fullPlusStrategy) + => string.Format( + GetString("FullChangeTrackingRequired", nameof(entityType), nameof(changeTrackingStrategy), nameof(fullStrategy), nameof(fullPlusStrategy)), + entityType, changeTrackingStrategy, fullStrategy, fullPlusStrategy); + /// /// The '{methodName}' method is not supported because the query has switched to client-evaluation. Inspect the log to determine which query expressions are triggering client-evaluation. /// diff --git a/src/EFCore/Properties/CoreStrings.resx b/src/EFCore/Properties/CoreStrings.resx index 400d08b03cf..422665c431c 100644 --- a/src/EFCore/Properties/CoreStrings.resx +++ b/src/EFCore/Properties/CoreStrings.resx @@ -1,17 +1,17 @@  - @@ -483,6 +483,9 @@ The foreign key {foreignKey} targeting the key {key} on '{principalType}' cannot be removed from the entity type '{entityType}' because it is defined on the entity type '{otherEntityType}'. + + The entity type '{entityType}' is configured to use the '{changeTrackingStrategy}' change tracking strategy when full change tracking notifications are required. Configure all entity types in the model to use the '{fullStrategy}' or '{fullPlusStrategy}' strategy. + The '{methodName}' method is not supported because the query has switched to client-evaluation. Inspect the log to determine which query expressions are triggering client-evaluation. diff --git a/test/EFCore.Proxies.Tests/ChangeDetectionProxyTests.cs b/test/EFCore.Proxies.Tests/ChangeDetectionProxyTests.cs index 6f958f04839..26fdf97aec8 100644 --- a/test/EFCore.Proxies.Tests/ChangeDetectionProxyTests.cs +++ b/test/EFCore.Proxies.Tests/ChangeDetectionProxyTests.cs @@ -3,6 +3,7 @@ using System; using System.ComponentModel; +using Microsoft.EntityFrameworkCore.Diagnostics; using Microsoft.EntityFrameworkCore.Internal; using Microsoft.EntityFrameworkCore.Metadata.Builders; using Microsoft.EntityFrameworkCore.TestUtilities; @@ -46,55 +47,64 @@ public void Throws_if_non_virtual_navigation() public void Sets_default_change_tracking_strategy() { using var context = new ChangeContext(); - Assert.Equal( - ChangeTrackingStrategy.ChangingAndChangedNotifications, - context.Model.GetChangeTrackingStrategy()); + + Assert.Equal(ChangeTrackingStrategy.ChangingAndChangedNotifications, context.Model.GetChangeTrackingStrategy()); } [ConditionalFact] public void Default_change_tracking_strategy_doesnt_overwrite_entity_strategy() { - using var context = new ChangeContext( - entityBuilderAction: b => - { - b.HasChangeTrackingStrategy(ChangeTrackingStrategy.Snapshot); - }); + using var context = new ChangingAndChangedNotificationsWithOriginalValuesContext(); var entityType = context.Model.FindEntityType(typeof(ChangeValueEntity)); - Assert.Equal( - ChangeTrackingStrategy.Snapshot, - entityType.GetChangeTrackingStrategy()); + + Assert.Equal(ChangeTrackingStrategy.ChangingAndChangedNotificationsWithOriginalValues, entityType.GetChangeTrackingStrategy()); } private static readonly Type changeInterface = typeof(INotifyPropertyChanged); private static readonly Type changingInterface = typeof(INotifyPropertyChanging); [ConditionalFact] - public void Proxies_correct_interfaces_for_Snapshot() + public void Throws_when_proxies_are_used_with_snapshot_tracking() { - using var context = new ProxyGenerationContext(ChangeTrackingStrategy.Snapshot); - var proxy = context.CreateProxy(); - var proxyType = proxy.GetType(); + using var context = new SnapshotContext(); + + Assert.Equal( + CoreStrings.FullChangeTrackingRequired( + nameof(ChangeValueEntity), nameof(ChangeTrackingStrategy.Snapshot), + nameof(ChangeTrackingStrategy.ChangingAndChangedNotifications), + nameof(ChangeTrackingStrategy.ChangingAndChangedNotificationsWithOriginalValues)), + Assert.Throws(() => _ = context.Model).Message); + } - Assert.False(changeInterface.IsAssignableFrom(proxyType)); - Assert.False(changingInterface.IsAssignableFrom(proxyType)); + [ConditionalFact] + public void Throws_when_proxies_are_used_with_changed_only_tracking() + { + using var context = new ChangedNotificationsContext(); + + Assert.Equal( + CoreStrings.FullChangeTrackingRequired( + nameof(ChangeValueEntity), nameof(ChangeTrackingStrategy.ChangedNotifications), + nameof(ChangeTrackingStrategy.ChangingAndChangedNotifications), + nameof(ChangeTrackingStrategy.ChangingAndChangedNotificationsWithOriginalValues)), + Assert.Throws(() => _ = context.Model).Message); } [ConditionalFact] - public void Proxies_correct_interfaces_for_ChangedNotifications() + public void Proxies_correct_interfaces_for_default_strategy() { - using var context = new ProxyGenerationContext(ChangeTrackingStrategy.ChangedNotifications); + using var context = new DefaultContext(); var proxy = context.CreateProxy(); var proxyType = proxy.GetType(); Assert.True(changeInterface.IsAssignableFrom(proxyType)); - Assert.False(changingInterface.IsAssignableFrom(proxyType)); + Assert.True(changingInterface.IsAssignableFrom(proxyType)); } [ConditionalFact] public void Proxies_correct_interfaces_for_ChangingAndChangedNotifications() { - using var context = new ProxyGenerationContext(ChangeTrackingStrategy.ChangingAndChangedNotifications); + using var context = new ChangingAndChangedNotificationsContext(); var proxy = context.CreateProxy(); var proxyType = proxy.GetType(); @@ -105,7 +115,7 @@ public void Proxies_correct_interfaces_for_ChangingAndChangedNotifications() [ConditionalFact] public void Proxies_correct_interfaces_for_ChangingAndChangedNotificationsWithOriginalValues() { - using var context = new ProxyGenerationContext(ChangeTrackingStrategy.ChangingAndChangedNotificationsWithOriginalValues); + using var context = new ChangingAndChangedNotificationsWithOriginalValuesContext(); var proxy = context.CreateProxy(); var proxyType = proxy.GetType(); @@ -322,11 +332,44 @@ public class ChangeSelfRefEntity public virtual ChangeSelfRefEntity SelfRef { get; set; } } - private class ProxyGenerationContext : TestContext + private class DefaultContext : TestContext { - public ProxyGenerationContext( - ChangeTrackingStrategy changeTrackingStrategy) - : base("ProxyGenerationContext", false, true, true, changeTrackingStrategy) + public DefaultContext() + : base(nameof(DefaultContext), false, true, true, null) + { + } + } + + private class SnapshotContext : TestContext + { + public SnapshotContext() + : base(nameof(SnapshotContext), false, true, true, ChangeTrackingStrategy.Snapshot) + { + } + } + + private class ChangedNotificationsContext : TestContext + { + public ChangedNotificationsContext() + : base(nameof(ChangedNotificationsContext), false, true, true, ChangeTrackingStrategy.ChangedNotifications) + { + } + } + + private class ChangingAndChangedNotificationsContext : TestContext + { + public ChangingAndChangedNotificationsContext() + : base(nameof(ChangingAndChangedNotificationsContext), false, true, true, ChangeTrackingStrategy.ChangingAndChangedNotifications) + { + } + } + + private class ChangingAndChangedNotificationsWithOriginalValuesContext : TestContext + { + public ChangingAndChangedNotificationsWithOriginalValuesContext() + : base( + nameof(ChangingAndChangedNotificationsWithOriginalValuesContext), false, true, true, + ChangeTrackingStrategy.ChangingAndChangedNotificationsWithOriginalValues) { } }