diff --git a/src/lib/features/feature-toggle/createFeatureToggleService.ts b/src/lib/features/feature-toggle/createFeatureToggleService.ts index f00bd234bb..4f616357fb 100644 --- a/src/lib/features/feature-toggle/createFeatureToggleService.ts +++ b/src/lib/features/feature-toggle/createFeatureToggleService.ts @@ -39,6 +39,8 @@ import { createFakeSegmentService, createSegmentService, } from '../segment/createSegmentService'; +import StrategyStore from '../../db/strategy-store'; +import FakeStrategiesStore from '../../../test/fixtures/fake-strategies-store'; export const createFeatureToggleService = ( db: Db, @@ -76,6 +78,7 @@ export const createFeatureToggleService = ( flagResolver, ); const groupStore = new GroupStore(db); + const strategyStore = new StrategyStore(db, getLogger); const accountStore = new AccountStore(db, getLogger); const accessStore = new AccessStore(db, eventBus, getLogger); const roleStore = new RoleStore(db, eventBus, getLogger); @@ -105,6 +108,7 @@ export const createFeatureToggleService = ( featureTagStore, featureEnvironmentStore, contextFieldStore, + strategyStore, }, { getLogger, flagResolver }, segmentService, @@ -119,6 +123,7 @@ export const createFakeFeatureToggleService = ( ): FeatureToggleService => { const { getLogger, flagResolver } = config; const eventStore = new FakeEventStore(); + const strategyStore = new FakeStrategiesStore(); const featureStrategiesStore = new FakeFeatureStrategiesStore(); const featureToggleStore = new FakeFeatureToggleStore(); const featureToggleClientStore = new FakeFeatureToggleClientStore(); @@ -152,6 +157,7 @@ export const createFakeFeatureToggleService = ( featureTagStore, featureEnvironmentStore, contextFieldStore, + strategyStore, }, { getLogger, flagResolver }, segmentService, diff --git a/src/lib/services/feature-toggle-service.ts b/src/lib/services/feature-toggle-service.ts index 40cf990c07..a2393bccf4 100644 --- a/src/lib/services/feature-toggle-service.ts +++ b/src/lib/services/feature-toggle-service.ts @@ -42,6 +42,7 @@ import { WeightType, StrategiesOrderChangedEvent, PotentiallyStaleOnEvent, + IStrategyStore, } from '../types'; import { Logger } from '../logger'; import BadDataError from '../error/bad-data-error'; @@ -118,6 +119,8 @@ class FeatureToggleService { private featureStrategiesStore: IFeatureStrategiesStore; + private strategyStore: IStrategyStore; + private featureToggleStore: IFeatureToggleStore; private featureToggleClientStore: IFeatureToggleClientStore; @@ -150,6 +153,7 @@ class FeatureToggleService { featureTagStore, featureEnvironmentStore, contextFieldStore, + strategyStore, }: Pick< IUnleashStores, | 'featureStrategiesStore' @@ -160,6 +164,7 @@ class FeatureToggleService { | 'featureTagStore' | 'featureEnvironmentStore' | 'contextFieldStore' + | 'strategyStore' >, { getLogger, @@ -171,6 +176,7 @@ class FeatureToggleService { ) { this.logger = getLogger('services/feature-toggle-service.ts'); this.featureStrategiesStore = featureStrategiesStore; + this.strategyStore = strategyStore; this.featureToggleStore = featureToggleStore; this.featureToggleClientStore = featureToggleClientStore; this.tagStore = featureTagStore; @@ -225,24 +231,24 @@ class FeatureToggleService { validateUpdatedProperties( { featureName, projectId }: IFeatureContext, - strategy: IFeatureStrategy, + existingStrategy: IFeatureStrategy, ): void { - if (strategy.projectId !== projectId) { + if (existingStrategy.projectId !== projectId) { throw new InvalidOperationError( 'You can not change the projectId for an activation strategy.', ); } - if (strategy.featureName !== featureName) { + if (existingStrategy.featureName !== featureName) { throw new InvalidOperationError( 'You can not change the featureName for an activation strategy.', ); } if ( - strategy.parameters && - 'stickiness' in strategy.parameters && - strategy.parameters.stickiness === '' + existingStrategy.parameters && + 'stickiness' in existingStrategy.parameters && + existingStrategy.parameters.stickiness === '' ) { throw new InvalidOperationError( 'You can not have an empty string for stickiness.', @@ -271,6 +277,19 @@ class FeatureToggleService { } } + async validateStrategyType( + strategyName: string | undefined, + ): Promise { + if (strategyName !== undefined) { + const exists = await this.strategyStore.exists(strategyName); + if (!exists) { + throw new BadDataError( + `Could not find strategy type with name ${strategyName}`, + ); + } + } + } + async validateConstraints( constraints: IConstraint[], ): Promise { @@ -502,6 +521,7 @@ class FeatureToggleService { const { featureName, projectId, environment } = context; await this.validateFeatureBelongsToProject(context); + await this.validateStrategyType(strategyConfig.name); await this.validateProjectCanAccessSegments( projectId, strategyConfig.segments, @@ -602,7 +622,7 @@ class FeatureToggleService { */ async updateStrategy( id: string, - updates: Partial, + updates: Partial, context: IFeatureStrategyContext, userName: string, user?: User, @@ -640,13 +660,15 @@ class FeatureToggleService { async unprotectedUpdateStrategy( id: string, - updates: Partial, + updates: Partial, context: IFeatureStrategyContext, userName: string, ): Promise> { const { projectId, environment, featureName } = context; const existingStrategy = await this.featureStrategiesStore.get(id); + this.validateUpdatedProperties(context, existingStrategy); + await this.validateStrategyType(updates.name); await this.validateProjectCanAccessSegments( projectId, updates.segments, 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 63d673108a..ac58a46e50 100644 --- a/src/test/e2e/api/admin/project/features.e2e.test.ts +++ b/src/test/e2e/api/admin/project/features.e2e.test.ts @@ -57,26 +57,28 @@ const createSegment = async (segmentName: string) => { const createStrategy = async ( featureName: string, payload: IStrategyConfig, + expectedCode = 200, ) => { return app.request .post( `/api/admin/projects/default/features/${featureName}/environments/default/strategies`, ) .send(payload) - .expect(200); + .expect(expectedCode); }; const updateStrategy = async ( featureName: string, strategyId: string, payload: IStrategyConfig, + expectedCode = 200, ) => { const { body } = await app.request .put( `/api/admin/projects/default/features/${featureName}/environments/default/strategies/${strategyId}`, ) .send(payload) - .expect(200); + .expect(expectedCode); return body; }; @@ -110,8 +112,6 @@ afterEach(async () => { ), ), ); - - await db.stores.strategyStore.deleteAll(); }); afterAll(async () => { @@ -2355,7 +2355,7 @@ test('should handle strategy variants', async () => { await app.createFeature(feature.name); const strategyWithInvalidVariant = { - name: uuidv4(), + name: 'flexibleRollout', constraints: [], variants: [ { @@ -2386,7 +2386,7 @@ test('should handle strategy variants', async () => { stickiness: 'default', }; const strategyWithValidVariant = { - name: uuidv4(), + name: 'flexibleRollout', constraints: [], variants: [variant], }; @@ -2438,7 +2438,7 @@ test('should reject invalid constraint values for multi-valued constraints', asy }); const mockStrategy = (values: string[]) => ({ - name: uuidv4(), + name: 'flexibleRollout', constraints: [{ contextName: 'userId', operator: 'IN', values }], }); @@ -2497,7 +2497,7 @@ test('should add default constraint values for single-valued constraints', async }; const mockStrategy = (constraint: unknown) => ({ - name: uuidv4(), + name: 'flexibleRollout', constraints: [constraint], }); @@ -2544,7 +2544,7 @@ test('should allow long parameter values', async () => { }); const strategy = { - name: uuidv4(), + name: 'flexibleRollout', parameters: { a: 'b'.repeat(500) }, }; @@ -2582,7 +2582,7 @@ test('should change strategy sort order when payload is valid', async () => { `/api/admin/projects/default/features/${toggle.name}/environments/default/strategies`, ) .send({ - name: 'gradualrollout', + name: 'flexibleRollout', parameters: { userId: 'string', }, @@ -2668,7 +2668,7 @@ test('should return strategies in correct order when new strategies are added', `/api/admin/projects/default/features/${toggle.name}/environments/default/strategies`, ) .send({ - name: 'gradualrollout', + name: 'flexibleRollout', parameters: { userId: 'string', }, @@ -2705,7 +2705,7 @@ test('should return strategies in correct order when new strategies are added', `/api/admin/projects/default/features/${toggle.name}/environments/default/strategies`, ) .send({ - name: 'gradualrollout', + name: 'flexibleRollout', parameters: { userId: 'string', }, @@ -2717,7 +2717,7 @@ test('should return strategies in correct order when new strategies are added', `/api/admin/projects/default/features/${toggle.name}/environments/default/strategies`, ) .send({ - name: 'gradualrollout', + name: 'flexibleRollout', parameters: { userId: 'string', }, @@ -2993,7 +2993,7 @@ test('should return disabled strategies', async () => { `/api/admin/projects/default/features/${toggle.name}/environments/default/strategies`, ) .send({ - name: 'gradualrollout', + name: 'flexibleRollout', parameters: { userId: 'string', }, @@ -3355,3 +3355,42 @@ test('Updating feature strategy sort-order should trigger a an event', async () expect(res.body.events[0].type).toBe('strategy-order-changed'); }); }); + +test('should not be allowed to create with invalid strategy type name', async () => { + const feature = { name: uuidv4(), impressionData: false }; + await app.createFeature(feature.name); + await createStrategy( + feature.name, + { + name: 'random-type', + parameters: { + userId: 'string', + }, + }, + 400, + ); +}); + +test('should not be allowed to update with invalid strategy type name', async () => { + const feature = { name: uuidv4(), impressionData: false }; + await app.createFeature(feature.name); + const { body: strategyOne } = await createStrategy(feature.name, { + name: 'default', + parameters: { + userId: 'string', + }, + }); + + await updateStrategy( + feature.name, + strategyOne.id, + { + name: 'random-type', + parameters: { + userId: 'string', + }, + segments: [], + }, + 400, + ); +}); diff --git a/src/test/e2e/api/admin/state.e2e.test.ts b/src/test/e2e/api/admin/state.e2e.test.ts index a544c00d8c..56083ae689 100644 --- a/src/test/e2e/api/admin/state.e2e.test.ts +++ b/src/test/e2e/api/admin/state.e2e.test.ts @@ -382,21 +382,6 @@ test(`should not delete api_tokens on import when drop-flag is set`, async () => }, userName, ); - await app.services.featureToggleServiceV2.createStrategy( - { - name: 'default', - constraints: [ - { contextName: 'userId', operator: 'IN', values: ['123'] }, - ], - parameters: {}, - }, - { - projectId, - featureName, - environment, - }, - userName, - ); await app.services.apiTokenService.createApiTokenWithProjects({ tokenName: apiTokenName, type: ApiTokenType.CLIENT, diff --git a/src/test/e2e/api/admin/strategy.e2e.test.ts b/src/test/e2e/api/admin/strategy.e2e.test.ts index 646a6b7fca..5d8cbe2522 100644 --- a/src/test/e2e/api/admin/strategy.e2e.test.ts +++ b/src/test/e2e/api/admin/strategy.e2e.test.ts @@ -29,7 +29,7 @@ test('gets all strategies', async () => { .expect('Content-Type', /json/) .expect(200) .expect((res) => { - expect(res.body.strategies).toHaveLength(2); + expect(res.body.strategies).toHaveLength(3); }); }); diff --git a/src/test/e2e/api/client/feature.e2e.test.ts b/src/test/e2e/api/client/feature.e2e.test.ts index 82f4bb2ea1..7121597beb 100644 --- a/src/test/e2e/api/client/feature.e2e.test.ts +++ b/src/test/e2e/api/client/feature.e2e.test.ts @@ -217,7 +217,7 @@ test('Can get strategies for specific environment', async () => { `/api/admin/projects/default/features/${featureName}/environments/testing/strategies`, ) .send({ - name: 'custom1', + name: 'default', }) .expect(200); @@ -229,7 +229,7 @@ test('Can get strategies for specific environment', async () => { expect(res.body.name).toBe(featureName); expect(res.body.strategies).toHaveLength(1); expect( - res.body.strategies.find((s) => s.name === 'custom1'), + res.body.strategies.find((s) => s.name === 'default'), ).toBeDefined(); }); }); diff --git a/src/test/e2e/api/client/feature.token.access.e2e.test.ts b/src/test/e2e/api/client/feature.token.access.e2e.test.ts index 4992fd8476..670813d986 100644 --- a/src/test/e2e/api/client/feature.token.access.e2e.test.ts +++ b/src/test/e2e/api/client/feature.token.access.e2e.test.ts @@ -61,7 +61,7 @@ beforeAll(async () => { ); await featureToggleServiceV2.createStrategy( { - name: 'custom-testing', + name: 'flexibleRollout', constraints: [], parameters: {}, }, @@ -152,7 +152,7 @@ test('returns feature toggle with testing environment config', async () => { expect(features).toHaveLength(2); expect(f1.strategies).toHaveLength(1); - expect(f1.strategies[0].name).toBe('custom-testing'); + expect(f1.strategies[0].name).toBe('flexibleRollout'); expect(f2.strategies).toHaveLength(1); expect(query.project[0]).toBe(project); expect(query.environment).toBe(environment); diff --git a/src/test/e2e/api/client/segment.e2e.test.ts b/src/test/e2e/api/client/segment.e2e.test.ts index 04fc17cfe4..a0cbf7fffe 100644 --- a/src/test/e2e/api/client/segment.e2e.test.ts +++ b/src/test/e2e/api/client/segment.e2e.test.ts @@ -66,7 +66,7 @@ const updateSegment = ( const mockStrategy = (segments: number[] = []) => { return { - name: randomId(), + name: 'flexibleRollout', parameters: {}, constraints: [], segments, diff --git a/src/test/e2e/api/proxy/proxy.e2e.test.ts b/src/test/e2e/api/proxy/proxy.e2e.test.ts index df4cc569fa..d164e0224b 100644 --- a/src/test/e2e/api/proxy/proxy.e2e.test.ts +++ b/src/test/e2e/api/proxy/proxy.e2e.test.ts @@ -463,18 +463,10 @@ test('should filter features by strategies', async () => { enabled: false, strategies: [], }); - await createFeatureToggle({ - name: 'featureWithUnknownStrategy', - enabled: true, - strategies: [{ name: 'unknown', constraints: [], parameters: {} }], - }); await createFeatureToggle({ name: 'featureWithMultipleStrategies', enabled: true, - strategies: [ - { name: 'default', constraints: [], parameters: {} }, - { name: 'unknown', constraints: [], parameters: {} }, - ], + strategies: [{ name: 'default', constraints: [], parameters: {} }], }); await app.request .get('/api/frontend') diff --git a/src/test/e2e/helpers/database.json b/src/test/e2e/helpers/database.json index d90668d3d0..602bba301f 100644 --- a/src/test/e2e/helpers/database.json +++ b/src/test/e2e/helpers/database.json @@ -14,6 +14,30 @@ "type": "string" } ] + }, + { + "name": "flexibleRollout", + "description": "Roll out to a percentage of your userbase, and ensure that the experience is the same for the user on each visit.", + "parameters": [ + { + "name": "rollout", + "type": "percentage", + "description": "", + "required": false + }, + { + "name": "stickiness", + "type": "string", + "description": "Used define stickiness. Possible values: default, userId, sessionId, random", + "required": true + }, + { + "name": "groupId", + "type": "string", + "description": "Used to define a activation groups, which allows you to correlate across feature toggles.", + "required": true + } + ] } ], "contextFields": [