From 06ea70ef004eb907a6888c6f02aa378f72a98ac9 Mon Sep 17 00:00:00 2001 From: Mateusz Kwasniewski Date: Mon, 25 Sep 2023 10:12:32 +0200 Subject: [PATCH] feat: enforce no transitive parents (#4818) --- .../createDependentFeaturesService.ts | 10 +++++++ .../dependent-features-controller.ts | 28 +++++++++++++------ .../dependent-features-service.ts | 21 ++++++++++---- .../dependent-features-store-type.ts | 1 + .../dependent-features-store.ts | 10 +++++++ .../dependent.features.e2e.test.ts | 20 +++++++++++++ .../fake-dependent-features-store.ts | 4 +++ src/lib/routes/admin-api/project/index.ts | 9 +++++- src/lib/services/index.ts | 4 +++ src/lib/types/services.ts | 3 ++ 10 files changed, 95 insertions(+), 15 deletions(-) create mode 100644 src/lib/features/dependent-features/createDependentFeaturesService.ts diff --git a/src/lib/features/dependent-features/createDependentFeaturesService.ts b/src/lib/features/dependent-features/createDependentFeaturesService.ts new file mode 100644 index 0000000000..f0b3afb099 --- /dev/null +++ b/src/lib/features/dependent-features/createDependentFeaturesService.ts @@ -0,0 +1,10 @@ +import { Db } from '../../db/db'; +import { DependentFeaturesService } from './dependent-features-service'; +import { DependentFeaturesStore } from './dependent-features-store'; + +export const createDependentFeaturesService = ( + db: Db, +): DependentFeaturesService => { + const dependentFeaturesStore = new DependentFeaturesStore(db); + return new DependentFeaturesService(dependentFeaturesStore); +}; diff --git a/src/lib/features/dependent-features/dependent-features-controller.ts b/src/lib/features/dependent-features/dependent-features-controller.ts index 5604fd3451..495d6139c3 100644 --- a/src/lib/features/dependent-features/dependent-features-controller.ts +++ b/src/lib/features/dependent-features/dependent-features-controller.ts @@ -17,6 +17,7 @@ import { import { IAuthRequest } from '../../routes/unleash-types'; import { InvalidOperationError } from '../../error'; import { DependentFeaturesService } from './dependent-features-service'; +import { TransactionCreator, UnleashTransaction } from '../../db/transaction'; interface FeatureParams { featureName: string; @@ -28,11 +29,15 @@ const PATH_DEPENDENCIES = `${PATH_FEATURE}/dependencies`; type DependentFeaturesServices = Pick< IUnleashServices, - 'dependentFeaturesService' | 'openApiService' + 'transactionalDependentFeaturesService' | 'openApiService' >; export default class DependentFeaturesController extends Controller { - private dependentFeaturesService: DependentFeaturesService; + private transactionalDependentFeaturesService: ( + db: UnleashTransaction, + ) => DependentFeaturesService; + + private readonly startTransaction: TransactionCreator; private openApiService: OpenApiService; @@ -42,12 +47,18 @@ export default class DependentFeaturesController extends Controller { constructor( config: IUnleashConfig, - { dependentFeaturesService, openApiService }: DependentFeaturesServices, + { + transactionalDependentFeaturesService, + openApiService, + }: DependentFeaturesServices, + startTransaction: TransactionCreator, ) { super(config); - this.dependentFeaturesService = dependentFeaturesService; + this.transactionalDependentFeaturesService = + transactionalDependentFeaturesService; this.openApiService = openApiService; this.flagResolver = config.flagResolver; + this.startTransaction = startTransaction; this.logger = config.getLogger( '/dependent-features/dependent-feature-service.ts', ); @@ -84,13 +95,14 @@ export default class DependentFeaturesController extends Controller { const { variants, enabled, feature } = req.body; if (this.config.flagResolver.isEnabled('dependentFeatures')) { - await this.dependentFeaturesService.upsertFeatureDependency( - featureName, - { + await this.startTransaction(async (tx) => + this.transactionalDependentFeaturesService( + tx, + ).upsertFeatureDependency(featureName, { variants, enabled, feature, - }, + }), ); res.status(200).end(); } else { diff --git a/src/lib/features/dependent-features/dependent-features-service.ts b/src/lib/features/dependent-features/dependent-features-service.ts index d437a108fe..e68787811c 100644 --- a/src/lib/features/dependent-features/dependent-features-service.ts +++ b/src/lib/features/dependent-features/dependent-features-service.ts @@ -1,3 +1,4 @@ +import { InvalidOperationError } from '../../error'; import { CreateDependentFeatureSchema } from '../../openapi'; import { IDependentFeaturesStore } from './dependent-features-store-type'; @@ -17,20 +18,28 @@ export class DependentFeaturesService { } async upsertFeatureDependency( - childFeature: string, + child: string, dependentFeature: CreateDependentFeatureSchema, ): Promise { - const { enabled, feature, variants } = dependentFeature; + const { enabled, feature: parent, variants } = dependentFeature; + + const children = await this.dependentFeaturesStore.getChildren(child); + if (children.length > 0) { + throw new InvalidOperationError( + 'Transitive dependency detected. Cannot add a dependency to the feature that other features depend on.', + ); + } + const featureDependency: FeatureDependency = enabled === false ? { - parent: feature, - child: childFeature, + parent, + child, enabled, } : { - parent: feature, - child: childFeature, + parent, + child, enabled: true, variants, }; diff --git a/src/lib/features/dependent-features/dependent-features-store-type.ts b/src/lib/features/dependent-features/dependent-features-store-type.ts index f82fbc056f..0a96144fd9 100644 --- a/src/lib/features/dependent-features/dependent-features-store-type.ts +++ b/src/lib/features/dependent-features/dependent-features-store-type.ts @@ -2,4 +2,5 @@ import { FeatureDependency } from './dependent-features-service'; export interface IDependentFeaturesStore { upsert(featureDependency: FeatureDependency): Promise; + getChildren(parent: string): Promise; } diff --git a/src/lib/features/dependent-features/dependent-features-store.ts b/src/lib/features/dependent-features/dependent-features-store.ts index cb3574e411..db59c192d4 100644 --- a/src/lib/features/dependent-features/dependent-features-store.ts +++ b/src/lib/features/dependent-features/dependent-features-store.ts @@ -5,6 +5,7 @@ import { IDependentFeaturesStore } from './dependent-features-store-type'; type SerializableFeatureDependency = Omit & { variants?: string; }; + export class DependentFeaturesStore implements IDependentFeaturesStore { private db: Db; @@ -28,4 +29,13 @@ export class DependentFeaturesStore implements IDependentFeaturesStore { .onConflict(['parent', 'child']) .merge(); } + + async getChildren(parent: string): Promise { + const rows = await this.db('dependent_features').where( + 'parent', + parent, + ); + + return rows.map((row) => row.child); + } } diff --git a/src/lib/features/dependent-features/dependent.features.e2e.test.ts b/src/lib/features/dependent-features/dependent.features.e2e.test.ts index 15defbadb0..501d150f3c 100644 --- a/src/lib/features/dependent-features/dependent.features.e2e.test.ts +++ b/src/lib/features/dependent-features/dependent.features.e2e.test.ts @@ -61,3 +61,23 @@ test('should add feature dependency', async () => { variants: ['variantB'], }); }); + +test('should not allow to add a parent dependency to a feature that already has children', async () => { + const grandparent = uuidv4(); + const parent = uuidv4(); + const child = uuidv4(); + await app.createFeature(grandparent); + await app.createFeature(parent); + await app.createFeature(child); + + await addFeatureDependency(child, { + feature: parent, + }); + await addFeatureDependency( + parent, + { + feature: grandparent, + }, + 403, + ); +}); diff --git a/src/lib/features/dependent-features/fake-dependent-features-store.ts b/src/lib/features/dependent-features/fake-dependent-features-store.ts index 27c7eb4a73..c338ccc1b3 100644 --- a/src/lib/features/dependent-features/fake-dependent-features-store.ts +++ b/src/lib/features/dependent-features/fake-dependent-features-store.ts @@ -4,4 +4,8 @@ export class FakeDependentFeaturesStore implements IDependentFeaturesStore { async upsert(): Promise { return Promise.resolve(); } + + getChildren(): Promise { + return Promise.resolve([]); + } } diff --git a/src/lib/routes/admin-api/project/index.ts b/src/lib/routes/admin-api/project/index.ts index 90ae03a60f..15f5d83700 100644 --- a/src/lib/routes/admin-api/project/index.ts +++ b/src/lib/routes/admin-api/project/index.ts @@ -113,7 +113,14 @@ export default class ProjectApi extends Controller { createKnexTransactionStarter(db), ).router, ); - this.use('/', new DependentFeaturesController(config, services).router); + this.use( + '/', + new DependentFeaturesController( + config, + services, + createKnexTransactionStarter(db), + ).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/services/index.ts b/src/lib/services/index.ts index 03265589bd..6a8899e998 100644 --- a/src/lib/services/index.ts +++ b/src/lib/services/index.ts @@ -69,6 +69,7 @@ import { createGetActiveUsers, } from '../features/instance-stats/getActiveUsers'; import { DependentFeaturesService } from '../features/dependent-features/dependent-features-service'; +import { createDependentFeaturesService } from '../features/dependent-features/createDependentFeaturesService'; // TODO: will be moved to scheduler feature directory export const scheduleServices = async ( @@ -299,6 +300,8 @@ export const createServices = ( const dependentFeaturesService = new DependentFeaturesService( stores.dependentFeaturesStore, ); + const transactionalDependentFeaturesService = (txDb: Knex.Transaction) => + createDependentFeaturesService(txDb); return { accessService, @@ -351,6 +354,7 @@ export const createServices = ( transactionalGroupService, privateProjectChecker, dependentFeaturesService, + transactionalDependentFeaturesService, }; }; diff --git a/src/lib/types/services.ts b/src/lib/types/services.ts index e929b06c12..d9e6740280 100644 --- a/src/lib/types/services.ts +++ b/src/lib/types/services.ts @@ -101,4 +101,7 @@ export interface IUnleashServices { transactionalGroupService: (db: Knex.Transaction) => GroupService; privateProjectChecker: IPrivateProjectChecker; dependentFeaturesService: DependentFeaturesService; + transactionalDependentFeaturesService: ( + db: Knex.Transaction, + ) => DependentFeaturesService; }