NETBEANS-4035: CDI Bean as Named is not listed in xhtml for selection #5866

juneau001 · 2023-04-21T13:44:11Z

This PR adds support for CDI injection in Jakarta EE 9+ projects. This will allow @nAmed beans to be injectable within JSF views via expression language.

^Add meaningful description above

By opening a pull request you confirm that, unless explicitly stated otherwise, the changes -

are all your own work, and you have the right to contribute them.
are contributed solely under the terms and conditions of the Apache License 2.0 (see section 5 of the license for more information).

Please make sure (eg. git log) that all commits have a valid name and email address for you in the Author field.

If you're a first time contributor, see the Contributing guidelines for more information.

If you're a committer, please label the PR before pressing "Create pull request" so that the right test jobs can run.

neilcsmith-net · 2023-04-24T16:22:35Z

Did you mean to put NB18 on this? It's open against master, not delivery, and would seem a lot to land after feature freeze. OTOH it's also marked as a high priority bug.

Added do-not-merge label for now pending clarification and review.

matthiasblaesing · 2023-04-24T17:45:05Z

@juneau001 to make review easier: Am I right if I suspect, that jakarta.web.beans was created by copying web.beans and then modifying the resulting copy (different namespace for example)?

juneau001 · 2023-04-24T18:02:34Z

Hi @matthiasblaesing and @neilcsmith-net I did wish to try and get this fix into NB18, but I certainly can understand if it needs to be pushed to NB19 due to the timing. You are correct @matthiasblaesing that jakarta.web.beans is a replica of web.beans with the namespaces changed. I also made some code adjustments within other modules to determine which of the web beans modules to load on a per-project basis.

If you wish to try and get this into NB18, then I can work on opening a different pull request against the delivery branch. Otherwise, I can re-label to NB19 if you'd rather hold off for now...it is certainly understandable. Thanks for the feedback!

pepness · 2023-04-24T18:36:06Z

Hi @juneau001, I submit a PR to your branch.

matthiasblaesing

Thank you for the work - it is great, that you are working on this.

When building from source, I noticed, that I can't activate the java cluster or the java ee cluster in the plugin manager, so I could not have a look at the new situation.

For the naming I would suggest to try to keep the directories together, so instead of jakarta.web.beans, I'd suggest web.beans.jakarta.

I noticed that the new module is named identically to the original one, I think some kind of suffix should be added to discern the two. The same might be required for templates and similar stuff.

Please don't take this negatively, the work is appreciated.

matthiasblaesing · 2023-04-24T19:27:59Z

...eb.beans/src/org/netbeans/modules/web/beans/analysis/analyzer/AbstractDecoratorAnalyzer.java

