gh-69643: Allow json.encode() to handle mixed keys when sort_keys=True - #8011
gh-69643: Allow json.encode() to handle mixed keys when sort_keys=True#8011jheiv wants to merge 11 commits into
Conversation
…tead of creating a PyList object.
…tead of creating a PyList object.
…keys that aren't strings This fixes one of the problems reported in ansible#49343 Upstream Python3 bug for the json traceback: https://bugs.python.org/issue25457 and PR that may fix it: python/cpython#8011
…keys that aren't strings This fixes one of the problems reported in #49343 Upstream Python3 bug for the json traceback: https://bugs.python.org/issue25457 and PR that may fix it: python/cpython#8011
…s dictionary keys that aren't strings This fixes one of the problems reported in ansible#49343 Upstream Python3 bug for the json traceback: https://bugs.python.org/issue25457 and PR that may fix it: python/cpython#8011. (cherry picked from commit c817bef) Co-authored-by: Toshio Kuratomi <a.badger@gmail.com>
…keys that aren't strings This fixes one of the problems reported in ansible#49343 Upstream Python3 bug for the json traceback: https://bugs.python.org/issue25457 and PR that may fix it: python/cpython#8011
serhiy-storchaka
left a comment
serhiy-storchaka
left a comment
There was a problem hiding this comment.
In principle looks good to me, bet the drawback of this PR is that it makes slower the common case, when all keys are strings.
How significant is the performance loss? I guess the question is whether or not providing the functionality to properly sort mixed keys is worth slowing down the sorting process in the common cases. Would it be reasonable to provide the users a kwarg for json.encode so that it provides this functionality optionally without slowing down the sorting process for the more common cases? This option would be |
|
Removing the stale label since #15691 was closed. This also seems to be more of an enhancement than a bugfix, but I think we should look into updating this PR and providing users an option to accurately sort mixed keys. |
aeros
added
type-feature
|
I'm the author of #15691, my modification of source code seemed to work for special case, but it didn't directly call json.dumps, it pass through jsonify passing through itsdangerous. Let's go up to the river : BuildingInfo_Names = {
'Wood': 1,
'Clay': 2,
'Iron': 3,
'Crop': 4,
...}add of reversed keys, for code simplification (in the __init__ of a module) (maybe not the best way ^^") for name, b_type in list(BuildingInfo_Names.items()):
BuildingInfo_Names[b_type] = namemy line of code causing the issue jsonify from flask (pallets) from itsdangerous import json as _json
...
def dumps(obj, app=None, **kwargs):
_dump_arg_defaults(kwargs, app=app)
encoding = kwargs.pop("encoding", None)
rv = _json.dumps(obj, **kwargs)
if encoding is not None and isinstance(rv, text_type):
rv = rv.encode(encoding)
return rv
...
def _dump_arg_defaults(kwargs, app=None):
"""Inject default arguments for dump functions."""
if app is None:
app = current_app
if app:
bp = app.blueprints.get(request.blueprint) if request else None
kwargs.setdefault(
"cls", bp.json_encoder if bp and bp.json_encoder else app.json_encoder
)
if not app.config["JSON_AS_ASCII"]:
kwargs.setdefault("ensure_ascii", False)
kwargs.setdefault("sort_keys", app.config["JSON_SORT_KEYS"])
else:
kwargs.setdefault("sort_keys", True)
kwargs.setdefault("cls", JSONEncoder)
...
def jsonify(*args, **kwargs):
indent = None
separators = (",", ":")
if current_app.config["JSONIFY_PRETTYPRINT_REGULAR"] or current_app.debug:
indent = 2
separators = (", ", ": ")
if args and kwargs:
raise TypeError("jsonify() behavior undefined when passed both args and kwargs")
elif len(args) == 1: # single args are passed directly to dumps()
data = args[0]
else:
data = args or kwargs
return current_app.response_class(
dumps(data, indent=indent, separators=separators) + "\n",
mimetype=current_app.config["JSONIFY_MIMETYPE"],
)itsdangerous (pallets) in __init__ : try:
import simplejson as json
except ImportError:
import json
class _CompactJSON(object):
"""Wrapper around json module that strips whitespace."""
@staticmethod
def loads(payload):
return json.loads(payload)
@staticmethod
def dumps(obj, **kwargs):
kwargs.setdefault("ensure_ascii", False)
kwargs.setdefault("separators", (",", ":"))
return json.dumps(obj, **kwargs)I hope the water was not too cold and that no one drowns ! nb : I have done the modification that I have submitted and it worked, but finally I have opted for putting True on app.config["JSON_SORT_KEYS"] for compatibility with default behavior |
|
@jheiv, please resolve the merge conflict and respond to the code reviews. Thank you! |
|
@csabella Sorry for the delay -- merge conflicts are now resolved and the failing tests have been updated to account for mixed-type-keys. Re @serhiy-storchaka 's concern about slowing down the common case, I agree and will follow up with some profiling tomorrow/this week so you guys can have an idea of the cost. |
|
Here are the results of some benchmarks:
def time_dumps(self, module, sort_keys, runs=5, runs_per_run=10, now=time.perf_counter_ns):
encoder = module.encoder.JSONEncoder(sort_keys=sort_keys)
encode = encoder.encode
times = []
for _ in range(runs):
t0 = now()
for _ in range(runs_per_run):
for obj in self.objects:
_ = encode(obj)
td = now() - t0
times.append(td)For each pickle file I "kept" the minimum time from each of the runs deltas (though mean showed similar results). I ran the test (loading the same pickled lists of dictionaries) for both the upstream master (without patch) and remote master (my fork, with patch). The results are... confusing -- with the c implementation seeming slightly faster with the changes and the python implementation seeming slightly slower (but all, in general, less than +/- 2%) If there's another method of benchmarking that's more appropriate here, I'd be happy to run them -- please let me know. |
|
-1 from me. IMO sorting mix keys is an anti-pattern that shouldn't be encouraged. This is doubly true on the encoding side when the user have complete control over the data going in. For the most part, it is a feature that sorting fails here. It is an indication that something is seriously wrong with the data modeling. |
|
This PR is stale because it has been open for 30 days with no activity. |
serhiy-storchaka
changed the title
github-actions
Bot
removed
the
stale
Co-authored-by: Kyle Stanley <aeros167@gmail.com> Co-authored-by: Serhiy Storchaka <storchaka@gmail.com>
|
Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool. If this change has little impact on Python users, wait for a maintainer to apply the |
Resolve the conflicts with the rewritten encoder_listencode_dict(): * Modules/_json.c -- the key conversion is factored out into encoder_coerce_key(), used both when encoding a key/value pair and by the new encoder_coerce_items(), which builds the list of converted pairs sorted in encoder_listencode_dict(). The critical sections and the borrowed reference fixes (pythongh-142831, pythongh-145244) are preserved. * Lib/json/encoder.py -- _make_iterencode() no longer has the _intstr parameter, use int.__repr__() instead. * test_speedups.py -- keep the new tests, drop test_unsortable_keys().
|
Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool. If this change has little impact on Python users, wait for a maintainer to apply the |
|
Thank you for the patch, and sorry that it took so long. Converting all keys to strings before sorting slows down the common case, when all keys are strings, and it changes the order of numeric keys: GH-156985 fixes the same issue without these drawbacks, so I am closing this in favour of it. Thank you for your contribution! |
10 participants
Note: This causes 3 regression tests to fail. The first two fail because the ordering of items when sort_keys=True is different than the hard coded expectation. The third failure is because specifying sort_keys=True and mixed keys no longer raises the expected TypeError:
BPO Issue: https://bugs.python.org/issue25457
https://bugs.python.org/issue25457