Repository: netty/netty
Issue: QueryStringDecoder and ByteBufUtil decoding hex string duplication

QueryStringDecoder has methods which deal with decoding hex into bytes (e.g. `decodeHexByte`, `decodeHexNibble`) and `ByteBufUtil` has `decodeHexDump`. Can we share code between these two classes?

Issue comments:

Pull request review comments:
nit: `length % 2 != 0` -> `(length & 1) != 0`. you can avoid modulo for a cheaper bit mask operation.
please change the indexing to the way it was previously. You are adding additional arithmetic on each loop iteration which can be avoided.
can you include a microbenchmark comparing this method vs `Character.digit(x, 16)`? If `decodeHexNibble` is sufficiently faster we may want to continue to use it.
In my tests, a `QueryStringDecoder.decodeHexNibble()` faster than `Character.digit()` by ~30%.
Also `Character.digit()` supports some exotic cases:
```java
Character.digit('\uFF21', 16) => 10 // 'Ａ'
vs normal
Character.digit('\u0041', 16) => 10 // 'A'
```
Not sure that this is correctly for hex-encoding.
It's too wasteful to allocate new byte array on each encoded byte...
this will incur an intermediate `byte[]` array and copy operation. We should consider creating a variant of `decodeHexDump` which can directly write into a `ByteBuf` ... something like:

```java
public static void decodHexDump(CharSequence hexDump, int start, int len, ByteBuf result) {
...
}
```
+1 ... I have the same comment below.
+1 ... I have the same comment below.
> directly write into a ByteBuf

Or maybe just a function to deconing one byte?
I guess either would work in this cause however writing into a `ByteBuf` may be useful if users want to avoid the `byte[]` translation all together ... and this way users don't *have* to know how to properly iterate `hexDump` and the required characteristics.
@fenik17 Yes, the docs explicitly states so. QueryStringDecoder.decodeHexNibble() is simple and fits our purposes better, I think.
@Scottmitch
Test result given by @fenik17 above shows that "QueryStringDecoder.decodeHexNibble() faster than Character.digit() by ~30%." This agrees with the fact that Character.digit() tries to address a larger set of letters (ASCII latin+full-width latin; not exactly what we'd expect in escaped URL strings anyway) with a much more complex algorithm, etc.

So, let's, say, continue to use decodeHexNibble and drop this PR?
@Scottmitch For the original code, if I call decodeHexDump() with fromIndex=1024, length=2, "bytes[fromIndex >>> 1] = ... "will be referencing bytes[512] while the bytes array is of length 1. That's why I re-wrote this.
Ok, it makes sense. Just one moment: here `ByteBuffer`, not `ByteBuf`.
> and drop this PR?

No.. Just use `decodeHexNibble()` instead of `Character.digit(x, 16)`.
makes sense ... that was a bug ... previously we were only decoding from 0 so this went unnoticed.
@fenik17 - Are you suggesting adding a utility method for `ByteBuffer` instead of `ByteBuf`?
Move `decodeHexNibble` into `ByteBufUtil` and make it `private static` and use it instead of `Character.digit`
@Scottmitch I just pointing that in `QueryStringDecoder.decodeComponent()` used a `ByteBuffer`. If we want to use there the method you proposed, will have to make a variant with `ByteBuffer`.
Oh I see what you are saying now ... makes sense. Not ideal to add a `ByteBuffer` method in `ByteBufUtil` ... maybe we should change `QueryStringDecoder` to use a `ByteBuf` (so we can take advantage of our allocators) and then use `ByteBuf#nioBuffer()` to do the decoding.
Use `ByteBuf#nioBuffer()` will be too expensive. It smells of overengineering )
In `QueryStringDecoder.decodeComponent()` needed only decode two characters to one byte...
> Use ByteBuf#nioBuffer() will be too expensive. It smells of overengineering )

Perhaps ... if it is too much I'm not against directly adding a `ByteBuffer` utility method.
I agree with @fenik17 lets keep it simple... 

Patch that resolved the issue:
diff --git a/buffer/src/main/java/io/netty/buffer/ByteBufUtil.java b/buffer/src/main/java/io/netty/buffer/ByteBufUtil.java
index 2dda96bac28..98574503233 100644
--- a/buffer/src/main/java/io/netty/buffer/ByteBufUtil.java
+++ b/buffer/src/main/java/io/netty/buffer/ByteBufUtil.java
@@ -125,6 +125,13 @@ public static String hexDump(byte[] array, int fromIndex, int length) {
         return HexUtil.hexDump(array, fromIndex, length);
     }
 
+    /**
+     * Decode a 2-digit hex byte from within a string.
+     */
+    public static byte decodeHexByte(CharSequence s, int pos) {
+        return HexUtil.decodeHexByte(s, pos);
+    }
+
     /**
      * Decodes a string generated by {@link #hexDump(byte[])}
      */
@@ -132,6 +139,13 @@ public static byte[] decodeHexDump(CharSequence hexDump) {
         return HexUtil.decodeHexDump(hexDump, 0, hexDump.length());
     }
 
+    /**
+     * Decodes part of a string generated by {@link #hexDump(byte[])}
+     */
+    public static byte[] decodeHexDump(CharSequence hexDump, int fromIndex, int length) {
+        return HexUtil.decodeHexDump(hexDump, fromIndex, length);
+    }
+
     /**
      * Calculates the hash code of the specified buffer.  This method is
      * useful when implementing a new buffer type.
@@ -877,20 +891,50 @@ private static String hexDump(ByteBuf buffer, int fromIndex, int length) {
             return new String(buf);
         }
 
+        /**
+         * Helper to decode half of a hexadecimal number from a string.
+         * @param c The ASCII character of the hexadecimal number to decode.
+         * Must be in the range {@code [0-9a-fA-F]}.
+         * @return The hexadecimal value represented in the ASCII character
+         * given, or {@code -1} if the character is invalid.
+         */
+        private static int decodeHexNibble(final char c) {
+            // Character.digit() is not used here, as it addresses a larger
+            // set of characters (both ASCII and full-width latin letters).
+            if ('0' <= c && c <= '9') {
+                return c - '0';
+            }
+            if ('a' <= c && c <= 'f') {
+                return c - 'a' + 0xA;
+            }
+            if ('A' <= c && c <= 'F') {
+                return c - 'A' + 0xA;
+            }
+            return -1;
+        }
+
+        private static byte decodeHexByte(CharSequence s, int pos) {
+            int hi = decodeHexNibble(s.charAt(pos));
+            int lo = decodeHexNibble(s.charAt(pos + 1));
+            if (hi == -1 || lo == -1) {
+                throw new IllegalArgumentException(String.format(
+                        "invalid hex byte '%s' at index %d of '%s'", s.subSequence(pos, pos + 2), pos, s));
+            }
+            return (byte) ((hi << 4) + lo);
+        }
+
         private static byte[] decodeHexDump(CharSequence hexDump, int fromIndex, int length) {
-            if (length < 0) {
+            if (length < 0 || (length & 1) != 0) {
                 throw new IllegalArgumentException("length: " + length);
             }
             if (length == 0) {
                 return EmptyArrays.EMPTY_BYTES;
             }
 
-            int endIndex = fromIndex + length - 1;
             byte[] bytes = new byte[length >>> 1];
 
-            for (; fromIndex < endIndex; fromIndex += 2) {
-                bytes[fromIndex >>> 1] = (byte) ((Character.digit(hexDump.charAt(fromIndex), 16) << 4) +
-                                                  Character.digit(hexDump.charAt(fromIndex + 1), 16));
+            for (int i = 0; i < length; i += 2) {
+                bytes[i >>> 1] = decodeHexByte(hexDump, fromIndex + i);
             }
 
             return bytes;
diff --git a/codec-http/src/main/java/io/netty/handler/codec/http/QueryStringDecoder.java b/codec-http/src/main/java/io/netty/handler/codec/http/QueryStringDecoder.java
index 815d779f4fc..3df61e2ca48 100644
--- a/codec-http/src/main/java/io/netty/handler/codec/http/QueryStringDecoder.java
+++ b/codec-http/src/main/java/io/netty/handler/codec/http/QueryStringDecoder.java
@@ -33,6 +33,7 @@
 
 import static io.netty.util.internal.ObjectUtil.*;
 import static io.netty.util.internal.StringUtil.*;
+import static io.netty.buffer.ByteBufUtil.decodeHexByte;
 
 /**
  * Splits an HTTP query string into a path string and key-value parameter pairs.
@@ -330,7 +331,10 @@ private static String decodeComponent(String s, int from, int toExcluded, Charse
 
             byteBuf.clear();
             do {
-                byteBuf.put(decodeHexByte(s, i, toExcluded));
+                if (i + 3 > toExcluded) {
+                    throw new IllegalArgumentException("unterminated escape sequence at index " + i + " of: " + s);
+                }
+                byteBuf.put(decodeHexByte(s, i + 1));
                 i += 3;
             } while (i < toExcluded && s.charAt(i) == '%');
             i--;
@@ -354,39 +358,6 @@ private static String decodeComponent(String s, int from, int toExcluded, Charse
         return strBuf.toString();
     }
 
-    private static byte decodeHexByte(String s, int pos, int len) {
-        if (pos + 2 >= len) {
-            throw new IllegalArgumentException("unterminated escape sequence at index " + pos + " of: " + s);
-        }
-        int hi = decodeHexNibble(s.charAt(pos + 1));
-        int lo = decodeHexNibble(s.charAt(pos + 2));
-        if (hi == -1 || lo == -1) {
-            throw new IllegalArgumentException(
-                    "invalid escape sequence '" + s.substring(pos, pos + 3) + "' at index " + pos + " of: " + s);
-        }
-        return (byte) ((hi << 4) + lo);
-    }
-
-    /**
-     * Helper to decode half of a hexadecimal number from a string.
-     * @param c The ASCII character of the hexadecimal number to decode.
-     * Must be in the range {@code [0-9a-fA-F]}.
-     * @return The hexadecimal value represented in the ASCII character
-     * given, or {@code -1} if the character is invalid.
-     */
-    private static int decodeHexNibble(final char c) {
-        if ('0' <= c && c <= '9') {
-            return c - '0';
-        }
-        if ('a' <= c && c <= 'f') {
-            return c - 'a' + 0xA;
-        }
-        if ('A' <= c && c <= 'F') {
-            return c - 'A' + 0xA;
-        }
-        return -1;
-    }
-
     private static int findPathEndIndex(String uri) {
         int len = uri.length();
         for (int i = 0; i < len; i++) {


Test patch:
diff --git a/buffer/src/test/java/io/netty/buffer/ByteBufUtilTest.java b/buffer/src/test/java/io/netty/buffer/ByteBufUtilTest.java
index e1bf859b2ca..9865ed0109f 100644
--- a/buffer/src/test/java/io/netty/buffer/ByteBufUtilTest.java
+++ b/buffer/src/test/java/io/netty/buffer/ByteBufUtilTest.java
@@ -20,6 +20,7 @@
 import org.junit.Test;
 
 import java.nio.charset.Charset;
+import java.util.Arrays;
 import java.util.Random;
 
 import static io.netty.buffer.Unpooled.unreleasableBuffer;
@@ -31,22 +32,35 @@
 
 public class ByteBufUtilTest {
     @Test
-    public void decodeHexEvenLength() {
-        decodeHex(256);
+    public void decodeRandomHexBytesWithEvenLength() {
+        decodeRandomHexBytes(256);
     }
 
     @Test
-    public void decodeHexOddLength() {
-        decodeHex(257);
+    public void decodeRandomHexBytesWithOddLength() {
+        decodeRandomHexBytes(257);
     }
 
-    private static void decodeHex(int len) {
+    private static void decodeRandomHexBytes(int len) {
         byte[] b = new byte[len];
         Random rand = new Random();
         rand.nextBytes(b);
         String hexDump = ByteBufUtil.hexDump(b);
-        byte[] decodedBytes = ByteBufUtil.decodeHexDump(hexDump);
-        assertArrayEquals(b, decodedBytes);
+        for (int i = 0; i <= len; i++) {  // going over sub-strings of various lengths including empty byte[].
+            byte[] b2 = Arrays.copyOfRange(b, i, b.length);
+            byte[] decodedBytes = ByteBufUtil.decodeHexDump(hexDump, i * 2, (len - i) * 2);
+            assertArrayEquals(b2, decodedBytes);
+        }
+    }
+
+    @Test(expected = IllegalArgumentException.class)
+    public void decodeHexDumpWithOddLength() {
+        ByteBufUtil.decodeHexDump("abc");
+    }
+
+    @Test(expected = IllegalArgumentException.class)
+    public void decodeHexDumpWithInvalidChar() {
+        ByteBufUtil.decodeHexDump("fg");
     }
 
     @Test
diff --git a/codec-http/src/test/java/io/netty/handler/codec/http/QueryStringDecoderTest.java b/codec-http/src/test/java/io/netty/handler/codec/http/QueryStringDecoderTest.java
index 68eb4b808c0..d0d5c1ce90e 100644
--- a/codec-http/src/test/java/io/netty/handler/codec/http/QueryStringDecoderTest.java
+++ b/codec-http/src/test/java/io/netty/handler/codec/http/QueryStringDecoderTest.java
@@ -205,8 +205,8 @@ public void testUrlDecoding() throws Exception {
             "%42",            "B",
             "%5f",            "_",
             "f%4",            "unterminated escape sequence at index 1 of: f%4",
-            "%x2",            "invalid escape sequence '%x2' at index 0 of: %x2",
-            "%4x",            "invalid escape sequence '%4x' at index 0 of: %4x",
+            "%x2",            "invalid hex byte 'x2' at index 1 of '%x2'",
+            "%4x",            "invalid hex byte '4x' at index 1 of '%4x'",
             "Caff%C3%A9",     caffe,
             "случайный праздник",               "случайный праздник",
             "случайный%20праздник",             "случайный праздник",