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

feat: LRU cache key #504

Merged
merged 7 commits into from
Nov 20, 2024
Merged

feat: LRU cache key #504

merged 7 commits into from
Nov 20, 2024

Conversation

ilteoood
Copy link
Contributor

Closes #503

@simoneb
Copy link
Member

simoneb commented Nov 10, 2024

Ok, the change is easy enough, but I wonder if there was a reason to hash it before storing it in the cache in the first place. The main thing I can think of is security, but the cache is in memory, so I'm not sure that's a valid assumption. Also, is avoiding the hashing really faster? I wouldn't expect the hashing to really slow down the whole thing.

@ilteoood
Copy link
Contributor Author

I did the same question to myself and wasn't able to find the proper answer.
The only thing I thought was about avoiding collision, but different algorithms bring different keys.
Maybe it is just to avoid storing the user's key locally, which could contain sensitive data (like name, email..)

The hashing part btw makes it a bit slower. We should understand what's the right balance.

@simoneb
Copy link
Member

simoneb commented Nov 12, 2024

I would not do this change until we discover why it was done in this way in the first place, and unless benchmarks show that it's much faster to do it without hashing the token.

@ilteoood
Copy link
Contributor Author

I was thinking about giving the option to pass a hasher.
So if someone wants to use the identity hasher it is able to do so.
What do you think @simoneb ?

README.md Outdated Show resolved Hide resolved
@ilteoood ilteoood requested a review from simoneb November 13, 2024 21:24
Copy link
Member

@simoneb simoneb left a comment

Choose a reason for hiding this comment

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

Can you please expand the caching section to mention the performance implication of the default cache key builder and what to do to improve performance (i.e. use the identity function)?

README.md Outdated Show resolved Hide resolved
ilteoood and others added 2 commits November 14, 2024 12:11
Co-authored-by: Simone Busoli <simone.busoli@gmail.com>
@ilteoood ilteoood requested a review from simoneb November 20, 2024 07:17
@simoneb simoneb merged commit 79f367c into nearform:master Nov 20, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Question About LRU Cache Key Implementation
2 participants