Change the way sets and dicts are handled in the library #2
Conversation
In some (rare) cases, the sets from a Redis response contain nested types that are not hashable in Python, for example maps. To handle these cases uniformly, always return Python lists for Redis sets. The elements will still be unique, relying on the correctness of data arriving from the server.
Previous commit changed the way sets were handled. Since sets cannot be used as dict values, they have been replaced as lists internally. There is, however, the opposite side of the issue: sets or arrays being used as *keys* in the dict. It is absolutely valid RESP3-wise, but invalid from Python perspective as sets and lists are not hashable. This commit changes the way dicts are handled by making them lists of tuples instead. The library client is supposed to handle the conversion into proper types themselves. As previous commit, this commit introduces a breaking change. Signed-off-by: Mikhail Koviazin <[email protected]>
|
huh, apparently PyPy works in a slightly different. Investigating. |
|
When you say RESP3 dict, it's a RESP3 map, right? (Dict is a Python term, and a Valkey internal datastructure.) Even if RESP3 allows complex map keys and set elements, I don't think Valkey ever returns values like that. Hm... COMMAND seems to be the exception. How about some special handling just for COMMAND, replacing the set with a list only for COMMAND's response? I guess modules can return stuff like that too. We could possibly forbid modules from doing that in Valkey. That would be a breaking change too, but I think could be good to forbid values like that, not only for Python but also JavaScript and probably other languages too. Alternatives:
|
| PyObject *t = PyTuple_New(2); | ||
| PyTuple_SET_ITEM(t, 0, obj); | ||
| PyTuple_SET_ITEM(t, 1, Py_None); | ||
| PyList_Append(parent, t); |
There was a problem hiding this comment.
If I'm not mistaken there is a memleak here. You need a Py_DECREF after append.
AFAIK the API exposed by libvalkey-py is that of a simple RESP2/3 parser. With no knowledge about commands etc. I think the best way to go is:
And we will just keep it always on in valkey-py. |
|
Thanks @aiven-sal and @zuiderkwast ! I'll fix it accordingly |
@aiven-sal so this only affects the communication beween valkey-py and libvalkey-py? It does not affect how valkey-py returns the replies to the end users? |
No, it's only about communication between valkey-py and libvalkey-py. By "client" here I meant exactly valkey-py :) There won't be any breaking changes in valkey-py as it's meant to be widely-used |
|
I think I'll close this one in favour of creating a new one from scratch, as the proposed changes would require me to change even a commit that doesn't belong to me. |
This pull request backports yet unmerged PR from hiredis and additionally changes the way dicts are handled.
This changes are necessary because RESP3 allows:
None of these things are allowed in Python. Therefore it was decided to replace sets with lists and dicts with lists of tuples. The client is supposed to handle the types and convert if necessary themselves.
This is a breaking change