Skip to content

Fix attempt of static proxies to call base method for abstract classes #1884

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Oct 27, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
157 changes: 152 additions & 5 deletions src/NHibernate.Test/StaticProxyTest/StaticProxyFactoryFixture.cs
Original file line number Diff line number Diff line change
Expand Up @@ -33,13 +33,57 @@ public class InternalInterfaceTestClass : IInternal

public interface IPublic
{
int Id { get; }
int Id { get; set; }
string Name { get; set; }
}

[Serializable]
public class PublicInterfaceTestClass : IPublic
{
public virtual int Id { get; set; }
public virtual string Name { get; set; }

public PublicInterfaceTestClass()
{
// Check access to properties from the default constructor do not fail once proxified
Id = -1;
Assert.That(Id, Is.EqualTo(-1));
Name = "Unknown";
Assert.That(Name, Is.EqualTo("Unknown"));
}
}

[Serializable]
public class PublicExplicitInterfaceTestClass : IPublic
{
int IPublic.Id { get; set; }
string IPublic.Name { get; set; }

public PublicExplicitInterfaceTestClass()
{
// Check access to properties from the default constructor do not fail once proxified
IPublic pub = this;
pub.Id = -1;
Assert.That(pub.Id, Is.EqualTo(-1));
pub.Name = "Unknown";
Assert.That(pub.Name, Is.EqualTo("Unknown"));
}
}

[Serializable]
public abstract class AbstractTestClass : IPublic
{
protected AbstractTestClass()
{
Id = -1;
Assert.That(Id, Is.Zero);
Name = "Unknown";
Assert.That(Name, Is.Null);
}

public abstract int Id { get; set; }

public abstract string Name { get; set; }
}

[Serializable]
Expand Down Expand Up @@ -156,8 +200,8 @@ public void VerifyProxyForClassWithAdditionalInterface()
// By way of the "proxy" attribute on the "class" mapping, an interface to use for the
// lazy entity load proxy instead of the persistentClass can be specified. This is "translated" into
// having an additional interface in the interface list, instead of just having INHibernateProxy.
// (Quite a loosy semantic...)
new HashSet<System.Type> {typeof(INHibernateProxy), typeof(IPublic)},
// (Quite a loose semantic...)
new HashSet<System.Type> { typeof(INHibernateProxy), typeof(IPublic) },
null, null, null);

#if NETFX
Expand All @@ -174,6 +218,85 @@ public void VerifyProxyForClassWithAdditionalInterface()
#endif
}

[Test]
public void VerifyProxyForClassWithInterface()
{
var factory = new StaticProxyFactory();
factory.PostInstantiate(
typeof(PublicInterfaceTestClass).FullName,
typeof(PublicInterfaceTestClass),
new HashSet<System.Type> {typeof(INHibernateProxy)},
null, null, null);

#if NETFX
VerifyGeneratedAssembly(
() =>
{
#endif
var proxy = factory.GetProxy(1, null);
Assert.That(proxy, Is.Not.Null);
Assert.That(proxy, Is.InstanceOf<IPublic>());
Assert.That(proxy, Is.InstanceOf<PublicInterfaceTestClass>());

// Check interface and implicit implementations do both call the delegated state
var state = new PublicInterfaceTestClass { Id = 5, Name = "State" };
proxy.HibernateLazyInitializer.SetImplementation(state);
var pub = (IPublic) proxy;
var ent = (PublicInterfaceTestClass) proxy;
Assert.That(pub.Id, Is.EqualTo(5), "IPublic.Id");
Assert.That(ent.Id, Is.EqualTo(5), "entity.Id");
Assert.That(pub.Name, Is.EqualTo("State"), "IPublic.Name");
Assert.That(ent.Name, Is.EqualTo("State"), "entity.Name");
ent.Id = 10;
pub.Name = "Test";
Assert.That(pub.Id, Is.EqualTo(10), "IPublic.Id");
Assert.That(state.Id, Is.EqualTo(10), "state.Id");
Assert.That(ent.Name, Is.EqualTo("Test"), "entity.Name");
Assert.That(state.Name, Is.EqualTo("Test"), "state.Name");
#if NETFX
});
#endif
}

