Skip to content

Commit

Permalink
Graph update tests for change tracking proxies
Browse files Browse the repository at this point in the history 
Issue #10949

Found and fixed a couple of bugs:
* Proxies were reading directly from the property. This can cause recursive loops in some cases. Fixed by using EF's compiled delegates for writing to the field directly.
* Comparisons were being done by the default for the property/navigation. Instead force use of configured value comparer for regular properties or reference equality for navigations.

Also added overloads to `CreateProxy` to make it easier to do creation of proxy graphs inline.
  • Loading branch information
@ajcvickers
ajcvickers committed Jan 26, 2020
1 parent a355454 commit 70288d2
Show file tree
Hide file tree
Showing 10 changed files with 2,059 additions and 394 deletions.
41 changes: 17 additions & 24 deletions src/EFCore.Proxies/Proxies/Internal/PropertyChangedInterceptor.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,10 +2,13 @@
// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.

using System;
using System.Collections;
using System.ComponentModel;
using Castle.DynamicProxy;
using JetBrains.Annotations;
using Microsoft.EntityFrameworkCore.Internal;
using Microsoft.EntityFrameworkCore.Metadata;
using Microsoft.EntityFrameworkCore.Metadata.Internal;

namespace Microsoft.EntityFrameworkCore.Proxies.Internal
{
Expand All @@ -22,7 +25,6 @@ public class PropertyChangedInterceptor : IInterceptor
private readonly IEntityType _entityType;
private readonly bool _checkEquality;
private PropertyChangedEventHandler _handler;
private Type _proxyType;

/// <summary>
/// This is an internal API that supports the Entity Framework Core infrastructure and not subject to
Expand Down Expand Up @@ -68,14 +70,18 @@ public virtual void Intercept(IInvocation invocation)
var property = _entityType.FindProperty(propertyName);
if (property != null)
{
HandleChanged(invocation, propertyName);
var comparer = property.IsKeyOrForeignKey()
? property.GetKeyValueComparer()
: property.GetValueComparer();

HandleChanged(invocation, property, comparer);
}
else
{
var navigation = _entityType.FindNavigation(propertyName);
if (navigation != null)
{
HandleChanged(invocation, propertyName);
HandleChanged(invocation, navigation, ReferenceEqualityComparer.Instance);
}
else
{
Expand All @@ -89,29 +95,19 @@ public virtual void Intercept(IInvocation invocation)
}
}

private void HandleChanged(IInvocation invocation, string propertyName)
private void HandleChanged(IInvocation invocation, IPropertyBase property, IEqualityComparer comparer)
{
var newValue = invocation.Arguments[^1];

if (_checkEquality)
{
if (_proxyType == null)
{
_proxyType = invocation.Proxy.GetType();
}
var oldValue = property.GetGetter().GetClrValue(invocation.Proxy);

var property = _proxyType.GetProperty(propertyName);
if (property != null)
{
var oldValue = property.GetValue(invocation.Proxy);

invocation.Proceed();
invocation.Proceed();

if ((oldValue is null ^ newValue is null)
|| oldValue?.Equals(newValue) == false)
{
NotifyPropertyChanged(propertyName, invocation.Proxy);
}
if (!(comparer?.Equals(oldValue, newValue) ?? Equals(oldValue, newValue)))
{
NotifyPropertyChanged(property.Name, invocation.Proxy);
}
else
{
Expand All @@ -121,14 +117,11 @@ private void HandleChanged(IInvocation invocation, string propertyName)
else
{
invocation.Proceed();
NotifyPropertyChanged(propertyName, invocation.Proxy);
NotifyPropertyChanged(property.Name, invocation.Proxy);
}
}

private void NotifyPropertyChanged(string propertyName, object proxy)
{
var args = new PropertyChangedEventArgs(propertyName);
_handler?.Invoke(proxy, args);
}
=> _handler?.Invoke(proxy, new PropertyChangedEventArgs(propertyName));
}
}
39 changes: 16 additions & 23 deletions src/EFCore.Proxies/Proxies/Internal/PropertyChangingInterceptor.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,10 +2,13 @@
// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.

using System;
using System.Collections;
using System.ComponentModel;
using Castle.DynamicProxy;
using JetBrains.Annotations;
using Microsoft.EntityFrameworkCore.Internal;
using Microsoft.EntityFrameworkCore.Metadata;
using Microsoft.EntityFrameworkCore.Metadata.Internal;

namespace Microsoft.EntityFrameworkCore.Proxies.Internal
{
Expand All @@ -22,7 +25,6 @@ public class PropertyChangingInterceptor : IInterceptor
private readonly IEntityType _entityType;
private readonly bool _checkEquality;
private PropertyChangingEventHandler _handler;
private Type _proxyType;

/// <summary>
/// This is an internal API that supports the Entity Framework Core infrastructure and not subject to
Expand Down Expand Up @@ -68,14 +70,18 @@ public virtual void Intercept(IInvocation invocation)
var property = _entityType.FindProperty(propertyName);
if (property != null)
{
HandleChanging(invocation, propertyName);
var comparer = property.IsKeyOrForeignKey()
? property.GetKeyValueComparer()
: property.GetValueComparer();

HandleChanging(invocation, property, comparer);
}
else
{
var navigation = _entityType.FindNavigation(propertyName);
if (navigation != null)
{
HandleChanging(invocation, propertyName);
HandleChanging(invocation, navigation, ReferenceEqualityComparer.Instance);
}
else
{
Expand All @@ -89,40 +95,27 @@ public virtual void Intercept(IInvocation invocation)
}
}

private void HandleChanging(IInvocation invocation, string propertyName)
private void HandleChanging(IInvocation invocation, IPropertyBase property, IEqualityComparer comparer)
{
if (_checkEquality)
{
if (_proxyType == null)
{
_proxyType = invocation.Proxy.GetType();
}
var oldValue = property.GetGetter().GetClrValue(invocation.Proxy);
var newValue = invocation.Arguments[^1];

var property = _proxyType.GetProperty(propertyName);
if (property != null)
if (!(comparer?.Equals(oldValue, newValue) ?? Equals(oldValue, newValue)))
{
var oldValue = property.GetValue(invocation.Proxy);
var newValue = invocation.Arguments[^1];

if ((oldValue is null ^ newValue is null)
|| oldValue?.Equals(newValue) == false)
{
NotifyPropertyChanging(propertyName, invocation.Proxy);
}
NotifyPropertyChanging(property.Name, invocation.Proxy);
}
}
else
{
NotifyPropertyChanging(propertyName, invocation.Proxy);
NotifyPropertyChanging(property.Name, invocation.Proxy);
}

invocation.Proceed();
}

private void NotifyPropertyChanging(string propertyName, object proxy)
{
var args = new PropertyChangingEventArgs(propertyName);
_handler?.Invoke(proxy, args);
}
=> _handler?.Invoke(proxy, new PropertyChangingEventArgs(propertyName));
}
}
10 changes: 3 additions & 7 deletions src/EFCore.Proxies/Proxies/Internal/ProxyFactory.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ public virtual object Create(
entityType,
context.GetService<ILazyLoader>(),
constructorArguments);
}
}

