feat: add permalink to dashboard and explore by villebro · Pull Request #19078 · apache/superset

villebro · 2022-03-09T14:40:44Z

SUMMARY

This PR adds new permanent link functionality that makes it possible to persist Dashboard and Explore state in the metadata database. The state is persisted in a new metadata table called key_value, which is able to store pickleable binary objects (can be used in the future for other resources, too). The new permalinks look as follows:

Dashboard: http://host/superset/dashboard/p/<key>/
Explore: http://host/superset/explore/p/<key>/

The dashboard state is a dict with the following fields:

filterState: native filter state
hash (optional): the anchor (used for direct links to charts/tabs)
urlParams (optional): any URL params that are active, excluding the reserved URL params like native_filter_key etc. This is also where filter box state is persisted. Note that these are stored as a list of key-value tuples to support having multiple same keys (not currently supported by the chart data API but implemented like this for future proofing)

The Explore state is also a dict with the following fields:

formData the chart form data
urlParams (optional): any URL params that are active, excluding the reserved URL params like form_data_key, slice_id etc. (same format as in the dashboard state)

The benefits compared to the deprecated shorturl functionality:

Leverages existing resource security mechanism: if you don't have access to a chart/dashboard, you are also not able to view permalink state for those charts/dashboards
Uses UUID-based keys by default to make permalinks unguessable. However, where needed, numeric serial keys (similar to the legacy shorturl keys) can be used by flipping a config flag. Do note, however, that changing this setting will cause old links to stop working, i.e. it's not possible to retrieve permalink state based by its UUID key if the feature has been configured to work with the serial id and vice versa.
Is not restricted by the URL length limitations that affects shorturls (however, Filter Box state is still subject to URL overflows).

Notice that old shorturls will still work like before - but links created after this will be based on the new permalink feature.

Detailed list of changes:

Rename current key_value package to temporary_cache to better reflect the ephemeral nature of the state (not guaranteed to be persisted)
Add new key_value table to metadata database along with relevant commands for adding, updating, getting and deleting key-value pairs
Add permalink endpoints (POST and GET) to both Explore and Dashboard that leverage the new key_value store
Add PERMALINK_KEY_TYPE to config.py which makes it possible to switch between serial and UUID based keys (UUID = default)
fully support persisting filter box state and url parameters in permalink state (this was currently mostly broken)
remove old shorturl creation functionality from frontend and backend
Migrate all permalink buttons to new feature:
- Explore action buttons (above chart): "Copy permalink to clipboard" and "Share permalink by email"
- Explore embed iframe button (above chart): link now references a permalink
- Dashboard context menu: "Copy permalink to clipboard" and "Share permalink by email"
- Dashboard chart context menu: "Copy permalink to clipboard" and "Share permalink by email"
- Dashboard tab anchor link

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

Screen.Recording.2022-03-11.at.9.35.44.AM.mov

TESTING INSTRUCTIONS

Explore a dataset, add a metric/columns to a few controls and share a permalink both via clipboard and email. Change control values and verify that changes don't change the state in the previously shared permalink. Bonus: create a chart with Jinja that uses url_param and make sure URL params are correctly persisted in permalinks.
Click the Embed code button and make sure the link works
Save the chart and ensure sharing works similar to above
Go to a dashboard with native filters and tabs, select a few filters and share the dashboard from the context menu.
Click on the anchor button next to a tab and make sure both the link and email message works
Click on a chart context menu and make sure both link and email message works
Create a dashboard with a filter box and a chart, apply a filter on the filter box and make sure shared permalinks open with the predefined filters
Bonus: add a chart with Jinja that references a url_param and make sure the chart renders correctly 1) in the dashboard after creating a permalink 2) in the explore view after creating a permalink to the chart.

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-03-10T11:27:58Z

Codecov Report

Merging #19078 (c0fb6d7) into master (8d53db1) will increase coverage by 0.02%.
The diff coverage is 86.77%.

❗ Current head c0fb6d7 differs from pull request most recent head 4e9b423. Consider uploading reports for the commit 4e9b423 to get more accurate results

