> The problem is with interface methods, which do not need to be virtual to be overridden.
Interface methods implicitly implemented on the base class must be virtual to be overridden. See this fiddle <https://dotnetfiddle.net/h2r2uR>, which simulates a NHibernate proxy. With such a setup, the only way to get this fake override to work is to cast the proxy to the interface. But most proxy usages are done with a variable typed as the base class, not as one of its interfaces. And anyway, an actual override must also be called when we call the method through a variable typed as the base class. That is why the proxy factory does something more like this. <https://dotnetfiddle.net/32kCEo> > The NHibernate code is already doing the proper validity checks. It then, however, proceeds to generate code that only works in some cases. When I run Cfg.Environment.BytecodeProvider.ProxyFactoryFactory.ProxyValidator.ValidateType(typeof(A)), with A defined as here <https://dotnetfiddle.net/h2r2uR>, it returns two validation errors. (Regular return, that is not a throw: the throw is done in the Configuration class.) Le lundi 1 octobre 2018 03:19:08 UTC+2, Patrick Earl a écrit : > > I also originally thought the problem was related to base classes, but it > is not. The problem is with interface methods, which do not need to be > virtual to be overridden. The NHibernate code is already doing the proper > validity checks. It then, however, proceeds to generate code that only > works in some cases. > > On Sat, Sep 29, 2018, 4:18 AM Frédéric Delaporte wrote: > >> It should target master. It looks unlikely to me another 5.1.x will be >> released any time soon. >> >> But I doubt about the validity of the change. It seems to me that >> NHibernate proxies always had the mandatory prerequisite of having, in the >> proxied class, all public/protected methods and properties as virtual. >> >> There are checks done about this, but outside of the proxy factory >> itself. They are done in the configuration object. (See >> Configuration.ValidateEntities.) So it looks to me that Envers is using the >> proxy factory for its own proxies without performing the same validity >> checks that NHibernate does. >> >> The trouble with the change is the following: what will happen when >> calling such a property/method on a variable typed as the base class? The >> non-proxied base method will be called, instead of the proxied one, >> defeating the proxy purpose. >> >> I think the right fix is to perform in Envers the same validity checks >> than NHibernate does for classes that are to be proxified. >> >> Otherwise maybe Envers needs to use another proxy generator, more >> suitable to its own needs. >> >> >> Le vendredi 28 septembre 2018 22:25:16 UTC+2, Patrick Earl a écrit : >>> >>> Alexander, which branch would you like the TypeLoadException PR against? >>> We are short on time at this moment so putting the PR together will take >>> some time. However, I have provided the details below, including the line >>> of code to reproduce. >>> >>> The fix is for situations where we have something like >>> * class AbstractPersisentCollection : ILazyCollectionInitializer >>> * ILazyCollectionInitiatializer has a WasInitialized property >>> * AbstractPersistentCollection has a final WasInitialized property >>> * We create a proxy via ProxyFactory and ask it to provide the >>> ILazyCollectionInitialized interface. >>> >>> In such a case the explicit interface implementation method created for >>> the proxy conflicts with the method on AbstractPersistentCollection and >>> causes the TypeLoadException. >>> >>> The change, that follows, copies the .NET method attributes and naming >>> for explicitly implemented interface methods. >>> >>> It may be interesting to note that using NewSlot or changing the name >>> alone both prevented the TypeLoadException as well, but I figured it was >>> cleaner to go with what .NET would do itself when generating the explicit >>> interface method definitions. >>> >>> I realize there's no test case associated with it yet. To reproduce the >>> error you can do: >>> >>> new ProxyFactory().CreateProxy(typeof(PersistentGenericBag<SomeEntity>), >>> new SomeInterceptor(), typeof(ILazyInitializedCollection)) >>> >>> The fix is included below: >>> >>> src/NHibernate/Proxy/ProxyBuilderHelper.cs | 20 ++++++++++++++++++-- >>> 1 file changed, 18 insertions(+), 2 deletions(-) >>> >>> diff --git a/src/NHibernate/Proxy/ProxyBuilderHelper.cs >>> b/src/NHibernate/Proxy/ProxyBuilderHelper.cs >>> index 94af19db9..cd04ab4db 100644 >>> --- a/src/NHibernate/Proxy/ProxyBuilderHelper.cs >>> +++ b/src/NHibernate/Proxy/ProxyBuilderHelper.cs >>> @@ -137,13 +137,29 @@ 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; >>> + } >>> + else >>> + { >>> + methodAttributes = MethodAttributes.Public | >>> MethodAttributes.HideBySig | MethodAttributes.Virtual; >>> + } >>> >>> if (method.IsSpecialName) >>> methodAttributes |= MethodAttributes.SpecialName; >>> >>> var parameters = method.GetParameters(); >>> - >>> var methodBuilder = typeBuilder.DefineMethod( >>> name, >>> methodAttributes, >>> >>> >>> -- >> >> --- >> You received this message because you are subscribed to the Google Groups >> "nhibernate-development" group. >> To unsubscribe from this group and stop receiving emails from it, send an >> email to nhibernate-development+unsubscr...@googlegroups.com. >> For more options, visit https://groups.google.com/d/optout. >> > -- --- You received this message because you are subscribed to the Google Groups "nhibernate-development" group. To unsubscribe from this group and stop receiving emails from it, send an email to nhibernate-development+unsubscr...@googlegroups.com. For more options, visit https://groups.google.com/d/optout.
