From bslesinsky on July 15, 2007 12:43:51

See also the AssistedInject thread: http://groups.google.com/group/google-guice/browse_thread/thread/9a7e2c57583d21d6

From bslesinsky on October 27, 2007 12:48:16

(No comment was entered for this change.)

Labels: -Type-Defect Type-Enhancement

From limpbizkit on June 08, 2008 16:31:42

Deferred past 2.0. We'll stick with assistedinject for that release.

Labels: Milestone-Release2.1

From limpbizkit on October 27, 2008 11:17:17

Attached Daniel Martin's patch for AssistedInject v2. It's cool feature is support
for AOP on factory-built classes. It works using child injectors.

For it to work we need to make sure permgen space doesn't grow uncontrollably when
child injectors do AOP.

Attachment: gist
   injected_factory_patch.txt

From gili.tzabari on November 28, 2008 00:17:11

Regarding Daniel's patch The Javadoc in the patch needs to be updated. For one, the
code expects the user to invoke the FactoryModule constructor, not
FactoryModule.forInterface(). Secondly, the example code sniplet doesn't render for
me (it's completely missing). Maybe something is wrong with the <pre> block?

Jesse, do you plan on trying to commit Daniel's patch for 2.0 and partial injection
for 2.1? Or are they both for 2.1?

From limpbizkit on November 28, 2008 08:56:40

(No comment was entered for this change.)

Labels: -Milestone-Release2.1 Milestone-Release2.0

From gili.tzabari on November 28, 2008 10:03:35

I'm curious why FactoryModule.initFactoryProvider() is marked with
@SuppressWarnings("unused") because it really does seem to be unused. As a
consequence it looks like "inChildMode" is always equal to false and a whole bunch of
code is never used. This is just something to look at before the final 2.0 release.
The rest of the code seems to work wonderfully.

From sberlin on November 30, 2008 19:46:55

Is there any thought to keeping @Assisted support (for simple use-cases) and mapping
annotation-less params in the factory to the assisted parameters?  That is,

Bean {
 @Inject Bean(@Assisted String first, @Assisted String last, Service service) { ... }
}

BeanFactory {
  Bean createBean(String first, String last);
}

It could internally use a UniqueAnnotation to match up parameters that have
duplicate types.

I ask because it seems like it'd become a bit redundant in many cases to have to
specify the annotations both in the Bean & the BeanFactory whenever you want to pass
two of the same kind of parameter.  It's definitely useful -- kind of like parameter
naming in other languages -- but I find the current AssistedInject 's behaviour of
picking params in order to be spot-on most of the time.

(You can ignore this if the idea is to include Daniel's patch only as an
enhancement, not a replacement, of AssistedInject .)

From gili.tzabari on November 30, 2008 19:59:16

I tend to agree that parameter ordering alone was enough for me in the past. Then
again, in an ideal world Guice would (somehow) generate the factory for me directly
from the constructor prototype. If you had such an automatic mechanism you could
automatically add @Named annotations if they were missing by reading the parameter
name and using that as the @Named value. I don't think this is technically possible
without two compilation phases though...

Alternatively, you could provide an unsafe "Injector.getInstance(Class clazz,
Object... arguments)" method and fail at runtime. This would remove any sort of CRUD
code at the cost of type safety.

From limpbizkit on December 01, 2008 03:17:44

@sberlin, @gili, Don't worry, we'll be keeping FactoryProvider around — we've got to migrate our own code
too! But going forward we won't be using @Assisted or @ AssistedInject annotations.

--

I reviewed Daniel's FactoryModule(). It's awesome! But as I'd discussed with Daniel, it doesn't work naturally
with @Provides methods. We could allow the user to opt-out, but in general I really don't like the idea of the
user seeing a module installed and that module not being installed normally.

So I've changed his API to use builders instead. Here's what it looks like:
    bind(SummerCarFactory.class).toInstance(
        new FactoryBuilder().thatMakes(Corvette.class).build(CarFactory.class));