@@            Coverage Diff             @@
##           master   #19078      +/-   ##
==========================================
+ Coverage   66.54%   66.56%   +0.02%     
==========================================
  Files        1646     1668      +22     
  Lines       63630    64190     +560     
  Branches     6475     6476       +1     
==========================================
+ Hits        42343    42729     +386     
- Misses      19607    19779     +172     
- Partials     1680     1682       +2

Flag	Coverage Δ
hive	`?`
mysql	`81.96% <88.20%> (+0.08%)`	⬆️
postgres	`82.00% <88.20%> (+0.08%)`	⬆️
presto	`?`
python	`82.09% <88.20%> (-0.23%)`	⬇️
sqlite	`81.76% <88.20%> (?)`

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

Impacted Files	Coverage Δ
...erset-frontend/src/components/AnchorLink/index.jsx	`80.00% <ø> (ø)`
.../components/Header/HeaderActionsDropdown/index.jsx	`71.42% <ø> (ø)`
...dashboard/components/SliceHeaderControls/index.tsx	`66.66% <ø> (ø)`
...nd/src/dashboard/components/gridComponents/Tab.jsx	`80.48% <ø> (ø)`
...rd/components/nativeFilters/FilterBar/keyValue.tsx	`18.18% <ø> (ø)`
...nd/src/explore/components/ExploreActionButtons.tsx	`57.14% <0.00%> (ø)`
.../explore/components/ExploreViewContainer/index.jsx	`56.99% <0.00%> (ø)`
superset/explore/utils.py	`100.00% <ø> (ø)`
superset/temporary_cache/commands/entry.py	`100.00% <ø> (ø)`
superset/temporary_cache/commands/parameters.py	`100.00% <ø> (ø)`
... and 84 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 8d53db1...4e9b423. Read the comment docs.

michael-s-molina · 2022-03-10T12:07:51Z

I think we also need to use a permanent link for "Embed Code" in Explore to reflect the latest changes made by the user.

michael-s-molina · 2022-03-10T12:14:55Z

I created a permanent link for a dashboard as an admin and tried to access it using a user that doesn't have permission. I get an "Access Denied" tooltip as expected.

I tried the same thing but this time for an Explore chart. I get this:

michael-s-molina · 2022-03-10T12:31:01Z

Do you think we need the changed_on and changed_by_fk columns in the database? They'll always be null.

michael-s-molina · 2022-03-10T12:48:08Z

I found an existing problem but we can try to fix it here. When the network is not available (you can simulate using the Network tab of Chrome Dev Tools), and I try to copy a permalink, the message states "Sorry, your browser does not support copying." I'm fine with either handling the different types of errors or use the same generic message "Sorry, something went wrong. Try again later." that is used when sharing by email. If we do decide to handle by type, then we should also make the change to email sharing.

michael-s-molina · 2022-03-10T12:55:38Z

When trying to access a permanent link for a deleted chart I get the following message:

When trying to access a permanent link for a deleted dashboard I get the following message:

kgabryje · 2022-03-10T14:38:45Z

I'm not sure if using a word "permalink" brings value to the end user. I feel like it might be confusing, and simply saying "link" or "URL" makes the intention clearer. I'd suggest asking a product manager for an opinion on that.
(applies to changes in ExploreActionButtons too)

The inspiration here is coming from Github and Shortcut but we could validate the name as you suggested.

Agreed - I know @michael-s-molina did some investigative work and found many products use the term. Here's GitHub:

One other thing we may want to consider is removing "to clipboard", as it's pretty much self evident.

michael-s-molina

Awesome work @villebro! The new security checkings for permanent links are a BIG improvement to the previous version. I also really liked the reorganization of key_value and temporary_cache. I tested and reviewed the whole feature and left some comments. I’ll continue with the review as we add tests and fix some bugs.

villebro · 2022-03-11T11:13:12Z

I think we also need to use a permanent link for "Embed Code" in Explore to reflect the latest changes made by the user.

Good catch - fixed

villebro · 2022-03-11T11:14:24Z

Do you think we need the changed_on and changed_by_fk columns in the database? They'll always be null.

While this is true for permalinks, it is not necessarily true for other features that use the key-value table (note that we have commands for deleting and updating key value entries)

michael-s-molina · 2022-03-16T15:45:15Z

