Created attachment 135801 [details]
Sketch of possible solution

Attached patch is moving both packages to newly created module called j2ee.injection (I'm definitely up to discussion about better name if we can't find one).

I tried to move everything to web.beans module but it results in a lot of friend dependencies in web.beans which doesn't seem to be correct with respect to the fact that most of these modules would use only this "injection" functionality. Solution with new module is more clear and it is needed for issue 230831.

CCing to David to know also a different opinion about module naming or implementation in general.

Right now, the patch looks well. I only guess about more generic naming since we will likely need more specification/feature oriented APIs. 

I think that by J2eeProjectCapabilities clean-up I will need something like JpaSupport, JsfSupport, EjbSupport etc. but not server based as in cases of javaee.spec.support but rather project/client oriented. In that case would make sense something like javaee.spec.client and the Injection classes could be one of its feature specific supports.

But to go step by step is often better. The patch looks definitely great to me. Once I'd create something else, we can discuss whether it would make sense to move this from j2ee.injection module or not.

Thank! Unsorted comments:

* javac.source should be 1.7 for new module

* EEInjectiontargetQueryImplementation should not stay in web.common I think. Usually if there is a default or base impl of an SPI then such implementation is usually defined in the same module which defines the SPI. So moving EEInjectiontargetQueryImplementation to the new module would make sense to me. 

* I would be tempted to merge WebInjectionTargetQueryImplementation into EEInjectiontargetQueryImplementation

* I think for new modules we are using prefix "javaee" instead of "j2ee"

* re "web.beans" - it might be actually a good idea to put it here. In long term there will be (and is already happening) a re-alignment between CDI and all other "managed beans" defined by individual specs. I think in long term the CDI will rule them all and other specs will just depend on CDI capabilities. So having the SPI/API in web.beans is probably perfectly OK.

* "injection" name is quite limiting - I cannot imagine using the module for anything else (which may not be a bad thing but extreme side effect is dozens of modules with two or three classes on average which is messy too). Perhaps "queries" would be more generic and allows more stuff to be added into the module like Martin's J2eeProjectCapabilities.

I'm not sure. I wish I could give some clearer answers. :-) We should consider other classes in j2ee.common and see whether they could be moved together into this new module. But it is always OK to do it iteratively - it is just more work in the end as everything has to renamed and repackaged few times.

(In reply to comment #4)
> * javac.source should be 1.7 for new module

Good catch, I'll do that.

> * EEInjectiontargetQueryImplementation should not stay in web.common I think.
> Usually if there is a default or base impl of an SPI then such implementation
> is usually defined in the same module which defines the SPI. So moving
> EEInjectiontargetQueryImplementation to the new module would make sense to me. 

True! It shouldn't stay in web.common. But it doesn't look like "default implementation". All those SPI implementations (Web/EE/WebBean/Ejb etc. with InjectiontargetQueryImplementation suffix (I will shortened it as ITQI) seems to be doing similar thing within a different context - some of them are resolving JSP files, some of them are checking for a Web Service annotations etc.).

EE implementation is related only to the presence of the "javax.annotation.ManagedBean" annotation and thus I would move it to the module that typically cares about such things (j2ee.ejbcore ??). What do you think?

> * I would be tempted to merge WebInjectionTargetQueryImplementation into
> EEInjectiontargetQueryImplementation

I liked that idea at first but then I've been looking at the old discussions to know how and why these implementations were created (see details bellow) with respect to the historical context:
- web.beans:    web-main #b354d3eb5d91, issue 189579
- web.core:     web-main #0e1dbde05ef1
- j2ee.ejbcore: web-main #0e1dbde05ef1
- j2ee.common:  web-main #ef341b64ddbd, issue 188821

..after reading those discussions I think that Web-ITQI should stay in web.core because it's related to the JSP functionality (checking for "javax.servlet.Servlet" subclasses)

> * I think for new modules we are using prefix "javaee" instead of "j2ee"

Sure, I'll do that.

> * re "web.beans" - it might be actually a good idea to put it here. In long
> term there will be (and is already happening) a re-alignment between CDI and
> all other "managed beans" defined by individual specs. I think in long term the
> CDI will rule them all and other specs will just depend on CDI capabilities. So
> having the SPI/API in web.beans is probably perfectly OK.

I don't have strong opinion about this but from what I have understand the CDI specification and this injection ability for a certain class is something different. I'm fine with putting it into the web.beans, it just looks to me more clear to have dependency on javaee.injection where the purpose of the module is obvious then dependency on web.beans module which is quite complex with a lot of responsibilities. If you think those web.beans dependencies will be needed in the future anyway we still might easily merge both modules when the time will come (by merging I mean delete javaee.injection and move these classes to the web.beans). But as I said, don't have strong knowledge about this, so I'm open-minded to both solutions.
Martine, could you please comment on this?

> * "injection" name is quite limiting - I cannot imagine using the module for
> anything else (which may not be a bad thing but extreme side effect is dozens
> of modules with two or three classes on average which is messy too). Perhaps
> "queries" would be more generic and allows more stuff to be added into the
> module like Martin's J2eeProjectCapabilities.

I understand the concern but feel it a little bit differently. It's a quite separated area (we don't have many similar small-areas) and my argument would be actually the same as your :) ..I cannot imagine using the module for anything else. From my point of view this is a good thing - anyone who will be willing to put something there needs to think about it much more and if he decides to put it there, then probably also module name refactoring will be needed which is completely fine. OTOH with javaee.queries anyone who will find any type of query (from whatever area it is) will put it there - which, in a few years, might result in the same state as j2ee.common has now.
From that point of view I'm either for javaee.injection (or different name but something that shows directly the module purpose) or we can use web.beans as discussed above.

> I'm not sure. I wish I could give some clearer answers. :-)

It's fine, I don't have clear questions so I'm not even expecting clear answers ;)

> We should consider
> other classes in j2ee.common and see whether they could be moved together into
> this new module. But it is always OK to do it iteratively - it is just more
> work in the end as everything has to renamed and repackaged few times.

Ye, I agree - I'm just more for the iterative way in this case to minimize possible regressions :)

Thanks you for your comments Davide. It's funny at the beginning I was 100% for your ideas but at the end I'm reading my comment and it looks like I disagree with most of the things you said. It's not intentional, it's just when I read those old issues/comments I finally feel I know what are these classes doing :)

OMG, that is so long. Sorry!

(In reply to comment #3)
> In that case would make
> sense something like javaee.spec.client and the Injection classes could be one
> of its feature specific supports.

I see and agree :)

> Once I'd create something else, we can discuss whether it would make sense to
> move this from j2ee.injection module or not.

Yup I think we can easily remove the module if the future refactoring shows up it's not needed or the name should be different.

(In reply to comment #5)
> EE implementation is related only to the presence of the
> "javax.annotation.ManagedBean" annotation and thus I would move it to the
> module that typically cares about such things (j2ee.ejbcore ??). What do you
> think?

I have read [1] and now I understand your suggestion ..ejb.core isn't probably the only place where we are facing javax.annotation.ManagedBean. I agree with moving this implementation into the javaee.injection

Sorry for confusion obviously I'm still learning :)

[1] http://stackoverflow.com/questions/11986847/java-ee-6-javax-annotation-managedbean-vs-javax-inject-named-vs-javax-faces

> Thanks you for your comments Davide. It's funny at the beginning I was 100% for
> your ideas but at the end I'm reading my comment and it looks like I disagree
> with most of the things you said.

That happens to me often too. As you are learning more about a problem the solution keeps evolving. That's a right approach IMO. Regardless where it takes you. Even if it should contradict your initial thoughts. I really like it when that happens. :-)

re. modules - let's start with your original proposal: a new module named javaee.injection and later we will see whether it can be merged with web.beans

One thing I worry is about going from one "extreme" (a single j2ee.common module with bunch of loosely related APIs) to the other extreme where we have dozen modules each having two API classes. Such outcome would be as bad as current state - it will be hard to find right module/APIs because there will be so many modules to look into. I think javaee.injection is going to be fine but I would not want to handle this way each other class from j2ee.common. :-) So let's compromise somewhere in the middle. If something is used by many EE/Web modules it may just be a good candidate to stay in some common module like "j2ee.common" with adequately named Java package.

