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

[cabo40]

SUMMARY

If encoding is not None, the default encoder utils.json_iso_dttm_ser is not used for binary data and instead it tries to encode to the default 'utf-8'.
For data exploration on tables with blob columns doing SELECT * will throw an error.
This fixes #13829

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

before:

after:

TEST PLAN

Try executing select decode('DEADBEEF', 'hex'); on the sample DB with async turned on.

ADDITIONAL INFORMATION

Asynchronous query execution must be enabled for the database.

• Has associated issue: [SQL Lab] UnicodeDecodeError when querying blob data and DB has asynchronous query execution enabled #13829
• Changes UI
• Includes DB Migration (follow approval process in SIP-59)
  • Migration is atomic, supports rollback & is backwards-compatible
  • Confirm DB migration upgrade and downgrade tested
  • Runtime estimates and downtime expectations provided
• Introduces new feature or API
• Removes existing feature or API

[codecov]

Codecov Report

Merging #13830 (02c4839) into master (18ff484) will increase coverage by 2.17%.
The diff coverage is 88.52%.

❗ Current head 02c4839 differs from pull request most recent head e1345db. Consider uploading reports for the commit e1345db to get more accurate results

@@            Coverage Diff             @@
##           master   #13830      +/-   ##
==========================================
+ Coverage   77.22%   79.40%   +2.17%     
==========================================
  Files         935      939       +4     
  Lines       47266    47541     +275     
  Branches     5893     5938      +45     
==========================================
+ Hits        36502    37750    +1248     
+ Misses      10616     9670     -946     
+ Partials      148      121      -27     

Flag	Coverage Δ
cypress	56.04% <66.00%> (-3.97%)	⬇️
hive	?
mysql	80.71% <87.30%> (+0.11%)	⬆️
postgres	80.74% <87.30%> (+0.11%)	⬆️
presto	80.46% <90.00%> (+0.16%)	⬆️
python	81.04% <90.38%> (-0.10%)	⬇️
sqlite	80.34% <87.30%> (+0.14%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
...rset-frontend/src/SqlLab/components/QueryTable.jsx	66.66% <ø> (ø)
...tend/src/SqlLab/components/ScheduleQueryButton.tsx	28.12% <ø> (ø)
...end/src/common/components/DropdownButton/index.tsx	24.00% <0.00%> (ø)
...frontend/src/components/DatabaseSelector/index.tsx	92.70% <ø> (+1.04%)	⬆️
...rset-frontend/src/components/ErrorMessage/types.ts	100.00% <ø> (ø)
superset-frontend/src/components/Icon/index.tsx	100.00% <ø> (ø)
superset-frontend/src/components/Loading/index.tsx	100.00% <ø> (ø)
...rset-frontend/src/components/ProgressBar/index.tsx	100.00% <ø> (ø)
...ontend/src/components/URLShortLinkButton/index.jsx	100.00% <ø> (ø)
.../components/CrossFilterScopingModal/utils/index.ts	100.00% <ø> (ø)
... and 244 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 18ff484...e1345db. Read the comment docs.

[zhaoyongjie]

/testenv up

[junlincc]

/testenv up

[zhaoyongjie]

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

[cabo40]

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 :)

[github-actions]

@zhaoyongjie Ephemeral environment creation is currently limited to committers.

[github-actions]

@junlincc Ephemeral environment spinning up at http://54.191.32.199:8080. Credentials are admin/admin. Please allow several minutes for bootstrapping and startup.

[zhaoyongjie]

Hi @cabo40
I turned on Async Query, and enabled SQL Lab result cache, then I did not reproduce this issue. Could you double-check this issue on the latest master?

[zhaoyongjie]

Hi @cabo40,

Sorry, please abort my earlier reply, I reproduced the problem, but it doesn't seem to be caused by the global async query. Here is my configuration for reproducing this issue

import os
from cachelib.redis import RedisCache


SQLALCHEMY_DATABASE_URI = 'postgresql://postgres:postgres@localhost/superset'

REDIS_HOST = os.environ.get("REDIS_HOST", "localhost")
REDIS_PORT = os.environ.get("REDIS_PORT", "6379")
REDIS_CELERY_DB = os.environ.get("REDIS_CELERY_DB", 2)
REDIS_RESULTS_DB = os.environ.get("REDIS_RESULTS_DB", 3)


class CeleryConfig(object):
    BROKER_URL = f"redis://{REDIS_HOST}:{REDIS_PORT}/{REDIS_CELERY_DB}"
    CELERY_IMPORTS = ("superset.sql_lab",)
    CELERY_RESULT_BACKEND = f"redis://{REDIS_HOST}:{REDIS_PORT}/{REDIS_RESULTS_DB}"
    CONCURRENCY = 1


CELERY_CONFIG = CeleryConfig
RESULTS_BACKEND = RedisCache(
    host=REDIS_HOST,
    port=REDIS_PORT,
    db=REDIS_RESULTS_DB,
)

[zhaoyongjie]

LGTM, thanks for this fix. please run

tox -e pre-commit

to fix PEP8

[github-actions]

Ephemeral environment shutdown and build artifacts deleted.