bpo-25457: Remove sorting bug in json._iterencode_dict

[yoann9344]

Remove int / str keys sorting error due to dict with integers and strings keys
(TypeError: '<' not supported between instances of 'int' and 'str')
Using str() for each key could slow significantly the treatment !

nb : I don't want to put more than 10 mins on this little issue, the process seems heavy to me to pull it in the right way

https://bugs.python.org/issue38046

[the-knights-who-say-ni]

Hello, and thanks for your contribution!

I'm a bot set up to make sure that the project can legally accept your contribution by verifying you have signed the PSF contributor agreement (CLA).

Unfortunately we couldn't find an account corresponding to your GitHub username on bugs.python.org (b.p.o) to verify you have signed the CLA (this might be simply due to a missing "GitHub Name" entry in your b.p.o account settings). This is necessary for legal reasons before we can look at your contribution. Please follow the steps outlined in the CPython devguide to rectify this issue.

You can check yourself to see if the CLA has been received.

Thanks again for your contribution, we look forward to reviewing it!

[tirkarthi]

Though keys are converted to string in the below for loop some cases need to be special cased like False -> "false" . Directly using str would convert False -> "False" in similar manner for other cases as seen in test failures. In Python 2 str and int can be compared and hence sort comparison would have worked which is not the case in Python 3.

There is also an open issue for this with a PR on allowing sort_keys for mixed keys. Please see discussion at https://bugs.python.org/issue25457.

[aeros]

Thanks for the PR @yoann9344.

Since this is a behavioral change, I would highly recommend opening an issue for this over on https://bugs.python.org. We pretty much always require it for any code related changes that aren't backports. This will likely be where the experts of json will be able to provide feedback, and they can determine whether this behavior is intentional or if it is a bug and whether or not it should be addressed.

Also, I would recommend adjusting the title to something that more directly points at the section being modified. Something along the lines of Remove sorting bug in json._iterencode_dict would be a significant improvement. This will help attract attention from reviewers that are experienced with the json module.

It would also be helpful if you could provide a brief code snippet which causes the TypeError to be raised. This will assist in determining whether the behavior is intentional or not.

[aeros]

@tirkarthi

There is also an open issue for this with a PR on allowing sort_keys for mixed keys. Please see discussion at https://bugs.python.org/issue25457.

From what I can tell, the scope of the PR attached to issue 25457 seems to be significantly wider. While the discussion is still relevant to this PR, I think this could probably be opened in a separate issue (even if the other PR may address the issue in which this PR is attempting to solve). Particularly since that issue has not had any activity in the last year.

[yoann9344]

Though keys are converted to string in the below for loop some cases need to be special cased like False -> "false" . Directly using str would convert False -> "False" in similar manner for other cases as seen in test failures. In Python 2 str and int can be compared and hence sort comparison would have worked which is not the case in Python 3.

There is also an open issue for this with a PR on allowing sort_keys for mixed keys. Please see discussion at https://bugs.python.org/issue25457.

My idea is not have a good sorting, but at least remove exceptions.
Here comes the idea we could use try: sorted(..) except(TypeError): sorted(...str(..))
with that we can keep usual behavior
I don't know that much this json library but I think the best way to let user handle comportment as he wants to, would be to use OrderedDict or let user pass a method to sorted. But global design skills are required to make a choice.

[yoann9344]

Thanks for the PR @yoann9344.

Since this is a behavioral change, I would highly recommend opening an issue for this over on https://bugs.python.org. We pretty much always require it for any code related changes that aren't backports. This will likely be where the experts of json will be able to provide feedback, and they can determine whether this behavior is intentional or if it is a bug and whether or not it should be addressed.

Also, I would recommend adjusting the title to something that more directly points at the section being modified. Something along the lines of Remove sorting bug in json._iterencode_dict would be a significant improvement. This will help attract attention from reviewers that are experienced with the json module.

It would also be helpful if you could provide a brief code snippet which causes the TypeError to be raised. This will assist in determining whether the behavior is intentional or not.

I'm not good with these procedural things then I hope I haven't did weird mistakes :
file 48596 created
msg 351268 created
issue 38046 created

I have attached one python file example

[aeros]

@yoann9344:

I'm not good with these procedural things then I hope I haven't did weird mistakes :
file 48596 created
msg 351268 created
issue 38046 created

Thanks and no problem. Some of the process/workflow components can take a bit to get used to. Feel free to ask me any questions if any part of it isn't clear or doesn't make sense. A resource I would highly recommend is the devguide. In particular, the Lifecycle of a PR page provides a good summary of the PR process.

In order to properly link the PR to the issue, the PR title has to begin with "bpo-<issue_number>", in this case it would be "bpo-38046". This would be a good time to make the title change I recommended earlier as well. Including the bpo header, the new title for the PR would be "bpo-38046: Remove sorting bug in json._iterencode_dict".

Edit: After reading the bpo comments, it looks like this issue was already opened as bpo-25457. You can attach your PR to that issue by changing the PR name to "bpo-25457: Remove sorting bug in json._iterencode_dict".

The PR (#8011) currently open for bpo-25457 looks to be outdated and was not passing the CI tests, so it's okay to attach this PR.

[serhiy-storchaka]

It does not work correctly for True, False, None and some int and float subclasses (like IntEnum).

[yoann9344]

Yes, indeed it doesn't work. I have no idea how to correct it.

[aeros]

@yoann9344:

Yes, indeed it doesn't work. I have no idea how to correct it.

#8011 looks like a work in progress that might need to updated. You might be able to help with moving along this implementation by providing feedback or opening a PR based on it if the author of that one is unresponsive.

[yoann9344]

@yoann9344:

Yes, indeed it doesn't work. I have no idea how to correct it.

#8011 looks like a work in progress that might need to updated. You might be able to help with moving along this implementation by providing feedback or opening a PR based on it if the author of that one is unresponsive.

I'll put whole code involved in the #8011. Because my modification was working for my special case, but it was not directly json, it was jsonify, passing through flask then through itsdangerous. I hope give the code will help !