Should we get rid of the "optional" attribute from @Inject?

[GoogleCodeExporter]

I don't think this feature carries its weight. It's very tricky to specify
and implement. I think it's better to just bind a default value explicitly.

Original issue reported on code.google.com by crazybob...@gmail.com on 16 Jun 2009 at 8:03

[GoogleCodeExporter]

We can always add this feature later.

Original comment by crazybob...@gmail.com on 16 Jun 2009 at 8:09

[GoogleCodeExporter]

Please get rid of it. Since we already have the Provider interface it's just 
not needed.

Original comment by gavin.k...@gmail.com on 16 Jun 2009 at 8:23

[GoogleCodeExporter]

I may have misunderstood your point, but the Provider interface can't supplant
"optional", at least not in Guice.

Guice only skips injections with "optional=true" if the dependency or something 
it
depends on isn't configured at all. If Guice encounters an error while 
resolving the
dependency (like an exception thrown from a factory), Guice generates an error
instead of skipping the injection. This ensures that we never ignore real 
errors.

Further, if you inject Provider<Foo> using Guice and you don't have a binding 
to Foo
configured, Guice will generate an error immediately (initialization time at 
the latest).

I'm more than happy to drop "optional" though and will take it out of the spec
shortly unless someone objects.

Original comment by crazybob...@gmail.com on 16 Jun 2009 at 10:56

[GoogleCodeExporter]

Oh, OK, I assumed that you would not get the exception until you called get().
Whatever, I still think we don't need it :-)

Original comment by gavin.k...@gmail.com on 16 Jun 2009 at 11:01

[GoogleCodeExporter]

I agree that this feature is worth including in the specification.

Original comment by thiag...@gmail.com on 17 Jun 2009 at 12:26

[GoogleCodeExporter]

I found it very handy to be able to use optional injection in the 
implementation of the 
DWR-Guice integration package, and I don't see an easy alternative. So I'm 
tentatively 
objecting.

Original comment by tpeie...@gmail.com on 17 Jun 2009 at 2:14

[GoogleCodeExporter]

Optional is handy, especially when you want to supply a module for somebody 
else to consume...

   @Inject(optional=true) @Port int port = 8080;

Original comment by limpbizkit on 17 Jun 2009 at 8:31

[GoogleCodeExporter]

I would *much* prefer to handle this usecase by injecting a Provider.

Original comment by gavin.k...@gmail.com on 17 Jun 2009 at 4:30

[GoogleCodeExporter]

I'm sorry, I wrote exactly the opposite of what I think: I think this feature 
is 
*not* worth to be included in the specification.

Original comment by thiag...@gmail.com on 17 Jun 2009 at 6:46

[GoogleCodeExporter]

How is injecting a Provider equivalent?

The idea of @Inject(optional=true) is that the bean will take advantage of a 
dependency if there is one, 
otherwise it'll do its job using some meaningful default.

If you injected a Provider instead, the bean would always call get(), then it'd 
have to handle getting a null 
back, or worse an exception. Note that the Provider would look like any other 
one, so the contract for 
Provider.get() would have to allow not returning an instance. But that's bad: I 
wouldn't want all beans that use 
Provider to have to defend against nulls or exceptions as a regular occurrence! 
I guess I'm agreeing with 
Guice's behavior here.

We could keep the feature and still drop optional from @Inject by adding a new 
type for "faulty providers".

Original comment by roberto....@sun.com on 17 Jun 2009 at 6:59

[GoogleCodeExporter]

Yeah, Provider is not at all an alternative to optional=true. In fact, you can 
use them together:

  /**
   * If the user has configured an executor service, use that. Otherwise, we
   * fall back to using our own single-threaded executor.
   */
  @Inject(optional=true) Provider<ExecutorService> executorServiceProvider = new 
Provider<ExecutorService>() {
    ExecutorService result = null;
    public synchronized ExecutorService get() {
      if (result == null) {
        result = Executors.newSingleThreadExecutor() 
      }
      return result;
    }
  };

Original comment by limpbizkit on 17 Jun 2009 at 7:05

[GoogleCodeExporter]

"The idea of @Inject(optional=true) is that the bean will take advantage of a
dependency if there is one, otherwise it'll do its job using some meaningful 
default."

Not correct. Have you read the JavaDoc for @Inject. @Inject(optional=true) on a 
field
means "leave the field null". That's not a "meaningful" default.

"the bean would always call get(), then it'd have to handle getting a null 
back, or
worse an exception"

How is this different to having to handle a null value of the field, or a
NullPointerException?? 

In fact, having to call a method - get() - gives you a warning that you might 
need to
handle a null value/exception than having a field that might be null.

