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

fix(warm_up_cache): JSON serialization #23569

Conversation

john-bodley
Copy link
Member

@john-bodley john-bodley commented Apr 3, 2023

SUMMARY

A byproduct of #23012, when warming a non-legacy chart it's valid for the error to be a babel form of Empty query? which is not JSON serializable, i.e., json.dumps(...) raises the following error:

TypeError: Object of type LazyString is not JSON serializable

The fix to to use the the base_json_conv serialization method. Having json.dump(...) splattered throughout the code is rather unfortunate as there's no consistency in how payload are serialized. Ideally exceptions should be raised and handled in a uniform way per SIP-41.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

Added unit test.

ADDITIONAL INFORMATION

  • Has associated issue:
  • Required feature flags:
  • 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

@mock.patch.object(ChartDataCommand, "run")
def test_warm_up_cache_error(self, run_mock) -> None:
self.login()
slc = self.get_slice("Pivot Table v2", db.session)
Copy link
Member Author

@john-bodley john-bodley Apr 3, 2023

Choose a reason for hiding this comment

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

Another sad byproduct of having multiple ways of doing things. The Empty query? is handled differently depending on whether or not the slice has a query context defined as it seems other paths wont raise, i.e., it seems like only the ChartDataCommand exception will contain the LazyString whereas the others have likely been stringified via the str(...) function.

@codecov
Copy link

codecov bot commented Apr 3, 2023

Codecov Report

Merging #23569 (398e6aa) into master (f036adb) will decrease coverage by 11.17%.
The diff coverage is 0.00%.

❗ Current head 398e6aa differs from pull request most recent head 1e00f46. Consider uploading reports for the commit 1e00f46 to get more accurate results

@@             Coverage Diff             @@
##           master   #23569       +/-   ##
===========================================
- Coverage   68.24%   57.07%   -11.17%     
===========================================
  Files        1956     1956               
  Lines       75589    75589               
  Branches     8223     8223               
===========================================
- Hits        51582    43144     -8438     
- Misses      21899    30337     +8438     
  Partials     2108     2108               
Flag Coverage Δ
hive ?
postgres ?
presto 53.29% <0.00%> (ø)
python 59.62% <0.00%> (-23.09%) ⬇️
unit 53.41% <0.00%> (ø)

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

Impacted Files Coverage Δ
superset/views/core.py 36.47% <0.00%> (-38.58%) ⬇️

... and 304 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@john-bodley john-bodley requested review from villebro and removed request for etr2460 April 3, 2023 23:12
@john-bodley john-bodley force-pushed the john-bodley--fix-dump-cache-warmup-result branch from ebe555e to 1e00f46 Compare May 23, 2023 15:39
@alexryndin
Copy link

I tried this patch with superset 2.1.0, unfortunately it didn't help, legacy charts don't properly show error message, endlessly showing loading gif, whereas worker show this in its log:

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "/ss_venv/lib64/python3.8/site-packages/celery/app/trace.py", line 477, in trace_task
    R = retval = fun(*args, **kwargs)
  File "/ss_venv/lib64/python3.8/site-packages/superset/initialization/__init__.py", line 108, in __call__
    return task_base.__call__(self, *args, **kwargs)
  File "/ss_venv/lib64/python3.8/site-packages/celery/app/trace.py", line 760, in __protected_call__
    return self.run(*args, **kwargs)
  File "/ss_venv/lib64/python3.8/site-packages/superset/tasks/async_queries.py", line 95, in load_chart_data_into_cache
    async_query_manager.update_job(
  File "/ss_venv/lib64/python3.8/site-packages/superset/utils/async_query_manager.py", line 187, in update_job
    event_data = {"data": json.dumps({**job_metadata, **updates})}
  File "/usr/lib64/python3.8/json/__init__.py", line 231, in dumps
    return _default_encoder.encode(obj)
  File "/usr/lib64/python3.8/json/encoder.py", line 199, in encode
    chunks = self.iterencode(o, _one_shot=True)
  File "/usr/lib64/python3.8/json/encoder.py", line 257, in iterencode
    return _iterencode(o, 0)
  File "/usr/lib64/python3.8/json/encoder.py", line 179, in default
    raise TypeError(f'Object of type {o.__class__.__name__} '
TypeError: Object of type LazyString is not JSON serializable

@john-bodley
Copy link
Member Author

Closing in favor of #24671.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants