garydgregory commented on code in PR #222:
URL: https://github.com/apache/commons-codec/pull/222#discussion_r1409857343
##########
src/main/java/org/apache/commons/codec/net/PercentCodec.java:
##########
@@ -77,10 +77,15 @@ public PercentCodec() {
*
* @param alwaysEncodeChars the unsafe characters that should always be
encoded
* @param plusForSpace the flag defining if the space character
should be encoded as '+'
+ * @throws EncoderException if the alwaysEncodeChars byte array contains
invalid bytes
*/
- public PercentCodec(final byte[] alwaysEncodeChars, final boolean
plusForSpace) {
+ public PercentCodec(final byte[] alwaysEncodeChars, final boolean
plusForSpace) throws EncoderException {
Review Comment:
Hi @arthurscchan
Now that I look at this again, it doesn't make sense to me, this seems like
the exact use case for an `IllegalArgumentException`. In addition, the
constructor does not encode anything, so an `EncoderException` really does not
make sense IMO.
I propose:
```
diff --git a/src/main/java/org/apache/commons/codec/net/PercentCodec.java
b/src/main/java/org/apache/commons/codec/net/PercentCodec.java
index 8e51538..df623df 100644
--- a/src/main/java/org/apache/commons/codec/net/PercentCodec.java
+++ b/src/main/java/org/apache/commons/codec/net/PercentCodec.java
@@ -231,6 +231,9 @@
* @param b the byte that is candidate for min and max limit
*/
private void insertAlwaysEncodeChar(final byte b) {
+ if (b < 0) {
+ throw new IllegalArgumentException("byte must be >= 0");
+ }
this.alwaysEncodeChars.set(b);
if (b < alwaysEncodeCharsMin) {
alwaysEncodeCharsMin = b;
diff --git
a/src/test/java/org/apache/commons/codec/net/PercentCodecTest.java
b/src/test/java/org/apache/commons/codec/net/PercentCodecTest.java
index 34f8ecb..a537efb 100644
--- a/src/test/java/org/apache/commons/codec/net/PercentCodecTest.java
+++ b/src/test/java/org/apache/commons/codec/net/PercentCodecTest.java
@@ -106,6 +106,12 @@
}
@Test
+ public void testInvalidByte() throws Exception {
+ final byte[] invalid = { (byte) -1, (byte) 'A' };
+ assertThrows(IllegalArgumentException.class, () -> new
PercentCodec(invalid, true));
+ }
+
+ @Test
public void testPercentEncoderDecoderWithNullOrEmptyInput() throws
Exception {
final PercentCodec percentCodec = new PercentCodec(null, true);
assertNull(percentCodec.encode(null), "Null input value encoding
test");
```
WDYT?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
