Closing all resolved issues in CDI 1.x

https://github.com/cdi-spec/cdi/pull/225
This PR also resolve CDI-372

this issue is related to other issues dealing with Bean Defining annotations

Yes pmuiratbleepbleep and jgreene@redhat.com we all agree on that (I hope so). For me it was implicit that option 3) includes a mention in the spec regarding that, while option 2) would warn the user at boot time.

Now the majority chose the option 3 (aka the RTFM option) we should be sure that the spec include something about it : "you shouldn't use default bean discovery mode to use Producers and Observers or annotate the beans that contains them with bean defining annotation (that's pretty clear). Should we spread it in different chapters (Producers and Event) or should we put it somewhere else ?

pmuiratbleepbleep your comment reminds me that the following producer should not be discovered either:

class Producer {
  @Produces @ApplicationScoped
  Foo produce() {
    ... 
  }
}

Also not very intuitive for users although legal and covered in (among others) "5.1.2 Enabled and disabled beans".

I agree with jgreene@redhat.com, I don't want to extend the behavior, and instead to make it clear you need to use a class level annotation to make it a bean.

I should clarify that my preference for Option 1 was only because I didn't like the other two options. 2 logs a warning that should already be generally known (and could be too verbose). 3 encourages "all". To be clear, I would also go for a different option (4): Tell the user to put a defining annotation on the class if they want it picked up.

Option 3 for me. Unless the likelihood of having an app with @Produces/@Observes but no other bean-defining annotationa is high. If that is the case then option 1 makes more sense to me.

Right, I would prefer to keep bean defining annotations reasonably simple to understand.

This subclass will not get picked up if we don't look up in the class hierarchy.

I find the fact that only classes that declare a bean defining annotation within their definition are picked up quite easy to grasp by application developers.

There is no reason why picking @Observes and @Produces but not @Inject... So either we have a clear class-only or we go the full route imo.

I agree that picking up based on @Produces but not on @Inject is counter-intuitive. However, we cannot go "the full route" because @Inject is not CDI-specific which means that we would end up in a situation similar to the one with @Singletons. So after all, option 3 is probably the best one.

[jharting] you have a class which has some CDI 'features'. Then you create a subclass for it. This subclass will not get picked up if we don't look up in the class hierarchy.

To be honest. If we start with 1. then people will complain why we cannot pickup beans as @Dependent if they have an @Inject field. There is no reason why picking @Observes and @Produces but not @Inject... So either we have a clear class-only or we go the full route imo.

but we would also need to look up all fields and methods in the super-classes!

Why?

I'd vote for option 2, but looks a bit too expensive so I'd vote for #3.

+1 for option 3. Simply because option 1 would need us to not only look inside a class for annotations, but we would also need to look up all fields and methods in the super-classes!

(Even though my vote will not count) I vote for option 1 but we will be happy with option 3 if documentation takes place in MR 1.2.

Just in case we go with option 3 I will create a new ticket containing option 1 for CDI 2.0.
Thanks for the discussion.

Option 1 for me

Ok, I vote for option 3.

+1 rhn-engineering-jharting. I was only thinking aloud.

Sure (2) will have less side effect but at the same time it's a waste to do this analysis work and not implement (1) . Ambivalent was your word pmuiratbleepbleep :-D.
Perhaps we should vote here...

If we add any requirements on what is logged by the application server it should have a form of a recommendation instead of a strict requirement IMO because:
1) it cannot be tested by TCK anyway
2) AFAIK there is no precedence for this in EE. I have only seen recommendations on logging in EE specs.

I guess that (2) is possible...

pmuiratbleepbleep I'm not a big fan ever but in fact I'm not a big fan of the annotated mode and this is linked to it. That said I really think we should try to avoid option 3 IMO. Or at least request that the impl logs the list of ignored class in annotated mode.

I have to say that from a functional perspective I am quite ambivalent about this feature.

You should also think about what happens with a producer method which is contained in a super class.

I think that if we go with option 1 we should only detect classes that declare an observer/producer method. This would be consistent with how we treat bean defining annotations now where we require them to be declared on a class (if they are inherited that is not enough).

Honestly I don't understand struberg. Today in CDI 1.0 and in CDI 1.1 with bean-discovery-mode=all mode we do these deep class analysis. So the code is already there in implementations. Ok, it probably has to be adapted but I don't see where is the "much more work" in term of dev.
Solution at the impl level are multiple :

• Start as if we where in bean-discovery-mode=all and use the same mechanism that reject non eligible class (after finding no default constructor for instance) or that @Vetoed it, if it doesn't contain bean defining annotation
• Use annotation processing to decide at compile time.

My understanding of the annotated bean discovery mode is not about optimization, it's about CDI automatic activation. If it's optimized it's better but the first goal is to provide CDI implicitly. For me it's not important if the annotated bean discovery mode take the same amount of time at boot time than all bean-discovery-mode and that's the only risk we take here in term of "much more work".
IMO The real question is "does this feature is interesting for the end user ?" and then "how much does it costs in terms of dev and perf ?".
In this particular case I won't say the feature is awesome but the lack of it is very not user friendly.
We have zillion forum post and stack exchange "bug" out there about CDI 1.0 users forgetting beans.xml in their app. Now that we provide the mean in CDI 1.1+ to get rid of this file I really think we should do it as complete as possible.

Regarding the producer method in superclass, I'm not sure to totally understand your use case. Can you give an example ?

extending the bean defining annotations with annotations from the class itself is no problem. But having to go into the class and define producer methods or fields, etc as bean defining would be much more work.

You should also think about what happens with a producer method which is contained in a super class. Does that mean we should pick up the child class as dependent scoped?

Imo we should have a few very clear rules. Not too many rules and not too complex ones. If a user expects a class to get picked up is only a matter the learning curve

Thanks jgreene@redhat.com and pmuiratbleepbleep .
That means that we should enhanced the list of bean defining annotations by adding @Observes and @Produces to them and introduce the fact that bean defining annotations can be inside a class.
Is it ok for everyone?

It's not a problem to pick up on annotations at the method level, since that information is already analyzed. So I would go with option 1 IMO.

jgreene@redhat.com, sdouglas1@redhat.com can you comment on any performance concerns with antoinesabot-durand summary of options?

IMHO we have 3 solution to address this issue (

1. Add to the bean defining annotation list @Produces and @Observes. That will add to the list annotations inside a class.
2. Detect @Produces and @Observes in class that don't have bean defining annotation and warn the user at boot time. It seems to be a work very close to previous point in term of scanning.
3. Do nothing in the container and only document in spec the fact that in annotated mode these will be ignored silently. Not very user friendly but bean-discovery-mode=all is not out of reach .

Ideally I would prefer the first solution but do we have time to implement this for the MR? Input from implementors (rhn-engineering-jharting, struberg or mkouba@redhat.com) seems mandatory here.

Please note that the following things also do not get discovered right now:

• Interfaces - this is often used by Extensions
• static Producer methods. Usually the Classes containing these producer methods do not have a CDI Scope...

Since CDI-404 is probably covered by CDI-1.1-MR would it be possible to include this one in CDI-1.1-MR too?