Skip to content

Commit

Permalink
[release/6.0] Avoid proxying Clone methods on record types
Browse files Browse the repository at this point in the history
Fixes #26602

**Description**

The C# compiler changed the emit strategy for records in .NET 6 to take advantage of covariant returns. The Clone method now always has a return type that matches the containing type. Castle dynamic proxies throws when attempting to create a proxy for such types. This means EF Core lazy-loading and change-tracking proxies are broken for any record with a base type.

An bug has been filed on Castle proxies (castleproject/Core#601), which is the correct place to ultimately fix this issue. However, since EF Core never needs to override the Clone method for its proxies, the fix here simply excludes the Clone method from the proxies we create.

**Customer impact**

Proxies for records with base types cannot be used. Reported by multiple customers. No known workaround.

How found

Multiple customer reports on 6.0.

Regression

Yes, From 5.0

Testing

Added unit and functional tests for records with base types.

Risk

Very low; Clone methods are never used by EF proxies. Also added quirk to revert to previous behavior.
  • Loading branch information
ajcvickers committed Jan 6, 2022
1 parent 241c4ee commit 190dae0
Show file tree
Hide file tree
Showing 4 changed files with 111 additions and 3 deletions.
19 changes: 16 additions & 3 deletions src/EFCore.Proxies/Proxies/Internal/ProxyFactory.cs
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
using System;
using System.Collections.Generic;
using System.ComponentModel;
using System.Reflection;
using Castle.DynamicProxy;
using Microsoft.EntityFrameworkCore.Diagnostics;
using Microsoft.EntityFrameworkCore.Infrastructure;
Expand All @@ -25,6 +26,11 @@ public class ProxyFactory : IProxyFactory
private static readonly Type _notifyPropertyChangedInterface = typeof(INotifyPropertyChanged);
private static readonly Type _notifyPropertyChangingInterface = typeof(INotifyPropertyChanging);

private static readonly ProxyGenerationOptions _generationOptions
= AppContext.TryGetSwitch("Microsoft.EntityFrameworkCore.Issue26602", out var enabled) && enabled
? ProxyGenerationOptions.Default
: new(new ClonelessProxyGenerationHook());

private readonly ProxyGenerator _generator = new();

/// <summary>
Expand Down Expand Up @@ -64,7 +70,7 @@ public virtual Type CreateProxyType(
=> _generator.ProxyBuilder.CreateClassProxyType(
entityType.ClrType,
GetInterfacesToProxy(options, entityType.ClrType),
ProxyGenerationOptions.Default);
_generationOptions);

/// <summary>
/// This is an internal API that supports the Entity Framework Core infrastructure and not subject to
Expand Down Expand Up @@ -99,7 +105,7 @@ private object CreateLazyLoadingProxy(
=> _generator.CreateClassProxy(
entityType.ClrType,
GetInterfacesToProxy(options, entityType.ClrType),
ProxyGenerationOptions.Default,
_generationOptions,
constructorArguments,
GetNotifyChangeInterceptors(options, entityType, new LazyLoadingInterceptor(entityType, loader)));

Expand Down Expand Up @@ -142,7 +148,7 @@ private object CreateProxy(
=> _generator.CreateClassProxy(
entityType.ClrType,
GetInterfacesToProxy(options, entityType.ClrType),
ProxyGenerationOptions.Default,
_generationOptions,
constructorArguments,
GetNotifyChangeInterceptors(options, entityType));

Expand Down Expand Up @@ -200,5 +206,12 @@ private IInterceptor[] GetNotifyChangeInterceptors(

return interceptors.ToArray();
}

private sealed class ClonelessProxyGenerationHook : AllMethodsHook
{
public override bool ShouldInterceptMethod(Type type, MethodInfo methodInfo)
=> methodInfo.Name != "<Clone>$"
&& base.ShouldInterceptMethod(type, methodInfo);
}
}
}
20 changes: 20 additions & 0 deletions test/EFCore.Proxies.Tests/ProxyTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,15 @@ public void CreateProxy_works_for_shared_type_entity_types()
Assert.Same(typeof(SharedTypeEntityType), context.Set<SharedTypeEntityType>("STET1").CreateProxy(_ => { }).GetType().BaseType);
}

