Skip to content

feat: LRU cache key#504

Merged
simoneb merged 7 commits intonearform:masterfrom
ilteoood:feat/lru-cache-key
Nov 20, 2024
Merged

feat: LRU cache key#504
simoneb merged 7 commits intonearform:masterfrom
ilteoood:feat/lru-cache-key

Conversation

@ilteoood
Copy link
Copy Markdown
Contributor

Closes #503

@simoneb
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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 ?

Comment thread README.md Outdated
@ilteoood ilteoood requested a review from simoneb November 13, 2024 21:24
Copy link
Copy Markdown
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)?

Comment thread README.md Outdated
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
aheckmann added a commit to sortxyz/fast-jwt that referenced this pull request Feb 7, 2025
aheckmann added a commit to sortxyz/fast-jwt that referenced this pull request Feb 10, 2025
simoneb pushed a commit that referenced this pull request Feb 11, 2025
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