revert(#17570): fix dashboard editing for dashboards with non-ascii characters

[serenajiang]

SUMMARY

The bug: When a user saved a dashboard that contained a chart with non-ascii characters, the json_metadata became malformed, causing the dashboard to fail to load. The root cause is in #17570, which added positions into the json_metadata blob. Something went wrong in the json parsing which causes json_metadata to be truncated if the positions json contains emojis or other non-standard characters.

The root cause PR is pretty large, so we had trouble figuring out where this goes wrong. I suspect the fix forward is some combination of

• Fix json parsing to handle non-ascii characters correctly
• Validate that json_metadata is well-formed before writing it
• Don't write the positions json, since it's already contained by a different column

Opening this revert PR as a starting point. This PR reverts #17570 and the accompanying #17707.

TESTING INSTRUCTIONS

to reproduce bug:

• make dashboard
• add chart with emoji in title
• save dashboard
• see error after save
• refresh and see error on load

to verify this fix:

• make dashboard
• add chart with emoji in title
• save dashboard
• verify dashboard saved
• refresh and view dashboard

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

@geido @etr2460 @graceguo-supercat @rusackas

[graceguo-supercat]

LGTM.

I think we need to revert related PR from master ASAP. After our recent release, all the newly updated dashboards, their position_json are inserted into json_metadata. It's troublesome to clean this up.

[codecov]

Codecov Report

Merging #17753 (282a29c) into master (63d9693) will decrease coverage by 0.01%.
The diff coverage is 73.07%.

@@            Coverage Diff             @@
##           master   #17753      +/-   ##
==========================================
- Coverage   67.77%   67.75%   -0.02%     
==========================================
  Files        1602     1602              
  Lines       64151    64066      -85     
  Branches     6773     6764       -9     
==========================================
- Hits        43476    43407      -69     
+ Misses      18822    18812      -10     
+ Partials     1853     1847       -6     

Flag	Coverage Δ
hive	81.77% <96.87%> (-0.01%)	⬇️
javascript	54.88% <68.00%> (+0.03%)	⬆️
mysql	82.14% <96.87%> (?)
postgres	82.20% <96.87%> (-0.01%)	⬇️
presto	?
python	82.54% <96.87%> (-0.11%)	⬇️
sqlite	81.88% <96.87%> (-0.01%)	⬇️

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

Impacted Files	Coverage Δ
...frontend/src/dashboard/components/Header/index.jsx	59.44% <0.00%> (-2.10%)	⬇️
...et-frontend/src/dashboard/components/SaveModal.tsx	50.00% <0.00%> (-1.17%)	⬇️
...uperset-frontend/src/utils/getClientErrorObject.ts	71.87% <ø> (ø)
superset/dashboards/commands/update.py	83.07% <ø> (-0.75%)	⬇️
superset/dashboards/schemas.py	99.40% <ø> (-0.01%)	⬇️
...t-frontend/src/dashboard/actions/dashboardState.js	35.33% <35.71%> (-0.46%)	⬇️
...src/dashboard/components/PropertiesModal/index.jsx	76.98% <76.98%> (ø)
superset/dashboards/dao.py	96.09% <96.77%> (-0.18%)	⬇️
superset/dashboards/api.py	91.86% <100.00%> (-0.03%)	⬇️
superset/db_engine_specs/presto.py	84.34% <0.00%> (-5.64%)	⬇️
... and 4 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 63d9693...282a29c. Read the comment docs.

[kgabryje]

@serenajiang I can't reproduce the issue with the steps that you shared on current master (07bbe84). Is there any other way to trigger the bug?

Screen.Recording.2021-12-15.at.10.57.37.mov

[kgabryje]

Confirmed with @zhaoyongjie and @villebro that they can't repro either 🙁

[geido]

@serenajiang @graceguo-supercat as hard as I tried I could not reproduce either. Do you have any additional information to help with reproducing the problem?

Please watch the video below. If we can reproduce it a fix might be a more desirable way to solve this problem and it might be as quick.

DEV.Box.plot.mp4

[graceguo-supercat]

• for simple dashboard, once you edit dashboard and save it, you will see position_json column is inserted into json_metadata column, this is pretty easy to reproduce
• for large dashboard:

1. set SUPERSET_DASHBOARD_POSITION_DATA_LIMIT = 16777215
2. create a large dashboard with charts and tabs, add emojis in tabs
3. make dashboard position_json data is > 65k (if you don't have config in 1, you can not save dashboard larger than 65k).
4. edit dashboard, or save_as.

[geido]

Hello @graceguo-supercat Erik gave us a hint that the limit might be the problem as you just correctly pointed out. I have released a PR #17766 that removes the positions from the metadata as they are unnecessarily saved and tagged you there. Thank you 🙏

[serenajiang]

@geido @kgabryje We initially thought the size was the problem too, but we realized that it was emojis from repro with small dashboard. More specific repro instructions:

1. create dashboard
2. add chart to dashboard and edit the chart title (in the dashboard) to contain an emoji, OR add a header with an emoji in it
3. save dashboard

Removing positions should fix this problem, since afaik, there aren't any other ways for emojis to get in. Thanks for the PR!