Removed pack-bytes and added some tests.

redis · chayim · Feb 2, 2023 · Jan 12, 2023 · Jan 14, 2023 · Jan 16, 2023
commit fdb131466833fb787938bfebc5dacc3a23d20020
diff --git a/hiredis/__init__.py b/hiredis/__init__.py
@@ -1,10 +1,9 @@
-from .hiredis import Reader, HiredisError, pack_bytes, pack_command, ProtocolError, ReplyError
+from .hiredis import Reader, HiredisError, pack_command, ProtocolError, ReplyError
 from .version import __version__
 
 __all__ = [
   "Reader",
   "HiredisError",
-  "pack_bytes",
   "pack_command",
   "ProtocolError",
   "ReplyError",

diff --git a/hiredis/hiredis.pyi b/hiredis/hiredis.pyi
@@ -1,8 +1,17 @@
 from typing import Any, Callable, Optional, Union, Tuple
 
-class HiredisError(Exception): ...
-class ProtocolError(HiredisError): ...
-class ReplyError(HiredisError): ...
+
+class HiredisError(Exception):
+    ...
+
+
+class ProtocolError(HiredisError):
+    ...
+
+
+class ReplyError(HiredisError):
+    ...
+
 
 class Reader:
     def __init__(
@@ -13,6 +22,7 @@ class Reader:
         errors: Optional[str] = ...,
         notEnoughData: Any = ...,
     ) -> None: ...
+
     def feed(
         self, __buf: Union[str, bytes], __off: int = ..., __len: int = ...
     ) -> None: ...
@@ -21,12 +31,10 @@ class Reader:
     def getmaxbuf(self) -> int: ...
     def len(self) -> int: ...
     def has_data(self) -> bool: ...
+
     def set_encoding(
         self, encoding: Optional[str] = ..., errors: Optional[str] = ...
     ) -> None: ...
 
 
-def pack_bytes(bytes: bytes): ...
-
-
-def pack_command(cmd: Tuple[str|int|float|bytes|memoryview]): ...
+def pack_command(cmd: Tuple[str | int | float | bytes | memoryview]): ...
diff --git a/src/hiredis.c b/src/hiredis.c
@@ -22,14 +22,7 @@ py_pack_command(PyObject* self, PyObject* cmd)
     return pack_command(cmd);
 }
 
