Skip to content

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

Merged
merged 1 commit into from
Apr 8, 2018

Conversation

hazzik
Copy link
Member

@hazzik hazzik commented Apr 8, 2018

Fixes #1643

@hazzik hazzik added the t: Fix label Apr 8, 2018
@hazzik hazzik added this to the 5.1.1 milestone Apr 8, 2018
@@ -74,6 +74,8 @@ public TypeInfo CreateProxyType(System.Type baseType, IReadOnlyCollection<System
interfaces.Add(baseType);
}

interfaces.RemoveWhere(i => !i.IsVisible);

Copy link
Member

@fredericDelaporte fredericDelaporte Apr 8, 2018

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

Copy link
Member

@fredericDelaporte fredericDelaporte Apr 8, 2018

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

Copy link
Member Author

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.

@hazzik hazzik merged commit 6e8f190 into nhibernate:master Apr 8, 2018
@hazzik hazzik deleted the only-visible-interfaces branch April 8, 2018 21:51
@hazzik hazzik added the r: Fixed label Apr 8, 2018
@hazzik
Copy link
Member Author

hazzik commented Apr 8, 2018

There is a possibility to improve that behaviour in a future major or minor version.

  1. Detect that assembly has InternalsVisibleToAttribute pointing to the assembly containing static proxy (it will require to settle the assembly name, as now it's dynamic).
  2. The problem with nested classes and interfaces - it is hard to properly identify if the class would be actually visible if InternalsVisibleToAttribute applies. To properly detect that it would require to traverse the nesting tree.
  3. Emit warning once per proxy type suggesting to make interfaces public or apply InternalsVisibleToAttribute.

@fredericDelaporte
Copy link
Member

I am not really convinced it is worth bothering with InternalsVisibleToAttribute. It works only if both assemblies are signed, or both unsigned.

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

@hazzik
Copy link
Member Author

hazzik commented Apr 8, 2018

Agree

It works only if both assemblies are signed, or both unsigned.

We could build either signed (with known key) or unsigned assembly depending on the circumstances, it should not be a problem.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants