Skip to content

GH-1881 - Fix interface method attributes when generating proxies #1882

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.

Sign up for GitHub

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

Closed
wants to merge 7 commits into from
Closed

GH-1881 - Fix interface method attributes when generating proxies #1882

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
53154a7
GH-1881 - Fix interface method attributes when generating proxies
PleasantD Oct 19, 2018
27deceb
fixup! GH-1881 - Fix interface method attributes when generating proxies
fredericDelaporte Oct 23, 2018
2000843
Merge remote-tracking branch 'origin/master' into GH1881
hazzik Oct 30, 2018
402f14c
fixup! GH-1881 - Fix interface method attributes when generating proxies
hazzik Oct 30, 2018
5c87d9a
fixup! GH-1881 - Fix interface method attributes when generating proxies
fredericDelaporte Oct 30, 2018
61ed84f
fixup! GH-1881 - Fix interface method attributes when generating proxies
fredericDelaporte Oct 30, 2018
783c575
Merge branch 'master' into GH1881
fredericDelaporte Oct 30, 2018
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
6 changes: 6 additions & 0 deletions src/NHibernate.Test/DynamicEntity/DataProxyHandler.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
using System;
using System.Collections;
using System.Runtime.CompilerServices;
using NHibernate.Proxy.DynamicProxy;

namespace NHibernate.Test.DynamicEntity
Expand Down Expand Up @@ -53,6 +54,11 @@ public object Intercept(InvocationInfo info)
{
return GetHashCode();
}
else if ("Equals".Equals(methodName))
{
// Redo the base object Equals implementation.
return RuntimeHelpers.Equals(info.Target, info.Arguments[0]);
}
return null;
}

Expand Down
33 changes: 33 additions & 0 deletions src/NHibernate.Test/NHSpecificTest/GH1881/TestFixture.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
using System;
using System.Linq;
using System.Reflection;
using NHibernate.Collection;
using NHibernate.Collection.Generic;
using NHibernate.Proxy.DynamicProxy;
using NUnit.Framework;

