Details
-
Type:
Bug
-
Status:
Closed
-
Priority:
Major
-
Resolution: Fixed
-
Affects Version/s: None
-
Fix Version/s: 1.2
-
Component/s: PicoContainer (Java)
-
Labels:None
-
Number of attachments :
Description
Add this to ComponentKeysTestCase if you may, my ssh cvs is still borked. (Are we moving to svn anytime soon?)
public void testComponentKeysFromParentCannotConfuseTheChild() throws Exception
{ DefaultPicoContainer pico = new DefaultPicoContainer(); pico.registerComponentImplementation("test", SimpleTouchable.class); DefaultPicoContainer child = new DefaultPicoContainer(pico); child.registerComponentImplementation("test", DependsOnTouchable.class); DependsOnTouchable dot = (DependsOnTouchable) child.getComponentInstance("test"); assertNotNull(dot); }You will see it breaks, changing one key will fix it. However I feel that parent keys shouldn't be able to break a child container.
The code in question is at CICA:
// we can't depend on ourself
if (adapter.equals(this))
else
{ adapterDependencies.add(adapter); }I'd probably just delete the "else if".
I remember having a discussion with Aslak about unique keys accross the tree but I can't find it in the archive.
Activity
I think there is a precidence issue at heart here.
Some might view that parent containers should be able to assert themselves over child containers (for reasons of security :: sandbox concept) for keys registered.
Some might view that we should leave it open, and allow child containers to overwrite (within scope) keys previously registered by parent containers.
Thoughts?
Paul Hammant
added a comment - I think there is a precidence issue at heart here.
Some might view that parent containers should be able to assert themselves over child containers (for reasons of security :: sandbox concept) for keys registered.
Some might view that we should leave it open, and allow child containers to overwrite (within scope) keys previously registered by parent containers.
Thoughts? I don't think there is any precedence issue involved here.
Its just that a ComponentAdapter checks wether the matching dependency has the same key as itself. This case can never happen for the same container since its checked at registration so its always from a parent/child.
We had another issue some time ago concerning wether keys should be unique accross the whole tree but we agreed that it would be a restriction we don't need.
http://lists.codehaus.org/pipermail/picocontainer-dev/2004-March/002946.html
I don't think this issue is so critical and I'd propose we just remove the key comparison. A cyclic dependency is detected otherwise and also handled via adapter.equals(other).
Thomas Heller
added a comment - I don't think there is any precedence issue involved here.
Its just that a ComponentAdapter checks wether the matching dependency has the same key as itself. This case can never happen for the same container since its checked at registration so its always from a parent/child.
We had another issue some time ago concerning wether keys should be unique accross the whole tree but we agreed that it would be a restriction we don't need.
http://lists.codehaus.org/pipermail/picocontainer-dev/2004-March/002946.html
I don't think this issue is so critical and I'd propose we just remove the key comparison. A cyclic dependency is detected otherwise and also handled via adapter.equals(other). The issue is still valid, but the offending code is meanwhile in BasicComponentAdapter.resolveInstance:
// can't depend on ourselves
if (adapter != null && adapter.getComponentKey().equals(result.getComponentKey()))
Jörg Schaible
added a comment - The issue is still valid, but the offending code is meanwhile in BasicComponentAdapter.resolveInstance:
// can't depend on ourselves
if (adapter != null && adapter.getComponentKey().equals(result.getComponentKey()))
{
return null;
} I am not sure if this does still happen with the current code. Must be validated.
Jörg Schaible
added a comment - I am not sure if this does still happen with the current code. Must be validated. Jörg Schaible
made changes -
| Field | Original Value | New Value |
|---|---|---|
| Environment | ||
| Fix Version/s | 1.2 [ 11330 ] |
Jörg Schaible
made changes -
| Assignee | Joerg Schaible [ joehni ] |
Yes - issue is still valid in current code.
I'll add a test to the CICATestCase as soon as monitoring refactor is done.
Mauro Talevi
added a comment - Yes - issue is still valid in current code.
I'll add a test to the CICATestCase as soon as monitoring refactor is done. Jörg Schaible
made changes -
| Component/s | PicoContainer (Java) [ 10191 ] |
This is currently not solvable. The culprit is now in BasicComponentParameter:
public Object resolveInstance(PicoContainer container, ComponentAdapter adapter, Class expectedType)
throws PicoInstantiationException {
final ComponentAdapter componentAdapter = resolveAdapter(container, adapter, expectedType);
if (componentAdapter != null)
return null;
}
"resolveAdapter" will find the Touchable in the parent. Since it returns only the found adapter, but not where it found it, we get stuck at the container.getComponentInstance call. Since the current container has also a component with the same key, it will fail. Unfortunately we cannot use the CA directly:
componentAdapter.getComponentInstance(container);
While it solves the test above, it will violate another, since now a component from the parent can be satisfied with parameters from the current container.
Any ideas ?
Jörg Schaible
added a comment - This is currently not solvable. The culprit is now in BasicComponentParameter:
public Object resolveInstance(PicoContainer container, ComponentAdapter adapter, Class expectedType)
throws PicoInstantiationException {
final ComponentAdapter componentAdapter = resolveAdapter(container, adapter, expectedType);
if (componentAdapter != null)
{
return container.getComponentInstance(componentAdapter.getComponentKey());
}
return null;
}
"resolveAdapter" will find the Touchable in the parent. Since it returns only the found adapter, but not where it found it, we get stuck at the container.getComponentInstance call. Since the current container has also a component with the same key, it will fail. Unfortunately we cannot use the CA directly:
componentAdapter.getComponentInstance(container);
While it solves the test above, it will violate another, since now a component from the parent can be satisfied with parameters from the current container.
Any ideas ? This was solved in Avalon by cascading a failed lookup to the parent container.
In DPC.getInstance(), rather than
Object instance = componentAdapter.getComponentInstance(this);
do
Object instance = null;
try
catch( PicoInitializationException e )
{
if( parent != null )
throw e;
}
catch( PicoIntrospectionException e )
{
if( parent != null )
{ return parent.getComponentInstance( componentAdapter.getComponentKey() ); }
throw e;
}
(that's totally patched together in minimal time, but it makes the provided test pass).
peter royal
added a comment - This was solved in Avalon by cascading a failed lookup to the parent container.
In DPC.getInstance(), rather than
Object instance = componentAdapter.getComponentInstance(this);
do
Object instance = null;
try
{
instance = componentAdapter.getComponentInstance(this);
}
catch( PicoInitializationException e )
{
if( parent != null )
{
return parent.getComponentInstance( componentAdapter.getComponentKey() );
}
throw e;
}
catch( PicoIntrospectionException e )
{
if( parent != null )
{
return parent.getComponentInstance( componentAdapter.getComponentKey() );
}
throw e;
}
(that's totally patched together in minimal time, but it makes the provided test pass). Applied Peter's fix
Paul Hammant
added a comment - Applied Peter's fix Paul Hammant
made changes -
| Status | Open [ 1 ] | Closed [ 6 ] |
| Resolution | Fixed [ 1 ] |
Ah whoops I switched to browsing the archive wihout finishing my sentence.
... I'd probably just delete the "else if". Or add
&& getContainer().equals(adapter.getContainer()))
if it makes sense, however I think the whole check for the key is obsolete since its testet at registration so one container can only contain a key once.