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.

Sign up for GitHub

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

fix: Use utils.json_iso_dttm_ser to dump jsons when async query execution #13830

Merged
merged 2 commits into from
Apr 19, 2021
Merged

fix: Use utils.json_iso_dttm_ser to dump jsons when async query execution #13830

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0a5752f
Use utils.json_iso_dttm_ser to dump jsons
cabo40 Mar 27, 2021
e1345db
Change to comply with tox -m pre-commit
cabo40 Apr 10, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion superset/views/core.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2211,7 +2211,8 @@ def results_exec( # pylint: disable=too-many-return-statements
obj = apply_display_max_row_limit(obj, rows)

return json_success(
json.dumps(obj, default=utils.json_iso_dttm_ser, ignore_nan=True)
json.dumps(obj, default=utils.json_iso_dttm_ser, ignore_nan=True,
encoding=None)
Copy link
Member

Choose a reason for hiding this comment

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

If we don't specify the encoding type, I guess json.dumps might use ASCII encoding, which might be a problem for Asian characters.

https://github.com/simplejson/simplejson/blob/8bef979ad8272cbc2903970f4b9992f603d50973/simplejson/encoder.py#L275-L302

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi!
Thanks for checking the PR. I looked into the encoding and it seems that if a utf-8 string is in the json as a binary string, it will pass through

superset/superset/utils/core.py

Line 550 in b5c95c5

return obj.decode("utf-8")
and get encoded into utf-8, and if the string is a str it will go through https://github.com/simplejson/simplejson/blob/8bef979ad8272cbc2903970f4b9992f603d50973/simplejson/encoder.py#L51 and also get encoded into utf-8.
There are two other places where the dump is done with encoding=None:

superset/superset/views/core.py

Line 2302 in b5c95c5

encoding=None,

superset/superset/views/core.py

Line 2428 in b5c95c5

encoding=None,

I also think that the other calls to json.dumps with default=utils.* should have encoding set to None.
I did a brief test of the change with non ASCII strings and they were correctly displayed.
Is there some other test you think could help mitigate the risk of querying non ASCII chars?
Cheers :)
image

)

@has_access_api
Expand Down