Mailing-List: contact dev-help@tomcat.apache.org; run by ezmlm
Precedence: bulk
Reply-To: "Tomcat Developers List" <dev@tomcat.apache.org>
Received-SPF: softfail (athena.apache.org: transitioning domain of
 brian@pingtoo.com does not designate 217.154.193.216 as permitted sender)
Message-ID: <5068CBAF.30306@pingtoo.com>
Date: Sun, 30 Sep 2012 23:46:07 +0100
From: Brian Burch <brian@pingtoo.com>
User-Agent: Mozilla/5.0 (X11; Linux i686;
 rv:15.0) Gecko/20120912 Thunderbird/15.0.1
MIME-Version: 1.0
To: Tomcat Developers List <dev@tomcat.apache.org>
Subject: Re: FormAuthenticatorTest for cases without cookies - implementation
 issues
References: <50630AAA.60700@pingtoo.com>
 <CABzHfV=M-VSX4kFr13iSSGfxcegXPuEyw2fQv9N18=53tiWXpw@mail.gmail.com>
In-Reply-To: 
 <CABzHfV=M-VSX4kFr13iSSGfxcegXPuEyw2fQv9N18=53tiWXpw@mail.gmail.com>
Content-Type: text/plain; charset=ISO-8859-1; format=flowed
Content-Transfer-Encoding: 7bit

On 30/09/12 21:56, Konstantin Kolinko wrote:
> 2012/9/26 Brian Burch <brian@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.

Thanks for thinking about my worries, Konstantin. I appreciate you 
spending time analysing what must appear to be a peripheral issue.

In fact, when I didn't receive a quick reaction, I started to think 
along the same lines as you. At the moment, I am working on refactoring, 
so that I can submit (a) entirely on its own, without breaking the 
existing tests, but then being able to enable (b) as the next change.

> 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.

Yes, that is the philosophy of the current test suite. It checks the 
expected behaviour of the tomcat example jsps, but in a manner that 
makes the tests appear brittle to an outsider such as me.

I suppose I like to write tests that are a) extensible to explore edge 
cases (I think there may be some not yet covered), and b) are robust 
enough to handle external cosmetic changes to the unimportant aspects of 
the test environment. e.g. if someone were to slightly alter the page 
title of /examples/jsp/security/protected/index.jsp or login.jsp, many 
of the FormAuthenticator tests would break... I don't feel comfortable 
ignoring that possibility.

> 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().

Yes, but the existing test suite is closely crafted around HttpClient 
and has been in place (at least) since tomcat6. I assume the original 
author wanted "exact control over transmitted data", but I haven't 
worked my way through the other the test cases and so I don't know for sure.

I saw no reason to think about throwing out the existing methodology and 
prefer to try making it more flexible and robust (within reason).

Once again, thanks for your valuable opinions. I hope to submit the 
first patches later this week. This discussion has provided useful 
background which should make the code review much simpler for you.

Brian

> Best regards,
> Konstantin Kolinko
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
> For additional commands, e-mail: dev-help@tomcat.apache.org
>


---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org