Skip to content
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.

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

Don't sort JSON keys in the Web API #787

Merged
merged 2 commits into from
Dec 9, 2018
Merged

Conversation

fpagnoux
Copy link
Member

@fpagnoux fpagnoux commented Dec 3, 2018

By default, FLASK sorts the JSON keys of the API responses. This PR opts out of this behavior.

In some use-cases (merge with IPP parameters in France), having the same orders in API responses that in the parameter YAML files avoid extra work and boilerplate.


Edit by @maukoquiroga 2018-12-09:

I'm ok with the fact that we don't order an object defined as unordered by the JSON specification. However, as discussed in the comments, I omitted any mention of use-case from the motivation of this PR's introduced changes.

@fpagnoux
Copy link
Member Author

fpagnoux commented Dec 3, 2018

@Morendil this should allow to avoid the order keys in the IPP YAML parameters file.

Copy link
Member

@bonjourmauko bonjourmauko left a comment

Choose a reason for hiding this comment

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

Hi @fpagnoux, thanks for your contribution.

I'm convinced that, currently, this contribution is, or should be, backwards compatible: JSON schema isn't changed, and we can expect from users to do not expect JSON keys to be in any specific order, and to manage that by themselves.

Or, as I understand it, the use case described here implies that a client will be relying on a specific order of the served JSON keys.

That means that, from now on, any change to this behaviour will have to be considered de facto as a breaking-change.

So for now I'm personally against the merge of this contribution, and propose we close for the reasons exposed supra.

If you see any shortcomings on my reasoning, please challenge it. I'm also requesting other contributor's reviews to do so as well.

@bonjourmauko bonjourmauko requested a review from a team December 7, 2018 17:16
@fpagnoux
Copy link
Member Author

fpagnoux commented Dec 7, 2018

Or, as I understand it, the use case described here implies that a client will be relying on a specific order of the served JSON keys.
That means that, from now on, any change to this behaviour will have to be considered de facto as a breaking-change.

Well, not really. If a client of your library decide to use an internal method of a class, it doesn't mean that this method becomes part of your interface. It means that your clients are taking a risk, and that's on them ;).

I think NodeJS de facto preserves dictionary insertion order. That's however not part of their API contract. Some people rely on that, even if it's not advised. That's a risk they are taking. Node warns against doing that, but Node doesn't shuffle the dictionary order just for the sake of it. This is what we are doing right now, and this PR just suggests to stop doing so, without any promise to the API user.

I haven't written the changelog yet, but I'd phrase it like "Do not alphabetically sort JSON keys in Web API response". Once again, no promise, and no contract.

Does this address your concern ?

@bonjourmauko bonjourmauko force-pushed the preserve-json-order branch 2 times, most recently from b7dec8a to e27d221 Compare December 9, 2018 20:07
@bonjourmauko
Copy link
Member

@fpagnoux I edited the description

@bonjourmauko bonjourmauko merged commit fefca8b into master Dec 9, 2018
@bonjourmauko bonjourmauko deleted the preserve-json-order branch December 9, 2018 20:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants