GH-1881 - Fix interface method attributes when generating proxies

[PleasantD]

See #1881

[PleasantD]

Added the necessary test.
Also fixed the other tests that were failing, which included fixing an assumption about creating a proxy from an interface and fixing the IL generated for explicit implementations of interface methods.

[fredericDelaporte]

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.

[PleasantD]

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.

[fredericDelaporte]

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.)

[hazzik]

In the example above Bar will fail the validation.

[hazzik]

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.

[fredericDelaporte]

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.)

[PleasantD]

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

[fredericDelaporte]

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.

[PleasantD]

I suppose as an alternative we could just patch AbstractPersistentCollection.

Envers is generating this error because it generates proxies of the collections so it can hook into ILazyInitializedCollection.
AbstractPersistentCollection implements ILazyInitializedCollection but doesn't implement WasInitialized as virtual.

If we add the virtual keyword then it should fix Envers' issue with creating proxies of collections.

nhibernate-core/src/NHibernate/Collection/AbstractPersistentCollection.cs

Line 633 in 5c18452

public bool WasInitialized

[hazzik]

Agree

…

On Wed, 24 Oct 2018 at 10:47, Duncan Munro ***@***.***> wrote: I suppose as an alternative we could just fix AbstractPersistentCollection . Envers is generating this error because it generates proxies of the collections so it can hook into ILazyInitializedCollection. AbstractPersistentCollection implements ILazyInitializedCollection but doesn't implement WasInitialized as virtual. If we add the virtual keyword then it should fix Envers' issue with creating proxies of collections. https://github.com/nhibernate/nhibernate-core/blob/5c184527752b6e1fe7d40e91536dfdae300e287c/src/NHibernate/Collection/AbstractPersistentCollection.cs#L633 — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#1882 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAI1l3XgCURm2-fSLUf5BxWY6XdFtEG8ks5un45ogaJpZM4Xxhs9> .

[PleasantD]

How would you like to proceed with this?

Please let me know what needs to be done to fix this up or if you'd rather just patch AbstractPersistentCollection

[fredericDelaporte]

It seems patching AbstractPersistentCollection will be less controversial.

[hazzik]

This is not what .NET have for methods.

[fredericDelaporte]

I suppose as an alternative we could just patch AbstractPersistentCollection.

Envers is generating this error because it generates proxies of the collections so it can hook into ILazyInitializedCollection.
AbstractPersistentCollection implements ILazyInitializedCollection but doesn't implement WasInitialized as virtual.

If we add the virtual keyword then it should fix Envers' issue with creating proxies of collections.

nhibernate-core/src/NHibernate/Collection/AbstractPersistentCollection.cs

Line 633 in 5c18452

public bool WasInitialized

Looking into it, there are many more properties in AbstractPersistentCollection which are implicit non virtual interface implementations. They could trigger the same trouble, if their interface is included in the list of interfaces for the proxy to build.
The static proxy would include them if the proxy base type is the AbstractPersistentCollection class. The dynamic proxy would include them only if directly listed in the list of interfaces to implement.

So, proxified by the dynamic proxy, it may not trigger the issue. But would a proxy of AbstractPersistentCollection not also proxifying IPersistentCollection be actually valid? I do not think so.

So implementing WasInitialized as virtual will just dodge one issue for then falling in less obvious ones: having calls to IPersistentCollection methods or properties hitting the base state instead of the delegated state. Or else Envers needs to include IPersistentCollection in the proxied interfaces list, and then many more changes are required in AbstractPersistentCollection.

[fredericDelaporte]

For the same reason than #1881, closing.

Closing because of following:

1. Envers does not use the NHibernate's ProxyFactory anymore
2. The proxy validator correctly identifies that the alleged type is not proxyable