Skip to content

add logic to convert dict list key to tuple #633

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 5 commits into from
Closed

add logic to convert dict list key to tuple #633

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
419fe6c
add logic to convert dict list key to tuple
jensbjorgensen May 12, 2025
fbaa8ed
fix formatting
jensbjorgensen May 12, 2025
30b0c6c
add pure python logic
jensbjorgensen May 12, 2025
887f102
else if
jensbjorgensen May 15, 2025
08400e0
use type() is list for exact checking
jensbjorgensen May 15, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions msgpack/fallback.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -529,6 +529,8 @@ def _unpack(self, execute=EX_CONSTRUCT):
raise ValueError("%s is not allowed for map key" % str(type(key)))
if isinstance(key, str):
key = sys.intern(key)
if isinstance(key, list):
key = tuple(key)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is tuple the only data type that is valid as a dict key and that would come back as a list after pack/unpack?

Nitpick: use elif

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So you could conceivably subtype list and then add eq and hash to make it eligible as a key, is that what you mean?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suppose that could be changed to type(key) is list instead of using isinstance? How do you like that?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah I think that makes a lot of sense, if type(key) is list then the ret[key] = after that will definitely throw

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ThomasWaldmann are you ok with type(key) is list ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what I meant is research which data types msgpack will serialize into something that comes back as a list after pack/unpack.

because if that is more than just tuple and you are always casting it to a tuple, it is something else than it was before packing.

as you are only doing it in dict key context, it can't be a list, because that would not be valid as a dict key, but maybe it could be sth else?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, that is true. To speak concretely a value to be used as a key in a dict must be hashable, so it's easy to sub-type list and define hash and eq and make it valid as a key. Consider the following:

from __future__ import annotations
import msgpack

class MyList(list):
    def __hash__(self) -> int:
        return self[0]

    def __eq__(self, rhs: MyList) -> bool:
        return self[0] == rhs[0]

l0 = MyList((1, 2, 3, 4))
l1 = MyList((1, 2, 3, 4, 5))
d = {l0: 'one through four'}
d[l1] = 'one through five'
print(d)

data = msgpack.packb(d)
print(msgpack.unpackb(data)) #exception occurs here

The way I'm thinking about this though is that if you as the programmer have used an object which gets serialized as a list and you haven't already done something yourself to make sure when it is unpacked it gets unpacked as something that is not simply "list" then you will get an exception. One way you could've gotten around this already would be to define a pairs_hook. I thought the above would be more elegant. I guess the one scenario I could think of that my proposed change would break is if they had defined their own dict object that was willing to accept a plain list as a key. Would it be more palatable if I actually just went ahead and tried to make the assignment, check for an exception due to it not being a suitable key value and then convert it to a list?

ret[key] = self._unpack(EX_CONSTRUCT)
if self._object_hook is not None:
ret = self._object_hook(ret)
Expand Down
19 changes: 19 additions & 0 deletions msgpack/unpack.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -199,6 +199,25 @@ static inline int unpack_callback_map_item(unpack_user* u, unsigned int current,
if (PyUnicode_CheckExact(k)) {
PyUnicode_InternInPlace(&k);
}
if (PyList_CheckExact(k)) {
Py_ssize_t list_size = PyList_Size(k);
PyObject* tuple = PyTuple_New(list_size);

if (tuple == NULL) {
return -1;
}

for (Py_ssize_t i = 0; i < list_size; i++) {
PyObject* item = PyList_GetItem(k, i);
Py_INCREF(item);
if (PyTuple_SetItem(tuple, i, item) != 0) {
Py_DECREF(tuple);
return -1;
}
}
Py_DECREF(k);
k = tuple;
}
if (u->has_pairs_hook) {
msgpack_unpack_object item = PyTuple_Pack(2, k, v);
if (!item)
Expand Down
4 changes: 4 additions & 0 deletions test/test_case.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -134,3 +134,7 @@ def test_match():

def test_unicode():
assert unpackb(packb("foobar"), use_list=1) == "foobar"


def test_dict_tuple_key():
unpackb(packb({(1, 2): 3}), strict_map_key=False)
Loading