Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Don't require entry point classes to have @Inject annotations. #57

Merged
merged 1 commit into from

3 participants

@swankjesse
Owner

This is necessary so that generic framework code can inject arbitrary
entry points (like JUnit test cases or Android activities) without
concern for whether the specific entry point has any @Inject annotations.

@swankjesse swankjesse Don't require entry point classes to have @Inject annotations.
This is necessary so that generic framework code can inject arbitrary
entry points (like JUnit test cases or Android activities) without
concern for whether the specific entry point has any @Inject annotations.
e27a178
@cgruber
Collaborator

Can you give an example? I'm not sure what you mean by "generic framework code can inject".

@pforhan pforhan merged commit 7222847 into master
@swankjesse
Owner

@cgruber some of our tests have a test runner that injects the test case:

@Override public void beforeTest(Method method) {
  makeObjectGraph().inject(this);
}

This is especially useful for integration testing.

Unfortunately we use the same test runner for several tests, and sometimes the injected test doesn't have any @Inject annotated fields. We need to avoid crashing in that case.

@cgruber
Collaborator

So inject() in this case is basically doing a no-op? I can see the utility, but it feels unsafe - like someone will pass in something with no @Inject and end up not realizing until run-time that nothing happened.

This feels like an alternate graph implementation with different behaviour suitable for testing, to be honest. I'm not sure this behaviour is the right one in all cases.

@cgruber
Collaborator

Or an injector with some configuration points with strict defaults.

@swankjesse
Owner

Yeah, the inject() call is a no-op. For us it's come up in several places, mostly in framework code that blindly injects an instance of an abstract type like TestCase or Activity.

Since the offending type still needs to be explicitly registered as an injection point, it's hard to be surprised by this.

@cgruber
Collaborator

Ah - fair point. Ok... as long as we keep some explicit signal there and don't end up with some situation where someone can trip over it. Still less strict than I'd like, but you're right - the class is at least deliberately added.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Oct 11, 2012
  1. @swankjesse

    Don't require entry point classes to have @Inject annotations.

    swankjesse authored
    This is necessary so that generic framework code can inject arbitrary
    entry points (like JUnit test cases or Android activities) without
    concern for whether the specific entry point has any @Inject annotations.
This page is out of date. Refresh to see the latest.
View
2  core/src/main/java/dagger/ObjectGraph.java
@@ -167,7 +167,7 @@ private void linkStaticInjections() {
private void linkEntryPoints() {
for (Map.Entry<String, Class<?>> entry : entryPoints.entrySet()) {
- linker.requestBinding(entry.getKey(), entry.getValue());
+ linker.requestEntryPoint(entry.getKey(), entry.getValue());
}
}
View
7 core/src/main/java/dagger/internal/AtInjectBinding.java
@@ -135,11 +135,10 @@ private AtInjectBinding(String provideKey, String membersKey, boolean singleton,
}
/**
- * @param forMembersInjection true if the binding is being created to inject
- * members only. Such injections do not require {@code @Inject}
+ * @param mustBeInjectable true if the binding must have {@code @Inject}
* annotations.
*/
- public static <T> Binding<T> create(Class<T> type, boolean forMembersInjection) {
+ public static <T> Binding<T> create(Class<T> type, boolean mustBeInjectable) {
boolean singleton = type.isAnnotationPresent(Singleton.class);
List<String> keys = new ArrayList<String>();
@@ -170,7 +169,7 @@ private AtInjectBinding(String provideKey, String membersKey, boolean singleton,
injectedConstructor = constructor;
}
if (injectedConstructor == null) {
- if (injectedFields.isEmpty() && !forMembersInjection) {
+ if (injectedFields.isEmpty() && mustBeInjectable) {
throw new IllegalArgumentException("No injectable members on " + type.getName()
+ ". Do you want to add an injectable constructor?");
}
View
37 core/src/main/java/dagger/internal/Linker.java
@@ -78,12 +78,14 @@ public final void linkRequested() {
Binding binding;
while ((binding = toLink.poll()) != null) {
if (binding instanceof DeferredBinding) {
- String key = ((DeferredBinding<?>) binding).deferredKey;
+ DeferredBinding deferredBinding = (DeferredBinding) binding;
+ String key = deferredBinding.deferredKey;
+ boolean mustBeInjectable = deferredBinding.mustBeInjectable;
if (bindings.containsKey(key)) {
continue; // A binding for this key has since been linked.
}
try {
- Binding<?> jitBinding = createJitBinding(key, binding.requiredBy);
+ Binding<?> jitBinding = createJitBinding(key, binding.requiredBy, mustBeInjectable);
// Fail if the type of binding we got wasn't capable of what was requested.
if (!key.equals(jitBinding.provideKey) && !key.equals(jitBinding.membersKey)) {
throw new IllegalStateException("Unable to create binding for " + key);
@@ -126,7 +128,8 @@ public final void linkRequested() {
* <li>Injections of other types will use the injectable constructors of those classes.
* </ul>
*/
- private Binding<?> createJitBinding(String key, Object requiredBy) throws ClassNotFoundException {
+ private Binding<?> createJitBinding(String key, Object requiredBy, boolean mustBeInjectable)
+ throws ClassNotFoundException {
String builtInBindingsKey = Keys.getBuiltInBindingsKey(key);
if (builtInBindingsKey != null) {
return new BuiltInBinding<Object>(key, requiredBy, builtInBindingsKey);
@@ -138,7 +141,7 @@ public final void linkRequested() {
String className = Keys.getClassName(key);
if (className != null && !Keys.isAnnotated(key)) {
- Binding<?> atInjectBinding = createAtInjectBinding(key, className);
+ Binding<?> atInjectBinding = createAtInjectBinding(key, className, mustBeInjectable);
if (atInjectBinding != null) {
return atInjectBinding;
}
@@ -151,8 +154,8 @@ public final void linkRequested() {
* Returns a binding that uses {@code @Inject} annotations, or null if no such
* binding can be created.
*/
- protected abstract Binding<?> createAtInjectBinding(String key, String className)
- throws ClassNotFoundException;
+ protected abstract Binding<?> createAtInjectBinding(
+ String key, String className, boolean mustBeInjectable) throws ClassNotFoundException;
/**
* Returns the binding if it exists immediately. Otherwise this returns
@@ -160,10 +163,24 @@ public final void linkRequested() {
* enqueued to be linked.
*/
public final Binding<?> requestBinding(String key, Object requiredBy) {
+ return requestBinding(key, true, requiredBy);
+ }
+
+ /**
+ * Like {@link #requestBinding}, but this doesn't require the referenced key
+ * to be injectable. This is necessary so that generic framework code can
+ * inject arbitrary entry points (like JUnit test cases or Android activities)
+ * without concern for whether the specific entry point is injectable.
+ */
+ public final Binding<?> requestEntryPoint(String key, Class<?> requiredByModule) {
+ return requestBinding(key, false, requiredByModule);
+ }
+
+ private Binding<?> requestBinding(String key, boolean mustBeInjectable, Object requiredBy) {
Binding<?> binding = bindings.get(key);
if (binding == null) {
// We can't satisfy this binding. Make sure it'll work next time!
- DeferredBinding<Object> deferredBinding = new DeferredBinding<Object>(key, requiredBy);
+ Binding<?> deferredBinding = new DeferredBinding(key, requiredBy, mustBeInjectable);
toLink.add(deferredBinding);
attachSuccess = false;
return null;
@@ -264,11 +281,13 @@ private SingletonBinding(Binding<T> binding) {
}
}
- private static class DeferredBinding<T> extends Binding<T> {
+ private static class DeferredBinding extends Binding<Object> {
final String deferredKey;
- private DeferredBinding(String deferredKey, Object requiredBy) {
+ final boolean mustBeInjectable;
+ private DeferredBinding(String deferredKey, Object requiredBy, boolean mustBeInjectable) {
super(null, null, false, requiredBy);
this.deferredKey = deferredKey;
+ this.mustBeInjectable = mustBeInjectable;
}
}
}
View
8 core/src/main/java/dagger/internal/RuntimeLinker.java
@@ -23,8 +23,8 @@
* and falls back to reflection.
*/
public final class RuntimeLinker extends Linker {
- @Override protected Binding<?> createAtInjectBinding(String key, String className)
- throws ClassNotFoundException {
+ @Override protected Binding<?> createAtInjectBinding(
+ String key, String className, boolean mustBeInjectable) throws ClassNotFoundException {
try {
Class<?> c = Class.forName(className + "$InjectAdapter");
Constructor<?> constructor = c.getConstructor();
@@ -40,7 +40,7 @@
return null;
}
- return AtInjectBinding.create(c, Keys.isMembersInjection(key));
+ return AtInjectBinding.create(c, mustBeInjectable);
}
@Override protected void reportErrors(List<String> errors) {
@@ -52,6 +52,6 @@
for (String error : errors) {
message.append("\n ").append(error);
}
- throw new IllegalArgumentException(message.toString());
+ throw new IllegalStateException(message.toString());
}
}
View
7 core/src/main/java/dagger/internal/codegen/AtInjectBinding.java
@@ -44,11 +44,10 @@ private AtInjectBinding(String provideKey, String membersKey,
}
/**
- * @param forMembersInjection true if the binding is being created to inject
- * members only. Such injections do not require {@code @Inject}
+ * @param mustBeInjectable true if the binding must have {@code @Inject}
* annotations.
*/
- static AtInjectBinding create(TypeElement type, boolean forMembersInjection) {
+ static AtInjectBinding create(TypeElement type, boolean mustBeInjectable) {
List<String> requiredKeys = new ArrayList<String>();
boolean hasInjectAnnotatedConstructor = false;
boolean isConstructable = false;
@@ -87,7 +86,7 @@ static AtInjectBinding create(TypeElement type, boolean forMembersInjection) {
}
}
- if (!hasInjectAnnotatedConstructor && requiredKeys.isEmpty() && !forMembersInjection) {
+ if (!hasInjectAnnotatedConstructor && requiredKeys.isEmpty() && mustBeInjectable) {
throw new IllegalArgumentException("No injectable members on "
+ type.getQualifiedName().toString() + ". Do you want to add an injectable constructor?");
}
View
6 core/src/main/java/dagger/internal/codegen/BuildTimeLinker.java
@@ -16,7 +16,6 @@
package dagger.internal.codegen;
import dagger.internal.Binding;
-import dagger.internal.Keys;
import dagger.internal.Linker;
import java.util.List;
import javax.annotation.processing.ProcessingEnvironment;
@@ -39,7 +38,8 @@
this.moduleName = moduleName;
}
- @Override protected Binding<?> createAtInjectBinding(String key, String className) {
+ @Override protected Binding<?> createAtInjectBinding(
+ String key, String className, boolean mustBeInjectable) {
String sourceClassName = className.replace('$', '.');
TypeElement type = processingEnv.getElementUtils().getTypeElement(sourceClassName);
if (type == null) {
@@ -52,7 +52,7 @@
if (type.getKind() == ElementKind.INTERFACE) {
return null;
}
- return AtInjectBinding.create(type, Keys.isMembersInjection(key));
+ return AtInjectBinding.create(type, mustBeInjectable);
}
@Override protected void reportErrors(List<String> errors) {
View
35 core/src/test/java/dagger/InjectionTest.java
@@ -247,7 +247,7 @@
try {
graph.validate();
fail();
- } catch (IllegalArgumentException expected) {
+ } catch (IllegalStateException expected) {
}
}
@@ -391,7 +391,7 @@
try {
graph.validate();
fail();
- } catch (IllegalArgumentException expected) {
+ } catch (IllegalStateException expected) {
}
}
@@ -408,7 +408,7 @@
try {
graph.validate();
fail();
- } catch (IllegalArgumentException expected) {
+ } catch (IllegalStateException expected) {
}
}
@@ -499,7 +499,7 @@
try {
graph.validate();
fail();
- } catch (IllegalArgumentException expected) {
+ } catch (IllegalStateException expected) {
}
}
@@ -593,4 +593,31 @@ BoundTwoWays provideBoundTwoWays() {
graph.inject(membersInjected);
assertEquals("Coke", membersInjected.s);
}
+
+ static class NoInjections {
+ }
+
+ @Test public void entryPointNeedsNoInjectAnnotation() {
+ @Module(entryPoints = NoInjections.class)
+ class TestModule {
+ }
+
+ ObjectGraph.get(new TestModule()).validate();
+ }
+
+ @Test public void nonEntryPointNeedsInjectAnnotation() {
+ @Module
+ class TestModule {
+ @Provides String provideString(NoInjections noInjections) {
+ throw new AssertionError();
+ }
+ }
+
+ ObjectGraph graph = ObjectGraph.get(new TestModule());
+ try {
+ graph.validate();
+ fail();
+ } catch (IllegalStateException expected) {
+ }
+ }
}
View
8 core/src/test/java/dagger/MembersInjectorTest.java
@@ -94,7 +94,7 @@
try {
graph.getInstance(TestEntryPoint.class);
fail();
- } catch (IllegalArgumentException expected) {
+ } catch (IllegalStateException expected) {
}
}
@@ -111,7 +111,7 @@
try {
graph.getInstance(TestEntryPoint.class);
fail();
- } catch (IllegalArgumentException expected) {
+ } catch (IllegalStateException expected) {
}
}
@@ -128,7 +128,7 @@
try {
graph.getInstance(TestEntryPoint.class);
fail();
- } catch (IllegalArgumentException expected) {
+ } catch (IllegalStateException expected) {
}
}
@@ -177,7 +177,7 @@
try {
graph.getInstance(TestEntryPoint.class);
fail();
- } catch (IllegalArgumentException expected) {
+ } catch (IllegalStateException expected) {
}
}
Something went wrong with that request. Please try again.