Community
Participate
Working Groups
Build ID: I20070621-1340 Steps To Reproduce: 1.Create two plugins that are not dependent on each other 2.In the first plugin, have an IAdapterFactory class implementation that is registered programatically adapting (for example) IResource to a class defined in the second plugin (this class must be loaded via the Bundle api, since it is not visible on the class loader of the first plugin) 3. Ensure that both plugins are activated 4. Invoke AdapterManager.getAdapter(Object adaptable,String adapterType) and observe that the method returns null, when it should return the registered adapter. More information: This is problematic for plugins that wish to register an IAdapterFactory for an adapter type that is defined in another plugin that is not visible on the ClassLoader of the plugin defining the IAdapterFactory. The problem is apparent when AdapterManager.getAdapter(Object adaptable, String adapterType) is invoked, which is the case for enablement expressions that use adapt. The problem appears to be in the method AdapterManager.classForName(IAdapterFactory factory, String typeName) which assumes that the typeName is loadable via the factory.getClass().getClassLoader()
Created attachment 76135 [details] A candidate fix that uses an alternate method to find the class if the ClassLoader fails first
This issue has been sitting in the "inbox" for 6 weeks now.
Hmm... Submitted August 15, it is August 28th :-). But anyway: - In my quickly hacked test the patch did not work as "factory.getClass().getClassLoader().loadClass(typeName);" threw a ClassNotFoundException thus avoiding the code you added. Did it work for you? If so, could you attach an example (ideally, in a form of JUnit that fails with no patch and works with the patch). - More importantly, this seems like fixing a wrong place. Is there some reason why you can't add dependency on the bundle that contains the class you need? Optional dependency or dynamic import if this is something that might not always be available? That would solve the need to use Bundle API to load the class from that other plugin and will eliminate the problem with the adaptors you are having. - Another concern has to do with bundle activation. Depending on how IAdapterFactory is implemented by developers this could lead to extra bundles being activated. It is probably not going to be a problem in most cases, but there is one of anything under the moon. Overall, to me this seems like an edge case that has a potential to introduce new problem if modified as suggested. The straightforward way to deal with it would be to add bundle dependency to your bundle.
Okay, my date math was a little off :) I had not tested the patch thoroughly, which is why I did not get the ClassNotFoundException. I will attach a modified patch as you suggest. The use case for this fix is as follows: - plugin A may publish an extension point whereby any other plugin (B) may register a class implementing an interface defined in plugin A. - for each such implementing class defined in plugin B, plugin A may register an adapter factory at runtime - due to the nature of extension points, it may not be possible to know about all of the plugins using it at compile time (plugin A may be compiled and distributed before plugin B is created), hence it is impossible to add them as dependencies in the plugin A bundle manifest. I don't think that bundle activation in this case would be an issue since the platform should automatically activate plugin B when plugin A attempts to load the class declared in the extension point. I hope that the use case is clear. Please let me know if it is not clear. I'll attach the modified patch shortly. (In reply to comment #3) > Hmm... Submitted August 15, it is August 28th :-). > > But anyway: > > - In my quickly hacked test the patch did not work as > "factory.getClass().getClassLoader().loadClass(typeName);" threw a > ClassNotFoundException thus avoiding the code you added. > > Did it work for you? If so, could you attach an example (ideally, in a form of > JUnit that fails with no patch and works with the patch). > > - More importantly, this seems like fixing a wrong place. Is there some reason > why you can't add dependency on the bundle that contains the class you need? > Optional dependency or dynamic import if this is something that might not > always be available? That would solve the need to use Bundle API to load the > class from that other plugin and will eliminate the problem with the adaptors > you are having. > > - Another concern has to do with bundle activation. Depending on how > IAdapterFactory is implemented by developers this could lead to extra bundles > being activated. It is probably not going to be a problem in most cases, but > there is one of anything under the moon. > > Overall, to me this seems like an edge case that has a potential to introduce > new problem if modified as suggested. The straightforward way to deal with it > would be to add bundle dependency to your bundle. >
Created attachment 77182 [details] Modified version of the candidate fix that behaves properly when there is a ClassNotFoundException Modified the patch as per Oleg's suggestion
Created attachment 77894 [details] Updated patch This is essentially the same patch as David's - code is arranged in a different way and couple extra checks added. The actual use case to trigger the problem has to be a bit stranger than what you described however :-). There are two ways to create an adapter factory: (a) declarative via plugin.xml and (b) programmatically by registering with the adapter manager. If factory is registered programmatically, getAdapterList() is called and the code above is not triggered. Hence, for (b) adapters will work as they are. For (a) you have to describe the destination type in the plugin.xml. For this you have to know in advance the type you are adapting to. It is possible that you declare that you adapting to some generic type and then use another bundle to provide conversion to a subtype of that generic type (or a different conversion).
Created attachment 77895 [details] JUnit This patch adds JUnit to test the case above. Note that the .class file created from org.eclipse.core.tests.runtime.adapterLoader.TestAdaptableUnknown.java is not included as CVS can't include binary files in the patch.
Patches applied to CVS Head. David, thank you for help!
Thanks so much for getting this one in so quickly. My workaround for this one was ugly, using javaassist to subclass the adapter factory on another classloader. Thanks again!