namespace NHibernate.Test.NHSpecificTest.GH1881
{
[TestFixture]
[Obsolete]
public class TestFixture
{
[Test]
public void InterfacesShouldBeImplementedExplicitlyOnProxies()
Copy link
Member

Choose a reason for hiding this comment

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

A static proxy test should also be added, since the dynamic proxy is to be dropped.

Example code for reproducing the issue with static proxy:

var factory = new StaticProxyFactory();
factory.PostInstantiate("PersistentGenericBag<object>", typeof(PersistentGenericBag<object>),
    new HashSet<System.Type> {typeof(ILazyInitializedCollection)}, null, null, null);
var proxy = factory.GetProxy(1, null);

Of course, a custom class should be used here too, for avoiding troubles due to PersistentGenericBag<object> being not meant to comply with proxification prerequisites.

The static test could be added in NHibernate.Test.StaticProxyTest.StaticProxyFactoryFixture, since NHSpecificTest is for things which do not really have a suitable feature test fixture. (Dynamic proxy test folder does not seem to contain a suitable fixture currently for the test here.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added the test and it fails

NHibernate.HibernateException : Creating a proxy instance failed
  ----> System.ArgumentException : Expression of type 'PersistentGenericBag`1Proxy' cannot be used for return type 'NHibernate.Proxy.INHibernateProxy'
   at NHibernate.Proxy.StaticProxyFactory.GetProxy(Object id, ISessionImplementor session) in D:\ExternalGit\nhibernate-core-pleasantd\src\NHibernate\Proxy\StaticProxyFactory.cs:line 31
   at NHibernate.Test.StaticProxyTest.StaticProxyFactoryFixture.VerifyInterfacesImplementedExplicitlyOnProxies() in D:\ExternalGit\nhibernate-core-pleasantd\src\NHibernate.Test\StaticProxyTest\StaticProxyFactoryFixture.cs:line 153
--ArgumentException
   at System.Linq.Expressions.Expression.ValidateLambdaArgs(Type delegateType, Expression& body, ReadOnlyCollection`1 parameters, String paramName)
   at System.Linq.Expressions.Expression.Lambda[TDelegate](Expression body, String name, Boolean tailCall, IEnumerable`1 parameters)
   at System.Linq.Expressions.Expression.Lambda[TDelegate](Expression body, Boolean tailCall, IEnumerable`1 parameters)
   at System.Linq.Expressions.Expression.Lambda[TDelegate](Expression body, ParameterExpression[] parameters)
   at NHibernate.Proxy.StaticProxyFactory.CreateProxyActivator(ProxyCacheEntry pke) in D:\ExternalGit\nhibernate-core-pleasantd\src\NHibernate\Proxy\StaticProxyFactory.cs:line 42
   at NHibernate.Proxy.StaticProxyFactory.<GetProxy>b__3_0(ProxyCacheEntry pke) in D:\ExternalGit\nhibernate-core-pleasantd\src\NHibernate\Proxy\StaticProxyFactory.cs:line 23
   at System.Collections.Concurrent.ConcurrentDictionary`2.GetOrAdd(TKey key, Func`2 valueFactory)
   at NHibernate.Proxy.StaticProxyFactory.GetProxy(Object id, ISessionImplementor session) in D:\ExternalGit\nhibernate-core-pleasantd\src\NHibernate\Proxy\StaticProxyFactory.cs:line 23

Copy link
Member

@fredericDelaporte fredericDelaporte Oct 23, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Yes, that should be new HashSet<System.Type> { typeof(INHibernateProxy) } instead.

But well, as per the comment I have posted some seconds ago here, it is maybe useless to support this in the static proxy factory, which is way less a general purpose proxifying mechanism than the dynamic proxy, and which may be unsuitable for Envers needs.

Update: it should only include INHibernateProxy, otherwise it will use the second interface as the base for the implementation. The ILazyInitializedCollection will be added by inspection of the base type.

{
var proxy = new ProxyFactory().CreateProxy(typeof(PersistentGenericBag<object>), null, typeof(ILazyInitializedCollection));
Copy link
Member

Choose a reason for hiding this comment

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

Adding a test:

[Test]
public void PassProxyValidation()
{
	var validator = new DynProxyTypeValidator();
	Assert.That(validator.ValidateType(typeof(PersistentGenericBag<object>)), Is.Null.Or.Count.EqualTo(0));
}

Shows that PersistentGenericBag<object> does not fulfil prerequisites for being a proxy, with a long list of errors. This may cause the test to fail for unrelated reasons with future changes. This test should use a custom class instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well... no, but that's kind of the point.
To reproduce the issue I would need a type that isn't validatable.

public interface IFoo
{
    bool Prop { get; }
}

public class Bar : IFoo
{
    public bool Prop => false;
}

// throws the same error
new ProxyFactory().CreateProxy(typeof(Bar), null, typeof(IFoo));

The proxy factory generates a proxy type for Bar, skipping Prop because it isn't proxyable.
But it adds the property from IFoo, which is proxyable, which is wrong and causes errors.
With the explicit implementation of the interface the proxy is valid for the interface, if not for the base type.

Copy link
Member

@fredericDelaporte fredericDelaporte Oct 23, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

It would be still better to use this type for the test than this big thing that PersistentGenericBag is.

So, there are actually no ways to reproduce the failure when using the dynamic proxy factory only through NHibernate's proxy handling, isn't it? (Or does Envers disable the default proxy validation in NHibernate? There is a setting allowing this.)

So this change looks needed for somewhat external users of the dynamic proxy factory, using it for their own needs with less strict prerequisites on proxied classes. But that is not the purpose of NHibernate to provide a general purpose proxifying mechanism. In this spirit the new static proxy factory API is way less exposed (more internal), and the old dynamic proxy should be gone in 6.0.

Maybe Envers will need to use an external proxy library rather than using the NHibernate internal one. (Unless it can be changed to use the static proxy factory, if it is not too specific to NHibernate for being used for Envers case. But I doubt about this.)

(In other words, this all looks to me at lot like this other discussion.)

Copy link
Member

Choose a reason for hiding this comment

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

In the example above Bar will fail the validation.

Copy link
Member

Choose a reason for hiding this comment

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

Or does Envers disable the default proxy validation in NHibernate? There is a setting allowing this.

They use it for something different - as a general purpose proxy factory to inject an interceptor, not for nhibernate entities.

Assert.That(proxy, Is.Not.Null);

foreach (var method in proxy.GetType().GetMethods().Where(m => m.DeclaringType == typeof(ILazyInitializedCollection)))
{
// These attributes are what .NET uses for explicitly implemented interface methods
Assert.That(method.Attributes, Is.EqualTo(MethodAttributes.Private |
MethodAttributes.Final |
MethodAttributes.Virtual |
MethodAttributes.HideBySig |
MethodAttributes.NewSlot |
MethodAttributes.SpecialName));
}
}
}
}
2 changes: 1 addition & 1 deletion src/NHibernate/Proxy/DynamicProxy/ProxyFactory.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,7 +118,7 @@ private TypeInfo CreateUncachedProxyType(System.Type baseType, IReadOnlyCollecti

// Provide a custom implementation of ISerializable
// instead of redirecting it back to the interceptor
foreach (MethodInfo method in ProxyBuilderHelper.GetProxiableMethods(baseType, interfaces).Where(method => method.DeclaringType != typeof(ISerializable)))
foreach (MethodInfo method in ProxyBuilderHelper.GetProxiableMethods(parentType, interfaces).Where(method => method.DeclaringType != typeof(ISerializable)))
{
ProxyMethodBuilder.CreateProxiedMethod(interceptorField, method, typeBuilder);
}
Expand Down
19 changes: 18 additions & 1 deletion src/NHibernate/Proxy/ProxyBuilderHelper.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -137,7 +137,24 @@ internal static MethodBuilder GetObjectDataMethodBuilder(TypeBuilder typeBuilder
internal static MethodBuilder GenerateMethodSignature(string name, MethodInfo method, TypeBuilder typeBuilder)
{
//TODO: Should we use attributes of base method?
var methodAttributes = MethodAttributes.Public | MethodAttributes.HideBySig | MethodAttributes.Virtual;
MethodAttributes methodAttributes;
if (method.DeclaringType.IsInterface)
{
// These are the attributes used for an explicit interface method implementation in .NET.
methodAttributes =
MethodAttributes.Private |
MethodAttributes.Final |
MethodAttributes.Virtual |
MethodAttributes.HideBySig |
MethodAttributes.NewSlot |
MethodAttributes.SpecialName;
// .NET uses an expanded name for explicit interface implementation methods.
name = typeBuilder.FullName + "." + method.DeclaringType.FullName + "." + name;
Copy link
Member

Choose a reason for hiding this comment

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

This is not what .NET have for methods.

}
else
{
methodAttributes = MethodAttributes.Public | MethodAttributes.HideBySig | MethodAttributes.Virtual;
}

if (method.IsSpecialName)
methodAttributes |= MethodAttributes.SpecialName;
Expand Down