2012/9/26 Brian Burch <br...@pingtoo.com>: > Thanks for all the help while I was developing the new test case for > https://issues.apache.org/bugzilla/show_bug.cgi?id=53584. The thread on the > users mailing list is called "AuthenticatorBase > setChangeSessionIdOnAuthentication without cookies". > > I now have two new test cases working successfully against a > recently-updated trunk. I hope to use them in future as boilerplates, to > expand the set of tests, and also to examine SSO behaviour. > > Before I open a bugzilla enhancement request and submit my patch files, I > would like to discuss my implementation decisions in general, to ensure that > my effort, and other developer's time, isn't wasted. > > I found it necessary to modify both > org.apache.catalina.authenticator.FormAuthenticatorTest, and its parent > class org.apache.catalina.startup.SimpleHttpClient. > > To save you looking it up, here is the unchanged class comment to > SimpleHttpClient: > > /** > * Simple client for unit testing. It isn't robust, it isn't secure and > * should not be used as the basis for production code. Its only purpose > * is to do the bare minimum for the unit tests. > */ > > FormAuthenticatorTest doesn't have a class-level comment at all, but I have > written a new one based on the conclusions in my thread on the users list. > > I would have preferred to make fairly localised changes to both of these > classes, but the existing logic seems to incorporate some fundamental > assumptions that made my intention too difficult to implement. > > I am not at all proud of my code, but I believe it a) works, b) is fairly > harmonious with the existing design, and c) is amenable to the extensions I > intend to add in due course. > > Firstly, I've written several small blocks of parser logic for urls and http > headers, which are specific to the test environment and not very pretty. I > looked for suitable parsers to re-use in the various tomcat utils packages, > but haven't found them yet, even though tomcat MUST be doing similar work in > an elegant and robust manner. I haven't found any unit tests, either. > > Then, I looked at the apache HttpClient project. An ideal solution would > have been to use the parsers from that project, or perhaps even the entire > client. This would have required starting with a blank sheet of paper, and I > am very reluctant to take that approach. I might have been swayed if I had > found HttpClient already in use by other tomcat unit tests, but I couldn't > find it in the dependencies and didn't want to add a new one. > > Next, the current version only supports cookies, so I had to add some extra > boolean arguments to control the use of server and client cookies in each > test case. In my professional work, I would have use singleton inner classes > to achieve type-safety and make these crucial arguments self-documenting, > but this wouldn't be compatible with the existing style of the various > current authenticator unit test classes. Also, I wanted to make my initial > change as transparent as possible, so it could be reviewed (and accepted) > with as little effort from others as possible. > > I didn't want to touch SimpleHttpClient at all, but that turned out to be > unavoidable. This class does most of the http header processing, and so it > seemed ugly to split this work between the two classes. On the other hand, > all the url handling is done by FormAuthenticatorTest, so it felt ugly to > start doing any of that work in SimpleHttpClient. The consequence is that > the two classes need to be cross-wired to some extent. This was always the > case, but I had to cross some more wires for the new test cases. > > So, to summarise: I would like to make quite a bulky change to these two > classes. I am somewhat embarrassed by my ugly style and DIY approach to > parsing. Many of the line-changes in the patch are trivial, but a lot of > lines will be hit at once. I haven't gone mad with comments, but have tried > to add useful tips when an existing section of uncommented code didn't make > sense to me. On the other hand, to maintain similar look'n'feel, I haven't > been heavy-handed with comments in my new code. Of course, I will make sure > it passes checkstyle before I actually cut the patches! > > To put things in perspective, the tests only demonstrate that Mark's fix > works - and that wasn't even in question. However, I'd like to get my change > committed soon, so that I can move forward with additional test cases. > > What do you think? Should I publish and be damned, or would you like me to > do more work to eliminate some of my compromises? >
1. If you a set of changes (a, b, c) and you cannot separate them into distinct patches, I would prefer to see (a), (a+b) and (a+b+c). Seeing (a+b+c) is usually also good enough if you can explain the changes such that a committer would be able to separate them. 2. Why do you need sophisticated header parsing in SimpleHttpClient? The server side for this client is always Tomcat, so its responses are well known. Usually it is sufficient to check them for exact expected values. 3. SimpleHttpClient is used when we need exact control over transmitted data. In simpler cases we just use an URLConnection. See TomcatBaseTest methods getUrl(), postUrl(), methodUrl(). Best regards, Konstantin Kolinko --------------------------------------------------------------------- To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