FactoryBuilder has the following API. All methods are optional:
  class FactoryBuilder {
    public FactoryBuilder();
    public FactoryBuilder withModule(Module module)
    public FactoryBuilder thatMakes(Class<?> producedType);
    public <F> F build(Class<F> factoryType);
  }

I'm happy with the builder but the withModule() and thatMakes() method names could use some lovin'.

Status: Started
Owner: limpbizkit

From bslesinsky on December 01, 2008 09:46:24

thatMakes() seems good.  Can you call it multiple times?  If so maybe it should be:
thatMakes(Class<?>... modules)

It's not obvious what withModule() is supposed to do, or whether you can call it
multiple times.

Where is the latest version of the code?

From limpbizkit on December 01, 2008 13:14:39

@bslesinsky - I'm still iterating on the API, so I haven't checked it in yet. When I
do, the Javadoc for 'thatMakes()' will say that it can only make a single type.

I talked to jmourits, the other author of AssistedInject and he observed that in this
model, it becomes difficult to differentiate between 'regular dependencies' and
'factory dependencies'. The old assistedinject API had clear signals (ie. @Assisted)
when factories were necessary.

To accommodate this, I intend to change the behaviour so that all factory parameters
are bound with an @Assisted binding annotation. To allow for duplicate keys, this
annotation will permit an optional name.

From sberlin on December 01, 2008 13:32:53

I wholeheartedly agree with the comment that it's hard to differentiate between
regular & factory dependencies.  It's a six-of-one/half-dozen-of-the-other thing in a
sense, because the new model technically erases the need for all the assisted
parameters to really be assisted -- you could create a bean that has two assisted
params and a factory that has one param, then while binding the factory you can bind
the other assisted param to a constant.  It opens up a lot of doors for what's
possible with assisted constructors shared between code & tests.  Still, if there was
a way to distinguish what's "supposed" to be assisted vs what's supposed to be a real
dependency -- it'd be a nice improvement.

From bslesinsky on December 01, 2008 13:39:50

What will @Assisted mean?  Where do you use it?

From gili.tzabari on December 01, 2008 14:05:38

Why it is important to annotate which parameters come from the user and which from
the Guice Module? My only beef against both FactoryModule and FactoryProvider is that
they don't totally do away with the CRUD needed to make this work. I still haven't
made up on mind on this point but I'm leaning towards "the less differentiation
between Guice-provider and User-provider parameters, the better".

The way I see it, there should be two sets of arguments at any injection point:
global (Guice module) bindings and local (user-provided) bindings. User-provided
bindings would act as overrides to the global bindings. At any given injection point,
Guice would try to provide all parameters that need to be injected. If it can't
figure out how to inject a parameter from either set then it should throw an
exception. Otherwise, it should pull values from the override set first, and global
set second.

If you generalize this enough, it should be possible to do away with the CRUD
factories altogether.

From dhanji on December 01, 2008 14:29:46

Yea keeping @Assisted sounds OK.

How about:
Factory.thatMakes(Corvette.class).using(CarFactory.class)

Or:
new AssistedFactory(CarFactory.class).thatMakes(Corvette.class)

Just throwing out ideas, the build() seems somewhat out of place in the DSL.

From limpbizkit on December 01, 2008 22:21:14

r714 contains the first draft of the new implementation. This combines Daniel's code and the old AssistedInject API. It's also been simplified since my last update - the 'withModule()' method has been removed. I figure that
when this is necessary, by-hand factories should work fine.

Javadoc: http://google-guice.googlecode.com/svn/trunk/latest- javadoc/com/google/inject/assistedinject/Factories.html#create(java.lang.Class,%20java.lang.Class)

From limpbizkit on December 01, 2008 22:48:18

This can't be closed until the Permgen issue has been addressed -- creating child injectors that use AOP
shouldn't leak Permgen space...

From gili.tzabari on December 01, 2008 22:57:31

Jesse,

Compare Daniel's API:

binder.install(new FactoryModule(PaymentFactory.class));

with the new one:

binder.bind(PaymentFactory.class).toInstance(Factories.create(PaymentFactory.class,
RealPayment.class));

