On Mon, Jun 17, 2019 at 10:52 AM Romain Manni-Bucau <rmannibu...@gmail.com>
wrote:
> Hi Rémy,
>
> Great progression! Congrats!
>
> I have a few (details) notes - guess i'm opening an open door but just to
> ensure:
>
> 1. Is it intended to bind cxf to /rest/* in any case? I tend to see it
> bound on on /api but generally thanks to an Application
> (+ @ApplicationPath("api")) so cxf is bound on /*. Wonder if the listener
> shouldn't host that config and we would link it through a context listener
> or servletcontextinitializer (both will work and rely on tomcat internals)
>
I am not comfortable mapping a servlet on /* by default, it would be
annoying, so I changed the mapping to /api/*. I used a web-fragment for now
since it's a decent Servlet API feature and has plenty of configuration
flexibility.
Adding additional CXF integration would be good but is work, a filter could
useful. Moving CXF to a container integration is even more work and doesn't
looks that practical to me.
The CDI context listener configuring CXF if is present sounds possible (it
can programmatically add the CDI CXF servlet, no problem), it does sound a
bit weird conceptually but it's a good option.
> 2. In beans.xml, rather than annotated mode - which skips a lot of classes
> which can be used in CDI - we tend to prefer discovery mode "full" + trim:
> https://github.com/apache/geronimo-health/blob/master/geronimo-health/src/main/resources/META-INF/beans.xml
>
I don't understand how that works, if I add scanning to tomcat-cxf.jar, it
loads all classes (which fails). So that's why I'm not doing it.
> 3. Why not merging SPI files thanks to maven? (shade part, end of
> http://openwebbeans.apache.org/meecrowave/meecrowave-maven/index.html)
> 4. You have a spring config (cxf.xml), not sure it is intended?
> 5. jettison is not needed since it is not the one used for json
> serialization (guess you can drop several cxf deps)
>
Very good idea, I made changes and fixed these items.
Rémy
