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.

Sign up for GitHub

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

Jump to bottom

fix(common): fix cache ttl not beeing respected #11131

Merged
merged 5 commits into from
Mar 15, 2023
Merged

fix(common): fix cache ttl not beeing respected #11131

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
e151c47
fix(common): fix cache ttl not beeing respected
Flusinerd Feb 17, 2023
63814af
Update packages/common/cache/interceptors/cache.interceptor.ts
Flusinerd Feb 19, 2023
e7b38a0
refactor(common): refactor cache-manager version detection
Flusinerd Feb 19, 2023
7ef71f3
fix(common): fix cache-manager version detection running every request
Flusinerd Feb 19, 2023
ee11bea
Update packages/common/cache/interceptors/cache.interceptor.ts
kamilmysliwiec Mar 15, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 35 additions & 0 deletions integration/cache/e2e/custom-ttl.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
import { INestApplication } from '@nestjs/common';
import { Test } from '@nestjs/testing';
import * as request from 'supertest';
import { CustomTtlModule } from '../src/custom-ttl/custom-ttl.module';

describe('Caching Custom TTL', () => {
let server;
let app: INestApplication;

beforeEach(async () => {
const module = await Test.createTestingModule({
imports: [CustomTtlModule],
}).compile();

app = module.createNestApplication();
server = app.getHttpServer();
await app.init();
});

it('should return a differnt value after the TTL is elapsed', async () => {
await request(server).get('/').expect(200, '0');
await new Promise(resolve => setTimeout(resolve, 500));
await request(server).get('/').expect(200, '1');
});

it('should return the cached value within the TTL', async () => {
await request(server).get('/').expect(200, '0');
await new Promise(resolve => setTimeout(resolve, 200));
await request(server).get('/').expect(200, '0');
});

afterEach(async () => {
await app.close();
});
});
20 changes: 20 additions & 0 deletions integration/cache/src/custom-ttl/custom-ttl.controller.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
import {
CacheInterceptor,
CacheTTL,
Controller,
Get,
UseInterceptors,
} from '@nestjs/common';

@Controller()
export class CustomTtlController {
counter = 0;
constructor() {}

@Get()
@CacheTTL(500)
@UseInterceptors(CacheInterceptor)
getNumber() {
return this.counter++;
}
}
8 changes: 8 additions & 0 deletions integration/cache/src/custom-ttl/custom-ttl.module.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
import { CacheModule, Module } from '@nestjs/common';
import { CustomTtlController } from './custom-ttl.controller';

@Module({
imports: [CacheModule.register()],
controllers: [CustomTtlController],
})
export class CustomTtlModule {}
40 changes: 40 additions & 0 deletions integration/cache/src/custom-ttl/tsconfig.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
{
"compilerOptions": {
"module": "commonjs",
"declaration": false,
"noImplicitAny": false,
"removeComments": true,
"noLib": false,
"emitDecoratorMetadata": true,
"experimentalDecorators": true,
"target": "es6",
"sourceMap": true,
"allowJs": true,
"outDir": "./dist",
"paths": {
"@nestjs/common": ["../../packages/common"],
"@nestjs/common/*": ["../../packages/common/*"],
"@nestjs/core": ["../../packages/core"],
"@nestjs/core/*": ["../../packages/core/*"],
"@nestjs/microservices": ["../../packages/microservices"],
"@nestjs/microservices/*": ["../../packages/microservices/*"],
"@nestjs/websockets": ["../../packages/websockets"],
"@nestjs/websockets/*": ["../../packages/websockets/*"],
"@nestjs/testing": ["../../packages/testing"],
"@nestjs/testing/*": ["../../packages/testing/*"],
"@nestjs/platform-express": ["../../packages/platform-express"],
"@nestjs/platform-express/*": ["../../packages/platform-express/*"],
"@nestjs/platform-socket.io": ["../../packages/platform-socket.io"],
"@nestjs/platform-socket.io/*": ["../../packages/platform-socket.io/*"],
"@nestjs/platform-ws": ["../../packages/platform-ws"],
"@nestjs/platform-ws/*": ["../../packages/platform-ws/*"]
}
},
"include": [
"src/**/*",
"e2e/**/*"
],
"exclude": [
"node_modules",
]
}
16 changes: 14 additions & 2 deletions packages/common/cache/interceptors/cache.interceptor.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import { Store } from 'cache-manager';
Flusinerd marked this conversation as resolved.
Show resolved Hide resolved
import { Observable, of } from 'rxjs';
import { tap } from 'rxjs/operators';
import { Inject, Injectable, Optional } from '../../decorators';
Expand All @@ -9,7 +10,8 @@ import {
NestInterceptor,
} from '../../interfaces';
import { Logger } from '../../services/logger.service';
import { isFunction, isNil } from '../../utils/shared.utils';
import { loadPackage } from '../../utils/load-package.util';
import { isFunction, isNil, isNumber } from '../../utils/shared.utils';
import {
CACHE_KEY_METADATA,
CACHE_MANAGER,
Expand All @@ -19,6 +21,13 @@ import {
const HTTP_ADAPTER_HOST = 'HttpAdapterHost';
const REFLECTOR = 'Reflector';

// We need to check if the cache-manager package is v5 or greater
// because the set method signature changed in v5
const cacheManager = loadPackage('cache-manager', 'CacheModule', () =>
require('cache-manager'),
);
const cacheManagerIsv5OrGreater = 'memoryStore' in cacheManager;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

" 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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 👍

Copy link
Member

@micalevisk micalevisk Feb 19, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@Flusinerd Flusinerd Feb 19, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


export interface HttpAdapterHost<T extends HttpServer = any> {
httpAdapter: T;
}
Expand Down Expand Up @@ -65,7 +74,10 @@ export class CacheInterceptor implements NestInterceptor {
return;
}

const args = isNil(ttl) ? [key, response] : [key, response, { ttl }];
const args = [key, response];
if (!isNil(ttl)) {
args.push(cacheManagerIsv5OrGreater ? ttl : { ttl });
}

try {
await this.cacheManager.set(...args);
Expand Down