Merge pull request #16 from valkey-io/mkmkme/fix-return-value

reader: partly restore the previous behaviour

Author: Mikhail Koviazin

libvalkey/libvalkey.pyi

@@ -21,7 +21,7 @@ class Reader:
         encoding: Optional[str] = ...,
         errors: Optional[str] = ...,
         notEnoughData: Any = ...,
-        listOnly: bool = ...,
+        convertSetsToLists: bool = ...,
     ) -> None: ...
 
     def feed(

src/reader.c

@@ -14,7 +14,7 @@ static PyObject *Reader_getmaxbuf(libvalkey_ReaderObject *self);
 static PyObject *Reader_len(libvalkey_ReaderObject *self);
 static PyObject *Reader_has_data(libvalkey_ReaderObject *self);
 static PyObject *Reader_set_encoding(libvalkey_ReaderObject *self, PyObject *args, PyObject *kwds);
-static PyObject *Reader_listOnly(PyObject *self, void *closure);
+static PyObject *Reader_convertSetsToLists(PyObject *self, void *closure);
 
 static PyMethodDef libvalkey_ReaderMethods[] = {
     {"feed", (PyCFunction)Reader_feed, METH_VARARGS, NULL },
@@ -28,7 +28,7 @@ static PyMethodDef libvalkey_ReaderMethods[] = {
 };
 
 static PyGetSetDef libvalkey_ReaderGetSet[] = {
-    {"listOnly", (getter)Reader_listOnly, NULL, NULL, NULL},
+    {"convertSetsToLists", (getter)Reader_convertSetsToLists, NULL, NULL, NULL},
     {NULL}  /* Sentinel */
 };
 
@@ -73,88 +73,46 @@ PyTypeObject libvalkey_ReaderType = {
     Reader_new,                   /*tp_new */
 };
 
-static int tryParentize_impl(const valkeyReadTask *task, PyObject *obj,
-                             PyObject *parent, libvalkey_ReaderObject *self) {
-    switch (task->parent->type) {
-        case VALKEY_REPLY_MAP:
-            if (task->idx % 2 == 0) {
-                /* Save the object as a key. */
-                self->pendingObject = obj;
-            } else {
-                if (self->pendingObject == NULL) {
-                    Py_DECREF(obj);
-                    return -1;
-                }
-                if (PyDict_SetItem(parent, self->pendingObject, obj) < 0) {
-                    Py_DECREF(obj);
-                    Py_DECREF(self->pendingObject);
+static void *tryParentize(const valkeyReadTask *task, PyObject *obj) {
+    libvalkey_ReaderObject *self = (libvalkey_ReaderObject*)task->privdata;
+    if (task && task->parent) {
+        PyObject *parent = (PyObject*)task->parent->obj;
+        switch (task->parent->type) {
+            case VALKEY_REPLY_MAP:
+                if (task->idx % 2 == 0) {
+                    /* Save the object as a key. */
+                    self->pendingObject = obj;
+                } else {
+                    if (self->pendingObject == NULL) {
+                        Py_DECREF(obj);
+                        return NULL;
+                    }
+                    if (PyDict_SetItem(parent, self->pendingObject, obj) < 0) {
+                        Py_DECREF(obj);
+                        Py_DECREF(self->pendingObject);
+                        self->pendingObject = NULL;
+                        return NULL;
+                    }
                     self->pendingObject = NULL;
-                    return -1;
                 }
-                self->pendingObject = NULL;
-            }
-            break;
-        case VALKEY_REPLY_SET:
-            assert(PyAnySet_CheckExact(parent));
-            if (PySet_Add(parent, obj) < 0) {
-                Py_DECREF(obj);
-                return -1;
-            }
-            break;
-        default:
-            assert(PyList_CheckExact(parent));
-            if (PyList_SetItem(parent, task->idx, obj) < 0) {
-                Py_DECREF(obj);
-                return -1;
-            }
-    }
-    return 0;
-}
-
-static int tryParentize_ListOnly(const valkeyReadTask *task, PyObject *obj,
-                                 PyObject *parent, libvalkey_ReaderObject *self) {
-    switch (task->parent->type) {
-        case VALKEY_REPLY_MAP:
-            if (task->idx % 2 == 0) {
-                /* Set a temporary item to save the object as a key. */
-                self->pendingObject = PyTuple_New(2);
-                if (self->pendingObject == NULL) {
-                    Py_DECREF(obj);
-                    return -1;
+                break;
+            case VALKEY_REPLY_SET:
+                if (!self->convertSetsToLists) {
+                    assert(PyAnySet_CheckExact(parent));
+                    if (PySet_Add(parent, obj) < 0) {
+                        Py_DECREF(obj);
+                        return NULL;
+                    }
+                    break;
                 }
-                PyTuple_SET_ITEM(self->pendingObject, 0, obj);
-            } else {
-                if (self->pendingObject == NULL) {
+                /* fallthrough */
+            default:
+                assert(PyList_CheckExact(parent));
+                if (PyList_SetItem(parent, task->idx, obj) < 0) {
                     Py_DECREF(obj);
-                    return -1;
+                    return NULL;
                 }
-                PyTuple_SET_ITEM(self->pendingObject, 1, obj);
-                int res = PyList_Append(parent, self->pendingObject);
-                Py_DECREF(self->pendingObject);
-                self->pendingObject = NULL;
-                if (res < 0)
-                    return -1;
-            }
-            break;
-        default:
-            assert(PyList_CheckExact(parent));
-            if (PyList_SetItem(parent, task->idx, obj) < 0) {
-                Py_DECREF(obj);
-                return -1;
-            }
-    }
-    return 0;
-}
-
-static void *tryParentize(const valkeyReadTask *task, PyObject *obj) {
-    libvalkey_ReaderObject *self = (libvalkey_ReaderObject*)task->privdata;
-    if (task && task->parent) {
-        PyObject *parent = (PyObject*)task->parent->obj;
-        int res = self->listOnly
-            ? tryParentize_ListOnly(task, obj, parent, self)
-            : tryParentize_impl(task, obj, parent, self);
-        if (res < 0)
-            return NULL;
+        }
     }
     return obj;
 }
@@ -224,22 +182,18 @@ static void *createStringObject(const valkeyReadTask *task, char *str, size_t le
 static void *createArrayObject(const valkeyReadTask *task, size_t elements) {
     libvalkey_ReaderObject *self = (libvalkey_ReaderObject*)task->privdata;
     PyObject *obj;
-    if (self->listOnly) {
-        /* For map, don't preallocate listand use append later. */
-        if (task->type == VALKEY_REPLY_MAP)
-            elements = 0;
-        obj = PyList_New(elements);
-    } else {
-        switch (task->type) {
-            case VALKEY_REPLY_MAP:
-                obj = PyDict_New();
-                break;
-            case VALKEY_REPLY_SET:
+    switch (task->type) {
+        case VALKEY_REPLY_MAP:
+            obj = PyDict_New();
+            break;
+        case VALKEY_REPLY_SET:
+            if (!self->convertSetsToLists) {
                 obj = PySet_New(NULL);
                 break;
-            default:
-                obj = PyList_New(elements);
-        }
+            }
+            /* fallthrough */
+        default:
+            obj = PyList_New(elements);
     }
     return tryParentize(task, obj);
 }
@@ -357,18 +311,18 @@ static int Reader_init(libvalkey_ReaderObject *self, PyObject *args, PyObject *k
         "encoding",
         "errors",
         "notEnoughData",
-        "listOnly",
+        "convertSetsToLists",
         NULL,
     };
     PyObject *protocolErrorClass = NULL;
     PyObject *replyErrorClass = NULL;
     PyObject *notEnoughData = NULL;
     char *encoding = NULL;
     char *errors = NULL;
-    int listOnly = 0;
+    int convertSetsToLists = 0;
 
     if (!PyArg_ParseTupleAndKeywords(args, kwds, "|OOzzOp", kwlist,
-        &protocolErrorClass, &replyErrorClass, &encoding, &errors, &notEnoughData, &listOnly))
+        &protocolErrorClass, &replyErrorClass, &encoding, &errors, &notEnoughData, &convertSetsToLists))
             return -1;
 
     if (protocolErrorClass)
@@ -386,7 +340,7 @@ static int Reader_init(libvalkey_ReaderObject *self, PyObject *args, PyObject *k
         Py_INCREF(self->notEnoughDataObject);
     }
 
-    self->listOnly = listOnly;
+    self->convertSetsToLists = convertSetsToLists;
 
     return _Reader_set_encoding(self, encoding, errors);
 }
@@ -406,7 +360,7 @@ static PyObject *Reader_new(PyTypeObject *type, PyObject *args, PyObject *kwds)
         self->protocolErrorClass = LIBVALKEY_STATE->VkErr_ProtocolError;
         self->replyErrorClass = LIBVALKEY_STATE->VkErr_ReplyError;
         self->pendingObject = NULL;
-        self->listOnly = 0;
+        self->convertSetsToLists = 0;
         Py_INCREF(self->protocolErrorClass);
         Py_INCREF(self->replyErrorClass);
         Py_INCREF(self->notEnoughDataObject);
@@ -549,9 +503,9 @@ static PyObject *Reader_set_encoding(libvalkey_ReaderObject *self, PyObject *arg
 
 }
 
-static PyObject *Reader_listOnly(PyObject *obj, void *closure) {
+static PyObject *Reader_convertSetsToLists(PyObject *obj, void *closure) {
     libvalkey_ReaderObject *self = (libvalkey_ReaderObject*)obj;
-    PyObject *result = PyBool_FromLong(self->listOnly);
+    PyObject *result = PyBool_FromLong(self->convertSetsToLists);
     Py_INCREF(result);
     return result;
 }

src/reader.h

@@ -13,7 +13,7 @@ typedef struct {
     PyObject *protocolErrorClass;
     PyObject *replyErrorClass;
     PyObject *notEnoughDataObject;
-    int listOnly;
+    int convertSetsToLists;
 
     PyObject *pendingObject;
 

tests/test_reader.py

@@ -3,7 +3,7 @@
 
 @pytest.fixture(params=[True, False])
 def reader(request):
-  return libvalkey.Reader(listOnly=request.param)
+  return libvalkey.Reader(convertSetsToLists=request.param)
 
 # def reply():
 #   return reader.gets()
@@ -138,55 +138,48 @@ def test_set(reader):
     reader.feed(b"~3\r\n+tangerine\r\n_\r\n,10.5\r\n")
     expected = (
       [b"tangerine", None, 10.5]
-      if reader.listOnly
+      if reader.convertSetsToLists
       else {b"tangerine", None, 10.5}
     )
     assert expected == reader.gets()
 
 def test_dict(reader):
   reader.feed(b"%2\r\n+radius\r\n,4.5\r\n+diameter\r\n:9\r\n")
-  expected = (
-      [(b"radius", 4.5), (b"diameter", 9)]
-      if reader.listOnly
-      else {b"radius": 4.5, b"diameter": 9}
-  )
-  assert expected == reader.gets()
+  assert {b"radius": 4.5, b"diameter": 9} == reader.gets()
 
 def test_set_with_nested_dict(reader):
   reader.feed(b"~2\r\n+tangerine\r\n%1\r\n+a\r\n:1\r\n")
-  if reader.listOnly:
-    assert [b"tangerine", [(b"a", 1)]] == reader.gets()
+  if reader.convertSetsToLists:
+    assert [b"tangerine", {b"a": 1}] == reader.gets()
   else:
     with pytest.raises(TypeError):
       reader.gets()
 
 def test_dict_with_nested_set(reader):
   reader.feed(b"%1\r\n+a\r\n~2\r\n:1\r\n:2\r\n")
-  expected = (
-      [(b"a", [1, 2])]
-      if reader.listOnly
-      else {b"a": {1, 2}}
-  )
+  expected: dict[bytes, set[int] | list[int]] = {b"a": {1, 2}}
+  if reader.convertSetsToLists:
+    expected[b"a"] = list(expected[b"a"])
   assert expected == reader.gets()
 
 def test_map_inside_list(reader):
   reader.feed(b"*1\r\n%1\r\n+a\r\n:1\r\n")
-  expected = (
-      [[(b"a", 1)]]
-      if reader.listOnly
-      else [{b"a": 1}]
-  )
-  assert expected == reader.gets()
+  assert [{b"a": 1}] == reader.gets()
 
 def test_map_inside_set(reader):
   reader.feed(b"~1\r\n%1\r\n+a\r\n:1\r\n")
-  if reader.listOnly:
-    assert [[(b"a", 1)]] == reader.gets()
+  if reader.convertSetsToLists:
+    assert [{b"a": 1}] == reader.gets()
   else:
     # Map inside set is not allowed in Python
     with pytest.raises(TypeError):
       reader.gets()
 
+def test_set_as_map_key(reader):
+  reader.feed(b"%1\r\n~1\r\n:1\r\n:2\r\n")
+  with pytest.raises(TypeError):
+    reader.gets()
+
 def test_vector(reader):
   reader.feed(b">4\r\n+pubsub\r\n+message\r\n+channel\r\n+message\r\n")
   assert [b"pubsub", b"message", b"channel", b"message"] == reader.gets()