partial injection a.k.a. factory generation

[GoogleCodeExporter]

(Adding issue for what we talked about already.)

Suppose you have a constructor:

@Inject
Foo(@A String a, @B String b, @C String c);

The caller wants to create a Foo and provide some of these arguments, while
the other ones get injected.  We can define a interface for this:

interface FooFactory {
  Foo make(@A String a);
}

Then Guice should be able to automatically create the factory if we ask:

bind(FooFactory.class).generates(Foo.class);

Furthermore, it's not an error if there is no binding for @A because Guice
never needs to create it.

Original issue reported on code.google.com by bslesinsky on 8 Jul 2007 at 1:29

[GoogleCodeExporter]

See also the AssistedInject thread:

http://groups.google.com/group/google-guice/browse_thread/thread/9a7e2c57583d21d
6

Original comment by bslesinsky on 15 Jul 2007 at 7:43

[GoogleCodeExporter]

Original comment by bslesinsky on 27 Oct 2007 at 7:48

• Added labels: Type-Enhancement
• Removed labels: Type-Defect

[GoogleCodeExporter]

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

Original comment by limpbizkit on 8 Jun 2008 at 11:31

• Added labels: Milestone-Release2.1

[GoogleCodeExporter]

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.

Original comment by limpbizkit on 27 Oct 2008 at 6:17

Attachments:

• injected_factory_patch.txt

[GoogleCodeExporter]

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?

Original comment by gili.tza...@gmail.com on 28 Nov 2008 at 8:17

[GoogleCodeExporter]

Original comment by limpbizkit on 28 Nov 2008 at 4:56

• Added labels: Milestone-Release2.0
• Removed labels: Milestone-Release2.1

[GoogleCodeExporter]

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.

Original comment by gili.tza...@gmail.com on 28 Nov 2008 at 6:03

[GoogleCodeExporter]

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.)

Original comment by sberlin on 1 Dec 2008 at 3:46

[GoogleCodeExporter]

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.

Original comment by gili.tza...@gmail.com on 1 Dec 2008 at 3:59

[GoogleCodeExporter]

@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'.

Original comment by limpbizkit on 1 Dec 2008 at 11:17

• Changed state: Started

[GoogleCodeExporter]

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?

Original comment by bslesinsky on 1 Dec 2008 at 5:46

[GoogleCodeExporter]

@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.

Original comment by limpbizkit on 1 Dec 2008 at 9:14

[GoogleCodeExporter]

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.

Original comment by sberlin on 1 Dec 2008 at 9:32

[GoogleCodeExporter]

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

Original comment by bslesinsky on 1 Dec 2008 at 9:39

[GoogleCodeExporter]

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.

Original comment by gili.tza...@gmail.com on 1 Dec 2008 at 10:05

[GoogleCodeExporter]

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.

Original comment by dha...@gmail.com on 1 Dec 2008 at 10:29

[GoogleCodeExporter]

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)

Original comment by limpbizkit on 2 Dec 2008 at 6:21

[GoogleCodeExporter]

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

Original comment by limpbizkit on 2 Dec 2008 at 6:48

[GoogleCodeExporter]

Jesse,

Compare Daniel's API:

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

with the new one:

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

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

Original comment by gili.tza...@gmail.com on 2 Dec 2008 at 6:57

[GoogleCodeExporter]

@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.

Original comment by limpbizkit on 2 Dec 2008 at 8:14

[GoogleCodeExporter]

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.

Original comment by sberlin on 2 Dec 2008 at 2:34

[GoogleCodeExporter]

@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.

Original comment by limpbizkit on 2 Dec 2008 at 5:29

[GoogleCodeExporter]

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?

Original comment by gili.tza...@gmail.com on 2 Dec 2008 at 5:47

[GoogleCodeExporter]

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(AbstractClas
sGenerator.java:237)
[snip]
Caused by: java.lang.reflect.InvocationTargetException
        at sun.reflect.GeneratedMethodAccessor50.invoke(Unknown Source)
        at
sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.jav
a: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(AbstractClas
sGenerator.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?

Original comment by gili.tza...@gmail.com on 2 Dec 2008 at 6:34

[GoogleCodeExporter]

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.

Original comment by limpbizkit on 9 Dec 2008 at 6:40

• Changed state: Fixed