[Test]
public void VerifyProxyForClassWithExplicitInterface()
{
var factory = new StaticProxyFactory();
factory.PostInstantiate(
typeof(PublicExplicitInterfaceTestClass).FullName,
typeof(PublicExplicitInterfaceTestClass),
new HashSet<System.Type> {typeof(INHibernateProxy)},
null, null, null);
#if NETFX
VerifyGeneratedAssembly(
() =>
{
#endif
var proxy = factory.GetProxy(1, null);
Assert.That(proxy, Is.Not.Null);
Assert.That(proxy, Is.InstanceOf<IPublic>());
Assert.That(proxy, Is.InstanceOf<PublicExplicitInterfaceTestClass>());

// Check interface and implicit implementations do both call the delegated state
IPublic state = new PublicExplicitInterfaceTestClass();
state.Id = 5;
state.Name = "State";
proxy.HibernateLazyInitializer.SetImplementation(state);
var entity = (IPublic) proxy;
Assert.That(entity.Id, Is.EqualTo(5), "Id");
Assert.That(entity.Name, Is.EqualTo("State"), "Name");

entity.Id = 10;
entity.Name = "Test";
Assert.That(entity.Id, Is.EqualTo(10), "entity.Id");
Assert.That(state.Id, Is.EqualTo(10), "state.Id");
Assert.That(entity.Name, Is.EqualTo("Test"), "entity.Name");
Assert.That(state.Name, Is.EqualTo("Test"), "state.Name");
#if NETFX
});
#endif
}

[Test]
public void VerifyProxyForRefOutClass()
{
Expand Down Expand Up @@ -219,6 +342,30 @@ public void VerifyProxyForRefOutClass()
#endif
}

[Test]
public void VerifyProxyForAbstractClass()
{
var factory = new StaticProxyFactory();
factory.PostInstantiate(
typeof(AbstractTestClass).FullName,
typeof(AbstractTestClass),
new HashSet<System.Type> { typeof(INHibernateProxy) },
null, null, null);

#if NETFX
VerifyGeneratedAssembly(
() =>
{
#endif
var proxy = factory.GetProxy(1, null);
Assert.That(proxy, Is.Not.Null);
Assert.That(proxy, Is.InstanceOf<IPublic>());
Assert.That(proxy, Is.InstanceOf<AbstractTestClass>());
#if NETFX
});
#endif
}

[Test]
public void InitializedProxyStaysInitializedAfterDeserialization()
{
Expand Down Expand Up @@ -369,10 +516,10 @@ public void VerifyFieldInterceptorProxyWithAdditionalInterface()
// By way of the "proxy" attribute on the "class" mapping, an interface to use for the
// lazy entity load proxy instead of the persistentClass can be specified. This is "translated" into
// having an additional interface in the interface list, instead of just having INHibernateProxy.
// (Quite a loosy semantic...)
// (Quite a loose semantic...)
// The field interceptor proxy ignores this setting, as it does not delegate its implementation
// to an instance of the persistentClass, and so cannot implement interface methods if it does not
// inherit the persitentClass.
// inherit the persistentClass.
new HashSet<System.Type> {typeof(INHibernateProxy), typeof(IPublic)},
null, null, null);
#if NETFX
Expand Down
91 changes: 80 additions & 11 deletions src/NHibernate/Proxy/NHibernateProxyBuilder.cs
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
using System;
using System.Collections.Generic;
using System.Linq;
using System.Linq.Expressions;
using System.Reflection;
using System.Reflection.Emit;
using System.Runtime.Serialization;
Expand Down Expand Up @@ -366,19 +367,20 @@ private static void ImplementCallMethodOnImplementation(
}

