feat: enforce no transitive parents (#4818)

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);
+};

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<UnleashTransaction>;
 
     private openApiService: OpenApiService;
 
@@ -42,12 +47,18 @@ export default class DependentFeaturesController extends Controller {
 
     constructor(
         config: IUnleashConfig,
-        { dependentFeaturesService, openApiService }: DependentFeaturesServices,
+        {
+            transactionalDependentFeaturesService,
+            openApiService,
+        }: DependentFeaturesServices,
+        startTransaction: TransactionCreator<UnleashTransaction>,
     ) {
         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 {

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<void> {
-        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,
                   };

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<void>;
+    getChildren(parent: string): Promise<string[]>;
 }

src/lib/features/dependent-features/dependent-features-store.ts

@@ -5,6 +5,7 @@ import { IDependentFeaturesStore } from './dependent-features-store-type';
 type SerializableFeatureDependency = Omit<FeatureDependency, 'variants'> & {
     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<string[]> {
+        const rows = await this.db('dependent_features').where(
+            'parent',
+            parent,
+        );
+
+        return rows.map((row) => row.child);
+    }
 }

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,
+    );
+});

src/lib/features/dependent-features/fake-dependent-features-store.ts

@@ -4,4 +4,8 @@ export class FakeDependentFeaturesStore implements IDependentFeaturesStore {
     async upsert(): Promise<void> {
         return Promise.resolve();
     }
+
+    getChildren(): Promise<string[]> {
+        return Promise.resolve([]);
+    }
 }

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);

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,
     };
 };
 

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;
 }