@michael-s-molina this is an interesting edge case - in this case the chart has been removed, hence the chart doesn't have a title, but since the form data is available in the permalink state, all the relevant state is available to render the chart, and is comparable to a permalink for a chart that hasn't been saved yet. What would be the expected behavior here?

I think it's a different use case. We don't know why a chart has been deleted, it may be because the information is no longer valid or the underlying dataset has been changed/removed, etc. We should consider cached data for removed charts as no longer valid and redirect the user to the charts list.

villebro · 2022-03-16T15:49:42Z

@michael-s-molina this is an interesting edge case - in this case the chart has been removed, hence the chart doesn't have a title, but since the form data is available in the permalink state, all the relevant state is available to render the chart, and is comparable to a permalink for a chart that hasn't been saved yet. What would be the expected behavior here?

I think it's a different use case. We don't know why a chart has been deleted, it may be because the information is no longer valid or the underlying dataset has been changed/removed, etc. We should consider cached data for removed charts as no longer valid and redirect the user to the charts list.

Makes sense - I'll change as follows: if chart permalink metadata has a chart id, and said chart id is no longer available, we redirect to the list view and issue a toast for missing chart. Btw, now I'm confused why there wasn't an exception when it tried to validate the perms for the missing chart, but I guess I'll soon find out.

michael-s-molina · 2022-03-16T15:52:57Z

Btw, now I'm confused why there wasn't an exception when it tried to validate the perms for the missing chart, but I guess I'll soon find out.

😮 Yes, it's important to check that.

michael-s-molina

LGTM. Thank you so much for all the hard work @villebro! After all the challenges the application is in a much better state now with the endpoints for cached and permanent states and all the security improvements.

jinghua-qa

LGTM

villebro · 2022-03-16T23:15:39Z

Thanks @michael-s-molina and @jinghua-qa for all the help with this! ❤️

github-actions · 2022-03-16T23:16:11Z

Ephemeral environment shutdown and build artifacts deleted.

ktmud

Thanks for working on this, @villebro ! Sorry for being late to the party, but I was half way through the reviewing then got distracted by something else.

Sharing the comments nonetheless in case they are useful for future iterations.

ktmud · 2022-03-16T23:46:56Z