-static PyObject*
-py_pack_bytes(PyObject* self, PyObject* cmd)
-{
-    return pack_bytes(cmd);
-}
-
-PyDoc_STRVAR(pack_command_doc, "Pack ...... ");
-PyDoc_STRVAR(pack_bytes_doc, "Pack ...... ");
+PyDoc_STRVAR(pack_command_doc, "Pack a series of arguments into the Redis protocol");
 
 PyMethodDef pack_command_method = {
     "pack_command",                 /* The name as a C string. */
@@ -38,16 +31,9 @@ PyMethodDef pack_command_method = {
     pack_command_doc,               /* The docstring as a C string. */
 };
 
-PyMethodDef pack_bytes_method = {
-    "pack_bytes",                 /* The name as a C string. */
-    (PyCFunction) py_pack_bytes,  /* The C function to invoke. */
-    METH_O,                         /* Flags telling Python how to invoke */
-    pack_bytes_doc,               /* The docstring as a C string. */
-};
 
 PyMethodDef methods[] = {
     {"pack_command", (PyCFunction) py_pack_command, METH_O, pack_command_doc},
-    {"pack_bytes", (PyCFunction) py_pack_bytes, METH_O, pack_bytes_doc},
     {NULL},
 };
 

diff --git a/src/pack.c b/src/pack.c
@@ -21,27 +21,27 @@ pack_command(PyObject *cmd)
     {
         return PyErr_NoMemory();
     }
+
     memset(tokens, 0, sizeof(sds) * tokens_number);
 
     size_t *lenghts = hi_malloc(sizeof(size_t) * tokens_number);
-    Py_ssize_t len = 0;
     if (lenghts == NULL)
     {
         sds_free(tokens);
         return PyErr_NoMemory();
     }
 
+    Py_ssize_t len = 0;
+
     for (Py_ssize_t i = 0; i < PyTuple_Size(cmd); i++)
     {
         PyObject *item = PyTuple_GetItem(cmd, i);
 
         if (PyBytes_Check(item))
         {
-            // Does it need to PyObject_CheckBuffer
+            char *bytes = NULL;
             Py_buffer buffer;
             PyObject_GetBuffer(item, &buffer, PyBUF_SIMPLE);
-            char *bytes = NULL;
-            // check result?
             PyBytes_AsStringAndSize(item, &bytes, &len);
             tokens[i] = sdsempty();
             tokens[i] = sdscpylen(tokens[i], bytes, len);
@@ -60,12 +60,6 @@ pack_command(PyObject *cmd)
             tokens[i] = sdsnewlen(bytes, len);
             lenghts[i] = len;
         }
-        // else if (PyByteArray_Check(item))
-        // {
-        //     //not tested. Is it supported
-        //     tokens[i] = sdsnewlen(PyByteArray_AS_STRING(item), PyByteArray_GET_SIZE(item));
-        //     lenghts[i] = PyByteArray_GET_SIZE(item);
-        // }
         else if (PyMemoryView_Check(item))
         {
             Py_buffer *p_buf = PyMemoryView_GET_BUFFER(item);
@@ -109,33 +103,4 @@ pack_command(PyObject *cmd)
     sdsfreesplitres(tokens, tokens_number);
     hi_free(lenghts);
     return result;
-}
-
-PyObject *
-pack_bytes(PyObject *cmd)
-{
-    assert(cmd);
-    if (cmd == NULL || !PyBytes_Check(cmd))
-    {
-        PyErr_SetString(PyExc_TypeError,
-                        "The argument must be bytes.");
-        return NULL;
-    }
-
-    char *obj_buf = NULL;
-    Py_ssize_t obj_len = 0;
-
-    if (PyBytes_AsStringAndSize(cmd, &obj_buf, &obj_len) == -1)
-    {
-        return NULL;
-    }
-
-    char *str_result = NULL;
-    obj_len = redisFormatCommand(&str_result, obj_buf);
-
-    PyObject *result = PyBytes_FromStringAndSize(str_result, obj_len);
-
-    hi_free(str_result);
-
-    return result;
-}
+}
diff --git a/src/pack.h b/src/pack.h
@@ -4,6 +4,5 @@
 #include <Python.h>
 
 extern PyObject* pack_command(PyObject* cmd);
-extern PyObject* pack_bytes(PyObject* bytes);
 
 #endif
diff --git a/tests/test_pack.py b/tests/test_pack.py
@@ -0,0 +1,24 @@
+import hiredis
+
+
+def test_basic():
+    cmd = ('HSET', 'foo', 'key', 'value1', b'key_b', b'bytes str', 'key_mv',
+           memoryview(b'bytes str'), b'key_i', 67, 'key_f', 3.14159265359)
+    expected_packed = b'*12\r\n$4\r\nHSET\r\n$3\r\nfoo\r\n$3\r\nkey\r\n$6\r\nvalue1\r\n$5\r\nkey_b\r\n$9\r\nbytes str\r\n$6\r\nkey_mv\r\n$9\r\nbytes str\r\n$5\r\nkey_i\r\n$2\r\n67\r\n$5\r\nkey_f\r\n$13\r\n3.14159265359\r\n'
+    packed_cmd = hiredis.pack_command(cmd)
+    assert packed_cmd == expected_packed
+
+
+def test_spaces_in_cmd():
+    cmd = ('COMMAND', 'GETKEYS', 'EVAL',
+           'return {KEYS[1],KEYS[2],ARGV[1],ARGV[2]}', 2, 'key1', 'key2', 'first', 'second')
+    expected_packed = b'*9\r\n$7\r\nCOMMAND\r\n$7\r\nGETKEYS\r\n$4\r\nEVAL\r\n$40\r\nreturn {KEYS[1],KEYS[2],ARGV[1],ARGV[2]}\r\n$1\r\n2\r\n$4\r\nkey1\r\n$4\r\nkey2\r\n$5\r\nfirst\r\n$6\r\nsecond\r\n'
+    packed_cmd = hiredis.pack_command(cmd)
+    assert packed_cmd == expected_packed
+
+
+def test_null_chars():
+    cmd = ('SET', 'a', bytes(b'\xaa\x00\xffU'))
+    expected_packed = b'*3\r\n$3\r\nSET\r\n$1\r\na\r\n$4\r\n\xaa\x00\xffU\r\n'
+    packed_cmd = hiredis.pack_command(cmd)
+    assert packed_cmd == expected_packed