feat(dashboard): make permalinks stable #20632

ktmud · 2022-07-06T23:04:31Z

SUMMARY

Make dashboard permalink API always return the same key if the request payload is the same---i.e., for a given dashboard state, a user should always get a stable permalink when they request one.

This is a required change for #20552 and #19354 as we don't want to generate a new permalink each time a dashboard report is executed.

The Explore permalink should probably do the same, but this PR only changes the behavior for dashboard permalink just in case it causes any problems.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

N/A. This is a pure backend change. There should be no visible changes to users.

TESTING INSTRUCTIONS

Copy a permalink for dashboard---you can do it in the "..." menu on the dashboard page, or in charts or tabs.
The link should always be the same no matter how many times you click on the trigger button/link

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

codecov · 2022-07-06T23:12:48Z

Codecov Report

Merging #20632 (43e9ff2) into master (5317462) will decrease coverage by 0.19%.
The diff coverage is 100.00%.

❗ Current head 43e9ff2 differs from pull request most recent head 6fd93bd. Consider uploading reports for the commit 6fd93bd to get more accurate results

@@            Coverage Diff             @@
##           master   #20632      +/-   ##
==========================================
- Coverage   66.85%   66.65%   -0.20%     
==========================================
  Files        1753     1752       -1     
  Lines       65833    65647     -186     
  Branches     7007     6940      -67     
==========================================
- Hits        44011    43758     -253     
- Misses      20036    20127      +91     
+ Partials     1786     1762      -24

Flag	Coverage Δ
hive	`?`
mysql	`82.39% <100.00%> (-0.08%)`	⬇️
postgres	`82.47% <100.00%> (-0.09%)`	⬇️
presto	`?`
python	`82.54% <100.00%> (-0.44%)`	⬇️
sqlite	`82.25% <100.00%> (-0.08%)`	⬇️
unit	`?`

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

Impacted Files	Coverage Δ
...uperset/dashboards/filter_state/commands/create.py	`100.00% <100.00%> (ø)`
...uperset/dashboards/filter_state/commands/delete.py	`95.83% <100.00%> (+0.18%)`	⬆️
...uperset/dashboards/filter_state/commands/update.py	`100.00% <100.00%> (ø)`
superset/dashboards/permalink/commands/create.py	`94.11% <100.00%> (+5.22%)`	⬆️
superset/explore/form_data/commands/create.py	`95.23% <100.00%> (+0.11%)`	⬆️
superset/explore/form_data/commands/delete.py	`92.50% <100.00%> (+0.39%)`	⬆️
superset/explore/form_data/commands/update.py	`93.87% <100.00%> (+0.12%)`	⬆️
superset/key_value/commands/create.py	`93.61% <100.00%> (+0.43%)`	⬆️
superset/key_value/commands/upsert.py	`89.58% <100.00%> (+0.45%)`	⬆️
superset/key_value/utils.py	`95.12% <100.00%> (+1.00%)`	⬆️
... and 154 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 5317462...6fd93bd. Read the comment docs.

ktmud · 2022-07-06T23:57:09Z

superset/dashboards/permalink/commands/create.py

-            }
-            key = CreateKeyValueCommand(
+            key = UpsertKeyValueCommand(
+                key=get_deterministic_uuid(self.salt, self.signature),


Theoretically the uuid3 would never collide with another uuid3, but there is an extremely small chance of colliding with a uuid4. Once we change the Explore permalink to this deterministic id, too, this should not be a concern.

john-bodley · 2022-07-07T00:37:45Z

superset/key_value/utils.py

+    return uuid3(get_uuid_namespace(namespace), payload_str)
+
+
+def get_owner_id(user: Optional[User]) -> Optional[int]:


Note to self. This should be replaced with get_user_id if/when #20499 is merged.

john-bodley · 2022-07-07T05:08:30Z

superset/dashboards/permalink/commands/create.py

+        }
+
+    @property
+    def signature(self) -> Tuple[Optional[int], DashboardPermalinkValue]:


Why is the signature a function of the user?

Just thought it'd be safer to give each user a unique permalink in case we want to do anything special about it. (Can't think of a use case just yet) Plus user id is also saved for KeyValue so you can also argue this is for completeness sake.

ktmud · 2022-07-11T18:04:21Z

superset/key_value/utils.py

+
+def get_deterministic_uuid(namespace: str, payload: Any) -> UUID:
+    """Get a deterministic UUID (uuid3) from a salt and a payload."""
+    payload_str = json_dumps_w_dates(payload, sort_keys=True)


@villebro @michael-s-molina any particular reason why KeyValue was stored with Python pickle instead of JSON? IMO we should avoid storing pickles in database as they are not portable and could be versioned out if an advanced python object was used. It'd also be impossible to export the database directly for offline analysis.

@ktmud not all KeyValue implementations will necessarily be JSON serializable. For instance, the SupersetMetastoreCache KeyValue objects aren't JSON serialisable. While pickle isn't necessarily perfect in all scenarios, IMO it's the most generally supported serialising/deserializing utility for Python, and as such seems like the best solution for a general purpose key-value store.

At the time this feature was designed there was really no requirement for analysing the contents of the value payload, so this wasn't considered. But if this is needed I don't see any reason why we couldn't make the serializer and deserialiser customizable to make it possible to use another library for encoding/decoding the values and then swapping it out for json.dumps and json.loads for the permalink implementations (would of course require a migration).

In the case of the proposed function, I'd recommend adding a comment in the doctoring that the payload needs to be JSON serializable.

I still don't think it's a good idea to store language-specific serialized data in databases---even though Superset is predominantly a Python app. I can see the case when we need to store other binary data, e.g., image files, cache for zipped exports, encrypted data, etc, so maybe the value column indeed needs to be binary. A customizable serializer sounds like a good idea. Let's revisit when offline analysis becomes a real need.

pull-request-size bot added the size/L label Jul 6, 2022

ktmud marked this pull request as ready for review July 6, 2022 23:06

ktmud force-pushed the keyvalue-deterministic-key branch from 91ed359 to 43a6f74 Compare July 6, 2022 23:13

ktmud changed the title ~~feat(dashboard): make permalink deterministic~~ feat(dashboard): make permalinks stable Jul 6, 2022

ktmud force-pushed the keyvalue-deterministic-key branch from 43a6f74 to 02d3699 Compare July 6, 2022 23:16

ktmud requested review from villebro, zhaoyongjie, etr2460, john-bodley and michael-s-molina July 6, 2022 23:17

ktmud commented Jul 6, 2022

View reviewed changes

john-bodley reviewed Jul 7, 2022

View reviewed changes

ktmud force-pushed the keyvalue-deterministic-key branch from 02d3699 to 53c1c1a Compare July 9, 2022 00:59

pull-request-size bot added size/M and removed size/L labels Jul 9, 2022

ktmud force-pushed the keyvalue-deterministic-key branch 2 times, most recently from 8addd03 to c6041a5 Compare July 11, 2022 18:00

ktmud commented Jul 11, 2022

View reviewed changes

feat(dashboard): make permalink deterministic

6fd93bd

ktmud force-pushed the keyvalue-deterministic-key branch from c6041a5 to 6fd93bd Compare July 12, 2022 20:37

ktmud mentioned this pull request Jul 12, 2022

feat(report): allow capturing dashboard reports in specific state #20552

Merged

11 tasks

john-bodley approved these changes Jul 12, 2022

View reviewed changes

ktmud merged commit c3ac612 into apache:master Jul 12, 2022

john-bodley deleted the keyvalue-deterministic-key branch February 17, 2023 22:16

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(dashboard): make permalinks stable #20632

feat(dashboard): make permalinks stable #20632

ktmud commented Jul 6, 2022 •

edited

codecov bot commented Jul 6, 2022 •

edited

ktmud Jul 6, 2022

john-bodley Jul 7, 2022

john-bodley Jul 7, 2022

ktmud Jul 8, 2022 •

edited

ktmud Jul 11, 2022

villebro Jul 12, 2022 •

edited

ktmud Jul 12, 2022 •

edited

		return uuid3(get_uuid_namespace(namespace), payload_str)


		def get_owner_id(user: Optional[User]) -> Optional[int]:

feat(dashboard): make permalinks stable #20632

feat(dashboard): make permalinks stable #20632

Conversation

ktmud commented Jul 6, 2022 • edited

SUMMARY

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

codecov bot commented Jul 6, 2022 • edited

Codecov Report

ktmud Jul 6, 2022

Choose a reason for hiding this comment

john-bodley Jul 7, 2022

Choose a reason for hiding this comment

john-bodley Jul 7, 2022

Choose a reason for hiding this comment

ktmud Jul 8, 2022 • edited

Choose a reason for hiding this comment

ktmud Jul 11, 2022

Choose a reason for hiding this comment

villebro Jul 12, 2022 • edited

Choose a reason for hiding this comment

ktmud Jul 12, 2022 • edited

Choose a reason for hiding this comment

ktmud commented Jul 6, 2022 •

edited

codecov bot commented Jul 6, 2022 •

edited

ktmud Jul 8, 2022 •

edited

villebro Jul 12, 2022 •

edited

ktmud Jul 12, 2022 •

edited