From a9c45d648eb781d434920cb0ee73c8e0cf432828 Mon Sep 17 00:00:00 2001 From: Mikhail Koviazin Date: Fri, 2 Aug 2024 14:44:33 +0200 Subject: [PATCH] reader: partly restore the previous behaviour Version 3.0.0 changed the way sets and maps are handled in libvalkey-py. Now, all the sets and dicts were treated as lists, maps being lists of tuples. This change was done without keeping `valkey-py` in mind. As it turned out, this way of handling maps is not really applicable to `valkey-py` because `valkey-py` doesn't know what data type it expects. Hence, it can't tell whether the returned list should be treated as a map or left as it is. This commit partly reverts the behaviour to what it was pre-3.0.0, leaving maps as dicts all the time. All sets, though, are being treated as lists now when `convertSetsToLists=True`. While this behaviour excludes some exotic use cases allowed by RESP like set or array being a key of a map, it does not affect the consumer library as much, and none of such exotic use cases are being used in the real world anyway. Signed-off-by: Mikhail Koviazin --- libvalkey/libvalkey.pyi | 2 +- src/reader.c | 154 ++++++++++++++-------------------------- src/reader.h | 2 +- tests/test_reader.py | 39 +++++----- 4 files changed, 72 insertions(+), 125 deletions(-) diff --git a/libvalkey/libvalkey.pyi b/libvalkey/libvalkey.pyi index c5b7271..f165f36 100644 --- a/libvalkey/libvalkey.pyi +++ b/libvalkey/libvalkey.pyi @@ -21,7 +21,7 @@ class Reader: encoding: Optional[str] = ..., errors: Optional[str] = ..., notEnoughData: Any = ..., - listOnly: bool = ..., + convertSetsToLists: bool = ..., ) -> None: ... def feed( diff --git a/src/reader.c b/src/reader.c index 4ca77d2..2ccab96 100644 --- a/src/reader.c +++ b/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,7 +311,7 @@ static int Reader_init(libvalkey_ReaderObject *self, PyObject *args, PyObject *k "encoding", "errors", "notEnoughData", - "listOnly", + "convertSetsToLists", NULL, }; PyObject *protocolErrorClass = NULL; @@ -365,10 +319,10 @@ static int Reader_init(libvalkey_ReaderObject *self, PyObject *args, PyObject *k 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, ¬EnoughData, &listOnly)) + &protocolErrorClass, &replyErrorClass, &encoding, &errors, ¬EnoughData, &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; } diff --git a/src/reader.h b/src/reader.h index e52a27a..e6e0a99 100644 --- a/src/reader.h +++ b/src/reader.h @@ -13,7 +13,7 @@ typedef struct { PyObject *protocolErrorClass; PyObject *replyErrorClass; PyObject *notEnoughDataObject; - int listOnly; + int convertSetsToLists; PyObject *pendingObject; diff --git a/tests/test_reader.py b/tests/test_reader.py index 0bbbb97..338f3b9 100644 --- a/tests/test_reader.py +++ b/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()