-
Notifications
You must be signed in to change notification settings - Fork 936
Fix TypeLoadException in StaticProxyFactory when class has not visible interfaces #1644
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
Conversation
@@ -74,6 +74,8 @@ public TypeInfo CreateProxyType(System.Type baseType, IReadOnlyCollection<System | |||
interfaces.Add(baseType); | |||
} | |||
|
|||
interfaces.RemoveWhere(i => !i.IsVisible); | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't it emit a warning there?
It is not the dynamic proxy (which should be removed once lazy properties are handled by static proxies).
Something like "Interface {0} is not accessible, its members may not be proxified for entity {1}. If they are explicitly implemented and access directly some internal state of the entity, unexpected outcome may occur.".
Due to the way NHibernate's proxies work, it could even be considered that trying to proxify entities implementing some internal interfaces is an error. But since the old proxies were not detecting the case and were letting silently these members unproxified, it would be a breaking change doable only for 6.0.
(It is an error because if some proxy instance is given back to some methods of the assembly holding the internal interfaces, it may call those methods. If they access directly internal or private state, they will then access the inherited proxy state instead of the delegated entity state handled by the proxy.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, the error case would also require, for actually being an error, to have these methods explicitly implemented. So unless this additional condition can be reliably detected, it should not be rejected as an error.
(Proposed warning message updated.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we need any warnings here.
There is a possibility to improve that behaviour in a future major or minor version.
|
I am not really convinced it is worth bothering with And well, the interface being internal is not a trouble if none of its members is declared explicitly: they will then be proxified as public virtual members of the class. It is only if the interface is internal and if some of its members are explicitly implemented that a trouble can occur, because those members will not be proxified. (But if their implementations only use other visible members, proxified, then it will still not cause any trouble.) |
Agree
We could build either signed (with known key) or unsigned assembly depending on the circumstances, it should not be a problem. |
Fixes #1643