Skip to content

Commit

Permalink
reader: partly restore the previous behaviour
Browse files Browse the repository at this point in the history 
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 <[email protected]>
  • Loading branch information
@mkmkme
mkmkme committed Aug 2, 2024
1 parent 2578712 commit a9c45d6
Show file tree
Hide file tree
Showing 4 changed files with 72 additions and 125 deletions.
2 changes: 1 addition & 1 deletion libvalkey/libvalkey.pyi
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ class Reader:
encoding: Optional[str] = ...,
errors: Optional[str] = ...,
notEnoughData: Any = ...,
listOnly: bool = ...,
convertSetsToLists: bool = ...,
) -> None: ...

def feed(
Expand Down
154 changes: 54 additions & 100 deletions src/reader.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 },
Expand All @@ -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 */
};

Expand Down Expand Up @@ -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;
}
Expand Down Expand Up @@ -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);
}
Expand Down Expand Up @@ -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)
Expand All @@ -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);
}
Expand All @@ -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);
Expand Down Expand Up @@ -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;
}
2 changes: 1 addition & 1 deletion src/reader.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ typedef struct {
PyObject *protocolErrorClass;
PyObject *replyErrorClass;
PyObject *notEnoughDataObject;
int listOnly;
int convertSetsToLists;

PyObject *pendingObject;

Expand Down
39 changes: 16 additions & 23 deletions tests/test_reader.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand Down Expand Up @@ -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()
Expand Down

0 comments on commit a9c45d6

Please sign in to comment.