From fa168dd6ee205ccf247a9ef1382b6032270332d9 Mon Sep 17 00:00:00 2001 From: RidhamShah Date: Mon, 12 Aug 2024 11:33:32 +0530 Subject: [PATCH 1/7] adding segment list for import --- .../api/controllers/FeatureFlagController.ts | 30 +++--- .../src/api/services/FeatureFlagService.ts | 97 ++++++++++++++----- 2 files changed, 92 insertions(+), 35 deletions(-) diff --git a/backend/packages/Upgrade/src/api/controllers/FeatureFlagController.ts b/backend/packages/Upgrade/src/api/controllers/FeatureFlagController.ts index 0c69cad04d..aae9ad972a 100644 --- a/backend/packages/Upgrade/src/api/controllers/FeatureFlagController.ts +++ b/backend/packages/Upgrade/src/api/controllers/FeatureFlagController.ts @@ -659,14 +659,14 @@ export class FeatureFlagsController { /** * @swagger - * /experiments/{validation}: + * /flags/import/validation: * post: - * description: Validating Experiment + * description: Validating Feature Flag * consumes: * - application/json * parameters: * - in: body - * name: experiments + * name: featureFlags * required: true * schema: * type: array @@ -677,9 +677,9 @@ export class FeatureFlagsController { * type: string * fileContent: * type: string - * description: Experiment Files + * description: Import FeatureFlag Files * tags: - * - Experiments + * - Feature Flags * produces: * - application/json * responses: @@ -692,17 +692,25 @@ export class FeatureFlagsController { * properties: * fileName: * type: string - * error: + * compatibilityType: * type: string + * enum: [compatible, warning, incompatible] * '401': * description: AuthorizationRequiredError * '500': * description: Internal Server Error */ + @Post('/import/validation') + public async validateImportFeatureFlags( + @Body({ validate: true }) featureFlags: FeatureFlagFile[], + @Req() request: AppRequest + ): Promise { + return await this.featureFlagService.validateImportFeatureFlags(featureFlags, request.logger); + } /** * @swagger - * /flags/{validation}: + * /flags/import: * post: * description: Validating Feature Flag * consumes: @@ -743,11 +751,11 @@ export class FeatureFlagsController { * '500': * description: Internal Server Error */ - @Post('/import/validation') - public async validateImportFeatureFlags( + @Post('/import') + public async importFeatureFlags( @Body({ validate: true }) featureFlags: FeatureFlagFile[], @Req() request: AppRequest - ): Promise { - return await this.featureFlagService.validateImportFeatureFlags(featureFlags, request.logger); + ): Promise { + return await this.featureFlagService.importFeatureFlags(featureFlags, request.logger); } } diff --git a/backend/packages/Upgrade/src/api/services/FeatureFlagService.ts b/backend/packages/Upgrade/src/api/services/FeatureFlagService.ts index 389d5854e6..fe4abec486 100644 --- a/backend/packages/Upgrade/src/api/services/FeatureFlagService.ts +++ b/backend/packages/Upgrade/src/api/services/FeatureFlagService.ts @@ -258,7 +258,6 @@ export class FeatureFlagService { throw error; } featureFlagSegmentInclusionOrExclusion.segment = newSegment; - // } try { if (filterType === 'inclusion') { @@ -425,6 +424,56 @@ export class FeatureFlagService { return includedFeatureFlags; } + public async importFeatureFlags(featureFlagFiles: FeatureFlagFile[], logger: UpgradeLogger): Promise { + logger.info({ message: 'Import feature flags' }); + const validatedFlags = await this.validateImportFeatureFlags(featureFlagFiles, logger); + + const validFiles: FeatureFlag[] = featureFlagFiles + .filter((file) => { + const validation = validatedFlags.find((error) => error.fileName === file.fileName); + return validation && validation.compatibilityType !== FF_COMPATIBILITY_TYPE.INCOMPATIBLE; + }) + .map((featureFlagFile) => { + return JSON.parse(featureFlagFile.fileContent); + }); + + const createdFlags = await Promise.all( + validFiles.map(async (featureFlag) => { + const { featureFlagSegmentInclusion, featureFlagSegmentExclusion, ...rest } = featureFlag; + const newFlag = await this.addFeatureFlagInDB(rest as any, logger); + + const inclusionDoc = await Promise.all( + featureFlagSegmentInclusion.map(async (segmentInclusion) => { + segmentInclusion.segment.id = uuid(); + const listInput = { + flagId: newFlag.id, + enabled: false, + listType: segmentInclusion.listType, + list: this.segmentService.convertJSONStringToSegInputValFormat(JSON.stringify(segmentInclusion.segment)), + }; + return await this.addList(listInput, 'inclusion', logger); + }) + ); + + const exclusionDoc = await Promise.all( + featureFlagSegmentExclusion.map(async (segmentExclusion) => { + segmentExclusion.segment.id = uuid(); + const listInput = { + flagId: newFlag.id, + enabled: false, + listType: segmentExclusion.listType, + list: this.segmentService.convertJSONStringToSegInputValFormat(JSON.stringify(segmentExclusion.segment)), + }; + return await this.addList(listInput, 'exclusion', logger); + }) + ); + + return { ...newFlag, featureFlagSegmentInclusion: inclusionDoc, featureFlagSegmentExclusion: exclusionDoc }; + }) + ); + + return createdFlags; + } public async validateImportFeatureFlags( featureFlagFiles: FeatureFlagFile[], logger: UpgradeLogger @@ -491,31 +540,31 @@ export class FeatureFlagService { }); }); - if (segmentValidator) { - return { - fileName: fileName, - compatibilityType: FF_COMPATIBILITY_TYPE.INCOMPATIBLE, - }; - } - - if (!flag.name || !flag.key || !flag.context) { + // check for missing fields + if (!flag.name || !flag.key || !flag.context || segmentValidator) { compatibilityType = FF_COMPATIBILITY_TYPE.INCOMPATIBLE; } else { - const segmentIds = [ - ...flag.featureFlagSegmentInclusion.flatMap((segmentInclusion) => - segmentInclusion.segment.subSegments.map((subSegment) => subSegment.id) - ), - ...flag.featureFlagSegmentExclusion.flatMap((segmentExclusion) => - segmentExclusion.segment.subSegments.map((subSegment) => subSegment.id) - ), - ]; - - const segments = await this.segmentService.getSegmentByIds(segmentIds); - segments.forEach((segment) => { - if (segment == undefined) { - compatibilityType = FF_COMPATIBILITY_TYPE.WARNING; - } - }); + const keyExists = await this.featureFlagRepository.findOne({ key: flag.key }); + + if (keyExists) { + compatibilityType = FF_COMPATIBILITY_TYPE.INCOMPATIBLE; + } else { + const segmentIds = [ + ...flag.featureFlagSegmentInclusion.flatMap((segmentInclusion) => + segmentInclusion.segment.subSegments.map((subSegment) => subSegment.id) + ), + ...flag.featureFlagSegmentExclusion.flatMap((segmentExclusion) => + segmentExclusion.segment.subSegments.map((subSegment) => subSegment.id) + ), + ]; + + const segments = await this.segmentService.getSegmentByIds(segmentIds); + segments.forEach((segment) => { + if (segment == undefined) { + compatibilityType = FF_COMPATIBILITY_TYPE.WARNING; + } + }); + } } return { From c5e397544e9b8957d93c0fa91cd3ed995e0f3d2d Mon Sep 17 00:00:00 2001 From: RidhamShah Date: Tue, 13 Aug 2024 11:05:49 +0530 Subject: [PATCH 2/7] Update import return format for feature flag --- .../api/controllers/FeatureFlagController.ts | 4 +- .../src/api/services/FeatureFlagService.ts | 37 ++++++++++++++----- types/src/Experiment/interfaces.ts | 5 +++ types/src/index.ts | 1 + 4 files changed, 36 insertions(+), 11 deletions(-) diff --git a/backend/packages/Upgrade/src/api/controllers/FeatureFlagController.ts b/backend/packages/Upgrade/src/api/controllers/FeatureFlagController.ts index f5fabb202d..d67bb53522 100644 --- a/backend/packages/Upgrade/src/api/controllers/FeatureFlagController.ts +++ b/backend/packages/Upgrade/src/api/controllers/FeatureFlagController.ts @@ -10,7 +10,7 @@ import { } from './validators/FeatureFlagsPaginatedParamsValidator'; import { FeatureFlagFilterModeUpdateValidator } from './validators/FeatureFlagFilterModeUpdateValidator'; import { AppRequest, PaginationResponse } from '../../types'; -import { SERVER_ERROR, IFeatureFlagFile } from 'upgrade_types'; +import { SERVER_ERROR, IFeatureFlagFile, IImportError } from 'upgrade_types'; import { FeatureFlagValidation, IdValidator, UserParamsValidator } from './validators/FeatureFlagValidator'; import { ExperimentUserService } from '../services/ExperimentUserService'; import { FeatureFlagListValidator } from '../controllers/validators/FeatureFlagListValidator'; @@ -754,7 +754,7 @@ export class FeatureFlagsController { public async importFeatureFlags( @Body({ validate: true }) featureFlags: IFeatureFlagFile[], @Req() request: AppRequest - ): Promise { + ): Promise { return await this.featureFlagService.importFeatureFlags(featureFlags, request.logger); } } diff --git a/backend/packages/Upgrade/src/api/services/FeatureFlagService.ts b/backend/packages/Upgrade/src/api/services/FeatureFlagService.ts index 48486cfde4..48d3299863 100644 --- a/backend/packages/Upgrade/src/api/services/FeatureFlagService.ts +++ b/backend/packages/Upgrade/src/api/services/FeatureFlagService.ts @@ -17,7 +17,14 @@ import { FF_COMPATIBILITY_TYPE, } from '../controllers/validators/FeatureFlagsPaginatedParamsValidator'; import { FeatureFlagListValidator } from '../controllers/validators/FeatureFlagListValidator'; -import { SERVER_ERROR, FEATURE_FLAG_STATUS, FILTER_MODE, SEGMENT_TYPE, IFeatureFlagFile } from 'upgrade_types'; +import { + SERVER_ERROR, + FEATURE_FLAG_STATUS, + FILTER_MODE, + SEGMENT_TYPE, + IFeatureFlagFile, + IImportError, +} from 'upgrade_types'; import { UpgradeLogger } from '../../lib/logger/UpgradeLogger'; import { FeatureFlagValidation } from '../controllers/validators/FeatureFlagValidator'; import { ExperimentUser } from '../models/ExperimentUser'; @@ -423,16 +430,27 @@ export class FeatureFlagService { return includedFeatureFlags; } - public async importFeatureFlags(featureFlagFiles: IFeatureFlagFile[], logger: UpgradeLogger): Promise { + public async importFeatureFlags( + featureFlagFiles: IFeatureFlagFile[], + logger: UpgradeLogger + ): Promise { logger.info({ message: 'Import feature flags' }); const validatedFlags = await this.validateImportFeatureFlags(featureFlagFiles, logger); - const validFiles: FeatureFlag[] = featureFlagFiles - .filter((file) => { - const validation = validatedFlags.find((error) => error.fileName === file.fileName); - return validation && validation.compatibilityType !== FF_COMPATIBILITY_TYPE.INCOMPATIBLE; - }) - .map((featureFlagFile) => { + const fileStatusArray = featureFlagFiles.map((file) => { + const validation = validatedFlags.find((error) => error.fileName === file.fileName); + const isCompatible = validation && validation.compatibilityType !== FF_COMPATIBILITY_TYPE.INCOMPATIBLE; + + return { + fileName: file.fileName, + error: isCompatible ? null : FF_COMPATIBILITY_TYPE.INCOMPATIBLE, + }; + }); + + const validFiles: FeatureFlag[] = fileStatusArray + .filter((fileStatus) => fileStatus.error === null) + .map((fileStatus) => { + const featureFlagFile = featureFlagFiles.find((file) => file.fileName === fileStatus.fileName); return JSON.parse(featureFlagFile.fileContent as string); }); @@ -470,8 +488,9 @@ export class FeatureFlagService { return { ...newFlag, featureFlagSegmentInclusion: inclusionDoc, featureFlagSegmentExclusion: exclusionDoc }; }) ); + logger.info({ message: 'Imported feature flags', details: createdFlags }); - return createdFlags; + return fileStatusArray; } public async validateImportFeatureFlags( featureFlagFiles: IFeatureFlagFile[], diff --git a/types/src/Experiment/interfaces.ts b/types/src/Experiment/interfaces.ts index 9345bf5f0a..2d5faee2b3 100644 --- a/types/src/Experiment/interfaces.ts +++ b/types/src/Experiment/interfaces.ts @@ -250,3 +250,8 @@ export interface IFeatureFlagFile { fileName: string; fileContent: string | ArrayBuffer; } + +export interface IImportError { + fileName: string; + error: string | null; +} diff --git a/types/src/index.ts b/types/src/index.ts index e5df9187fc..aa5cb8b85c 100644 --- a/types/src/index.ts +++ b/types/src/index.ts @@ -67,4 +67,5 @@ export { ILogGroupMetrics, IMenuButtonItem, IFeatureFlagFile, + IImportError, } from './Experiment/interfaces'; From 711138363c4c262fd29f56b4c39ed0c34a18fc36 Mon Sep 17 00:00:00 2001 From: Yagnik Date: Tue, 13 Aug 2024 16:50:28 +0530 Subject: [PATCH 3/7] fetch feature flags after successful import --- .../upgrade/src/app/core/feature-flags/feature-flags.service.ts | 1 - .../import-feature-flag-modal.component.ts | 1 + 2 files changed, 1 insertion(+), 1 deletion(-) diff --git a/frontend/projects/upgrade/src/app/core/feature-flags/feature-flags.service.ts b/frontend/projects/upgrade/src/app/core/feature-flags/feature-flags.service.ts index 88127799e8..47e5530144 100644 --- a/frontend/projects/upgrade/src/app/core/feature-flags/feature-flags.service.ts +++ b/frontend/projects/upgrade/src/app/core/feature-flags/feature-flags.service.ts @@ -60,7 +60,6 @@ export class FeatureFlagsService { searchKey$ = this.store$.pipe(select(selectSearchKey)); sortKey$ = this.store$.pipe(select(selectSortKey)); sortAs$ = this.store$.pipe(select(selectSortAs)); - hasFeatureFlagsCountChanged$ = this.allFeatureFlags$.pipe( pairwise(), diff --git a/frontend/projects/upgrade/src/app/features/dashboard/feature-flags/modals/import-feature-flag-modal/import-feature-flag-modal.component.ts b/frontend/projects/upgrade/src/app/features/dashboard/feature-flags/modals/import-feature-flag-modal/import-feature-flag-modal.component.ts index a1cb87ae51..e17fc9430a 100644 --- a/frontend/projects/upgrade/src/app/features/dashboard/feature-flags/modals/import-feature-flag-modal/import-feature-flag-modal.component.ts +++ b/frontend/projects/upgrade/src/app/features/dashboard/feature-flags/modals/import-feature-flag-modal/import-feature-flag-modal.component.ts @@ -134,6 +134,7 @@ export class ImportFeatureFlagModalComponent { if (importSuccessFiles.length > 0) { importSuccessMsg = `Successfully imported ${importSuccessFiles.length} file/s: ${importSuccessFiles.join(', ')}`; this.closeModal(); + this.featureFlagsService.fetchFeatureFlags(true); } this.notificationService.showSuccess(importSuccessMsg); From cc21f55b62dc41bdc83adf1ddf2c1d714c85c7f5 Mon Sep 17 00:00:00 2001 From: RidhamShah Date: Thu, 22 Aug 2024 14:29:25 +0530 Subject: [PATCH 4/7] make all function of import in a transaction --- .../api/controllers/FeatureFlagController.ts | 4 +- .../FeatureFlagSegmentExclusionRepository.ts | 2 +- .../FeatureFlagSegmentInclusionRepository.ts | 2 +- .../src/api/services/FeatureFlagService.ts | 173 +++++++++++------- .../FeatureFlagInclusionExclusion.ts | 4 +- .../unit/services/FeatureFlagService.test.ts | 2 +- 6 files changed, 112 insertions(+), 75 deletions(-) diff --git a/backend/packages/Upgrade/src/api/controllers/FeatureFlagController.ts b/backend/packages/Upgrade/src/api/controllers/FeatureFlagController.ts index f19e109210..179e01443b 100644 --- a/backend/packages/Upgrade/src/api/controllers/FeatureFlagController.ts +++ b/backend/packages/Upgrade/src/api/controllers/FeatureFlagController.ts @@ -480,7 +480,7 @@ export class FeatureFlagsController { @Body({ validate: true }) inclusionList: FeatureFlagListValidator, @Req() request: AppRequest ): Promise { - return this.featureFlagService.addList(inclusionList, 'inclusion', request.logger); + return this.featureFlagService.addList([inclusionList], 'inclusion', request.logger)[0]; } /** @@ -510,7 +510,7 @@ export class FeatureFlagsController { @Body({ validate: true }) exclusionList: FeatureFlagListValidator, @Req() request: AppRequest ): Promise { - return this.featureFlagService.addList(exclusionList, 'exclusion', request.logger); + return this.featureFlagService.addList([exclusionList], 'exclusion', request.logger)[0]; } /** diff --git a/backend/packages/Upgrade/src/api/repositories/FeatureFlagSegmentExclusionRepository.ts b/backend/packages/Upgrade/src/api/repositories/FeatureFlagSegmentExclusionRepository.ts index 95f36816d2..5da260510a 100644 --- a/backend/packages/Upgrade/src/api/repositories/FeatureFlagSegmentExclusionRepository.ts +++ b/backend/packages/Upgrade/src/api/repositories/FeatureFlagSegmentExclusionRepository.ts @@ -6,7 +6,7 @@ import { FeatureFlagSegmentExclusion } from '../models/FeatureFlagSegmentExclusi @EntityRepository(FeatureFlagSegmentExclusion) export class FeatureFlagSegmentExclusionRepository extends Repository { public async insertData( - data: Partial, + data: Partial[], logger: UpgradeLogger, entityManager: EntityManager ): Promise { diff --git a/backend/packages/Upgrade/src/api/repositories/FeatureFlagSegmentInclusionRepository.ts b/backend/packages/Upgrade/src/api/repositories/FeatureFlagSegmentInclusionRepository.ts index ab032dd213..1d86369e30 100644 --- a/backend/packages/Upgrade/src/api/repositories/FeatureFlagSegmentInclusionRepository.ts +++ b/backend/packages/Upgrade/src/api/repositories/FeatureFlagSegmentInclusionRepository.ts @@ -6,7 +6,7 @@ import { FeatureFlagSegmentInclusion } from '../models/FeatureFlagSegmentInclusi @EntityRepository(FeatureFlagSegmentInclusion) export class FeatureFlagSegmentInclusionRepository extends Repository { public async insertData( - data: Partial, + data: Partial[], logger: UpgradeLogger, entityManager: EntityManager ): Promise { diff --git a/backend/packages/Upgrade/src/api/services/FeatureFlagService.ts b/backend/packages/Upgrade/src/api/services/FeatureFlagService.ts index 5785b56954..8c20212a32 100644 --- a/backend/packages/Upgrade/src/api/services/FeatureFlagService.ts +++ b/backend/packages/Upgrade/src/api/services/FeatureFlagService.ts @@ -7,7 +7,7 @@ import { InjectRepository } from 'typeorm-typedi-extensions'; import { FeatureFlagRepository } from '../repositories/FeatureFlagRepository'; import { FeatureFlagSegmentInclusionRepository } from '../repositories/FeatureFlagSegmentInclusionRepository'; import { FeatureFlagSegmentExclusionRepository } from '../repositories/FeatureFlagSegmentExclusionRepository'; -import { getConnection } from 'typeorm'; +import { EntityManager, getConnection } from 'typeorm'; import { v4 as uuid } from 'uuid'; import { IFeatureFlagSearchParams, @@ -205,25 +205,36 @@ export class FeatureFlagService { return this.updateFeatureFlagInDB(this.featureFlagValidatorToFlag(flagDTO), logger); } - private async addFeatureFlagInDB(flag: FeatureFlag, logger: UpgradeLogger): Promise { + private async addFeatureFlagInDB( + flag: FeatureFlag, + logger: UpgradeLogger, + entityManager?: EntityManager + ): Promise { flag.id = uuid(); // saving feature flag doc - let featureFlagDoc: FeatureFlag; - await getConnection().transaction(async (transactionalEntityManager) => { + + const executeTransaction = async (manager: EntityManager): Promise => { try { - featureFlagDoc = ( - await this.featureFlagRepository.insertFeatureFlag(flag as any, transactionalEntityManager) - )[0]; + const featureFlagDoc = (await this.featureFlagRepository.insertFeatureFlag(flag as any, manager))[0]; + return featureFlagDoc; } catch (err) { const error = new Error(`Error in creating feature flag document "addFeatureFlagInDB" ${err}`); (error as any).type = SERVER_ERROR.QUERY_FAILED; logger.error(error); throw error; } - }); + }; // TODO: Add log for feature flag creation - return featureFlagDoc; + if (entityManager) { + // Use the provided entity manager + return await executeTransaction(entityManager); + } else { + // Create a new transaction if no entity manager is provided + return await getConnection().transaction(async (manager) => { + return await executeTransaction(manager); + }); + } } private async updateFeatureFlagInDB(flag: FeatureFlag, logger: UpgradeLogger): Promise { @@ -255,28 +266,24 @@ export class FeatureFlagService { } public async addList( - listInput: FeatureFlagListValidator, + listsInput: FeatureFlagListValidator[], filterType: string, - logger: UpgradeLogger - ): Promise { + logger: UpgradeLogger, + transactionalEntityManager?: EntityManager + ): Promise<(FeatureFlagSegmentInclusion | FeatureFlagSegmentExclusion)[]> { logger.info({ message: `Add ${filterType} list to feature flag` }); - const createdList = await getConnection().transaction(async (transactionalEntityManager) => { - const featureFlagSegmentInclusionOrExclusion = - filterType === 'inclusion' ? new FeatureFlagSegmentInclusion() : new FeatureFlagSegmentExclusion(); - featureFlagSegmentInclusionOrExclusion.enabled = listInput.enabled; - featureFlagSegmentInclusionOrExclusion.listType = listInput.listType; - const featureFlag = await this.featureFlagRepository.findOne(listInput.flagId); - - featureFlagSegmentInclusionOrExclusion.featureFlag = featureFlag; + const executeTransaction = async (manager: EntityManager) => { // create a new private segment - listInput.list.type = SEGMENT_TYPE.PRIVATE; - let newSegment: Segment; + const segmentsToCreate = listsInput.map((listInput) => { + listInput.list.type = SEGMENT_TYPE.PRIVATE; + return listInput.list; + }); + + let newSegments: Segment[]; try { - newSegment = await this.segmentService.upsertSegmentInPipeline( - listInput.list, - logger, - transactionalEntityManager + newSegments = await Promise.all( + segmentsToCreate.map((segment) => this.segmentService.upsertSegmentInPipeline(segment, logger, manager)) ); } catch (err) { const error = new Error(`Error in creating private segment for feature flag ${filterType} list ${err}`); @@ -284,20 +291,35 @@ export class FeatureFlagService { logger.error(error); throw error; } - featureFlagSegmentInclusionOrExclusion.segment = newSegment; + + const featureFlags = await manager + .getRepository(FeatureFlag) + .findByIds(listsInput.map((listInput) => listInput.flagId)); + + const featureFlagSegmentInclusionOrExclusionArray = listsInput.map((listInput) => { + const featureFlagSegmentInclusionOrExclusion = + filterType === 'inclusion' ? new FeatureFlagSegmentInclusion() : new FeatureFlagSegmentExclusion(); + featureFlagSegmentInclusionOrExclusion.enabled = listInput.enabled; + featureFlagSegmentInclusionOrExclusion.listType = listInput.listType; + featureFlagSegmentInclusionOrExclusion.featureFlag = featureFlags.find((flag) => flag.id === listInput.flagId); + featureFlagSegmentInclusionOrExclusion.segment = newSegments.find( + (segment) => segment.id === listInput.list.id + ); + return featureFlagSegmentInclusionOrExclusion; + }); try { if (filterType === 'inclusion') { await this.featureFlagSegmentInclusionRepository.insertData( - featureFlagSegmentInclusionOrExclusion, + featureFlagSegmentInclusionOrExclusionArray, logger, - transactionalEntityManager + manager ); } else { await this.featureFlagSegmentExclusionRepository.insertData( - featureFlagSegmentInclusionOrExclusion, + featureFlagSegmentInclusionOrExclusionArray, logger, - transactionalEntityManager + manager ); } } catch (err) { @@ -306,9 +328,18 @@ export class FeatureFlagService { logger.error(error); throw error; } - return featureFlagSegmentInclusionOrExclusion; - }); - return createdList; + return featureFlagSegmentInclusionOrExclusionArray; + }; + + if (transactionalEntityManager) { + // Use the provided entity manager + return await executeTransaction(transactionalEntityManager); + } else { + // Create a new transaction if no entity manager is provided + return await getConnection().transaction(async (manager) => { + return await executeTransaction(manager); + }); + } } public async updateList( @@ -468,49 +499,55 @@ export class FeatureFlagService { }; }); - const validFiles: FeatureFlag[] = fileStatusArray + const validFiles: FeatureFlagValidation[] = fileStatusArray .filter((fileStatus) => fileStatus.error === null) .map((fileStatus) => { const featureFlagFile = featureFlagFiles.find((file) => file.fileName === fileStatus.fileName); return JSON.parse(featureFlagFile.fileContent as string); }); - const createdFlags = await Promise.all( - validFiles.map(async (featureFlag) => { - const { featureFlagSegmentInclusion, featureFlagSegmentExclusion, ...rest } = featureFlag; - const newFlag = await this.addFeatureFlagInDB(rest as any, logger); - - const inclusionDoc = await Promise.all( - featureFlagSegmentInclusion.map(async (segmentInclusion) => { - segmentInclusion.segment.id = uuid(); - const listInput = { - flagId: newFlag.id, - enabled: false, - listType: segmentInclusion.listType, - list: this.segmentService.convertJSONStringToSegInputValFormat(JSON.stringify(segmentInclusion.segment)), - }; - return await this.addList(listInput, 'inclusion', logger); - }) - ); + const createdFlags = await getConnection().transaction(async (transactionalEntityManager) => { + return await Promise.all( + validFiles.map(async (featureFlag) => { + const newFlag = await this.addFeatureFlagInDB( + this.featureFlagValidatorToFlag(featureFlag), + logger, + transactionalEntityManager + ); - const exclusionDoc = await Promise.all( - featureFlagSegmentExclusion.map(async (segmentExclusion) => { - segmentExclusion.segment.id = uuid(); - const listInput = { - flagId: newFlag.id, - enabled: false, - listType: segmentExclusion.listType, - list: this.segmentService.convertJSONStringToSegInputValFormat(JSON.stringify(segmentExclusion.segment)), - }; - return await this.addList(listInput, 'exclusion', logger); - }) - ); + const featureFlagSegmentInclusionList = featureFlag.featureFlagSegmentInclusion.map( + (segmentInclusionList) => { + segmentInclusionList.list.id = uuid(); + segmentInclusionList.flagId = newFlag.id; + return segmentInclusionList; + } + ); + const inclusionDoc = await this.addList( + featureFlagSegmentInclusionList, + 'inclusion', + logger, + transactionalEntityManager + ); - return { ...newFlag, featureFlagSegmentInclusion: inclusionDoc, featureFlagSegmentExclusion: exclusionDoc }; - }) - ); - logger.info({ message: 'Imported feature flags', details: createdFlags }); + const featureFlagSegmentExclusionList = featureFlag.featureFlagSegmentExclusion.map( + (segmentExclusionList) => { + segmentExclusionList.list.id = uuid(); + segmentExclusionList.flagId = newFlag.id; + return segmentExclusionList; + } + ); + const exclusionDoc = await await this.addList( + featureFlagSegmentExclusionList, + 'exclusion', + logger, + transactionalEntityManager + ); + return { ...newFlag, featureFlagSegmentInclusion: inclusionDoc, featureFlagSegmentExclusion: exclusionDoc }; + }) + ); + }); + logger.info({ message: 'Imported feature flags', details: createdFlags }); return fileStatusArray; } public async validateImportFeatureFlags( diff --git a/backend/packages/Upgrade/test/integration/FeatureFlags/FeatureFlagInclusionExclusion.ts b/backend/packages/Upgrade/test/integration/FeatureFlags/FeatureFlagInclusionExclusion.ts index 18aef804dc..24515ab1bd 100644 --- a/backend/packages/Upgrade/test/integration/FeatureFlags/FeatureFlagInclusionExclusion.ts +++ b/backend/packages/Upgrade/test/integration/FeatureFlags/FeatureFlagInclusionExclusion.ts @@ -46,8 +46,8 @@ export default async function FeatureFlagInclusionExclusionLogic(): Promise { }); it('should add an include list', async () => { - const result = await service.addList(mockList, 'include', logger); + const result = await service.addList([mockList], 'include', logger); expect(result).toBeTruthy(); }); From 9bc40e3b8b9fe60f93e5d258e6b8c81105b28649 Mon Sep 17 00:00:00 2001 From: RidhamShah Date: Fri, 23 Aug 2024 00:56:50 +0530 Subject: [PATCH 5/7] Solve failing testcases --- .../src/api/controllers/FeatureFlagController.ts | 4 ++-- .../unit/controllers/mocks/FeatureFlagServiceMock.ts | 6 ++++-- .../test/unit/services/FeatureFlagService.test.ts | 10 +++++++--- 3 files changed, 13 insertions(+), 7 deletions(-) diff --git a/backend/packages/Upgrade/src/api/controllers/FeatureFlagController.ts b/backend/packages/Upgrade/src/api/controllers/FeatureFlagController.ts index 179e01443b..a45e2c404e 100644 --- a/backend/packages/Upgrade/src/api/controllers/FeatureFlagController.ts +++ b/backend/packages/Upgrade/src/api/controllers/FeatureFlagController.ts @@ -480,7 +480,7 @@ export class FeatureFlagsController { @Body({ validate: true }) inclusionList: FeatureFlagListValidator, @Req() request: AppRequest ): Promise { - return this.featureFlagService.addList([inclusionList], 'inclusion', request.logger)[0]; + return (await this.featureFlagService.addList([inclusionList], 'inclusion', request.logger))[0]; } /** @@ -510,7 +510,7 @@ export class FeatureFlagsController { @Body({ validate: true }) exclusionList: FeatureFlagListValidator, @Req() request: AppRequest ): Promise { - return this.featureFlagService.addList([exclusionList], 'exclusion', request.logger)[0]; + return (await this.featureFlagService.addList([exclusionList], 'exclusion', request.logger))[0]; } /** diff --git a/backend/packages/Upgrade/test/unit/controllers/mocks/FeatureFlagServiceMock.ts b/backend/packages/Upgrade/test/unit/controllers/mocks/FeatureFlagServiceMock.ts index e1707edf6b..ef32dbed70 100644 --- a/backend/packages/Upgrade/test/unit/controllers/mocks/FeatureFlagServiceMock.ts +++ b/backend/packages/Upgrade/test/unit/controllers/mocks/FeatureFlagServiceMock.ts @@ -7,6 +7,7 @@ import { UpgradeLogger } from '../../../../src/lib/logger/UpgradeLogger'; import { FeatureFlagValidation } from '../../../../src/api/controllers/validators/FeatureFlagValidator'; import { RequestedExperimentUser } from '../../../../src/api/controllers/validators/ExperimentUserValidator'; import { FeatureFlagListValidator } from '../../../../src/api/controllers/validators/FeatureFlagListValidator'; +import { FeatureFlagSegmentInclusion } from '../../../../src/api/models/FeatureFlagSegmentInclusion'; @Service() export default class FeatureFlagServiceMock { @@ -49,8 +50,9 @@ export default class FeatureFlagServiceMock { return Promise.resolve([]); } - public addList(listInput: FeatureFlagListValidator, filterType: string, logger: UpgradeLogger): Promise<[]> { - return Promise.resolve([]); + // eslint-disable-next-line @typescript-eslint/ban-types + public addList(listInput: FeatureFlagListValidator[], filterType: string, logger: UpgradeLogger): Promise<{}[]> { + return Promise.resolve([{}]); } public deleteList(segmentId: string, logger: UpgradeLogger): Promise<[]> { diff --git a/backend/packages/Upgrade/test/unit/services/FeatureFlagService.test.ts b/backend/packages/Upgrade/test/unit/services/FeatureFlagService.test.ts index 783f872f11..af247650af 100644 --- a/backend/packages/Upgrade/test/unit/services/FeatureFlagService.test.ts +++ b/backend/packages/Upgrade/test/unit/services/FeatureFlagService.test.ts @@ -1,5 +1,5 @@ import * as sinon from 'sinon'; -import { Connection, ConnectionManager } from 'typeorm'; +import { Connection, ConnectionManager, getRepository } from 'typeorm'; import { Test, TestingModuleBuilder } from '@nestjs/testing'; import { getRepositoryToken } from '@nestjs/typeorm'; @@ -56,7 +56,7 @@ describe('Feature Flag Service Testing', () => { const mockList = new FeatureFlagListValidator(); mockList.enabled = true; - mockList.flagId = uuid(); + mockList.flagId = mockFlag1.id; mockList.listType = 'individual'; mockList.list = { name: 'name', @@ -86,7 +86,11 @@ describe('Feature Flag Service Testing', () => { getMany: jest.fn().mockResolvedValue(mockFlagArr), }; - const entityManagerMock = { createQueryBuilder: () => queryBuilderMock }; + const entityManagerMock = { + createQueryBuilder: () => queryBuilderMock, + getRepository: jest.fn().mockReturnThis(), + findByIds: jest.fn().mockResolvedValue([mockFlag1]), + }; const sandbox = sinon.createSandbox(); sandbox.stub(ConnectionManager.prototype, 'get').returns({ transaction: jest.fn(async (passedFunction) => await passedFunction(entityManagerMock)), From 661b1a83ad4e099c848128242eb2bbe0a29d4893 Mon Sep 17 00:00:00 2001 From: RidhamShah Date: Fri, 23 Aug 2024 12:11:10 +0530 Subject: [PATCH 6/7] Update import in FeatureFlagService.ts --- .../src/api/services/FeatureFlagService.ts | 41 +++++++++++-------- 1 file changed, 25 insertions(+), 16 deletions(-) diff --git a/backend/packages/Upgrade/src/api/services/FeatureFlagService.ts b/backend/packages/Upgrade/src/api/services/FeatureFlagService.ts index 8c20212a32..2a49724016 100644 --- a/backend/packages/Upgrade/src/api/services/FeatureFlagService.ts +++ b/backend/packages/Upgrade/src/api/services/FeatureFlagService.ts @@ -7,7 +7,7 @@ import { InjectRepository } from 'typeorm-typedi-extensions'; import { FeatureFlagRepository } from '../repositories/FeatureFlagRepository'; import { FeatureFlagSegmentInclusionRepository } from '../repositories/FeatureFlagSegmentInclusionRepository'; import { FeatureFlagSegmentExclusionRepository } from '../repositories/FeatureFlagSegmentExclusionRepository'; -import { EntityManager, getConnection } from 'typeorm'; +import { EntityManager, getConnection, In } from 'typeorm'; import { v4 as uuid } from 'uuid'; import { IFeatureFlagSearchParams, @@ -522,12 +522,6 @@ export class FeatureFlagService { return segmentInclusionList; } ); - const inclusionDoc = await this.addList( - featureFlagSegmentInclusionList, - 'inclusion', - logger, - transactionalEntityManager - ); const featureFlagSegmentExclusionList = featureFlag.featureFlagSegmentExclusion.map( (segmentExclusionList) => { @@ -536,12 +530,11 @@ export class FeatureFlagService { return segmentExclusionList; } ); - const exclusionDoc = await await this.addList( - featureFlagSegmentExclusionList, - 'exclusion', - logger, - transactionalEntityManager - ); + + const [inclusionDoc, exclusionDoc] = await Promise.all([ + this.addList(featureFlagSegmentInclusionList, 'inclusion', logger, transactionalEntityManager), + this.addList(featureFlagSegmentExclusionList, 'exclusion', logger, transactionalEntityManager), + ]); return { ...newFlag, featureFlagSegmentInclusion: inclusionDoc, featureFlagSegmentExclusion: exclusionDoc }; }) @@ -555,6 +548,18 @@ export class FeatureFlagService { logger: UpgradeLogger ): Promise { logger.info({ message: 'Validate feature flags' }); + + const featureFlagsIds = featureFlagFiles + .map((featureFlagFile) => { + try { + return JSON.parse(featureFlagFile.fileContent as string).key; + } catch (parseError) { + return null; + } + }) + .filter((key) => key !== null); + const existingFeatureFlags = await this.featureFlagRepository.find({ key: In(featureFlagsIds) }); + const validationErrors = await Promise.allSettled( featureFlagFiles.map(async (featureFlagFile) => { let featureFlag: FeatureFlagValidation; @@ -568,7 +573,7 @@ export class FeatureFlagService { }; } - const error = await this.validateImportFeatureFlag(featureFlagFile.fileName, featureFlag); + const error = await this.validateImportFeatureFlag(featureFlagFile.fileName, featureFlag, existingFeatureFlags); return error; }) ); @@ -585,7 +590,11 @@ export class FeatureFlagService { .filter((error) => error !== null); } - private async validateImportFeatureFlag(fileName: string, flag: FeatureFlagValidation) { + private async validateImportFeatureFlag( + fileName: string, + flag: FeatureFlagValidation, + existingFeatureFlags: FeatureFlag[] + ) { let compatibilityType = FF_COMPATIBILITY_TYPE.COMPATIBLE; flag = plainToClass(FeatureFlagValidation, flag); @@ -596,7 +605,7 @@ export class FeatureFlagService { }); if (compatibilityType === FF_COMPATIBILITY_TYPE.COMPATIBLE) { - const keyExists = await this.featureFlagRepository.findOne({ key: flag.key }); + const keyExists = existingFeatureFlags.find((existingFlag) => existingFlag.key === flag.key); if (keyExists) { compatibilityType = FF_COMPATIBILITY_TYPE.INCOMPATIBLE; From 07f7755a9cd0c1237d2efb0d08617f19a1ed77e8 Mon Sep 17 00:00:00 2001 From: RidhamShah Date: Mon, 26 Aug 2024 10:14:41 +0530 Subject: [PATCH 7/7] updated code to have transaction per import file --- .../src/api/services/FeatureFlagService.ts | 58 +++++++++---------- 1 file changed, 28 insertions(+), 30 deletions(-) diff --git a/backend/packages/Upgrade/src/api/services/FeatureFlagService.ts b/backend/packages/Upgrade/src/api/services/FeatureFlagService.ts index 2a49724016..598d0bfdc1 100644 --- a/backend/packages/Upgrade/src/api/services/FeatureFlagService.ts +++ b/backend/packages/Upgrade/src/api/services/FeatureFlagService.ts @@ -506,40 +506,38 @@ export class FeatureFlagService { return JSON.parse(featureFlagFile.fileContent as string); }); - const createdFlags = await getConnection().transaction(async (transactionalEntityManager) => { - return await Promise.all( - validFiles.map(async (featureFlag) => { - const newFlag = await this.addFeatureFlagInDB( - this.featureFlagValidatorToFlag(featureFlag), - logger, - transactionalEntityManager - ); + const createdFlags = []; - const featureFlagSegmentInclusionList = featureFlag.featureFlagSegmentInclusion.map( - (segmentInclusionList) => { - segmentInclusionList.list.id = uuid(); - segmentInclusionList.flagId = newFlag.id; - return segmentInclusionList; - } - ); + for (const featureFlag of validFiles) { + const createdFlag = await getConnection().transaction(async (transactionalEntityManager) => { + const newFlag = await this.addFeatureFlagInDB( + this.featureFlagValidatorToFlag(featureFlag), + logger, + transactionalEntityManager + ); - const featureFlagSegmentExclusionList = featureFlag.featureFlagSegmentExclusion.map( - (segmentExclusionList) => { - segmentExclusionList.list.id = uuid(); - segmentExclusionList.flagId = newFlag.id; - return segmentExclusionList; - } - ); + const featureFlagSegmentInclusionList = featureFlag.featureFlagSegmentInclusion.map((segmentInclusionList) => { + segmentInclusionList.list.id = uuid(); + segmentInclusionList.flagId = newFlag.id; + return segmentInclusionList; + }); - const [inclusionDoc, exclusionDoc] = await Promise.all([ - this.addList(featureFlagSegmentInclusionList, 'inclusion', logger, transactionalEntityManager), - this.addList(featureFlagSegmentExclusionList, 'exclusion', logger, transactionalEntityManager), - ]); + const featureFlagSegmentExclusionList = featureFlag.featureFlagSegmentExclusion.map((segmentExclusionList) => { + segmentExclusionList.list.id = uuid(); + segmentExclusionList.flagId = newFlag.id; + return segmentExclusionList; + }); - return { ...newFlag, featureFlagSegmentInclusion: inclusionDoc, featureFlagSegmentExclusion: exclusionDoc }; - }) - ); - }); + const [inclusionDoc, exclusionDoc] = await Promise.all([ + this.addList(featureFlagSegmentInclusionList, 'inclusion', logger, transactionalEntityManager), + this.addList(featureFlagSegmentExclusionList, 'exclusion', logger, transactionalEntityManager), + ]); + + return { ...newFlag, featureFlagSegmentInclusion: inclusionDoc, featureFlagSegmentExclusion: exclusionDoc }; + }); + + createdFlags.push(createdFlag); + } logger.info({ message: 'Imported feature flags', details: createdFlags }); return fileStatusArray; }