fix(core): Handle empty keys in cache service (no-changelog) (#6854)

* fix handle empty keys in cache service

* add test

* add cache mock test

* add simpler mocking, and add tests for all the updated methods

* don't use RedisStore specifically in the mock

---------

Co-authored-by: कारतोफ्फेलस्क्रिप्ट™ <aditya@netroy.in>

packages/cli/src/services/cache.service.ts

@@ -80,6 +80,9 @@ export class CacheService {
 			refreshTtl?: number;
 		} = {},
 	): Promise<unknown> {
+		if (!key || key.length === 0) {
+			return;
+		}
 		const value = await this.cache?.store.get(key);
 		if (value !== undefined) {
 			return value;
@@ -113,6 +116,9 @@ export class CacheService {
 			refreshTtl?: number;
 		} = {},
 	): Promise<unknown[]> {
+		if (keys.length === 0) {
+			return [];
+		}
 		let values = await this.cache?.store.mget(...keys);
 		if (values === undefined) {
 			values = keys.map(() => undefined);
@@ -163,6 +169,9 @@ export class CacheService {
 		if (!this.cache) {
 			await this.init();
 		}
+		if (!key || key.length === 0) {
+			return;
+		}
 		if (value === undefined || value === null) {
 			return;
 		}
@@ -183,8 +192,13 @@ export class CacheService {
 		if (!this.cache) {
 			await this.init();
 		}
+		if (values.length === 0) {
+			return;
+		}
 		// eslint-disable-next-line @typescript-eslint/naming-convention
-		const nonNullValues = values.filter(([_key, value]) => value !== undefined && value !== null);
+		const nonNullValues = values.filter(
+			([key, value]) => value !== undefined && value !== null && key && key.length > 0,
+		);
 		if (this.isRedisCache()) {
 			// eslint-disable-next-line @typescript-eslint/naming-convention
 			nonNullValues.forEach(([_key, value]) => {
@@ -201,6 +215,9 @@ export class CacheService {
 	 * @param key The key to delete
 	 */
 	async delete(key: string): Promise<void> {
+		if (!key || key.length === 0) {
+			return;
+		}
 		await this.cache?.store.del(key);
 	}
 
@@ -209,6 +226,9 @@ export class CacheService {
 	 * @param keys List of keys to delete
 	 */
 	async deleteMany(keys: string[]): Promise<void> {
+		if (keys.length === 0) {
+			return;
+		}
 		return this.cache?.store.mdel(...keys);
 	}
 

packages/cli/test/unit/services/cache-mock.service.test.ts

@@ -0,0 +1,67 @@
+import Container from 'typedi';
+import { mock } from 'jest-mock-extended';
+import { CacheService } from '@/services/cache.service';
+
+const cacheService = Container.get(CacheService);
+const store = mock<NonNullable<CacheService['cache']>['store']>({ isCacheable: () => true });
+Object.assign(cacheService, { cache: { store } });
+
+describe('CacheService (Mock)', () => {
+	beforeEach(() => jest.clearAllMocks());
+
+	describe('should prevent use of empty keys', () => {
+		test('get', async () => {
+			await cacheService.get('');
+			expect(store.get).not.toHaveBeenCalled();
+
+			await cacheService.get('key');
+			expect(store.get).toHaveBeenCalledWith('key');
+		});
+
+		test('getMany', async () => {
+			await cacheService.getMany([]);
+			expect(store.mget).not.toHaveBeenCalled();
+
+			await cacheService.getMany(['key1', 'key2']);
+			expect(store.mget).toHaveBeenCalledWith('key1', 'key2');
+		});
+
+		test('set', async () => {
+			await cacheService.set('', '');
+			expect(store.set).not.toHaveBeenCalled();
+
+			await cacheService.set('key', 'value');
+			expect(store.set).toHaveBeenCalledWith('key', 'value', undefined);
+
+			await cacheService.set('key', 'value', 123);
+			expect(store.set).toHaveBeenCalledWith('key', 'value', 123);
+		});
+
+		test('setMany', async () => {
+			await cacheService.setMany([]);
+			expect(store.mset).not.toHaveBeenCalled();
+
+			await cacheService.setMany([['key', 'value']]);
+			expect(store.mset).toHaveBeenCalledWith([['key', 'value']], undefined);
+
+			await cacheService.setMany([['key', 'value']], 123);
+			expect(store.mset).toHaveBeenCalledWith([['key', 'value']], 123);
+		});
+
+		test('delete', async () => {
+			await cacheService.delete('');
+			expect(store.del).not.toHaveBeenCalled();
+
+			await cacheService.delete('key');
+			expect(store.del).toHaveBeenCalledWith('key');
+		});
+
+		test('deleteMany', async () => {
+			await cacheService.deleteMany([]);
+			expect(store.mdel).not.toHaveBeenCalled();
+
+			await cacheService.deleteMany(['key1', 'key2']);
+			expect(store.mdel).toHaveBeenCalledWith('key1', 'key2');
+		});
+	});
+});

packages/cli/test/unit/services/cache.service.test.ts

@@ -339,4 +339,33 @@ describe('cacheService', () => {
 		await expect(cacheService.get('undefValue')).resolves.toBeUndefined();
 		await expect(cacheService.get('nullValue')).resolves.toBeUndefined();
 	});
+
+	test('should handle setting empty keys', async () => {
+		await cacheService.set('', null);
+		await expect(cacheService.get('')).resolves.toBeUndefined();
+		await cacheService.setMany([
+			['', 'something'],
+			['', 'something'],
+		]);
+		await expect(cacheService.getMany([''])).resolves.toStrictEqual([undefined]);
+		await cacheService.setMany([]);
+		await expect(cacheService.getMany([])).resolves.toStrictEqual([]);
+	});
+
+	test('should handle setting empty keys (redis)', async () => {
+		config.set('cache.backend', 'redis');
+		config.set('executions.mode', 'queue');
+		await cacheService.destroy();
+		await cacheService.init();
+
+		await cacheService.set('', null);
+		await expect(cacheService.get('')).resolves.toBeUndefined();
+		await cacheService.setMany([
+			['', 'something'],
+			['', 'something'],
+		]);
+		await expect(cacheService.getMany([''])).resolves.toStrictEqual([undefined]);
+		await cacheService.setMany([]);
+		await expect(cacheService.getMany([])).resolves.toStrictEqual([]);
+	});
 });