feat: add permalink to dashboard and explore #19078
Conversation
villebro
force-pushed
the
villebro/key-value-permalink
branch
from
21dc035
to
70cb62e
Compare
Codecov Report
@@ 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
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
|
I think we also need to use a permanent link for "Embed Code" in Explore to reflect the latest changes made by the user.
|
|
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:
|
|
Do you think we need the |
|
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. |
|
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:
|
| @@ -309,8 +310,8 @@ class SliceHeaderControls extends React.PureComponent< | |||
|
|
|||
| {supersetCanShare && ( | |||
| <ShareMenuItems | |||
| copyMenuItemTitle={t('Copy chart URL')} | |||
| emailMenuItemTitle={t('Share chart by email')} | |||
| copyMenuItemTitle={t('Copy permalink to clipboard')} | |||
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
The inspiration here is coming from Github and Shortcut but we could validate the name as you suggested.
There was a problem hiding this comment.
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.
superset-frontend/src/dashboard/components/gridComponents/Tab.jsx
Outdated
Show resolved
Hide resolved
superset-frontend/src/dashboard/components/menu/ShareMenuItems/index.tsx
Show resolved
Hide resolved
superset-frontend/src/dashboard/components/menu/ShareMenuItems/index.tsx
Outdated
Show resolved
Hide resolved
superset-frontend/src/dashboard/components/nativeFilters/FilterBar/index.tsx
Show resolved
Hide resolved
There was a problem hiding this comment.
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.
Good catch - fixed |
While this is true for permalinks, it is not necessarily true for other features that use the |
villebro
force-pushed
the
villebro/key-value-permalink
branch
from
aee4236
to
7b0f4b2
Compare
@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. |
😮 Yes, it's important to check that. |
There was a problem hiding this comment.
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.
|
Thanks @michael-s-molina and @jinghua-qa for all the help with this! ❤️ |
|
Ephemeral environment shutdown and build artifacts deleted. |
There was a problem hiding this comment.
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.
| const [shortUrl, setShortUrl] = useState(''); | ||
|
|
||
| async function getShortUrl(urlOverride?: string) { | ||
| if (update) { |
There was a problem hiding this comment.
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?
| url: PropTypes.string, | ||
| addDangerToast: PropTypes.func.isRequired, | ||
| anchorLinkId: PropTypes.string, | ||
| dashboardId: PropTypes.number, |
There was a problem hiding this comment.
Since this is just a 100L component, it may be worth just converting this to TypeScript instead of adding new proptypes
| .catch(this.props.addDangerToast); | ||
| const nativeFiltersKey = getUrlParam(URL_PARAMS.nativeFiltersKey); | ||
| if (this.props.dashboardId) { | ||
| getFilterValue(this.props.dashboardId, nativeFiltersKey) |
There was a problem hiding this comment.
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).
| @@ -17,6 +17,7 @@ | |||
| * under the License. | |||
| */ | |||
| import { SupersetClient, logging } from '@superset-ui/core'; | |||
There was a problem hiding this comment.
This file is still named keyValue.tsx, should we rename it for consistency?
| 500: | ||
| $ref: '#/components/responses/500' | ||
| """ | ||
| key_type = current_app.config["PERMALINK_KEY_TYPE"] |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I added this option to per a request from @graceguo-supercat, see discussion here: #18181 (comment)
There was a problem hiding this 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).
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Hashids are deterministic based on input (numeric ids) whilst token.url is completely random. I think we may want to be deterministic here.
|
🏷️ 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)
|
@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. |
mistercrunch
added
🍒 1.5.0
🍒 1.5.1
🍒 1.5.2
🍒 1.5.3
🏷️ bot
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:http://host/superset/dashboard/p/<key>/http://host/superset/explore/p/<key>/The dashboard state is a
dictwith the following fields:filterState: native filter statehash(optional): the anchor (used for direct links to charts/tabs)urlParams(optional): any URL params that are active, excluding the reserved URL params likenative_filter_keyetc. 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
dictwith the following fields:formDatathe chart form dataurlParams(optional): any URL params that are active, excluding the reserved URL params likeform_data_key,slice_idetc. (same format as in the dashboard state)The benefits compared to the deprecated
shorturlfunctionality: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:
key_valuepackage totemporary_cacheto better reflect the ephemeral nature of the state (not guaranteed to be persisted)key_valuetable to metadata database along with relevant commands for adding, updating, getting and deleting key-value pairspermalinkendpoints (POST and GET) to both Explore and Dashboard that leverage the newkey_valuestorePERMALINK_KEY_TYPEtoconfig.pywhich makes it possible to switch between serial and UUID based keys (UUID = default)BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Screen.Recording.2022-03-11.at.9.35.44.AM.mov
TESTING INSTRUCTIONS
url_paramand make sure URL params are correctly persisted in permalinks.url_paramand 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