diff --git a/src/lib/features/dependent-features/dependent-features-read-model-type.ts b/src/lib/features/dependent-features/dependent-features-read-model-type.ts index 1351248956..e08c950010 100644 --- a/src/lib/features/dependent-features/dependent-features-read-model-type.ts +++ b/src/lib/features/dependent-features/dependent-features-read-model-type.ts @@ -2,6 +2,9 @@ import { IDependency } from '../../types'; export interface IDependentFeaturesReadModel { getChildren(parents: string[]): Promise; + // given a list of parents and children verifies if some children would be orphaned after deletion + // we're interested in the list of parents, not orphans + getOrphanParents(parentsAndChildren: string[]): Promise; getParents(child: string): Promise; getParentOptions(child: string): Promise; hasDependencies(feature: string): Promise; diff --git a/src/lib/features/dependent-features/dependent-features-read-model.ts b/src/lib/features/dependent-features/dependent-features-read-model.ts index b52b4484c8..fb03a64016 100644 --- a/src/lib/features/dependent-features/dependent-features-read-model.ts +++ b/src/lib/features/dependent-features/dependent-features-read-model.ts @@ -9,6 +9,21 @@ export class DependentFeaturesReadModel implements IDependentFeaturesReadModel { this.db = db; } + async getOrphanParents(parentsAndChildren: string[]): Promise { + const rows = await this.db('dependent_features') + .distinct('parent') + .whereIn('parent', parentsAndChildren) + .andWhere(function () { + this.whereIn('parent', function () { + this.select('parent') + .from('dependent_features') + .whereNotIn('child', parentsAndChildren); + }); + }); + + return rows.map((row) => row.parent); + } + async getChildren(parents: string[]): Promise { const rows = await this.db('dependent_features').whereIn( 'parent', diff --git a/src/lib/features/dependent-features/fake-dependent-features-read-model.ts b/src/lib/features/dependent-features/fake-dependent-features-read-model.ts index 6f7ca3cc6e..75657618bf 100644 --- a/src/lib/features/dependent-features/fake-dependent-features-read-model.ts +++ b/src/lib/features/dependent-features/fake-dependent-features-read-model.ts @@ -19,4 +19,8 @@ export class FakeDependentFeaturesReadModel hasDependencies(): Promise { return Promise.resolve(false); } + + getOrphanParents(parentsAndChildren: string[]): Promise { + return Promise.resolve([]); + } } diff --git a/src/lib/features/index.ts b/src/lib/features/index.ts index e2ef8a921f..89ebe134ab 100644 --- a/src/lib/features/index.ts +++ b/src/lib/features/index.ts @@ -4,3 +4,4 @@ export * from './feature-toggle/createFeatureToggleService'; export * from './project/createProjectService'; export * from './change-request-access-service/createChangeRequestAccessReadModel'; export * from './segment/createSegmentService'; +export * from './dependent-features/createDependentFeaturesService'; diff --git a/src/lib/services/feature-toggle-service.ts b/src/lib/services/feature-toggle-service.ts index 2dd8196c63..aca23b0795 100644 --- a/src/lib/services/feature-toggle-service.ts +++ b/src/lib/services/feature-toggle-service.ts @@ -256,13 +256,27 @@ class FeatureToggleService { } } - async validateNoChildren(featureNames: string[]): Promise { + async validateNoChildren(featureName: string): Promise { + if (this.flagResolver.isEnabled('dependentFeatures')) { + const children = await this.dependentFeaturesReadModel.getChildren([ + featureName, + ]); + if (children.length > 0) { + throw new InvalidOperationError( + 'You can not archive/delete this feature since other features depend on it.', + ); + } + } + } + + async validateNoOrphanParents(featureNames: string[]): Promise { if (this.flagResolver.isEnabled('dependentFeatures')) { if (featureNames.length === 0) return; - const children = await this.dependentFeaturesReadModel.getChildren( - featureNames, - ); - if (children.length > 0) { + const parents = + await this.dependentFeaturesReadModel.getOrphanParents( + featureNames, + ); + if (parents.length > 0) { throw new InvalidOperationError( featureNames.length > 1 ? `You can not archive/delete those features since other features depend on them.` @@ -1460,7 +1474,7 @@ class FeatureToggleService { }); } - await this.validateNoChildren([featureName]); + await this.validateNoChildren(featureName); await this.featureToggleStore.archive(featureName); @@ -1479,7 +1493,7 @@ class FeatureToggleService { projectId: string, ): Promise { await this.validateFeaturesContext(featureNames, projectId); - await this.validateNoChildren(featureNames); + await this.validateNoOrphanParents(featureNames); const features = await this.featureToggleStore.getAllByNames( featureNames, @@ -1780,7 +1794,7 @@ class FeatureToggleService { // TODO: add project id. async deleteFeature(featureName: string, createdBy: string): Promise { - await this.validateNoChildren([featureName]); + await this.validateNoChildren(featureName); const toggle = await this.featureToggleStore.get(featureName); const tags = await this.tagStore.getAllTagsForFeature(featureName); await this.featureToggleStore.delete(featureName); @@ -1802,7 +1816,7 @@ class FeatureToggleService { createdBy: string, ): Promise { await this.validateFeaturesContext(featureNames, projectId); - await this.validateNoChildren(featureNames); + await this.validateNoOrphanParents(featureNames); const features = await this.featureToggleStore.getAllByNames( featureNames, diff --git a/src/test/e2e/api/admin/project/features.e2e.test.ts b/src/test/e2e/api/admin/project/features.e2e.test.ts index 2ba3d3046f..cee7163a17 100644 --- a/src/test/e2e/api/admin/project/features.e2e.test.ts +++ b/src/test/e2e/api/admin/project/features.e2e.test.ts @@ -295,6 +295,41 @@ test('Should not allow to archive/delete feature with children', async () => { ); }); +test('Should allow to archive/delete feature with children if no orphans are left', async () => { + const parent = uuidv4(); + const child = uuidv4(); + await app.createFeature(parent, 'default'); + await app.createFeature(child, 'default'); + await app.addDependency(child, parent); + + const { body: deleteBody } = await app.request + .post(`/api/admin/projects/default/delete`) + .set('Content-Type', 'application/json') + .send({ features: [parent, child] }) + .expect(200); +}); + +test('Should not allow to archive/delete feature when orphans are left', async () => { + const parent = uuidv4(); + const child = uuidv4(); + const orphan = uuidv4(); + await app.createFeature(parent, 'default'); + await app.createFeature(child, 'default'); + await app.createFeature(orphan, 'default'); + await app.addDependency(child, parent); + await app.addDependency(orphan, parent); + + const { body: deleteBody } = await app.request + .post(`/api/admin/projects/default/delete`) + .set('Content-Type', 'application/json') + .send({ features: [parent, child] }) + .expect(403); + + expect(deleteBody.message).toBe( + 'You can not archive/delete those features since other features depend on them.', + ); +}); + test('should clone feature with parent dependencies', async () => { const parent = uuidv4(); const child = uuidv4();