diff --git a/src/lib/features/export-import-toggles/export-import.e2e.test.ts b/src/lib/features/export-import-toggles/export-import.e2e.test.ts index 324aece5d0..ece621a082 100644 --- a/src/lib/features/export-import-toggles/export-import.e2e.test.ts +++ b/src/lib/features/export-import-toggles/export-import.e2e.test.ts @@ -100,23 +100,25 @@ const createVariants = async (feature: string, variants: IVariant[]) => { ); }; -const createProject = async () => { +const createProjects = async (projects: string[] = [DEFAULT_PROJECT]) => { await db.stores.environmentStore.create({ name: DEFAULT_ENV, type: 'production', }); - await db.stores.projectStore.create({ - name: DEFAULT_PROJECT, - description: '', - id: DEFAULT_PROJECT, - mode: 'open' as const, - }); - await app.request - .post(`/api/admin/projects/${DEFAULT_PROJECT}/environments`) - .send({ - environment: DEFAULT_ENV, - }) - .expect(200); + for (const project of projects) { + await db.stores.projectStore.create({ + name: project, + description: '', + id: project, + mode: 'open' as const, + }); + await app.request + .post(`/api/admin/projects/${project}/environments`) + .send({ + environment: DEFAULT_ENV, + }) + .expect(200); + } }; const createSegment = (postData: UpsertSegmentSchema): Promise => { @@ -193,9 +195,74 @@ afterAll(async () => { await db.destroy(); }); +describe('import-export for project-specific segments', () => { + test('exports features with project-specific-segments', async () => { + const segmentName = 'my-segment'; + const project = 'with-segments'; + await createProjects([project]); + const segment = await createSegment({ + name: segmentName, + project, + constraints: [], + }); + const strategy = { + name: 'default', + parameters: { rollout: '100', stickiness: 'default' }, + constraints: [ + { + contextName: 'appName', + values: ['test'], + operator: 'IN' as const, + }, + ], + segments: [segment.id], + }; + await createToggle( + { + name: 'first_feature', + description: 'the #1 feature', + }, + strategy, + [], + project, + ); + const { body } = await app.request + .post('/api/admin/features-batch/export') + .send({ + features: ['first_feature'], + environment: 'default', + }) + .set('Content-Type', 'application/json') + .expect(200); + + const { name, ...resultStrategy } = strategy; + expect(body).toMatchObject({ + features: [ + { + name: 'first_feature', + }, + ], + featureStrategies: [resultStrategy], + featureEnvironments: [ + { + enabled: false, + environment: 'default', + featureName: 'first_feature', + }, + ], + segments: [ + { + id: segment.id, + name: segmentName, + }, + ], + }); + }); +}); + test('exports features', async () => { const segmentName = 'my-segment'; - await createProject(); + await createProjects(); const segment = await createSegment({ name: segmentName, constraints: [] }); const strategy = { name: 'default', @@ -249,6 +316,7 @@ test('exports features', async () => { ], segments: [ { + id: segment.id, name: segmentName, }, ], @@ -256,7 +324,7 @@ test('exports features', async () => { }); test('should export custom context fields from strategies and variants', async () => { - await createProject(); + await createProjects(); const strategyContext = { name: 'strategy-context', legalValues: [ @@ -358,7 +426,7 @@ test('should export custom context fields from strategies and variants', async ( test('should export tags', async () => { const featureName = 'first_feature'; - await createProject(); + await createProjects(); await createToggle( { name: featureName, @@ -397,7 +465,7 @@ test('should export tags', async () => { }); test('returns no features, when no feature was requested', async () => { - await createProject(); + await createProjects(); await createToggle({ name: 'first_feature', description: 'the #1 feature', @@ -551,7 +619,7 @@ const validateImport = (importPayload: ImportTogglesSchema, status = 200) => .expect(status); test('import features to existing project and environment', async () => { - await createProject(); + await createProjects(); await importToggles(defaultImportPayload); @@ -587,7 +655,7 @@ test('import features to existing project and environment', async () => { }); test('importing same JSON should work multiple times in a row', async () => { - await createProject(); + await createProjects(); await importToggles(defaultImportPayload); await importToggles(defaultImportPayload); @@ -619,7 +687,7 @@ test('importing same JSON should work multiple times in a row', async () => { }); test('reject import with unknown context fields', async () => { - await createProject(); + await createProjects(); const contextField = { name: 'ContextField1', legalValues: [{ value: 'Value1', description: '' }], @@ -650,7 +718,7 @@ test('reject import with unknown context fields', async () => { }); test('reject import with unsupported strategies', async () => { - await createProject(); + await createProjects(); const importPayloadWithContextFields: ImportTogglesSchema = { ...defaultImportPayload, data: { @@ -673,7 +741,7 @@ test('reject import with unsupported strategies', async () => { }); test('validate import data', async () => { - await createProject(); + await createProjects(); const contextField: IContextFieldDto = { name: 'validate_context_field', legalValues: [{ value: 'Value1' }], @@ -731,7 +799,7 @@ test('validate import data', async () => { }); test('should create new context', async () => { - await createProject(); + await createProjects(); const context = { name: 'create-new-context', legalValues: [{ value: 'Value1' }], @@ -751,7 +819,7 @@ test('should create new context', async () => { }); test('should not import archived features tags', async () => { - await createProject(); + await createProjects(); await importToggles(defaultImportPayload); await archiveFeature(defaultFeature); diff --git a/src/lib/openapi/spec/create-feature-strategy-schema.ts b/src/lib/openapi/spec/create-feature-strategy-schema.ts index 40ad81e566..56df6a795c 100644 --- a/src/lib/openapi/spec/create-feature-strategy-schema.ts +++ b/src/lib/openapi/spec/create-feature-strategy-schema.ts @@ -22,6 +22,13 @@ export const createFeatureStrategySchema = { parameters: { $ref: '#/components/schemas/parametersSchema', }, + segments: { + type: 'array', + description: 'Ids of segments to use for this strategy', + items: { + type: 'number', + }, + }, }, components: { schemas: { diff --git a/src/lib/routes/admin-api/project/project-features.ts b/src/lib/routes/admin-api/project/project-features.ts index e145d6a0f7..3b6b47ed03 100644 --- a/src/lib/routes/admin-api/project/project-features.ts +++ b/src/lib/routes/admin-api/project/project-features.ts @@ -80,10 +80,7 @@ const PATH_STRATEGY = `${PATH_STRATEGIES}/:strategyId`; type ProjectFeaturesServices = Pick< IUnleashServices, - | 'featureToggleServiceV2' - | 'projectHealthService' - | 'openApiService' - | 'segmentService' + 'featureToggleServiceV2' | 'projectHealthService' | 'openApiService' >; export default class ProjectFeaturesController extends Controller { diff --git a/src/lib/segments/segment-service-interface.ts b/src/lib/segments/segment-service-interface.ts index 6cb5ec911d..7fc85c8791 100644 --- a/src/lib/segments/segment-service-interface.ts +++ b/src/lib/segments/segment-service-interface.ts @@ -11,6 +11,8 @@ export interface ISegmentService { getByStrategy(strategyId: string): Promise; + get(id: number): Promise; + getActive(): Promise; getAll(): Promise; diff --git a/src/lib/services/feature-toggle-service.ts b/src/lib/services/feature-toggle-service.ts index f7340ac4d2..c3ae7a08dc 100644 --- a/src/lib/services/feature-toggle-service.ts +++ b/src/lib/services/feature-toggle-service.ts @@ -194,7 +194,7 @@ class FeatureToggleService { } } - async validateFeatureContext({ + async validateFeatureBelongsToProject({ featureName, projectId, }: IFeatureContext): Promise { @@ -207,9 +207,9 @@ class FeatureToggleService { } } - validateFeatureStrategyContext( + validateUpdatedProperties( + { featureName, projectId }: IFeatureContext, strategy: IFeatureStrategy, - { featureName, projectId }: IFeatureStrategyContext, ): void { if (strategy.projectId !== projectId) { throw new InvalidOperationError( @@ -224,6 +224,27 @@ class FeatureToggleService { } } + async validateProjectCanAccessSegments( + projectId: string, + segmentIds?: number[], + ): Promise { + if (segmentIds && segmentIds.length > 0) { + await Promise.all( + segmentIds.map((segmentId) => + this.segmentService.get(segmentId), + ), + ).then((segments) => + segments.map((segment) => { + if (segment.project && segment.project !== projectId) { + throw new BadDataError( + `The segment "${segment.name}" with id ${segment.id} does not belong to project "${projectId}".`, + ); + } + }), + ); + } + } + async validateConstraints( constraints: IConstraint[], ): Promise { @@ -373,7 +394,12 @@ class FeatureToggleService { createdBy: string, ): Promise> { const { featureName, projectId, environment } = context; - await this.validateFeatureContext(context); + await this.validateFeatureBelongsToProject(context); + + await this.validateProjectCanAccessSegments( + projectId, + strategyConfig.segments, + ); if ( strategyConfig.constraints && @@ -468,7 +494,11 @@ class FeatureToggleService { ): Promise> { const { projectId, environment, featureName } = context; const existingStrategy = await this.featureStrategiesStore.get(id); - this.validateFeatureStrategyContext(existingStrategy, context); + this.validateUpdatedProperties(context, existingStrategy); + await this.validateProjectCanAccessSegments( + projectId, + updates.segments, + ); if (existingStrategy.id === id) { if (updates.constraints && updates.constraints.length > 0) { @@ -526,7 +556,7 @@ class FeatureToggleService { const { projectId, environment, featureName } = context; const existingStrategy = await this.featureStrategiesStore.get(id); - this.validateFeatureStrategyContext(existingStrategy, context); + this.validateUpdatedProperties(context, existingStrategy); if (existingStrategy.id === id) { existingStrategy.parameters[name] = String(value); @@ -589,7 +619,7 @@ class FeatureToggleService { ): Promise { const existingStrategy = await this.featureStrategiesStore.get(id); const { featureName, projectId, environment } = context; - this.validateFeatureStrategyContext(existingStrategy, context); + this.validateUpdatedProperties(context, existingStrategy); await this.featureStrategiesStore.delete(id); @@ -667,7 +697,10 @@ class FeatureToggleService { userId, }: IGetFeatureParams): Promise { if (projectId) { - await this.validateFeatureContext({ featureName, projectId }); + await this.validateFeatureBelongsToProject({ + featureName, + projectId, + }); } if (environmentVariants) { @@ -901,7 +934,7 @@ class FeatureToggleService { userName: string, featureName: string, ): Promise { - await this.validateFeatureContext({ featureName, projectId }); + await this.validateFeatureBelongsToProject({ featureName, projectId }); this.logger.info(`${userName} updates feature toggle ${featureName}`); @@ -1066,7 +1099,10 @@ class FeatureToggleService { const feature = await this.featureToggleStore.get(featureName); if (projectId) { - await this.validateFeatureContext({ featureName, projectId }); + await this.validateFeatureBelongsToProject({ + featureName, + projectId, + }); } await this.featureToggleStore.archive(featureName); diff --git a/src/test/e2e/api/client/segment.e2e.test.ts b/src/test/e2e/api/client/segment.e2e.test.ts index 2e7e49c218..3a9d89c756 100644 --- a/src/test/e2e/api/client/segment.e2e.test.ts +++ b/src/test/e2e/api/client/segment.e2e.test.ts @@ -14,12 +14,18 @@ import { } from '../../../../lib/util/segments'; import { collectIds } from '../../../../lib/util/collect-ids'; import { arraysHaveSameItems } from '../../../../lib/util/arraysHaveSameItems'; -import { UpsertSegmentSchema } from 'lib/openapi'; +import { + CreateFeatureSchema, + CreateFeatureStrategySchema, + FeatureStrategySchema, + UpsertSegmentSchema, +} from 'lib/openapi'; +import { DEFAULT_ENV } from '../../../../lib/util'; +import { DEFAULT_PROJECT } from '../../../../lib/types'; let db: ITestDb; let app: IUnleashTest; -const FEATURES_ADMIN_BASE_PATH = '/api/admin/features'; const FEATURES_CLIENT_BASE_PATH = '/api/client/features'; const fetchSegments = (): Promise => { @@ -28,7 +34,7 @@ const fetchSegments = (): Promise => { const fetchFeatures = (): Promise => { return app.request - .get(FEATURES_ADMIN_BASE_PATH) + .get(`/api/admin/features`) .expect(200) .then((res) => res.body.features); }; @@ -40,33 +46,86 @@ const fetchClientFeatures = (): Promise => { .then((res) => res.body.features); }; -const createSegment = (postData: UpsertSegmentSchema): Promise => { +const createSegment = (postData: UpsertSegmentSchema): Promise => { return app.services.segmentService.create(postData, { email: 'test@example.com', }); }; -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 => { +const mockStrategy = (segments: number[] = []) => { + return { + name: randomId(), + parameters: {}, + constraints: [], + segments, + }; +}; + +const createProjects = async (projects: string[] = [DEFAULT_PROJECT]) => { + for (const project of projects) { + await db.stores.projectStore.create({ + name: project, + description: '', + id: project, + mode: 'open' as const, + }); + await app.request + .post(`/api/admin/projects/${project}/environments`) + .send({ + environment: DEFAULT_ENV, + }) + .expect(200); + } +}; + +const createFeatureToggle = async ( + feature: CreateFeatureSchema, + strategies: CreateFeatureStrategySchema[] = [mockStrategy()], + project = DEFAULT_PROJECT, + environment = DEFAULT_ENV, + expectStatusCode = 201, + expectSegmentStatusCodes: { status: number; message?: string }[] = [ + { status: 200 }, + ], +): Promise => { + await app.request + .post(`/api/admin/projects/${project}/features`) + .send(feature) + .expect(expectStatusCode); + + let processed = 0; + for (const strategy of strategies) { + const { body, status } = await app.request + .post( + `/api/admin/projects/${project}/features/${feature.name}/environments/${environment}/strategies`, + ) + .send(strategy); + const expectation = expectSegmentStatusCodes[processed++]; + expect(status).toBe(expectation.status); + if (expectation.message) { + expect(JSON.stringify(body)).toContain(expectation.message); + } + } +}; + +const updateFeatureStrategy = async ( + featureName: string, + strategy: FeatureStrategySchema, + project = DEFAULT_PROJECT, + environment = DEFAULT_ENV, + expectedStatus = 200, +): Promise => { + const { status } = await app.request + .put( + `/api/admin/projects/${project}/features/${featureName}/environments/${environment}/strategies/${strategy.id}`, + ) + .send(strategy); + expect(status).toBe(expectedStatus); +}; + +const mockFeatureToggle = () => { return { name: randomId(), - strategies: [{ name: randomId(), constraints: [], parameters: {} }], }; }; @@ -99,19 +158,19 @@ const fetchClientResponse = (): Promise<{ const createTestSegments = async () => { const constraints = mockConstraints(); - await createSegment({ name: 'S1', constraints }); - await createSegment({ name: 'S2', constraints }); - await createSegment({ name: 'S3', constraints }); + const segment1 = await createSegment({ name: 'S1', constraints }); + const segment2 = await createSegment({ name: 'S2', constraints }); + const segment3 = await createSegment({ name: 'S3', constraints }); - await createFeatureToggle(mockFeatureToggle()); - await createFeatureToggle(mockFeatureToggle()); + await createFeatureToggle(mockFeatureToggle(), [ + mockStrategy([segment1.id, segment2.id]), + ]); + await createFeatureToggle(mockFeatureToggle(), [ + mockStrategy([segment2.id]), + ]); 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); + return [segment1, segment2, segment3]; }; beforeAll(async () => { @@ -168,53 +227,50 @@ test('should validate segment constraint values limit with multiple constraints' }); test('should validate feature strategy segment limit', async () => { - await createSegment({ name: 'S1', constraints: [] }); - await createSegment({ name: 'S2', constraints: [] }); - await createSegment({ name: 'S3', constraints: [] }); - await createSegment({ name: 'S4', constraints: [] }); - await createSegment({ name: 'S5', constraints: [] }); - await createSegment({ name: 'S6', constraints: [] }); - await createFeatureToggle(mockFeatureToggle()); - const [feature1] = await fetchFeatures(); - const segments = await fetchSegments(); + const segments: ISegment[] = []; + for (const id of [1, 2, 3, 4, 5, 6]) { + segments.push(await createSegment({ name: `S${id}`, constraints: [] })); + } - await addSegmentToStrategy(segments[0].id, feature1.strategies[0].id); - await addSegmentToStrategy(segments[1].id, feature1.strategies[0].id); - await addSegmentToStrategy(segments[2].id, feature1.strategies[0].id); - await addSegmentToStrategy(segments[3].id, feature1.strategies[0].id); - await addSegmentToStrategy(segments[4].id, feature1.strategies[0].id); - - await expect( - addSegmentToStrategy(segments[5].id, feature1.strategies[0].id), - ).rejects.toThrow( - `Strategies may not have more than ${DEFAULT_STRATEGY_SEGMENTS_LIMIT} segments`, + await createFeatureToggle( + mockFeatureToggle(), + [mockStrategy(segments.map((s) => s.id))], + DEFAULT_PROJECT, + DEFAULT_ENV, + 201, + [ + { + status: 400, + message: `Strategies may not have more than ${DEFAULT_STRATEGY_SEGMENTS_LIMIT} segments`, + }, + ], ); }); test('should clone feature strategy segments', async () => { const constraints = mockConstraints(); - await createSegment({ name: 'S1', constraints }); - await createFeatureToggle(mockFeatureToggle()); + const segment1 = await createSegment({ name: 'S1', constraints }); + await createFeatureToggle(mockFeatureToggle(), [ + mockStrategy([segment1.id]), + ]); 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); + 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, + strategy1!, + strategy2!, ); - segments1 = await app.services.segmentService.getByStrategy(strategy1); - segments2 = await app.services.segmentService.getByStrategy(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]); }); @@ -239,9 +295,18 @@ test('should inline segment constraints into features by default', async () => { 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); + + // add segment3 to all features + for (const feature of [feature1, feature2, feature3]) { + const strategy = { + ...feature.strategies[0], + segments: feature.strategies[0].segments ?? [], + }; + await updateFeatureStrategy(feature.name, { + ...strategy, + segments: [...strategy.segments, segment3.id], + }); + } const clientFeatures = await fetchClientFeatures(); const clientStrategies = clientFeatures.flatMap((f) => f.strategies); @@ -316,3 +381,59 @@ test('should send all segments that are in use by feature', async () => { true, ); }); + +describe('project-specific segments', () => { + test(`can create a toggle with a project-specific segment`, async () => { + const segmentName = 'my-segment'; + const project = randomId(); + await createProjects([project]); + const segment = await createSegment({ + name: segmentName, + project, + constraints: [], + }); + const strategy = { + name: 'default', + parameters: {}, + constraints: [], + segments: [segment.id], + }; + await createFeatureToggle( + { + name: 'first_feature', + description: 'the #1 feature', + }, + [strategy], + project, + ); + }); + + test(`can't create a toggle with a segment from a different project`, async () => { + const segmentName = 'my-segment'; + const project1 = randomId(); + const project2 = randomId(); + await createProjects([project1, project2]); + const segment = await createSegment({ + name: segmentName, + project: project1, + constraints: [], + }); + const strategy = { + name: 'default', + parameters: {}, + constraints: [], + segments: [segment.id], + }; + await createFeatureToggle( + { + name: 'first_feature', + description: 'the #1 feature', + }, + [strategy], + project2, + DEFAULT_ENV, + 201, + [{ status: 400 }], + ); + }); +}); diff --git a/src/test/e2e/api/openapi/__snapshots__/openapi.e2e.test.ts.snap b/src/test/e2e/api/openapi/__snapshots__/openapi.e2e.test.ts.snap index c415e90176..36d6971e2e 100644 --- a/src/test/e2e/api/openapi/__snapshots__/openapi.e2e.test.ts.snap +++ b/src/test/e2e/api/openapi/__snapshots__/openapi.e2e.test.ts.snap @@ -889,6 +889,13 @@ exports[`should serve the OpenAPI spec 1`] = ` "parameters": { "$ref": "#/components/schemas/parametersSchema", }, + "segments": { + "description": "Ids of segments to use for this strategy", + "items": { + "type": "number", + }, + "type": "array", + }, "sortOrder": { "type": "number", },