Bug 329282 - [DI] The current 'uninject' scenarios break the API contract
diff --git a/bundles/org.eclipse.e4.core.contexts/src/org/eclipse/e4/core/contexts/ContextInjectionFactory.java b/bundles/org.eclipse.e4.core.contexts/src/org/eclipse/e4/core/contexts/ContextInjectionFactory.java
index c748875..1d22684 100644
--- a/bundles/org.eclipse.e4.core.contexts/src/org/eclipse/e4/core/contexts/ContextInjectionFactory.java
+++ b/bundles/org.eclipse.e4.core.contexts/src/org/eclipse/e4/core/contexts/ContextInjectionFactory.java
@@ -46,7 +46,7 @@
* </p>
* <p>
* When a context is disposed, the injection factory will attempt to notify all injected objects by
- * calling methods with the <tt>javax.annotation.PreDestroy</tt> annotation before resetting injected values to <code>null</code>.
+ * calling methods with the <tt>javax.annotation.PreDestroy</tt> annotation.
*
* @noextend This class is not intended to be extended by clients.
* @noinstantiate This class is not intended to be instantiated by clients.
@@ -124,7 +124,8 @@
}
/**
- * Un-injects the context from the object.
+ * Un-injects the context from the object. The un-injection requires that all injected
+ * values were marked as optional, or the un-injection will fail.
*
* @param object The domain object previously injected with the context
* @param context The context previously injected into the object
diff --git a/bundles/org.eclipse.e4.core.di/src/org/eclipse/e4/core/di/IInjector.java b/bundles/org.eclipse.e4.core.di/src/org/eclipse/e4/core/di/IInjector.java
index e0f1509..97fe036 100644
--- a/bundles/org.eclipse.e4.core.di/src/org/eclipse/e4/core/di/IInjector.java
+++ b/bundles/org.eclipse.e4.core.di/src/org/eclipse/e4/core/di/IInjector.java
@@ -42,8 +42,7 @@
* </p>
* <p>
* When supplier is disposed, the injector will attempt to notify all injected objects by
- * calling methods with the {@link PreDestroy} annotation before resetting injected values
- * to <code>null</code>.
+ * calling methods with the {@link PreDestroy} annotation.
*
* @noimplement This interface is not intended to be implemented by clients.
* @noextend This interface is not intended to be extended by clients.
@@ -66,7 +65,8 @@
public void inject(Object object, PrimaryObjectSupplier objectSupplier) throws InjectionException;
/**
- * Un-injects the supplier from the object.
+ * Un-injects the supplier from the object. All un-injected values have to be optional,
+ * or un-injection will fail.
* @param object the domain object previously injected with the supplier's data
* @param objectSupplier primary object supplier for the injection
* @throws InjectionException if an exception occurred while performing this operation
diff --git a/bundles/org.eclipse.e4.core.di/src/org/eclipse/e4/core/internal/di/InjectorImpl.java b/bundles/org.eclipse.e4.core.di/src/org/eclipse/e4/core/internal/di/InjectorImpl.java
index daf3c65..624bf6a 100644
--- a/bundles/org.eclipse.e4.core.di/src/org/eclipse/e4/core/internal/di/InjectorImpl.java
+++ b/bundles/org.eclipse.e4.core.di/src/org/eclipse/e4/core/internal/di/InjectorImpl.java
@@ -43,12 +43,17 @@
import org.eclipse.e4.core.di.suppliers.IObjectDescriptor;
import org.eclipse.e4.core.di.suppliers.IRequestor;
import org.eclipse.e4.core.di.suppliers.PrimaryObjectSupplier;
+import org.eclipse.e4.core.internal.di.osgi.DIActivator;
+import org.eclipse.e4.core.internal.di.osgi.LogHelper;
/**
* Reflection-based dependency injector.
*/
public class InjectorImpl implements IInjector {
+ final static private String DEBUG_INJECTION = "org.eclipse.e4.core.di/debug/injector"; //$NON-NLS-1$
+ final static private boolean shouldDebug = DIActivator.getDefault().getBooleanDebugOption(DEBUG_INJECTION, false);
+
final static private String JAVA_OBJECT = "java.lang.Object"; //$NON-NLS-1$
final private static Boolean DEFAULT_BOOLEAN = new Boolean(false);
@@ -160,15 +165,32 @@
if (!forgetInjectedObject(object, objectSupplier))
return; // not injected at this time
processAnnotated(PreDestroy.class, object, object.getClass(), objectSupplier, null, new ArrayList<Class<?>>(5));
- // Two stages: first, go and collect {requestor, descriptor[] }
+
ArrayList<Requestor> requestors = new ArrayList<Requestor>();
processClassHierarchy(object, objectSupplier, null, true /* track */, false /* inverse order */, requestors);
- // might still need to get resolved values from secondary suppliers
- // Ask suppliers to fill actual values {requestor, descriptor[], actualvalues[] }
- resolveRequestorArgs(requestors, null, null, true /* fill with nulls */, false, false);
- // Call requestors in order
for (Requestor requestor : requestors) {
+ // Ask suppliers to fill actual values {requestor, descriptor[], actualvalues[] }
+ Object[] actualArgs = resolveArgs(requestor, null, null, true, false, false);
+ int unresolved = unresolved(actualArgs);
+ if (unresolved == -1) {
+ requestor.setResolvedArgs(actualArgs);
+ } else {
+ if (requestor.isOptional())
+ requestor.setResolvedArgs(null);
+ else {
+ if (shouldDebug) {
+ StringBuffer tmp = new StringBuffer();
+ tmp.append("Uninjecting object \""); //$NON-NLS-1$
+ tmp.append(object.toString());
+ tmp.append("\": dependency on \""); //$NON-NLS-1$
+ tmp.append(requestor.getDependentObjects()[unresolved].toString());
+ tmp.append("\" is not optional."); //$NON-NLS-1$
+ LogHelper.logError(tmp.toString(), null);
+ }
+ continue; // do not execute requestors with unresolved arguments
+ }
+ }
requestor.execute();
}
}
@@ -301,10 +323,19 @@
}
public void resolveArguments(IRequestor requestor, boolean initial) {
- ArrayList<Requestor> list = new ArrayList<Requestor>(1);
Requestor internalRequestor = ((Requestor) requestor);
- list.add(internalRequestor);
- resolveRequestorArgs(list, internalRequestor.getPrimarySupplier(), internalRequestor.getTempSupplier(), true, initial, internalRequestor.shouldTrack());
+ Object[] actualArgs = resolveArgs(internalRequestor, internalRequestor.getPrimarySupplier(), internalRequestor.getTempSupplier(), false, initial, internalRequestor.shouldTrack());
+ int unresolved = unresolved(actualArgs);
+ if (unresolved == -1)
+ internalRequestor.setResolvedArgs(actualArgs);
+ else {
+ if (internalRequestor.isOptional())
+ internalRequestor.setResolvedArgs(null);
+ else {
+ String msg = resolutionError(internalRequestor, unresolved);
+ LogHelper.logError(msg, null);
+ }
+ }
}
public void disposed(PrimaryObjectSupplier objectSupplier) {
@@ -321,14 +352,17 @@
}
}
for (int i = 0; i < count; i++) {
- uninject(objects[i], objectSupplier);
+ Object object = objects[i];
+ if (!forgetInjectedObject(object, objectSupplier))
+ continue; // not injected at this time
+ processAnnotated(PreDestroy.class, object, object.getClass(), objectSupplier, null, new ArrayList<Class<?>>(5));
}
forgetSupplier(objectSupplier);
}
- private void resolveRequestorArgs(ArrayList<Requestor> requestors, PrimaryObjectSupplier objectSupplier, PrimaryObjectSupplier tempSupplier, boolean fillNulls, boolean initial, boolean track) {
+ private void resolveRequestorArgs(ArrayList<Requestor> requestors, PrimaryObjectSupplier objectSupplier, PrimaryObjectSupplier tempSupplier, boolean uninject, boolean initial, boolean track) {
for (Requestor requestor : requestors) {
- Object[] actualArgs = resolveArgs(requestor, objectSupplier, tempSupplier, fillNulls, initial, track);
+ Object[] actualArgs = resolveArgs(requestor, objectSupplier, tempSupplier, uninject, initial, track);
int unresolved = unresolved(actualArgs);
if (unresolved == -1) {
requestor.setResolvedArgs(actualArgs);
@@ -343,16 +377,20 @@
}
private void reportUnresolvedArgument(Requestor requestor, int argIndex) {
+ throw new InjectionException(resolutionError(requestor, argIndex));
+ }
+
+ private String resolutionError(Requestor requestor, int argIndex) {
StringBuffer tmp = new StringBuffer();
tmp.append("Unable to process \""); //$NON-NLS-1$
tmp.append(requestor.toString());
tmp.append("\": no actual value was found for the argument \""); //$NON-NLS-1$
tmp.append(requestor.getDependentObjects()[argIndex].toString());
tmp.append("\"."); //$NON-NLS-1$
- throw new InjectionException(tmp.toString());
+ return tmp.toString();
}
- private Object[] resolveArgs(Requestor requestor, PrimaryObjectSupplier objectSupplier, PrimaryObjectSupplier tempSupplier, boolean fillNulls, boolean initial, boolean track) {
+ private Object[] resolveArgs(Requestor requestor, PrimaryObjectSupplier objectSupplier, PrimaryObjectSupplier tempSupplier, boolean uninject, boolean initial, boolean track) {
IObjectDescriptor[] descriptors = requestor.getDependentObjects();
// 0) initial fill - all values are unresolved
@@ -395,7 +433,7 @@
}
// 5) create simple classes (implied bindings) - unless we uninject or optional
- if (!fillNulls && !requestor.isOptional()) {
+ if (!uninject && !requestor.isOptional()) {
for (int i = 0; i < actualArgs.length; i++) {
if (actualArgs[i] != NOT_A_VALUE)
continue; // already resolved
@@ -436,7 +474,7 @@
actualArgs[i] = IInjector.NOT_A_VALUE;
}
if (actualArgs[i] == IInjector.NOT_A_VALUE) { // still unresolved?
- if (fillNulls || descriptors[i].hasQualifier(Optional.class)) { // uninject or optional - fill defaults
+ if (descriptors[i].hasQualifier(Optional.class)) { // uninject or optional - fill defaults
Class<?> descriptorsClass = getDesiredClass(descriptors[i].getDesiredType());
if (descriptorsClass.isPrimitive()) {
if (descriptorsClass.equals(boolean.class))
diff --git a/bundles/org.eclipse.e4.core.di/src/org/eclipse/e4/core/internal/di/osgi/LogHelper.java b/bundles/org.eclipse.e4.core.di/src/org/eclipse/e4/core/internal/di/osgi/LogHelper.java
index be7f561..b34b7bc 100644
--- a/bundles/org.eclipse.e4.core.di/src/org/eclipse/e4/core/internal/di/osgi/LogHelper.java
+++ b/bundles/org.eclipse.e4.core.di/src/org/eclipse/e4/core/internal/di/osgi/LogHelper.java
@@ -18,8 +18,16 @@
static final private String plugin_name = "org.eclipse.e4.core"; //$NON-NLS-1$
static public void logError(String msg, Throwable e) {
+ log(msg, FrameworkLogEntry.ERROR, e);
+ }
+
+ static public void logWarning(String msg, Throwable e) {
+ log(msg, FrameworkLogEntry.WARNING, e);
+ }
+
+ static public void log(String msg, int severity, Throwable e) {
FrameworkLog log = DIActivator.getDefault().getFrameworkLog();
- FrameworkLogEntry logEntry = new FrameworkLogEntry(plugin_name, FrameworkLogEntry.ERROR, 0, msg, 0, e, null);
+ FrameworkLogEntry logEntry = new FrameworkLogEntry(plugin_name, severity, 0, msg, 0, e, null);
log.log(logEntry);
}
}
diff --git a/tests/org.eclipse.e4.core.tests/src/org/eclipse/e4/core/internal/tests/contexts/inject/AnnotationsInjectionTest.java b/tests/org.eclipse.e4.core.tests/src/org/eclipse/e4/core/internal/tests/contexts/inject/AnnotationsInjectionTest.java
index 6fe02d4..1630774 100644
--- a/tests/org.eclipse.e4.core.tests/src/org/eclipse/e4/core/internal/tests/contexts/inject/AnnotationsInjectionTest.java
+++ b/tests/org.eclipse.e4.core.tests/src/org/eclipse/e4/core/internal/tests/contexts/inject/AnnotationsInjectionTest.java
@@ -170,7 +170,7 @@
@SuppressWarnings("unused")
@Inject
- public void injectedMethod(@Named("valueMethod") Object arg) {
+ public void injectedMethod(@Optional @Named("valueMethod") Object arg) {
try {
assertTrue(injectedField != null);
} catch (AssertionFailedError e) {
@@ -363,7 +363,7 @@
context.dispose();
assertEquals(1, object.preDestoryCalled);
- assertNull(object.value);
- assertNull(object.directFieldInjection);
+ assertNotNull(object.value);
+ assertNotNull(object.directFieldInjection);
}
}
diff --git a/tests/org.eclipse.e4.core.tests/src/org/eclipse/e4/core/internal/tests/contexts/inject/ContextInjectionDisposeTest.java b/tests/org.eclipse.e4.core.tests/src/org/eclipse/e4/core/internal/tests/contexts/inject/ContextInjectionDisposeTest.java
index ef3af96..bb75144 100644
--- a/tests/org.eclipse.e4.core.tests/src/org/eclipse/e4/core/internal/tests/contexts/inject/ContextInjectionDisposeTest.java
+++ b/tests/org.eclipse.e4.core.tests/src/org/eclipse/e4/core/internal/tests/contexts/inject/ContextInjectionDisposeTest.java
@@ -20,6 +20,7 @@
import org.eclipse.e4.core.contexts.ContextInjectionFactory;
import org.eclipse.e4.core.contexts.EclipseContextFactory;
import org.eclipse.e4.core.contexts.IEclipseContext;
+import org.eclipse.e4.core.di.annotations.Optional;
/**
* Tests for injection handling of context dispose, and handling disposal of injected objects.
@@ -92,10 +93,10 @@
assertEquals(fieldValue, object.Field);
assertEquals(methodValue, object.methodValue);
- // disposing context should clear values
+ // disposing context calls @PreDestory, but does not clear injected values
context.dispose();
- assertNull(object.Field);
- assertNull(object.methodValue);
+ assertNotNull(object.Field);
+ assertNotNull(object.methodValue);
assertTrue(object.disposeInvoked);
}
@@ -115,7 +116,7 @@
@SuppressWarnings("unused")
@Inject
- public void InjectedMethod(String arg) {
+ public void InjectedMethod(@Optional String arg) {
methodValue = arg;
}
}
@@ -134,7 +135,7 @@
// disposing the context does.
ContextInjectionFactory.uninject(object, context);
- assertNull(object.Field);
+ assertEquals(fieldValue, object.Field);
assertNull(object.methodValue);
assertTrue(object.disposeInvoked);
}
diff --git a/tests/org.eclipse.e4.core.tests/src/org/eclipse/e4/core/internal/tests/contexts/inject/ExtraDependenciesTest.java b/tests/org.eclipse.e4.core.tests/src/org/eclipse/e4/core/internal/tests/contexts/inject/ExtraDependenciesTest.java
index 2ca0087..2328442 100644
--- a/tests/org.eclipse.e4.core.tests/src/org/eclipse/e4/core/internal/tests/contexts/inject/ExtraDependenciesTest.java
+++ b/tests/org.eclipse.e4.core.tests/src/org/eclipse/e4/core/internal/tests/contexts/inject/ExtraDependenciesTest.java
@@ -96,8 +96,8 @@
// check that disposal of the injected context causes disposal of the injected object
context.dispose();
assertTrue(object.disposed);
- assertNull(object.string);
- assertNull(object.integer);
+ assertNotNull(object.string);
+ assertNotNull(object.integer);
assertNull(object.other);
}
diff --git a/tests/org.eclipse.e4.core.tests/src/org/eclipse/e4/core/internal/tests/contexts/inject/InjectStaticContextTest.java b/tests/org.eclipse.e4.core.tests/src/org/eclipse/e4/core/internal/tests/contexts/inject/InjectStaticContextTest.java
index 64270d4..f0f9033 100644
--- a/tests/org.eclipse.e4.core.tests/src/org/eclipse/e4/core/internal/tests/contexts/inject/InjectStaticContextTest.java
+++ b/tests/org.eclipse.e4.core.tests/src/org/eclipse/e4/core/internal/tests/contexts/inject/InjectStaticContextTest.java
@@ -21,6 +21,7 @@
import org.eclipse.e4.core.contexts.EclipseContextFactory;
import org.eclipse.e4.core.contexts.IEclipseContext;
import org.eclipse.e4.core.di.annotations.Execute;
+import org.eclipse.e4.core.di.annotations.Optional;
/**
* Tests for the context injection functionality using 2 contexts
@@ -39,12 +40,12 @@
public int preDestroyCalled = 0;
@Inject
- public void contextSet(IEclipseContext context) {
+ public void contextSet(@Optional IEclipseContext context) {
injectedContext = context;
}
@Inject
- public void setA(@Named("a") String aString) {
+ public void setA(@Optional @Named("a") String aString) {
this.aString = aString;
}
diff --git a/tests/org.eclipse.e4.core.tests/src/org/eclipse/e4/core/internal/tests/contexts/inject/ObjectBasic.java b/tests/org.eclipse.e4.core.tests/src/org/eclipse/e4/core/internal/tests/contexts/inject/ObjectBasic.java
index 294adc6..b55bfb9 100644
--- a/tests/org.eclipse.e4.core.tests/src/org/eclipse/e4/core/internal/tests/contexts/inject/ObjectBasic.java
+++ b/tests/org.eclipse.e4.core.tests/src/org/eclipse/e4/core/internal/tests/contexts/inject/ObjectBasic.java
@@ -15,6 +15,7 @@
import javax.inject.Inject;
import org.eclipse.e4.core.contexts.IEclipseContext;
+import org.eclipse.e4.core.di.annotations.Optional;
/**
* Test class to check injection mechanism
@@ -22,7 +23,7 @@
public class ObjectBasic {
// Injected directly
- @Inject
+ @Inject @Optional
public String injectedString;
@Inject
private Integer injectedInteger;
@@ -50,7 +51,7 @@
}
@Inject
- public void arguments(Float f, Character c) {
+ public void arguments(Float f, @Optional Character c) {
setMethodCalled2++;
this.f = f;
this.c = c;
diff --git a/tests/org.eclipse.e4.core.tests/src/org/eclipse/e4/core/internal/tests/di/DisposeClassLinkTest.java b/tests/org.eclipse.e4.core.tests/src/org/eclipse/e4/core/internal/tests/di/DisposeClassLinkTest.java
index 2efc39d..354c65b 100644
--- a/tests/org.eclipse.e4.core.tests/src/org/eclipse/e4/core/internal/tests/di/DisposeClassLinkTest.java
+++ b/tests/org.eclipse.e4.core.tests/src/org/eclipse/e4/core/internal/tests/di/DisposeClassLinkTest.java
@@ -118,7 +118,7 @@
context.dispose();
- assertNull("The object should have been uninjected", obj.context);
+ assertNotNull(obj.context);
assertEquals("@PostConstruct should only have been called once", 1, obj.postConstruct);
assertEquals("@PreDestroy should have been called during uninjection", 1, obj.preDestroy);
}
diff --git a/tests/org.eclipse.e4.core.tests/src/org/eclipse/e4/core/internal/tests/di/InjectBaseTypeTest.java b/tests/org.eclipse.e4.core.tests/src/org/eclipse/e4/core/internal/tests/di/InjectBaseTypeTest.java
index 18360d4..37e4f72 100644
--- a/tests/org.eclipse.e4.core.tests/src/org/eclipse/e4/core/internal/tests/di/InjectBaseTypeTest.java
+++ b/tests/org.eclipse.e4.core.tests/src/org/eclipse/e4/core/internal/tests/di/InjectBaseTypeTest.java
@@ -47,7 +47,7 @@
@Inject @Named("test_byte")
public byte byteField;
- @Inject @Named("test_boolean")
+ @Inject @Optional @Named("test_boolean")
public boolean booleanField;
@Inject @Named("test_char")
@@ -96,19 +96,21 @@
// test end-of-life reset of values
ContextInjectionFactory.uninject(testClass, context);
- assertEquals(0, testClass.intField);
- assertEquals(0, testClass.intFieldOptional);
- assertEquals(0L, testClass.longField);
- assertEquals(0f, testClass.floatField);
- assertEquals(0.0d, testClass.doubleField);
- assertEquals((short)0, testClass.shortField);
- assertEquals((byte)0, testClass.byteField);
- assertEquals(false, testClass.booleanField);
- assertEquals((char)0, testClass.charField);
+ // optional fields are reset to default;
+ // non-optional keep their values
+ assertEquals(12, testClass.intField);
+ assertEquals(0, testClass.intFieldOptional); // optional
+ assertEquals(124564523466L, testClass.longField);
+ assertEquals(12.34f, testClass.floatField);
+ assertEquals(12.34534534563463466546d, testClass.doubleField);
+ assertEquals((short)10, testClass.shortField);
+ assertEquals((byte)55, testClass.byteField);
+ assertEquals(false, testClass.booleanField); // optional
+ assertEquals('a', testClass.charField);
- assertEquals(0, testClass.intArg);
- assertEquals((char)0, testClass.charArg);
- assertEquals(false, testClass.booleanArg);
+ assertEquals(12, testClass.intArg);
+ assertEquals('a', testClass.charArg);
+ assertEquals(true, testClass.booleanArg);
}
}
