You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@tomcat.apache.org by Brian Burch <br...@pingtoo.com> on 2013/01/04 19:52:31 UTC
Seeking guidance on use of apache commons classes
On 29/11/12 16:11, Brian Burch wrote:
> On 29/11/12 14:37, bugzilla@apache.org wrote:
>> https://issues.apache.org/bugzilla/show_bug.cgi?id=54190
Re: [Bug 54190] TestNonLoginAndBasicAuthenticator does not test session
timeout properly
>>
>> Mark Thomas <ma...@apache.org> changed:
>>
>> What |Removed |Added
>> ----------------------------------------------------------------------------
>>
>> Status|NEW |RESOLVED
>> Resolution|--- |FIXED
>>
>> --- Comment #6 from Mark Thomas <ma...@apache.org> ---
<snip/>
>
>> If you start looking at the TODOs, I suggest you take a look at
>> org.apache.tomcat.util.http.parser.HttpParser#parseAuthorizationDigest()
>>
>> I suspect a new parseAuthorizationBasic() method is the way to go as that
>> should handle the various whitespace issues noted.
>
> Noted. Assume that I will look into it unless you hear otherwise. I
> won't open a bug against BasicAuthenticator yet.
My motive is the original thread above, but my question has a wider
scope. I have started a new thread, even if it doesn't run far, just to
make it easier for others to find.
I started looking at Mark's suggestion and got a long way with the change.
Currently, BasicAuthenticator.authenticate uses
org.apache.catalina.util.Base64.decode(ByteChunk,CharChunk).
I decided to follow the pattern used by DigestAuthenticator and create a
new method
org.apache.tomcat.util.http.parser.HttpParser.parseAuthorizationBasic(StringReader).
Once I got there, I discovered that org.apache.catalina.util.Base64
doesn't currently have any methods to decode a StringReader or String
object.
It didn't take me long to find
org.apache.commons.codec.binary.Base64.Base64, which has all the right
methods to make my own change lightweight and in need of fewer unit tests.
I then started hacking the build to use the commons Base64 jar as an
external dependency. Unfortunately, that turned out to be more difficult
than I expected, and I haven't yet finished the job.
I had a lot of non-tomcat priorities and have only just gone back to my
work-in-progress. Before I go any further, I would like to find out what
the general policy is in this sort of situation:
1. we have code that uses a tomcat-specific utility class that has been
stable for a long time.
2. The function of that class is similar, but not identical, to that of
one in apache commons.
Should we:
a) extend the tomcat-specific utility class to provide methods for new
logic that is not related to an important bugfix (effectively increasing
the duplication of a commons class)
or
b) use the commons class for the new logic, but leave everything else
unchanged (effectively having two classes performing similar tasks)
or
c) use the commons class and eliminate any redundant logic in
tomcat-specific classes (our class wraps the commons class to provide
extra functionality but no duplication of core logic)
I'm happy to go in any of these 3 directions. My preference would be to
use (b) initially, and follow up with (c) soon after.
Is there a policy, or a generally held opinion?
tia,
Brian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: Seeking guidance on use of apache commons classes
Posted by Brian Burch <br...@pingtoo.com>.
On 04/01/13 19:58, Konstantin Kolinko wrote:
> 2013/1/4 Brian Burch <br...@pingtoo.com>:
>> On 29/11/12 16:11, Brian Burch wrote:
>>>
>>> On 29/11/12 14:37, bugzilla@apache.org wrote:
>>>>
>>>> https://issues.apache.org/bugzilla/show_bug.cgi?id=54190
>>
>>
>> Re: [Bug 54190] TestNonLoginAndBasicAuthenticator does not test session
>> timeout properly
>>
>>>>
>>>> Mark Thomas <ma...@apache.org> changed:
>>>>
>>>> What |Removed |Added
>>>>
>>>> ----------------------------------------------------------------------------
>>>>
>>>> Status|NEW |RESOLVED
>>>> Resolution|--- |FIXED
>>>>
>>>> --- Comment #6 from Mark Thomas <ma...@apache.org> ---
>>
>> <snip/>
>>>
>>>
>>>> If you start looking at the TODOs, I suggest you take a look at
>>>> org.apache.tomcat.util.http.parser.HttpParser#parseAuthorizationDigest()
>>>>
>>>> I suspect a new parseAuthorizationBasic() method is the way to go as that
>>>> should handle the various whitespace issues noted.
>>>
>>>
>>> Noted. Assume that I will look into it unless you hear otherwise. I
>>> won't open a bug against BasicAuthenticator yet.
>>
>>
>> My motive is the original thread above, but my question has a wider scope. I
>> have started a new thread, even if it doesn't run far, just to make it
>> easier for others to find.
>>
>> I started looking at Mark's suggestion and got a long way with the change.
>>
>> Currently, BasicAuthenticator.authenticate uses
>> org.apache.catalina.util.Base64.decode(ByteChunk,CharChunk).
>>
>> I decided to follow the pattern used by DigestAuthenticator and create a new
>> method
>> org.apache.tomcat.util.http.parser.HttpParser.parseAuthorizationBasic(StringReader).
>>
>> Once I got there, I discovered that org.apache.catalina.util.Base64 doesn't
>> currently have any methods to decode a StringReader or String object.
>>
>> It didn't take me long to find
>> org.apache.commons.codec.binary.Base64.Base64, which has all the right
>> methods to make my own change lightweight and in need of fewer unit tests.
>>
>> I then started hacking the build to use the commons Base64 jar as an
>> external dependency. Unfortunately, that turned out to be more difficult
>> than I expected, and I haven't yet finished the job.
>>
>> I had a lot of non-tomcat priorities and have only just gone back to my
>> work-in-progress. Before I go any further, I would like to find out what the
>> general policy is in this sort of situation:
>>
>> 1. we have code that uses a tomcat-specific utility class that has been
>> stable for a long time.
>>
>> 2. The function of that class is similar, but not identical, to that of one
>> in apache commons.
>>
>> Should we:
>>
>> a) extend the tomcat-specific utility class to provide methods for new logic
>> that is not related to an important bugfix (effectively increasing the
>> duplication of a commons class)
>>
>> or
>>
>> b) use the commons class for the new logic, but leave everything else
>> unchanged (effectively having two classes performing similar tasks)
>>
>> or
>>
>> c) use the commons class and eliminate any redundant logic in
>> tomcat-specific classes (our class wraps the commons class to provide extra
>> functionality but no duplication of core logic)
>>
>>
>> I'm happy to go in any of these 3 directions. My preference would be to use
>> (b) initially, and follow up with (c) soon after.
>>
>> Is there a policy, or a generally held opinion?
>>
>
> The general rule is that Tomcat cannot directly use the commons
> library, as it might clash with such use in a web application. (Though
> if you need them for tests only, such a clash should not be a matter).
>
> Whenever we need classes from commons, we copy them into our source
> tree into a different package (usually with "svn copy" to preserve
> history).
>
> Mark usually removes unused code from the copy, leaving only those
> bits that are actually used by Tomcat.
>
> Examples:
> org.apache.tomcat.util.http.fileupload
> org.apache.tomcat.util.bcel
> org.apache.tomcat.util.digester
>
>> Once I got there, I discovered that org.apache.catalina.util.Base64 doesn't
>> currently have any methods to decode a StringReader or String object.
>
> HTTP headers are converted from bytes to characters using ISO-8859-1
> encoding. (Actually they should be 7-bit ASCII). This conversion is
> reversible.
>
> If all you need it for is a test method, I think I would just convert
> the string to bytes and use the existing API. Do you need to support
> a Reader there?
In fact, the change is not aimed at test code - it will be used by
BasicAuthenticator, although there will also be new tests for the parser
method too.
Thanks very much for your helpful explanation. I understand the reasons,
but I had not thought about the problem that way before - it is almost
the opposite of my intuition. I had thought the dependent jars section
of the tomcat build was a kind of prototype of maven dependencies, and
so assumed that more external dependencies would be a good thing because
it would eliminate duplicate code. It is interesting to be shown a valid
counter-argument.
I will back out the work I've done already to use commons, and proceed
with an extension to the org.apache.catalina.util.Base64 class.
Regards,
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
Re: Seeking guidance on use of apache commons classes
Posted by Mark Thomas <ma...@apache.org>.
On 04/01/2013 19:58, Konstantin Kolinko wrote:
> 2013/1/4 Brian Burch <br...@pingtoo.com>:
>> On 29/11/12 16:11, Brian Burch wrote:
>> Once I got there, I discovered that org.apache.catalina.util.Base64 doesn't
>> currently have any methods to decode a StringReader or String object.
>
> HTTP headers are converted from bytes to characters using ISO-8859-1
> encoding. (Actually they should be 7-bit ASCII). This conversion is
> reversible.
>
> If all you need it for is a test method, I think I would just convert
> the string to bytes and use the existing API. Do you need to support
> a Reader there?
Even in non-test code I'd be tempted to at least start with that
approach. Anything that handles String is going to have to convert to
bytes anyway.
Mark
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: Seeking guidance on use of apache commons classes
Posted by Konstantin Kolinko <kn...@gmail.com>.
2013/1/4 Brian Burch <br...@pingtoo.com>:
> On 29/11/12 16:11, Brian Burch wrote:
>>
>> On 29/11/12 14:37, bugzilla@apache.org wrote:
>>>
>>> https://issues.apache.org/bugzilla/show_bug.cgi?id=54190
>
>
> Re: [Bug 54190] TestNonLoginAndBasicAuthenticator does not test session
> timeout properly
>
>>>
>>> Mark Thomas <ma...@apache.org> changed:
>>>
>>> What |Removed |Added
>>>
>>> ----------------------------------------------------------------------------
>>>
>>> Status|NEW |RESOLVED
>>> Resolution|--- |FIXED
>>>
>>> --- Comment #6 from Mark Thomas <ma...@apache.org> ---
>
> <snip/>
>>
>>
>>> If you start looking at the TODOs, I suggest you take a look at
>>> org.apache.tomcat.util.http.parser.HttpParser#parseAuthorizationDigest()
>>>
>>> I suspect a new parseAuthorizationBasic() method is the way to go as that
>>> should handle the various whitespace issues noted.
>>
>>
>> Noted. Assume that I will look into it unless you hear otherwise. I
>> won't open a bug against BasicAuthenticator yet.
>
>
> My motive is the original thread above, but my question has a wider scope. I
> have started a new thread, even if it doesn't run far, just to make it
> easier for others to find.
>
> I started looking at Mark's suggestion and got a long way with the change.
>
> Currently, BasicAuthenticator.authenticate uses
> org.apache.catalina.util.Base64.decode(ByteChunk,CharChunk).
>
> I decided to follow the pattern used by DigestAuthenticator and create a new
> method
> org.apache.tomcat.util.http.parser.HttpParser.parseAuthorizationBasic(StringReader).
>
> Once I got there, I discovered that org.apache.catalina.util.Base64 doesn't
> currently have any methods to decode a StringReader or String object.
>
> It didn't take me long to find
> org.apache.commons.codec.binary.Base64.Base64, which has all the right
> methods to make my own change lightweight and in need of fewer unit tests.
>
> I then started hacking the build to use the commons Base64 jar as an
> external dependency. Unfortunately, that turned out to be more difficult
> than I expected, and I haven't yet finished the job.
>
> I had a lot of non-tomcat priorities and have only just gone back to my
> work-in-progress. Before I go any further, I would like to find out what the
> general policy is in this sort of situation:
>
> 1. we have code that uses a tomcat-specific utility class that has been
> stable for a long time.
>
> 2. The function of that class is similar, but not identical, to that of one
> in apache commons.
>
> Should we:
>
> a) extend the tomcat-specific utility class to provide methods for new logic
> that is not related to an important bugfix (effectively increasing the
> duplication of a commons class)
>
> or
>
> b) use the commons class for the new logic, but leave everything else
> unchanged (effectively having two classes performing similar tasks)
>
> or
>
> c) use the commons class and eliminate any redundant logic in
> tomcat-specific classes (our class wraps the commons class to provide extra
> functionality but no duplication of core logic)
>
>
> I'm happy to go in any of these 3 directions. My preference would be to use
> (b) initially, and follow up with (c) soon after.
>
> Is there a policy, or a generally held opinion?
>
The general rule is that Tomcat cannot directly use the commons
library, as it might clash with such use in a web application. (Though
if you need them for tests only, such a clash should not be a matter).
Whenever we need classes from commons, we copy them into our source
tree into a different package (usually with "svn copy" to preserve
history).
Mark usually removes unused code from the copy, leaving only those
bits that are actually used by Tomcat.
Examples:
org.apache.tomcat.util.http.fileupload
org.apache.tomcat.util.bcel
org.apache.tomcat.util.digester
> Once I got there, I discovered that org.apache.catalina.util.Base64 doesn't
> currently have any methods to decode a StringReader or String object.
HTTP headers are converted from bytes to characters using ISO-8859-1
encoding. (Actually they should be 7-bit ASCII). This conversion is
reversible.
If all you need it for is a test method, I think I would just convert
the string to bytes and use the existing API. Do you need to support
a Reader there?
Best regards,
Konstantin Kolinko
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org