Mirrors/unleash.unleash
1
0
Fork 0
mirror of https://github.com/Unleash/unleash.git synced 2025-08-23 13:46:45 +02:00
Code Issues Projects Releases Wiki Activity

fix: stickiness should be preserved on strategy updates (#10439)

Browse Source 
## 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
This commit is contained in:
Gastón Fournier 2025-07-30 15:47:51 +02:00 committed by GitHub
parent 629624fd1c
commit 9eb19618bf
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
9 changed files with 208 additions and 234 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

4
src/lib/features/feature-toggle/fakes/fake-feature-strategies-store.ts
View File

@ -343,4 +343,8 @@ export default class FakeFeatureStrategiesStore
getCustomStrategiesInUseCount(): Promise<number> {
return Promise.resolve(3);
}
getDefaultStickiness(_projectId: string): Promise<string> {
return Promise.resolve('default');
}
}

152
src/lib/features/feature-toggle/feature-toggle-service.ts
View File

@ -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<IStrategyConfig>,
existing?: IFeatureStrategy,
): Promise<
{ strategyName: string } & Pick<
Partial<IFeatureStrategy>,
| '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<IStrategyConfig>,
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)) {

36
src/lib/features/feature-toggle/feature-toggle-strategies-store.ts
View File

@ -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<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 === '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<IFeatureStrategy, 'id' | 'createdAt'>,
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<string> {
async getDefaultStickiness(projectName: string): Promise<string> {
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,

102
src/lib/features/feature-toggle/tests/feature-toggle-service.e2e.test.ts
View File

@ -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({

100
src/lib/features/feature-toggle/tests/feature-toggle-strategies-store.e2e.test.ts
View File

@ -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',
});
});

2
src/lib/features/feature-toggle/types/feature-toggle-strategies-store-type.ts
View File

@ -124,4 +124,6 @@ export interface IFeatureStrategiesStore
): Promise<IFeatureStrategy[]>;
getCustomStrategiesInUseCount(): Promise<number>;
getDefaultStickiness(projectId: string): Promise<string>;
}

6
src/lib/features/metrics/sizes/largest-resources-read-model.test.ts
View File

@ -79,7 +79,11 @@ test('can calculate resource size', async () => {
await createFeature({
featureName: 'featureB',
parameters: {},
parameters: {
groupId: 'featureB',
rollout: '100',
stickiness: 'default',
},
constraints: [],
variants: [],
});

35
src/lib/features/project/project-store-type.ts
View File

@ -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<IProjectSettings> {
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;

5
src/lib/features/project/project-store.ts
View File

@ -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<IProject> {
async create(project: IProjectInsert): Promise<IProject> {
const row = await this.db(TABLE)
.insert({ ...this.fieldToRow(project), created_at: new Date() })
.returning('*');