Make it clear how to do an optional Injector.getInstance

[GoogleCodeExporter]

In a Google project, I wanted to inject an instance optionally but could not 
use @Inject(optional = true) because of a binding annotation with a 
non-constant member, so I looked to Injector.getInstance.  The proper way to do 
an optional Injector.getInstance is not obvious from the documentation.  If 
catching the ConfigurationException (as Guice does internally at 
http://code.google.com/p/google-guice/source/browse/trunk/core/src/com/google/in
ject/spi/InjectionPoint.java#735) is the recommended way, that should be 
documented.

Original issue reported on code.google.com by mattmccu...@google.com on 6 Oct 2011 at 10:00

[GoogleCodeExporter]

It's not obvious basically because using getInstance is a "last resort" kind of 
thing, and doesn't expose optional-ity.  Your best workaround is to use 
Injector.getExistingBinding, and if it returns null, it wasn't bound.  (Then 
call getProvider().get().)  Binding annotations with non-constant members sound 
incredibly scary though!

Original comment by sberlin on 6 Oct 2011 at 10:05

• Changed state: WontFix

[GoogleCodeExporter]

> Your best workaround is to use Injector.getExistingBinding, and if it returns 
null, it wasn't bound.

Is there a reason you suggest this for clients, while Guice does something 
different internally (catch the ConfigurationException)?

Original comment by mattmccu...@google.com on 6 Oct 2011 at 10:20

[GoogleCodeExporter]

Two reasons
 1) The code in question in Guice was written before getExistingBinding was added
 2) The better reason: It's an entirely different purpose.  Raw InjectionPoints have nothing to do with Injectors or Bindings or anything like that.  The code is looking reflectively at a class for all points with an @Inject annotation.  As part of doing that, it validates all the annotations on the class (see InjectionPoint.forMember, line 106.)  It adds an error (which is converted to a ConfigurationException) if there are two "binding annotations".  If the injection point is optional, the exception is ignored & the injection point is just dropped from the list of potential injection points.   To be honest, it probably shouldn't be dropped & should instead error, since it's a real error and will _never_ get an injection...    but, that's why it does it.

Original comment by sberlin on 6 Oct 2011 at 10:51

[GoogleCodeExporter]

> 2) The better reason: It's an entirely different purpose.

Oops, I guess I wasn't looking closely at what the InjectionPoint code actually 
did...

Original comment by mattmccu...@google.com on 6 Oct 2011 at 10:59

[GoogleCodeExporter]

For the public record, I showed Sam the use case for the non-constant binding 
annotation and he pointed me to use MapBinder instead.

Original comment by mattmccu...@google.com on 6 Oct 2011 at 11:09

[GoogleCodeExporter]

> It's not obvious basically because using getInstance is a "last resort" kind 
of thing, and doesn't expose optional-ity.

With all due respect, I find it hard to believe that all of the references to 
Injector#getInstance in the Google codebase (I won't state the number here, but 
I'm confident you can search and get it for yourself) are cleanly avoidable.  I 
see a number that look like framework code, and various other cases.

IMO, as long as Injector#getInstance exists, it should have feature parity with 
@Inject.  If I prepared a patch that adds some methods for optional retrieval 
of instances and providers, or even just updates the javadoc with your 
suggestion, would you accept it?

Original comment by mattmccu...@google.com on 7 Oct 2011 at 4:51

[GoogleCodeExporter]

+1 for expanding the javadoc, -1 for adding additional variants of the 
getInstance method

IMHO anything that could be achieved without changing the core should really 
belong in an extension or utility library. In this case checking the binding 
exists before calling its provider (and returning null or a default value if it 
doesn't exist) could easily be performed by a simple static utility method.

I also think getInstance should be used as sparingly as possible, since you're 
adding a direct dependency to the Guice API rather than rely on the standard 
annotations (which provide looser coupling). Ideally the only places you should 
need to call getInstance are when bootstrapping the application or when calling 
into the injected graph from an external context.

Most of the issues I've dealt with recently wrt. application portability have 
been down to uses of getInstance.

Original comment by mccu...@gmail.com on 7 Oct 2011 at 10:35

[GoogleCodeExporter]

Sometimes the last resort is the only resort, and this is pretty much the case 
with framework code that wants to do some funkier things.  Still, I would pin 
the # of getInstance calls more on folks just trying to complete their task in 
any way they know how, as opposed to doing the "right" thing.  getInstance is, 
at it's best, the last resort to do what you need, but 99% of the time, it's 
the wrong thing and indicative of some poor design.

I'm with mcculls on the point that the API shouldn't expose every little 
feature.  That's one of the main things of the Guice API -- expose only what's 
absolutely necessary in the core API, and allow extensions to do the rest.  
This was one of Bob's key insights in creating it to begin with.  The more you 
offer, the harder it becomes to figure out how to use it properly, and the 
easier it becomes to hang yourself.

Original comment by sberlin on 7 Oct 2011 at 1:19

[GoogleCodeExporter]

> IMHO anything that could be achieved without changing the core should really 
belong in an extension or utility library. 

Fair enough.

> In this case checking the binding exists before calling its provider (and 
returning null or a default value if it doesn't exist) could easily be 
performed by a simple static utility method.

Yes, that's what I ended up writing (for the Google-internal code that may 
later be converted to MapBinder so it doesn't need Injector#getInstance).  Is 
there a catch-all utility extension where such a method could be placed, or do 
you prefer to force every client to duplicate it as a way of harassing them for 
using Injector#getInstance?  :P

I would then like to patch the Injector#getInstance javadoc to either refer to 
the extension method or, if the extension method is not to be, describe the 
technique right there.

Original comment by mattmccu...@google.com on 7 Oct 2011 at 3:03

[GoogleCodeExporter]

Sam, if you would be willing to accept either the javadoc addition or an 
extension module, could you please reopen this issue?  I cannot.

Remaining remarks:

> I also think getInstance should be used as sparingly as possible, since 
you're adding a direct dependency to the Guice API rather than rely on the 
standard annotations (which provide looser coupling)

Let's break this argument into two pieces:

- "standard": That might hold if I used @javax.inject.Inject, but 
@com.google.inject.Inject seems to be preferred at Google.

- "looser coupling": It's worth noting that this is not unambiguously a good 
thing, but rather the same trade-off as using Guice in general: looser coupling 
via behind-the-scenes action for weaker static verification and poorer code 
understandability (at least until the IDEs catch up).

> getInstance is, at it's best, the last resort to do what you need, but 99% of 
the time, it's the wrong thing and indicative of some poor design.

I'll buy that.  My concern is for the 1%, and for cases like mine where the 
better design has been identified but I don't want to block my current work on 
that cleanup.

Original comment by mattmccu...@google.com on 7 Oct 2011 at 10:38

[GoogleCodeExporter]

I think you just want to catch the exception. Its like four lines of code and 
it'll potentially run much faster than getExistingBindings, which needs O(N) 
time+space and will be flaky if you need a JIt binding in your result.

  try {
    return injector.getInstance(key);
  } catch(ConfigurationException ignored) {}

Original comment by jessewil...@google.com on 8 Oct 2011 at 12:02