From 2843388673568e59e359c840b361fc1589ad935d Mon Sep 17 00:00:00 2001 From: Mateusz Kwasniewski Date: Tue, 19 Sep 2023 11:23:21 +0200 Subject: [PATCH] refactor: feature oriented architecture for feature dependencies (#4771) --- .../dependent-features-controller.ts | 102 ++++++++++++++++++ .../dependent-features-service.ts | 32 ++++++ .../features.dependencies.e2e.test.ts | 8 +- src/lib/routes/admin-api/project/index.ts | 2 + .../admin-api/project/project-features.ts | 48 +-------- src/lib/services/feature-toggle-service.ts | 55 +++------- src/lib/services/index.ts | 5 + src/lib/types/services.ts | 2 + 8 files changed, 160 insertions(+), 94 deletions(-) create mode 100644 src/lib/features/dependent-features/dependent-features-controller.ts create mode 100644 src/lib/features/dependent-features/dependent-features-service.ts rename src/{test/e2e/api/admin/project => lib/features/dependent-features}/features.dependencies.e2e.test.ts (82%) diff --git a/src/lib/features/dependent-features/dependent-features-controller.ts b/src/lib/features/dependent-features/dependent-features-controller.ts new file mode 100644 index 0000000000..5604fd3451 --- /dev/null +++ b/src/lib/features/dependent-features/dependent-features-controller.ts @@ -0,0 +1,102 @@ +import { Response } from 'express'; +import Controller from '../../routes/controller'; +import { OpenApiService } from '../../services'; +import { + CREATE_FEATURE, + IFlagResolver, + IUnleashConfig, + IUnleashServices, +} from '../../types'; +import { Logger } from '../../logger'; +import { + CreateDependentFeatureSchema, + createRequestSchema, + emptyResponse, + getStandardResponses, +} from '../../openapi'; +import { IAuthRequest } from '../../routes/unleash-types'; +import { InvalidOperationError } from '../../error'; +import { DependentFeaturesService } from './dependent-features-service'; + +interface FeatureParams { + featureName: string; +} + +const PATH = '/:projectId/features'; +const PATH_FEATURE = `${PATH}/:featureName`; +const PATH_DEPENDENCIES = `${PATH_FEATURE}/dependencies`; + +type DependentFeaturesServices = Pick< + IUnleashServices, + 'dependentFeaturesService' | 'openApiService' +>; + +export default class DependentFeaturesController extends Controller { + private dependentFeaturesService: DependentFeaturesService; + + private openApiService: OpenApiService; + + private flagResolver: IFlagResolver; + + private readonly logger: Logger; + + constructor( + config: IUnleashConfig, + { dependentFeaturesService, openApiService }: DependentFeaturesServices, + ) { + super(config); + this.dependentFeaturesService = dependentFeaturesService; + this.openApiService = openApiService; + this.flagResolver = config.flagResolver; + this.logger = config.getLogger( + '/dependent-features/dependent-feature-service.ts', + ); + + this.route({ + method: 'post', + path: PATH_DEPENDENCIES, + handler: this.addFeatureDependency, + permission: CREATE_FEATURE, + middleware: [ + openApiService.validPath({ + tags: ['Features'], + summary: 'Add a feature dependency.', + description: + 'Add a dependency to a parent feature. Each environment will resolve corresponding dependency independently.', + operationId: 'addFeatureDependency', + requestBody: createRequestSchema( + 'createDependentFeatureSchema', + ), + responses: { + 200: emptyResponse, + ...getStandardResponses(401, 403, 404), + }, + }), + ], + }); + } + + async addFeatureDependency( + req: IAuthRequest, + res: Response, + ): Promise { + const { featureName } = req.params; + const { variants, enabled, feature } = req.body; + + if (this.config.flagResolver.isEnabled('dependentFeatures')) { + await this.dependentFeaturesService.upsertFeatureDependency( + featureName, + { + variants, + enabled, + feature, + }, + ); + res.status(200).end(); + } else { + throw new InvalidOperationError( + 'Dependent features are not enabled', + ); + } + } +} diff --git a/src/lib/features/dependent-features/dependent-features-service.ts b/src/lib/features/dependent-features/dependent-features-service.ts new file mode 100644 index 0000000000..bd512c514a --- /dev/null +++ b/src/lib/features/dependent-features/dependent-features-service.ts @@ -0,0 +1,32 @@ +import { CreateDependentFeatureSchema } from '../../openapi'; + +export type FeatureDependency = + | { + parent: string; + child: string; + enabled: true; + variants?: string[]; + } + | { parent: string; child: string; enabled: false }; +export class DependentFeaturesService { + async upsertFeatureDependency( + parentFeature: string, + dependentFeature: CreateDependentFeatureSchema, + ): Promise { + const { enabled, feature, variants } = dependentFeature; + const featureDependency: FeatureDependency = + enabled === false + ? { + parent: parentFeature, + child: feature, + enabled, + } + : { + parent: parentFeature, + child: feature, + enabled: true, + variants, + }; + console.log(featureDependency); + } +} diff --git a/src/test/e2e/api/admin/project/features.dependencies.e2e.test.ts b/src/lib/features/dependent-features/features.dependencies.e2e.test.ts similarity index 82% rename from src/test/e2e/api/admin/project/features.dependencies.e2e.test.ts rename to src/lib/features/dependent-features/features.dependencies.e2e.test.ts index e41762516d..7ac45786c1 100644 --- a/src/test/e2e/api/admin/project/features.dependencies.e2e.test.ts +++ b/src/lib/features/dependent-features/features.dependencies.e2e.test.ts @@ -1,11 +1,11 @@ import { v4 as uuidv4 } from 'uuid'; -import { CreateDependentFeatureSchema } from '../../../../../lib/openapi'; +import dbInit, { ITestDb } from '../../../test/e2e/helpers/database-init'; import { IUnleashTest, setupAppWithCustomConfig, -} from '../../../helpers/test-helper'; -import dbInit, { ITestDb } from '../../../helpers/database-init'; -import getLogger from '../../../../fixtures/no-logger'; +} from '../../../test/e2e/helpers/test-helper'; +import getLogger from '../../../test/fixtures/no-logger'; +import { CreateDependentFeatureSchema } from '../../openapi'; let app: IUnleashTest; let db: ITestDb; diff --git a/src/lib/routes/admin-api/project/index.ts b/src/lib/routes/admin-api/project/index.ts index 7de2cd7cc5..90ae03a60f 100644 --- a/src/lib/routes/admin-api/project/index.ts +++ b/src/lib/routes/admin-api/project/index.ts @@ -30,6 +30,7 @@ import ProjectArchiveController from './project-archive'; import { createKnexTransactionStarter } from '../../../db/transaction'; import { Db } from '../../../db/db'; import { InvalidOperationError } from '../../../error'; +import DependentFeaturesController from '../../../features/dependent-features/dependent-features-controller'; export default class ProjectApi extends Controller { private projectService: ProjectService; @@ -112,6 +113,7 @@ export default class ProjectApi extends Controller { createKnexTransactionStarter(db), ).router, ); + this.use('/', new DependentFeaturesController(config, services).router); this.use('/', new EnvironmentsController(config, services).router); this.use('/', new ProjectHealthReport(config, services).router); this.use('/', new VariantsController(config, services).router); diff --git a/src/lib/routes/admin-api/project/project-features.ts b/src/lib/routes/admin-api/project/project-features.ts index 5b27f6e455..fc168f0394 100644 --- a/src/lib/routes/admin-api/project/project-features.ts +++ b/src/lib/routes/admin-api/project/project-features.ts @@ -52,8 +52,7 @@ import { TransactionCreator, UnleashTransaction, } from '../../../db/transaction'; -import { BadDataError, InvalidOperationError } from '../../../error'; -import { CreateDependentFeatureSchema } from '../../../openapi/spec/create-dependent-feature-schema'; +import { BadDataError } from '../../../error'; interface FeatureStrategyParams { projectId: string; @@ -100,7 +99,6 @@ const PATH_ENV = `${PATH_FEATURE}/environments/:environment`; const BULK_PATH_ENV = `/:projectId/bulk_features/environments/:environment`; const PATH_STRATEGIES = `${PATH_ENV}/strategies`; const PATH_STRATEGY = `${PATH_STRATEGIES}/:strategyId`; -const PATH_DEPENDENCIES = `${PATH_FEATURE}/dependencies`; type ProjectFeaturesServices = Pick< IUnleashServices, @@ -299,29 +297,6 @@ export default class ProjectFeaturesController extends Controller { ], }); - this.route({ - method: 'post', - path: PATH_DEPENDENCIES, - handler: this.addFeatureDependency, - permission: CREATE_FEATURE, - middleware: [ - openApiService.validPath({ - tags: ['Features'], - summary: 'Add a feature dependency.', - description: - 'Add a dependency to a parent feature. Each environment will resolve corresponding dependency independently.', - operationId: 'addFeatureDependency', - requestBody: createRequestSchema( - 'createDependentFeatureSchema', - ), - responses: { - 200: emptyResponse, - ...getStandardResponses(401, 403, 404), - }, - }), - ], - }); - this.route({ method: 'get', path: PATH_STRATEGY, @@ -997,27 +972,6 @@ export default class ProjectFeaturesController extends Controller { res.status(200).json(updatedStrategy); } - async addFeatureDependency( - req: IAuthRequest, - res: Response, - ): Promise { - const { featureName } = req.params; - const { variants, enabled, feature } = req.body; - - if (this.config.flagResolver.isEnabled('dependentFeatures')) { - await this.featureService.upsertFeatureDependency(featureName, { - variants, - enabled, - feature, - }); - res.status(200).end(); - } else { - throw new InvalidOperationError( - 'Dependent features are not enabled', - ); - } - } - async getFeatureStrategies( req: Request, res: Response, diff --git a/src/lib/services/feature-toggle-service.ts b/src/lib/services/feature-toggle-service.ts index d830162d45..635ff40ffe 100644 --- a/src/lib/services/feature-toggle-service.ts +++ b/src/lib/services/feature-toggle-service.ts @@ -1,6 +1,5 @@ import { CREATE_FEATURE_STRATEGY, - StrategyIds, EnvironmentVariantEvent, FEATURE_UPDATED, FeatureArchivedEvent, @@ -23,6 +22,7 @@ import { IEventStore, IFeatureEnvironmentInfo, IFeatureEnvironmentStore, + IFeatureNaming, IFeatureOverview, IFeatureStrategy, IFeatureTagStore, @@ -33,29 +33,29 @@ import { IProjectStore, ISegment, IStrategyConfig, + IStrategyStore, IUnleashConfig, IUnleashStores, IVariant, + PotentiallyStaleOnEvent, Saved, SKIP_CHANGE_REQUEST, + StrategiesOrderChangedEvent, + StrategyIds, Unsaved, WeightType, - StrategiesOrderChangedEvent, - PotentiallyStaleOnEvent, - IStrategyStore, - IFeatureNaming, } from '../types'; import { Logger } from '../logger'; -import { PatternError } from '../error'; +import { + ForbiddenError, + FOREIGN_KEY_VIOLATION, + OperationDeniedError, + PatternError, + PermissionError, +} from '../error'; import BadDataError from '../error/bad-data-error'; import NameExistsError from '../error/name-exists-error'; import InvalidOperationError from '../error/invalid-operation-error'; -import { - FOREIGN_KEY_VIOLATION, - OperationDeniedError, - PermissionError, - ForbiddenError, -} from '../error'; import { constraintSchema, featureMetadataSchema, @@ -96,7 +96,6 @@ import { ISegmentService } from 'lib/segments/segment-service-interface'; import { IChangeRequestAccessReadModel } from '../features/change-request-access-service/change-request-access-read-model'; import { checkFeatureFlagNamesAgainstPattern } from '../features/feature-naming-pattern/feature-naming-validation'; import { IPrivateProjectChecker } from '../features/private-project/privateProjectCheckerType'; -import { CreateDependentFeatureSchema } from '../openapi'; interface IFeatureContext { featureName: string; @@ -123,15 +122,6 @@ export type FeatureNameCheckResultWithFeaturePattern = featureNaming: IFeatureNaming; }; -export type FeatureDependency = - | { - parent: string; - child: string; - enabled: true; - variants?: string[]; - } - | { parent: string; child: string; enabled: false }; - const oneOf = (values: string[], match: string) => { return values.some((value) => value === match); }; @@ -2211,27 +2201,6 @@ class FeatureToggleService { ); } } - - async upsertFeatureDependency( - parentFeature: string, - dependentFeature: CreateDependentFeatureSchema, - ): Promise { - const { enabled, feature, variants } = dependentFeature; - const featureDependency: FeatureDependency = - enabled === false - ? { - parent: parentFeature, - child: feature, - enabled, - } - : { - parent: parentFeature, - child: feature, - enabled: true, - variants, - }; - console.log(featureDependency); - } } export default FeatureToggleService; diff --git a/src/lib/services/index.ts b/src/lib/services/index.ts index a58ff3e6f0..4cd83d2b2f 100644 --- a/src/lib/services/index.ts +++ b/src/lib/services/index.ts @@ -68,6 +68,7 @@ import { createFakeGetActiveUsers, createGetActiveUsers, } from '../features/instance-stats/getActiveUsers'; +import { DependentFeaturesService } from '../features/dependent-features/dependent-features-service'; // TODO: will be moved to scheduler feature directory export const scheduleServices = async ( @@ -289,6 +290,8 @@ export const createServices = ( const eventAnnouncerService = new EventAnnouncerService(stores, config); + const dependentFeaturesService = new DependentFeaturesService(); + return { accessService, accountService, @@ -339,6 +342,7 @@ export const createServices = ( transactionalFeatureToggleService, transactionalGroupService, privateProjectChecker, + dependentFeaturesService, }; }; @@ -382,4 +386,5 @@ export { InstanceStatsService, FavoritesService, SchedulerService, + DependentFeaturesService, }; diff --git a/src/lib/types/services.ts b/src/lib/types/services.ts index b9e1284e08..e929b06c12 100644 --- a/src/lib/types/services.ts +++ b/src/lib/types/services.ts @@ -44,6 +44,7 @@ import { ISegmentService } from '../segments/segment-service-interface'; import ConfigurationRevisionService from '../features/feature-toggle/configuration-revision-service'; import EventAnnouncerService from 'lib/services/event-announcer-service'; import { IPrivateProjectChecker } from '../features/private-project/privateProjectCheckerType'; +import { DependentFeaturesService } from '../features/dependent-features/dependent-features-service'; export interface IUnleashServices { accessService: AccessService; @@ -99,4 +100,5 @@ export interface IUnleashServices { ) => FeatureToggleService; transactionalGroupService: (db: Knex.Transaction) => GroupService; privateProjectChecker: IPrivateProjectChecker; + dependentFeaturesService: DependentFeaturesService; }