[CODEC-333] Add separate methods for strict Standard and URL Safe Base64 decoding #419
Conversation
garydgregory
changed the title
There was a problem hiding this comment.
Hello @bsanchezb
Thank you for the PR. Overall, this is great. I have small comments to address scattered throughout.
I am concerned we are increasing the public footprint in this class by a lot. For example, I do not see a public use case for:
isBase64Standard(byte)isBase64Url(byte)
If you disagree, please explain. The idea is that we should YAGNI the API here.
Thank you!
| */ | ||
| private static final byte[] DECODE_TABLE = { | ||
| // 0 1 2 3 4 5 6 7 8 9 A B C D E F | ||
| // 0 1 2 3 4 5 6 7 8 9 A B C D E F |
There was a problem hiding this comment.
Please don't edit this line, it's a poor-man's column header.
| /** | ||
| * Returns whether or not the {@code octet} is in the base 64 alphabet. | ||
| * <p> | ||
| * Note: this method threats both characters '+' and '/' and '-' and '_' as valid base64 characters. |
There was a problem hiding this comment.
The phrasing is off here: Using "both" implies 2, but you list 4, each with the word 'and'. What do you really mean?
| * Tests a given byte array to see if it contains only valid characters within the Base64 alphabet. Currently the | ||
| * method treats whitespace as valid. | ||
| * <p> | ||
| * Note: this method threats both characters '+' and '/' and '-' and '_' as valid base64 characters. |
There was a problem hiding this comment.
The phrasing is off here: Using "both" implies 2, but you list 4, each with the word 'and'. What do you really mean?
| * Tests a given String to see if it contains only valid characters within the Base64 alphabet. Currently the | ||
| * method treats whitespace as valid. | ||
| * <p> | ||
| * Note: this method threats both characters '+' and '/' and '-' and '_' as valid base64 characters. |
There was a problem hiding this comment.
The phrasing is off here: Using "both" implies 2, but you list 4, each with the word 'and'. What do you really mean?
| * both URL_SAFE and STANDARD base64. | ||
| * </p> | ||
| * | ||
| * @param format table format to be used on Base64 decoding. |
There was a problem hiding this comment.
Document null input resets to the default.
| * This method allows to explicitly state whether a "standard" or "URL Safe" Base64 decoding is expected. | ||
| * <p> | ||
| * Note: By default, the implementation uses the MIXED approach, allowing a seamless handling of | ||
| * both URL_SAFE and STANDARD base64. |
There was a problem hiding this comment.
Use @link when referring to elements in this class. For example {@link DecodeTableFormat#URL_SAFE}.
| * in Table 1 of RFC 2045) into their 6-bit positive integer equivalents. Characters that are not in the Base64 | ||
| * alphabet but fall within the bounds of the array are translated to -1. | ||
| * <p> | ||
| * Note: This decoding table handles only the "standard" base64 characters, such as '+' and '/'. |
| * Characters that are not in the Base64 URL Safe alphabet but fall within the bounds of the array | ||
| * are translated to -1. | ||
| * <p> | ||
| * Note: This decoding table handles only the "URL Safe" base64 characters, such as '-' and '_'. |
| * <p> | ||
| * <strong>Note:</strong> this method seamlessly handles data encoded in URL-safe or normal mode. | ||
| * For enforcing verification against strict standard Base64 or Base64 URL Safe tables, | ||
| * please use {@code #decodeBase64Standard} or {@code decodeBase64Url} methods respectively. |
There was a problem hiding this comment.
Use @link where you can instead of @code. Saying {@code #... doesn't mean anything in Javadoc, only in {@link #....
| } | ||
|
|
||
| /** | ||
| * Sets the format of the decoding table. |
There was a problem hiding this comment.
This method and Builder.setUrlSafe(boolean) should make it clear that it doesn'r affect the action of the other. For example, Builder.setUrlSafe(boolean) already says that it only affects encoding but we should also say that if you care about decoding then you should call... I tihnk we want to make it clear what to call for what use case. It might be worth mentioning at the class level and Builder class level as well.
|
Hi @garydgregory , Thank you for the thoughtful review. I added changes according to your comments. Regarding the
I do not have a strong opinion on this, but I was trying to apply the same design pattern used for the |
|
Hello @bsanchezb |
2 participants
This PR implements strict Base64 decoding mechanisms, as explained in CODEC-333.
The proposed implementation keeps the current behavior that supports both "standard" and URL Safe Base64 characters on decoding by default, but adds supplementary methods allowing to enforce a strict processing either for the "standard" Base64 or URL Safe Base64.
Also added unit tests demonstrating the difference in behavior.
NOTE: During the testing it was also found that on Base64#decodeBase64 method, the implementation silently skips unsupported characters. In my opinion, the decoding should fail in such case, by throwing an exception instead of silent processing. However, this PR does not address this issue and the same behavior is implemented in the new methods.