From 8d3612f56a137da0d26b83d00507ff2f11bca9bb Mon Sep 17 00:00:00 2001 From: jheiv Date: Fri, 29 Jun 2018 02:55:02 -0400 Subject: [PATCH 1/8] Allow json(..., sort_keys=True) to handle mixed keys. (https://bugs.python.org/issue25457) --- Lib/json/encoder.py | 55 +++++++++++++---------- Modules/_json.c | 106 ++++++++++++++++++++++++++++---------------- 2 files changed, 99 insertions(+), 62 deletions(-) diff --git a/Lib/json/encoder.py b/Lib/json/encoder.py index fb083ed61bb1f8b..4c0fecd565c52cf 100644 --- a/Lib/json/encoder.py +++ b/Lib/json/encoder.py @@ -331,6 +331,30 @@ def _iterencode_list(lst, _current_indent_level): del markers[markerid] def _iterencode_dict(dct, _current_indent_level): + def _coerce_key(key): + if isinstance(key, str): + return key + # JavaScript is weakly typed for these, so it makes sense to + # also allow them. Many encoders seem to do something like this. + if isinstance(key, float): + # see comment for int/float in _make_iterencode + return _floatstr(key) + if key is True: + return 'true' + if key is False: + return 'false' + if key is None: + return 'null' + if isinstance(key, int): + # see comment for int/float in _make_iterencode + return _intstr(key) + + if _skipkeys: + return None + else: + raise TypeError(f'keys must be str, int, float, bool or None, ' + f'not {key.__class__.__name__}') + if not dct: yield '{}' return @@ -349,32 +373,17 @@ def _iterencode_dict(dct, _current_indent_level): newline_indent = None item_separator = _item_separator first = True + + # Coerce keys to strings (or None if _skipkeys) + items = ((_coerce_key(k), v) for (k,v) in dct.items()) + if _sort_keys: - items = sorted(dct.items(), key=lambda kv: kv[0]) - else: - items = dct.items() + items = sorted((k,v) for (k,v) in items if k is not None) + for key, value in items: - if isinstance(key, str): - pass - # JavaScript is weakly typed for these, so it makes sense to - # also allow them. Many encoders seem to do something like this. - elif isinstance(key, float): - # see comment for int/float in _make_iterencode - key = _floatstr(key) - elif key is True: - key = 'true' - elif key is False: - key = 'false' - elif key is None: - key = 'null' - elif isinstance(key, int): - # see comment for int/float in _make_iterencode - key = _intstr(key) - elif _skipkeys: + if key is None: + # If specified, skip keys that we weren't able to coerce to strings continue - else: - raise TypeError(f'keys must be str, int, float, bool or None, ' - f'not {key.__class__.__name__}') if first: first = False else: diff --git a/Modules/_json.c b/Modules/_json.c index 5a9464e34fb7f33..ca260ee9d3bc9ac 100644 --- a/Modules/_json.c +++ b/Modules/_json.c @@ -1562,6 +1562,7 @@ encoder_listencode_dict(PyEncoderObject *s, _PyAccu *acc, PyObject *ident = NULL; PyObject *it = NULL; PyObject *items; + PyObject *coerced_items; PyObject *item = NULL; Py_ssize_t idx; @@ -1607,55 +1608,81 @@ encoder_listencode_dict(PyEncoderObject *s, _PyAccu *acc, items = PyMapping_Items(dct); if (items == NULL) goto bail; - if (s->sort_keys && PyList_Sort(items) < 0) { - Py_DECREF(items); + + coerced_items = PyList_New(0); + it = PyObject_GetIter(items); + Py_DECREF(items); + if (it == NULL) + goto bail; + + while ((item = PyIter_Next(it)) != NULL) { + PyObject *key, *value, *coerced_item; + if (!PyTuple_Check(item) || PyTuple_GET_SIZE(item) != 2) { + PyErr_SetString(PyExc_ValueError, "items must return 2-tuples"); + goto bail; + } + key = PyTuple_GET_ITEM(item, 0); + if (PyUnicode_Check(key)) { + Py_INCREF(key); + kstr = key; + } + else if (PyFloat_Check(key)) { + kstr = encoder_encode_float(s, key); + if (kstr == NULL) + goto bail; + } + else if (key == Py_True || key == Py_False || key == Py_None) { + /* This must come before the PyLong_Check because + True and False are also 1 and 0.*/ + kstr = _encoded_const(key); + if (kstr == NULL) + goto bail; + } + else if (PyLong_Check(key)) { + kstr = PyLong_Type.tp_str(key); + if (kstr == NULL) { + goto bail; + } + } + else if (s->skipkeys) { + Py_DECREF(item); + continue; + } + else { + PyErr_Format(PyExc_TypeError, + "keys must be str, int, float, bool or None, " + "not %.100s", key->ob_type->tp_name); + goto bail; + } + + value = PyTuple_GET_ITEM(item, 1); + coerced_item = PyTuple_Pack(2, kstr, value); + if (coerced_item == NULL) { + goto bail; + } + /* Append instead of set because skipkeys=True may + "shrink" the number of items */ + if (-1 == PyList_Append(coerced_items, coerced_item)) + goto bail; + } + + if (s->sort_keys && PyList_Sort(coerced_items) < 0) { + Py_DECREF(coerced_items); goto bail; } - it = PyObject_GetIter(items); - Py_DECREF(items); + it = PyObject_GetIter(coerced_items); + Py_DECREF(coerced_items); if (it == NULL) goto bail; idx = 0; while ((item = PyIter_Next(it)) != NULL) { - PyObject *encoded, *key, *value; + PyObject *encoded, *value; if (!PyTuple_Check(item) || PyTuple_GET_SIZE(item) != 2) { PyErr_SetString(PyExc_ValueError, "items must return 2-tuples"); goto bail; } - key = PyTuple_GET_ITEM(item, 0); - if (PyUnicode_Check(key)) { - Py_INCREF(key); - kstr = key; - } - else if (PyFloat_Check(key)) { - kstr = encoder_encode_float(s, key); - if (kstr == NULL) - goto bail; - } - else if (key == Py_True || key == Py_False || key == Py_None) { - /* This must come before the PyLong_Check because - True and False are also 1 and 0.*/ - kstr = _encoded_const(key); - if (kstr == NULL) - goto bail; - } - else if (PyLong_Check(key)) { - kstr = PyLong_Type.tp_str(key); - if (kstr == NULL) { - goto bail; - } - } - else if (s->skipkeys) { - Py_DECREF(item); - continue; - } - else { - PyErr_Format(PyExc_TypeError, - "keys must be str, int, float, bool or None, " - "not %.100s", key->ob_type->tp_name); - goto bail; - } - + kstr = PyTuple_GET_ITEM(item, 0); + if (idx) { if (_PyAccu_Accumulate(acc, s->item_separator)) goto bail; @@ -1703,6 +1730,7 @@ encoder_listencode_dict(PyEncoderObject *s, _PyAccu *acc, Py_XDECREF(item); Py_XDECREF(kstr); Py_XDECREF(ident); + Py_XDECREF(coerced_items); return -1; } From 9ffe28bf519213efce4209740a8d5ba3b94482c6 Mon Sep 17 00:00:00 2001 From: jheiv Date: Fri, 29 Jun 2018 03:13:48 -0400 Subject: [PATCH 2/8] Tabs -> Spaces (woops) --- Modules/_json.c | 118 ++++++++++++++++++++++++------------------------ 1 file changed, 59 insertions(+), 59 deletions(-) diff --git a/Modules/_json.c b/Modules/_json.c index ca260ee9d3bc9ac..f319031129cba1a 100644 --- a/Modules/_json.c +++ b/Modules/_json.c @@ -1562,7 +1562,7 @@ encoder_listencode_dict(PyEncoderObject *s, _PyAccu *acc, PyObject *ident = NULL; PyObject *it = NULL; PyObject *items; - PyObject *coerced_items; + PyObject *coerced_items; PyObject *item = NULL; Py_ssize_t idx; @@ -1609,63 +1609,63 @@ encoder_listencode_dict(PyEncoderObject *s, _PyAccu *acc, if (items == NULL) goto bail; - coerced_items = PyList_New(0); - it = PyObject_GetIter(items); - Py_DECREF(items); - if (it == NULL) - goto bail; - - while ((item = PyIter_Next(it)) != NULL) { - PyObject *key, *value, *coerced_item; - if (!PyTuple_Check(item) || PyTuple_GET_SIZE(item) != 2) { - PyErr_SetString(PyExc_ValueError, "items must return 2-tuples"); - goto bail; - } - key = PyTuple_GET_ITEM(item, 0); - if (PyUnicode_Check(key)) { - Py_INCREF(key); - kstr = key; - } - else if (PyFloat_Check(key)) { - kstr = encoder_encode_float(s, key); - if (kstr == NULL) - goto bail; - } - else if (key == Py_True || key == Py_False || key == Py_None) { - /* This must come before the PyLong_Check because - True and False are also 1 and 0.*/ - kstr = _encoded_const(key); - if (kstr == NULL) - goto bail; - } - else if (PyLong_Check(key)) { - kstr = PyLong_Type.tp_str(key); - if (kstr == NULL) { - goto bail; - } - } - else if (s->skipkeys) { - Py_DECREF(item); - continue; - } - else { - PyErr_Format(PyExc_TypeError, - "keys must be str, int, float, bool or None, " - "not %.100s", key->ob_type->tp_name); - goto bail; - } - - value = PyTuple_GET_ITEM(item, 1); - coerced_item = PyTuple_Pack(2, kstr, value); - if (coerced_item == NULL) { - goto bail; - } - /* Append instead of set because skipkeys=True may - "shrink" the number of items */ - if (-1 == PyList_Append(coerced_items, coerced_item)) - goto bail; - } - + coerced_items = PyList_New(0); + it = PyObject_GetIter(items); + Py_DECREF(items); + if (it == NULL) + goto bail; + + while ((item = PyIter_Next(it)) != NULL) { + PyObject *key, *value, *coerced_item; + if (!PyTuple_Check(item) || PyTuple_GET_SIZE(item) != 2) { + PyErr_SetString(PyExc_ValueError, "items must return 2-tuples"); + goto bail; + } + key = PyTuple_GET_ITEM(item, 0); + if (PyUnicode_Check(key)) { + Py_INCREF(key); + kstr = key; + } + else if (PyFloat_Check(key)) { + kstr = encoder_encode_float(s, key); + if (kstr == NULL) + goto bail; + } + else if (key == Py_True || key == Py_False || key == Py_None) { + /* This must come before the PyLong_Check because + True and False are also 1 and 0.*/ + kstr = _encoded_const(key); + if (kstr == NULL) + goto bail; + } + else if (PyLong_Check(key)) { + kstr = PyLong_Type.tp_str(key); + if (kstr == NULL) { + goto bail; + } + } + else if (s->skipkeys) { + Py_DECREF(item); + continue; + } + else { + PyErr_Format(PyExc_TypeError, + "keys must be str, int, float, bool or None, " + "not %.100s", key->ob_type->tp_name); + goto bail; + } + + value = PyTuple_GET_ITEM(item, 1); + coerced_item = PyTuple_Pack(2, kstr, value); + if (coerced_item == NULL) { + goto bail; + } + /* Append instead of set because skipkeys=True may + "shrink" the number of items */ + if (-1 == PyList_Append(coerced_items, coerced_item)) + goto bail; + } + if (s->sort_keys && PyList_Sort(coerced_items) < 0) { Py_DECREF(coerced_items); goto bail; @@ -1730,7 +1730,7 @@ encoder_listencode_dict(PyEncoderObject *s, _PyAccu *acc, Py_XDECREF(item); Py_XDECREF(kstr); Py_XDECREF(ident); - Py_XDECREF(coerced_items); + Py_XDECREF(coerced_items); return -1; } From 07fb4d168bd2f24ef5b3745c1f2462bd5a6ed29b Mon Sep 17 00:00:00 2001 From: jheiv Date: Fri, 29 Jun 2018 03:33:49 -0400 Subject: [PATCH 3/8] Better mirror what _json.c now does, but use a yielding generator instead of creating a PyList object. --- Lib/json/encoder.py | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/Lib/json/encoder.py b/Lib/json/encoder.py index 4c0fecd565c52cf..7878a67e11b4fad 100644 --- a/Lib/json/encoder.py +++ b/Lib/json/encoder.py @@ -355,6 +355,13 @@ def _coerce_key(key): raise TypeError(f'keys must be str, int, float, bool or None, ' f'not {key.__class__.__name__}') + def _coerce_items(items): + for (k,v) in items: + k = _coerce_key(k) # Coerce or throw + if k is None: + continue + yield (k,v) + if not dct: yield '{}' return @@ -374,11 +381,11 @@ def _coerce_key(key): item_separator = _item_separator first = True - # Coerce keys to strings (or None if _skipkeys) - items = ((_coerce_key(k), v) for (k,v) in dct.items()) + # Coerce keys to strings + items = _coerce_items(dct.items()) if _sort_keys: - items = sorted((k,v) for (k,v) in items if k is not None) + items = sorted(items) for key, value in items: if key is None: From d94498ff5de612b2fa0499e9f6ec5c7e1fcd45e4 Mon Sep 17 00:00:00 2001 From: jheiv Date: Fri, 29 Jun 2018 03:33:49 -0400 Subject: [PATCH 4/8] Better mirror what _json.c now does, but use a yielding generator instead of creating a PyList object. --- Lib/json/encoder.py | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/Lib/json/encoder.py b/Lib/json/encoder.py index 4c0fecd565c52cf..03cab7e0178f5b9 100644 --- a/Lib/json/encoder.py +++ b/Lib/json/encoder.py @@ -355,6 +355,13 @@ def _coerce_key(key): raise TypeError(f'keys must be str, int, float, bool or None, ' f'not {key.__class__.__name__}') + def _coerce_items(items): + for (k,v) in items: + k = _coerce_key(k) # Coerce or throw + if k is None: + continue + yield (k,v) + if not dct: yield '{}' return @@ -374,16 +381,13 @@ def _coerce_key(key): item_separator = _item_separator first = True - # Coerce keys to strings (or None if _skipkeys) - items = ((_coerce_key(k), v) for (k,v) in dct.items()) + # Coerce keys to strings + items = _coerce_items(dct.items()) if _sort_keys: - items = sorted((k,v) for (k,v) in items if k is not None) + items = sorted(items) for key, value in items: - if key is None: - # If specified, skip keys that we weren't able to coerce to strings - continue if first: first = False else: From e659cad04faf19e8af1962c1b577c7f43512219f Mon Sep 17 00:00:00 2001 From: jheiv Date: Fri, 29 Jun 2018 13:06:42 -0400 Subject: [PATCH 5/8] Fix (?) negative refcount --- Modules/_json.c | 1 - 1 file changed, 1 deletion(-) diff --git a/Modules/_json.c b/Modules/_json.c index f319031129cba1a..6bdf8e2b08bac07 100644 --- a/Modules/_json.c +++ b/Modules/_json.c @@ -1730,7 +1730,6 @@ encoder_listencode_dict(PyEncoderObject *s, _PyAccu *acc, Py_XDECREF(item); Py_XDECREF(kstr); Py_XDECREF(ident); - Py_XDECREF(coerced_items); return -1; } From 9e52b24c7a89e07305e2f83f55b44b0aa988712b Mon Sep 17 00:00:00 2001 From: jheiv Date: Tue, 24 Mar 2020 04:31:50 -0400 Subject: [PATCH 6/8] update failing tests to handle mixed-key-type sorting, rm now obsolete 'unsortable' xfail test --- Lib/test/test_json/test_dump.py | 2 +- Lib/test/test_json/test_speedups.py | 3 --- 2 files changed, 1 insertion(+), 4 deletions(-) diff --git a/Lib/test/test_json/test_dump.py b/Lib/test/test_json/test_dump.py index 13b40020781bae3..e723e11621467f6 100644 --- a/Lib/test/test_json/test_dump.py +++ b/Lib/test/test_json/test_dump.py @@ -28,7 +28,7 @@ def test_encode_truefalse(self): '{"false": true, "true": false}') self.assertEqual(self.dumps( {2: 3.0, 4.0: 5, False: 1, 6: True}, sort_keys=True), - '{"false": 1, "2": 3.0, "4.0": 5, "6": true}') + '{"2": 3.0, "4.0": 5, "6": true, "false": 1}') # Issue 16228: Crash on encoding resized list def test_encode_mutated(self): diff --git a/Lib/test/test_json/test_speedups.py b/Lib/test/test_json/test_speedups.py index fbfee1a582095b7..87c6e896af11527 100644 --- a/Lib/test/test_json/test_speedups.py +++ b/Lib/test/test_json/test_speedups.py @@ -68,6 +68,3 @@ def test(name): self.assertRaises(ZeroDivisionError, test, 'allow_nan') self.assertRaises(ZeroDivisionError, test, 'sort_keys') - def test_unsortable_keys(self): - with self.assertRaises(TypeError): - self.json.encoder.JSONEncoder(sort_keys=True).encode({'a': 1, 1: 'a'}) From fe71ad623979d9bae7fcbe274d6645db37c11bb4 Mon Sep 17 00:00:00 2001 From: jheiv Date: Tue, 24 Mar 2020 04:39:47 -0400 Subject: [PATCH 7/8] rm trailing whitespace --- Lib/test/test_json/test_speedups.py | 1 - 1 file changed, 1 deletion(-) diff --git a/Lib/test/test_json/test_speedups.py b/Lib/test/test_json/test_speedups.py index 87c6e896af11527..ef1f073eeed55b6 100644 --- a/Lib/test/test_json/test_speedups.py +++ b/Lib/test/test_json/test_speedups.py @@ -67,4 +67,3 @@ def test(name): self.assertRaises(ZeroDivisionError, test, 'check_circular') self.assertRaises(ZeroDivisionError, test, 'allow_nan') self.assertRaises(ZeroDivisionError, test, 'sort_keys') - From 9fab4f3c5d87f704b41fe61c03d4dd2c56780fda Mon Sep 17 00:00:00 2001 From: Serhiy Storchaka Date: Sat, 5 Sep 2026 15:58:12 +0300 Subject: [PATCH 8/8] Apply batched suggestions from code review Co-authored-by: Kyle Stanley Co-authored-by: Serhiy Storchaka --- Modules/_json.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/Modules/_json.c b/Modules/_json.c index 52b1d99cdba10de..e87b02e3fc77c54 100644 --- a/Modules/_json.c +++ b/Modules/_json.c @@ -1574,7 +1574,10 @@ encoder_listencode_dict(PyEncoderObject *s, _PyAccu *acc, if (items == NULL) goto bail; - coerced_items = PyList_New(0); + coerced_items = PyList_New(0); + if (coerced_items == NULL) { + goto bail; + } it = PyObject_GetIter(items); Py_DECREF(items); if (it == NULL) @@ -1598,7 +1601,7 @@ encoder_listencode_dict(PyEncoderObject *s, _PyAccu *acc, } else if (key == Py_True || key == Py_False || key == Py_None) { /* This must come before the PyLong_Check because - True and False are also 1 and 0.*/ + True and False are also 1 and 0.*/ kstr = _encoded_const(key); if (kstr == NULL) goto bail;