-
Notifications
You must be signed in to change notification settings - Fork 13.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(dashboard): make permalinks stable #20632
Conversation
Codecov Report
@@ 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
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
91ed359
to
43a6f74
Compare
43a6f74
to
02d3699
Compare
} | ||
key = CreateKeyValueCommand( | ||
key = UpsertKeyValueCommand( | ||
key=get_deterministic_uuid(self.salt, self.signature), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
superset/key_value/utils.py
Outdated
return uuid3(get_uuid_namespace(namespace), payload_str) | ||
|
||
|
||
def get_owner_id(user: Optional[User]) -> Optional[int]: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note to self. This should be replaced with get_user_id if/when #20499 is merged.
} | ||
|
||
@property | ||
def signature(self) -> Tuple[Optional[int], DashboardPermalinkValue]: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is the signature a function of the user?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
02d3699
to
53c1c1a
Compare
8addd03
to
c6041a5
Compare
|
||
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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
c6041a5
to
6fd93bd
Compare
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