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

[Human App] fix: cache-manager ttl usage #2742

Merged
merged 4 commits into from
Nov 4, 2024
Merged

[Human App] fix: cache-manager ttl usage #2742

merged 4 commits into from
Nov 4, 2024

Conversation

dnechay
Copy link
Contributor

@dnechay dnechay commented Nov 1, 2024

Description

Fixing invalid usage of cache-manager ttl value.

Summary of changes

We use @nestjs/[email protected] that under the hood just creates instance of cache-manager package (ref) that is being installed in the project (5.4.0 in our case). When we import Cache type for it - we see that ttl is just a third argument and is in milliseconds. Under the hood cache-manager calls the set method of the store (ref) in the same fashion. However, when we register cache module, we create a redis store based on cache-manager-redis-store, that accepts ttl in options argument (ref), so it's not compatible with the used version of cache-manager, so in order to fix that we have two options:

  • downgrade cache-manager to v4, that used same signature (ref)
  • use different store, e.g. cache-manager-redis-yet that is compatible with v5 (ref)

I decided to go with the second option due to next reasons:

  • using this store is mentioned in nestjs docs, so it's not surprising
  • downgrading to v4 might introduce some bugs/regressions
  • even though cache-manager-redis-yet is deprecated, it can be removed from npm due to dependencies and number of downloads; and at the same time nestjs/cache-manager should introduce support for the cache-manager/v6 soone that should eliminate such dep (ref); once it's there - we will upgrade and also get back to using seconds

Also refactored healthcheck for cache-manager: under the hood it can be any storage (Redis, Memcached, Mongo, etc.) and in case if health indicator depends on store's API we can miss updating our healthcheck if we change the store

How test the changes

  • simply run the app and use TTL command to check that values are set on all necessary keys
  • existing unit tests pass

Related issues

To close #2671, #2672

Copy link

vercel bot commented Nov 1, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
human-app ✅ Ready (Inspect) Visit Preview 💬 Add feedback Nov 4, 2024 10:20am
3 Skipped Deployments
Name Status Preview Comments Updated (UTC)
faucet-frontend ⬜️ Ignored (Inspect) Visit Preview Nov 4, 2024 10:20am
faucet-server ⬜️ Ignored (Inspect) Visit Preview Nov 4, 2024 10:20am
human-dashboard-frontend ⬜️ Skipped (Inspect) Nov 4, 2024 10:20am

@vercel vercel bot temporarily deployed to Preview – human-dashboard-frontend November 4, 2024 09:30 Inactive
@dnechay dnechay removed the WIP Work In Progress label Nov 4, 2024
@portuu3 portuu3 merged commit b71589e into develop Nov 4, 2024
12 checks passed
@portuu3 portuu3 deleted the dnechay/2671 branch November 4, 2024 11:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants