OpenLiberty / open-liberty

Open Liberty is a highly composable, fast to start, dynamic application server runtime environment
https://openliberty.io
Eclipse Public License 2.0
1.15k stars 592 forks source link

Currently WebSphereCDIExtension is used by the faces-3.0 bnd.bnd investigate using CDIExtensionMetadata #13933

Closed pnicolucci closed 3 years ago

pnicolucci commented 4 years ago

We've used the WebSphereCDIExtension historically for JSF and we need to use it or something similar for faces-3.0.

We've currently ported the existing use of com.ibm.ws.jsf.shared.ext.CdiExtension for faces-3.0 but we should move away from it since it is now deprecated in favor of CDIExtensionMetadata.

volosied commented 3 years ago

It seems that will be on hold until bean.defining.annotation is supported by CDIExtensionMetadata SPI.

Please see this comment here: https://github.com/OpenLiberty/open-liberty/blob/cf2d72ed7d00834d8adbe81272057613b1e11663/dev/io.openliberty.org.jboss.resteasy.server/src/io/openliberty/org/jboss/resteasy/common/component/ResteasyCDIExtensionMetadata.java#L39-L44

We use this annotation in our faces bnd: https://github.com/OpenLiberty/open-liberty/blob/822ce094ec1377112a2a4a803d25b452cb34e79b/dev/io.openliberty.org.apache.myfaces.3.0/bnd.bnd#L41

We'll speak with the CDI team once everyone is back from the holidays.

isaacrivriv commented 3 years ago

After some discussion with the CDI team, the feature will be implemented and is being tracked here. Once that is implemented and merged we can give it a try to switch to the extension.

isaacrivriv commented 3 years ago

The SPI was designed to be made simple and some functionality originally added in the deprecated extension was purposefully left out. We worked with the CDI team to migrate to the new SPI but necessary functionality for us to move was not included in the new SPI. We tried a couple of workarounds but couldn't move to the extension. We were missing the property extension.classes.only for our move to work. Discussing with the CDI team, there are no plans for property to be added to the SPI and our issue is an edge case. The plan for the edge cases would be to stick with the deprecated extension which we are already doing.

volosied commented 3 years ago

Since new extension is not the same as the old one, we encounter Bean resolution problems where JSF/CDI cannot find the bean used within the application when evaluating EL expressions -- identifier [bean] resolved to null. In short, the wrong bean manger (contained in the WeldELResolver) is passed to JSF. See below for more details.


For future reference -- why extension.classes.only matters.

When true, I discovered that the WeldELResolver passed into Liberty has an incorrect BeanManger. So when JSF does the bean lookup, it can’t find the application bean and null is returned. Working backwards from there I learned how extClassesOnly affects the classes within the BeanDeploymentArchiveImpl when the new SPI Extension is used.

One solution is just to use cdiService.getCurrentModuleBeanManager() within CDIJSFInitializerImpl instead since that’s what’s eventually used in the working case and it gets the JSF bucket passing again. I bought this up with the CDI team and they still recommended sticking with the old extension for performance reasons (related -- see defect 178315). The number of classes stored in BDA (1400+ compared to just < 30) is just quite a lot.