Can't you avoid repetition as Daniel's API did?

From limpbizkit on December 02, 2008 00:14:15

@gili Dan's API actually looks like this in its most compact form:
  install(new FactoryModule(PaymentFactory.class).binding(Payment.class, RealPayment.class));
But in any case, an install() API is less repetitive.

The bind() API is very simple. It's explicit that one binding is added and what type the binding is for. You don't
need to read the Javadoc to figure out which bindings (or scopes, or interceptors) will be created.

I could be persuaded either way on this, but the duplication doesn't offend me. Trying to minimize characters
isn't the ultimate way to write maintainable, predictable programs.

From sberlin on December 02, 2008 06:34:16

It's good to see AssistedInjectV2 checked in, but I think the draft that's checked
in now is a significant step down from Daniel's patch.  It loses the functionality
of being able to have a factory that can return different things, support sub-
interfaces that return different things, or arbitrarily bind Assisted parameters
during factory-creation that may be required by the assisted class (but not the
factory).

OTOH, it does support all current AssistedInject usecases.

I'd like to see the Daniel's API introduced as a way of supporting the things this
draft lacks, but perhaps as an addition to this draft instead of a replacement.  It
seems it'd just need reintroduction of 'binder.install(new FactoryModule
(MyFactory.class));' as a means of binding.

While AOP is nice, I don't use it internally -- the one features that really stood
out to me from Daniel's patch was support for multiple methods returning different
types.  That is,
   FooFactory {
        SimpleFoo createSimple(@Assisted String name);
        ComplexFoo createComplex(@Assisted String name);
   }
   Although I didn't actually run his code, from scanning the patch it looked like
it would support this.  It's very useful to have a factory that can return different
(but similar types) and not have to create a factory for each method.

From limpbizkit on December 02, 2008 09:29:11

@sberlin yeah, the draft from Sunday supported all of the original use cases. But Jerome pointed out that the
additional flexibility results in more complicated API. What we have now will be easy to use.

From gili.tzabari on December 02, 2008 09:47:02

Jesse,

The error handling in the new code is a bit misleading. It complains:

"No implementation for MyFactory was bound.
  at SomeStackTrace"

But the real problem was that none of the parameters in the constructedType
constructor were annotated with @Assisted. Can you please update the error message to
reflect this possibility?

From gili.tzabari on December 02, 2008 10:34:43

Another error:

Error injecting method,
com.google.inject.internal.cglib.core.CodeGenerationException:
java.lang.reflect.InvocationTargetException-->null
  at com.google.inject.assistedinject.Factories$Base.initialize(Factories.java:190)
[snip]
Caused by: com.google.inject.internal.cglib.core.CodeGenerationException:
java.lang.reflect.InvocationTargetException-->null
        at
com.google.inject.internal.cglib.core.AbstractClassGenerator.create(AbstractClassGenerator.java:237)
[snip]
Caused by: java.lang.reflect.InvocationTargetException
        at sun.reflect.GeneratedMethodAccessor50.invoke(Unknown Source)
        at
sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:25)
        at java.lang.reflect.Method.invoke(Method.java:597)
        at
com.google.inject.internal.cglib.core.ReflectUtils.defineClass(ReflectUtils.java:384)
        at
com.google.inject.internal.cglib.core.AbstractClassGenerator.create(AbstractClassGenerator.java:219)
Caused by: java.lang.NoClassDefFoundError:
com/google/inject/internal/cglib/reflect/FastClass

I tried adding this line to common.xml but it did not help:

<keep pattern="net.sf.cglib.reflect.FastClass"/>

Any ideas?

From limpbizkit on December 08, 2008 22:40:21

We've come full circle. The latest r723 implements Dan Martin's new approach using the old API.
FactoryProvider.newFactory() can be used to do new-style assisted injection. It switches depending on whether
the constructor has @ AssistedInject or not. r720 addresses the permgen problems.

I'm marking this as closed. If there's backlash about the API unification, that can be considered a separate issue.

Status: Fixed