Remove jetty dependencies in jetty jasper classes

[janbartel]

Remove jetty dependencies from: JettyJasperInitializer and JettyJspServlet

[janbartel]

Easy to remove the jetty log dependency from JettyJasperInitializer.

A bit harder to remove the Resource dependency in JettyJspServlet: the purpose of the JettyJspServlet is to handle the case where web.xml contains a jsp-property-group url mapping (that always map to the jsp servlet) that is too general, and would also apply to directories. The JettyJspServlet tests the resource that is the subject of the request to see if it is actually a directory: if it is, then it redirects the request to the DefaultServlet. When using our Resource class to test for being a directory, it correctly handles dirs inside packed jars (eg inside META-INF/resources/xxx). However, not using the Resource class means we must use the api method ServletContext.getRealPath() instead, which doesn't deal with packed jars. So in the specific circumstance of a combination of: a) a too-generous jsp-property-group mapping and b) a static resource dir inside a WEB-INF/lib jar, we would not return the static resource correctly.

Perhaps I'm being too cautious: in testing it's hard to define a too-generous jsp-property-group pattern that isn't ignored in favour of the '/' mapping to DefaultServlet (ie '/' is considered a more specific match than something like '/*').

Even if this does cause problems, there's no reason that a user shouldn't be able to come up with a better series of jsp-property-group url patterns that specifically match only jsps.

[ludoch]

Thanks you for the change (I am tracking it), it is great. Will you also address org.eclipse.jetty.apache.jsp.JuliLog or it is something we need to provide our own implementation?

[janbartel]

@ludoch I think we need to leave the o.e.j.a.j.JuliLog in place in this module: as its whole purpose is to bridge jetty logging to jasper logging it needs jetty classes, and I think its reasonable to expose it in the META-INF/services of our apache-jsp jar as the logging mechanism to pick up. If you can just remove the META-INF/services file that points to our JuliLog and replace it with your own impl of jasper logging, I think that would be best.

Let me know if that is not possible and I'll contemplate another way around this.

[ludoch]

I've already done a proto of overwriting these 3 files and the services definition. Now, what you are proposing is a mix of using your jar, and my single override. Not sure how to do that (I guess I need to control the order of the jars in the class path, which may be tricky?) Or could you try first to see if the jetty log classes are available and if not, put a debug message, but default to a simpler solution (like the one I sent by email?) Or if there is an easy way for an application to provide the service override, I am all for it. Thanks!

[janbartel]

@ludoch mmmmm .... I think it is a bit weird for the jetty logging bridge to not expect the jetty classes to be available and to fall back to java util logging. Consider that the juli mechanism, if it doesn't find a log impl will default to direct java util logging anyway.

So it seems cleaner to me that we would take the jetty Juli bridge class out of the apache-jsp module and put it into another module - say apache-juli - and that way you would have the option of deploying it or not: if you don't deploy it, then java util logging will ultimately be used. So I'll investigate what it would impact it would have on the rest of jetty runtime if I split the JuliLog class out into its own module and get back to you.

[janbartel]

@ludoch ok so I think for jetty-9.3 we'd have to go with your solution (check for jetty log class on classpath, default to java util logging if not) as I can't make a big change in a stable release series.

But for jetty-9.4, I'll go with my suggested solution: make a new apache-juli module and put the log class in there, so if you don't want to use it you can not deploy it.

[janbartel]

@ludoch urrrggggh, the reflection is horrible. @gregw suggested a better way, which is how to achieve my preferred solution in jetty-9.4 via a different means: we make an additional, variant apache-jsp maven artifact that simply does not have the META-INF/services file for juli logging. The existing apache-jsp artifact is unaffected, and you would select the new artifact via a maven classifier ("nolog" perhaps?).

Can you consume a jar with a classifier like that in your environment?

[janbartel]

Committed to jetty-9.3 and will be merged upwards.

The new artifact has a classifier of "nolog" eg apache-jsp-9.3.x-nolog.jar

[ludoch]

I guess the 2 jars will be in the uber zip distro? Under the apache-jsp/ directory? or the no log elsewhere?

[janbartel]

They will be alongside the other apache-jsp artifacts (ie those with classifier -source, -javadoc, -config etc) in the usual place in the central maven repository, eg:

/org/eclipse/jetty/apache-jsp/9.4.0.RC2/apache-jsp-9.4.0.RC2-nolog.jar

You should be able to access them in a maven build with:

<dependency>
  <groupId>org.eclipse.jetty</groupId>
  <artifactId>apache-jsp</artifactId>
  <version>9.4.0.RC2</version>
  <classifier>nolog</classifier>
</dependency>

[ludoch]

LGTM

On Wed, Oct 26, 2016 at 4:41 PM, Jan Bartel notifications@github.com wrote:

Closed #1032 https://github.com/eclipse/jetty.project/issues/1032.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/eclipse/jetty.project/issues/1032#event-837881447, or mute the thread https://github.com/notifications/unsubscribe-auth/AAE4zcx3CkiUinbzibcnCtDC5ZjzQdZMks5q3-VDgaJpZM4Kgv9u .

[gregw]

Jan, how about having a JuliLog implementation that routes to ServletContext.log? That will be portable then. If we wanted to support debug/warn/etc we could specially encode the string which can optionally be decoded and routed to the right log stream by the container.

On 27 October 2016 at 07:54, Jan Bartel notifications@github.com wrote:

@ludoch https://github.com/ludoch I think we need to leave the o.e.j.a.j.JuliLog in place in this module: as its whole purpose is to bridge jetty logging to jasper logging it needs jetty classes, and I think its reasonable to expose it in the META-INF/services of our apache-jsp jar as the logging mechanism to pick up. If you can just remove the META-INF/services file that points to our JuliLog and replace it with your own impl of jasper logging, I think that would be best.

Let me know if that is not possible and I'll contemplate another way around this.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/eclipse/jetty.project/issues/1032#issuecomment-256473885, or mute the thread https://github.com/notifications/unsubscribe-auth/AAEUrWk1RLh0y2l0jodfWBKrK4BPHeFRks5q3735gaJpZM4Kgv9u .

Greg Wilkins gregw@webtide.com CTO http://webtide.com

[ludoch]

Still wondering if this jar will be in the uber distro zip. The module is defined as: #

Apache JSP Module

#

[name] apache-jsp

[lib] lib/apache-jsp/*.jar

so if the 2 jars are in lib/apache-jsp it might be some duplicated classes (not a big deal)

[janbartel]

Oh, I see what you mean, I thought you meant the aggregate jars that we build.

No, I wasn't planning on having it in the distro at all, just available from maven. Is that a problem?

[ludoch]

No, just need to update my internal docs

[janbartel]

Cool, I'll close this again then.

[ludoch]

No plan for 9.4 to investigate what Greg mentioned about having a JuliLog implementation that routes to ServletContext.log ?

[janbartel]

No, I think that would be a messier solution: too much parsing of strings to replicate what a logger with levels would do.

Is that a problem?