jaredwray / cacheable

Caching for Nodej.js
https://cacheable.org
MIT License
1.54k stars 163 forks source link

in NestJS when setting a TTL value, the TTL value is always automatically replaced with the default TTL in the config #621

Closed eizeiky111 closed 7 months ago

eizeiky111 commented 11 months ago

Describe the bug in NestJS when setting a TTL value, the TTL value is always automatically replaced with the default TTL in the config

How To Reproduce (best to provide workable code or tests!) NestJs version: 10.1.17

NestJS package version "cache-manager": "^5.3.1", "cache-manager-redis-store": "^2.0.0", "@types/cache-manager-redis-store": "^2.0.4", "@nestjs/cache-manager": "^2.1.1",

First Method ( in milliseconds ) this.redis.set("key", "val", 30000) // the value of 30000 always auto overriden with the default TTL in config

Second Method ( in seconds ) this.redis.set("key", "val", { ttl: 30 } as any) // using this method 100% successfully set the TTL value without being replaced

Sorry for my bad english, this is my first time making report in Redis 😶‍🌫️ thank you very much

jaredwray commented 10 months ago

@eizeiky111 or @smileeunsong - can you you build a test file I can validate this with as right now I cannot get this to happen.

kejiahp commented 10 months ago

@jaredwray you can check this repository here specifically this file for a working example of the issue and the walk around I used which I'm about to explain below.

I did some searching and I believe the issue only occurs in versions of cache-manager where the Cache set function overload with CachingConfig options, e.g. await this.cacheManager.set(key, value, **{ ttl: timeToLive}**), was removed, I might be wrong🤷‍♀️.

I think this overload was removed from cache-manager v5.0.0 and above I might be wrong again🤷‍♀️.

my solution: rollback to cache-manger v4.0.0 or lower and use the overload function. This overload await this.cacheManager.set(key, value, **{ ttl: timeToLive}**) to set the time to live for your cache.

hope this helps someone.

gmesml commented 9 months ago

Things get tricky when you also rely on stores like redis: https://docs.nestjs.com/techniques/caching#different-stores . Speaking from experience things work according to official docs when you stay at cache-manager v4 and redis-store v2, when either is bumped to a major ahead you'll be having headaches. Pay attention to auto upgrading bots.

jaredwray commented 7 months ago

@kejiahp - Looks like to fix this with v5 you need to do the following:

  public async savetoCache(
    key: string,
    value: string,
    ttl = 10,
  ): Promise<void> {
    await this.cacheManager.set(key, value, ttl);
  }

The reason for this is that the ttl is now a number on v5

ygweric commented 6 months ago

I met the same bug, this is because different store implements the function set with different options, they do NOT follow the same rules :(

Following is cache-manager-redis-store's source code about set, the thrid arguments is an object, ,not ttl

https://github.com/dabroek/node-cache-manager-redis-store/blob/master/index.js

  const set = async (key, value, options) => {
    if (!isCacheableValue(value)) {
      throw new Error(`"${value}" is not a cacheable value`);
    }

    const ttl = (options?.ttl || options?.ttl === 0) ? options.ttl : config.ttl;

    if (ttl) {
      return redisCache.setEx(key, ttl, encodeValue(value));
    } else {
      return redisCache.set(key, encodeValue(value));
    }
  };

So your second method is right, you can add @ts-ignore to disable the ts error


      // eslint-disable-next-line @typescript-eslint/ban-ts-comment
      // @ts-ignore
      await this.cacheManager.set(cachedKey, cameras, { ttl: 20 });