private static void EmitCallBaseIfLazyInitializerIsNull(
ILGenerator IL, MethodInfo method, FieldInfo lazyInitializerField, System.Type parentType)
ILGenerator IL,
MethodInfo method,
FieldInfo lazyInitializerField,
System.Type parentType)
{
/*
<if (method.DeclaringType.IsAssignableFrom(parentType))
{>
if (this.__lazyInitializer == null)
return base.<method>(args..)
<if (method.IsAbstract)
{>
return default;
<} else {>
return base.<method>(args..);
<}>
*/
if (!method.DeclaringType.IsAssignableFrom(parentType))
// The proxy does not derive from a type implementing the method, do not attempt
// calling its base. In such case, the lazy initializer is never null.
return;

// When deriving from the entity class, the entity class constructor may trigger
// virtual calls accessing the proxy state before its own constructor has a chance
Expand All @@ -393,9 +395,76 @@ private static void EmitCallBaseIfLazyInitializerIsNull(
IL.Emit(OpCodes.Ldnull);
IL.Emit(OpCodes.Bne_Un, skipBaseCall);

IL.Emit(OpCodes.Ldarg_0);
EmitCallMethod(IL, OpCodes.Call, method);
IL.Emit(OpCodes.Ret);
if (method.DeclaringType.IsInterface &&
method.DeclaringType.IsAssignableFrom(parentType))
{
var interfaceMap = parentType.GetInterfaceMap(method.DeclaringType);
var methodIndex = Array.IndexOf(interfaceMap.InterfaceMethods, method);
method = interfaceMap.TargetMethods[methodIndex];
}

if (method.IsAbstract)
{
/*
* return default(<ReturnType>);
*/

if (!method.ReturnType.IsValueType)
{
IL.Emit(OpCodes.Ldnull);

This comment was marked as resolved.

}
else if (method.ReturnType != typeof(void))
{
var local = IL.DeclareLocal(method.ReturnType);
IL.Emit(OpCodes.Ldloca, local);
IL.Emit(OpCodes.Initobj, method.ReturnType);
IL.Emit(OpCodes.Ldloc, local);
}

IL.Emit(OpCodes.Ret);
}
else if (method.IsPrivate)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is an explicit interface method.

Instead we can emit no-op (same as for the abstract method).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was not considering to do the effort of calling a base explicit implementation, since that is already not easily doable in C# code, and previous releases were not supporting it anyway. So no-op would have suited me. But now that it is done and tested, why not keeping it. It is still better.

{
/*
* var mi = (MethodInfo)MethodBase.GetMethodFromHandle(<method>, <parentType>);
* var delegate = (<delegateType>)mi.CreateDelegate(typeof(<delegateType>), this);
* delegate.Invoke(args...);
*/

var parameters = method.GetParameters();

var delegateType = Expression.GetDelegateType(
parameters.Select(p => p.ParameterType).Concat(new[] {method.ReturnType}).ToArray());

var invokeDelegate = delegateType.GetMethod("Invoke");

IL.Emit(OpCodes.Ldtoken, method);
IL.Emit(OpCodes.Ldtoken, parentType);
IL.Emit(OpCodes.Call, ReflectionCache.MethodBaseMethods.GetMethodFromHandleWithDeclaringType);
IL.Emit(OpCodes.Castclass, typeof(MethodInfo));
IL.Emit(OpCodes.Ldtoken, delegateType);
IL.Emit(OpCodes.Call, ReflectionCache.TypeMethods.GetTypeFromHandle);
IL.Emit(OpCodes.Ldarg_0);
IL.Emit(
OpCodes.Callvirt,
typeof(MethodInfo).GetMethod(
nameof(MethodInfo.CreateDelegate),
new[] {typeof(System.Type), typeof(object)}));
IL.Emit(OpCodes.Castclass, delegateType);

EmitCallMethod(IL, OpCodes.Callvirt, invokeDelegate);
IL.Emit(OpCodes.Ret);
}
else
Copy link
Member Author

@hazzik hazzik Oct 26, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With the current tests this branch is not needed at all. If the if condition replace with if(true) all tests still pass.

I'm considering to make a breaking change and throw exception if __lazyInitializer is null. Because the code in the constructor will not execute properly in any way.

An alternative to an exception would be to set __lazyInitializer before calling base constructor. This way the code will execute properly, but the proxy would be initialized.

{
/*
* base.<method>(args...);
*/

IL.Emit(OpCodes.Ldarg_0);
EmitCallMethod(IL, OpCodes.Call, method);
IL.Emit(OpCodes.Ret);
}

IL.MarkLabel(skipBaseCall);
}
Expand Down