Fix PATCH variants (old endpoint) when variants per environment are enabled (#2591)

## About the changes
This PR addresses some issues when working with variants after migrating
to variants per environment.

**Problem:** since PATCH
`/api/admin/projects/default/features/${featureName}/variants` does not
take into account `featureEnvironments`, when variantsPerEnvironment
gets enabled, this method will override the variants in other
environments (i.e. not doing a patch). This method has to be maintained
because of backward compatibility but it has to be adapted to deal with
variants per environment


https://linear.app/unleash/issue/2-476/when-using-patch-for-variants-without-environments-it-wipes-out

Co-authored-by: Nuno Góis <github@nunogois.com>

src/lib/db/feature-strategy-store.ts

@@ -303,7 +303,15 @@ class FeatureStrategiesStore implements IFeatureStrategiesStore {
                 if (withEnvironmentVariants) {
                     env.variants = variants;
                 }
-                acc.variants = variants;
+
+                // this code sets variants at the feature level (should be deprecated with variants per environment)
+                const currentVariants = new Map(
+                    acc.variants?.map((v) => [v.name, v]),
+                );
+                variants.forEach((variant) => {
+                    currentVariants.set(variant.name, variant);
+                });
+                acc.variants = Array.from(currentVariants.values());
 
                 env.enabled = r.enabled;
                 env.type = r.environment_type;

src/lib/routes/admin-api/project/variants.ts

@@ -143,6 +143,11 @@ export default class VariantsController extends Controller {
         });
     }
 
+    /**
+     * @deprecated - Variants should be fetched from featureService.getVariantsForEnv (since variants are now; since 4.18, connected to environments)
+     * @param req
+     * @param res
+     */
     async getVariants(
         req: Request<FeatureParams, any, any, any>,
         res: Response<FeatureVariantsSchema>,
@@ -158,7 +163,6 @@ export default class VariantsController extends Controller {
     ): Promise<void> {
         const { projectId, featureName } = req.params;
         const userName = extractUsername(req);
-
         const updatedFeature = await this.featureService.updateVariants(
             featureName,
             projectId,

src/lib/services/feature-toggle-service.ts

@@ -655,6 +655,7 @@ class FeatureToggleService {
 
     /**
      * GET /api/admin/projects/:project/features/:featureName/variants
+     * @deprecated - Variants should be fetched from FeatureEnvironmentStore (since variants are now; since 4.18, connected to environments)
      * @param featureName
      * @return The list of variants
      */
@@ -1250,9 +1251,22 @@ class FeatureToggleService {
         newVariants: Operation[],
         createdBy: string,
     ): Promise<FeatureToggle> {
-        const oldVariants = await this.getVariants(featureName);
-        const { newDocument } = applyPatch(oldVariants, newVariants);
-        return this.saveVariants(featureName, project, newDocument, createdBy);
+        const ft =
+            await this.featureStrategiesStore.getFeatureToggleWithVariantEnvs(
+                featureName,
+            );
+        const promises = ft.environments.map((env) =>
+            this.updateVariantsOnEnv(
+                featureName,
+                project,
+                env.name,
+                newVariants,
+                createdBy,
+            ).then((resultingVariants) => (env.variants = resultingVariants)),
+        );
+        await Promise.all(promises);
+        ft.variants = ft.environments[0].variants;
+        return ft;
     }
 
     async updateVariantsOnEnv(
@@ -1273,6 +1287,7 @@ class FeatureToggleService {
             environment,
             newDocument,
             createdBy,
+            oldVariants,
         );
     }
 
@@ -1312,15 +1327,18 @@ class FeatureToggleService {
         environment: string,
         newVariants: IVariant[],
         createdBy: string,
+        oldVariants?: IVariant[],
     ): Promise<IVariant[]> {
         await variantsArraySchema.validateAsync(newVariants);
         const fixedVariants = this.fixVariantWeights(newVariants);
-        const oldVariants = (
-            await this.featureEnvironmentStore.get({
-                featureName,
-                environment,
-            })
-        ).variants;
+        const theOldVariants: IVariant[] =
+            oldVariants ||
+            (
+                await this.featureEnvironmentStore.get({
+                    featureName,
+                    environment,
+                })
+            ).variants;
 
         await this.eventStore.store(
             new EnvironmentVariantEvent({
@@ -1328,7 +1346,7 @@ class FeatureToggleService {
                 environment,
                 project: projectId,
                 createdBy,
-                oldVariants,
+                oldVariants: theOldVariants,
                 newVariants: fixedVariants,
             }),
         );

src/test/e2e/api/admin/project/variants.e2e.test.ts

@@ -120,6 +120,104 @@ test('Can patch variants for a feature and get a response of new variant', async
         });
 });
 
+test('Can patch variants for a feature patches all environments independently', async () => {
+    const featureName = 'feature-to-patch';
+    const addedVariantName = 'patched-variant-name';
+    const variants = (name: string) => [
+        {
+            name,
+            stickiness: 'default',
+            weight: 1000,
+            weightType: WeightType.VARIABLE,
+        },
+    ];
+
+    await db.stores.environmentStore.create({
+        name: 'development',
+        type: 'development',
+    });
+    await db.stores.environmentStore.create({
+        name: 'production',
+        type: 'production',
+    });
+    await db.stores.featureToggleStore.create('default', {
+        name: featureName,
+    });
+    await db.stores.featureEnvironmentStore.addEnvironmentToFeature(
+        featureName,
+        'development',
+        true,
+    );
+    await db.stores.featureEnvironmentStore.addEnvironmentToFeature(
+        featureName,
+        'production',
+        true,
+    );
+    await db.stores.featureEnvironmentStore.addVariantsToFeatureEnvironment(
+        featureName,
+        'development',
+        variants('dev-variant'),
+    );
+    await db.stores.featureEnvironmentStore.addVariantsToFeatureEnvironment(
+        featureName,
+        'production',
+        variants('prod-variant'),
+    );
+
+    const patch = [
+        {
+            op: 'add',
+            path: '/1',
+            value: {
+                name: addedVariantName,
+                weightType: 'fix',
+                weight: 50,
+            },
+        },
+    ];
+
+    await app.request
+        .patch(`/api/admin/projects/default/features/${featureName}/variants`)
+        .send(patch)
+        .expect(200)
+        .expect((res) => {
+            expect(res.body.version).toBe(1);
+            expect(res.body.variants).toHaveLength(2);
+            // it picks variants from the first environment (sorted by name)
+            expect(res.body.variants[0].name).toBe('dev-variant');
+            expect(res.body.variants[1].name).toBe(addedVariantName);
+        });
+
+    await app.request
+        .get(
+            `/api/admin/projects/default/features/${featureName}?variantEnvironments=true`,
+        )
+        .expect((res) => {
+            const environments = res.body.environments;
+            expect(environments).toHaveLength(2);
+            const developmentVariants = environments.find(
+                (x) => x.name === 'development',
+            ).variants;
+            const productionVariants = environments.find(
+                (x) => x.name === 'production',
+            ).variants;
+            expect(developmentVariants).toHaveLength(2);
+            expect(productionVariants).toHaveLength(2);
+            expect(
+                developmentVariants.find((x) => x.name === addedVariantName),
+            ).toBeTruthy();
+            expect(
+                productionVariants.find((x) => x.name === addedVariantName),
+            ).toBeTruthy();
+            expect(
+                developmentVariants.find((x) => x.name === 'dev-variant'),
+            ).toBeTruthy();
+            expect(
+                productionVariants.find((x) => x.name === 'prod-variant'),
+            ).toBeTruthy();
+        });
+});
+
 test('Can add variant for a feature', async () => {
     const featureName = 'feature-variants-patch-add';
     const variantName = 'fancy-variant-patch';
@@ -315,6 +413,11 @@ test('Invalid variant in PATCH also throws 400 exception', async () => {
     await db.stores.featureToggleStore.create('default', {
         name: featureName,
     });
+    await db.stores.featureEnvironmentStore.addEnvironmentToFeature(
+        featureName,
+        'default',
+        true,
+    );
 
     const invalidPatch = `[{
         "op": "add",
@@ -439,6 +542,11 @@ test('PATCHING with no variable variants fails with 400', async () => {
     await db.stores.featureToggleStore.create('default', {
         name: featureName,
     });
+    await db.stores.featureEnvironmentStore.addEnvironmentToFeature(
+        featureName,
+        'default',
+        true,
+    );
 
     const newVariants: IVariant[] = [];