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.

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<T>(objects: Partial<T>[]): T {
+    return merge.all<T>(objects.filter((i) => i));
+}
+
+const defaultParameters = (
+    params: PartialSome<IFeatureStrategy, 'id' | 'createdAt'>,
+    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<IFeatureStrategy, 'id' | 'createdAt'>,
+    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<string> {
+        const defaultFromDb = await this.db(T.projectSettings)
+            .select('default_stickiness')
+            .where('project', projectName)
+            .first();
+        return defaultFromDb?.default_stickiness || 'default';
+    }
     async createStrategyFeatureEnv(
         strategyConfig: PartialSome<IFeatureStrategy, 'id' | 'createdAt'>,
     ): Promise<IFeatureStrategy> {
@@ -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,

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);
+    });
+});