@@ -168,7 +168,7 @@ protected boolean checkBuiltInBeans( VariableElement element,
        Map<String, ? extends AnnotationMirror> qualifiersFqns = helper.
            getAnnotationsByType(qualifiers);
        boolean hasOnlyDefault = false;
-        if ( qualifiersFqns.containsKey(AnnotationUtil.DEFAULT_FQN)){
+        if ( qualifiersFqns.keySet().contains(AnnotationUtil.DEFAULT_FQN)){


Unnecessary change.

matthiasblaesing · 2023-04-24T19:28:28Z

...beans/src/org/netbeans/modules/web/beans/analysis/analyzer/field/InjectionPointAnalyzer.java

@@ -182,7 +182,7 @@ private void checkInjectionPointMetadata( VariableElement element,
        Map<String, ? extends AnnotationMirror> qualifiersFqns = helper.
            getAnnotationsByType(qualifiers);
        boolean hasDefault = model.hasImplicitDefaultQualifier( element );
-        if ( !hasDefault && qualifiersFqns.containsKey(AnnotationUtil.DEFAULT_FQN)){
+        if ( !hasDefault && qualifiersFqns.keySet().contains(AnnotationUtil.DEFAULT_FQN)){


Unnecessary change.

matthiasblaesing · 2023-04-24T19:28:40Z

...org/netbeans/modules/web/beans/analysis/analyzer/method/InjectionPointParameterAnalyzer.java

@@ -194,7 +194,7 @@ private void checkInjectionPointMetadata( VariableElement var,
                .getAnnotationsByType(qualifiers);
        boolean hasDefault = model.hasImplicitDefaultQualifier(varElement);
        if (!hasDefault
-                && qualifiersFqns.containsKey(AnnotationUtil.DEFAULT_FQN))
+                && qualifiersFqns.keySet().contains(AnnotationUtil.DEFAULT_FQN))


Unnecessary change.

matthiasblaesing · 2023-04-24T19:33:05Z

enterprise/web.beans/src/org/netbeans/modules/web/beans/impl/model/EnableBeansFilter.java

@@ -471,7 +471,7 @@ else if ( type instanceof WildcardType ){
    private boolean hasModifier ( Element element , Modifier mod){
        Set<Modifier> modifiers = element.getModifiers();
        for (Modifier modifier : modifiers) {
-            if (modifier == mod) {
+            if ( modifier.equals( mod )){


Modifier is an enum, thus identity comparison is valid here. If I'm not mistaken, all changes to web.beans can be rolled back. If there are equal constructs in jakarta.web.beans, it should be modified there also.

enterprise/web.jsf.editor/src/org/netbeans/modules/web/jsf/editor/JsfSupportImpl.java

enterprise/web.jsf/src/org/netbeans/modules/web/jsf/wizards/ManagedBeanPanelVisual.java

enterprise/web.jsf/src/org/netbeans/modules/web/jsf/wizards/PersistenceClientIterator.java

ide/languages.hcl/nbproject/project.properties

java/java.lsp.server/nbcode/nbproject/platform.properties

nbbuild/cluster.properties

juneau001 · 2023-04-24T20:55:04Z

@matthiasblaesing Thanks for the review...it is certainly appreciated! I agree with your comments and I will make the necessary adjustments. I apologize for leaving in the System.out...that was sloppy. I admit that I was working quickly to try and get this into NB18. I appreciate your time and feedback.

juneau001 · 2023-04-25T01:34:55Z

Thank you for the work - it is great, that you are working on this.

When building from source, I noticed, that I can't activate the java cluster or the java ee cluster in the plugin manager, so I could not have a look at the new situation.

For the naming I would suggest to try to keep the directories together, so instead of jakarta.web.beans, I'd suggest web.beans.jakarta.

I noticed that the new module is named identically to the original one, I think some kind of suffix should be added to discern the two. The same might be required for templates and similar stuff.

Please don't take this negatively, the work is appreciated.

@matthiasblaesing I was thinking that it may make sense to keep all of the new jakarta packages together. It makes sense to keep web.beans and web.beans.jakarta together for now, but thinking of the long term does it make sense to keep this new package named jakarta.web.beans in an effort to group jakarta-specific packages together? Thanks

matthiasblaesing · 2023-04-25T18:49:26Z

@matthiasblaesing I was thinking that it may make sense to keep all of the new jakarta packages together. It makes sense to keep web.beans and web.beans.jakarta together for now, but thinking of the long term does it make sense to keep this new package named jakarta.web.beans in an effort to group jakarta-specific packages together?

Understood. My reasoning would be that javamail and jakarta-mail or web.beans and jakarta.web.beans have more in common with each other, than jakarta mail and jakarta.web.beans. On the other hand, this is a side discussion and was just my first impression. If you follow my reasoning great, if not, also ok, I'd like to see jakarta support in NetBeans and that is the important point ;-)

mbien · 2023-04-25T20:01:00Z

as @neilcsmith-net mentioned before. It needs clarification if this PR is meant to get into NB 18 or not.

if not -> change milestone to NB 19 please
if it is intended to get into NB 18, it has to be rebased onto the delivery branch and the target has to be changed to delivery too

I am fairly neutral on this, but since jakarta support is a bit limited atm, I would be ok in taking the risk to integrate a larger PR like this one here even during the RC phase. Usually we would try to integrate only bug fixes during the RC phase. (edit: I mean this is technically a bug fix, but it touches a lot of files)

juneau001 · 2023-04-26T04:00:07Z

I believe I have addressed all of the comments with the exception of renaming from jakarta.web.beans to web.beans.jakarta. In an effort to position the jakarta specific packages together, I am going to leave it as jakarta.web.beans. Also added some commits from @pepness into this PR. Thanks for the contributions.

matthiasblaesing · 2023-04-26T15:46:47Z

I think it would be better to get this into NB19 early and give it bit of time to settle. This is IMHO still work in progress and not ready for merging.

neilcsmith-net · 2023-04-26T16:03:39Z

I tend to agree with Matthias on this, particularly having missed rc2.

matthiasblaesing

@juneau001 thank you for the updates. Much looks good to me now, but I think you overlooked some comments. I marked all comments as resolved, that I think were either addressed or where you explained the decisions.

While looking through the changes I noticed two more things, which I left inline.

Running a build from source with ant tryme gives the the following messages.log:
messages.log.zip. What I notice:

Warnings from ergonomics:

WARNING [org.netbeans.core.projects.cache]: layer jar:file:/home/matthias/src/netbeans/nbbuild/netbeans/ergonomics/modules/org-netbeans-modules-ide-ergonomics.jar!/org/netbeans/modules/ide/ergonomics/enterprise/layer.xml contains duplicate attributes SystemFileSystem.localizingBundle for Templates/CDI/beans.xml

I suspect, that ergonomics pulls all string ids together and this results in these warnings, as the strings are now in two modules.

WARNING [org.openide.filesystems.Ordering]: Found same position 799 for both Services/MIMEResolver/org-netbeans-modules-jakarta-web-beans-BeansDataObject-Namespace.xml and Services/MIMEResolver/org-netbeans-modules-web-beans-BeansDataObject-Namespace.xml

The position of the MimeResolver should be adjusted for the new module, so that the to entries don't occupy the same position.

Caused: java.io.FileNotFoundException: jar:file:/home/matthias/src/netbeans/nbbuild/netbeans/enterprise/modules/org-netbeans-modules-jakarta-web-beans.jar!/org/netbeans/modules/jakarta/web/beans/resources/org-netbeans-modules-jakarta-web-beans-annotations-intercepted.xml
	at org.netbeans.JarClassLoader$NbJarURLConnection.connect(JarClassLoader.java:1187)
	at org.netbeans.JarClassLoader$NbJarURLConnection.getInputStream(JarClassLoader.java:1223)
	at org.netbeans.core.startup.layers.BinaryFS$BFSFile.getInputStream(BinaryFS.java:863)
Caused: java.io.FileNotFoundException: Cannot find 'jar:file:/home/matthias/src/netbeans/nbbuild/netbeans/enterprise/modules/org-netbeans-modules-jakarta-web-beans.jar!/org/netbeans/modules/jakarta/web/beans/resources/org-netbeans-modules-jakarta-web-beans-annotations-intercepted.xml'
	at org.netbeans.core.startup.layers.BinaryFS$BFSFile.getInputStream(BinaryFS.java:866)
	at org.openide.filesystems.MIMESupport$CachedFileObject.getInputStream(MIMESupport.java:377)
[catch] at org.netbeans.modules.openide.filesystems.declmime.DefaultParser.parse(DefaultParser.java:115)
	at org.netbeans.modules.openide.filesystems.declmime.XMLMIMEComponent$SniffingParser.sniff(XMLMIMEComponent.java:269)
	at org.netbeans.modules.openide.filesystems.declmime.XMLMIMEComponent.acceptFileObject(XMLMIMEComponent.java:81)
	at org.netbeans.modu

this looks as if files are missing (this is repeated for several files).

...se/web.jsf.editor/src/org/netbeans/modules/web/jsf/editor/el/WebBeansELVariableResolver.java

java/java.lsp.server/nbcode/nbproject/platform.properties

matthiasblaesing · 2023-05-10T19:32:09Z

@juneau001 I gave the latest version of this PR a spin and noticed warnings and also saw the commit validation test failing here. I had a quick look and indeed the layer file has duplicate entries. This patch fixes this:

warnings.patch.zip

If I'm not mistaken though, this is still not quite correct, as I was in a JavaEE project and asked NetBeans to add a JakarataEE beans.xml, but got a JavaEE one. While that is correct for the project, it is strange to see two template entries then. Anyway, maybe the patch can help to push this further.

juneau001 · 2023-05-11T03:11:02Z

@juneau001 I gave the latest version of this PR a spin and noticed warnings and also saw the commit validation test failing here. I had a quick look and indeed the layer file has duplicate entries. This patch fixes this:

warnings.patch.zip

If I'm not mistaken though, this is still not quite correct, as I was in a JavaEE project and asked NetBeans to add a JakarataEE beans.xml, but got a JavaEE one. While that is correct for the project, it is strange to see two template entries then. Anyway, maybe the patch can help to push this further.

Thanks for the patch...I will apply it. Actually, the fix in this PR does not repair the incorrect beans.xml namespaces. The code changes in this PR repair the code completion issues only. I am working on another PR that will repair XHTML namespaces for Jakarta Faces 4+. I can also look into repairing the beans.xml namespaces.

Closes: apache#4035 Co-authored-by: José Contreras <pepness@apache.org> Co-authored-by: Matthias Bläsing <mblaesing@doppel-helix.eu>

matthiasblaesing · 2023-05-16T20:53:28Z

@pepness @juneau001 I think it would be good to squash this into a single commit. We had the situation in the past where it was necessary to revert a change and having a disconnected set of commits with intermittent merges makes that hard.

Locally I did this:

git fetch github
git checkout -b pr-5866 github/pr/5866
git rebase master
added my own changes
git rebase -i HEAD~9

Marked all commits as "squash", saved and updated the commit message to read

    NETBEANS-4035: CDI Bean as Named is not listed in xhtml for selection
    
    Closes: #4035
    Co-authored-by: José Contreras <pepness@apache.org>
    Co-authored-by: Matthias Bläsing <mblaesing@doppel-helix.eu>

and retained @juneau001 as author of the commit.

The result can be observed here: matthiasblaesing@697d032

What do you think about this?

juneau001 · 2023-05-20T16:09:20Z

@pepness @juneau001 I think it would be good to squash this into a single commit. We had the situation in the past where it was necessary to revert a change and having a disconnected set of commits with intermittent merges makes that hard.

Locally I did this:

git fetch github

git checkout -b pr-5866 github/pr/5866

git rebase master

added my own changes

git rebase -i HEAD~9

Marked all commits as "squash", saved and updated the commit message to read
    NETBEANS-4035: CDI Bean as Named is not listed in xhtml for selection
    
    Closes: #4035
    Co-authored-by: José Contreras <pepness@apache.org>
    Co-authored-by: Matthias Bläsing <mblaesing@doppel-helix.eu>
and retained @juneau001 as author of the commit.

The result can be observed here: matthiasblaesing@697d032

What do you think about this?

Hi @matthiasblaesing have you already performed these steps? Is there anything you need me to do? Everything looks good and I agree with your result. Thanks for your time and assistance.

matthiasblaesing · 2023-05-21T06:40:17Z

Hi @matthiasblaesing have you already performed these steps? Is there anything you need me to do? Everything looks good and I agree with your result. Thanks for your time and assistance.

Yes, the referenced branch was created by this. You allowed edits for this PR, so I could force push the updated branch into the branch that is backing this PR. We would then see whether unittests all run green and if so merge.

Please indicate whether I should do that.

juneau001 · 2023-05-22T11:15:27Z

Hi @matthiasblaesing. Thanks for the response. Yes, please feel free to move forward. I appreciate your time and assistance!

matthiasblaesing · 2023-05-22T17:22:03Z

Tests are green, will merge tomorrow unless someone objects.

IbrahimMSoliman · 2023-05-22T17:31:57Z

Thank you so much for fixing this issue

matthiasblaesing · 2023-05-23T17:11:48Z

@juneau001 @pepness thank you both for working on this!

juneau001 · 2023-05-23T17:14:10Z

@juneau001 @pepness thank you both for working on this!

Thank you both @pepness @matthiasblaesing for your time and assistance!

neilcsmith-net · 2023-05-30T11:22:39Z

Please can you do a double check on module versions and sig files in this, thanks! There were a few conflicts while preparing an updated version of #5856 It looks like at least one module version has gone backwards (web.beans). It should rarely be necessary to touch sig files or module versions between releases.

juneau001 requested a review from pepness April 21, 2023 13:44

mbien added the Java EE/Jakarta EE [ci] enable enterprise job label Apr 21, 2023

juneau001 self-assigned this Apr 22, 2023

juneau001 added this to the NB18 milestone Apr 22, 2023

juneau001 added kind:bug Bug report or fix priority:high High priority issue that should, if possible, be fixed in next release labels Apr 22, 2023

neilcsmith-net added the do not merge Don't merge this PR, it is not ready or just demonstration purposes. label Apr 24, 2023

matthiasblaesing reviewed Apr 24, 2023

View reviewed changes

mbien added ci:all-tests [ci] enable all tests ci:dev-build [ci] produce a dev-build zip artifact (7 days expiration, see link on workflow summary page) labels Apr 25, 2023

neilcsmith-net modified the milestones: NB18, NB19 Apr 26, 2023

matthiasblaesing reviewed Apr 26, 2023

View reviewed changes

...se/web.jsf.editor/src/org/netbeans/modules/web/jsf/editor/el/WebBeansELVariableResolver.java Outdated Show resolved Hide resolved

java/java.lsp.server/nbcode/nbproject/platform.properties Outdated Show resolved Hide resolved

NETBEANS-4035: CDI Bean as Named is not listed in xhtml for selection

697d032

Closes: apache#4035 Co-authored-by: José Contreras <pepness@apache.org> Co-authored-by: Matthias Bläsing <mblaesing@doppel-helix.eu>

matthiasblaesing force-pushed the NETBEANS-4035 branch from b7c0d31 to 697d032 Compare May 22, 2023 15:46

matthiasblaesing removed the do not merge Don't merge this PR, it is not ready or just demonstration purposes. label May 22, 2023

matthiasblaesing merged commit 788ba1f into apache:master May 23, 2023

matthiasblaesing mentioned this pull request May 30, 2023

Fix specification version going backwards in web.beans and jakarta.web.beans #6001

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

NETBEANS-4035: CDI Bean as Named is not listed in xhtml for selection #5866

NETBEANS-4035: CDI Bean as Named is not listed in xhtml for selection #5866

juneau001 commented Apr 21, 2023

neilcsmith-net commented Apr 24, 2023 •

edited

Loading

matthiasblaesing commented Apr 24, 2023

juneau001 commented Apr 24, 2023

pepness commented Apr 24, 2023

matthiasblaesing left a comment

matthiasblaesing Apr 24, 2023

matthiasblaesing Apr 24, 2023

matthiasblaesing Apr 24, 2023

matthiasblaesing Apr 24, 2023

juneau001 commented Apr 24, 2023

juneau001 commented Apr 25, 2023

matthiasblaesing commented Apr 25, 2023

mbien commented Apr 25, 2023 •

edited

Loading

juneau001 commented Apr 26, 2023

matthiasblaesing commented Apr 26, 2023

neilcsmith-net commented Apr 26, 2023

matthiasblaesing left a comment

matthiasblaesing commented May 10, 2023

juneau001 commented May 11, 2023

matthiasblaesing commented May 16, 2023

juneau001 commented May 20, 2023

matthiasblaesing commented May 21, 2023

juneau001 commented May 22, 2023

matthiasblaesing commented May 22, 2023

IbrahimMSoliman commented May 22, 2023

matthiasblaesing commented May 23, 2023

juneau001 commented May 23, 2023

neilcsmith-net commented May 30, 2023

NETBEANS-4035: CDI Bean as Named is not listed in xhtml for selection #5866

NETBEANS-4035: CDI Bean as Named is not listed in xhtml for selection #5866

Conversation

juneau001 commented Apr 21, 2023

neilcsmith-net commented Apr 24, 2023 • edited Loading

matthiasblaesing commented Apr 24, 2023

juneau001 commented Apr 24, 2023

pepness commented Apr 24, 2023

matthiasblaesing left a comment

Choose a reason for hiding this comment

matthiasblaesing Apr 24, 2023

Choose a reason for hiding this comment

matthiasblaesing Apr 24, 2023

Choose a reason for hiding this comment

matthiasblaesing Apr 24, 2023

Choose a reason for hiding this comment

matthiasblaesing Apr 24, 2023

Choose a reason for hiding this comment

juneau001 commented Apr 24, 2023

juneau001 commented Apr 25, 2023

matthiasblaesing commented Apr 25, 2023

mbien commented Apr 25, 2023 • edited Loading

juneau001 commented Apr 26, 2023

matthiasblaesing commented Apr 26, 2023

neilcsmith-net commented Apr 26, 2023

matthiasblaesing left a comment

Choose a reason for hiding this comment

matthiasblaesing commented May 10, 2023

juneau001 commented May 11, 2023

matthiasblaesing commented May 16, 2023

juneau001 commented May 20, 2023

matthiasblaesing commented May 21, 2023

juneau001 commented May 22, 2023

matthiasblaesing commented May 22, 2023

IbrahimMSoliman commented May 22, 2023

matthiasblaesing commented May 23, 2023

juneau001 commented May 23, 2023

neilcsmith-net commented May 30, 2023

neilcsmith-net commented Apr 24, 2023 •

edited

Loading

mbien commented Apr 25, 2023 •

edited

Loading