Urm, so from my point of view I could turn that round and say: "I wouldn't want 
all
beans that access an injected instance variable have to defend against nulls or
exceptions as a regular occurrence!"

Original comment by gavin.k...@gmail.com on 17 Jun 2009 at 7:15

[GoogleCodeExporter]

"Yeah, Provider is not at all an alternative to optional=true. In fact, you can 
use
them together"

Are you (a) describing the documented semantics of Provider, (b) telling me 
that this
is what your product does today, or (c) making an argument that this is the 
right
thing to do?

If (a), I don't see where this is documented, and if it is, I'm not sure that 
it is
correct.

If (b), I don't care.

If (c), you have not actually made the argument.

Basically, the point I'm making, is that I would prefer that an "optional 
dependency"
be expressed by a non-null field of type Provider, than by a null field. The 
case of
"optional dependencies" is amazingly rare, IMO, and I therefore don't want to 
pollute
the @Inject annotation to handle this case.

Original comment by gavin.k...@gmail.com on 17 Jun 2009 at 7:22

[GoogleCodeExporter]

"We could keep the feature and still drop optional from @Inject by adding a new 
type
for "faulty providers"."

This could be as simple as adding a binding type.

Original comment by gavin.k...@gmail.com on 17 Jun 2009 at 7:26

[GoogleCodeExporter]

Excuse me, qualifier.

Original comment by gavin.k...@gmail.com on 17 Jun 2009 at 7:27

[GoogleCodeExporter]

gavin.king:
 > Have you read the JavaDoc for @Inject. @Inject(optional=true) on a field means "leave the field null". That's 
not a "meaningful" default."

I don't know what spec you're reading, but the @Inject Javadoc says, "If a 
dependency matching the field can't be 
found, the injector will not set the field." It doesn't mention null at all. 
Therefore a default value (like 8080) may 
be used.
  http://atinject.googlecode.com/svn/trunk/javadoc/javax/inject/Inject.html

Original comment by limpbizkit on 17 Jun 2009 at 7:48

[GoogleCodeExporter]

Ah, fair enough. It's true that optional=true is less objectionable *if* the 
field
has an explicit initializer. Unfortunately there's no way to limit its usage to
fields with initializers.

But seriously, is this thing *really* common enough that it really warrants
vandalizing the main @Inject annotation with a boolean annotation member? I 
*hate*
boolean-valued annotation members....

Original comment by gavin.k...@gmail.com on 17 Jun 2009 at 8:15

[GoogleCodeExporter]

Tim, could something similar to Guice's Modules.override() work as an 
alternative?

Optional injections have been one of the biggest sources of confusion for Guice 
users; it's difficult for them to 
differentiate between missing configuration vs. other types of errors. There's 
also the matter of whether we skip 
injections when transitive deps are missing vs. just the direct dep.

Original comment by crazybob...@gmail.com on 19 Jun 2009 at 3:04

[GoogleCodeExporter]

[Bob Lee] Could something similar to Guice's Modules.override() work as an 
alternative?

I don't know enough about Modules.override() yet, but I bet you're right. I'll 
learn 
more and report whether my objection disappears.

Original comment by tpeie...@gmail.com on 19 Jun 2009 at 3:12

[GoogleCodeExporter]

OK, I see how I could rewrite the DWR-Guice integration package (for example) 
to 
avoid @Inject(optional=true) using Modules.override:

    return Guice.createInjector(
        Modules.override(new DwrDefaultsModule()).with(/* user modules */));

It's not as convenient as being able to specify the default bindings on the 
fields 
themselves, but I do see that decoupling those defaults is a good thing in 
general.

But is specifying and mandating a facility like Modules.override really within 
the 
purview of JSR-330? If so, I cheerfully withdraw my objection. If not, well ... 
I 
wouldn't put up a fight about losing optional injection, but I wouldn't be 
cheerful 
about it, either.

Original comment by tpeie...@gmail.com on 22 Jun 2009 at 3:11

[GoogleCodeExporter]

I'll definitely push for something like Modules.override() in the config API.

Original comment by crazybob...@gmail.com on 22 Jun 2009 at 6:56

[GoogleCodeExporter]

It's gone as of r4 (http://code.google.com/p/atinject/source/detail?r=4).

Original comment by crazybob...@gmail.com on 23 Jun 2009 at 11:18

• Changed state: Fixed

[GoogleCodeExporter]

Great, thanks.

Original comment by gavin.k...@gmail.com on 24 Jun 2009 at 1:36

[GoogleCodeExporter]

Plese consider collection injection... see issue #20

Original comment by mccallum...@gmail.com on 8 Feb 2010 at 7:05

[GoogleCodeExporter]

Please reopen this, as there is no way to have collection injection!

Original comment by ptit...@gmail.com on 13 Dec 2011 at 2:47