fix: Should not remove variants when updating feature toggle metadata (#1234)

2025-02-09 00:18:00 +01:00 · 2022-01-06 10:23:52 +01:00 · 2022-01-06 10:23:52 +01:00 · 2b59a4219a
commit 2b59a4219a
parent e409e9e751
10 changed files with 148 additions and 82 deletions
--- 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) {
--- 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,
--- a/src/lib/services/feature-toggle-service.ts
+++ b/src/lib/services/feature-toggle-service.ts
@ -101,7 +101,7 @@ class FeatureToggleService {
        >,
        { getLogger }: Pick<IUnleashConfig, 'getLogger'>,
    ) {
-        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;
--- 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,
+                    });
+                }),
        );
    }

--- 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 {
--- 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';
--- 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`)
--- 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')
--- 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 () => {
--- 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(