Skip to content
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

Cache the current user profile #501

Merged
merged 2 commits into from
Aug 21, 2024
Merged

Cache the current user profile #501

merged 2 commits into from
Aug 21, 2024

Conversation

kbolashev
Copy link
Member

This PR adds caching for the UserAPI.get_user_from_token() function.
This will lead to, e.g, caching of the current user, which will be useful for annotations later :)

@kbolashev kbolashev added the enhancement New feature or request label Jul 18, 2024
@kbolashev kbolashev self-assigned this Jul 18, 2024
Copy link

dagshub bot commented Jul 18, 2024

Copy link
Contributor

@simonlsk simonlsk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Let's discuss the security issue and decide.

@guysmoilov
Copy link
Member

One the one hand, I don't see a particular security issue since if the user has malicious code running in their process, then yes of course it can grab and steal anything. Do you imagine a more exotic threat scenario?

OTOH, why is this necessary? This caching helps with something frequent?

@kbolashev
Copy link
Member Author

kbolashev commented Jul 21, 2024

OTOH, why is this necessary? This caching helps with something frequent?

This will help with this PR: #503

In particular at the moment user clicks dp.save() with annotations, I have to get the current user to put into the LS task. Having the current user ID cached saves a round-trip every time they're saving annotations.

I initially threw lru_cache on the current user function, but it doesn't account for cases where the token was changed/they changed the user in the middle somehow, so instead I decided to cache this for all tokens.

@kbolashev kbolashev merged commit 7bbd9f9 into master Aug 21, 2024
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants