On 21/03/2013 07:56, Brian Burch wrote:
On 20/03/13 10:17, bugzi...@apache.org wrote:
https://issues.apache.org/bugzilla/show_bug.cgi?id=54729
Sorry I could not reply more quickly, Mark, but I am currently in
Australia and I am probably asleep while you are working.
Not a problem. One of the reasons the ASF insists on using mailing lists
rather than teleconferences is to ensure everyone in all timezones has
an equal chance to participate.
Judging by the flurry of activity in this area, I conclude my change is
no longer necessary and you might want to close this bug report.
You know that better than I. My understanding from reading your report
was that there were a number of issues of which Base64 was just one and
one that was making fixing the others look a little ugly.
Just for completeness, I'll briefly answer your comments and the later
one from Konstantin.
--- Comment #1 from Mark Thomas <ma...@apache.org> ---
Regarding your points:
1. Clarity vs efficiency is always a trade off. It depends how much
efficiency
has been lost. Do you have any performance numbers?
You were quick to write a performance test! Given more time, I would
have explained that I (like Konstantin) did not like the conversion from
MessageBytes to a StringReader, and did not like the way I had created a
second StringReader to parse the decoded the Base64 blob. However, I
felt the performance hit would be small relative to the original
implementation, although it would create extra short-lived instances.
My earlier approach had attempted to push the MessageBytes into the new
HttpParser method, but that meant the parsing logic for BASIC became
very different to that of DIGEST. I couldn't see a way to square the
circle, so I trashed it and started again on a more harmonious path.
The conversions aren't ideal but I think they are a price worth paying
for not having to maintain another Base64 encoder/decoder.
2. I don't see a test case either. I'd rather get any bugs in the
decoder fixed
than put sticking plasters over other bits of code.
I've worked with Base64 implementations a lot in the past, in many
different languages. However, the last time I looked (showing my age!)
the java class was in a sun.* package and well worth avoiding. When I
researched the apache commons version, I did not realise it had been
superseded by javax.xml.bind.DatatypeConverter.
I've just noticed the discussion on the dev list "Use of JAXB api to
process Base64 in Tomcat 7 and 8". I understand the arguments presented
to not use this converter, but if you back out your change and reinstate
org.apache.catalina.util.Base64, then we just go back to where I started
with a broken implementation and no unit tests....
I have a plan B in mind for that. More details on that thread.
3. I'd prefer to do this first. That code is only still in the code base
because it support ByteChunk / CharChunk which should allow a more
efficient
interface with the rest of the Tomcat code base. It is probably time
that that
assumption is tested.
I understand. However, the two decode methods look superficially quite
similar to me. There must be scope for refactoring them, writing a
proper unit test suite, and fixing the bugs that I wrapped in my
suggested change to Basic Authenticator. That is a fair amount of work
to achieve little more than just copying another implementation and add
some novel method signatures. Wrappering a robust implementation feels
like a better approach to me.
+1. I'm all for code re-use.
4. It is very unlikely we will be adding Commons Codec as a dependency to
Tomcat. We may do an svn copy and rename much like we have done for
FileUpload
but the usefulness of that depends on the ByteChunk/CharChunk issues
in 3. For
FileUpload I am looking at replacing the decoder with the JVM
implementation.
I didn't follow your FileUpload change closely, especially before you
switched to the DatatypeConverter. Did you basically clone the commons
class? If yes, surely it would be better to put that code in tomcat.util
- FileUpload could use it from there and I suspect the
ByteChunk/CharChunk users would only need some pre/post processing.
The Base64 decoder in FileUpload was copied from Geronimo to Commons and
then I updated Tomcat's copy of FileUpload. That Base64 code has issues
of it's own. Looking for a solution to that prior to the 7.0.39 release
was co-incidental with you looking at Base64 for BASIC auth.
It seems to me there are enough developers (in the same time zone)
working on these issues. I don't have a lot of time to spend on tomcat
at the moment, and it is fairly disheartening to find my efforts in what
seemed to be a quiet backwater have been washed away by a flood. I'll
step back until the dust settles! Let me know if I can help.
I'm sorry you feel that your efforts have been wasted. My intention was
to get you a robust Base64 implementation to work with for the other
improvements you had in mind for BASIC auth. If there are other
improvements you have in mind for BASIC auth then your help there would
be much appreciated.
Hopefully, by the time you wake up the Base64 issue will have a solution
everyone can agree with.