return CreateProxy(
options,
Expand Down Expand Up @@ -171,7 +171,7 @@ private Type[] GetInterfacesToProxy(
{
interfacesToProxy.Add(_notifyPropertyChangedInterface);
}

break;
case ChangeTrackingStrategy.ChangingAndChangedNotifications:
case ChangeTrackingStrategy.ChangingAndChangedNotificationsWithOriginalValues:
Expand All @@ -186,8 +186,6 @@ private Type[] GetInterfacesToProxy(
interfacesToProxy.Add(_notifyPropertyChangingInterface);
}

break;
default:
break;
}
}
Expand Down Expand Up @@ -232,9 +230,7 @@ private Castle.DynamicProxy.IInterceptor[] GetNotifyChangeInterceptors(
{
interceptors.Add(new PropertyChangingInterceptor(entityType, options.CheckEquality));
}

break;
default:

break;
}
}
Expand Down
44 changes: 41 additions & 3 deletions src/EFCore.Proxies/ProxiesExtensions.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -159,7 +159,27 @@ public static object CreateProxy(
public static TEntity CreateProxy<TEntity>(
[NotNull] this DbContext context,
[NotNull] params object[] constructorArguments)
=> (TEntity)context.CreateProxy(typeof(TEntity), constructorArguments);
=> CreateProxy<TEntity>(context, null, constructorArguments);

/// <summary>
/// Creates a proxy instance for an entity type if proxy creation has been turned on.
/// </summary>
/// <typeparam name="TEntity"> The entity type for which a proxy is needed. </typeparam>
/// <param name="context"> The <see cref="DbContext" />. </param>
/// <param name="configureEntity"> Called after the entity is created to set property values, etc. </param>
/// <param name="constructorArguments"> Arguments to pass to the entity type constructor. </param>
/// <returns> The proxy instance. </returns>
public static TEntity CreateProxy<TEntity>(
[NotNull] this DbContext context,
[CanBeNull] Action<TEntity> configureEntity,
[NotNull] params object[] constructorArguments)
{
var entity = (TEntity)context.CreateProxy(typeof(TEntity), constructorArguments);

configureEntity?.Invoke(entity);

return entity;
}

/// <summary>
/// Creates a proxy instance for an entity type if proxy creation has been turned on.
Expand All @@ -172,13 +192,31 @@ public static TEntity CreateProxy<TEntity>(
[NotNull] this DbSet<TEntity> set,
[NotNull] params object[] constructorArguments)
where TEntity : class
=> CreateProxy(set, null, constructorArguments);

/// <summary>
/// Creates a proxy instance for an entity type if proxy creation has been turned on.
/// </summary>
/// <typeparam name="TEntity"> The entity type for which a proxy is needed. </typeparam>
/// <param name="set"> The <see cref="DbSet{TEntity}" />. </param>
/// <param name="configureEntity"> Called after the entity is created to set property values, etc. </param>
/// <param name="constructorArguments"> Arguments to pass to the entity type constructor. </param>
/// <returns> The proxy instance. </returns>
public static TEntity CreateProxy<TEntity>(
[NotNull] this DbSet<TEntity> set,
[CanBeNull] Action<TEntity> configureEntity,
[NotNull] params object[] constructorArguments)
where TEntity : class
{
Check.NotNull(set, nameof(set));
Check.NotNull(constructorArguments, nameof(constructorArguments));

return (TEntity)set.GetInfrastructure().CreateProxy(typeof(TEntity), constructorArguments);
}
var entity = (TEntity)set.GetInfrastructure().CreateProxy(typeof(TEntity), constructorArguments);

configureEntity?.Invoke(entity);

return entity;
}
private static object CreateProxy(
this IServiceProvider serviceProvider,
Type entityType,
Expand Down
31 changes: 30 additions & 1 deletion src/EFCore/Internal/ReferenceEqualityComparer.cs
View file
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
// 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;
using System.Collections.Generic;
using System.Runtime.CompilerServices;

Expand All @@ -13,7 +14,7 @@ namespace Microsoft.EntityFrameworkCore.Internal
/// doing so can result in application failures when updating to a new Entity Framework Core release.
/// </summary>
// Sealed for perf
public sealed class ReferenceEqualityComparer : IEqualityComparer<object>
public sealed class ReferenceEqualityComparer : IEqualityComparer<object>, IEqualityComparer
{
private ReferenceEqualityComparer()
{
Expand All @@ -27,8 +28,36 @@ private ReferenceEqualityComparer()
/// </summary>
public static ReferenceEqualityComparer Instance { get; } = new ReferenceEqualityComparer();

/// <summary>
/// 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.
/// </summary>
bool IEqualityComparer<object>.Equals(object x, object y) => ReferenceEquals(x, y);

/// <summary>
/// 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.
/// </summary>
bool IEqualityComparer.Equals(object x, object y) => ReferenceEquals(x, y);

/// <summary>
/// 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.
/// </summary>
int IEqualityComparer.GetHashCode(object obj) => RuntimeHelpers.GetHashCode(obj);

/// <summary>
/// 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.
/// </summary>
int IEqualityComparer<object>.GetHashCode(object obj) => RuntimeHelpers.GetHashCode(obj);
}
}
Loading

0 comments on commit 70288d2

Please sign in to comment.