Use HexFormat instead of custom Hex codec - #19641
Conversation
Replace internal org.springframework.security.crypto.codec.Hex utility class with standard Java 17 java.util.HexFormat. We already did something similar in this module earlier: 2f53a2e Signed-off-by: Andrey Litvitski <andrey1010102008@gmail.com>
| return new String(Hex.encode(sha(data))); | ||
| return HexFormat.of().formatHex(sha(data)); |
There was a problem hiding this comment.
Previously, the string was created without regard for the string pool, but now the string pool is being used. Perhaps it should be wrapped in new String()?
There was a problem hiding this comment.
Thanks for checking. I believe the code is fine as-is. The new String() from before and Hex.encode returning char[] was not about preventing the value from entering the String pool. In that case, to add it, someone would need to call .intern().
And since HexFormat does not do that, the interning semantics of this method do not change with your edit. 👍
spring-projects-issues
added
the
status: waiting-for-triage
|
It also renders #19023 unnecessary. |
jzheaux
left a comment
jzheaux
left a comment
There was a problem hiding this comment.
Thanks, @therepanic! It's great to be able to change to use official libraries.
In addition to my inline feedback, will you please also still deprecate Hex? We need to do this since it is a public class. Will you add a @Deprecated annotation as well as a @deprecated javadoc note that indicates folks should use HexFormat?
This can go in an earlier commit so you can reference the other ticket you created.
| return new String(Hex.encode(sha(data))); | ||
| return HexFormat.of().formatHex(sha(data)); |
There was a problem hiding this comment.
Thanks for checking. I believe the code is fine as-is. The new String() from before and Hex.encode returning char[] was not about preventing the value from entering the String pool. In that case, to add it, someone would need to call .intern().
And since HexFormat does not do that, the interning semantics of this method do not change with your edit. 👍
3 participants
Replace internal
org.springframework.security.crypto.codec.Hexutility class with standard Java 17java.util.HexFormat. We already did something similar in this module earlier: 2f53a2e