From 9eb19618bf8c76651d3c6bf5e9188f6b0399e33b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gast=C3=B3n=20Fournier?= Date: Wed, 30 Jul 2025 15:47:51 +0200 Subject: [PATCH] fix: stickiness should be preserved on strategy updates (#10439) ## About the changes When stickiness was set to empty or undefined while updating a strategy via API the stickiness would be lost. This adds a validation step after creating a strategy, that updating with the same data used to create the strategy yields the same result. The main change was lifting the default logic from the store layer to the service layer and adapting tests accordingly --- .../fakes/fake-feature-strategies-store.ts | 4 + .../feature-toggle/feature-toggle-service.ts | 152 ++++++++++-------- .../feature-toggle-strategies-store.ts | 36 +---- .../tests/feature-toggle-service.e2e.test.ts | 102 +++++++++--- ...eature-toggle-strategies-store.e2e.test.ts | 100 ++---------- .../feature-toggle-strategies-store-type.ts | 2 + .../largest-resources-read-model.test.ts | 6 +- .../features/project/project-store-type.ts | 35 ++-- src/lib/features/project/project-store.ts | 5 +- 9 files changed, 208 insertions(+), 234 deletions(-) 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('*');