WIP Harmonize serialization between aiohttp and flask #1095
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Rename _jsonify_data to _serialize_data to make its purpose easier to understand (this was also known as _cast_body in aiohttp_api). In exploring how to harmonize json serialization between aiothttp and flask, we needed to be able to adjust the mimetype from within _serialize_data. Harmonizing the actual serialization has to wait until backwards incompatible changes can be made, but we can keep the new interface, as these functions were introduced in this PR (spec-first#849).
Also adds a couple of missing get_response tests.
The old aiohttp_api._cast_body did not jsonify when mimetype was None. The old flask_api._jsonify_data jsonified data, or raised a TypeError if the data could not be jsonified, even when mimetype was not None and when mimetype was not a JSON mimetype. This unifies the behavior so that only JSON or None mimetypes can trigger jsonification, and only JSON mimetypes will raise a TypeError if the data is not jsonifiable. Non-JSON mimetypes and non-jsonifiable data with None mimetype are stringified. This means that the data can serialize itself when - mimetype is None and data is not jsonifiable - mimetype is not a JSON mimetype This removes the flask_api version of _jsonify_data. This should be backwards compatible because: For the non-jsonifiable data, when mimetype is None or a non-json type, using str() instead of throwing TypeError is an improvement. Could anyone depend on throwing a TypeError in this case? It would just throw 500 errors, but this makes that more useful. So it is a "new feature". The only gotcha is when mimetype is not None and is not JSON, but the data is jsonifiable, we should NOT serialize it as JSON, but today we are for flask but not for aiohttp. Instead flask_api should just cast it as a string (like aiohttp does) assuming that if someone wanted a non-json type, they will handle serialization, possibly by subclassing dict with their own__str__ implementation or something. This is probably a bugfix (and therefore ok to introduce now) because jsonifying data with a non-json mimetype with flask does not make sense, even if the data is jsonifiable.
This was referenced Dec 7, 2019
This was referenced Dec 13, 2019
Member
|
Some of these changes have been merged as part of other PRs and others are no longer relevant after #1491. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Was part of #849. Please refer to the long chain of discussion there for background behind this PR.
Please note that many of the commits here come from #849. Once that is merged the remaining commits will be easier to review.
@dtkav and I discussed quite a bit about what the right interface would be for users so that it is not surprising when things are or are not serialized as json. But, harmonizing them ends up breaking tests for aiohttp, flask, or both depending on which set of assumptions we prefer.
So, we would like for the interface to be more sane and jsonify data for both text/plain and application/json or if mimetype is not defined. But we'd also like to prevent jsonification for mimetypes where that does not make sense. Eventually,
_serialize_data()should become pluggable, possibly where a chain of serializers can vote on which mimetypes they can handle.