From 2b59a4219a049e87984a99e1dde25d402f74a064 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ivar=20Conradi=20=C3=98sthus?= Date: Thu, 6 Jan 2022 10:23:52 +0100 Subject: [PATCH] fix: Should not remove variants when updating feature toggle metadata (#1234) --- src/lib/db/feature-toggle-store.ts | 9 +-- src/lib/routes/admin-api/feature.ts | 24 ++++--- src/lib/services/feature-toggle-service.ts | 2 +- src/lib/services/state-service.ts | 25 ++++--- src/lib/types/model.ts | 2 +- .../api/admin/project/features.e2e.test.ts | 40 +++++++++++ .../api/admin/project/variants.e2e.test.ts | 67 ++++++++++++------- src/test/e2e/api/admin/state.e2e.test.ts | 27 ++++---- src/test/e2e/api/client/feature.e2e.test.ts | 33 +++++---- .../e2e/services/environment-service.test.ts | 1 - 10 files changed, 148 insertions(+), 82 deletions(-) diff --git a/src/lib/db/feature-toggle-store.ts b/src/lib/db/feature-toggle-store.ts index d8d18b5000..4331be1258 100644 --- a/src/lib/db/feature-toggle-store.ts +++ b/src/lib/db/feature-toggle-store.ts @@ -23,7 +23,7 @@ export interface FeaturesTable { description: string; type: string; stale: boolean; - variants: string; + variants?: string; project: string; last_seen_at?: Date; created_at?: Date; @@ -187,13 +187,6 @@ export default class FeatureToggleStore implements IFeatureToggleStore { project, archived: data.archived || false, stale: data.stale, - variants: data.variants - ? JSON.stringify( - data.variants.sort((a, b) => - a.name.localeCompare(b.name), - ), - ) - : JSON.stringify([]), created_at: data.createdAt, }; if (!row.created_at) { diff --git a/src/lib/routes/admin-api/feature.ts b/src/lib/routes/admin-api/feature.ts index 8488996d44..afe86a78ee 100644 --- a/src/lib/routes/admin-api/feature.ts +++ b/src/lib/routes/admin-api/feature.ts @@ -141,9 +141,9 @@ class FeatureController extends Controller { const toggle = req.body; const validatedToggle = await featureSchema.validateAsync(toggle); - const { enabled } = validatedToggle; + const { enabled, project, name, variants = [] } = validatedToggle; const createdFeature = await this.service.createFeatureToggle( - validatedToggle.project, + project, validatedToggle, userName, true, @@ -153,8 +153,8 @@ class FeatureController extends Controller { this.service.createStrategy( s, { - projectId: createdFeature.project, - featureName: createdFeature.name, + projectId: project, + featureName: name, environment: DEFAULT_ENV, }, userName, @@ -162,15 +162,17 @@ class FeatureController extends Controller { ), ); await this.service.updateEnabled( - createdFeature.project, - createdFeature.name, + project, + name, DEFAULT_ENV, enabled, userName, ); + await this.service.saveVariants(name, project, variants, userName); res.status(201).json({ ...createdFeature, + variants, enabled, strategies, }); @@ -183,7 +185,7 @@ class FeatureController extends Controller { updatedFeature.name = featureName; - const projectId = await this.service.getProjectId(updatedFeature.name); + const projectId = await this.service.getProjectId(featureName); const value = await featureSchema.validateAsync(updatedFeature); await this.service.updateFeatureToggle(projectId, value, userName); @@ -203,11 +205,17 @@ class FeatureController extends Controller { } await this.service.updateEnabled( projectId, - updatedFeature.name, + featureName, DEFAULT_ENV, updatedFeature.enabled, userName, ); + await this.service.saveVariants( + featureName, + projectId, + value.variants || [], + userName, + ); const feature = await this.service.storeFeatureUpdatedEventLegacy( featureName, diff --git a/src/lib/services/feature-toggle-service.ts b/src/lib/services/feature-toggle-service.ts index eaf844c148..b7baef6253 100644 --- a/src/lib/services/feature-toggle-service.ts +++ b/src/lib/services/feature-toggle-service.ts @@ -101,7 +101,7 @@ class FeatureToggleService { >, { getLogger }: Pick, ) { - this.logger = getLogger('services/feature-toggle-service-v2.ts'); + this.logger = getLogger('services/feature-toggle-service.ts'); this.featureStrategiesStore = featureStrategiesStore; this.featureToggleStore = featureToggleStore; this.featureToggleClientStore = featureToggleClientStore; diff --git a/src/lib/services/state-service.ts b/src/lib/services/state-service.ts index e697fc04ef..600dbd9e98 100644 --- a/src/lib/services/state-service.ts +++ b/src/lib/services/state-service.ts @@ -324,17 +324,20 @@ export default class StateService { features .filter(filterExisting(keepExisting, oldToggles)) .filter(filterEqual(oldToggles)) - .map((feature) => - this.toggleStore - .create(feature.project, feature) - .then(() => { - this.eventStore.store({ - type: FEATURE_IMPORT, - createdBy: userName, - data: feature, - }); - }), - ), + .map(async (feature) => { + const { name, project, variants = [] } = feature; + await this.toggleStore.create(feature.project, feature); + await this.toggleStore.saveVariants( + project, + name, + variants, + ); + await this.eventStore.store({ + type: FEATURE_IMPORT, + createdBy: userName, + data: feature, + }); + }), ); } diff --git a/src/lib/types/model.ts b/src/lib/types/model.ts index f83f82866a..b80ffd80ce 100644 --- a/src/lib/types/model.ts +++ b/src/lib/types/model.ts @@ -37,7 +37,6 @@ export interface FeatureToggleDTO { type?: string; stale?: boolean; archived?: boolean; - variants?: IVariant[]; createdAt?: Date; } @@ -45,6 +44,7 @@ export interface FeatureToggle extends FeatureToggleDTO { project: string; lastSeenAt?: Date; createdAt?: Date; + variants?: IVariant[]; } export interface IFeatureToggleClient { 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 edec135748..d186982824 100644 --- a/src/test/e2e/api/admin/project/features.e2e.test.ts +++ b/src/test/e2e/api/admin/project/features.e2e.test.ts @@ -537,6 +537,46 @@ test('Should patch feature toggle', async () => { expect(updateForOurToggle.data.type).toBe('kill-switch'); }); +test('Should patch feature toggle and not remove variants', async () => { + const url = '/api/admin/projects/default/features'; + const name = 'new.toggle.variants'; + await app.request + .post(url) + .send({ name, description: 'some', type: 'release' }) + .expect(201); + await app.request + .put(`${url}/${name}/variants`) + .send([ + { + name: 'variant1', + weightType: 'variable', + weight: 500, + stickiness: 'default', + }, + { + name: 'variant2', + weightType: 'variable', + weight: 500, + stickiness: 'default', + }, + ]) + .expect(200); + await app.request + .patch(`${url}/${name}`) + .send([ + { op: 'replace', path: '/description', value: 'New desc' }, + { op: 'replace', path: '/type', value: 'kill-switch' }, + ]) + .expect(200); + + const { body: toggle } = await app.request.get(`${url}/${name}`); + + expect(toggle.name).toBe(name); + expect(toggle.description).toBe('New desc'); + expect(toggle.type).toBe('kill-switch'); + expect(toggle.variants).toHaveLength(2); +}); + test('Patching feature toggles to stale should trigger FEATURE_STALE_ON event', async () => { const url = '/api/admin/projects/default/features'; const name = 'toggle.stale.on.patch'; diff --git a/src/test/e2e/api/admin/project/variants.e2e.test.ts b/src/test/e2e/api/admin/project/variants.e2e.test.ts index d19b98ad16..a741832502 100644 --- a/src/test/e2e/api/admin/project/variants.e2e.test.ts +++ b/src/test/e2e/api/admin/project/variants.e2e.test.ts @@ -20,17 +20,15 @@ afterAll(async () => { test('Can get variants for a feature', async () => { const featureName = 'feature-variants'; const variantName = 'fancy-variant'; - await db.stores.featureToggleStore.create('default', { - name: featureName, - variants: [ - { - name: variantName, - stickiness: 'default', - weight: 1000, - weightType: WeightType.VARIABLE, - }, - ], - }); + await db.stores.featureToggleStore.create('default', { name: featureName }); + await db.stores.featureToggleStore.saveVariants('default', featureName, [ + { + name: variantName, + stickiness: 'default', + weight: 1000, + weightType: WeightType.VARIABLE, + }, + ]); await app.request .get(`/api/admin/projects/default/features/${featureName}/variants`) .expect(200) @@ -90,8 +88,12 @@ test('Can patch variants for a feature and get a response of new variant', async await db.stores.featureToggleStore.create('default', { name: featureName, - variants, }); + await db.stores.featureToggleStore.saveVariants( + 'default', + featureName, + variants, + ); const observer = jsonpatch.observe(variants); variants[0].name = expectedVariantName; @@ -123,8 +125,12 @@ test('Can add variant for a feature', async () => { await db.stores.featureToggleStore.create('default', { name: featureName, - variants, }); + await db.stores.featureToggleStore.saveVariants( + 'default', + featureName, + variants, + ); const observer = jsonpatch.observe(variants); variants.push({ @@ -167,8 +173,12 @@ test('Can remove variant for a feature', async () => { await db.stores.featureToggleStore.create('default', { name: featureName, - variants, }); + await db.stores.featureToggleStore.saveVariants( + 'default', + featureName, + variants, + ); const observer = jsonpatch.observe(variants); variants.pop(); @@ -200,8 +210,12 @@ test('PUT overwrites current variant on feature', async () => { ]; await db.stores.featureToggleStore.create('default', { name: featureName, - variants, }); + await db.stores.featureToggleStore.saveVariants( + 'default', + featureName, + variants, + ); const newVariants: IVariant[] = [ { @@ -646,18 +660,24 @@ test('If sum of fixed variant weight equals 1000 variable variants gets weight 0 test('PATCH endpoint validates uniqueness of variant names', async () => { const featureName = 'variants-uniqueness-names'; + const variants = [ + { + name: 'variant1', + weight: 1000, + weightType: WeightType.VARIABLE, + stickiness: 'default', + }, + ]; await db.stores.featureToggleStore.create('default', { name: featureName, - variants: [ - { - name: 'variant1', - weight: 1000, - weightType: WeightType.VARIABLE, - stickiness: 'default', - }, - ], }); + await db.stores.featureToggleStore.saveVariants( + 'default', + featureName, + variants, + ); + const newVariants: IVariant[] = []; const observer = jsonpatch.observe(newVariants); @@ -689,7 +709,6 @@ test('PUT endpoint validates uniqueness of variant names', async () => { const featureName = 'variants-put-uniqueness-names'; await db.stores.featureToggleStore.create('default', { name: featureName, - variants: [], }); await app.request .put(`/api/admin/projects/default/features/${featureName}/variants`) diff --git a/src/test/e2e/api/admin/state.e2e.test.ts b/src/test/e2e/api/admin/state.e2e.test.ts index b922f903c7..e2f4ffc51f 100644 --- a/src/test/e2e/api/admin/state.e2e.test.ts +++ b/src/test/e2e/api/admin/state.e2e.test.ts @@ -32,8 +32,6 @@ test('exports strategies and features as json by default', async () => { }); test('exports strategies and features as yaml', async () => { - expect.assertions(0); - return app.request .get('/api/admin/state/export?format=yaml') .expect('Content-Type', /yaml/) @@ -41,8 +39,6 @@ test('exports strategies and features as yaml', async () => { }); test('exports only features as yaml', async () => { - expect.assertions(0); - return app.request .get('/api/admin/state/export?format=yaml&featureToggles=1') .expect('Content-Type', /yaml/) @@ -50,8 +46,6 @@ test('exports only features as yaml', async () => { }); test('exports strategies and features as attachment', async () => { - expect.assertions(0); - return app.request .get('/api/admin/state/export?download=1') .expect('Content-Type', /json/) @@ -60,17 +54,26 @@ test('exports strategies and features as attachment', async () => { }); test('imports strategies and features', async () => { - expect.assertions(0); - return app.request .post('/api/admin/state/import') .send(importData) .expect(202); }); -test('does not not accept gibberish', async () => { - expect.assertions(0); +test('imports features with variants', async () => { + await app.request + .post('/api/admin/state/import') + .send(importData) + .expect(202); + const { body } = await app.request.get( + '/api/admin/projects/default/features/feature.with.variants', + ); + + expect(body.variants).toHaveLength(2); +}); + +test('does not not accept gibberish', async () => { return app.request .post('/api/admin/state/import') .send({ features: 'nonsense' }) @@ -78,8 +81,6 @@ test('does not not accept gibberish', async () => { }); test('imports strategies and features from json file', async () => { - expect.assertions(0); - return app.request .post('/api/admin/state/import') .attach('file', 'src/test/examples/import.json') @@ -87,8 +88,6 @@ test('imports strategies and features from json file', async () => { }); test('imports strategies and features from yaml file', async () => { - expect.assertions(0); - return app.request .post('/api/admin/state/import') .attach('file', 'src/test/examples/import.yml') diff --git a/src/test/e2e/api/client/feature.e2e.test.ts b/src/test/e2e/api/client/feature.e2e.test.ts index de21d581ee..d1ffe22886 100644 --- a/src/test/e2e/api/client/feature.e2e.test.ts +++ b/src/test/e2e/api/client/feature.e2e.test.ts @@ -77,23 +77,28 @@ beforeAll(async () => { { name: 'feature.with.variants', description: 'A feature toggle with variants', - variants: [ - { - name: 'control', - weight: 50, - weightType: 'fix', - stickiness: 'default', - }, - { - name: 'new', - weight: 50, - weightType: 'fix', - stickiness: 'default', - }, - ], }, 'test', ); + await app.services.featureToggleServiceV2.saveVariants( + 'feature.with.variants', + 'default', + [ + { + name: 'control', + weight: 50, + weightType: 'fix', + stickiness: 'default', + }, + { + name: 'new', + weight: 50, + weightType: 'variable', + stickiness: 'default', + }, + ], + 'ivar', + ); }); afterAll(async () => { diff --git a/src/test/e2e/services/environment-service.test.ts b/src/test/e2e/services/environment-service.test.ts index aab32d1184..8cb953b733 100644 --- a/src/test/e2e/services/environment-service.test.ts +++ b/src/test/e2e/services/environment-service.test.ts @@ -49,7 +49,6 @@ test('Can connect environment to project', async () => { type: 'release', description: '', stale: false, - variants: [], }); await service.addEnvironmentToProject('test-connection', 'default'); const overview = await stores.featureStrategiesStore.getFeatureOverview(