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().

Author: jdufresne

redis/_compat.py

@@ -143,9 +143,6 @@ def nativestr(x):
     def next(x):
         return x.next()
 
-    def byte_to_chr(x):
-        return x
-
     unichr = unichr
     xrange = xrange
     basestring = basestring
@@ -166,9 +163,6 @@ def iterkeys(x):
     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')
 

redis/connection.py

@@ -10,7 +10,7 @@
 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 @@ def can_read(self, timeout):
         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 @@ def read_response(self):
             # 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

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