Revision: 1189
Author: [email protected]
Date: Sat Jul 17 09:39:00 2010
Log: remove confusing behavior from assistedinject -- it used to allow
factory creations to be resolved by the injector if the return key matched
something in the injector. now it always creates new objects. the prior
behavior created a scenario where, if the following pseudo-code was used...
public class Bar { ... }
interface BarFactory { Bar createBar(...); }
...
protected void configure() {
// allow custom Bars to be created if using BarFactory
install(new FactoryModuleBuilder().build(BarFactory.class));
}
@Provides Bar defaultBar() { // allow a default value
return new Bar(...);
}
...Someone wants to allow default Bars to be injected AND create custom
ones if using BarFactory. But, AssistedInject didn't actually do that.
When someone called BarFactory.create(...), it returned the bar created
from the @Provides method, because AssistedInject saw the main injector had
a binding for Bar.
This removes that behavior, so AssistedInject will always create a new Bar
when its factory method is called.
http://code.google.com/p/google-guice/source/detail?r=1189
Modified:
/trunk/extensions/assistedinject/src/com/google/inject/assistedinject/FactoryModuleBuilder.java
/trunk/extensions/assistedinject/src/com/google/inject/assistedinject/FactoryProvider2.java
/trunk/extensions/assistedinject/test/com/google/inject/assistedinject/FactoryModuleBuilderTest.java
=======================================
---
/trunk/extensions/assistedinject/src/com/google/inject/assistedinject/FactoryModuleBuilder.java
Sun May 23 06:40:06 2010
+++
/trunk/extensions/assistedinject/src/com/google/inject/assistedinject/FactoryModuleBuilder.java
Sat Jul 17 09:39:00 2010
@@ -157,8 +157,7 @@
* install(new FactoryModuleBuilder().build(FruitFactory.class));
* }</pre>
*
- * Note that any type returned by the factory in this manner needs to be
an implementation class
- * unless the return types can be resolved using your regular injector
configuration.
+ * Note that any type returned by the factory in this manner needs to be
an implementation class.
* <p/>
* To return two different implementations for the same interface from
your factory, use binding
* annotations on your return types:
@@ -174,24 +173,6 @@
* .implement(Car.class, Names.named("clean"), Prius.class)
* .build(CarFactory.class));
* }</pre>
- * <p/>
- * All return types in your factory are further resolved using your
regular injector configuration.
- * This means that in the following example you'll get a {...@code Rooster}
instead of a generic
- * chicken:
- *
- * <pre>interface Animal {}
- * public class Chicken implements Animal {}
- * public class Rooster extends Chicken {}
- * interface AnimalFactory {
- * Animal getAnimal();
- * }
- * ...
- * protected void configure() {
- * bind(Chicken.class).to(Rooster.class);
- * install(new FactoryModuleBuilder()
- * .implement(Animal.class, Chicken.class)
- * .build(AnimalFactory.class));
- * }</pre>
*
* @author [email protected] (Peter Schmitt)
*/
=======================================
---
/trunk/extensions/assistedinject/src/com/google/inject/assistedinject/FactoryProvider2.java
Sat Jul 3 08:51:31 2010
+++
/trunk/extensions/assistedinject/src/com/google/inject/assistedinject/FactoryProvider2.java
Sat Jul 17 09:39:00 2010
@@ -31,6 +31,7 @@
import com.google.inject.internal.BytecodeGen;
import com.google.inject.internal.Errors;
import com.google.inject.internal.ErrorsException;
+import com.google.inject.internal.util.Classes;
import com.google.inject.internal.util.ImmutableList;
import com.google.inject.internal.util.ImmutableMap;
import com.google.inject.internal.util.Iterables;
@@ -49,6 +50,7 @@
import java.lang.reflect.Constructor;
import java.lang.reflect.InvocationHandler;
import java.lang.reflect.Method;
+import java.lang.reflect.Modifier;
import java.lang.reflect.Proxy;
import java.util.Arrays;
import java.util.Collection;
@@ -186,20 +188,16 @@
if(implementation == null) {
implementation = returnType.getTypeLiteral();
}
- InjectionPoint ctorInjectionPoint =
findMatchingConstructorInjectionPoint(method, implementation,
immutableParamList, errors);
- Constructor<?> constructor = null;
- if(ctorInjectionPoint != null) {
- // There are three reasons this could be null:
- // 1) The implementation is an interface and is forwarded
(explicitly or implicitly)
- // to another binding (see
FactoryModuleBuildTest.test[Implicit|Explicit]ForwardingAssistedBinding.
- // In this case, things are OK and the exception is right to
be ignored.
- // 2) The implementation has something wrong with its
constructors (two @injects, invalid ctor, etc..)
- // In this case, by having a null constructor we let the
proper exception be recreated later on.
- // 3) findMatchingConstructor added errors to the Errors object
and returned null, in which case
- // this method will appropriately throw an exception later on.
- constructor = (Constructor)ctorInjectionPoint.getMember();
+ InjectionPoint ctorInjectionPoint;
+ try {
+ ctorInjectionPoint =
+ findMatchingConstructorInjectionPoint(method, returnType,
implementation, immutableParamList);
+ } catch(ErrorsException ee) {
+ errors.merge(ee.getErrors());
+ continue;
}
+ Constructor<?> constructor =
(Constructor)ctorInjectionPoint.getMember();
List<ThreadLocalProvider> providers = Collections.emptyList();
boolean optimized = false;
// Now go through all dependencies of the implementation and see
if it is OK to
@@ -258,12 +256,36 @@
* {...@link AssistedInject} constructors exist, this will default to
looking for an
* {...@literal @}...@link Inject} constructor.
*/
- private InjectionPoint findMatchingConstructorInjectionPoint(Method
method,
- TypeLiteral<?> implementation, List<Key<?>> paramList, Errors
errors) throws ErrorsException {
+ private InjectionPoint findMatchingConstructorInjectionPoint(
+ Method method, Key<?> returnType, TypeLiteral<?> implementation,
List<Key<?>> paramList)
+ throws ErrorsException {
+ Errors errors = new Errors(method);
+ if(returnType.getTypeLiteral().equals(implementation)) {
+ errors = errors.withSource(implementation);
+ } else {
+ errors = errors.withSource(returnType).withSource(implementation);
+ }
+
+ Class<?> rawType = implementation.getRawType();
+ if (Modifier.isInterface(rawType.getModifiers())) {
+ errors.addMessage(
+ "%s is an interface, not a concrete class. Unable to create
AssistedInject factory.",
+ implementation);
+ throw errors.toException();
+ } else if (Modifier.isAbstract(rawType.getModifiers())) {
+ errors.addMessage(
+ "%s is abstract, not a concrete class. Unable to create
AssistedInject factory.",
+ implementation);
+ throw errors.toException();
+ } else if (Classes.isInnerClass(rawType)) {
+ errors.cannotInjectInnerClass(rawType);
+ throw errors.toException();
+ }
+
Constructor<?> matchingConstructor = null;
boolean anyAssistedInjectConstructors = false;
// Look for AssistedInject constructors...
- for (Constructor<?> constructor :
implementation.getRawType().getDeclaredConstructors()) {
+ for (Constructor<?> constructor : rawType.getDeclaredConstructors()) {
if (constructor.isAnnotationPresent(AssistedInject.class)) {
anyAssistedInjectConstructors = true;
if (constructorHasMatchingParams(implementation, constructor,
paramList, errors)) {
@@ -273,7 +295,7 @@
"%s has more than one constructor annotated with
@AssistedInject"
+ " that matches the parameters in method %s.
Unable to create AssistedInject factory.",
implementation, method);
- return null;
+ throw errors.toException();
} else {
matchingConstructor = constructor;
}
@@ -285,9 +307,9 @@
// If none existed, use @Inject.
try {
return InjectionPoint.forConstructorOf(implementation);
- } catch(ConfigurationException ignored) {
- // This will be handled later more appropriately (with a good
message).
- return null;
+ } catch(ConfigurationException e) {
+ errors.merge(e.getErrorMessages());
+ throw errors.toException();
}
} else {
// Otherwise, use it or fail with a good error message.
@@ -302,7 +324,7 @@
"%s has @AssistedInject constructors, but none of them match
the"
+ " parameters in method %s. Unable to create AssistedInject
factory.",
implementation, method);
- return null;
+ throw errors.toException();
}
}
}
@@ -460,30 +482,17 @@
binder.bind((Key)
paramKey).toProvider(data.providers.get(p++));
}
}
-
-
- Constructor<?> constructor = data.constructor;
- // If the injector already has a binding for the return type, don't
- // bother binding to a specific constructor. Otherwise, there
could be
- // bugs where an implicit binding isn't used (or an explicitly
forwarded
- // binding isn't used)
- if (injector.getExistingBinding(returnType) != null) {
- constructor = null;
- }
-
- TypeLiteral<?> implementation =
collector.getBindings().get(returnType);
- if (implementation != null) {
- if(constructor == null) {
-
binder.bind(assistedReturnType).to((TypeLiteral)implementation);
+
+ Constructor constructor = data.constructor;
+ // Constructor *should* always be non-null here,
+ // but if it isn't, we'll end up throwing a fairly good error
+ // message for the user.
+ if(constructor != null) {
+ TypeLiteral implementation =
collector.getBindings().get(returnType);
+ if (implementation != null) {
+ binder.bind(assistedReturnType).toConstructor(constructor,
implementation);
} else {
-
binder.bind(assistedReturnType).toConstructor((Constructor)constructor,
(TypeLiteral)implementation);
- }
- } else {
- // no implementation, but need to bind from assisted key to
actual key.
- if(constructor == null) {
- binder.bind(assistedReturnType).to((Key)returnType);
- } else {
-
binder.bind(assistedReturnType).toConstructor((Constructor)constructor,
(TypeLiteral)returnType.getTypeLiteral());
+ binder.bind(assistedReturnType).toConstructor(constructor,
(TypeLiteral) returnType.getTypeLiteral());
}
}
}
=======================================
---
/trunk/extensions/assistedinject/test/com/google/inject/assistedinject/FactoryModuleBuilderTest.java
Sat Jul 3 08:51:31 2010
+++
/trunk/extensions/assistedinject/test/com/google/inject/assistedinject/FactoryModuleBuilderTest.java
Sat Jul 17 09:39:00 2010
@@ -16,13 +16,15 @@
package com.google.inject.assistedinject;
-import com.google.inject.AbstractModule;
-
+import static com.google.inject.Asserts.assertContains;
+
+import com.google.inject.AbstractModule;
import com.google.inject.CreationException;
import com.google.inject.Guice;
import com.google.inject.Inject;
import com.google.inject.Injector;
import com.google.inject.Key;
+import com.google.inject.Provides;
import com.google.inject.TypeLiteral;
import com.google.inject.internal.util.Iterables;
import com.google.inject.name.Named;
@@ -36,30 +38,123 @@
public class FactoryModuleBuilderTest extends TestCase {
- public void testImplicitForwardingAssistedBinding() {
+ public void testImplicitForwardingAssistedBindingFailsWithInterface() {
+ try {
+ Guice.createInjector(new AbstractModule() {
+ @Override
+ protected void configure() {
+ bind(Car.class).to(Golf.class);
+ install(new
FactoryModuleBuilder().build(ColoredCarFactory.class));
+ }
+ });
+ fail();
+ } catch (CreationException ce) {
+ assertContains(
+ ce.getMessage(), "1) " + Car.class.getName() + " is an
interface, not a concrete class.",
+ "Unable to create AssistedInject factory.",
+ "while locating " + Car.class.getName(),
+ "at " + ColoredCarFactory.class.getName() + ".create(");
+ assertEquals(1, ce.getErrorMessages().size());
+ }
+ }
+
+ public void
testImplicitForwardingAssistedBindingFailsWithAbstractClass() {
+ try {
+ Guice.createInjector(new AbstractModule() {
+ @Override
+ protected void configure() {
+ bind(AbstractCar.class).to(ArtCar.class);
+ install(new
FactoryModuleBuilder().build(ColoredAbstractCarFactory.class));
+ }
+ });
+ fail();
+ } catch (CreationException ce) {
+ assertContains(
+ ce.getMessage(), "1) " + AbstractCar.class.getName() + " is
abstract, not a concrete class.",
+ "Unable to create AssistedInject factory.",
+ "while locating " + AbstractCar.class.getName(),
+ "at " + ColoredAbstractCarFactory.class.getName() + ".create(");
+ assertEquals(1, ce.getErrorMessages().size());
+ }
+ }
+
+ public void testImplicitForwardingAssistedBindingCreatesNewObjects() {
+ final Mustang providedMustang = new Mustang(Color.BLUE);
Injector injector = Guice.createInjector(new AbstractModule() {
@Override protected void configure() {
- bind(Car.class).to(Golf.class);
- install(new FactoryModuleBuilder().build(ColoredCarFactory.class));
- }
+ install(new FactoryModuleBuilder().build(MustangFactory.class));
+ }
+ @Provides Mustang provide() { return providedMustang; }
});
-
- ColoredCarFactory factory =
injector.getInstance(ColoredCarFactory.class);
- assertTrue(factory.create(Color.BLUE) instanceof Golf);
+ assertSame(providedMustang, injector.getInstance(Mustang.class));
+ MustangFactory factory = injector.getInstance(MustangFactory.class);
+ Mustang created = factory.create(Color.GREEN);
+ assertNotSame(providedMustang, created);
+ assertEquals(Color.BLUE, providedMustang.color);
+ assertEquals(Color.GREEN, created.color);
+ }
+
+ public void testExplicitForwardingAssistedBindingFailsWithInterface() {
+ try {
+ Guice.createInjector(new AbstractModule() {
+ @Override
+ protected void configure() {
+ bind(Volkswagen.class).to(Golf.class);
+ install(new FactoryModuleBuilder()
+ .implement(Car.class, Volkswagen.class)
+ .build(ColoredCarFactory.class));
+ }
+ });
+ fail();
+ } catch (CreationException ce) {
+ assertContains(
+ ce.getMessage(), "1) " + Volkswagen.class.getName() + " is an
interface, not a concrete class.",
+ "Unable to create AssistedInject factory.",
+ "while locating " + Volkswagen.class.getName(),
+ "while locating " + Car.class.getName(),
+ "at " + ColoredCarFactory.class.getName() + ".create(");
+ assertEquals(1, ce.getErrorMessages().size());
+ }
}
- public void testExplicitForwardingAssistedBinding() {
+ public void
testExplicitForwardingAssistedBindingFailsWithAbstractClass() {
+ try {
+ Guice.createInjector(new AbstractModule() {
+ @Override
+ protected void configure() {
+ bind(AbstractCar.class).to(ArtCar.class);
+ install(new FactoryModuleBuilder()
+ .implement(Car.class, AbstractCar.class)
+ .build(ColoredCarFactory.class));
+ }
+ });
+ fail();
+ } catch (CreationException ce) {
+ assertContains(
+ ce.getMessage(), "1) " + AbstractCar.class.getName() + " is
abstract, not a concrete class.",
+ "Unable to create AssistedInject factory.",
+ "while locating " + AbstractCar.class.getName(),
+ "while locating " + Car.class.getName(),
+ "at " + ColoredCarFactory.class.getName() + ".create(");
+ assertEquals(1, ce.getErrorMessages().size());
+ }
+ }
+
+ public void testExplicitForwardingAssistedBindingCreatesNewObjects() {
+ final Mustang providedMustang = new Mustang(Color.BLUE);
Injector injector = Guice.createInjector(new AbstractModule() {
@Override protected void configure() {
- bind(Volkswagen.class).to(Golf.class);
- install(new FactoryModuleBuilder()
- .implement(Car.class, Volkswagen.class)
- .build(ColoredCarFactory.class));
- }
+ install(new FactoryModuleBuilder().implement(Car.class,
Mustang.class).build(
+ ColoredCarFactory.class));
+ }
+ @Provides Mustang provide() { return providedMustang; }
});
-
+ assertSame(providedMustang, injector.getInstance(Mustang.class));
ColoredCarFactory factory =
injector.getInstance(ColoredCarFactory.class);
- assertTrue(factory.create(Color.BLUE) instanceof Golf);
+ Mustang created = (Mustang)factory.create(Color.GREEN);
+ assertNotSame(providedMustang, created);
+ assertEquals(Color.BLUE, providedMustang.color);
+ assertEquals(Color.GREEN, created.color);
}
public void testAnnotatedAndParentBoundReturnValue() {
@@ -241,4 +336,11 @@
public static class Bar {}
public static class Baz<E> {}
-}
+ abstract static class AbstractCar implements Car {}
+ interface ColoredAbstractCarFactory {
+ AbstractCar create(Color color);
+ }
+ public static class ArtCar extends AbstractCar {}
+
+
+}
--
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.