[ConditionalFact]
public void CreateProxy_works_for_record_with_base_type_entity_types()
{
using var context = new NeweyContext();

Assert.Same(typeof(March86C), context.Set<March86C>().CreateProxy().GetType().BaseType);
Assert.Same(typeof(March86C), context.Set<March86C>().CreateProxy(_ => { }).GetType().BaseType);
}

[ConditionalFact]
public void CreateProxy_throws_for_shared_type_entity_types_when_entity_type_name_not_known()
{
Expand Down Expand Up @@ -345,6 +354,15 @@ public class IsOwnedButNotWeak
{
}

public record March86C : IndyCar
{
public virtual int Id { get; init; }
}

public record IndyCar
{
}

private class NeweyContext : DbContext
{
private readonly IServiceProvider _internalServiceProvider;
Expand Down Expand Up @@ -418,6 +436,8 @@ protected override void OnModelCreating(ModelBuilder modelBuilder)
modelBuilder.SharedTypeEntity<SharedTypeEntityType>("STET2");

modelBuilder.Entity<WithWeak>();

modelBuilder.Entity<March86C>();
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -388,6 +388,8 @@ protected override void OnModelCreating(ModelBuilder modelBuilder, DbContext con
.HasForeignKey<Car>("fk_PersonId")
.IsRequired();
});

modelBuilder.Entity<RecordCar>();
}

protected virtual object CreateFullGraph(DbContext context)
Expand Down Expand Up @@ -1906,6 +1908,23 @@ public class Person
public virtual Car Vehicle { get; set; }
}

public record RecordBase
{
public virtual int Id { get; set; }
}

public record RecordCar : RecordBase
{
public virtual RecordPerson Owner { get; set; }
public virtual int? OwnerId { get; set; }
}

public record RecordPerson : RecordBase
{
public virtual ICollection<RecordCar> Vehicles { get; }
= new ObservableHashSet<RecordCar>(LegacyReferenceEqualityComparer.Instance);
}

protected DbContext CreateContext()
=> Fixture.CreateContext();

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,62 @@ public virtual void Attempting_to_save_two_entity_cycle_with_lazy_loading_throws
});
}

[ConditionalFact]
public virtual void Can_use_record_proxies_with_base_types_to_load_reference()
{
ExecuteWithStrategyInTransaction(
context =>
{
context.AddRange(
context.CreateProxy<RecordCar>(
car =>
{
car.Owner = context.CreateProxy<RecordPerson>();
}));
context.SaveChanges();
},
context =>
{
var car = context.Set<RecordCar>().Single();
if (!DoesLazyLoading)
{
context.Entry(car).Reference(e => e.Owner).Load();
}
Assert.Equal(car.Owner.Id, car.OwnerId);
Assert.Same(car, car.Owner.Vehicles.Single());
});
}

[ConditionalFact]
public virtual void Can_use_record_proxies_with_base_types_to_load_collection()
{
ExecuteWithStrategyInTransaction(
context =>
{
context.AddRange(
context.CreateProxy<RecordCar>(
car =>
{
car.Owner = context.CreateProxy<RecordPerson>();
}));
context.SaveChanges();
},
context =>
{
var owner = context.Set<RecordPerson>().Single();
if (!DoesLazyLoading)
{
context.Entry(owner).Collection(e => e.Vehicles).Load();
}
Assert.Equal(owner.Id, owner.Vehicles.Single().Id);
Assert.Same(owner, owner.Vehicles.Single().Owner);
});
}

[ConditionalFact]
public virtual void Avoid_nulling_shared_FK_property_when_deleting()
{
Expand Down

0 comments on commit 190dae0

Please sign in to comment.