From 1f21770977821f531051d65908341a1924c3bcb3 Mon Sep 17 00:00:00 2001 From: andreas-unleash Date: Mon, 17 Jul 2023 17:12:59 +0300 Subject: [PATCH] Feat/feature environment strategy execution reorder (#4248) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When reordering strategies for a feature environment: - Adds stop when CR are enabled - Emits an event ## About the changes Closes # ### Important files ## Discussion points --------- Signed-off-by: andreas-unleash --- src/lib/db/event-store.ts | 6 +- src/lib/openapi/spec/event-schema.ts | 7 + .../admin-api/project/project-features.ts | 20 ++- src/lib/services/feature-toggle-service.ts | 120 +++++++++++++--- src/lib/types/events.ts | 36 ++++- .../api/admin/project/features.e2e.test.ts | 129 ++++++++++++++++++ 6 files changed, 294 insertions(+), 24 deletions(-) diff --git a/src/lib/db/event-store.ts b/src/lib/db/event-store.ts index 8adabcd61e..5619f36e3e 100644 --- a/src/lib/db/event-store.ts +++ b/src/lib/db/event-store.ts @@ -372,8 +372,10 @@ class EventStore implements IEventStore { return { type: e.type, created_by: e.createdBy, - data: e.data, - pre_data: e.preData, + data: Array.isArray(e.data) ? JSON.stringify(e.data) : e.data, + pre_data: Array.isArray(e.preData) + ? JSON.stringify(e.preData) + : e.preData, // @ts-expect-error workaround for json-array tags: JSON.stringify(e.tags), feature_name: e.featureName, diff --git a/src/lib/openapi/spec/event-schema.ts b/src/lib/openapi/spec/event-schema.ts index 9dd6cac18e..0a1288af35 100644 --- a/src/lib/openapi/spec/event-schema.ts +++ b/src/lib/openapi/spec/event-schema.ts @@ -12,27 +12,32 @@ const eventDataSchema = { description: 'Name of the feature toggle/strategy/environment that this event relates to', example: 'my.first.toggle', + nullable: true, }, description: { type: 'string', description: 'The description of the object this event relates to', example: 'Toggle description', + nullable: true, }, type: { type: 'string', description: 'If this event relates to a feature toggle, the type of feature toggle.', example: 'release', + nullable: true, }, project: { type: 'string', description: 'The project this event relates to', example: 'default', + nullable: true, }, stale: { description: 'Is the feature toggle this event relates to stale', type: 'boolean', example: true, + nullable: true, }, variants: { description: 'Variants configured for this toggle', @@ -40,6 +45,7 @@ const eventDataSchema = { items: { $ref: '#/components/schemas/variantSchema', }, + nullable: true, }, createdAt: { type: 'string', @@ -47,6 +53,7 @@ const eventDataSchema = { description: 'The time the event happened as a RFC 3339-conformant timestamp.', example: '2023-07-05T12:56:00.000Z', + nullable: true, }, lastSeenAt: { type: 'string', diff --git a/src/lib/routes/admin-api/project/project-features.ts b/src/lib/routes/admin-api/project/project-features.ts index 1cb9cd9973..d3b9327d15 100644 --- a/src/lib/routes/admin-api/project/project-features.ts +++ b/src/lib/routes/admin-api/project/project-features.ts @@ -985,7 +985,7 @@ export default class ProjectFeaturesController extends Controller { } async setStrategiesSortOrder( - req: Request< + req: IAuthRequest< FeatureStrategyParams, any, SetStrategySortOrderSchema, @@ -993,10 +993,20 @@ export default class ProjectFeaturesController extends Controller { >, res: Response, ): Promise { - const { featureName } = req.params; - await this.featureService.updateStrategiesSortOrder( - featureName, - req.body, + const { featureName, projectId, environment } = req.params; + const createdBy = extractUsername(req); + await this.startTransaction(async (tx) => + this.transactionalFeatureToggleService( + tx, + ).updateStrategiesSortOrder( + { + featureName, + environment, + projectId, + }, + req.body, + createdBy, + ), ); res.status(200).send(); diff --git a/src/lib/services/feature-toggle-service.ts b/src/lib/services/feature-toggle-service.ts index 925023e453..53ec3d0bf1 100644 --- a/src/lib/services/feature-toggle-service.ts +++ b/src/lib/services/feature-toggle-service.ts @@ -1,5 +1,6 @@ import { CREATE_FEATURE_STRATEGY, + StrategyIds, EnvironmentVariantEvent, FEATURE_UPDATED, FeatureArchivedEvent, @@ -40,6 +41,7 @@ import { Unsaved, WeightType, FEATURE_POTENTIALLY_STALE_UPDATED, + StrategiesOrderChangedEvent, } from '../types'; import { Logger } from '../logger'; import BadDataError from '../error/bad-data-error'; @@ -370,7 +372,7 @@ class FeatureToggleService { featureStrategy: IFeatureStrategy, segments: ISegment[] = [], ): Saved { - return { + const result: Saved = { id: featureStrategy.id, name: featureStrategy.strategyName, title: featureStrategy.title, @@ -379,17 +381,96 @@ class FeatureToggleService { parameters: featureStrategy.parameters, segments: segments.map((segment) => segment.id) ?? [], }; + + if (this.flagResolver.isEnabled('strategyVariant')) { + result.sortOrder = featureStrategy.sortOrder; + } + return result; } async updateStrategiesSortOrder( - featureName: string, + context: IFeatureStrategyContext, sortOrders: SetStrategySortOrderSchema, + createdBy: string, + user?: User, ): Promise> { - await Promise.all( - sortOrders.map(async ({ id, sortOrder }) => - this.featureStrategiesStore.updateSortOrder(id, sortOrder), - ), + await this.stopWhenChangeRequestsEnabled( + context.projectId, + context.environment, + user, ); + + return this.unprotectedUpdateStrategiesSortOrder( + context, + sortOrders, + createdBy, + ); + } + + async unprotectedUpdateStrategiesSortOrder( + context: IFeatureStrategyContext, + sortOrders: SetStrategySortOrderSchema, + createdBy: string, + ): Promise> { + const { featureName, environment, projectId: project } = context; + const existingOrder = ( + await this.getStrategiesForEnvironment( + project, + featureName, + environment, + ) + ) + .sort((strategy1, strategy2) => { + if ( + typeof strategy1.sortOrder === 'number' && + typeof strategy2.sortOrder === 'number' + ) { + return strategy1.sortOrder - strategy2.sortOrder; + } + return 0; + }) + .map((strategy) => strategy.id); + + const eventPreData: StrategyIds = { strategyIds: existingOrder }; + + await Promise.all( + sortOrders.map(async ({ id, sortOrder }) => { + await this.featureStrategiesStore.updateSortOrder( + id, + sortOrder, + ); + }), + ); + const newOrder = ( + await this.getStrategiesForEnvironment( + project, + featureName, + environment, + ) + ) + .sort((strategy1, strategy2) => { + if ( + typeof strategy1.sortOrder === 'number' && + typeof strategy2.sortOrder === 'number' + ) { + return strategy1.sortOrder - strategy2.sortOrder; + } + return 0; + }) + .map((strategy) => strategy.id); + + const eventData: StrategyIds = { strategyIds: newOrder }; + const tags = await this.tagStore.getAllTagsForFeature(featureName); + const event = new StrategiesOrderChangedEvent({ + featureName, + environment, + project, + createdBy, + preData: eventPreData, + data: eventData, + tags: tags, + }); + await this.eventStore.store(event); } async createStrategy( @@ -473,24 +554,31 @@ class FeatureToggleService { ); } - const tags = await this.tagStore.getAllTagsForFeature(featureName); const segments = await this.segmentService.getByStrategy( newFeatureStrategy.id, ); + const strategy = this.featureStrategyToPublic( newFeatureStrategy, segments, ); - await this.eventStore.store( - new FeatureStrategyAddEvent({ - project: projectId, + + if (this.flagResolver.isEnabled('strategyVariant')) { + const tags = await this.tagStore.getAllTagsForFeature( featureName, - createdBy, - environment, - data: strategy, - tags, - }), - ); + ); + + await this.eventStore.store( + new FeatureStrategyAddEvent({ + project: projectId, + featureName, + createdBy, + environment, + data: strategy, + tags, + }), + ); + } return strategy; } catch (e) { if (e.code === FOREIGN_KEY_VIOLATION) { diff --git a/src/lib/types/events.ts b/src/lib/types/events.ts index d0bb6452a2..359a329920 100644 --- a/src/lib/types/events.ts +++ b/src/lib/types/events.ts @@ -31,7 +31,7 @@ export const FEATURE_ENVIRONMENT_ENABLED = 'feature-environment-enabled' as const; export const FEATURE_ENVIRONMENT_DISABLED = 'feature-environment-disabled' as const; - +export const STRATEGY_ORDER_CHANGED = 'strategy-order-changed'; export const STRATEGY_CREATED = 'strategy-created' as const; export const STRATEGY_DELETED = 'strategy-deleted' as const; export const STRATEGY_DEPRECATED = 'strategy-deprecated' as const; @@ -142,6 +142,7 @@ export const IEventTypes = [ FEATURE_STRATEGY_UPDATE, FEATURE_STRATEGY_ADD, FEATURE_STRATEGY_REMOVE, + STRATEGY_ORDER_CHANGED, DROP_FEATURE_TAGS, FEATURE_UNTAGGED, FEATURE_STALE_ON, @@ -330,6 +331,39 @@ export class FeatureEnvironmentEvent extends BaseEvent { this.environment = p.environment; } } +export type StrategyIds = { strategyIds: string[] }; +export class StrategiesOrderChangedEvent extends BaseEvent { + readonly project: string; + + readonly featureName: string; + + readonly environment: string; + + readonly data: StrategyIds; + + readonly preData: StrategyIds; + + /** + * @param createdBy accepts a string for backward compatibility. Prefer using IUser for standardization + */ + constructor(p: { + project: string; + featureName: string; + environment: string; + createdBy: string | IUser; + data: StrategyIds; + preData: StrategyIds; + tags: ITag[]; + }) { + super(STRATEGY_ORDER_CHANGED, p.createdBy, p.tags); + const { project, featureName, environment, data, preData } = p; + this.project = project; + this.featureName = featureName; + this.environment = environment; + this.data = data; + this.preData = preData; + } +} export class FeatureVariantEvent extends BaseEvent { readonly project: string; diff --git a/src/test/e2e/api/admin/project/features.e2e.test.ts b/src/test/e2e/api/admin/project/features.e2e.test.ts index 6db66a3fc2..d028e0a92b 100644 --- a/src/test/e2e/api/admin/project/features.e2e.test.ts +++ b/src/test/e2e/api/admin/project/features.e2e.test.ts @@ -26,6 +26,7 @@ import { v4 as uuidv4 } from 'uuid'; import supertest from 'supertest'; import { randomId } from '../../../../../lib/util/random-id'; import { DEFAULT_PROJECT } from '../../../../../lib/types'; +import { FeatureStrategySchema, SetStrategySortOrderSchema } from 'lib/openapi'; let app: IUnleashTest; let db: ITestDb; @@ -3227,3 +3228,131 @@ test('Enabling a feature environment should add the default strategy when only d expect(res.body.strategies[1].disabled).toBeFalsy(); }); }); +test('Updating feature strategy sort-order should return strategies in correct order', async () => { + app = await setupAppWithCustomConfig( + db.stores, + { + experimental: { + flags: { + strictSchemaValidation: true, + strategyVariant: true, + }, + }, + }, + db.rawDatabase, + ); + + const envName = 'sort-order-within-environment-strategyVariant2'; + const featureName = 'feature.sort.order.event.list2'; + + await db.stores.environmentStore.create({ + name: envName, + type: 'test', + }); + + await app.request + .post('/api/admin/projects/default/environments') + .send({ + environment: envName, + }) + .expect(200); + + await app.request + .post('/api/admin/projects/default/features') + .send({ name: featureName }) + .expect(201); + + await addStrategies(featureName, envName); + const { body } = await app.request.get( + `/api/admin/projects/default/features/${featureName}/environments/${envName}/strategies`, + ); + + const strategies: FeatureStrategySchema[] = body; + let order = 1; + const sortOrders: SetStrategySortOrderSchema = []; + + strategies.forEach((strategy) => { + sortOrders.push({ id: strategy.id!, sortOrder: order++ }); + }); + + await app.request + .post( + `/api/admin/projects/default/features/${featureName}/environments/${envName}/strategies/set-sort-order`, + ) + .send(sortOrders) + .expect(200); + + await app.request + .get( + `/api/admin/projects/default/features/${featureName}/environments/${envName}`, + ) + .expect(200) + .expect((res) => { + expect(res.body.strategies.length).toBe(3); + expect(res.body.strategies[0].sortOrder).toBe(1); + expect(res.body.strategies[1].sortOrder).toBe(2); + expect(res.body.strategies[2].sortOrder).toBe(3); + }); +}); + +test('Updating feature strategy sort-order should trigger a an event', async () => { + app = await setupAppWithCustomConfig( + db.stores, + { + experimental: { + flags: { + strictSchemaValidation: false, + strategyVariant: true, + }, + }, + }, + db.rawDatabase, + ); + + const envName = 'sort-order-within-environment-strategyVariant'; + const featureName = 'feature.sort.order.event.list-strategyVariant'; + + await db.stores.environmentStore.create({ + name: envName, + type: 'test', + }); + + await app.request + .post('/api/admin/projects/default/environments') + .send({ + environment: envName, + }) + .expect(200); + + await app.request + .post('/api/admin/projects/default/features') + .send({ name: featureName }) + .expect(201); + + await addStrategies(featureName, envName); + const { body } = await app.request.get( + `/api/admin/projects/default/features/${featureName}/environments/${envName}/strategies`, + ); + + const strategies: FeatureStrategySchema[] = body; + let order = 1; + const sortOrders: SetStrategySortOrderSchema = []; + + strategies.forEach((strategy) => { + sortOrders.push({ id: strategy.id!, sortOrder: order++ }); + }); + + await app.request + .post( + `/api/admin/projects/default/features/${featureName}/environments/${envName}/strategies/set-sort-order`, + ) + .send(sortOrders) + .expect(200); + + await app.request + .get(`/api/admin/events`) + .expect(200) + .expect((res) => { + expect(res.body.events[0].type).toBe('strategy-order-changed'); + }); +});