From 6adcf103f05344f3a8b7fcecd0873913ef229b60 Mon Sep 17 00:00:00 2001 From: olav Date: Wed, 8 Jun 2022 15:41:02 +0200 Subject: [PATCH] fix: clone segments when cloning a toggle (#1678) * refactor: merge segment test files * fix: clone segments when cloning a toggle --- src/lib/services/feature-toggle-service.ts | 34 ++-- src/lib/services/index.ts | 8 +- src/lib/services/segment-service.ts | 18 ++ .../e2e/api/client/global.segment.e2e.test.ts | 145 --------------- src/test/e2e/api/client/segment.e2e.test.ts | 172 ++++++++++++------ .../e2e/services/access-service.e2e.test.ts | 7 +- .../services/api-token-service.e2e.test.ts | 7 +- .../feature-toggle-service-v2.e2e.test.ts | 7 +- .../project-health-service.e2e.test.ts | 7 +- .../e2e/services/project-service.e2e.test.ts | 7 +- 10 files changed, 190 insertions(+), 222 deletions(-) delete mode 100644 src/test/e2e/api/client/global.segment.e2e.test.ts diff --git a/src/lib/services/feature-toggle-service.ts b/src/lib/services/feature-toggle-service.ts index eb0f023646..14cb64e3c7 100644 --- a/src/lib/services/feature-toggle-service.ts +++ b/src/lib/services/feature-toggle-service.ts @@ -41,7 +41,6 @@ import { FeatureToggleLegacy, FeatureToggleWithEnvironment, IConstraint, - IEnvironmentDetail, IFeatureEnvironmentInfo, IFeatureOverview, IFeatureStrategy, @@ -70,6 +69,7 @@ import { } from '../util/validators/constraint-types'; import { IContextFieldStore } from 'lib/types/stores/context-field-store'; import { Saved, Unsaved } from '../types/saved'; +import { SegmentService } from './segment-service'; interface IFeatureContext { featureName: string; @@ -103,6 +103,8 @@ class FeatureToggleService { private contextFieldStore: IContextFieldStore; + private segmentService: SegmentService; + constructor( { featureStrategiesStore, @@ -125,6 +127,7 @@ class FeatureToggleService { | 'contextFieldStore' >, { getLogger }: Pick, + segmentService: SegmentService, ) { this.logger = getLogger('services/feature-toggle-service.ts'); this.featureStrategiesStore = featureStrategiesStore; @@ -135,6 +138,7 @@ class FeatureToggleService { this.eventStore = eventStore; this.featureEnvironmentStore = featureEnvironmentStore; this.contextFieldStore = contextFieldStore; + this.segmentService = segmentService; } async validateFeatureContext({ @@ -618,37 +622,29 @@ class FeatureToggleService { ); const newToggle = { ...cToggle, name: newFeatureName }; - - // Create feature toggle const created = await this.createFeatureToggle( projectId, newToggle, userName, ); - const createStrategies = []; - newToggle.environments.forEach((e: IEnvironmentDetail) => - e.strategies.forEach((s: IStrategyConfig) => { + const tasks = newToggle.environments.flatMap((e) => + e.strategies.map((s) => { if (replaceGroupId && s.parameters.hasOwnProperty('groupId')) { s.parameters.groupId = newFeatureName; } - delete s.id; - createStrategies.push( - this.createStrategy( - s, - { - projectId, - featureName: newFeatureName, - environment: e.name, - }, - userName, - ), + const context = { + projectId, + featureName: newFeatureName, + environment: e.name, + }; + return this.createStrategy(s, context, userName).then((s2) => + this.segmentService.cloneStrategySegments(s.id, s2.id), ); }), ); - // Create strategies - await Promise.allSettled(createStrategies); + await Promise.allSettled(tasks); return created; } diff --git a/src/lib/services/index.ts b/src/lib/services/index.ts index e04389c17c..8c8491b18f 100644 --- a/src/lib/services/index.ts +++ b/src/lib/services/index.ts @@ -62,7 +62,12 @@ export const createServices = ( const versionService = new VersionService(stores, config); const healthService = new HealthService(stores, config); const userFeedbackService = new UserFeedbackService(stores, config); - const featureToggleServiceV2 = new FeatureToggleService(stores, config); + const segmentService = new SegmentService(stores, config); + const featureToggleServiceV2 = new FeatureToggleService( + stores, + config, + segmentService, + ); const environmentService = new EnvironmentService(stores, config); const featureTagService = new FeatureTagService(stores, config); const projectHealthService = new ProjectHealthService( @@ -77,7 +82,6 @@ export const createServices = ( featureToggleServiceV2, ); const userSplashService = new UserSplashService(stores, config); - const segmentService = new SegmentService(stores, config); const openApiService = new OpenApiService(config); const clientSpecService = new ClientSpecService(config); diff --git a/src/lib/services/segment-service.ts b/src/lib/services/segment-service.ts index 7dbf21004b..68e848b663 100644 --- a/src/lib/services/segment-service.ts +++ b/src/lib/services/segment-service.ts @@ -109,6 +109,24 @@ export class SegmentService { }); } + async cloneStrategySegments( + sourceStrategyId: string, + targetStrategyId: string, + ): Promise { + const sourceStrategySegments = await this.getByStrategy( + sourceStrategyId, + ); + + await Promise.all( + sourceStrategySegments.map((sourceStrategySegment) => { + return this.addToStrategy( + sourceStrategySegment.id, + targetStrategyId, + ); + }), + ); + } + // Used by unleash-enterprise. async addToStrategy(id: number, strategyId: string): Promise { await this.validateStrategySegmentLimit(strategyId); diff --git a/src/test/e2e/api/client/global.segment.e2e.test.ts b/src/test/e2e/api/client/global.segment.e2e.test.ts deleted file mode 100644 index a1d024487c..0000000000 --- a/src/test/e2e/api/client/global.segment.e2e.test.ts +++ /dev/null @@ -1,145 +0,0 @@ -import dbInit, { ITestDb } from '../../helpers/database-init'; -import getLogger from '../../../fixtures/no-logger'; -import { - IUnleashTest, - setupAppWithCustomConfig, -} from '../../helpers/test-helper'; -import { - IConstraint, - IFeatureToggleClient, - ISegment, -} from '../../../../lib/types/model'; -import { randomId } from '../../../../lib/util/random-id'; -import User from '../../../../lib/types/user'; - -let db: ITestDb; -let app: IUnleashTest; - -const FEATURES_ADMIN_BASE_PATH = '/api/admin/features'; -const FEATURES_CLIENT_BASE_PATH = '/api/client/features'; - -interface ApiResponse { - features: IFeatureToggleClient[]; - version: number; - segments: ISegment[]; -} - -const fetchSegments = (): Promise => { - return app.services.segmentService.getAll(); -}; - -const fetchFeatures = (): Promise => { - return app.request - .get(FEATURES_ADMIN_BASE_PATH) - .expect(200) - .then((res) => res.body.features); -}; - -const fetchClientResponse = (): Promise => { - return app.request - .get(FEATURES_CLIENT_BASE_PATH) - .set('Unleash-Client-Spec', '4.2.0') - .expect(200) - .then((res) => res.body); -}; - -const createSegment = (postData: object): Promise => { - const user = { email: 'test@example.com' } as User; - return app.services.segmentService.create(postData, user); -}; - -const createFeatureToggle = ( - postData: object, - expectStatusCode = 201, -): Promise => { - return app.request - .post(FEATURES_ADMIN_BASE_PATH) - .send(postData) - .expect(expectStatusCode); -}; - -const addSegmentToStrategy = ( - segmentId: number, - strategyId: string, -): Promise => { - return app.services.segmentService.addToStrategy(segmentId, strategyId); -}; - -const mockFeatureToggle = (): object => { - return { - name: randomId(), - strategies: [{ name: randomId(), constraints: [], parameters: {} }], - }; -}; - -const mockConstraints = (): IConstraint[] => { - return Array.from({ length: 5 }).map(() => ({ - values: ['x', 'y', 'z'], - operator: 'IN', - contextName: 'a', - })); -}; - -const createTestSegments = async (): Promise => { - const constraints = mockConstraints(); - await createSegment({ name: 'S1', constraints }); - await createSegment({ name: 'S2', constraints }); - await createSegment({ name: 'S3', constraints }); - await createFeatureToggle(mockFeatureToggle()); - await createFeatureToggle(mockFeatureToggle()); - await createFeatureToggle(mockFeatureToggle()); - const [feature1, feature2] = await fetchFeatures(); - const [segment1, segment2] = await fetchSegments(); - - await addSegmentToStrategy(segment1.id, feature1.strategies[0].id); - await addSegmentToStrategy(segment2.id, feature1.strategies[0].id); - await addSegmentToStrategy(segment2.id, feature2.strategies[0].id); -}; - -beforeAll(async () => { - const config = { inlineSegmentConstraints: false }; - db = await dbInit('global_segments', getLogger, config); - app = await setupAppWithCustomConfig(db.stores, config); -}); - -afterAll(async () => { - await app.destroy(); - await db.destroy(); -}); - -afterEach(async () => { - await db.stores.segmentStore.deleteAll(); - await db.stores.featureToggleStore.deleteAll(); -}); - -test('should return segments in base of toggle response if inline is disabled', async () => { - await createTestSegments(); - - const clientFeatures = await fetchClientResponse(); - expect(clientFeatures.segments.length).toBeDefined(); -}); - -test('should only send segments that are in use', async () => { - await createTestSegments(); - - const clientFeatures = await fetchClientResponse(); - //3 segments were created in createTestSegments, only 2 are in use - expect(clientFeatures.segments.length).toEqual(2); -}); - -test('should send all segments that are in use by feature', async () => { - await createTestSegments(); - - const clientFeatures = await fetchClientResponse(); - const globalSegments = clientFeatures.segments; - expect(globalSegments).toHaveLength(2); - - const globalSegmentIds = globalSegments.map((segment) => segment.id); - const allSegmentIds = clientFeatures.features - .map((feat) => feat.strategies.map((strategy) => strategy.segments)) - .flat() - .flat() - .filter((x) => !!x); - const toggleSegmentIds = [...new Set(allSegmentIds)]; - expect(globalSegmentIds).toEqual(toggleSegmentIds); -}); diff --git a/src/test/e2e/api/client/segment.e2e.test.ts b/src/test/e2e/api/client/segment.e2e.test.ts index 5eaaed7ed6..578a641575 100644 --- a/src/test/e2e/api/client/segment.e2e.test.ts +++ b/src/test/e2e/api/client/segment.e2e.test.ts @@ -81,6 +81,36 @@ const mockConstraintValues = (length: number): string[] => { }); }; +const fetchClientResponse = (): Promise<{ + features: IFeatureToggleClient[]; + version: number; + segments: ISegment[]; +}> => { + return app.request + .get(FEATURES_CLIENT_BASE_PATH) + .set('Unleash-Client-Spec', '4.2.0') + .expect(200) + .then((res) => res.body); +}; + +const createTestSegments = async () => { + const constraints = mockConstraints(); + + await createSegment({ name: 'S1', constraints }); + await createSegment({ name: 'S2', constraints }); + await createSegment({ name: 'S3', constraints }); + + await createFeatureToggle(mockFeatureToggle()); + await createFeatureToggle(mockFeatureToggle()); + await createFeatureToggle(mockFeatureToggle()); + + const [feature1, feature2] = await fetchFeatures(); + const [segment1, segment2] = await fetchSegments(); + await addSegmentToStrategy(segment1.id, feature1.strategies[0].id); + await addSegmentToStrategy(segment2.id, feature1.strategies[0].id); + await addSegmentToStrategy(segment2.id, feature2.strategies[0].id); +}; + beforeAll(async () => { db = await dbInit('segments', getLogger); app = await setupApp(db.stores); @@ -97,37 +127,6 @@ afterEach(async () => { await db.stores.eventStore.deleteAll(); }); -test('should inline segment constraints into features by default', async () => { - const constraints = mockConstraints(); - await createSegment({ name: 'S1', constraints }); - await createSegment({ name: 'S2', constraints }); - await createSegment({ name: 'S3', constraints }); - await createFeatureToggle(mockFeatureToggle()); - await createFeatureToggle(mockFeatureToggle()); - await createFeatureToggle(mockFeatureToggle()); - const [feature1, feature2, feature3] = await fetchFeatures(); - const [segment1, segment2, segment3] = await fetchSegments(); - - await addSegmentToStrategy(segment1.id, feature1.strategies[0].id); - await addSegmentToStrategy(segment2.id, feature1.strategies[0].id); - await addSegmentToStrategy(segment2.id, feature2.strategies[0].id); - await addSegmentToStrategy(segment3.id, feature1.strategies[0].id); - await addSegmentToStrategy(segment3.id, feature2.strategies[0].id); - await addSegmentToStrategy(segment3.id, feature3.strategies[0].id); - - const clientFeatures = await fetchClientFeatures(); - const clientStrategies = clientFeatures.flatMap((f) => f.strategies); - const clientConstraints = clientStrategies.flatMap((s) => s.constraints); - const clientValues = clientConstraints.flatMap((c) => c.values); - const uniqueValues = [...new Set(clientValues)]; - - expect(clientFeatures.length).toEqual(3); - expect(clientStrategies.length).toEqual(3); - expect(clientConstraints.length).toEqual(5 * 6); - expect(clientValues.length).toEqual(5 * 6 * 3); - expect(uniqueValues.length).toEqual(3); -}); - test('should validate segment constraint values limit', async () => { const constraints: IConstraint[] = [ { @@ -189,22 +188,77 @@ test('should validate feature strategy segment limit', async () => { ); }); -test('should only return segments to clients with the segments capability', async () => { +test('should clone feature strategy segments', async () => { const constraints = mockConstraints(); await createSegment({ name: 'S1', constraints }); - await createSegment({ name: 'S2', constraints }); - await createSegment({ name: 'S3', constraints }); - await createFeatureToggle(mockFeatureToggle()); await createFeatureToggle(mockFeatureToggle()); await createFeatureToggle(mockFeatureToggle()); + const [feature1, feature2] = await fetchFeatures(); + const strategy1 = feature1.strategies[0].id; + const strategy2 = feature2.strategies[0].id; + const [segment1] = await fetchSegments(); + await addSegmentToStrategy(segment1.id, feature1.strategies[0].id); + + let segments1 = await app.services.segmentService.getByStrategy(strategy1); + let segments2 = await app.services.segmentService.getByStrategy(strategy2); + expect(collectIds(segments1)).toEqual([segment1.id]); + expect(collectIds(segments2)).toEqual([]); + + await app.services.segmentService.cloneStrategySegments( + strategy1, + strategy2, + ); + + segments1 = await app.services.segmentService.getByStrategy(strategy1); + segments2 = await app.services.segmentService.getByStrategy(strategy2); + expect(collectIds(segments1)).toEqual([segment1.id]); + expect(collectIds(segments2)).toEqual([segment1.id]); +}); + +test('should store segment-created and segment-deleted events', async () => { + const constraints = mockConstraints(); + const user = new User({ id: 1, email: 'test@example.com' }); + + await createSegment({ name: 'S1', constraints }); + const [segment1] = await fetchSegments(); + await app.services.segmentService.delete(segment1.id, user); + const events = await db.stores.eventStore.getEvents(); + + expect(events[0].type).toEqual('segment-deleted'); + expect(events[0].data.id).toEqual(segment1.id); + expect(events[1].type).toEqual('segment-created'); + expect(events[1].data.id).toEqual(segment1.id); +}); + +test('should inline segment constraints into features by default', async () => { + await createTestSegments(); + + const [feature1, feature2, feature3] = await fetchFeatures(); + const [, , segment3] = await fetchSegments(); + await addSegmentToStrategy(segment3.id, feature1.strategies[0].id); + await addSegmentToStrategy(segment3.id, feature2.strategies[0].id); + await addSegmentToStrategy(segment3.id, feature3.strategies[0].id); + + const clientFeatures = await fetchClientFeatures(); + const clientStrategies = clientFeatures.flatMap((f) => f.strategies); + const clientConstraints = clientStrategies.flatMap((s) => s.constraints); + const clientValues = clientConstraints.flatMap((c) => c.values); + const uniqueValues = [...new Set(clientValues)]; + + expect(clientFeatures.length).toEqual(3); + expect(clientStrategies.length).toEqual(3); + expect(clientConstraints.length).toEqual(5 * 6); + expect(clientValues.length).toEqual(5 * 6 * 3); + expect(uniqueValues.length).toEqual(3); +}); + +test('should only return segments to clients that support the spec', async () => { + await createTestSegments(); + const [segment1, segment2] = await fetchSegments(); const segmentIds = collectIds([segment1, segment2]); - await addSegmentToStrategy(segment1.id, feature1.strategies[0].id); - await addSegmentToStrategy(segment2.id, feature1.strategies[0].id); - await addSegmentToStrategy(segment2.id, feature2.strategies[0].id); - const unknownClientResponse = await app.request .get(FEATURES_CLIENT_BASE_PATH) .expect(200) @@ -227,17 +281,33 @@ test('should only return segments to clients with the segments capability', asyn expect(supportedClientConstraints.length).toEqual(0); }); -test('should store segment-created and segment-deleted events', async () => { - const constraints = mockConstraints(); - const user = new User({ id: 1, email: 'test@example.com' }); +test('should return segments in base of toggle response if inline is disabled', async () => { + await createTestSegments(); - await createSegment({ name: 'S1', constraints }); - const [segment1] = await fetchSegments(); - await app.services.segmentService.delete(segment1.id, user); - const events = await db.stores.eventStore.getEvents(); - - expect(events[0].type).toEqual('segment-deleted'); - expect(events[0].data.id).toEqual(segment1.id); - expect(events[1].type).toEqual('segment-created'); - expect(events[1].data.id).toEqual(segment1.id); + const clientFeatures = await fetchClientResponse(); + expect(clientFeatures.segments.length).toBeDefined(); +}); + +test('should only send segments that are in use', async () => { + await createTestSegments(); + + const clientFeatures = await fetchClientResponse(); + expect(clientFeatures.segments.length).toEqual(2); +}); + +test('should send all segments that are in use by feature', async () => { + await createTestSegments(); + + const clientFeatures = await fetchClientResponse(); + const globalSegments = clientFeatures.segments; + expect(globalSegments).toHaveLength(2); + + const globalSegmentIds = globalSegments.map((segment) => segment.id); + const allSegmentIds = clientFeatures.features + .map((feat) => feat.strategies.map((strategy) => strategy.segments)) + .flat() + .flat() + .filter((x) => !!x); + const toggleSegmentIds = [...new Set(allSegmentIds)]; + expect(globalSegmentIds).toEqual(toggleSegmentIds); }); diff --git a/src/test/e2e/services/access-service.e2e.test.ts b/src/test/e2e/services/access-service.e2e.test.ts index c5227a646f..3fd28e9852 100644 --- a/src/test/e2e/services/access-service.e2e.test.ts +++ b/src/test/e2e/services/access-service.e2e.test.ts @@ -14,6 +14,7 @@ import FeatureToggleService from '../../../lib/services/feature-toggle-service'; import ProjectService from '../../../lib/services/project-service'; import { createTestConfig } from '../../config/test-config'; import { DEFAULT_PROJECT } from '../../../lib/types/project'; +import { SegmentService } from '../../../lib/services/segment-service'; let db: ITestDb; let stores: IUnleashStores; @@ -212,7 +213,11 @@ beforeAll(async () => { editorRole = roles.find((r) => r.name === RoleName.EDITOR); adminRole = roles.find((r) => r.name === RoleName.ADMIN); readRole = roles.find((r) => r.name === RoleName.VIEWER); - featureToggleService = new FeatureToggleService(stores, config); + featureToggleService = new FeatureToggleService( + stores, + config, + new SegmentService(stores, config), + ); projectService = new ProjectService( stores, config, diff --git a/src/test/e2e/services/api-token-service.e2e.test.ts b/src/test/e2e/services/api-token-service.e2e.test.ts index 44aa4b947f..db5d3c59b8 100644 --- a/src/test/e2e/services/api-token-service.e2e.test.ts +++ b/src/test/e2e/services/api-token-service.e2e.test.ts @@ -8,6 +8,7 @@ import { addDays, subDays } from 'date-fns'; import ProjectService from '../../../lib/services/project-service'; import FeatureToggleService from '../../../lib/services/feature-toggle-service'; import { AccessService } from '../../../lib/services/access-service'; +import { SegmentService } from '../../../lib/services/segment-service'; let db; let stores; @@ -21,7 +22,11 @@ beforeAll(async () => { db = await dbInit('api_token_service_serial', getLogger); stores = db.stores; const accessService = new AccessService(stores, config); - const featureToggleService = new FeatureToggleService(stores, config); + const featureToggleService = new FeatureToggleService( + stores, + config, + new SegmentService(stores, config), + ); const project = { id: 'test-project', name: 'Test Project', diff --git a/src/test/e2e/services/feature-toggle-service-v2.e2e.test.ts b/src/test/e2e/services/feature-toggle-service-v2.e2e.test.ts index 30bf3fdfcb..9d384c962c 100644 --- a/src/test/e2e/services/feature-toggle-service-v2.e2e.test.ts +++ b/src/test/e2e/services/feature-toggle-service-v2.e2e.test.ts @@ -3,6 +3,7 @@ import { createTestConfig } from '../../config/test-config'; import dbInit from '../helpers/database-init'; import { DEFAULT_ENV } from '../../../lib/util/constants'; import { StrategySchema } from '../../../lib/openapi/spec/strategy-schema'; +import { SegmentService } from '../../../lib/services/segment-service'; let stores; let db; @@ -15,7 +16,11 @@ beforeAll(async () => { config.getLogger, ); stores = db.stores; - service = new FeatureToggleService(stores, config); + service = new FeatureToggleService( + stores, + config, + new SegmentService(stores, config), + ); }); afterAll(async () => { diff --git a/src/test/e2e/services/project-health-service.e2e.test.ts b/src/test/e2e/services/project-health-service.e2e.test.ts index 19de4de859..1600f4141b 100644 --- a/src/test/e2e/services/project-health-service.e2e.test.ts +++ b/src/test/e2e/services/project-health-service.e2e.test.ts @@ -7,6 +7,7 @@ import ProjectHealthService from '../../../lib/services/project-health-service'; import { createTestConfig } from '../../config/test-config'; import { IUnleashStores } from '../../../lib/types'; import { IUser } from '../../../lib/server-impl'; +import { SegmentService } from '../../../lib/services/segment-service'; let stores: IUnleashStores; let db: ITestDb; @@ -25,7 +26,11 @@ beforeAll(async () => { email: 'test@getunleash.io', }); accessService = new AccessService(stores, config); - featureToggleService = new FeatureToggleService(stores, config); + featureToggleService = new FeatureToggleService( + stores, + config, + new SegmentService(stores, config), + ); projectService = new ProjectService( stores, config, diff --git a/src/test/e2e/services/project-service.e2e.test.ts b/src/test/e2e/services/project-service.e2e.test.ts index 0ca22048a3..f4a819a9ad 100644 --- a/src/test/e2e/services/project-service.e2e.test.ts +++ b/src/test/e2e/services/project-service.e2e.test.ts @@ -9,6 +9,7 @@ import { RoleName } from '../../../lib/types/model'; import { randomId } from '../../../lib/util/random-id'; import EnvironmentService from '../../../lib/services/environment-service'; import IncompatibleProjectError from '../../../lib/error/incompatible-project-error'; +import { SegmentService } from '../../../lib/services/segment-service'; let stores; let db: ITestDb; @@ -32,7 +33,11 @@ beforeAll(async () => { experimental: { environments: { enabled: true } }, }); accessService = new AccessService(stores, config); - featureToggleService = new FeatureToggleService(stores, config); + featureToggleService = new FeatureToggleService( + stores, + config, + new SegmentService(stores, config), + ); environmentService = new EnvironmentService(stores, config); projectService = new ProjectService( stores,