From 0da48cc0d1552cb3a3c9f0e8a00805bef4cf49fa Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gast=C3=B3n=20Fournier?= Date: Wed, 4 Oct 2023 15:16:37 +0200 Subject: [PATCH] chore: revamp transactional impl (#4916) ## About the changes This transactional implementation decorates a service with a transactional method that removes the need to start transactions in the method using the service. This is a gradual rollout with a feature toggle, just because transactions are not easy. --- .../__snapshots__/create-config.test.ts.snap | 2 + src/lib/db/transaction.ts | 31 +++ .../createExportImportService.ts | 213 ++++++++++-------- .../export-import-controller.ts | 48 +++- src/lib/services/index.ts | 7 +- src/lib/types/experimental.ts | 7 +- src/lib/types/services.ts | 4 + src/server-dev.ts | 1 + 8 files changed, 205 insertions(+), 108 deletions(-) diff --git a/src/lib/__snapshots__/create-config.test.ts.snap b/src/lib/__snapshots__/create-config.test.ts.snap index 334a2e21eae5..2a1727fcd715 100644 --- a/src/lib/__snapshots__/create-config.test.ts.snap +++ b/src/lib/__snapshots__/create-config.test.ts.snap @@ -105,6 +105,7 @@ exports[`should create default config 1`] = ` "proPlanAutoCharge": false, "responseTimeWithAppNameKillSwitch": false, "strictSchemaValidation": false, + "transactionalDecorator": false, "variantTypeNumber": false, }, }, @@ -144,6 +145,7 @@ exports[`should create default config 1`] = ` "proPlanAutoCharge": false, "responseTimeWithAppNameKillSwitch": false, "strictSchemaValidation": false, + "transactionalDecorator": false, "variantTypeNumber": false, }, "externalResolver": { diff --git a/src/lib/db/transaction.ts b/src/lib/db/transaction.ts index 7e8150fdb70d..701f2f1052b2 100644 --- a/src/lib/db/transaction.ts +++ b/src/lib/db/transaction.ts @@ -20,3 +20,34 @@ export const createKnexTransactionStarter = ( } return transaction; }; + +export type DbServiceFactory = (db: Knex) => S; +export type WithTransactional = S & { + transactional: (fn: (service: S) => R) => Promise; +}; + +export function withTransactional( + serviceFactory: (db: Knex) => S, + db: Knex, +): WithTransactional { + const service = serviceFactory(db) as WithTransactional; + + service.transactional = async (fn: (service: S) => R) => + db.transaction(async (trx: Knex.Transaction) => { + const transactionalService = serviceFactory(trx); + return fn(transactionalService); + }); + + return service; +} + +/** Just for testing purposes */ +export function withFakeTransactional(service: S): WithTransactional { + const serviceWithFakeTransactional = service as WithTransactional; + + serviceWithFakeTransactional.transactional = async ( + fn: (service: S) => R, + ) => fn(serviceWithFakeTransactional); + + return serviceWithFakeTransactional; +} diff --git a/src/lib/features/export-import-toggles/createExportImportService.ts b/src/lib/features/export-import-toggles/createExportImportService.ts index 0b0b1b7d5bdb..e8a589f6ba92 100644 --- a/src/lib/features/export-import-toggles/createExportImportService.ts +++ b/src/lib/features/export-import-toggles/createExportImportService.ts @@ -43,6 +43,7 @@ import { createFakePrivateProjectChecker, createPrivateProjectChecker, } from '../private-project/createPrivateProjectChecker'; +import { DbServiceFactory } from 'lib/db/transaction'; export const createFakeExportImportTogglesService = ( config: IUnleashConfig, @@ -127,109 +128,121 @@ export const createFakeExportImportTogglesService = ( return exportImportService; }; -export const createExportImportTogglesService = ( - db: Db, +export const deferredExportImportTogglesService = ( config: IUnleashConfig, -): ExportImportService => { - const { eventBus, getLogger, flagResolver } = config; - const importTogglesStore = new ImportTogglesStore(db); - const featureToggleStore = new FeatureToggleStore(db, eventBus, getLogger); - const tagStore = new TagStore(db, eventBus, getLogger); - const tagTypeStore = new TagTypeStore(db, eventBus, getLogger); - const segmentStore = new SegmentStore( - db, - eventBus, - getLogger, - flagResolver, - ); - const projectStore = new ProjectStore( - db, - eventBus, - getLogger, - flagResolver, - ); - const featureTagStore = new FeatureTagStore(db, eventBus, getLogger); - const strategyStore = new StrategyStore(db, getLogger); - const contextFieldStore = new ContextFieldStore( - db, - getLogger, - flagResolver, - ); - const featureStrategiesStore = new FeatureStrategiesStore( - db, - eventBus, - getLogger, - flagResolver, - ); - const featureEnvironmentStore = new FeatureEnvironmentStore( - db, - eventBus, - getLogger, - ); - const eventStore = new EventStore(db, getLogger); - const accessService = createAccessService(db, config); - const featureToggleService = createFeatureToggleService(db, config); - const privateProjectChecker = createPrivateProjectChecker(db, config); +): DbServiceFactory => { + return (db: Db) => { + const { eventBus, getLogger, flagResolver } = config; + const importTogglesStore = new ImportTogglesStore(db); + const featureToggleStore = new FeatureToggleStore( + db, + eventBus, + getLogger, + ); + const tagStore = new TagStore(db, eventBus, getLogger); + const tagTypeStore = new TagTypeStore(db, eventBus, getLogger); + const segmentStore = new SegmentStore( + db, + eventBus, + getLogger, + flagResolver, + ); + const projectStore = new ProjectStore( + db, + eventBus, + getLogger, + flagResolver, + ); + const featureTagStore = new FeatureTagStore(db, eventBus, getLogger); + const strategyStore = new StrategyStore(db, getLogger); + const contextFieldStore = new ContextFieldStore( + db, + getLogger, + flagResolver, + ); + const featureStrategiesStore = new FeatureStrategiesStore( + db, + eventBus, + getLogger, + flagResolver, + ); + const featureEnvironmentStore = new FeatureEnvironmentStore( + db, + eventBus, + getLogger, + ); + const eventStore = new EventStore(db, getLogger); + const accessService = createAccessService(db, config); + const featureToggleService = createFeatureToggleService(db, config); + const privateProjectChecker = createPrivateProjectChecker(db, config); - const eventService = new EventService( - { - eventStore, - featureTagStore, - }, - config, - ); + const eventService = new EventService( + { + eventStore, + featureTagStore, + }, + config, + ); - const featureTagService = new FeatureTagService( - { - tagStore, - featureTagStore, - featureToggleStore, - }, - { getLogger }, - eventService, - ); - const contextService = new ContextService( - { - projectStore, - contextFieldStore, - featureStrategiesStore, - }, - { getLogger, flagResolver }, - eventService, - privateProjectChecker, - ); - const strategyService = new StrategyService( - { strategyStore }, - { getLogger }, - eventService, - ); - const tagTypeService = new TagTypeService( - { tagTypeStore }, - { getLogger }, - eventService, - ); - const exportImportService = new ExportImportService( - { - importTogglesStore, - featureStrategiesStore, - contextFieldStore, - featureToggleStore, - featureTagStore, - segmentStore, - tagTypeStore, - featureEnvironmentStore, - }, - config, - { - featureToggleService, - featureTagService, - accessService, + const featureTagService = new FeatureTagService( + { + tagStore, + featureTagStore, + featureToggleStore, + }, + { getLogger }, eventService, - contextService, - strategyService, - tagTypeService, - }, - ); + ); + const contextService = new ContextService( + { + projectStore, + contextFieldStore, + featureStrategiesStore, + }, + { getLogger, flagResolver }, + eventService, + privateProjectChecker, + ); + const strategyService = new StrategyService( + { strategyStore }, + { getLogger }, + eventService, + ); + const tagTypeService = new TagTypeService( + { tagTypeStore }, + { getLogger }, + eventService, + ); + const exportImportService = new ExportImportService( + { + importTogglesStore, + featureStrategiesStore, + contextFieldStore, + featureToggleStore, + featureTagStore, + segmentStore, + tagTypeStore, + featureEnvironmentStore, + }, + config, + { + featureToggleService, + featureTagService, + accessService, + eventService, + contextService, + strategyService, + tagTypeService, + }, + ); - return exportImportService; + return exportImportService; + }; +}; +export const createExportImportTogglesService = ( + db: Db, + config: IUnleashConfig, +): ExportImportService => { + const unboundService = deferredExportImportTogglesService(config); + return unboundService(db); }; diff --git a/src/lib/features/export-import-toggles/export-import-controller.ts b/src/lib/features/export-import-toggles/export-import-controller.ts index 7a75645e0cad..8f144ffb5ba8 100644 --- a/src/lib/features/export-import-toggles/export-import-controller.ts +++ b/src/lib/features/export-import-toggles/export-import-controller.ts @@ -3,7 +3,11 @@ import Controller from '../../routes/controller'; import { Logger } from '../../logger'; import ExportImportService from './export-import-service'; import { OpenApiService } from '../../services'; -import { TransactionCreator, UnleashTransaction } from '../../db/transaction'; +import { + TransactionCreator, + UnleashTransaction, + WithTransactional, +} from '../../db/transaction'; import { IUnleashConfig, IUnleashServices, @@ -28,14 +32,19 @@ import ApiUser from '../../types/api-user'; class ExportImportController extends Controller { private logger: Logger; + /** @deprecated gradually rolling out exportImportV2 */ private exportImportService: ExportImportService; + /** @deprecated gradually rolling out exportImportV2 */ private transactionalExportImportService: ( db: UnleashTransaction, ) => ExportImportService; + private exportImportServiceV2: WithTransactional; + private openApiService: OpenApiService; + /** @deprecated gradually rolling out exportImportV2 */ private readonly startTransaction: TransactionCreator; constructor( @@ -43,10 +52,12 @@ class ExportImportController extends Controller { { exportImportService, transactionalExportImportService, + exportImportServiceV2, openApiService, }: Pick< IUnleashServices, | 'exportImportService' + | 'exportImportServiceV2' | 'openApiService' | 'transactionalExportImportService' >, @@ -57,6 +68,7 @@ class ExportImportController extends Controller { this.exportImportService = exportImportService; this.transactionalExportImportService = transactionalExportImportService; + this.exportImportServiceV2 = exportImportServiceV2; this.startTransaction = startTransaction; this.openApiService = openApiService; this.route({ @@ -128,7 +140,13 @@ class ExportImportController extends Controller { this.verifyExportImportEnabled(); const query = req.body; const userName = extractUsername(req); - const data = await this.exportImportService.export(query, userName); + + const useTransactionalDecorator = this.config.flagResolver.isEnabled( + 'transactionalDecorator', + ); + const data = useTransactionalDecorator + ? await this.exportImportServiceV2.export(query, userName) + : await this.exportImportService.export(query, userName); this.openApiService.respondWithValidation( 200, @@ -145,9 +163,17 @@ class ExportImportController extends Controller { this.verifyExportImportEnabled(); const dto = req.body; const { user } = req; - const validation = await this.startTransaction(async (tx) => - this.transactionalExportImportService(tx).validate(dto, user), + + const useTransactionalDecorator = this.config.flagResolver.isEnabled( + 'transactionalDecorator', ); + const validation = useTransactionalDecorator + ? await this.exportImportServiceV2.transactional((service) => + service.validate(dto, user), + ) + : await this.startTransaction(async (tx) => + this.transactionalExportImportService(tx).validate(dto, user), + ); this.openApiService.respondWithValidation( 200, @@ -172,10 +198,20 @@ class ExportImportController extends Controller { const dto = req.body; - await this.startTransaction(async (tx) => - this.transactionalExportImportService(tx).import(dto, user), + const useTransactionalDecorator = this.config.flagResolver.isEnabled( + 'transactionalDecorator', ); + if (useTransactionalDecorator) { + await this.exportImportServiceV2.transactional((service) => + service.import(dto, user), + ); + } else { + await this.startTransaction(async (tx) => + this.transactionalExportImportService(tx).import(dto, user), + ); + } + res.status(200).end(); } diff --git a/src/lib/services/index.ts b/src/lib/services/index.ts index 38c025f45f2d..0df9ca9d70db 100644 --- a/src/lib/services/index.ts +++ b/src/lib/services/index.ts @@ -50,8 +50,10 @@ import { Knex } from 'knex'; import { createExportImportTogglesService, createFakeExportImportTogglesService, + deferredExportImportTogglesService, } from '../features/export-import-toggles/createExportImportService'; import { Db } from '../db/db'; +import { withFakeTransactional, withTransactional } from '../db/transaction'; import { createChangeRequestAccessReadModel, createFakeChangeRequestAccessService, @@ -274,10 +276,12 @@ export const createServices = ( projectService, ); - // TODO: this is a temporary seam to enable packaging by feature const exportImportService = db ? createExportImportTogglesService(db, config) : createFakeExportImportTogglesService(config); + const exportImportServiceV2 = db + ? withTransactional(deferredExportImportTogglesService(config), db) + : withFakeTransactional(createFakeExportImportTogglesService(config)); const transactionalExportImportService = (txDb: Knex.Transaction) => createExportImportTogglesService(txDb, config); const transactionalFeatureToggleService = (txDb: Knex.Transaction) => @@ -380,6 +384,7 @@ export const createServices = ( maintenanceService, exportImportService, transactionalExportImportService, + exportImportServiceV2, schedulerService, configurationRevisionService, transactionalFeatureToggleService, diff --git a/src/lib/types/experimental.ts b/src/lib/types/experimental.ts index c01111566c2d..a02a311e8916 100644 --- a/src/lib/types/experimental.ts +++ b/src/lib/types/experimental.ts @@ -31,7 +31,8 @@ export type IFlagKey = | 'privateProjects' | 'dependentFeatures' | 'datadogJsonTemplate' - | 'disableMetrics'; + | 'disableMetrics' + | 'transactionalDecorator'; export type IFlags = Partial<{ [key in IFlagKey]: boolean | Variant }>; @@ -147,6 +148,10 @@ const flags: IFlags = { process.env.UNLEASH_EXPERIMENTAL_DISABLE_METRICS, false, ), + transactionalDecorator: parseEnvVarBoolean( + process.env.UNLEASH_EXPERIMENTAL_TRANSACTIONAL_DECORATOR, + false, + ), }; export const defaultExperimentalOptions: IExperimentalOptions = { diff --git a/src/lib/types/services.ts b/src/lib/types/services.ts index d9e674028081..ff66b785fee4 100644 --- a/src/lib/types/services.ts +++ b/src/lib/types/services.ts @@ -45,6 +45,7 @@ import ConfigurationRevisionService from '../features/feature-toggle/configurati import EventAnnouncerService from 'lib/services/event-announcer-service'; import { IPrivateProjectChecker } from '../features/private-project/privateProjectCheckerType'; import { DependentFeaturesService } from '../features/dependent-features/dependent-features-service'; +import { WithTransactional } from 'lib/db/transaction'; export interface IUnleashServices { accessService: AccessService; @@ -88,10 +89,13 @@ export interface IUnleashServices { instanceStatsService: InstanceStatsService; favoritesService: FavoritesService; maintenanceService: MaintenanceService; + /** @deprecated prefer exportImportServiceV2, we're doing a gradual rollout */ exportImportService: ExportImportService; + exportImportServiceV2: WithTransactional; configurationRevisionService: ConfigurationRevisionService; schedulerService: SchedulerService; eventAnnouncerService: EventAnnouncerService; + /** @deprecated prefer exportImportServiceV2, we're doing a gradual rollout */ transactionalExportImportService: ( db: Knex.Transaction, ) => ExportImportService; diff --git a/src/server-dev.ts b/src/server-dev.ts index b997164923ec..dd282fb59291 100644 --- a/src/server-dev.ts +++ b/src/server-dev.ts @@ -45,6 +45,7 @@ process.nextTick(async () => { accessOverview: true, datadogJsonTemplate: true, dependentFeatures: true, + transactionalDecorator: true, }, }, authentication: {