diff --git a/src/lib/features/feature-toggle/fakes/fake-feature-strategies-store.ts b/src/lib/features/feature-toggle/fakes/fake-feature-strategies-store.ts index 710d89bd73..fb50a844e9 100644 --- a/src/lib/features/feature-toggle/fakes/fake-feature-strategies-store.ts +++ b/src/lib/features/feature-toggle/fakes/fake-feature-strategies-store.ts @@ -343,4 +343,8 @@ export default class FakeFeatureStrategiesStore getCustomStrategiesInUseCount(): Promise { return Promise.resolve(3); } + + getDefaultStickiness(_projectId: string): Promise { + return Promise.resolve('default'); + } } diff --git a/src/lib/features/feature-toggle/feature-toggle-service.ts b/src/lib/features/feature-toggle/feature-toggle-service.ts index cb48c05cbe..d495502eb4 100644 --- a/src/lib/features/feature-toggle/feature-toggle-service.ts +++ b/src/lib/features/feature-toggle/feature-toggle-service.ts @@ -117,7 +117,6 @@ import { sortStrategies } from '../../util/sortStrategies.js'; import type { ResourceLimitsSchema } from '../../openapi/index.js'; import type FeatureLinkService from '../feature-links/feature-link-service.js'; import type { IFeatureLink } from '../feature-links/feature-links-read-model-type.js'; - interface IFeatureContext { featureName: string; projectId: string; @@ -126,7 +125,6 @@ interface IFeatureContext { interface IFeatureStrategyContext extends IFeatureContext { environment: string; } - export interface IGetFeatureParams { featureName: string; archived?: boolean; @@ -370,16 +368,6 @@ export class FeatureToggleService { 'You can not change the featureName for an activation strategy.', ); } - - if ( - existingStrategy.parameters && - 'stickiness' in existingStrategy.parameters && - existingStrategy.parameters.stickiness === '' - ) { - throw new InvalidOperationError( - 'You can not have an empty string for stickiness.', - ); - } } async validateProjectCanAccessSegments( @@ -680,6 +668,78 @@ export class FeatureToggleService { ); } + private async parametersWithDefaults( + projectId: string, + featureName: string, + strategyName: string, + params: IFeatureStrategy['parameters'] | undefined, + ) { + if (strategyName === 'flexibleRollout') { + const stickiness = + !params?.stickiness || params?.stickiness === '' + ? await this.featureStrategiesStore.getDefaultStickiness( + projectId, + ) + : params?.stickiness; + return { + ...params, + rollout: params?.rollout ?? '100', + stickiness, + groupId: params?.groupId ?? featureName, + }; + } else { + // We don't really have good defaults for the other kinds of known strategies, so return an empty map. + return params ?? {}; + } + } + private async standardizeStrategyConfig( + projectId: string, + featureName: string, + strategyConfig: Unsaved, + existing?: IFeatureStrategy, + ): Promise< + { strategyName: string } & Pick< + Partial, + | 'title' + | 'disabled' + | 'variants' + | 'sortOrder' + | 'constraints' + | 'parameters' + > + > { + const { name, title, disabled, sortOrder } = strategyConfig; + let { constraints, parameters, variants } = strategyConfig; + if (constraints && constraints.length > 0) { + this.validateConstraintsLimit({ + updated: constraints, + existing: existing?.constraints ?? [], + }); + constraints = await this.validateConstraints(constraints); + } + + parameters = await this.parametersWithDefaults( + projectId, + featureName, + name, + strategyConfig.parameters, + ); + if (variants && variants.length > 0) { + await variantsArraySchema.validateAsync(variants); + const fixedVariants = this.fixVariantWeights(variants); + variants = fixedVariants; + } + + return { + strategyName: name, + title, + disabled, + sortOrder, + constraints, + variants, + parameters, + }; + } async unprotectedCreateStrategy( strategyConfig: Unsaved, context: IFeatureStrategyContext, @@ -694,34 +754,11 @@ export class FeatureToggleService { strategyConfig.segments, ); - if ( - strategyConfig.constraints && - strategyConfig.constraints.length > 0 - ) { - this.validateConstraintsLimit({ - updated: strategyConfig.constraints, - existing: [], - }); - strategyConfig.constraints = await this.validateConstraints( - strategyConfig.constraints, - ); - } - - if ( - strategyConfig.parameters && - 'stickiness' in strategyConfig.parameters && - strategyConfig.parameters.stickiness === '' - ) { - strategyConfig.parameters.stickiness = 'default'; - } - - if (strategyConfig.variants && strategyConfig.variants.length > 0) { - await variantsArraySchema.validateAsync(strategyConfig.variants); - const fixedVariants = this.fixVariantWeights( - strategyConfig.variants, - ); - strategyConfig.variants = fixedVariants; - } + const standardizedConfig = await this.standardizeStrategyConfig( + projectId, + featureName, + strategyConfig, + ); await this.validateStrategyLimit({ featureName, @@ -732,13 +769,10 @@ export class FeatureToggleService { try { const newFeatureStrategy = await this.featureStrategiesStore.createStrategyFeatureEnv({ - strategyName: strategyConfig.name, - title: strategyConfig.title, - disabled: strategyConfig.disabled, - constraints: strategyConfig.constraints || [], - variants: strategyConfig.variants || [], - parameters: strategyConfig.parameters || {}, - sortOrder: strategyConfig.sortOrder, + ...standardizedConfig, + constraints: standardizedConfig.constraints || [], + variants: standardizedConfig.variants || [], + parameters: standardizedConfig.parameters || {}, projectId, featureName, environment, @@ -864,25 +898,15 @@ export class FeatureToggleService { const existingSegments = await this.segmentService.getByStrategy(id); if (existingStrategy.id === id) { - if (updates.constraints && updates.constraints.length > 0) { - this.validateConstraintsLimit({ - updated: updates.constraints, - existing: existingStrategy.constraints, - }); - updates.constraints = await this.validateConstraints( - updates.constraints, - ); - } - - if (updates.variants && updates.variants.length > 0) { - await variantsArraySchema.validateAsync(updates.variants); - const fixedVariants = this.fixVariantWeights(updates.variants); - updates.variants = fixedVariants; - } - + const standardizedUpdates = await this.standardizeStrategyConfig( + projectId, + featureName, + { ...updates, name: updates.name! }, + existingStrategy, + ); const strategy = await this.featureStrategiesStore.updateStrategy( id, - updates, + standardizedUpdates, ); if (updates.segments && Array.isArray(updates.segments)) { 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 6303b9487f..1663d25c1d 100644 --- a/src/lib/features/feature-toggle/feature-toggle-strategies-store.ts +++ b/src/lib/features/feature-toggle/feature-toggle-strategies-store.ts @@ -28,7 +28,6 @@ import { import type { IFeatureProjectUserParams } from './feature-toggle-controller.js'; import type { Db } from '../../db/db.js'; import { isAfter } from 'date-fns'; -import merge from 'deepmerge'; import Raw = Knex.Raw; import type { ITag } from '../../tags/index.js'; @@ -157,32 +156,6 @@ 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 === 'flexibleRollout') { - 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; @@ -248,7 +221,7 @@ class FeatureStrategiesStore implements IFeatureStrategiesStore { }); return Number.isInteger(max) ? max + 1 : 0; } - private async getDefaultStickiness(projectName: string): Promise { + async getDefaultStickiness(projectName: string): Promise { const defaultFromDb = await this.db(T.projectSettings) .select('default_stickiness') .where('project', projectName) @@ -264,13 +237,6 @@ 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-service.e2e.test.ts b/src/lib/features/feature-toggle/tests/feature-toggle-service.e2e.test.ts index 0682911929..f63e609891 100644 --- a/src/lib/features/feature-toggle/tests/feature-toggle-service.e2e.test.ts +++ b/src/lib/features/feature-toggle/tests/feature-toggle-service.e2e.test.ts @@ -141,7 +141,9 @@ test('Should be able to update existing strategy configuration', async () => { TEST_AUDIT_USER, ); expect(createdConfig.id).toEqual(updatedConfig.id); - expect(updatedConfig.parameters).toEqual({ b2b: 'true' }); + expect(updatedConfig.parameters).toEqual({ + b2b: 'true', + }); }); test('Should be able to get strategy by id', async () => { @@ -758,34 +760,82 @@ test('Should return last seen at per environment', async () => { expect(featureToggle.lastSeenAt).toEqual(new Date(lastSeenAtStoreDate)); }); -test('Should return "default" for stickiness when creating a flexibleRollout strategy with empty stickiness', async () => { - const strategy = { - name: 'flexibleRollout', - parameters: { - rollout: '100', - stickiness: '', - }, - constraints: [], - }; - const feature = { - name: 'test-feature-stickiness-1', - description: 'the #1 feature', - }; - const projectId = 'default'; +test.each([ + ['empty stickiness', { rollout: '100', stickiness: '' }], + ['undefined stickiness', { rollout: '100' }], + ['undefined parameters', undefined], + [ + 'different group id and stickiness', + { rollout: '100', groupId: 'test-group', stickiness: 'userId' }, + ], + ['different rollout', { rollout: '25' }], + ['empty parameters', {}], + ['extra parameters are preserved', { extra: 'value', rollout: '100' }], +])( + 'Should use default parameters when creating a flexibleRollout strategy with %s', + async (description, parameters: { [key: string]: any }) => { + const strategy = { + name: 'flexibleRollout', + parameters, + constraints: [], + }; + const feature = { + name: `test-feature-create-${description.replaceAll(' ', '-')}`, + }; + const projectId = 'default'; + const defaultStickiness = `not-default-${description.replaceAll(' ', '-')}`; + const expectedStickiness = + parameters?.stickiness === '' + ? defaultStickiness + : (parameters?.stickiness ?? defaultStickiness); + const expectedParameters = { + ...parameters, // expect extra parameters to be preserved + groupId: parameters?.groupId ?? feature.name, + stickiness: expectedStickiness, + rollout: parameters?.rollout ?? '100', // default rollout + }; + await stores.projectStore.update({ + id: projectId, + name: 'stickiness-project-test', + defaultStickiness, + }); + const context = { + projectId, + featureName: feature.name, + environment: DEFAULT_ENV, + }; - await service.createFeatureToggle(projectId, feature, TEST_AUDIT_USER); - await service.createStrategy( - strategy, - { projectId, featureName: feature.name, environment: DEFAULT_ENV }, - TEST_AUDIT_USER, - ); + await service.createFeatureToggle(projectId, feature, TEST_AUDIT_USER); + const createdStrategy = await service.createStrategy( + strategy, + context, + TEST_AUDIT_USER, + ); - const featureDB = await service.getFeature({ featureName: feature.name }); + const featureDB = await service.getFeature({ + featureName: feature.name, + }); - expect(featureDB.environments[0]).toMatchObject({ - strategies: [{ parameters: { stickiness: 'default' } }], - }); -}); + expect( + featureDB.environments[0].strategies[0].parameters, + ).toStrictEqual(expectedParameters); + + // Verify that updating the strategy with same data is idempotent + await service.updateStrategy( + createdStrategy.id, + strategy, + context, + TEST_AUDIT_USER, + ); + const featureDBAfterUpdate = await service.getFeature({ + featureName: feature.name, + }); + + expect( + featureDBAfterUpdate.environments[0].strategies[0].parameters, + ).toStrictEqual(expectedParameters); + }, +); test('Should not allow to add flags to archived projects', async () => { const project = await stores.projectStore.create({ 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 feaf1db119..248c017e85 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 @@ -159,94 +159,24 @@ 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: 'flexibleRollout', - projectId: 'default', - environment: DEFAULT_ENV, - featureName: toggle.name, - constraints: [], - sortOrder: 15, - parameters: {}, - }); - expect(strategy.parameters).toEqual({ - rollout: '100', - groupId: toggle.name, - stickiness: 'default', - }); +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, }); - 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: 'flexibleRollout', - projectId: 'default', - environment: DEFAULT_ENV, - 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_ENV, - featureName: toggle.name, - constraints: [], - sortOrder: 15, - parameters: { - hostnames: 'myfantastichost', - }, - }); - expect(strategy.parameters).toEqual({ + const strategy = await featureStrategiesStore.createStrategyFeatureEnv({ + strategyName: 'applicationHostname', + projectId: 'default', + environment: DEFAULT_ENV, + featureName: toggle.name, + constraints: [], + sortOrder: 15, + parameters: { 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: 'flexibleRollout', - projectId: project.id, - environment: DEFAULT_ENV, - featureName: toggle.name, - constraints: [], - sortOrder: 15, - parameters: {}, - }); - expect(strategy.parameters.stickiness).toBe(defaultStickiness); + expect(strategy.parameters).toEqual({ + hostnames: 'myfantastichost', }); }); diff --git a/src/lib/features/feature-toggle/types/feature-toggle-strategies-store-type.ts b/src/lib/features/feature-toggle/types/feature-toggle-strategies-store-type.ts index 4e5c9fdba3..1887187c0c 100644 --- a/src/lib/features/feature-toggle/types/feature-toggle-strategies-store-type.ts +++ b/src/lib/features/feature-toggle/types/feature-toggle-strategies-store-type.ts @@ -124,4 +124,6 @@ export interface IFeatureStrategiesStore ): Promise; getCustomStrategiesInUseCount(): Promise; + + getDefaultStickiness(projectId: string): Promise; } diff --git a/src/lib/features/metrics/sizes/largest-resources-read-model.test.ts b/src/lib/features/metrics/sizes/largest-resources-read-model.test.ts index c57e366413..694c8b4ba6 100644 --- a/src/lib/features/metrics/sizes/largest-resources-read-model.test.ts +++ b/src/lib/features/metrics/sizes/largest-resources-read-model.test.ts @@ -79,7 +79,11 @@ test('can calculate resource size', async () => { await createFeature({ featureName: 'featureB', - parameters: {}, + parameters: { + groupId: 'featureB', + rollout: '100', + stickiness: 'default', + }, constraints: [], variants: [], }); diff --git a/src/lib/features/project/project-store-type.ts b/src/lib/features/project/project-store-type.ts index 02069714ec..e9bcb4b086 100644 --- a/src/lib/features/project/project-store-type.ts +++ b/src/lib/features/project/project-store-type.ts @@ -13,25 +13,6 @@ import type { import type { Store } from '../../types/stores/store.js'; import type { CreateFeatureStrategySchema } from '../../openapi/index.js'; -export interface IProjectInsert { - id: string; - name: string; - description?: string; - updatedAt?: Date; - changeRequestsEnabled?: boolean; - mode?: ProjectMode; - featureLimit?: number; - featureNaming?: IFeatureNaming; - linkTemplates?: IProjectLinkTemplate[]; -} - -export interface IProjectEnterpriseSettingsUpdate { - id: string; - mode?: ProjectMode; - featureNaming?: IFeatureNaming; - linkTemplates?: IProjectLinkTemplate[]; -} - export interface IProjectSettings { mode: ProjectMode; defaultStickiness: string; @@ -42,6 +23,22 @@ export interface IProjectSettings { linkTemplates?: IProjectLinkTemplate[]; } +export interface IProjectInsert extends Partial { + id: string; + name: string; + description?: string; + updatedAt?: Date; + changeRequestsEnabled?: boolean; + featureNaming?: IFeatureNaming; +} + +export interface IProjectEnterpriseSettingsUpdate { + id: string; + mode?: ProjectMode; + featureNaming?: IFeatureNaming; + linkTemplates?: IProjectLinkTemplate[]; +} + export interface IProjectHealthUpdate { id: string; health: number; diff --git a/src/lib/features/project/project-store.ts b/src/lib/features/project/project-store.ts index 7cf575e9be..572edb8ede 100644 --- a/src/lib/features/project/project-store.ts +++ b/src/lib/features/project/project-store.ts @@ -16,7 +16,6 @@ import type { IProjectHealthUpdate, IProjectInsert, IProjectQuery, - IProjectSettings, IProjectEnterpriseSettingsUpdate, IProjectStore, ProjectEnvironment, @@ -199,9 +198,7 @@ class ProjectStore implements IProjectStore { }); } - async create( - project: IProjectInsert & IProjectSettings, - ): Promise { + async create(project: IProjectInsert): Promise { const row = await this.db(TABLE) .insert({ ...this.fieldToRow(project), created_at: new Date() }) .returning('*');