Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: prevent adding dependency to archived or removed parent #4987

Merged
merged 2 commits into from
Oct 11, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,8 @@ import {
createChangeRequestAccessReadModel,
createFakeChangeRequestAccessService,
} from '../change-request-access-service/createChangeRequestAccessReadModel';
import { FeaturesReadModel } from '../feature-toggle/features-read-model';
import { FakeFeaturesReadModel } from '../feature-toggle/fake-features-read-model';

export const createDependentFeaturesService = (
db: Db,
Expand All @@ -35,12 +37,14 @@ export const createDependentFeaturesService = (
db,
config,
);
return new DependentFeaturesService(
const featuresReadModel = new FeaturesReadModel(db);
return new DependentFeaturesService({
dependentFeaturesStore,
dependentFeaturesReadModel,
changeRequestAccessReadModel,
featuresReadModel,
eventService,
);
});
};

export const createFakeDependentFeaturesService = (
Expand All @@ -58,11 +62,13 @@ export const createFakeDependentFeaturesService = (
const dependentFeaturesStore = new FakeDependentFeaturesStore();
const dependentFeaturesReadModel = new FakeDependentFeaturesReadModel();
const changeRequestAccessReadModel = createFakeChangeRequestAccessService();
const featuresReadModel = new FakeFeaturesReadModel();

return new DependentFeaturesService(
return new DependentFeaturesService({
dependentFeaturesStore,
dependentFeaturesReadModel,
changeRequestAccessReadModel,
featuresReadModel,
eventService,
);
});
};
37 changes: 29 additions & 8 deletions src/lib/features/dependent-features/dependent-features-service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,15 @@ import { User } from '../../server-impl';
import { SKIP_CHANGE_REQUEST } from '../../types';
import { IChangeRequestAccessReadModel } from '../change-request-access-service/change-request-access-read-model';
import { extractUsernameFromUser } from '../../util';
import { IFeaturesReadModel } from '../feature-toggle/features-read-model-type';

interface IDependentFeaturesServiceDeps {
dependentFeaturesStore: IDependentFeaturesStore;
dependentFeaturesReadModel: IDependentFeaturesReadModel;
changeRequestAccessReadModel: IChangeRequestAccessReadModel;
featuresReadModel: IFeaturesReadModel;
eventService: EventService;
}

export class DependentFeaturesService {
private dependentFeaturesStore: IDependentFeaturesStore;
Expand All @@ -16,17 +25,21 @@ export class DependentFeaturesService {

private changeRequestAccessReadModel: IChangeRequestAccessReadModel;

private featuresReadModel: IFeaturesReadModel;

private eventService: EventService;

constructor(
dependentFeaturesStore: IDependentFeaturesStore,
dependentFeaturesReadModel: IDependentFeaturesReadModel,
changeRequestAccessReadModel: IChangeRequestAccessReadModel,
eventService: EventService,
) {
constructor({
featuresReadModel,
dependentFeaturesReadModel,
dependentFeaturesStore,
eventService,
changeRequestAccessReadModel,
}: IDependentFeaturesServiceDeps) {
this.dependentFeaturesStore = dependentFeaturesStore;
this.dependentFeaturesReadModel = dependentFeaturesReadModel;
this.changeRequestAccessReadModel = changeRequestAccessReadModel;
this.featuresReadModel = featuresReadModel;
this.eventService = eventService;
}

Expand Down Expand Up @@ -77,15 +90,23 @@ export class DependentFeaturesService {
): Promise<void> {
const { enabled, feature: parent, variants } = dependentFeature;

const children = await this.dependentFeaturesReadModel.getChildren([
child,
const [children, parentExists] = await Promise.all([
this.dependentFeaturesReadModel.getChildren([child]),
this.featuresReadModel.featureExists(parent),
]);

if (children.length > 0) {
throw new InvalidOperationError(
'Transitive dependency detected. Cannot add a dependency to the feature that other features depend on.',
);
}

if (!parentExists) {
throw new InvalidOperationError(
`No active feature ${parent} exists`,
);
}

const featureDependency: FeatureDependency =
enabled === false
? {
Expand Down
31 changes: 31 additions & 0 deletions src/lib/features/dependent-features/dependent.features.e2e.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -136,3 +136,34 @@ test('should not allow to add a parent dependency to a feature that already has
403,
);
});

test('should not allow to add non-existent parent dependency', async () => {
const grandparent = uuidv4();
const parent = uuidv4();
const child = uuidv4();
await app.createFeature(child);

await addFeatureDependency(
child,
{
feature: parent,
},
403,
);
});

test('should not allow to add archived parent dependency', async () => {
const parent = uuidv4();
const child = uuidv4();
await app.createFeature(child);
await app.createFeature(parent);
await app.archiveFeature(parent);

await addFeatureDependency(
child,
{
feature: parent,
},
403,
);
});
7 changes: 7 additions & 0 deletions src/lib/features/feature-toggle/fake-features-read-model.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
import { IFeaturesReadModel } from './features-read-model-type';

export class FakeFeaturesReadModel implements IFeaturesReadModel {
featureExists(): Promise<boolean> {
return Promise.resolve(false);
}
}
3 changes: 3 additions & 0 deletions src/lib/features/feature-toggle/features-read-model-type.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
export interface IFeaturesReadModel {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

introduced new read model in feature-toggle submodule so that we don't have to depend on feature-toggle-service with all write model operations.

featureExists(parent: string): Promise<boolean>;
}
19 changes: 19 additions & 0 deletions src/lib/features/feature-toggle/features-read-model.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
import { Db } from '../../db/db';
import { IFeaturesReadModel } from './features-read-model-type';

export class FeaturesReadModel implements IFeaturesReadModel {
private db: Db;

constructor(db: Db) {
this.db = db;
}

async featureExists(parent: string): Promise<boolean> {
const rows = await this.db('features')
.where('name', parent)
.andWhere('archived_at', null)
.select('name');

return rows.length > 0;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I find it more readable than inline select exists

}
}
Loading