fix(common): fix cache ttl not beeing respected

[Flusinerd]

PR Checklist

Please check if your PR fulfills the following requirements:

• The commit message follows our guidelines: https://github.com/nestjs/nest/blob/master/CONTRIBUTING.md
• Tests for the changes have been added (for bug fixes / features)
• Docs have been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

• Bugfix
• Feature
• Code style update (formatting, local variables)
• Refactoring (no functional changes, no api changes)
• Build related changes
• CI related changes
• Other... Please describe:

What is the current behavior?

Currently the @CacheTTL decorator does not work. With cachemanager v5

Issue Number: #11119

What is the new behavior?

@CacheTTL Works again and the behavior is inline with the docs (expects milliseconds)

The interceptor has been updated to also check, like the provider does, if version 5 or later is beeing used.
If so the inteceptor uses the new signature for the set(key, value, ttl) method. Version 4 used an options object as third paremeter. V5 uses a number directly.

Does this PR introduce a breaking change?

• Yes
• No

Because it checks which version of cache manager is beeing used.

Other information

[coveralls]

Pull Request Test Coverage Report for Build ded6960b-f2c1-4f0d-920b-4552aa922554

• 0 of 0 changed or added relevant lines in 0 files are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage remained the same at 92.885%

---

Totals
Change from base Build aa3fcab8-9499-4650-b471-bc16ef351958:	0.0%
Covered Lines:	6488
Relevant Lines:	6985

---

💛 - Coveralls

[Tony133]

Only the name of this constant cacheManagerIsv5OrGreater I would change, to make it a little more readable, otherwise it looks good. 👍

Just one question the interceptor continues to work with both cache-manager v4 and v5, right?

[Flusinerd]

It falls back to using the old signature if the version is below v5. So there should be no change to the code for that case.

cacheManagerIsv5OrGreater is beeing used in the packages/common/cache/cache.providers.ts as well to use milliseconds instead of seconds for the default TTL of five seconds. So I assumed the check is correct.
Any suggestions about naming this?

[Tony133]

Ok, for the name cacheManagerIsv5OrGreater if it's already used internally already in common then it's better to use this, sure it's a bit strange name 😄 . Anyway the variable name from what I understand should mean: " Cache manager is version 5 or higher " .

At the moment one name could be "cacheManagerVersions", it is an idea 😄, if you change it here you should also change it to packages/common/cache/cache.providers.ts.

Howerer if I have other names I will write to you here.

[Flusinerd]

" Cache manager is version 5 or higher " This is exactly what the variable is ment to be.
It's a boolean that is true if the package beeing loaded is v5 or greater

[Tony133]

Yes, I agree with that, no problem, but I think the name could be improved, maybe even something like "cacheManagerCheckVersions", however i repeat no problem 👍

[micalevisk]

we need to do this check lazily otherwise we end up forcing everyone to have cache-manager installed

What about having a private method at CacheInterceptor just for this role? it would return a boolean

Or, better yet(?), we could have a internal utility function at cache.providers.ts that will do that check. Then we can use the same function on createCacheManager as well to avoid duplicating logic

[Flusinerd]

Or, better yet(?), we could have a internal utility function at cache.providers.ts that will do that check. Then we can use the same function on createCacheManager as well to avoid duplicating logic

That seems like the best solution.

The loadPackage function is logging when it could not load the dependency in its catch.
Do we want to modify that function with a parameter to disable that log, or do we want to refactor that function so it uses a new function without the logging?

[micalevisk]

we should keep the current behavior, which is just like how createCacheManager works.

[micalevisk]

this will be called everytime we intercept something, which isn't needed as this won't change over time.

What about moving such logic to constructor instead?

[Flusinerd]

Oh, ye good catch. My bad :)

[Swift188]

I'm still having this issue. In tableplus the TTL on all of my records is -1..

[email protected]
[email protected]

Using the following in my controller

@Get()
@UseInterceptors(CacheInterceptor)
@CacheTTL(2 * 60)

[snc]

@Swift188 If you use cache-manager v5, you possibly want @CacheTTL(2 * 60 * 1000) because it expects milliseconds and not seconds (see warning at https://docs.nestjs.com/techniques/caching).