-  const [shortUrl, setShortUrl] = useState('');
-
-  async function getShortUrl(urlOverride?: string) {
-    if (update) {


One of the benefits of this hook is that the generated URL will not update if dashboard states didn't update. I.e., when you click on "Copy URL" twice, it will only generate one short-url record in the database, as opposed to now, each click will generated a new and different short-url. Do you think it's worth implementing a similar deduping effort either in the frontend or backend?

ktmud · 2022-03-17T00:13:01Z

-  url: PropTypes.string,
+  addDangerToast: PropTypes.func.isRequired,
+  anchorLinkId: PropTypes.string,
+  dashboardId: PropTypes.number,


Since this is just a 100L component, it may be worth just converting this to TypeScript instead of adding new proptypes

ktmud · 2022-03-17T00:15:33Z

-      .catch(this.props.addDangerToast);
+    const nativeFiltersKey = getUrlParam(URL_PARAMS.nativeFiltersKey);
+    if (this.props.dashboardId) {
+      getFilterValue(this.props.dashboardId, nativeFiltersKey)


Maybe getFilterValue should be renamed to getSavedFilterState for clarify.

We should probably also just pass the full state to URLShortLinkButton itself so we can avoid a round trip to the server for the saved states---because the client states should always be in sync with the URL. If the states already exist somewhere on the client, then we can pass it around for generating the short URL.

By passing full states (an arbitrary JSON object + a base URL template), it also decouples URLShortLinkButton with either dashboard or chart API endpoint, so it can be used in even other places (e.g. SQL queries, etc).

ktmud · 2022-03-17T00:25:51Z

@@ -17,6 +17,7 @@
 * under the License.
 */
 import { SupersetClient, logging } from '@superset-ui/core';


This file is still named keyValue.tsx, should we rename it for consistency?

ktmud · 2022-03-17T00:34:01Z

+            500:
+              $ref: '#/components/responses/500'
+        """
+        key_type = current_app.config["PERMALINK_KEY_TYPE"]


I don't think we should provide this option. Adding one more config options means more effort to support it in the future. We should not have to provide more flexibility in the config file unless users request it. The numeric ids were deprecated based on arguments of security risks---if security risk with numeric ids is real, then it's real for everyone. If not, then we should just not worry about it and keep using numeric ids that users are used to.

If URL readability was the concern for UUID, we should probably just change the keys from uuid (which only uses 16 hex characters) to case-sensitive hashids used in real short-url services (e.g., bit.ly and t.co).

Personally I think hashids might be a better end state.

I added this option to per a request from @graceguo-supercat, see discussion here: #18181 (comment)

Having said that, I really like the readability of hashids, so I'm happy to make that the default and only supported permalink key type, thus eliminating this config flag. @graceguo-supercat is that ok for you? I can open a follow-up PR to address this (should be fairly simple).

The cache endpoint is using token_url. We could use it with a smaller length (with collisions in mind) instead of introducing another dependency. I'm fine with either solution since hashids also has its merits.

Hashids are deterministic based on input (numeric ids) whilst token.url is completely random. I think we may want to be deterministic here.

sadpandajoe · 2022-03-31T15:33:03Z

🏷️ preset:2022.11

* rename key_value to temporary_cache * add migration * create new key_value package * add commands * lots of new stuff * fix schema reference * remove redundant filter state from bootstrap data * add missing license headers * fix pylint * fix dashboard permalink access * use valid json mocks for filter state tests * fix temporary cache tests * add anchors to dashboard state * lint * fix util test * fix url shortlink button tests * remove legacy shortner * remove unused imports * fix js tests * fix test * add native filter state to anchor link * add UPDATING.md section * address comments * address comments * lint * fix test * add utils tests + other test stubs * add key_value integration tests * add filter box state to permalink state * fully support persisting url parameters * lint, add redirects and a few integration tests * fix test + clean up trailing comma * fix anchor bug * change value to LargeBinary to support persisting binary values * fix urlParams type and simplify urlencode * lint * add optional entry expiration * fix incorrect chart id + add test (cherry picked from commit b7a0559)

john-bodley · 2022-04-14T18:04:26Z

@michael-s-molina or @villebro do you know whether this change may have introduced a regression in how inline dashboard permalink works? Previously you could create an anchor to the chart within the dashboard though now it seems like the permalink links to the chart in explorer.

The feature was loved/used by a number of people at Airbnb.

* rename key_value to temporary_cache * add migration * create new key_value package * add commands * lots of new stuff * fix schema reference * remove redundant filter state from bootstrap data * add missing license headers * fix pylint * fix dashboard permalink access * use valid json mocks for filter state tests * fix temporary cache tests * add anchors to dashboard state * lint * fix util test * fix url shortlink button tests * remove legacy shortner * remove unused imports * fix js tests * fix test * add native filter state to anchor link * add UPDATING.md section * address comments * address comments * lint * fix test * add utils tests + other test stubs * add key_value integration tests * add filter box state to permalink state * fully support persisting url parameters * lint, add redirects and a few integration tests * fix test + clean up trailing comma * fix anchor bug * change value to LargeBinary to support persisting binary values * fix urlParams type and simplify urlencode * lint * add optional entry expiration * fix incorrect chart id + add test

villebro requested a review from a team as a code owner March 9, 2022 14:40

superset-github-bot Bot added the preset-io label Mar 9, 2022

pull-request-size Bot added the size/XXL label Mar 9, 2022

michael-s-molina added the request:qa-review Requests a QA review label Mar 9, 2022

michael-s-molina requested review from jinghua-qa and michael-s-molina March 9, 2022 16:25

villebro force-pushed the villebro/key-value-permalink branch from 21dc035 to 70cb62e Compare March 9, 2022 23:17

kgabryje reviewed Mar 10, 2022

View reviewed changes

Comment thread superset-frontend/src/dashboard/components/gridComponents/Tab.jsx Outdated

kgabryje reviewed Mar 10, 2022

View reviewed changes

Comment thread superset-frontend/src/dashboard/components/menu/ShareMenuItems/index.tsx Outdated