fix: Remove positions from json_metadata #17766

geido · 2021-12-15T16:30:33Z

SUMMARY

This PR is a follow-up of PR #17570 in response to the issue that was brought in revert-PR #17753.
The changes in PR #17570 saved the positions in the json_metadata of a Dashboard even if that wasn't really necessary. It appears that very large dashboards might encounter an error as the positions might become too large AND/OR non-ascii characters might cause the json parsing to fail.

This PR forcefully removes the positions from the metadata, while making sure that all the related logics remain in place. It also introduces a temporary fix to make sure that Dashboards currently affected by this issue should ignore the positions in their metadata entirely.

TESTING INSTRUCTIONS

Open a very large Dashboard
Edit and Save the changes
Make sure everything works as expected

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 · 2021-12-15T16:31:57Z

Confirmation that the issue is resolved will be very much appreciated. CC @serenajiang @etr2460 @graceguo-supercat

kgabryje · 2021-12-15T16:35:45Z

superset/dashboards/dao.py

@@ -238,6 +238,8 @@ def set_dash_metadata(  # pylint: disable=too-many-locals
                if int(key) in slice_ids
            }
            md["default_filters"] = json.dumps(applicable_filters)
+        # positions have its own column, no need to store it in metadata
+        md.pop("positions", None)


Maybe we should do a migration that removes positions from metadata instead of popping it in dao?

positions still need to be passed to the set_dash_metadata function as certain logics are happening here that use it. What it does not need is to be stored as its own object in the metadata, that's why it's now being popped out from the metadata object.

In general, the set_dash_metadata function has some pretty mixed logic and it would require a bigger refactor. Things like the title, the css, and other data that is unrelated to the dashboard metadata is being used here when it shouldn't. Let's plan for a full refactor. CC @rusackas

Thanks for clarification!

superset-frontend/src/dashboard/components/PropertiesModal/index.tsx

…ex.tsx Co-authored-by: Kamil Gabryjelski <[email protected]>

….com/geido/incubator-superset into fix/remove_positions_in_jsonmetadata

geido · 2021-12-15T16:49:09Z

superset-frontend/src/dashboard/components/PropertiesModal/index.tsx

