Fix https://github.com/google/guice/issues/888.
When Binder.requireExplicitBindings() is in effect, don't allow any
JIT bindings to be created in parent injectors, even JIT bindings that
are normally exempt such as the targets of linked key bindings.
diff --git a/core/src/com/google/inject/internal/Errors.java b/core/src/com/google/inject/internal/Errors.java
index f684e39..65f7e0b 100644
--- a/core/src/com/google/inject/internal/Errors.java
+++ b/core/src/com/google/inject/internal/Errors.java
@@ -140,6 +140,10 @@
return addMessage("Explicit bindings are required and %s is not explicitly bound.", key);
}
+ public Errors jitDisabledInParent(Key<?> key) {
+ return addMessage("Explicit bindings are required and %s would be bound in a parent injector.", key);
+ }
+
public Errors atInjectRequired(Class clazz) {
return addMessage(
"Explicit @Inject annotations are required on constructors,"
diff --git a/core/src/com/google/inject/internal/InjectorImpl.java b/core/src/com/google/inject/internal/InjectorImpl.java
index bcf481d..e20035a 100644
--- a/core/src/com/google/inject/internal/InjectorImpl.java
+++ b/core/src/com/google/inject/internal/InjectorImpl.java
@@ -101,7 +101,7 @@
NO_JIT,
/** allows existing just in time bindings, but does not allow new ones */
EXISTING_JIT,
- /** allows existing just in time bindings & allows new ones to be created */
+ /** allows existing just in time bindings & allows new ones to be created in the current injector */
NEW_OR_EXISTING_JIT,
}
@@ -779,10 +779,14 @@
boolean jitDisabled, JitLimitation jitType) throws ErrorsException {
// ask the parent to create the JIT binding
if (parent != null) {
- try {
- return parent.createJustInTimeBindingRecursive(key, new Errors(), jitDisabled,
- parent.options.jitDisabled ? JitLimitation.NO_JIT : jitType);
- } catch (ErrorsException ignored) {
+ if (jitDisabled && jitType == JitLimitation.NEW_OR_EXISTING_JIT) {
+ throw errors.jitDisabledInParent(key).toException();
+ } else {
+ try {
+ return parent.createJustInTimeBindingRecursive(key, new Errors(), jitDisabled,
+ parent.options.jitDisabled ? JitLimitation.NO_JIT : jitType);
+ } catch (ErrorsException ignored) {
+ }
}
}
diff --git a/core/test/com/google/inject/JitBindingsTest.java b/core/test/com/google/inject/JitBindingsTest.java
index 10ecb33..fd61021 100644
--- a/core/test/com/google/inject/JitBindingsTest.java
+++ b/core/test/com/google/inject/JitBindingsTest.java
@@ -19,7 +19,6 @@
import static com.google.common.collect.ImmutableSet.of;
import static com.google.inject.Asserts.assertContains;
import static com.google.inject.JitBindingsTest.GetBindingCheck.ALLOW_BINDING;
-import static com.google.inject.JitBindingsTest.GetBindingCheck.ALLOW_BINDING_PROVIDER;
import static com.google.inject.JitBindingsTest.GetBindingCheck.FAIL_ALL;
import junit.framework.TestCase;
@@ -40,6 +39,14 @@
private String jitFailed(TypeLiteral<?> clazz) {
return "Explicit bindings are required and " + clazz + " is not explicitly bound.";
}
+
+ private String jitInParentFailed(Class<?> clazz) {
+ return jitInParentFailed(TypeLiteral.get(clazz));
+ }
+
+ private String jitInParentFailed(TypeLiteral<?> clazz) {
+ return "Explicit bindings are required and " + clazz + " would be bound in a parent injector.";
+ }
private String inChildMessage(Class<?> clazz) {
return "Unable to create binding for "
@@ -161,8 +168,8 @@
});
fail();
} catch(CreationException expected) {
- assertContains(expected.getMessage(), "1) " + jitFailed(ScopedFooImpl.class));
- assertTrue(expected.getMessage(), !expected.getMessage().contains("2) "));
+ assertContains(expected.getMessage(), "\n1) " + jitFailed(ScopedFooImpl.class));
+ assertTrue(expected.getMessage(), !expected.getMessage().contains("\n2) "));
}
}
@@ -191,8 +198,8 @@
}).getInstance(Bar.class);
fail("should have failed");
} catch(ConfigurationException expected) {
- assertContains(expected.getMessage(), "1) " + jitFailed(Bar.class));
- assertTrue(expected.getMessage(), !expected.getMessage().contains("2) "));
+ assertContains(expected.getMessage(), "\n1) " + jitFailed(Bar.class));
+ assertTrue(expected.getMessage(), !expected.getMessage().contains("\n2) "));
}
}
@@ -208,8 +215,8 @@
});
fail("should have failed");
} catch (CreationException expected) {
- assertContains(expected.getMessage(), "1) " + jitFailed(Bar.class));
- assertTrue(expected.getMessage(), !expected.getMessage().contains("2) "));
+ assertContains(expected.getMessage(), "\n1) " + jitFailed(Bar.class));
+ assertTrue(expected.getMessage(), !expected.getMessage().contains("\n2) "));
}
}
@@ -223,8 +230,8 @@
}).getProvider(Bar.class);
fail("should have failed");
} catch (ConfigurationException expected) {
- assertContains(expected.getMessage(), "1) " + jitFailed(Bar.class));
- assertTrue(expected.getMessage(), !expected.getMessage().contains("2) "));
+ assertContains(expected.getMessage(), "\n1) " + jitFailed(Bar.class));
+ assertTrue(expected.getMessage(), !expected.getMessage().contains("\n2) "));
}
}
@@ -240,8 +247,8 @@
});
fail("should have failed");
} catch (CreationException expected) {
- assertContains(expected.getMessage(), "1) " + jitFailed(Bar.class));
- assertTrue(expected.getMessage(), !expected.getMessage().contains("2) "));
+ assertContains(expected.getMessage(), "\n1) " + jitFailed(Bar.class));
+ assertTrue(expected.getMessage(), !expected.getMessage().contains("\n2) "));
}
}
@@ -311,18 +318,18 @@
});
fail("should have failed");
} catch(CreationException expected) {
- assertContains(expected.getMessage(), "1) " + jitFailed(Foo.class));
- assertTrue(expected.getMessage(), !expected.getMessage().contains("2) "));
+ assertContains(expected.getMessage(), "\n1) " + jitFailed(Foo.class));
+ assertTrue(expected.getMessage(), !expected.getMessage().contains("\n2) "));
}
Injector child = parent.createChildInjector(new AbstractModule() {
@Override
protected void configure() {
bind(Foo.class).to(FooImpl.class);
+ bind(FooImpl.class);
}
});
ensureWorks(child, Foo.class, Bar.class);
- ensureFails(child, ALLOW_BINDING, FooImpl.class);
ensureInChild(parent, FooImpl.class, Foo.class);
// TODO(sameb): FooBar may or may not be in a child injector, depending on if GC has run.
// We should fix failed child injectors to remove their contents from the parent blacklist
@@ -337,9 +344,7 @@
}
});
ensureWorks(grandchild, FooBar.class, Foo.class, Bar.class);
- ensureFails(grandchild, ALLOW_BINDING, FooImpl.class);
- ensureFails(child, ALLOW_BINDING, FooImpl.class);
- ensureInChild(parent, FooImpl.class, FooBar.class, Foo.class);
+ ensureInChild(parent, FooImpl.class, FooBar.class, Foo.class);
}
public void testChildInjectorAddsOption() {
@@ -361,8 +366,8 @@
});
fail("should have failed");
} catch(CreationException expected) {
- assertContains(expected.getMessage(), "1) " + jitFailed(Foo.class));
- assertTrue(expected.getMessage(), !expected.getMessage().contains("2) "));
+ assertContains(expected.getMessage(), "\n1) " + jitFailed(Foo.class));
+ assertTrue(expected.getMessage(), !expected.getMessage().contains("\n2) "));
}
assertEquals(totalParentBindings, parent.getAllBindings().size());
@@ -371,26 +376,11 @@
protected void configure() {
binder().requireExplicitBindings();
bind(Foo.class).to(FooImpl.class);
+ bind(FooImpl.class);
}
});
- totalParentBindings++; // creating this child added FooImpl to the parent.
assertEquals(totalParentBindings, parent.getAllBindings().size());
ensureWorks(child, Foo.class, Bar.class);
- ensureFails(child, ALLOW_BINDING_PROVIDER, FooImpl.class);
- // Make extra certain that if something tries to inject a FooImpl from child
- // that it fails, even if calling getBinding().getProvider works.. because
- // the binding is built with the parent injector.
- try {
- child.injectMembers(new Object() {
- @SuppressWarnings("unused")
- @Inject
- void inject(FooImpl fooImpl) {}
- });
- fail();
- } catch(ConfigurationException expected) {
- assertContains(expected.getMessage(), "1) " + jitFailed(FooImpl.class));
- assertTrue(expected.getMessage(), !expected.getMessage().contains("2) "));
- }
Injector grandchild = child.createChildInjector(new AbstractModule() {
@Override
@@ -400,8 +390,6 @@
});
assertEquals(totalParentBindings, parent.getAllBindings().size());
ensureWorks(grandchild, FooBar.class, Foo.class, Bar.class);
- ensureFails(grandchild, ALLOW_BINDING_PROVIDER, FooImpl.class);
- ensureFails(child, ALLOW_BINDING_PROVIDER, FooImpl.class);
// Make sure siblings of children don't inherit each others settings...
// a new child should be able to get FooImpl.
@@ -426,8 +414,8 @@
});
fail("should have failed");
} catch(CreationException expected) {
- assertContains(expected.getMessage(), "1) " + jitFailed(Bar.class));
- assertTrue(expected.getMessage(), !expected.getMessage().contains("2) "));
+ assertContains(expected.getMessage(), "\n1) " + jitFailed(Bar.class));
+ assertTrue(expected.getMessage(), !expected.getMessage().contains("\n2) "));
}
Injector injector = Guice.createInjector(new AbstractModule() {
@@ -438,6 +426,7 @@
public void configure() {
bind(Foo.class).to(FooImpl.class);
expose(Foo.class);
+ bind(FooImpl.class);
}
});
}
@@ -464,8 +453,8 @@
});
fail("should have failed");
} catch(CreationException expected) {
- assertContains(expected.getMessage(), "1) " + jitFailed(Bar.class));
- assertTrue(expected.getMessage(), !expected.getMessage().contains("2) "));
+ assertContains(expected.getMessage(), "\n1) " + jitFailed(Bar.class));
+ assertTrue(expected.getMessage(), !expected.getMessage().contains("\n2) "));
}
}
@@ -523,6 +512,90 @@
String data = injector.getInstance(String.class);
assertEquals("foo", data);
}
+
+ public void testJitLinkedBindingInParentFails() {
+ try {
+ Guice.createInjector(new AbstractModule() {
+ @Override
+ protected void configure() {
+ install(new PrivateModule() {
+ @Override
+ protected void configure() {
+ binder().requireExplicitBindings();
+ bind(Foo.class).to(FooImpl.class);
+ }
+ });
+ }
+ });
+ fail("should have failed");
+ } catch (CreationException expected) {
+ assertContains(expected.getMessage(), "\n1) " + jitInParentFailed(FooImpl.class));
+ assertTrue(expected.getMessage(), !expected.getMessage().contains("\n2) "));
+ }
+ }
+
+ public void testJitProviderBindingInParentFails() {
+ try {
+ Guice.createInjector(new AbstractModule() {
+ @Override
+ protected void configure() {
+ install(new PrivateModule() {
+ @Override
+ protected void configure() {
+ binder().requireExplicitBindings();
+ bind(Foo.class).toProvider(FooProvider.class);
+ }
+ });
+ }
+ });
+ fail("should have failed");
+ } catch (CreationException expected) {
+ assertContains(expected.getMessage(), "\n1) " + jitInParentFailed(FooProvider.class));
+ assertTrue(expected.getMessage(), !expected.getMessage().contains("\n2) "));
+ }
+ }
+
+ public void testJitImplementedByBindingInParentFails() {
+ try {
+ Guice.createInjector(new AbstractModule() {
+ @Override
+ protected void configure() {
+ install(new PrivateModule() {
+ @Override
+ protected void configure() {
+ binder().requireExplicitBindings();
+ bind(ImplBy.class);
+ }
+ });
+ }
+ });
+ fail("should have failed");
+ } catch (CreationException expected) {
+ assertContains(expected.getMessage(), "\n1) " + jitInParentFailed(ImplByImpl.class));
+ assertTrue(expected.getMessage(), !expected.getMessage().contains("\n2) "));
+ }
+ }
+
+ public void testJitProvidedByBindingInParentFails() {
+ try {
+ Guice.createInjector(new AbstractModule() {
+ @Override
+ protected void configure() {
+ install(new PrivateModule() {
+ @Override
+ protected void configure() {
+ binder().requireExplicitBindings();
+ bind(ProvBy.class);
+ }
+ });
+ }
+ });
+ fail("should have failed");
+ } catch (CreationException expected) {
+ assertContains(expected.getMessage(), "\n1) " + jitInParentFailed(ProvByProvider.class));
+ assertTrue(expected.getMessage(), !expected.getMessage().contains("\n2) "));
+ }
+ }
private void ensureWorks(Injector injector, Class<?>... classes) {
for(int i = 0; i < classes.length; i++) {
@@ -539,16 +612,16 @@
injector.getInstance(classes[i]);
fail("should have failed tring to retrieve class: " + classes[i]);
} catch(ConfigurationException expected) {
- assertContains(expected.getMessage(), "1) " + jitFailed(classes[i]));
- assertTrue(expected.getMessage(), !expected.getMessage().contains("2) "));
+ assertContains(expected.getMessage(), "\n1) " + jitFailed(classes[i]));
+ assertTrue(expected.getMessage(), !expected.getMessage().contains("\n2) "));
}
try {
injector.getProvider(classes[i]);
fail("should have failed tring to retrieve class: " + classes[i]);
} catch(ConfigurationException expected) {
- assertContains(expected.getMessage(), "1) " + jitFailed(classes[i]));
- assertTrue(expected.getMessage(), !expected.getMessage().contains("2) "));
+ assertContains(expected.getMessage(), "\n1) " + jitFailed(classes[i]));
+ assertTrue(expected.getMessage(), !expected.getMessage().contains("\n2) "));
}
if (getBinding == GetBindingCheck.ALLOW_BINDING
@@ -563,16 +636,16 @@
if (getBinding == GetBindingCheck.ALLOW_BINDING_PROVIDER) {
throw expected;
}
- assertContains(expected.getMessage(), "1) " + jitFailed(classes[i]));
- assertTrue(expected.getMessage(), !expected.getMessage().contains("2) "));
+ assertContains(expected.getMessage(), "\n1) " + jitFailed(classes[i]));
+ assertTrue(expected.getMessage(), !expected.getMessage().contains("\n2) "));
}
} else {
try {
injector.getBinding(classes[i]);
fail("should have failed tring to retrieve class: " + classes[i]);
} catch(ConfigurationException expected) {
- assertContains(expected.getMessage(), "1) " + jitFailed(classes[i]));
- assertTrue(expected.getMessage(), !expected.getMessage().contains("2) "));
+ assertContains(expected.getMessage(), "\n1) " + jitFailed(classes[i]));
+ assertTrue(expected.getMessage(), !expected.getMessage().contains("\n2) "));
}
}
}
@@ -584,24 +657,24 @@
injector.getInstance(classes[i]);
fail("should have failed tring to retrieve class: " + classes[i]);
} catch(ConfigurationException expected) {
- assertContains(expected.getMessage(), "1) " + inChildMessage(classes[i]));
- assertTrue(expected.getMessage(), !expected.getMessage().contains("2) "));
+ assertContains(expected.getMessage(), "\n1) " + inChildMessage(classes[i]));
+ assertTrue(expected.getMessage(), !expected.getMessage().contains("\n2) "));
}
try {
injector.getProvider(classes[i]);
fail("should have failed tring to retrieve class: " + classes[i]);
} catch(ConfigurationException expected) {
- assertContains(expected.getMessage(), "1) " + inChildMessage(classes[i]));
- assertTrue(expected.getMessage(), !expected.getMessage().contains("2) "));
+ assertContains(expected.getMessage(), "\n1) " + inChildMessage(classes[i]));
+ assertTrue(expected.getMessage(), !expected.getMessage().contains("\n2) "));
}
try {
injector.getBinding(classes[i]);
fail("should have failed tring to retrieve class: " + classes[i]);
} catch(ConfigurationException expected) {
- assertContains(expected.getMessage(), "1) " + inChildMessage(classes[i]));
- assertTrue(expected.getMessage(), !expected.getMessage().contains("2) "));
+ assertContains(expected.getMessage(), "\n1) " + inChildMessage(classes[i]));
+ assertTrue(expected.getMessage(), !expected.getMessage().contains("\n2) "));
}
}
}