From ece5a634bf1f4c7afca817d017da156f6c841195 Mon Sep 17 00:00:00 2001 From: Thomas Heartman Date: Thu, 9 Nov 2023 12:09:39 +0100 Subject: [PATCH] feat: API prevents you from deleting segments in crs (#5308) This PR hooks up the changes introduced in #5301 to the API and puts them behind a feature flag. A new test has been added and the test setup has been slightly tweaked to allow this test. When the flag is enabled, the API will now not let you delete a segment that's used in any active CRs. --- ...e-request-segment-usage-read-model.test.ts | 4 +- ...change-request-segment-usage-read-model.ts | 0 ...reateChangeRequestSegmentUsageReadModel.ts | 4 +- ...change-request-segment-usage-read-model.ts | 0 ...change-request-segment-usage-read-model.ts | 0 .../features/segment/createSegmentService.ts | 12 ++++ .../features/segment/segment-controller.ts | 11 ++- src/lib/segments/segment-service-interface.ts | 2 + src/lib/services/index.ts | 10 +++ src/lib/services/segment-service.ts | 17 +++++ src/test/e2e/api/admin/segment.e2e.test.ts | 67 +++++++++++++++++-- 11 files changed, 115 insertions(+), 12 deletions(-) rename src/lib/features/{change-request-segment-usage-read-model.ts => change-request-segment-usage-service}/change-request-segment-usage-read-model.test.ts (95%) rename src/lib/features/{change-request-segment-usage-read-model.ts => change-request-segment-usage-service}/change-request-segment-usage-read-model.ts (100%) rename src/lib/features/{change-request-segment-usage-read-model.ts => change-request-segment-usage-service}/createChangeRequestSegmentUsageReadModel.ts (82%) rename src/lib/features/{change-request-segment-usage-read-model.ts => change-request-segment-usage-service}/fake-change-request-segment-usage-read-model.ts (100%) rename src/lib/features/{change-request-segment-usage-read-model.ts => change-request-segment-usage-service}/sql-change-request-segment-usage-read-model.ts (100%) diff --git a/src/lib/features/change-request-segment-usage-read-model.ts/change-request-segment-usage-read-model.test.ts b/src/lib/features/change-request-segment-usage-service/change-request-segment-usage-read-model.test.ts similarity index 95% rename from src/lib/features/change-request-segment-usage-read-model.ts/change-request-segment-usage-read-model.test.ts rename to src/lib/features/change-request-segment-usage-service/change-request-segment-usage-read-model.test.ts index cb9a735ead..26ae25c690 100644 --- a/src/lib/features/change-request-segment-usage-read-model.ts/change-request-segment-usage-read-model.test.ts +++ b/src/lib/features/change-request-segment-usage-service/change-request-segment-usage-read-model.test.ts @@ -2,7 +2,7 @@ import { IUser } from 'lib/server-impl'; import dbInit, { ITestDb } from '../../../test/e2e/helpers/database-init'; import getLogger from '../../../test/fixtures/no-logger'; import { IChangeRequestSegmentUsageReadModel } from './change-request-segment-usage-read-model'; -import { createChangeRequestSegmentUsageModel } from './createChangeRequestSegmentUsageReadModel'; +import { createChangeRequestSegmentUsageReadModel } from './createChangeRequestSegmentUsageReadModel'; import { randomId } from '../../../lib/util'; let db: ITestDb; @@ -20,7 +20,7 @@ beforeAll(async () => { username: 'cr-creator', }); - readModel = createChangeRequestSegmentUsageModel(db.rawDatabase); + readModel = createChangeRequestSegmentUsageReadModel(db.rawDatabase); await db.stores.featureToggleStore.create('default', { name: FLAG_NAME, diff --git a/src/lib/features/change-request-segment-usage-read-model.ts/change-request-segment-usage-read-model.ts b/src/lib/features/change-request-segment-usage-service/change-request-segment-usage-read-model.ts similarity index 100% rename from src/lib/features/change-request-segment-usage-read-model.ts/change-request-segment-usage-read-model.ts rename to src/lib/features/change-request-segment-usage-service/change-request-segment-usage-read-model.ts diff --git a/src/lib/features/change-request-segment-usage-read-model.ts/createChangeRequestSegmentUsageReadModel.ts b/src/lib/features/change-request-segment-usage-service/createChangeRequestSegmentUsageReadModel.ts similarity index 82% rename from src/lib/features/change-request-segment-usage-read-model.ts/createChangeRequestSegmentUsageReadModel.ts rename to src/lib/features/change-request-segment-usage-service/createChangeRequestSegmentUsageReadModel.ts index 6865a20ae1..522013fdec 100644 --- a/src/lib/features/change-request-segment-usage-read-model.ts/createChangeRequestSegmentUsageReadModel.ts +++ b/src/lib/features/change-request-segment-usage-service/createChangeRequestSegmentUsageReadModel.ts @@ -3,13 +3,13 @@ import { ChangeRequestSegmentUsageReadModel } from './sql-change-request-segment import { FakeChangeRequestSegmentUsageReadModel } from './fake-change-request-segment-usage-read-model'; import { IChangeRequestSegmentUsageReadModel } from './change-request-segment-usage-read-model'; -export const createChangeRequestSegmentUsageModel = ( +export const createChangeRequestSegmentUsageReadModel = ( db: Db, ): IChangeRequestSegmentUsageReadModel => { return new ChangeRequestSegmentUsageReadModel(db); }; -export const createFakeChangeRequestAccessService = +export const createFakeChangeRequestSegmentUsageReadModel = (): IChangeRequestSegmentUsageReadModel => { return new FakeChangeRequestSegmentUsageReadModel(); }; diff --git a/src/lib/features/change-request-segment-usage-read-model.ts/fake-change-request-segment-usage-read-model.ts b/src/lib/features/change-request-segment-usage-service/fake-change-request-segment-usage-read-model.ts similarity index 100% rename from src/lib/features/change-request-segment-usage-read-model.ts/fake-change-request-segment-usage-read-model.ts rename to src/lib/features/change-request-segment-usage-service/fake-change-request-segment-usage-read-model.ts diff --git a/src/lib/features/change-request-segment-usage-read-model.ts/sql-change-request-segment-usage-read-model.ts b/src/lib/features/change-request-segment-usage-service/sql-change-request-segment-usage-read-model.ts similarity index 100% rename from src/lib/features/change-request-segment-usage-read-model.ts/sql-change-request-segment-usage-read-model.ts rename to src/lib/features/change-request-segment-usage-service/sql-change-request-segment-usage-read-model.ts diff --git a/src/lib/features/segment/createSegmentService.ts b/src/lib/features/segment/createSegmentService.ts index 7534d9f5bf..b1224e5b4a 100644 --- a/src/lib/features/segment/createSegmentService.ts +++ b/src/lib/features/segment/createSegmentService.ts @@ -11,6 +11,10 @@ import { createChangeRequestAccessReadModel, createFakeChangeRequestAccessService, } from '../change-request-access-service/createChangeRequestAccessReadModel'; +import { + createChangeRequestSegmentUsageReadModel, + createFakeChangeRequestSegmentUsageReadModel, +} from '../change-request-segment-usage-service/createChangeRequestSegmentUsageReadModel'; import { createFakePrivateProjectChecker, createPrivateProjectChecker, @@ -40,6 +44,10 @@ export const createSegmentService = ( db, config, ); + + const changeRequestSegmentUsageReadModel = + createChangeRequestSegmentUsageReadModel(db); + const privateProjectChecker = createPrivateProjectChecker(db, config); const eventService = new EventService( @@ -53,6 +61,7 @@ export const createSegmentService = ( return new SegmentService( { segmentStore, featureStrategiesStore }, changeRequestAccessReadModel, + changeRequestSegmentUsageReadModel, config, eventService, privateProjectChecker, @@ -66,6 +75,8 @@ export const createFakeSegmentService = ( const segmentStore = new FakeSegmentStore(); const featureStrategiesStore = new FakeFeatureStrategiesStore(); const changeRequestAccessReadModel = createFakeChangeRequestAccessService(); + const changeRequestSegmentUsageReadModel = + createFakeChangeRequestSegmentUsageReadModel(); const privateProjectChecker = createFakePrivateProjectChecker(); @@ -80,6 +91,7 @@ export const createFakeSegmentService = ( return new SegmentService( { segmentStore, featureStrategiesStore }, changeRequestAccessReadModel, + changeRequestSegmentUsageReadModel, config, eventService, privateProjectChecker, diff --git a/src/lib/features/segment/segment-controller.ts b/src/lib/features/segment/segment-controller.ts index c79728ca10..a54caa58ff 100644 --- a/src/lib/features/segment/segment-controller.ts +++ b/src/lib/features/segment/segment-controller.ts @@ -373,9 +373,16 @@ export class SegmentsController extends Controller { res: Response, ): Promise { const id = Number(req.params.id); - const strategies = await this.segmentService.getAllStrategies(id); - if (strategies.length > 0) { + let segmentIsInUse = false; + if (this.flagResolver.isEnabled('detectSegmentUsageInChangeRequests')) { + segmentIsInUse = await this.segmentService.isInUse(id); + } else { + const strategies = await this.segmentService.getAllStrategies(id); + segmentIsInUse = strategies.length > 0; + } + + if (segmentIsInUse) { res.status(409).send(); } else { await this.segmentService.delete(id, req.user); diff --git a/src/lib/segments/segment-service-interface.ts b/src/lib/segments/segment-service-interface.ts index a64fd865b1..8740ecbca2 100644 --- a/src/lib/segments/segment-service-interface.ts +++ b/src/lib/segments/segment-service-interface.ts @@ -60,4 +60,6 @@ export interface ISegmentService { sourceStrategyId: string, targetStrategyId: string, ): Promise; + + isInUse(id: number): Promise; } diff --git a/src/lib/services/index.ts b/src/lib/services/index.ts index c93b132707..36c2cbb752 100644 --- a/src/lib/services/index.ts +++ b/src/lib/services/index.ts @@ -63,6 +63,10 @@ import { createChangeRequestAccessReadModel, createFakeChangeRequestAccessService, } from '../features/change-request-access-service/createChangeRequestAccessReadModel'; +import { + createChangeRequestSegmentUsageReadModel, + createFakeChangeRequestSegmentUsageReadModel, +} from '../features/change-request-segment-usage-service/createChangeRequestSegmentUsageReadModel'; import ConfigurationRevisionService from '../features/feature-toggle/configuration-revision-service'; import { createFakeProjectService, @@ -322,9 +326,15 @@ export const createServices = ( const changeRequestAccessReadModel = db ? createChangeRequestAccessReadModel(db, config) : createFakeChangeRequestAccessService(); + + const changeRequestSegmentUsageReadModel = db + ? createChangeRequestSegmentUsageReadModel(db) + : createFakeChangeRequestSegmentUsageReadModel(); + const segmentService = new SegmentService( stores, changeRequestAccessReadModel, + changeRequestSegmentUsageReadModel, config, eventService, privateProjectChecker, diff --git a/src/lib/services/segment-service.ts b/src/lib/services/segment-service.ts index b5105a7520..cf1ab7224a 100644 --- a/src/lib/services/segment-service.ts +++ b/src/lib/services/segment-service.ts @@ -23,6 +23,7 @@ import { PermissionError } from '../error'; import { IChangeRequestAccessReadModel } from '../features/change-request-access-service/change-request-access-read-model'; import { IPrivateProjectChecker } from '../features/private-project/privateProjectCheckerType'; import EventService from './event-service'; +import { IChangeRequestSegmentUsageReadModel } from 'lib/features/change-request-segment-usage-service/change-request-segment-usage-read-model'; export class SegmentService implements ISegmentService { private logger: Logger; @@ -33,6 +34,8 @@ export class SegmentService implements ISegmentService { private changeRequestAccessReadModel: IChangeRequestAccessReadModel; + private changeRequestSegmentUsageReadModel: IChangeRequestSegmentUsageReadModel; + private config: IUnleashConfig; private flagResolver: IFlagResolver; @@ -47,6 +50,7 @@ export class SegmentService implements ISegmentService { featureStrategiesStore, }: Pick, changeRequestAccessReadModel: IChangeRequestAccessReadModel, + changeRequestSegmentUsageReadModel: IChangeRequestSegmentUsageReadModel, config: IUnleashConfig, eventService: EventService, privateProjectChecker: IPrivateProjectChecker, @@ -55,6 +59,8 @@ export class SegmentService implements ISegmentService { this.featureStrategiesStore = featureStrategiesStore; this.eventService = eventService; this.changeRequestAccessReadModel = changeRequestAccessReadModel; + this.changeRequestSegmentUsageReadModel = + changeRequestSegmentUsageReadModel; this.privateProjectChecker = privateProjectChecker; this.logger = config.getLogger('services/segment-service.ts'); this.flagResolver = config.flagResolver; @@ -108,6 +114,17 @@ export class SegmentService implements ISegmentService { return strategies; } + async isInUse(id: number): Promise { + const strategies = await this.getAllStrategies(id); + if (strategies.length > 0) { + return true; + } + + return await this.changeRequestSegmentUsageReadModel.isSegmentUsedInActiveChangeRequests( + id, + ); + } + async create( data: unknown, user: Partial>, diff --git a/src/test/e2e/api/admin/segment.e2e.test.ts b/src/test/e2e/api/admin/segment.e2e.test.ts index a27b6ec5fa..90dccbfa0a 100644 --- a/src/test/e2e/api/admin/segment.e2e.test.ts +++ b/src/test/e2e/api/admin/segment.e2e.test.ts @@ -108,14 +108,18 @@ const validateSegment = ( beforeAll(async () => { db = await dbInit('segments_api_serial', getLogger); - app = await setupAppWithCustomConfig(db.stores, { - experimental: { - flags: { - strictSchemaValidation: true, - anonymiseEventLog: true, + app = await setupAppWithCustomConfig( + db.stores, + { + experimental: { + flags: { + anonymiseEventLog: true, + detectSegmentUsageInChangeRequests: true, + }, }, }, - }); + db.rawDatabase, + ); }); afterAll(async () => { @@ -230,6 +234,57 @@ test('should not delete segments used by strategies', async () => { expect((await fetchSegments()).length).toEqual(1); }); +test('should not delete segments used by strategies in CRs', async () => { + await createSegment({ name: 'a', constraints: [] }); + const toggle = mockFeatureToggle(); + await createFeatureToggle(app, toggle); + const [segment] = await fetchSegments(); + + const CR_ID = 54321; + + const user = await db.stores.userStore.insert({ + username: 'test', + }); + + await db.rawDatabase.table('change_requests').insert({ + id: CR_ID, + environment: 'default', + state: 'In Review', + project: 'default', + created_by: user.id, + created_at: '2023-01-01 00:00:00', + min_approvals: 1, + title: 'My change request', + }); + + await db.rawDatabase.table('change_request_events').insert({ + feature: toggle.name, + action: 'addStrategy', + payload: { + name: 'flexibleRollout', + title: '', + disabled: false, + segments: [segment.id], + variants: [], + parameters: { + groupId: toggle.name, + rollout: '100', + stickiness: 'default', + }, + constraints: [], + }, + created_at: '2023-01-01 00:01:00', + change_request_id: CR_ID, + created_by: user.id, + }); + + expect((await fetchSegments()).length).toEqual(1); + + await app.request.delete(`${SEGMENTS_BASE_PATH}/${segment.id}`).expect(409); + + expect((await fetchSegments()).length).toEqual(1); +}); + test('should list strategies by segment', async () => { await createSegment({ name: 'S1', constraints: [] }); await createSegment({ name: 'S2', constraints: [] });