From 7456ae75f39bf6e0ce9af195f947aee98d357ede Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gast=C3=B3n=20Fournier?= Date: Wed, 30 Jul 2025 11:21:56 +0200 Subject: [PATCH] fix: stickiness should be preserved on strategy updates --- .../fakes/fake-feature-strategies-store.ts | 4 + .../feature-toggle/feature-toggle-service.ts | 129 +++++++++--------- .../feature-toggle-strategies-store.ts | 8 +- .../tests/feature-toggle-service.e2e.test.ts | 101 ++++++++++---- .../feature-toggle-strategies-store-type.ts | 2 + .../features/project/project-store-type.ts | 35 +++-- 6 files changed, 168 insertions(+), 111 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..ac69c3b948 100644 --- a/src/lib/features/feature-toggle/feature-toggle-service.ts +++ b/src/lib/features/feature-toggle/feature-toggle-service.ts @@ -370,16 +370,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 +670,59 @@ export class FeatureToggleService { ); } + async standardizeStrategyConfig( + projectId: 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); + } + + if ( + parameters && + (!('stickiness' in parameters) || + ('stickiness' in parameters && parameters.stickiness === '')) + ) { + parameters.stickiness = + existing?.parameters?.stickiness || + (await this.featureStrategiesStore.getDefaultStickiness( + projectId, + )); + } + + 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 +737,10 @@ 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, + strategyConfig, + ); await this.validateStrategyLimit({ featureName, @@ -732,13 +751,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 +880,14 @@ 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, + { ...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..91073690cf 100644 --- a/src/lib/features/feature-toggle/feature-toggle-strategies-store.ts +++ b/src/lib/features/feature-toggle/feature-toggle-strategies-store.ts @@ -248,7 +248,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,9 +264,9 @@ class FeatureStrategiesStore implements IFeatureStrategiesStore { strategyConfig.featureName, strategyConfig.environment, )); - const stickiness = await this.getDefaultStickiness( - strategyConfig.projectId, - ); + const stickiness = + strategyConfig.parameters?.stickiness ?? + (await this.getDefaultStickiness(strategyConfig.projectId)); strategyConfig.parameters = parametersWithDefaults( strategyConfig, stickiness, 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..0e9c118523 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,11 @@ 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', + // make sure stickiness is preserved + stickiness: createdConfig.parameters?.stickiness, + }); }); test('Should be able to get strategy by id', async () => { @@ -758,34 +762,79 @@ 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], +])( + 'Should set project default stickiness when creating a flexibleRollout strategy with %s', + async (description, parameters) => { + const strategy = { + name: 'flexibleRollout', + parameters, + constraints: [], + }; + const feature = { + name: `test-feature-create-${description.replaceAll(' ', '-')}`, + }; + const projectId = 'default'; + const defaultStickiness = `not-default-${description.replaceAll(' ', '-')}`; + const project = 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]).toMatchObject({ + strategies: [ + { + parameters: { + ...parameters, + stickiness: defaultStickiness, + }, + }, + ], + }); + + // 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]).toMatchObject({ + strategies: [ + { + parameters: { + ...parameters, + stickiness: defaultStickiness, + }, + }, + ], + }); + }, +); 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/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/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;