On 17.11.2012 15:35, Mark Thomas wrote: > On 17/11/2012 13:05, Rainer Jung wrote: >> Hi, >> >> I did some experiments about more flexible version mapping for contextes >> a while back and was a bit unhappy about the type unsafetyness of the >> Mapper. Internally it only uses plain Objects and the code using it has >> to cast everything. >> >> There is a patch available at >> >> http://people.apache.org/~rjung/patches/mapper-typesafe-trunk.patch > > There is a lot to take in there. I am generally in agreement but find > myself rather overwhelmed by the patch. May I suggest you go ahead with > just the renaming of Host->MappedHost etc and commit that to trunk > (should be simple to do in an IDE) and then prepare a patch that makes > the mapper type safe.
Renaming committed as r1410752. Remaining patch (half the size, biggest part in TestMapper, plus the generics in MapElement): http://people.apache.org/~rjung/patches/mapper-typesafe-trunk-v2.patch > If it would be easier for me to make the name changes and then you could > just svn up I'm happy to do so. Let me know. Thanks, it was easy for me as well :) >> One change is potentially problematic: MapperListener previously >> registered a new context using the Container returned by >> context.getParent(), ignoring whether it is a Host or something else. I >> had to cast to Host, because registering a Context for any type of >> Container is no longer possible. I doubt that previously it would have >> really worked, because all code using the container returned later by >> the mapper would have assumed it to be a Host. >> >> The Context interface mentions in comments, that the parent might be >> something else than a Host. Does anyone know a good example? > > I can't think of a single example. > >> Would it have worked? > > No. I see multiple places where the result would be an NPE if > Context->Host->Engine is not in place. > >> What would the expected behaviour of MapperListener have >> been (where would it have registered that context in the mapper)? > > I doubt the start up processes would have got that far. OK > I suspect that the initial design allowed for a different container > hierarchy but over time the Wrapper->Context->Host->Engine hierarchy has > become embedded in numerous places in the code. I see no reason to > unpick that so lets make it explicit and override getParent() for > Wrapper, Context and Host for Tomcat 8. It will enable us to remove lots > of casting and we may even find a few obscure bugs. Good, so that could be a next step after the remaining mapper type-safety changes. Regards, Rainer --------------------------------------------------------------------- To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
