Details
-
Type:
Bug
-
Status:
Open
-
Priority:
Major
-
Resolution: Unresolved
-
Affects Version/s: 2.11
-
Fix Version/s: None
-
Component/s: PicoContainer (Java)
-
Labels:None
-
Testcase included:yes
-
Number of attachments :
Description
I think I found a bug in PicoContainer's ProviderAdapter:
If I use a Provider, the class of the provider is used during reinjection and not the type that's being returned by the provide method of the Provider. The corresponding lines of code are:
public Object getComponentInstance(PicoContainer container, Type into) throws PicoCompositionException { return new Reinjector(container).reinject(key, provider.getClass(), provider, properties, new MethodInjection(provideMethod)); }
I guess this should rather be
public Object getComponentInstance(PicoContainer container, Type into) throws PicoCompositionException { return new Reinjector(container).reinject(key, key, provider, properties, new MethodInjection(provideMethod)); }
Also, since ProviderAdapter takes Object and not Provider, you might want to set the Provider interface to deprecated since it's no longer needed.
Test case!
@Test
public void providerTest() {
DefaultPicoContainer container = new DefaultPicoContainer();
ProviderAdapter adapter = new ProviderAdapter(new BlorbProvider());
container.addAdapter(adapter);
assertNotNull(container.getComponent(Blorb.class));
}
public static class BlorbProvider {
public Blorb provide() {
return new Blorb();
}
}
public static class Blorb {}
Expected result: Returns a Blorb. Actual result: java.lang.ClassCastException: com.eaio.concurrent.DynamicBlockingQueueTest$BlorbProvider is not a com.eaio.concurrent.DynamicBlockingQueueTest$Blorb
Activity
Uhm, make that
if (injectionMethod.getReturnType().isAssignableFrom(componentImplementation) || injectionMethod.getDeclaringClass().isAssignableFrom(componentImplementation)) {
Johann Burkard
added a comment - Uhm, make that
if (injectionMethod.getReturnType().isAssignableFrom(componentImplementation) || injectionMethod.getDeclaringClass().isAssignableFrom(componentImplementation)) {
Johann, in respect of the main issue you've filed:
I (1) tried to run the unit tests (mvn clean install) for Pico, after making the s/provider.getClass()/key change. Many tests broke (try it).
Then I (2) changed the interface to insist on ProviderAdapter having Provider (rather than Object) put into it. You've not seen, but there is plenty of "instanceof Provider" in the codebase, even though there are no methods specified on it. Its a speed issue. I don't want to scan a class speculatively for a method named provide (with differing params) as I'd have to go through every class. That said, there could be an improvement. More to follow. Addressing the main issue: change list #5743
Paul Hammant
added a comment - Johann, in respect of the main issue you've filed:
I (1) tried to run the unit tests (mvn clean install) for Pico, after making the s/provider.getClass()/key change. Many tests broke (try it).
Then I (2) changed the interface to insist on ProviderAdapter having Provider (rather than Object) put into it. You've not seen, but there is plenty of "instanceof Provider" in the codebase, even though there are no methods specified on it. Its a speed issue. I don't want to scan a class speculatively for a method named provide (with differing params) as I'd have to go through every class. That said, there could be an improvement. More to follow. Addressing the main issue: change list #5743 In respect of the "if (injectionMethod.getReturnType().isAssignableFrom(componentImplementation) || injectionMethod.getDeclaringClass().isAssignableFrom(componentImplementation)) {" change, I can apply it and no unit tests fail, so I can't as easily object to your change in that respect.
But tell me, is there a test case that you have (or could make) that supports this case ?
Paul Hammant
added a comment - In respect of the "if (injectionMethod.getReturnType().isAssignableFrom(componentImplementation) || injectionMethod.getDeclaringClass().isAssignableFrom(componentImplementation)) {" change, I can apply it and no unit tests fail, so I can't as easily object to your change in that respect.
But tell me, is there a test case that you have (or could make) that supports this case ? If it's not obvious Johann, I'll wait for you comments, and not do a snap release 
Paul Hammant
added a comment - If it's not obvious Johann, I'll wait for you comments, and not do a snap release I have just tried implementing Provider and that made it work, however, maybe because of the check in AbstractAdapter?
So both the changes to ProviderAdapter (getClass() to key) and MethodInjection don't seem to be necessary as long as you ask for Provider and not Object.
And yes, I don't check the bugs I report every minute. I occasionally have other things to do. 
Johann Burkard
added a comment - I have just tried implementing Provider and that made it work, however, maybe because of the check in AbstractAdapter ?
So both the changes to ProviderAdapter (getClass() to key) and MethodInjection don't seem to be necessary as long as you ask for Provider and not Object .
And yes, I don't check the bugs I report every minute. I occasionally have other things to do. 
Oh, and you'll want to change MethodInjection from
if (injectionMethod.getDeclaringClass().isAssignableFrom(componentImplementation)) {to
if (injectionMethod.getReturnType().isAssignableFrom(componentImplementation)) {