(In reply to comment #5)
> Martine, could you please comment on this?
+
(In reply to comment #9)
> re. modules - let's start with your original proposal: a new module named
> javaee.injection and later we will see whether it can be merged with web.beans

As I said I'm not fan of the web.beans. I think that pretty good natural object-oriented partitioning of J2EE functionality is by its specification or feature (depends how wide the specification is). CDI is one specification and the "InjectionTarget" is rather "feature" of the JavaEE spec. Also CDI is only one set of injection points used for injection. I think that "javaee.injection" module is the best now.

Today morning I thought about the first API naming a little bit. :) It looks to me that the "Injection" can be pretty misleading (since the CDI was created) and the real sense is rather "ManagedByContainer" (or "CanUseInjection"). But on the other side it isn't cogent name for all InjectionTargets. It's pretty difficult to get ideal naming using 2/3 words for such interchangeable features. Sorry, no important information here, just that the issues with placing are on place since it's pretty complicated.
 
> One thing I worry is about going from one "extreme" (a single j2ee.common
> module with bunch of loosely related APIs) to the other extreme where we have
> dozen modules each having two API classes. Such outcome would be as bad as
> current state - it will be hard to find right module/APIs because there will be
> so many modules to look into. I think javaee.injection is going to be fine but
> I would not want to handle this way each other class from j2ee.common. :-) So
> let's compromise somewhere in the middle. If something is used by many EE/Web
> modules it may just be a good candidate to stay in some common module like
> "j2ee.common" with adequately named Java package.

Good point - totally I agree with David. I don't want to spread it into tens of modules too. That is my intention to create spec/client oriented module where all such features/spec. API/SPI can go - on place, split package per specification+feature - could look pretty clear finally.

(In reply to comment #10)
> Today morning I thought about the first API naming a little bit. :) It looks to
> me that the "Injection" can be pretty misleading (since the CDI was created)
> and the real sense is rather "ManagedByContainer" (or "CanUseInjection"). But
> on the other side it isn't cogent name for all InjectionTargets. It's pretty
> difficult to get ideal naming using 2/3 words for such interchangeable
> features. Sorry, no important information here, just that the issues with
> placing are on place since it's pretty complicated.

Agree that injection is way to general. I like "ManagedByContainer", maybe we can refactor method names if there will be an agreement about it.

> Good point - totally I agree with David. I don't want to spread it into tens of
> modules too. That is my intention to create spec/client oriented module where
> all such features/spec. API/SPI can go - on place, split package per
> specification+feature - could look pretty clear finally.

I absolutely agree with you guys ;)

I increased the source level, renamed module to use "javaee" prefix and pushed the current state as web-main #e2397189d98f

I'll move also EEInjectiontargetQueryImplementation to the javaee.injection after resolving issue 230831 (I have to avoid cyclic dependencies)

Btw. check if some of those moves allowed to eliminate some of the j2ee.common "friends".

Not possible yet. I have to resolve issue 230831 and move few utilities methods first.

Btw. what is your plan to do with DDHelper? That one is used from many modules. I would be tempted (without thinking about it much) to move to j2ee.core - public API is OK in this case and DDHelper's API is straightforward.

Don't have concrete plan yet :) One idea was to move it to DD API, but not sure if the module serves for this purpose. Maybe I would tempted to let it in the general utility module

