Fix str/bytes mixup in PythonParser.read_response() (#1324)

Calling str() on a bytes object can result in a BytesWarning being
emitted and usually indicates a mixup between byte and string handling.

Now, in the event of an invalid RESP response, use the repr value of the
raw response in the exception message.

Can further simplify the bytes/str handling by comparing the first byte
as a bytes object instead of converting it to str. The bytes literal is
available on all supported Pythons. This removes the need for the
compatibility function, byte_to_chr().

3 files changed, 26 insertions, 18 deletions

diff --git a/redis/_compat.py b/redis/_compat.py
index 2a4b2b9..e4cc34c 100644
--- a/redis/_compat.py
+++ b/redis/_compat.py
@@ -143,9 +143,6 @@ if sys.version_info[0] < 3:
     def next(x):
         return x.next()
 
-    def byte_to_chr(x):
-        return x
-
     unichr = unichr
     xrange = xrange
     basestring = basestring
@@ -166,9 +163,6 @@ else:
     def itervalues(x):
         return iter(x.values())
 
-    def byte_to_chr(x):
-        return chr(x)
-
     def nativestr(x):
         return x if isinstance(x, str) else x.decode('utf-8', 'replace')
 
diff --git a/redis/connection.py b/redis/connection.py
index bdc1d2c..a13bcd3 100755
--- a/redis/connection.py
+++ b/redis/connection.py
@@ -10,7 +10,7 @@ import sys
 import threading
 import warnings
 
-from redis._compat import (xrange, imap, byte_to_chr, unicode, long,
+from redis._compat import (xrange, imap, unicode, long,
                            nativestr, basestring, iteritems,
                            LifoQueue, Empty, Full, urlparse, parse_qs,
                            recv, recv_into, unquote, BlockingIOError,
@@ -319,18 +319,17 @@ class PythonParser(BaseParser):
         return self._buffer and self._buffer.can_read(timeout)
 
     def read_response(self):
-        response = self._buffer.readline()
-        if not response:
+        raw = self._buffer.readline()
+        if not raw:
             raise ConnectionError(SERVER_CLOSED_CONNECTION_ERROR)
 
-        byte, response = byte_to_chr(response[0]), response[1:]
+        byte, response = raw[:1], raw[1:]
 
-        if byte not in ('-', '+', ':', '$', '*'):
-            raise InvalidResponse("Protocol Error: %s, %s" %
-                                  (str(byte), str(response)))
+        if byte not in (b'-', b'+', b':', b'$', b'*'):
+            raise InvalidResponse("Protocol Error: %r" % raw)
 
         # server returned an error
-        if byte == '-':
+        if byte == b'-':
             response = nativestr(response)
             error = self.parse_error(response)
             # if the error is a ConnectionError, raise immediately so the user
@@ -343,19 +342,19 @@ class PythonParser(BaseParser):
             # necessary, so just return the exception instance here.
             return error
         # single value
-        elif byte == '+':
+        elif byte == b'+':
             pass
         # int value
-        elif byte == ':':
+        elif byte == b':':
             response = long(response)
         # bulk response
-        elif byte == '$':
+        elif byte == b'$':
             length = int(response)
             if length == -1:
                 return None
             response = self._buffer.read(length)
         # multi-bulk response
-        elif byte == '*':
+        elif byte == b'*':
             length = int(response)
             if length == -1:
                 return None
diff --git a/tests/test_connection.py b/tests/test_connection.py
new file mode 100644
index 0000000..5ca9254
--- /dev/null
+++ b/tests/test_connection.py
@@ -0,0 +1,15 @@
+import mock
+import pytest
+
+from redis.exceptions import InvalidResponse
+from redis.utils import HIREDIS_AVAILABLE
+
+
+@pytest.mark.skipif(HIREDIS_AVAILABLE, reason='PythonParser only')
+def test_invalid_response(r):
+    raw = b'x'
+    parser = r.connection._parser
+    with mock.patch.object(parser._buffer, 'readline', return_value=raw):
+        with pytest.raises(InvalidResponse) as cm:
+            parser.read_response()
+    assert str(cm.value) == 'Protocol Error: %r' % raw