From 944de7672b00f93ac5b59ddb2b75f4cd6accb02a Mon Sep 17 00:00:00 2001 From: seth-riedel Date: Fri, 13 Dec 2024 14:47:44 -0600 Subject: [PATCH 1/2] CR-263: Optimize recommendations lookup and creation --- .../src/__mocks__/database-recommendation.ts | 1 + .../src/modules/browser/keys/keys.service.ts | 20 ++--- .../database-recommendation.service.ts | 83 +++++++++++++------ .../database-recommendation.repository.ts | 6 +- ...database.recommendation.repository.spec.ts | 8 +- ...ocal.database.recommendation.repository.ts | 21 +++-- .../database-connection.service.spec.ts | 41 ++++----- .../database/database-connection.service.ts | 18 ++-- 8 files changed, 105 insertions(+), 93 deletions(-) diff --git a/redisinsight/api/src/__mocks__/database-recommendation.ts b/redisinsight/api/src/__mocks__/database-recommendation.ts index e52dda8699..750c325c7a 100644 --- a/redisinsight/api/src/__mocks__/database-recommendation.ts +++ b/redisinsight/api/src/__mocks__/database-recommendation.ts @@ -34,6 +34,7 @@ export const mockDatabaseRecommendationService = () => ({ create: jest.fn(), list: jest.fn(), check: jest.fn(), + checkMulti: jest.fn(), read: jest.fn(), }); diff --git a/redisinsight/api/src/modules/browser/keys/keys.service.ts b/redisinsight/api/src/modules/browser/keys/keys.service.ts index 548586bc84..1eecd9943f 100644 --- a/redisinsight/api/src/modules/browser/keys/keys.service.ts +++ b/redisinsight/api/src/modules/browser/keys/keys.service.ts @@ -145,21 +145,17 @@ export class KeysService { const result = await this.keyInfoProvider.getStrategy(type).getInfo(client, key, type); this.logger.debug('Succeed to get key info', clientMetadata); - this.recommendationService.check( - clientMetadata, - RECOMMENDATION_NAMES.BIG_SETS, - result, - ); - this.recommendationService.check( - clientMetadata, - RECOMMENDATION_NAMES.BIG_STRINGS, - result, - ); - this.recommendationService.check( + + this.recommendationService.checkMulti( clientMetadata, - RECOMMENDATION_NAMES.COMPRESSION_FOR_LIST, + [ + RECOMMENDATION_NAMES.BIG_SETS, + RECOMMENDATION_NAMES.BIG_STRINGS, + RECOMMENDATION_NAMES.COMPRESSION_FOR_LIST, + ], result, ); + return plainToClass(GetKeyInfoResponse, result); } catch (error) { this.logger.error('Failed to get key info.', error, clientMetadata); diff --git a/redisinsight/api/src/modules/database-recommendation/database-recommendation.service.ts b/redisinsight/api/src/modules/database-recommendation/database-recommendation.service.ts index 8a4fad501e..f9f417c667 100644 --- a/redisinsight/api/src/modules/database-recommendation/database-recommendation.service.ts +++ b/redisinsight/api/src/modules/database-recommendation/database-recommendation.service.ts @@ -52,17 +52,38 @@ export class DatabaseRecommendationService { return this.databaseRecommendationRepository.list({ ...clientMetadata, db }); } - /** - * Check recommendation condition - * @param clientMetadata - * @param recommendationName - * @param data - */ - public async check( - clientMetadata: ClientMetadata, + private async checkRecommendation( recommendationName: string, - data: any, + exists: boolean, + clientMetadata: ClientMetadata, data: any, ): Promise { + if (exists) { + return null; + } + + const recommendation = await this.scanner.determineRecommendation( + clientMetadata.sessionMetadata, + recommendationName, + data, + ); + + if (recommendation) { + const entity = plainToClass( + DatabaseRecommendation, + { databaseId: clientMetadata?.databaseId, ...recommendation }, + ); + + return await this.create(clientMetadata, entity); + } + + return null; + } + + public async checkMulti( + clientMetadata: ClientMetadata, + recommendationNames: string[], + data: any, + ): Promise> { try { const newClientMetadata = { ...clientMetadata, @@ -72,32 +93,40 @@ export class DatabaseRecommendationService { }; const isRecommendationExist = await this.databaseRecommendationRepository.isExist( newClientMetadata, - recommendationName, + recommendationNames, ); - if (!isRecommendationExist) { - const recommendation = await this.scanner.determineRecommendation( - clientMetadata.sessionMetadata, - recommendationName, - data, - ); - - if (recommendation) { - const entity = plainToClass( - DatabaseRecommendation, - { databaseId: newClientMetadata?.databaseId, ...recommendation }, - ); - - return await this.create(newClientMetadata, entity); - } - } - return null; + const results = await Promise.all( + recommendationNames.map( + (name) => this.checkRecommendation(name, isRecommendationExist[name], newClientMetadata, data), + ), + ); + + return results.reduce((acc, result, idx) => ({ + ...acc, + [recommendationNames[idx]]: result, + }), {} as Map); } catch (e) { this.logger.warn('Unable to check recommendation', e, clientMetadata); return null; } } + /** + * Check recommendation condition + * @param clientMetadata + * @param recommendationName + * @param data + */ + public async check( + clientMetadata: ClientMetadata, + recommendationName: string, + data: any, + ): Promise { + const result = await this.checkMulti(clientMetadata, [recommendationName], data); + return result[recommendationName]; + } + /** * Mark all recommendations as read for particular database * @param clientMetadata diff --git a/redisinsight/api/src/modules/database-recommendation/repositories/database-recommendation.repository.ts b/redisinsight/api/src/modules/database-recommendation/repositories/database-recommendation.repository.ts index c03c489e92..1bf325626a 100644 --- a/redisinsight/api/src/modules/database-recommendation/repositories/database-recommendation.repository.ts +++ b/redisinsight/api/src/modules/database-recommendation/repositories/database-recommendation.repository.ts @@ -44,10 +44,10 @@ export abstract class DatabaseRecommendationRepository { /** * Check is recommendation already exist in repository * @param clientMetadata - * @param name - * @return Boolean + * @param names + * @return Map */ - abstract isExist(clientMetadata: ClientMetadata, name: string): Promise; + abstract isExist(clientMetadata: ClientMetadata, names: string[]): Promise>; /** * Get database recommendation by id diff --git a/redisinsight/api/src/modules/database-recommendation/repositories/local.database.recommendation.repository.spec.ts b/redisinsight/api/src/modules/database-recommendation/repositories/local.database.recommendation.repository.spec.ts index 39ce13b00b..f2b25e16e9 100644 --- a/redisinsight/api/src/modules/database-recommendation/repositories/local.database.recommendation.repository.spec.ts +++ b/redisinsight/api/src/modules/database-recommendation/repositories/local.database.recommendation.repository.spec.ts @@ -64,17 +64,17 @@ describe('LocalDatabaseRecommendationRepository', () => { describe('isExist', () => { it('should return true when receive database entity', async () => { - expect(await service.isExist(mockClientMetadata, mockRecommendationName)).toEqual(true); + expect(await service.isExist(mockClientMetadata, [mockRecommendationName])).toEqual({ [mockRecommendationName]: true }); }); it('should return false when no database received', async () => { repository.findOneBy.mockResolvedValueOnce(null); - expect(await service.isExist(mockClientMetadata, mockRecommendationName)).toEqual(false); + expect(await service.isExist(mockClientMetadata, [mockRecommendationName])).toEqual({ [mockRecommendationName]: false }); }); - it('should return false when received error', async () => { + it('should return empty object when received error', async () => { repository.findOneBy.mockRejectedValueOnce(new Error()); - expect(await service.isExist(mockClientMetadata, mockRecommendationName)).toEqual(false); + expect(await service.isExist(mockClientMetadata, [mockRecommendationName])).toEqual({}); }); }); diff --git a/redisinsight/api/src/modules/database-recommendation/repositories/local.database.recommendation.repository.ts b/redisinsight/api/src/modules/database-recommendation/repositories/local.database.recommendation.repository.ts index 26e9f95278..e933e56a2e 100644 --- a/redisinsight/api/src/modules/database-recommendation/repositories/local.database.recommendation.repository.ts +++ b/redisinsight/api/src/modules/database-recommendation/repositories/local.database.recommendation.repository.ts @@ -147,22 +147,25 @@ export class LocalDatabaseRecommendationRepository extends DatabaseRecommendatio /** * Check is recommendation exist in database * @param clientMetadata - * @param name + * @param names */ async isExist( clientMetadata: ClientMetadata, - name: string, - ): Promise { + names: string[], + ): Promise> { const { databaseId } = clientMetadata; try { - this.logger.debug(`Checking is recommendation ${name} exist`, clientMetadata); - const recommendation = await this.repository.findOneBy({ databaseId, name }); + this.logger.debug('Checking if recommendations exist', clientMetadata, names); - this.logger.debug(`Succeed to check is recommendation ${name} exist'`, clientMetadata); - return !!recommendation; + const results = await Promise.all(names.map((name) => this.repository.findOneBy({ databaseId, name }))); + + return results.reduce((acc, result, idx) => ({ + ...acc, + [names[idx]]: !!result, + }), {} as Map); } catch (err) { - this.logger.error(`Failed to check is recommendation ${name} exist'`, err, clientMetadata); - return false; + this.logger.error('Failed to check existence of recommendations', err, clientMetadata, names); + return {} as Map; } } diff --git a/redisinsight/api/src/modules/database/database-connection.service.spec.ts b/redisinsight/api/src/modules/database/database-connection.service.spec.ts index 0b01c8f2c3..f75aef4349 100644 --- a/redisinsight/api/src/modules/database/database-connection.service.spec.ts +++ b/redisinsight/api/src/modules/database/database-connection.service.spec.ts @@ -90,21 +90,15 @@ describe('DatabaseConnectionService', () => { it('should call recommendationService', async () => { expect(await service.connect(mockCommonClientMetadata)).toEqual(undefined); - expect(recommendationService.check).toHaveBeenCalledTimes(3); + expect(recommendationService.checkMulti).toHaveBeenCalledTimes(1); - expect(recommendationService.check).toBeCalledWith( - mockCommonClientMetadata, - RECOMMENDATION_NAMES.REDIS_VERSION, - mockRedisGeneralInfo, - ); - expect(recommendationService.check).toBeCalledWith( - mockCommonClientMetadata, - RECOMMENDATION_NAMES.LUA_SCRIPT, - mockRedisGeneralInfo, - ); - expect(recommendationService.check).toBeCalledWith( + expect(recommendationService.checkMulti).toBeCalledWith( mockCommonClientMetadata, - RECOMMENDATION_NAMES.BIG_AMOUNT_OF_CONNECTED_CLIENTS, + [ + RECOMMENDATION_NAMES.REDIS_VERSION, + RECOMMENDATION_NAMES.LUA_SCRIPT, + RECOMMENDATION_NAMES.BIG_AMOUNT_OF_CONNECTED_CLIENTS, + ], mockRedisGeneralInfo, ); }); @@ -116,21 +110,16 @@ describe('DatabaseConnectionService', () => { expect(await service.connect(mockCommonClientMetadata)).toEqual(undefined); - expect(recommendationService.check).toHaveBeenCalledTimes(4); + expect(recommendationService.check).toHaveBeenCalledTimes(1); + expect(recommendationService.checkMulti).toHaveBeenCalledTimes(1); - expect(recommendationService.check).toBeCalledWith( - mockCommonClientMetadata, - RECOMMENDATION_NAMES.REDIS_VERSION, - mockRedisGeneralInfo, - ); - expect(recommendationService.check).toBeCalledWith( - mockCommonClientMetadata, - RECOMMENDATION_NAMES.LUA_SCRIPT, - mockRedisGeneralInfo, - ); - expect(recommendationService.check).toBeCalledWith( + expect(recommendationService.checkMulti).toBeCalledWith( mockCommonClientMetadata, - RECOMMENDATION_NAMES.BIG_AMOUNT_OF_CONNECTED_CLIENTS, + [ + RECOMMENDATION_NAMES.REDIS_VERSION, + RECOMMENDATION_NAMES.LUA_SCRIPT, + RECOMMENDATION_NAMES.BIG_AMOUNT_OF_CONNECTED_CLIENTS, + ], mockRedisGeneralInfo, ); diff --git a/redisinsight/api/src/modules/database/database-connection.service.ts b/redisinsight/api/src/modules/database/database-connection.service.ts index 8770814cf3..627d061444 100644 --- a/redisinsight/api/src/modules/database/database-connection.service.ts +++ b/redisinsight/api/src/modules/database/database-connection.service.ts @@ -54,19 +54,13 @@ export class DatabaseConnectionService { const generalInfo = await this.databaseInfoProvider.getRedisGeneralInfo(client); - this.recommendationService.check( + this.recommendationService.checkMulti( clientMetadata, - RECOMMENDATION_NAMES.REDIS_VERSION, - generalInfo, - ); - this.recommendationService.check( - clientMetadata, - RECOMMENDATION_NAMES.LUA_SCRIPT, - generalInfo, - ); - this.recommendationService.check( - clientMetadata, - RECOMMENDATION_NAMES.BIG_AMOUNT_OF_CONNECTED_CLIENTS, + [ + RECOMMENDATION_NAMES.REDIS_VERSION, + RECOMMENDATION_NAMES.LUA_SCRIPT, + RECOMMENDATION_NAMES.BIG_AMOUNT_OF_CONNECTED_CLIENTS, + ], generalInfo, ); From cd0611c426f82d5dc7e7566a2e72a5b63cbf16b6 Mon Sep 17 00:00:00 2001 From: seth-riedel Date: Sat, 14 Dec 2024 10:21:45 -0600 Subject: [PATCH 2/2] CR-263: Update tests --- .../modules/browser/keys/keys.service.spec.ts | 18 +++------ .../database-recommendation.service.ts | 37 +++++++++--------- .../database-recommendation.repository.ts | 13 +++++-- ...database.recommendation.repository.spec.ts | 24 ++++++++++-- ...ocal.database.recommendation.repository.ts | 38 +++++++++++++++---- 5 files changed, 86 insertions(+), 44 deletions(-) diff --git a/redisinsight/api/src/modules/browser/keys/keys.service.spec.ts b/redisinsight/api/src/modules/browser/keys/keys.service.spec.ts index 3f3c98ec59..f7e4f5c6a3 100644 --- a/redisinsight/api/src/modules/browser/keys/keys.service.spec.ts +++ b/redisinsight/api/src/modules/browser/keys/keys.service.spec.ts @@ -153,19 +153,13 @@ describe('KeysService', () => { getKeyInfoResponse.name, ); - expect(recommendationService.check).toBeCalledWith( - mockBrowserClientMetadata, - RECOMMENDATION_NAMES.BIG_SETS, - result, - ); - expect(recommendationService.check).toBeCalledWith( + expect(recommendationService.checkMulti).toBeCalledWith( mockBrowserClientMetadata, - RECOMMENDATION_NAMES.BIG_STRINGS, - result, - ); - expect(recommendationService.check).toBeCalledWith( - mockBrowserClientMetadata, - RECOMMENDATION_NAMES.COMPRESSION_FOR_LIST, + [ + RECOMMENDATION_NAMES.BIG_SETS, + RECOMMENDATION_NAMES.BIG_STRINGS, + RECOMMENDATION_NAMES.COMPRESSION_FOR_LIST, + ], result, ); }); diff --git a/redisinsight/api/src/modules/database-recommendation/database-recommendation.service.ts b/redisinsight/api/src/modules/database-recommendation/database-recommendation.service.ts index f9f417c667..e2f6721977 100644 --- a/redisinsight/api/src/modules/database-recommendation/database-recommendation.service.ts +++ b/redisinsight/api/src/modules/database-recommendation/database-recommendation.service.ts @@ -57,23 +57,21 @@ export class DatabaseRecommendationService { exists: boolean, clientMetadata: ClientMetadata, data: any, ): Promise { - if (exists) { - return null; - } - - const recommendation = await this.scanner.determineRecommendation( - clientMetadata.sessionMetadata, - recommendationName, - data, - ); - - if (recommendation) { - const entity = plainToClass( - DatabaseRecommendation, - { databaseId: clientMetadata?.databaseId, ...recommendation }, + if (!exists) { + const recommendation = await this.scanner.determineRecommendation( + clientMetadata.sessionMetadata, + recommendationName, + data, ); - return await this.create(clientMetadata, entity); + if (recommendation) { + const entity = plainToClass( + DatabaseRecommendation, + { databaseId: clientMetadata?.databaseId, ...recommendation }, + ); + + return await this.create(clientMetadata, entity); + } } return null; @@ -91,14 +89,19 @@ export class DatabaseRecommendationService { ?? (await this.databaseService.get(clientMetadata.sessionMetadata, clientMetadata.databaseId))?.db ?? 0, }; - const isRecommendationExist = await this.databaseRecommendationRepository.isExist( + const isRecommendationExist = await this.databaseRecommendationRepository.isExistMulti( newClientMetadata, recommendationNames, ); const results = await Promise.all( recommendationNames.map( - (name) => this.checkRecommendation(name, isRecommendationExist[name], newClientMetadata, data), + (name) => this.checkRecommendation( + name, + isRecommendationExist[name], + newClientMetadata, + data, + ), ), ); diff --git a/redisinsight/api/src/modules/database-recommendation/repositories/database-recommendation.repository.ts b/redisinsight/api/src/modules/database-recommendation/repositories/database-recommendation.repository.ts index 1bf325626a..4f2e762f20 100644 --- a/redisinsight/api/src/modules/database-recommendation/repositories/database-recommendation.repository.ts +++ b/redisinsight/api/src/modules/database-recommendation/repositories/database-recommendation.repository.ts @@ -42,12 +42,19 @@ export abstract class DatabaseRecommendationRepository { ): Promise; /** - * Check is recommendation already exist in repository + * Check if recommendation already exist in repository + * @param clientMetadata + * @param name + * @return Boolean + */ + abstract isExist(clientMetadata: ClientMetadata, name: string): Promise; + + /** + * Check if one or more recommendations exist in repository * @param clientMetadata * @param names - * @return Map */ - abstract isExist(clientMetadata: ClientMetadata, names: string[]): Promise>; + abstract isExistMulti(clientMetadata: ClientMetadata, names: string[]): Promise>; /** * Get database recommendation by id diff --git a/redisinsight/api/src/modules/database-recommendation/repositories/local.database.recommendation.repository.spec.ts b/redisinsight/api/src/modules/database-recommendation/repositories/local.database.recommendation.repository.spec.ts index f2b25e16e9..b86c035591 100644 --- a/redisinsight/api/src/modules/database-recommendation/repositories/local.database.recommendation.repository.spec.ts +++ b/redisinsight/api/src/modules/database-recommendation/repositories/local.database.recommendation.repository.spec.ts @@ -64,17 +64,33 @@ describe('LocalDatabaseRecommendationRepository', () => { describe('isExist', () => { it('should return true when receive database entity', async () => { - expect(await service.isExist(mockClientMetadata, [mockRecommendationName])).toEqual({ [mockRecommendationName]: true }); + expect(await service.isExist(mockClientMetadata, mockRecommendationName)).toEqual(true); }); it('should return false when no database received', async () => { repository.findOneBy.mockResolvedValueOnce(null); - expect(await service.isExist(mockClientMetadata, [mockRecommendationName])).toEqual({ [mockRecommendationName]: false }); + expect(await service.isExist(mockClientMetadata, mockRecommendationName)).toEqual(false); }); - it('should return empty object when received error', async () => { + it('should return false when received error', async () => { repository.findOneBy.mockRejectedValueOnce(new Error()); - expect(await service.isExist(mockClientMetadata, [mockRecommendationName])).toEqual({}); + expect(await service.isExist(mockClientMetadata, mockRecommendationName)).toEqual(false); + }); + }); + + describe('isExistMulti', () => { + it('should return results for multiple recommendation names', async () => { + repository.findOneBy.mockResolvedValueOnce(null); + repository.findOneBy.mockResolvedValueOnce({}); + expect(await service.isExistMulti(mockClientMetadata, ['test1', 'test2'])).toEqual({ + test1: false, + test2: true, + }); + }); + + it('should return empty Map when received error', async () => { + repository.findOneBy.mockRejectedValueOnce(new Error()); + expect(await service.isExistMulti(mockClientMetadata, ['test1', 'test2'])).toEqual({}); }); }); diff --git a/redisinsight/api/src/modules/database-recommendation/repositories/local.database.recommendation.repository.ts b/redisinsight/api/src/modules/database-recommendation/repositories/local.database.recommendation.repository.ts index e933e56a2e..a6ad54a244 100644 --- a/redisinsight/api/src/modules/database-recommendation/repositories/local.database.recommendation.repository.ts +++ b/redisinsight/api/src/modules/database-recommendation/repositories/local.database.recommendation.repository.ts @@ -41,7 +41,7 @@ export class LocalDatabaseRecommendationRepository extends DatabaseRecommendatio /** * Save entire entity - * @param _ + * @param sessionMetadata * @param entity */ async create(sessionMetadata: SessionMetadata, entity: DatabaseRecommendation): Promise { @@ -102,7 +102,7 @@ export class LocalDatabaseRecommendationRepository extends DatabaseRecommendatio } /** - * Read all recommendations recommendations + * Read all recommendations * @param clientMetadata */ async read(clientMetadata: ClientMetadata): Promise { @@ -147,24 +147,46 @@ export class LocalDatabaseRecommendationRepository extends DatabaseRecommendatio /** * Check is recommendation exist in database * @param clientMetadata - * @param names + * @param name */ async isExist( clientMetadata: ClientMetadata, - names: string[], - ): Promise> { + name: string, + ): Promise { const { databaseId } = clientMetadata; try { - this.logger.debug('Checking if recommendations exist', clientMetadata, names); + this.logger.debug(`Checking is recommendation ${name} exist`, clientMetadata); + const recommendation = await this.repository.findOneBy({ databaseId, name }); + + this.logger.debug(`Succeed to check is recommendation ${name} exist'`, clientMetadata); + return !!recommendation; + } catch (err) { + this.logger.error(`Failed to check is recommendation ${name} exist'`, err, clientMetadata); + return false; + } + } - const results = await Promise.all(names.map((name) => this.repository.findOneBy({ databaseId, name }))); + /** + * Check if one or more recommendations exist in database + * @param clientMetadata + * @param names + */ + async isExistMulti(clientMetadata: ClientMetadata, names: string[]): Promise> { + const { databaseId } = clientMetadata; + + try { + this.logger.debug('Checking if recommendations exist', names, clientMetadata); + + const results = await Promise.all( + names.map((name) => this.repository.findOneBy({ databaseId, name })), + ); return results.reduce((acc, result, idx) => ({ ...acc, [names[idx]]: !!result, }), {} as Map); } catch (err) { - this.logger.error('Failed to check existence of recommendations', err, clientMetadata, names); + this.logger.error('Failed to check existence of recommendations', err, names, clientMetadata); return {} as Map; } }