You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@pivot.apache.org by Todd Volkert <tv...@gmail.com> on 2009/10/21 19:32:10 UTC
Re: svn commit: r828043 - in /incubator/pivot/trunk/web:
src/org/apache/pivot/web/ test/org/apache/pivot/web/test/
Question: what does the MD5 class do that java.security.MessageDigest
doesn't?
Re: svn commit: r828043
Posted by Greg Brown <gk...@mac.com>.
>> The Integer class provides methods for translating to and from hex - is there any reason we can't simply use those methods?
>Probably this class (the original version) was written when that
>feature wasn't existing or wasn't efficient ... I'll try to look at
>the code.
OK.
Re: svn commit: r828043
Posted by Sandro Martini <sa...@gmail.com>.
> The Integer class provides methods for translating to and from hex - is there any reason we can't simply use those methods?
Probably this class (the original version) was written when that
feature wasn't existing or wasn't efficient ... I'll try to look at
the code.
Ah, note that this class currently is optimized for byte [] , I have
to see using Integers (for doing the same things) if there are some
performances overhead ...
Bye
Re: svn commit: r828043
Posted by Greg Brown <gk...@mac.com>.
The Integer class provides methods for translating to and from hex - is there any reason we can't simply use those methods?
On Friday, October 23, 2009, at 11:52AM, "Sandro Martini" <sa...@gmail.com> wrote:
>Hi Greg,
>> Ah, I see. At a quick glance, it seemed pretty specialized. Is it really generic, or might it only be useful to MD5? If the latter, maybe it should be a private static inner class of MD5.
>At the moment is used only inside MD5, but has some generic logic to
>transform from/to Hex values, so I think it would be better (in terms
>or future use / reuse) to have it as a standard class ...
>
>Bye
>
>
Re: svn commit: r828043
Posted by Sandro Martini <sa...@gmail.com>.
Hi Greg,
> Ah, I see. At a quick glance, it seemed pretty specialized. Is it really generic, or might it only be useful to MD5? If the latter, maybe it should be a private static inner class of MD5.
At the moment is used only inside MD5, but has some generic logic to
transform from/to Hex values, so I think it would be better (in terms
or future use / reuse) to have it as a standard class ...
Bye
Re: svn commit: r828043
Posted by Greg Brown <gk...@mac.com>.
>HexUtils is a generic class to transform data from/to HEX format, so
>I'd put in the same package of MD5 ...
Ah, I see. At a quick glance, it seemed pretty specialized. Is it really generic, or might it only be useful to MD5? If the latter, maybe it should be a private static inner class of MD5.
Re: svn commit: r828043
Posted by Sandro Martini <sa...@gmail.com>.
>>> 1) I don't think we should deprecate methods that we've never released.
>>> IMO, deprecation is for backward compatibility for perhaps one release cycle
>>> to allow developers who've used a method to transition to the newer API.
>>> I'd say let's remove the methods instead of deprecating them. If we need
>>> them later, they're in SVN and can be easily restored.
>>Right, but if there aren't objections I'd prefer to keep them at the
>>moment (so i can not add the deprecation on them), and maybe add the
>>new version (by mixing two methods, to return a String).
>
> I agree that we should not be tagging any new methods as deprecated. This is only likely to cause confusion.
Ok.
>>> 2) I think MD5 should go in core/util -- there's nothing specifically "web" about it.
>>My initial version of it (and the HexUtils) was in core, but to avoid
>>fill with strange code I moved them to web, but I agree with you, so
>>if there aren't objections I can move them (MD5 and HexUtils) in core,
>>under the org.apache.pivot.util package.
>
> I haven't looked at these files closely yet, but I am guessing that org.apache.pivot.util is a good place for MD5, whereas HexUtils may be better off as a package private class in org.apache.pivot.web or as an inner class of DigestAuthentication.
HexUtils is a generic class to transform data from/to HEX format, so
I'd put in the same package of MD5 ...
Bye
Re: svn commit: r828043
Posted by Greg Brown <gk...@mac.com>.
>> 1) I don't think we should deprecate methods that we've never released.
>> IMO, deprecation is for backward compatibility for perhaps one release cycle
>> to allow developers who've used a method to transition to the newer API.
>> I'd say let's remove the methods instead of deprecating them. If we need
>> them later, they're in SVN and can be easily restored.
>Right, but if there aren't objections I'd prefer to keep them at the
>moment (so i can not add the deprecation on them), and maybe add the
>new version (by mixing two methods, to return a String).
I agree that we should not be tagging any new methods as deprecated. This is only likely to cause confusion.
>> 2) I think MD5 should go in core/util -- there's nothing specifically "web" about it.
>My initial version of it (and the HexUtils) was in core, but to avoid
>fill with strange code I moved them to web, but I agree with you, so
>if there aren't objections I can move them (MD5 and HexUtils) in core,
>under the org.apache.pivot.util package.
I haven't looked at these files closely yet, but I am guessing that org.apache.pivot.util is a good place for MD5, whereas HexUtils may be better off as a package private class in org.apache.pivot.web or as an inner class of DigestAuthentication.
G
Re: svn commit: r828043 - in /incubator/pivot/trunk/web:
src/org/apache/pivot/web/ test/org/apache/pivot/web/test/
Posted by Sandro Martini <sa...@gmail.com>.
Hi to all,
a quick note:
my sample folders for the Apache test were working on Apache 2.0.x,
but no more on 2.2.x, I just discovered that a little change in
.htaccess (used to setup a simple per-directory security) files were
needed ... tell me if someone needs a zip containing this little test
setup.
And I've just committed some little changes discussed today on MD5.
Good week-end to all.
Bye,
Sandro
Re: svn commit: r828043 - in /incubator/pivot/trunk/web:
src/org/apache/pivot/web/ test/org/apache/pivot/web/test/
Posted by Sandro Martini <sa...@gmail.com>.
Hi Todd,
> 1) I don't think we should deprecate methods that we've never released.
> IMO, deprecation is for backward compatibility for perhaps one release cycle
> to allow developers who've used a method to transition to the newer API.
> I'd say let's remove the methods instead of deprecating them. If we need
> them later, they're in SVN and can be easily restored.
Right, but if there aren't objections I'd prefer to keep them at the
moment (so i can not add the deprecation on them), and maybe add the
new version (by mixing two methods, to return a String).
> 2) I think MD5 should go in core/util -- there's nothing specifically "web" about it.
My initial version of it (and the HexUtils) was in core, but to avoid
fill with strange code I moved them to web, but I agree with you, so
if there aren't objections I can move them (MD5 and HexUtils) in core,
under the org.apache.pivot.util package.
Bye
Re: svn commit: r828043 - in /incubator/pivot/trunk/web:
src/org/apache/pivot/web/ test/org/apache/pivot/web/test/
Posted by Todd Volkert <tv...@gmail.com>.
Two new thoughts on this discussion:
1) I don't think we should deprecate methods that we've never released.
IMO, deprecation is for backward compatibility for perhaps one release cycle
to allow developers who've used a method to transition to the newer API.
I'd say let's remove the methods instead of deprecating them. If we need
them later, they're in SVN and can be easily restored.
2) I think MD5 should go in core/util -- there's nothing specifically "web"
about it.
Thoughts?
-T
On Fri, Oct 23, 2009 at 5:48 AM, Sandro Martini <sa...@gmail.com>wrote:
> Hi Chris,
>
> first the simple answer, for Todd:
> > I was just looking at that class... I usually wrap the MessageDigest
> > stuff in a utility class myself, mainly because it's a few lines of
> > code that would otherwise get repeated quite a lot.
> right.
>
>
> > I see that our MD5 class also lets you convert to bytes. It would be
> > easy enough to extract that from the above snippet - but does anyone
> > use the byte[] version of an MD5 hash?
> In this class, portions of code are taken from Apache Tomcat
> (org.apache.catalina.util.MD5Encoder), and this is only an "utility"
> class, to solve some things in the more complex DigestAuthentication,
> so when I wrote the code, this class was as closer as the original
> one, but your suggestions are good.
> We could add your method and deprecate others (they are working, and
> in some cases they could be useful ...).
> But in any case I'd keep the MD5 class, could be useful to reuse also
> from other things.
>
> > I would also question why our MD5 class is synchronising on itself -
> > as far as I was aware using MessageDigest was a thread-safe operation
> > - then again, I don't actually know and the API docs don't seem to
> > mention this one way or the other.
> In the original source (take a look here
>
> http://www.docjar.com/html/api/org/apache/catalina/authenticator/DigestAuthenticator.java.html
> ), they do the same, so to avoid problems I do it ... but probably you
> are right, it's unnecessary, I know little this (many years of Web
> Sites only) and I forgot this part.
>
> > Finally - why bother with the static decode method? Hashes are
> > intended to be one way, right? Even if not, this method seems totally
> redundant.
> Yes, this was an idea to have it as a marker method for other classes
> that needs to do something like MD5 (that could have the decode
> feature), but we can drop it.
>
>
> Ah, a little note:
> some of my things to verify were taken from features of the Digest as
> seen in the RFC, like using SHA instead of MD5, but I haven't found
> anyone that uses it, and the same for other things.
> Other things to verify were taken from the Tomcat implementation of
> this code but probably here are unnecessary because they were
> Tomcat-specific.
>
> Important tests still to do are to make it working also with Tomcat,
> and also try to call the same URL more times (the RFC says there is a
> counter in the answer that have to be read and do a +1 in the next
> request) but I wasn't able to make this test ... I hope to find time
> for this stuff next week.
>
> > Just some thoughts.
> No problem, comments/suggestions are always welcome :-) .
>
> If you want, be free to make changes to the code, but only remember to
> run the Unit Test to ensure that all works after changes ... only tell
> me, to avoid doing in two the same changes :-) .
>
> Byeee
>
Re: svn commit: r828043 - in /incubator/pivot/trunk/web:
src/org/apache/pivot/web/ test/org/apache/pivot/web/test/
Posted by Sandro Martini <sa...@gmail.com>.
Hi Chris,
first the simple answer, for Todd:
> I was just looking at that class... I usually wrap the MessageDigest
> stuff in a utility class myself, mainly because it's a few lines of
> code that would otherwise get repeated quite a lot.
right.
> I see that our MD5 class also lets you convert to bytes. It would be
> easy enough to extract that from the above snippet - but does anyone
> use the byte[] version of an MD5 hash?
In this class, portions of code are taken from Apache Tomcat
(org.apache.catalina.util.MD5Encoder), and this is only an "utility"
class, to solve some things in the more complex DigestAuthentication,
so when I wrote the code, this class was as closer as the original
one, but your suggestions are good.
We could add your method and deprecate others (they are working, and
in some cases they could be useful ...).
But in any case I'd keep the MD5 class, could be useful to reuse also
from other things.
> I would also question why our MD5 class is synchronising on itself -
> as far as I was aware using MessageDigest was a thread-safe operation
> - then again, I don't actually know and the API docs don't seem to
> mention this one way or the other.
In the original source (take a look here
http://www.docjar.com/html/api/org/apache/catalina/authenticator/DigestAuthenticator.java.html
), they do the same, so to avoid problems I do it ... but probably you
are right, it's unnecessary, I know little this (many years of Web
Sites only) and I forgot this part.
> Finally - why bother with the static decode method? Hashes are
> intended to be one way, right? Even if not, this method seems totally redundant.
Yes, this was an idea to have it as a marker method for other classes
that needs to do something like MD5 (that could have the decode
feature), but we can drop it.
Ah, a little note:
some of my things to verify were taken from features of the Digest as
seen in the RFC, like using SHA instead of MD5, but I haven't found
anyone that uses it, and the same for other things.
Other things to verify were taken from the Tomcat implementation of
this code but probably here are unnecessary because they were
Tomcat-specific.
Important tests still to do are to make it working also with Tomcat,
and also try to call the same URL more times (the RFC says there is a
counter in the answer that have to be read and do a +1 in the next
request) but I wasn't able to make this test ... I hope to find time
for this stuff next week.
> Just some thoughts.
No problem, comments/suggestions are always welcome :-) .
If you want, be free to make changes to the code, but only remember to
run the Unit Test to ensure that all works after changes ... only tell
me, to avoid doing in two the same changes :-) .
Byeee
Re: svn commit: r828043 - in /incubator/pivot/trunk/web:
src/org/apache/pivot/web/ test/org/apache/pivot/web/test/
Posted by Christopher Brind <br...@brindy.org.uk>.
*de-lurks*
I was just looking at that class... I usually wrap the MessageDigest
stuff in a utility class myself, mainly because it's a few lines of
code that would otherwise get repeated quite a lot.
For hashing a string I usually use this:
public static String getMd5Digest(String input) {
try {
MessageDigest md = MessageDigest.getInstance("MD5");
byte[] messageDigest = md.digest(input.getBytes());
BigInteger number = new BigInteger(1,messageDigest);
String hash = number.toString(16);
if (hash.length() == 31) {
hash = "0" + hash;
}
return hash;
} catch(NoSuchAlgorithmException e) {
throw new RuntimeException(e);
}
}
I see that our MD5 class also lets you convert to bytes. It would be
easy enough to extract that from the above snippet - but does anyone
use the byte[] version of an MD5 hash?
I would also question why our MD5 class is synchronising on itself -
as far as I was aware using MessageDigest was a thread-safe operation
- then again, I don't actually know and the API docs don't seem to
mention this one way or the other.
Finally - why bother with the static decode method? Hashes are
intended to be one way, right? Even if not, this method seems totally
redundant.
Just some thoughts.
Cheers,
Chris
2009/10/22 Sandro Martini <sa...@gmail.com>:
> Hi Todd,
>> Question: what does the MD5 class do that java.security.MessageDigest doesn't?
> Good question ... I have to look at the code (i made it many months
> ago), and tell you.
>
> Bye
>
Re: svn commit: r828043 - in /incubator/pivot/trunk/web:
src/org/apache/pivot/web/ test/org/apache/pivot/web/test/
Posted by Sandro Martini <sa...@gmail.com>.
Hi Todd,
> Question: what does the MD5 class do that java.security.MessageDigest doesn't?
Good question ... I have to look at the code (i made it many months
ago), and tell you.
Bye