On 10/03/2014 15:00, Rainer Jung wrote: > Here's a patch: > > http://people.apache.org/~rjung/patches/TC8-SessionIdGenerator-extensible.patch > > Any concerns about applying to TC 8? Please note "Known problems". > > Some notes: > > About the patch: > > 1) New interface org.apache.catalina.SessionIdGenerator > - setter and getter for jvmRoute > - setter and getter for session id length > - generateSessionId() (use jvmRoute if set) and > generateSessionId(String route). > 2) Renamed org.apache.catalina.util.SessionIdGenerator to > org.apache.catalina.util.SessionIdGeneratorBase. > Not strictly needed but for consistency with other similar cases.
Is it consistent? I'd expect a class with a name that ends in Base to be abstract rather than concrete. I'd expect the default implementation to be named something like DefaultSessionIdGenerator. Other than that +1 Mark > 3) Changed message keys in > org.apache.catalina.util.LocalStrings.properties > etc. from sessionIdGenerator.* to sessionIdGeneratorBase.*. > 4) Let org.apache.catalina.util.SessionIdGeneratorBase > implement org.apache.catalina.SessionIdGenerator. > 5) Adjust visibility of getRandomBytes(byte bytes[]) in base > class from private to protected to allow extension. > 6) Add getter and setter for SessionIdGenerator to Manager interface > 7) Adjust SessionIdGenerator initialization in ManagerBase: > uses generator set in context config if present, otherwise > uses SessionIdGeneratorBase as default. > 8) Implement SessionIdGenerator getter and setter in ManagerBase. > 9) Add Context/Manager/SessionIdGenerator to digester ContextRuleSet. > > Known problems: > > - Docs missing (will add to configuration reference) > > - Unrelated to the patch: Semantics of session id length is unclear. > Implemented as number of random bytes that get encoded into the id, > but documented as actual session id length (which is not true > currently and would after the patch also depend on the actual > encoding used by the id generator, e.g. hex or other, > added routing tags etc.). > I'd opt for keeping current semantics and only cleaning up docs. > > - Session id length can not be set on the SessionIdGenerator level: > For compatibility the Manager has a default of 16 and forwards that > or whatever was explicitly configured for it to the > SessionIdGenerator during its own ManagerBase.startInternal(). > That overwrites what the digester might have already set on the > SessionIdGenerator directly. > > Possible workaround 1: only call setSessionIdLength() from Manager to > SessionIdGenerator if setSessionIdLength() was explicitely called > on the Manager itself and getSessionIdLength() does return <=0 > (no sensible default or explicit setting on SessionIdGenerator) > > Possible workaround 2: remove setSessionIdLength() and > getSessionIdLength() completely from Manager. > I'd opt for this one for TC 8. Obviously not for a possible backport. > I'd suggest workaround 1 for a possible backport. > > Possible Backport (TC 7, can be decided later): > > - new interface SessionIdGenerator allowed? Should be OK. Nobody > needs to implement it unless she wants to replace the generator. > > - skip renaming of SessionIdGenerator to SessionIdGeneratorBase. > Fewer changes, lower chances of incompatibility. > > - addition of SessionIdGenerator setter and getter to Manager > interface probably not allowed? Backported code should still > work except digester should throw an exception if the Manager impl > doesn't have a setSessionIdGenerator() method. Add that method > to ManagerBase so most Manager impls will automatically support > replacing the SessionIdGenerator. > > - different workaround 1 for the setSessionIdLength() problem. > > Regards, > > Rainer > > --------------------------------------------------------------------- > To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org > For additional commands, e-mail: dev-h...@tomcat.apache.org > --------------------------------------------------------------------- To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
