On Wed, 19 Jun 2019 at 10:37, Romain Manni-Bucau <rmannibu...@gmail.com> wrote:
> Le mer. 19 juin 2019 à 11:16, Rémy Maucherat <r...@apache.org> a écrit : > > > On Tue, Jun 18, 2019 at 10:26 PM Romain Manni-Bucau < > rmannibu...@gmail.com > > > > > wrote: > > > > > +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) > > > > > > > I don't see the bug there. The tomcat7 code seems to do that and it > appears > > to work for me (no CDI logging without a /WEB-INF/beans.xml): > > > > > https://github.com/apache/openwebbeans/blob/trunk/webbeans-tomcat7/src/main/java/org/apache/webbeans/web/tomcat7/ContextLifecycleListener.java#L63 > > > > > This is the bug, CDI must start without a beans.xml (annotated mode is > implicit) > > Ahh I'll need to remove that bug from the jetty module too.... I just copied it from Tomcat! > > > > > > 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. > > > > > > > Ok, I imported this class. > > > > > > > > > > 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). > > > > > > > After having some problems for a while with some uses of embedding, I > made > > some changes to embedded to make it more flexible about processing > > server.xml and other configuration files (context.xml, default web.xml, > > certificates, etc). This is to allow avoiding reinventing server.xml when > > using embedded (as SSL configuration in particular became more complex > it's > > not a very good plan). I did not add any new fancier embedded API, and > Mark > > seems to be happy enough with the current one as well. > > > > Fair enough. > > > > > > Rémy > > > > 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 > > > > > > > > > >