Note: We are mapping the javaee.injection module to the dashboard Java EE/CDI component.

(In reply to comment #17)
> One idea was to move it to DD API

Sounds logical. The module itself is terrible though and I wish we could just delete it. But it is not worth rewriting. It generates Java API from XML Schema using schema2beans which nobody maintains anymore and there is dozen of awful "search and replace text" hacks on generated Java classes. And generated API contains methods for all possible DD element (dozens if not hundred) while handful of them are used these days. Handwritten API would be much better. So in that light I would prefer to let it be forgotten. :-)

Btw. in #d9a54f966af7 I moved some API to j2ee.dd but only because the API itself is very old and sort of as dead as j2ee.dd. :-)

Not sure about it. j2ee.dd is a public API and DDEditorNavigator does not seem (at least when looking to the implementation) it belongs there - only the name says it has something in common with DD but the implementation is quite abstract. But I don't know anything about both module and class so just noticing it's a public API :)

Re. d9a54f966af7 - MartinF, what do you think about DDEditorNavigator? We could probably delete the API completely as it is only used for CMP. On the other hand I do not mind to bury it in j2ee.dd and forget about it. j2ee.dd contains zillion of other public API classes which are not used so one more is not going to make a difference. I would like to think about myself as "pragmatic" :-) but I would understand if you guys think this is a hack. :-)

> DDEditorNavigator does not seem it belongs there - only the name
> says it has something in common with DD but the implementation is quite
> abstract.

j2ee.ddloaders module used to provide GUI for DD editing. And DDEditorNavigator was meant to allow to open and select GUI component corresponding to a DD element. The DDEditorNavigator SPI should have been 
  showElement(CommonDDBean anElement);
but for some reason it was not (probably because there is also XmlMultiViewDataObject.showElement(Object)?). In any case there is some relation to DD.

Let me know and I'm happy to change it if both of you disagree.

(In reply to comment #22)
> Re. d9a54f966af7 - MartinF, what do you think about DDEditorNavigator?

I think that move into public API w/out passing apireview process can be pretty dangerous.

In any case I would rather completely remove it since it's used only by navigation into appropriate tab and field of the ejb-jar DD multiview. But this multiview is disabled now and once it will be fixed+enabled again the CMP will be much more obsolete than it's now. ;)

Summary: It's unused now, hardly ever used after enabling ejb-jar DD view -> deletion is the best solution, I think.

> I think that move into public API w/out passing apireview process can be pretty
> dangerous.

I wish I could just go to your office and have a argument about this. :-) I so much disagree! :-) Have a look at other 100 Java interfaces in the public API of this module. DDEditorNavigator is not going to make any difference.

But deleting is ever better. I'm going to do it today.

> But deleting is ever better.
Done: #2ac556c36b52

(In reply to comment #24)
> I wish I could just go to your office and have a argument about this. :-) I so
> much disagree! :-) Have a look at other 100 Java interfaces in the public API
> of this module. DDEditorNavigator is not going to make any difference.

The same from me, let's open the theme once you would come to Prague. :) Sorry, I'm probably too much conservative in this but as you can see, the review process could stop the extra work you had on it and it would be another undocumented/unusable public API in that module. I agree that it can look like the small issue in compare to the pack of another classes but it still breaks good design principles. I believe that we will be able to talk about such topics personally. :)

(In reply to comment #25)
> > But deleting is ever better.
> Done: #2ac556c36b52

Great, thanks a lot!

...until then "Let's agree to disagree". :-)

Integrated into 'main-golden', will be available in build *201306252301* on http://bits.netbeans.org/dev/nightly/ (upload may still be in progress)
Changeset: http://hg.netbeans.org/main-golden/rev/2ac556c36b52
User: David Konecny <dkonecny@netbeans.org>
Log: fixing my previous refactoring by removing the old API completely (see discussion in #230847)

(In reply to comment #27)
> ...until then "Let's agree to disagree". :-)

Agree. :)