Bug 51588 - Change access modifiers in AccessLogValve to make it easier to extend
Summary: Change access modifiers in AccessLogValve to make it easier to extend
Status: RESOLVED FIXED
Alias: None
Product: Tomcat 6
Classification: Unclassified
Component: Catalina (show other bugs)
Version: 6.0.32
Hardware: PC Linux
: P2 minor (vote)
Target Milestone: default
Assignee: Tomcat Developers Mailing List
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-07-30 02:33 UTC by Rod
Modified: 2011-08-11 09:17 UTC (History)
0 users



Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Rod 2011-07-30 02:33:45 UTC
I'm adding some fields to a subclass of org.apache.catalina.valves.AccessLogValve but createLogElements() is the closest extension point available. Rather than reimplement the functionality there it would be preferable if both of the createAccessLogElement() methods were protected rather than private then it's a simple method of matching the char you want to use and falling back to super.createAccessLogElement().
Comment 1 Konstantin Kolinko 2011-07-30 12:18:47 UTC
Are those new fields specific for your environment? Maybe you can propose them to be added to AccessLogValve?
Comment 2 Rod 2011-07-30 12:49:30 UTC
I want to add a %f and %F as possible options in the custom log pattern to log IP address and name safely extracted from the X-Forwarded-For header with a fallback to HttpServletRequest.getRemoteAddr().

Just to back up and clarify my original bug report in case it wasn't clear (now that I re-read it I think I could have been clearer):

At the bottom of org.apache.catalina.valves.AccessLogValve, the last two private methods are:

private AccessLogElement createAccessLogElement(char pattern)

and

private AccessLogElement createAccessLogElement(String header, char pattern)

If you want to add another 'field' to the custom log format pattern by subclassing AccessLogValve then you have to go up to the 3rd last method:

protected AccessLogElement[] createLogElements()

but unfortunately it contains a lot of good logic that would have to be either completely copied or duplicated in some way. Overriding one or both of the two private methods lets you simply plug in your addition and not replace any of the functionality of the original AccessLogValve.

This is the same for both the version 6 and 7 sources.

So my suggestions is simply to replace 'private' with 'protected' in the last 2 methods in AccessLogValve.

Of course I'd love the new options to be added to the Catalina core but I suspected that they may be slightly peripheral? Perhaps not. I've just finished writing up the details for anyone else that may have a use for this functionality at: http://rod.vagg.org/2011/07/handling-x-forwarded-for-in-java-and-tomcat/

If you think this would be useful to integrate into Catalina then I'd be more than happy to refactor it all into AccessLogValve (it uses an additional utility class as it is now) and post a patch here; please let me know what you think about that option.

I think changing those access modifiers would be useful in either case by the way.
Comment 3 Konstantin Kolinko 2011-07-30 13:41:54 UTC
Note, that

1. Parsing of X-Forwarded-For is already implemented in Tomcat 7 in RemoteIpValve and RemoteIpFilter.  RemoteIpValve was backported to Tomcat 6, though maybe not all features of it.

2. AccessLogValve in Tomcat 7 can use request attributes that are set by RemoteIpValve if you set AccessLogValve.requestAttributesEnabled property to true.

3. You can set any attribute on the request object (even in the Valve itself) and reference it later as %{xxx}r
Comment 4 Mark Thomas 2011-07-31 17:41:17 UTC
Methods changed in 7.0.x and will be included in 7.0.20 onwards.

Same change proposed for 6.0.x.

Even if Tomcat does support this particular use case, the general one of extensibility is a valid one.
Comment 5 Mark Thomas 2011-08-11 09:17:36 UTC
This has been fixed in 6.0.x and will be included in 6.0.33 onwards.