Revision: 1498
Author: sberlin
Date: Sat Feb 19 16:05:28 2011
Log: issue 345 - better error message for private modules. show the source
of a child binding instead of just saying its in a child injector. also
give a hint that you may have wanted to expose the binding.
http://code.google.com/p/google-guice/source/detail?r=1498
Modified:
/trunk/core/src/com/google/inject/internal/BindingProcessor.java
/trunk/core/src/com/google/inject/internal/Errors.java
/trunk/core/src/com/google/inject/internal/InheritingState.java
/trunk/core/src/com/google/inject/internal/InjectorImpl.java
/trunk/core/src/com/google/inject/internal/State.java
/trunk/core/src/com/google/inject/internal/WeakKeySet.java
/trunk/core/test/com/google/inject/ParentInjectorTest.java
/trunk/core/test/com/google/inject/PrivateModuleTest.java
=======================================
--- /trunk/core/src/com/google/inject/internal/BindingProcessor.java Tue
Dec 14 21:38:37 2010
+++ /trunk/core/src/com/google/inject/internal/BindingProcessor.java Sat
Feb 19 16:05:28 2011
@@ -264,7 +264,7 @@
}
// prevent the parent from creating a JIT binding for this key
- injector.state.parent().blacklist(key);
+ injector.state.parent().blacklist(key, binding.getSource());
injector.state.putBinding(key, binding);
}
=======================================
--- /trunk/core/src/com/google/inject/internal/Errors.java Thu Nov 18
18:33:32 2010
+++ /trunk/core/src/com/google/inject/internal/Errors.java Sat Feb 19
16:05:28 2011
@@ -294,15 +294,18 @@
return addMessage("A binding to %s was already configured at %s.",
key, convert(source));
}
- public Errors childBindingAlreadySet(Key<?> key) {
- return addMessage("A binding to %s already exists on a child
injector.", key);
+ public Errors childBindingAlreadySet(Key<?> key, Object source) {
+ return addMessage(
+ "A binding to %s was already configured at %s.%n"
+ + " (If it was in a PrivateModule, did you forget to expose the
binding?)",
+ key, convert(source));
}
public Errors errorCheckingDuplicateBinding(Key<?> key, Object source,
Throwable t) {
return addMessage(
"A binding to %s was already configured at %s and an error was
thrown "
+ "while checking duplicate bindings. Error: %s",
- key, source, t);
+ key, convert(source), t);
}
public Errors errorInjectingMethod(Throwable cause) {
=======================================
--- /trunk/core/src/com/google/inject/internal/InheritingState.java Thu Nov
18 18:39:46 2010
+++ /trunk/core/src/com/google/inject/internal/InheritingState.java Sat Feb
19 16:05:28 2011
@@ -134,14 +134,18 @@
return result;
}
- public void blacklist(Key<?> key) {
- parent.blacklist(key);
- blacklistedKeys.add(key);
+ public void blacklist(Key<?> key, Object source) {
+ parent.blacklist(key, source);
+ blacklistedKeys.add(key, source);
}
public boolean isBlacklisted(Key<?> key) {
return blacklistedKeys.contains(key);
}
+
+ public Object getSourceForBlacklistedKey(Key<?> key) {
+ return blacklistedKeys.getSource(key);
+ }
public Object lock() {
return lock;
=======================================
--- /trunk/core/src/com/google/inject/internal/InjectorImpl.java Sun Dec 12
18:44:36 2010
+++ /trunk/core/src/com/google/inject/internal/InjectorImpl.java Sat Feb 19
16:05:28 2011
@@ -763,11 +763,12 @@
}
if (state.isBlacklisted(key)) {
- throw errors.childBindingAlreadySet(key).toException();
+ Object source = state.getSourceForBlacklistedKey(key);
+ throw errors.childBindingAlreadySet(key, source).toException();
}
BindingImpl<T> binding = createJustInTimeBinding(key, errors);
- state.parent().blacklist(key);
+ state.parent().blacklist(key, binding.getSource());
jitBindings.put(key, binding);
return binding;
}
@@ -790,7 +791,8 @@
int numErrorsBefore = errors.size();
if (state.isBlacklisted(key)) {
- throw errors.childBindingAlreadySet(key).toException();
+ Object source = state.getSourceForBlacklistedKey(key);
+ throw errors.childBindingAlreadySet(key, source).toException();
}
// Handle cases where T is a Provider<?>.
=======================================
--- /trunk/core/src/com/google/inject/internal/State.java Thu Nov 18
18:39:46 2010
+++ /trunk/core/src/com/google/inject/internal/State.java Sat Feb 19
16:05:28 2011
@@ -93,12 +93,16 @@
return ImmutableList.of();
}
- public void blacklist(Key<?> key) {
+ public void blacklist(Key<?> key, Object source) {
}
public boolean isBlacklisted(Key<?> key) {
return true;
}
+
+ public Object getSourceForBlacklistedKey(Key<?> key) {
+ throw new UnsupportedOperationException();
+ }
public Object lock() {
throw new UnsupportedOperationException();
@@ -148,13 +152,16 @@
* blacklist their bound keys on their parent injectors to prevent
just-in-time bindings on the
* parent injector that would conflict.
*/
- void blacklist(Key<?> key);
+ void blacklist(Key<?> key, Object source);
/**
* Returns true if {@code key} is forbidden from being bound in this
injector. This indicates that
* one of this injector's descendent's has bound the key.
*/
boolean isBlacklisted(Key<?> key);
+
+ /** Returns the source of a blacklisted key. */
+ Object getSourceForBlacklistedKey(Key<?> key);
/**
* Returns the shared lock for all injector data. This is a
low-granularity, high-contention lock
=======================================
--- /trunk/core/src/com/google/inject/internal/WeakKeySet.java Sat Jul 3
08:51:31 2010
+++ /trunk/core/src/com/google/inject/internal/WeakKeySet.java Sat Feb 19
16:05:28 2011
@@ -17,8 +17,9 @@
package com.google.inject.internal;
import com.google.inject.Key;
-import com.google.inject.internal.util.Sets;
-import java.util.Set;
+import com.google.inject.internal.util.Maps;
+
+import java.util.Map;
/**
* Minimal set that doesn't hold strong references to the contained keys.
@@ -34,18 +35,22 @@
* keys whose class names are equal but class loaders are different.
This shouldn't be an issue
* in practice.
*/
- private Set<String> backingSet;
-
- public boolean add(Key<?> key) {
+ private Map<String, Object> backingSet;
+
+ public void add(Key<?> key, Object source) {
if (backingSet == null) {
- backingSet = Sets.newHashSet();
- }
- return backingSet.add(key.toString());
+ backingSet = Maps.newHashMap();
+ }
+ backingSet.put(key.toString(), source);
}
- public boolean contains(Object o) {
+ public boolean contains(Key<?> key) {
// avoid calling key.toString() if the backing set is empty. toString
is expensive in aggregate,
// and most WeakKeySets are empty in practice (because they're used by
top-level injectors)
- return backingSet != null && o instanceof Key &&
backingSet.contains(o.toString());
+ return backingSet != null && backingSet.containsKey(key.toString());
+ }
+
+ public Object getSource(Key<?> key) {
+ return backingSet.get(key.toString());
}
}
=======================================
--- /trunk/core/test/com/google/inject/ParentInjectorTest.java Sat Jul 3
08:51:31 2010
+++ /trunk/core/test/com/google/inject/ParentInjectorTest.java Sat Feb 19
16:05:28 2011
@@ -52,8 +52,8 @@
parent.getInstance(A.class);
fail("Created a just-in-time binding on the parent that's the same
as a child's binding");
} catch (ConfigurationException e) {
- assertContains(e.getMessage(), "A binding to ", A.class.getName(),
- " already exists on a child injector.");
+ assertContains(e.getMessage(),
+ "A binding to " + A.class.getName() + " was already configured
at " + bindsA.getClass().getName());
}
}
=======================================
--- /trunk/core/test/com/google/inject/PrivateModuleTest.java Thu Aug 26
12:05:46 2010
+++ /trunk/core/test/com/google/inject/PrivateModuleTest.java Sat Feb 19
16:05:28 2011
@@ -17,6 +17,11 @@
package com.google.inject;
import static com.google.inject.Asserts.assertContains;
+
+import java.util.ArrayList;
+import java.util.Collection;
+import java.util.List;
+
import com.google.inject.internal.util.ImmutableSet;
import com.google.inject.name.Named;
import com.google.inject.name.Names;
@@ -446,4 +451,33 @@
Injector privateInjector = privateElements.getInjector();
assertEquals("private",
privateInjector.getInstance(Key.get(String.class, Names.named("a"))));
}
-}
+
+ public void testParentBindsSomethingInPrivate() {
+ try {
+ Guice.createInjector(new FailingModule());
+ fail();
+ } catch(CreationException expected) {
+ assertEquals(1, expected.getErrorMessages().size());
+ assertContains(expected.toString(),
+ "A binding to java.util.List was already configured at",
+ FailingPrivateModule.class.getName() + ".configure(",
+ "(If it was in a PrivateModule, did you forget to expose the
binding?)",
+ "at " + FailingModule.class.getName() + ".configure(");
+ }
+ }
+
+ private static class FailingModule extends AbstractModule {
+ @Override
+ protected void configure() {
+ bind(Collection.class).to(List.class);
+ install(new FailingPrivateModule());
+ }
+ }
+
+ private static class FailingPrivateModule extends PrivateModule {
+ @Override
+ protected void configure() {
+ bind(List.class).toInstance(new ArrayList());
+ }
+ }
+}
--
You received this message because you are subscribed to the Google Groups
"google-guice-dev" group.
To post to this group, send email to [email protected].
To unsubscribe from this group, send email to
[email protected].
For more options, visit this group at
http://groups.google.com/group/google-guice-dev?hl=en.
