+1 to merge (replace) it in owb module since tomcat adherance is reduced thanks your refactos.
The few weird things I saw - but guess it is inherited from our owb module and cdi 1 times are: 1: dont start cdi if there is no beans.xml in webinf (in cdi 2 it should be anyway) 2: security service is thread local based instead of using cdi request bean and it does not support enriching principal api (needed for mp jwt auth for instance). https://github.com/apache/meecrowave/blob/trunk/meecrowave-core/src/main/java/org/apache/meecrowave/openwebbeans/MeecrowaveSecurityService.java does that and should work in tomcat. Except that it looks good to me. Side question: any interest to try to make meecrowave converging too? I can envision a v2 with a nicer tomcat embedded api used as raw base of the project and all other features being plugged on top of it but not sure it makes sense for tomcat community. Every feature would just be initializers (with @priority support if possible to be fully deterministic and user friendly). Le mar. 18 juin 2019 à 22:07, Rémy Maucherat <r...@apache.org> a écrit : > Hi, > > I have made some improvements and refactorings to the Tomcat OWB > integration. The code is available here: > https://github.com/rmaucher/tomcat-owb > > The changes are roughly: > - I expected this would be a Tomcat external module (like JDBC pool), so > the code was changed to have the Tomcat code style, i18n, package rename > (it can be reverted), refactorings, etc. > - Works as a listener on the Server element, which would CDI enable the > container. The listener can be a good location for configuration in the > future. Various listener event timing issues prevented the tomcat7 > integration from working this way. It still has a listener at the context > level, so the integration can be used at the individual webapp level as > well. > - Requires Tomcat 9.0.21+ due to the said event changes plus some utility > class to avoid code duplication. > - Using this integration code, the pom builds a semi fat jar which can be > put in the Tomcat lib folder, then the listener should be added to > server.xml. I have verified CDI extension support is functional. > - I have tested that CXF (with the right packaging), and the Geronimo MP > impl JARs (without needing a "right" packaging ;) ) work well with this OWB > support using said CDI extension support. > > The shade packaging pom would be somewhere in Tomcat (like the packaging > I'm using for container and graal test bed: > https://github.com/apache/tomcat/tree/master/res/tomcat-maven ), but the > code needs to be somewhere. As I said I planned to host it at > https://github.com/apache/tomcat/tree/master/modules but, unexpectedly, > OWB > community members said it should be in OWB. > > If the code is picked up in OWB, I'd recommend: > - Package renaming back to org.apache.webbeans.web.tomcat (I'm not a fan of > "tomcat9", it ends up looking bad and/or outdated in the configuration, > like "tomcat7" does these days). > - Keeping "OpenWebBeans" in class names. Although it's not the usual > convention here, the classnames do appear in Tomcat configuration, its > monitoring with JMX, and it's more obvious this way. > "org.apache.webbeans.web.tomcat9.TomcatListener" in server.xml works I > guess, but "org.apache.webbeans.web.tomcat.OpenWebBeansListener" looks > better. Overall, only OpenWebBeansPlugin and OpenWebBeansSecurityService > could lose the "OpenWebBeans" name in favor of "Tomcat" (or whatever name). > - Keep the Tomcat i18n since it doesn't hurt. > - No idea about the code formatting, I'm ok if it is changed back. > > Rémy >
