Revision: 1508
Author: sberlin
Date: Sat Feb 26 16:02:03 2011
Log: significantly improve error reporting for binding a key already bound
in a child injector or private module. include all sources in the error
msg (since it can be in many sibling private modules or child injectors),
including whether or not it as a JIT binding.
http://code.google.com/p/google-guice/source/detail?r=1508
Modified:
/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/Errors.java Mon Feb 21
13:06:40 2011
+++ /trunk/core/src/com/google/inject/internal/Errors.java Sat Feb 26
16:02:03 2011
@@ -50,6 +50,7 @@
import java.util.Comparator;
import java.util.Formatter;
import java.util.List;
+import java.util.Set;
/**
* A collection of error messages. If this type is passed as a method
parameter, the method is
@@ -298,11 +299,22 @@
return addMessage("A just-in-time binding to %s was already configured
on a parent 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 childBindingAlreadySet(Key<?> key, Set<Object> sources) {
+ Formatter allSources = new Formatter();
+ for (Object source : sources) {
+ if (source == null) {
+ allSources.format("%n (bound by a just-in-time binding)");
+ } else {
+ allSources.format("%n bound at %s", source);
+ }
+ }
+ Errors errors = addMessage(
+ "Unable to create binding for %s."
+ + " It was already configured on one or more child injectors or
private modules"
+ + "%s%n"
+ + " If it was in a PrivateModule, did you forget to expose the
binding?",
+ key, allSources.out());
+ return errors;
}
public Errors errorCheckingDuplicateBinding(Key<?> key, Object source,
Throwable t) {
=======================================
--- /trunk/core/src/com/google/inject/internal/InheritingState.java Sat Feb
19 16:05:28 2011
+++ /trunk/core/src/com/google/inject/internal/InheritingState.java Sat Feb
26 16:02:03 2011
@@ -31,6 +31,7 @@
import java.util.Collections;
import java.util.List;
import java.util.Map;
+import java.util.Set;
/**
* @author [email protected] (Jesse Wilson)
@@ -143,8 +144,8 @@
return blacklistedKeys.contains(key);
}
- public Object getSourceForBlacklistedKey(Key<?> key) {
- return blacklistedKeys.getSource(key);
+ public Set<Object> getSourcesForBlacklistedKey(Key<?> key) {
+ return blacklistedKeys.getSources(key);
}
public Object lock() {
=======================================
--- /trunk/core/src/com/google/inject/internal/InjectorImpl.java Sat Feb 19
16:05:28 2011
+++ /trunk/core/src/com/google/inject/internal/InjectorImpl.java Sat Feb 26
16:02:03 2011
@@ -763,8 +763,8 @@
}
if (state.isBlacklisted(key)) {
- Object source = state.getSourceForBlacklistedKey(key);
- throw errors.childBindingAlreadySet(key, source).toException();
+ Set<Object> sources = state.getSourcesForBlacklistedKey(key);
+ throw errors.childBindingAlreadySet(key, sources).toException();
}
BindingImpl<T> binding = createJustInTimeBinding(key, errors);
@@ -791,8 +791,8 @@
int numErrorsBefore = errors.size();
if (state.isBlacklisted(key)) {
- Object source = state.getSourceForBlacklistedKey(key);
- throw errors.childBindingAlreadySet(key, source).toException();
+ Set<Object> sources = state.getSourcesForBlacklistedKey(key);
+ throw errors.childBindingAlreadySet(key, sources).toException();
}
// Handle cases where T is a Provider<?>.
=======================================
--- /trunk/core/src/com/google/inject/internal/State.java Sat Feb 19
16:05:28 2011
+++ /trunk/core/src/com/google/inject/internal/State.java Sat Feb 26
16:02:03 2011
@@ -28,6 +28,7 @@
import java.lang.annotation.Annotation;
import java.util.List;
import java.util.Map;
+import java.util.Set;
/**
* The inheritable data within an injector. This class is intended to
allow parent and local
@@ -100,7 +101,7 @@
return true;
}
- public Object getSourceForBlacklistedKey(Key<?> key) {
+ public Set<Object> getSourcesForBlacklistedKey(Key<?> key) {
throw new UnsupportedOperationException();
}
@@ -161,7 +162,7 @@
boolean isBlacklisted(Key<?> key);
/** Returns the source of a blacklisted key. */
- Object getSourceForBlacklistedKey(Key<?> key);
+ Set<Object> getSourcesForBlacklistedKey(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 Feb 19
16:05:28 2011
+++ /trunk/core/src/com/google/inject/internal/WeakKeySet.java Sat Feb 26
16:02:03 2011
@@ -18,8 +18,11 @@
import com.google.inject.Key;
import com.google.inject.internal.util.Maps;
+import com.google.inject.internal.util.Sets;
+import com.google.inject.internal.util.SourceProvider;
import java.util.Map;
+import java.util.Set;
/**
* Minimal set that doesn't hold strong references to the contained keys.
@@ -35,13 +38,24 @@
* keys whose class names are equal but class loaders are different.
This shouldn't be an issue
* in practice.
*/
- private Map<String, Object> backingSet;
+ private Map<String, Set<Object>> backingSet;
public void add(Key<?> key, Object source) {
if (backingSet == null) {
backingSet = Maps.newHashMap();
}
- backingSet.put(key.toString(), source);
+ // if it's an instanceof Class, it was a JIT binding, which we don't
+ // want to retain.
+ if (source instanceof Class || source ==
SourceProvider.UNKNOWN_SOURCE) {
+ source = null;
+ }
+ String k = key.toString();
+ Set<Object> sources = backingSet.get(k);
+ if (sources == null) {
+ sources = Sets.newLinkedHashSet();
+ backingSet.put(k, sources);
+ }
+ sources.add(Errors.convert(source));
}
public boolean contains(Key<?> key) {
@@ -50,7 +64,7 @@
return backingSet != null && backingSet.containsKey(key.toString());
}
- public Object getSource(Key<?> key) {
+ public Set<Object> getSources(Key<?> key) {
return backingSet.get(key.toString());
}
}
=======================================
--- /trunk/core/test/com/google/inject/ParentInjectorTest.java Mon Feb 21
13:09:26 2011
+++ /trunk/core/test/com/google/inject/ParentInjectorTest.java Sat Feb 26
16:02:03 2011
@@ -17,6 +17,7 @@
package com.google.inject;
import static com.google.inject.Asserts.assertContains;
+
import com.google.inject.internal.util.ImmutableList;
import com.google.inject.internal.util.Iterables;
import com.google.inject.matcher.Matchers;
@@ -54,7 +55,11 @@
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() + " was already configured
at " + bindsA.getClass().getName());
+ "Unable to create binding for " + A.class.getName(),
+ "It was already configured on one or more child injectors or
private modules",
+ "bound at " + bindsA.getClass().getName() + ".configure(",
+ "If it was in a PrivateModule, did you forget to expose the
binding?",
+ "while locating " + A.class.getName());
}
}
=======================================
--- /trunk/core/test/com/google/inject/PrivateModuleTest.java Sat Feb 19
16:05:28 2011
+++ /trunk/core/test/com/google/inject/PrivateModuleTest.java Sat Feb 26
16:02:03 2011
@@ -29,6 +29,8 @@
import com.google.inject.spi.Dependency;
import com.google.inject.spi.ExposedBinding;
import com.google.inject.spi.PrivateElements;
+import com.google.inject.util.Types;
+
import junit.framework.TestCase;
/**
@@ -459,25 +461,101 @@
} 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?)",
+ "Unable to create binding for java.util.List.",
+ "It was already configured on one or more child injectors or
private modules",
+ "bound at " + FailingPrivateModule.class.getName()
+ ".configure(",
+ "bound at " + SecondFailingPrivateModule.class.getName()
+ ".configure(",
+ "If it was in a PrivateModule, did you forget to expose the
binding?",
"at " + FailingModule.class.getName() + ".configure(");
}
}
+
+ public void testParentBindingToPrivateLinkedJitBinding() {
+ Injector injector = Guice.createInjector(new ManyPrivateModules());
+ try {
+ injector.getBinding(Key.get(Types.providerOf(List.class)));
+ fail();
+ } catch(ConfigurationException expected) {
+ assertEquals(1, expected.getErrorMessages().size());
+ assertContains(expected.toString(),
+ "Unable to create binding for
com.google.inject.Provider<java.util.List>.",
+ "It was already configured on one or more child injectors or
private modules",
+ "bound at " + FailingPrivateModule.class.getName()
+ ".configure(",
+ "bound at " + SecondFailingPrivateModule.class.getName()
+ ".configure(",
+ "If it was in a PrivateModule, did you forget to expose the
binding?",
+ "while locating com.google.inject.Provider<java.util.List>");
+ }
+ }
+
+ public void testParentBindingToPrivateJitBinding() {
+ Injector injector = Guice.createInjector(new ManyPrivateModules());
+ try {
+ injector.getBinding(PrivateFoo.class);
+ fail();
+ } catch(ConfigurationException expected) {
+ assertEquals(1, expected.getErrorMessages().size());
+ assertContains(expected.toString(),
+ "Unable to create binding for " + PrivateFoo.class.getName(),
+ "It was already configured on one or more child injectors or
private modules",
+ "(bound by a just-in-time binding)",
+ "If it was in a PrivateModule, did you forget to expose the
binding?",
+ "while locating " + PrivateFoo.class.getName());
+ }
+ }
private static class FailingModule extends AbstractModule {
@Override
protected void configure() {
bind(Collection.class).to(List.class);
+ install(new ManyPrivateModules());
+ }
+ }
+
+ private static class ManyPrivateModules extends AbstractModule {
+ @Override
+ protected void configure() {
+ // make sure duplicate sources are collapsed
install(new FailingPrivateModule());
+ install(new FailingPrivateModule());
+ // but additional sources are listed
+ install(new SecondFailingPrivateModule());
}
}
-
+
private static class FailingPrivateModule extends PrivateModule {
@Override
protected void configure() {
bind(List.class).toInstance(new ArrayList());
+
+ // Add the Provider<List> binding, created just-in-time,
+ // to make sure our linked JIT bindings have the correct source.
+ getProvider(Key.get(Types.providerOf(List.class)));
+
+ // Request a JIT binding for PrivateFoo, which can only
+ // be created in the private module because it depends
+ // on List.
+ getProvider(PrivateFoo.class);
}
}
-}
+
+ /** A second class, so we can see another name in the source list. */
+ private static class SecondFailingPrivateModule extends PrivateModule {
+ @Override
+ protected void configure() {
+ bind(List.class).toInstance(new ArrayList());
+
+ // Add the Provider<List> binding, created just-in-time,
+ // to make sure our linked JIT bindings have the correct source.
+ getProvider(Key.get(Types.providerOf(List.class)));
+
+ // Request a JIT binding for PrivateFoo, which can only
+ // be created in the private module because it depends
+ // on List.
+ getProvider(PrivateFoo.class);
+ }
+ }
+
+ private static class PrivateFoo {
+ @Inject List list;
+ }
+}
--
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.
