Add support for single non no-arg constructor injection without @Inject

[geoand]

Basically when the bean has a single constructor that contain arguments,
then the @Inject annotation is not necessary, since the injection point
can be safely deduced.
This is analogous to what Spring DI does since Spring 4.3

[mkouba]

So this is definitely against the spec. On the other hand, we don't run TCK and not even try to be fully spec-compliant. It really depends on how useful you guys think this "feature" is. @emmanuelbernard @stuartwdouglas @FroMage @Sanne and others?

[Sanne]

Not my area of expertise, but out of curiosity: by "against the spec" you mean the spec doesn't mention anything like this, or is it explicitly prohibiting such a thing to work?

In the second case, might be good to know why they did that.. might have good reasons?

(Other than that doubt I personally like it!)

[mkouba]

@Sanne http://docs.jboss.org/cdi/spec/2.0/cdi-spec.html#instantiation and http://docs.jboss.org/cdi/spec/2.0/cdi-spec.html#declaring_bean_constructor. So stricly speaking the constructor should not be found. But I don't think there is a tck test that would fail...

@manovotn @antoinesd Your opinion?

[manovotn]

This is definitely against CDI spec so the real question here is how much you want this implementation to pretend it's CDI compliant and how much you want to step away and say you are creating a different (C)DI implementation.

Settings that aside, I think this is more of a syntactic sugar rather then anything else. Bean with just one constructor cannot be a CDI bean anyway, as you need a no-args one by definition, so I think it should work.

[mkouba]

Bean with just one constructor cannot be a CDI bean anyway, as you need a no-args one by definition..

Hm, I disagree. A bean with non-normal scope can either have no-args constructor or a constructor with @Inject -> and this PR is only about the possibility to omit the @Inject annotation.

[manovotn]

Sorry, I wasn't clear. In my previous comment I meant that a class that has only once constructor and that one is parameterized.
In such a case, without the @Inject annotation, CDI will blow up complaining it doesn't have a no-args one. Hence as a programmer, if you wanted that class to become a bean, you simply missed the @Inject annotation which is something this PR addresses.

[Sanne]

@Sanne http://docs.jboss.org/cdi/spec/2.0/cdi-spec.html#instantiation and http://docs.jboss.org/cdi/spec/2.0/cdi-spec.html#declaring_bean_constructor. So stricly speaking the constructor should not be found. But I don't think there is a tck test that would fail...

The bean constructor may be identified by annotating the constructor @Inject.

My reading of the spec is that it describes one way which is expected to work. The language is open-ended, it doesn't preclude you to offer additional ways of constructing such beans.

As long as you don't violate the definition:

The bean constructor is a default-access, public, protected or private constructor of the bean class.

I'd argue you'd still be allowed to call this a CDI implementation (if not for missing other things..)

[manovotn]

@Sanne this part of spec http://docs.jboss.org/cdi/spec/2.0/cdi-spec.html#what_classes_are_beans states that:

It has an appropriate constructor - either:

• the class has a constructor with no parameters, or
• the class declares a constructor annotated @Inject.

[Sanne]

@manovotn good catch. That section is indeed more restrictive

[emmanuelbernard]

If the change makes sense, let's push for a change in the spec.

[geoand]

If the change makes sense, let's push for a change in the spec.

I know that I use the respective Spring capability whenever the opportunity arises :P, so from my point of view it makes a lot of sense

[stuartwdouglas]

I think we should include it and try and change the spec.

[manovotn]

Yea, we can make a proposal for sure. I'll put up an issue tomorrow

[manovotn]

FYI, created CDI-742. Feel free to add any remarks and notes.

[geoand]

So do we merge this in the meantime or wait?

[emmanuelbernard]

+1 to merge

[manovotn]

I'd say merge, one might grow a beard before there is next CDI release ;-)

[mkouba]

@geoand Ok, so we agreed that this "feature" should be merged. Now let's polish the code a little ;-)

[geoand]

@mkouba Of course! What sort of polish do you have in mind?

[mkouba]

The injections list contains all injection points found, i.e. injections.isEmpty() returns false if any injection point (field, method) is found. If you add @Inject Head head2 field to your test it fails.

[geoand]

That's a very interesting case I hadn't thought of :).
I will see how it can be fixed

[mkouba]

I would just check isFirstLevel first and then proceed if there is no injection for which Injection.isConstructor() returns true ;-)

[geoand]

That's what I'm thinking as well, but I'm trying to think if it would fail in any case - but probably not :)

[geoand]

Fixed. Should I also rebase onto the current master?

[stuartwdouglas]

Unless there are conflicts it should not really matter

[geoand]

No conflicts, so I'll leave as is then

[geoand]

@mkouba Tests passed with the latest fix. Is there anything else you would like to see?

[mkouba]

@geoand Thanks for your contribution ;-)

[geoand]

@mkouba You are most welcome!