+
+      // temporary fix to remove positions from dashboards' metadata
+      if (metadata?.positions) {
+        delete metadata.positions;


This is to ensure that dashboards that are being affected by an excessively large positions object can work properly. This check can be likely removed in the near future.

sorry, why would we be able to remove this in the future? or actually, why do we need this at all if we're removing the attribute in the backend below?

@etr2460 all the dashboards that have been saved after the changes were introduced have the positions object appearing in their metadata inside the properties modal. I could not reproduce the error in the first place, so I wasn't sure whether that might also cause any problem. This just ensures it won't ever appear in the metadata to the user. I think it can be removed in the future assuming most dashboards will be then be updated and their positions object removed by the backend when saved.

i see, so this is an attempt to fix up the potentially broken json_metadata. makes sense, thanks!

kgabryje

Code looks good to me, tested and everything works as before (which is good). Let's wait for confirmation that it solves the issue since I couldn't reproduce it.

etr2460 · 2021-12-15T17:21:53Z

/testenv up

etr2460 · 2021-12-15T17:23:03Z

spinning up a testenv here to both try and repro the bug and to see if this resolves the issue we were seeing with doubling the position json in the database

github-actions · 2021-12-15T17:43:54Z

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

serenajiang · 2021-12-15T18:20:19Z

Confirmed fix 🥳

etr2460

Thanks for testing @serenajiang. let's ship it!

rusackas

Code looks fine to me, and tested empirically - I wasn't able to break anything! 🥳

github-actions · 2021-12-15T18:32:56Z

Ephemeral environment shutdown and build artifacts deleted.

* Remove positions from json_metadata * Update superset-frontend/src/dashboard/components/PropertiesModal/index.tsx Co-authored-by: Kamil Gabryjelski <[email protected]> * Indent Co-authored-by: Kamil Gabryjelski <[email protected]> (cherry picked from commit 274fb37)

* Remove positions from json_metadata * Update superset-frontend/src/dashboard/components/PropertiesModal/index.tsx Co-authored-by: Kamil Gabryjelski <[email protected]> * Indent Co-authored-by: Kamil Gabryjelski <[email protected]>

Remove positions from json_metadata

2c168e9

superset-github-bot bot added the Flexiana label Dec 15, 2021

pull-request-size bot added the size/XS label Dec 15, 2021

geido requested review from etr2460, graceguo-supercat, rusackas, serenajiang, kgabryje and villebro December 15, 2021 16:30

kgabryje reviewed Dec 15, 2021

View reviewed changes

superset-frontend/src/dashboard/components/PropertiesModal/index.tsx Outdated Show resolved Hide resolved

geido and others added 3 commits December 15, 2021 17:41

Update superset-frontend/src/dashboard/components/PropertiesModal/ind…

2b309f9

…ex.tsx Co-authored-by: Kamil Gabryjelski <[email protected]>

Indent

0018642

Merge branch 'fix/remove_positions_in_jsonmetadata' of https://github…

37bbc01

….com/geido/incubator-superset into fix/remove_positions_in_jsonmetadata

pull-request-size bot added size/S and removed size/XS labels Dec 15, 2021

geido commented Dec 15, 2021

View reviewed changes

kgabryje approved these changes Dec 15, 2021

View reviewed changes

geido mentioned this pull request Dec 15, 2021

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

Closed

9 tasks

etr2460 approved these changes Dec 15, 2021

View reviewed changes

rusackas approved these changes Dec 15, 2021

View reviewed changes

rusackas merged commit 274fb37 into apache:master Dec 15, 2021

serenajiang mentioned this pull request Dec 16, 2021

fix(dashboard): commit update once #17781

Merged

eschutho added the v1.4 label Dec 20, 2021

mistercrunch added 🍒 1.4.0 🍒 1.4.1 🍒 1.4.2 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 1.5.0 labels Mar 13, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Remove positions from json_metadata #17766

fix: Remove positions from json_metadata #17766

geido commented Dec 15, 2021 •

edited

Loading

geido commented Dec 15, 2021

kgabryje Dec 15, 2021 •

edited

Loading

geido Dec 15, 2021 •

edited

Loading

kgabryje Dec 15, 2021

geido Dec 15, 2021 •

edited

Loading

etr2460 Dec 15, 2021

geido Dec 15, 2021 •

edited

Loading

etr2460 Dec 15, 2021

kgabryje left a comment

etr2460 commented Dec 15, 2021

etr2460 commented Dec 15, 2021

github-actions bot commented Dec 15, 2021

serenajiang commented Dec 15, 2021

etr2460 left a comment

rusackas left a comment

github-actions bot commented Dec 15, 2021

fix: Remove positions from json_metadata #17766

fix: Remove positions from json_metadata #17766

Conversation

geido commented Dec 15, 2021 • edited Loading

SUMMARY

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

geido commented Dec 15, 2021

kgabryje Dec 15, 2021 • edited Loading

Choose a reason for hiding this comment

geido Dec 15, 2021 • edited Loading

Choose a reason for hiding this comment

kgabryje Dec 15, 2021

Choose a reason for hiding this comment

geido Dec 15, 2021 • edited Loading

Choose a reason for hiding this comment

etr2460 Dec 15, 2021

Choose a reason for hiding this comment

geido Dec 15, 2021 • edited Loading

Choose a reason for hiding this comment

etr2460 Dec 15, 2021

Choose a reason for hiding this comment

kgabryje left a comment

Choose a reason for hiding this comment

etr2460 commented Dec 15, 2021

etr2460 commented Dec 15, 2021

github-actions bot commented Dec 15, 2021

serenajiang commented Dec 15, 2021

etr2460 left a comment

Choose a reason for hiding this comment

rusackas left a comment

Choose a reason for hiding this comment

github-actions bot commented Dec 15, 2021

geido commented Dec 15, 2021 •

edited

Loading

kgabryje Dec 15, 2021 •

edited

Loading

geido Dec 15, 2021 •

edited

Loading

geido Dec 15, 2021 •

edited

Loading

geido Dec 15, 2021 •

edited

Loading