Repository: netty/netty
Issue: CharSequenceValueConverter.convertToByte works differently for AsciiString vs String

### Netty version
4.1

Looking at function CharSequenceValueConverter.convertToByte (CharSequence):
https://github.com/netty/netty/blob/4.1/codec/src/main/java/io/netty/handler/codec/CharSequenceValueConverter.java#L79

For AsciiString it takes the first byte, but for any other kind of CharSequence it parse the number value of the entire sequence, i.e. using Byte.parseByte(). This is surely not expected behavior.

See also:
https://docs.oracle.com/javase/8/docs/api/java/lang/Byte.html#parseByte-java.lang.String-

Issue comments:
@henrik-lindqvist yes this sounds like a bug...

@carl-mastrangelo @ejona86 @Scottmitch @trustin What you think is the correct behaviour here ?
I agree there is inconsistency here, and it is not just with `convertToByte` but also with other `convertTo` methods that have a special case for `AsciiString`. The `AsciiString` special case was done for performance reasons, but I think it is more correct to consider the entire value during conversion. We may be able to keep the performance optimization and enforce a max length check too if we want.

We should also be consistent with convertToChar [1] which doesn't have an `AsciiString` optimization but should follow the same semantics.

[1] https://github.com/netty/netty/blob/4.1/codec/src/main/java/io/netty/handler/codec/CharSequenceValueConverter.java#L88
My own 2cents: Delete it, or return null.   I don't think there is a reasonable default behavior.  Also, there is hardly anyone using this method, except `getByteAndRemove`, which is itself seldomly used.  Poking around on github's search shows no users.

ValueConverter makes no claims about what subclasses _should_ do, so who can say what will happen?  Further, if I had an instance of type ValueConverter, the implementation would be really important to know.   It cannot provide the abstraction it promises.
To be consistent with convertByte is should parse the number, not use the first byte. See: 
https://github.com/netty/netty/blob/4.1/codec/src/main/java/io/netty/handler/codec/CharSequenceValueConverter.java#L74
@henrik-lindqvist PTAL https://github.com/netty/netty/pull/7994

Pull request review comments:

Patch that resolved the issue:
diff --git a/codec/src/main/java/io/netty/handler/codec/CharSequenceValueConverter.java b/codec/src/main/java/io/netty/handler/codec/CharSequenceValueConverter.java
index b6510d1ff7e..1157af0623e 100644
--- a/codec/src/main/java/io/netty/handler/codec/CharSequenceValueConverter.java
+++ b/codec/src/main/java/io/netty/handler/codec/CharSequenceValueConverter.java
@@ -77,7 +77,7 @@ public CharSequence convertByte(byte value) {
 
     @Override
     public byte convertToByte(CharSequence value) {
-        if (value instanceof AsciiString) {
+        if (value instanceof AsciiString && value.length() == 1) {
             return ((AsciiString) value).byteAt(0);
         }
         return Byte.parseByte(value.toString());


Test patch:
diff --git a/codec/src/test/java/io/netty/handler/codec/CharSequenceValueConverterTest.java b/codec/src/test/java/io/netty/handler/codec/CharSequenceValueConverterTest.java
index 5543e2f9095..2347f0d0bf9 100644
--- a/codec/src/test/java/io/netty/handler/codec/CharSequenceValueConverterTest.java
+++ b/codec/src/test/java/io/netty/handler/codec/CharSequenceValueConverterTest.java
@@ -14,6 +14,7 @@
  */
 package io.netty.handler.codec;
 
+import io.netty.util.AsciiString;
 import org.junit.Test;
 
 import static org.junit.Assert.assertEquals;
@@ -30,6 +31,16 @@ public void testBoolean() {
         assertFalse(converter.convertToBoolean(converter.convertBoolean(false)));
     }
 
+    @Test
+    public void testByteFromAsciiString() {
+        assertEquals(127, converter.convertToByte(AsciiString.of("127")));
+    }
+
+    @Test(expected = NumberFormatException.class)
+    public void testByteFromEmptyAsciiString() {
+        converter.convertToByte(AsciiString.EMPTY_STRING);
+    }
+
     @Test
     public void testByte() {
         assertEquals(Byte.MAX_VALUE, converter.convertToByte(converter.convertByte(Byte.MAX_VALUE)));