From 79fcfc26b8cdb08bdf4bad7e7631c312327e99d5 Mon Sep 17 00:00:00 2001 From: Christopher Kolstad Date: Mon, 25 Mar 2024 10:38:41 +0100 Subject: [PATCH] fix: use defaults when creating gradualRollout strategies (#6623) Via the API you can currently create gradualRollout strategies without any parameters set, when visiting the UI afterwards, you can edit this, because the UI reads the parameter list from the database and sees that some parameters are required, and refuses to accept the data. This PR adds defaults for gradualRollout strategies created from the API, making sure gradual rollout strategies always have `rollout`, `groupId` and `stickiness` set. --- .../feature-toggle-strategies-store.ts | 45 ++++++++- ...eature-toggle-strategies-store.e2e.test.ts | 95 ++++++++++++++++++- 2 files changed, 137 insertions(+), 3 deletions(-) diff --git a/src/lib/features/feature-toggle/feature-toggle-strategies-store.ts b/src/lib/features/feature-toggle/feature-toggle-strategies-store.ts index 71e44a69ed..a7c71e7ef8 100644 --- a/src/lib/features/feature-toggle/feature-toggle-strategies-store.ts +++ b/src/lib/features/feature-toggle/feature-toggle-strategies-store.ts @@ -24,8 +24,9 @@ import FeatureToggleStore from './feature-toggle-store'; import { ensureStringValue, mapValues } from '../../util'; import type { IFeatureProjectUserParams } from './feature-toggle-controller'; import type { Db } from '../../db/db'; -import Raw = Knex.Raw; import { isAfter } from 'date-fns'; +import merge from 'deepmerge'; +import Raw = Knex.Raw; const COLUMNS = [ 'id', @@ -47,6 +48,7 @@ const T = { featureStrategySegment: 'feature_strategy_segment', featureEnvs: 'feature_environments', strategies: 'strategies', + projectSettings: 'project_settings', }; interface IFeatureStrategiesTable { @@ -149,6 +151,32 @@ function mapStrategyUpdate( return update; } +function mergeAll(objects: Partial[]): T { + return merge.all(objects.filter((i) => i)); +} + +const defaultParameters = ( + params: PartialSome, + stickiness: string, +) => { + if (params.strategyName === 'gradualRollout') { + return { + rollout: '100', + stickiness, + groupId: params.featureName, + }; + } else { + /// We don't really have good defaults for the other kinds of known strategies, so return an empty map. + return {}; + } +}; + +const parametersWithDefaults = ( + params: PartialSome, + stickiness: string, +) => { + return mergeAll([defaultParameters(params, stickiness), params.parameters]); +}; class FeatureStrategiesStore implements IFeatureStrategiesStore { private db: Db; @@ -214,7 +242,13 @@ class FeatureStrategiesStore implements IFeatureStrategiesStore { }); return Number.isInteger(max) ? max + 1 : 0; } - + private async getDefaultStickiness(projectName: string): Promise { + const defaultFromDb = await this.db(T.projectSettings) + .select('default_stickiness') + .where('project', projectName) + .first(); + return defaultFromDb?.default_stickiness || 'default'; + } async createStrategyFeatureEnv( strategyConfig: PartialSome, ): Promise { @@ -224,6 +258,13 @@ class FeatureStrategiesStore implements IFeatureStrategiesStore { strategyConfig.featureName, strategyConfig.environment, )); + const stickiness = await this.getDefaultStickiness( + strategyConfig.projectId, + ); + strategyConfig.parameters = parametersWithDefaults( + strategyConfig, + stickiness, + ); const strategyRow = mapInput({ id: uuidv4(), ...strategyConfig, diff --git a/src/lib/features/feature-toggle/tests/feature-toggle-strategies-store.e2e.test.ts b/src/lib/features/feature-toggle/tests/feature-toggle-strategies-store.e2e.test.ts index c2e27c1ee4..9e938a612b 100644 --- a/src/lib/features/feature-toggle/tests/feature-toggle-strategies-store.e2e.test.ts +++ b/src/lib/features/feature-toggle/tests/feature-toggle-strategies-store.e2e.test.ts @@ -4,12 +4,13 @@ import dbInit, { type ITestDb, } from '../../../../test/e2e/helpers/database-init'; import getLogger from '../../../../test/fixtures/no-logger'; -import type { IUnleashStores } from '../../../types'; +import type { IProjectStore, IUnleashStores } from '../../../types'; let stores: IUnleashStores; let db: ITestDb; let featureStrategiesStore: IFeatureStrategiesStore; let featureToggleStore: IFeatureToggleStore; +let projectStore: IProjectStore; const featureName = 'test-strategies-move-project'; @@ -18,6 +19,7 @@ beforeAll(async () => { stores = db.stores; featureStrategiesStore = stores.featureStrategiesStore; featureToggleStore = stores.featureToggleStore; + projectStore = stores.projectStore; await featureToggleStore.create('default', { name: featureName, createdByUserId: 9999, @@ -144,3 +146,94 @@ test('Can query for features with namePrefix and tags', async () => { }); expect(features).toHaveLength(1); }); + +describe('strategy parameters default to sane defaults', () => { + test('Creating a gradualRollout strategy with no parameters uses the default for all necessary fields', async () => { + const toggle = await featureToggleStore.create('default', { + name: 'testing-strategy-parameters', + createdByUserId: 9999, + }); + const strategy = await featureStrategiesStore.createStrategyFeatureEnv({ + strategyName: 'gradualRollout', + projectId: 'default', + environment: 'default', + featureName: toggle.name, + constraints: [], + sortOrder: 15, + parameters: {}, + }); + expect(strategy.parameters).toEqual({ + rollout: '100', + groupId: toggle.name, + stickiness: 'default', + }); + }); + test('Creating a gradualRollout strategy with some parameters, only uses defaults for those not set', async () => { + const toggle = await featureToggleStore.create('default', { + name: 'testing-strategy-parameters-with-some-parameters', + createdByUserId: 9999, + }); + const strategy = await featureStrategiesStore.createStrategyFeatureEnv({ + strategyName: 'gradualRollout', + projectId: 'default', + environment: 'default', + featureName: toggle.name, + constraints: [], + sortOrder: 15, + parameters: { + rollout: '60', + stickiness: 'userId', + }, + }); + expect(strategy.parameters).toEqual({ + rollout: '60', + groupId: toggle.name, + stickiness: 'userId', + }); + }); + test('Creating an applicationHostname strategy does not get unnecessary parameters set', async () => { + const toggle = await featureToggleStore.create('default', { + name: 'testing-strategy-parameters-for-applicationHostname', + createdByUserId: 9999, + }); + const strategy = await featureStrategiesStore.createStrategyFeatureEnv({ + strategyName: 'applicationHostname', + projectId: 'default', + environment: 'default', + featureName: toggle.name, + constraints: [], + sortOrder: 15, + parameters: { + hostnames: 'myfantastichost', + }, + }); + expect(strategy.parameters).toEqual({ + hostnames: 'myfantastichost', + }); + }); + test('Strategy picks the default stickiness set for the project', async () => { + const project = await projectStore.create({ + name: 'customDefaultStickiness', + id: 'custom_default_stickiness', + }); + const defaultStickiness = 'userId'; + await db.rawDatabase.raw( + `UPDATE project_settings SET default_stickiness = ? WHERE project = ?`, + [defaultStickiness, project.id], + ); + const toggle = await featureToggleStore.create(project.id, { + name: 'testing-default-strategy-on-project', + createdByUserId: 9999, + }); + const strategy = await featureStrategiesStore.createStrategyFeatureEnv({ + strategyName: 'gradualRollout', + projectId: project.id, + environment: 'default', + featureName: toggle.name, + constraints: [], + sortOrder: 15, + parameters: {}, + }); + expect(strategy.parameters.stickiness).toBe(defaultStickiness); + }); +});