feature(common) Add CacheTTL decorator. Cache decorators priority over global defaults. #2943
Conversation
Pull Request Test Coverage Report for Build 3276
💛 - Coveralls |
| @@ -51,10 +58,12 @@ export class CacheInterceptor implements NestInterceptor { | |||
| trackBy(context: ExecutionContext): string | undefined { | |||
| const httpAdapter = this.httpAdapterHost.httpAdapter; | |||
| const isHttpApp = httpAdapter && !!httpAdapter.getRequestMethod; | |||
|
|
|||
| if (!isHttpApp) { | |||
| return this.reflector.get(CACHE_KEY_METADATA, context.getHandler()); | |||
There was a problem hiding this comment.
These lines shouldn't be removed - httpAdapter will be undefined for both microservices and websockets.
There was a problem hiding this comment.
Yes, I thought about that. Correct me if I'm misunderstanding, but since the check is happening for the reflector data first, the check for !isHttpApp won't ever be necessary, as the reflector check is happening no matter what (causing that check to be meaningless now). Happy to make changes, but not sure if it makes any difference given the logic change.
There was a problem hiding this comment.
Yes, but what if someone works on microservice/WS application and forgot to set CACHE_KEY_METADATA? The following line:
if (cacheMetadata)
will return false, so Nest will call this one:
httpAdapter.getRequestMethod()
whereas httpAdapter will be undefined in this case.
There was a problem hiding this comment.
Thanks for the response. I'll patch.
Cache decorators priority over global defaults
joeyslack
force-pushed
the
cache-ttl-decorate
branch
from
0ae2a62
to
da33fc4
Compare
|
Maybe it makes sense to see how this is done in other frameworks? |
|
@ruscon Fair discussion, I'm intimately familiar with symfony from a previous life. However, this PR is just to compliment an already existing cache system, to match the cache strategy already in place. The idea is just to make sure both parameters (key and ttl) are modifiable. I think, if you wanted to make new cache decorators entirely, that's cool - would love to contribute in another PR, but this PR was intended to stay completely in line with what already exists, and just allow users to override this one particular setting. |
|
@joeyslack would you like to create a PR to the docs with this feature? I'm planning to merge this shortly :) https://github.com/nestjs/docs.nestjs.com/pulls |
Co-Authored-By: Kamil Mysliwiec <mail@kamilmysliwiec.com>
|
@kamilmysliwiec Docs updated at nestjs/docs.nestjs.com#718 |
|
Thank you @joeyslack! |
|
@kamilmysliwiec |
|
I use both depending on what I need to achieve. |
PR Type
The
CacheTTLdecorator has been added to controller methods for adding additional cache features. This allows the user to set the Cache TTL before it's set in it's cache store.Additionally,
@CacheKey()and@CacheTTL()decorator usage with@CacheInterceptor()take priority over global Cache configuration defaults. This allows for sensible Global Caching defaults, with controller specific cache overrides.What is the current behavior?
Currently, when Global Cache is enabled, controller specific CacheKey settings are ignored. This forces the user to annotate all of their cached methods, or globally cache all, making customization tedious and repetitive, and potentially dangerous.
Furthermore, TTL override hasn't been possible before on a controller routing.
Issue Number: N/A
What is the new behavior?
The new behavior allows the user to override CacheTTL when globals are set, or not.
When
@CacheKey()or@CacheTTL()is provided on a cash intercepted route (@UseInterceptor()), it will take priority over default cache setting.Does this PR introduce a breaking change?
Other information
Open to making modifications as necessary. I built this to solve a problem in my own application, hopefully it helps others. I think this is a very simple cache improvement that should have no side-effects, performance implications or otherwise.