refactor: always add values to constraints (#1448)

src/lib/schema/constraint-value-types.ts

@@ -2,6 +2,9 @@ import joi from 'joi';
 
 export const constraintNumberTypeSchema = joi.number();
 
-export const constraintStringTypeSchema = joi.array().items(joi.string());
+export const constraintStringTypeSchema = joi
+    .array()
+    .items(joi.string())
+    .min(1);
 
 export const constraintDateTypeSchema = joi.date();

src/lib/schema/feature-schema.test.ts

@@ -210,7 +210,7 @@ test('should not accept empty constraint values', () => {
     );
 });
 
-test('should not accept empty list of constraint values', () => {
+test('should accept empty list of constraint values', async () => {
     const toggle = {
         name: 'app.constraints.empty.value.list',
         type: 'release',
@@ -231,10 +231,10 @@ test('should not accept empty list of constraint values', () => {
         ],
     };
 
-    const { error } = featureSchema.validate(toggle);
+    const validated = await featureSchema.validateAsync(toggle);
-    expect(error.details[0].message).toEqual(
+    expect(validated.strategies.length).toEqual(1);
-        '"strategies[0].constraints[0].values" must contain at least 1 items',
+    expect(validated.strategies[0].constraints.length).toEqual(1);
-    );
+    expect(validated.strategies[0].constraints[0].values).toEqual([]);
 });
 
 test('Filter queries should accept a list of tag values', () => {
@@ -289,3 +289,18 @@ test('constraint schema should only allow specified operators', async () => {
         );
     }
 });
+
+test('constraint schema should add a values array by default', async () => {
+    const validated = await constraintSchema.validateAsync({
+        contextName: 'x',
+        operator: 'NUM_EQ',
+        value: 1,
+    });
+
+    expect(validated).toEqual({
+        contextName: 'x',
+        operator: 'NUM_EQ',
+        value: 1,
+        values: [],
+    });
+});

src/lib/schema/feature-schema.ts

@@ -10,7 +10,8 @@ export const nameSchema = joi
 export const constraintSchema = joi.object().keys({
     contextName: joi.string(),
     operator: joi.string().valid(...ALL_OPERATORS),
-    values: joi.array().items(joi.string().min(1).max(100)).min(1).optional(),
+    // Constraints must have a values array to support legacy SDKs.
+    values: joi.array().items(joi.string().min(1).max(100)).default([]),
     value: joi.optional(),
     caseInsensitive: joi.boolean().optional(),
     inverted: joi.boolean().optional(),

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

@@ -165,17 +165,19 @@ class FeatureToggleService {
         }
     }
 
-    async validateConstraints(constraints: IConstraint[]): Promise<void> {
+    async validateConstraints(
+        constraints: IConstraint[],
+    ): Promise<IConstraint[]> {
         const validations = constraints.map((constraint) => {
             return this.validateConstraint(constraint);
         });
 
-        await Promise.all(validations);
+        return Promise.all(validations);
     }
 
-    async validateConstraint(constraint: IConstraint): Promise<void> {
+    async validateConstraint(input: IConstraint): Promise<IConstraint> {
+        const constraint = await constraintSchema.validateAsync(input);
         const { operator } = constraint;
-        await constraintSchema.validateAsync(constraint);
         const contextDefinition = await this.contextFieldStore.get(
             constraint.contextName,
         );
@@ -218,6 +220,8 @@ class FeatureToggleService {
                 );
             }
         }
+
+        return constraint;
     }
 
     async patchFeature(
@@ -282,7 +286,9 @@ class FeatureToggleService {
         await this.validateFeatureContext(context);
 
         if (strategyConfig.constraints?.length > 0) {
-            await this.validateConstraints(strategyConfig.constraints);
+            strategyConfig.constraints = await this.validateConstraints(
+                strategyConfig.constraints,
+            );
         }
 
         try {
@@ -342,15 +348,17 @@ class FeatureToggleService {
         this.validateFeatureStrategyContext(existingStrategy, context);
 
         if (existingStrategy.id === id) {
+            if (updates.constraints?.length > 0) {
+                updates.constraints = await this.validateConstraints(
+                    updates.constraints,
+                );
+            }
+
             const strategy = await this.featureStrategiesStore.updateStrategy(
                 id,
                 updates,
             );
 
-            if (updates.constraints?.length > 0) {
-                await this.validateConstraints(updates.constraints);
-            }
-
             // Store event!
             const tags = await this.tagStore.getAllTagsForFeature(featureName);
             const data = this.featureStrategyToPublic(strategy);

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

@@ -14,6 +14,8 @@ import ApiUser from '../../../../../lib/types/api-user';
 import { ApiTokenType } from '../../../../../lib/types/models/api-token';
 import IncompatibleProjectError from '../../../../../lib/error/incompatible-project-error';
 import { IVariant, WeightType } from '../../../../../lib/types/model';
+import { v4 as uuidv4 } from 'uuid';
+import supertest from 'supertest';
 
 let app: IUnleashTest;
 let db: ITestDb;
@@ -2066,3 +2068,108 @@ test('Can create toggle with impression data on different project', async () =>
             expect(res.body.impressionData).toBe(false);
         });
 });
+
+test('should reject invalid constraint values for multi-valued constraints', async () => {
+    const project = await db.stores.projectStore.create({
+        id: uuidv4(),
+        name: uuidv4(),
+        description: '',
+    });
+
+    const toggle = await db.stores.featureToggleStore.create(project.id, {
+        name: uuidv4(),
+        impressionData: true,
+    });
+
+    const mockStrategy = (values: string[]) => ({
+        name: uuidv4(),
+        constraints: [{ contextName: 'userId', operator: 'IN', values }],
+    });
+
+    const featureStrategiesPath = `/api/admin/projects/${project.id}/features/${toggle.name}/environments/default/strategies`;
+
+    await app.request
+        .post(featureStrategiesPath)
+        .send(mockStrategy([]))
+        .expect(400);
+    await app.request
+        .post(featureStrategiesPath)
+        .send(mockStrategy(['']))
+        .expect(400);
+    const { body: strategy } = await app.request
+        .post(featureStrategiesPath)
+        .send(mockStrategy(['a']))
+        .expect(200);
+
+    await app.request
+        .put(`${featureStrategiesPath}/${strategy.id}`)
+        .send(mockStrategy([]))
+        .expect(400);
+    await app.request
+        .put(`${featureStrategiesPath}/${strategy.id}`)
+        .send(mockStrategy(['']))
+        .expect(400);
+    await app.request
+        .put(`${featureStrategiesPath}/${strategy.id}`)
+        .send(mockStrategy(['a']))
+        .expect(200);
+});
+
+test('should add default constraint values for single-valued constraints', async () => {
+    const project = await db.stores.projectStore.create({
+        id: uuidv4(),
+        name: uuidv4(),
+        description: '',
+    });
+
+    const toggle = await db.stores.featureToggleStore.create(project.id, {
+        name: uuidv4(),
+        impressionData: true,
+    });
+
+    const constraintValue = {
+        contextName: 'userId',
+        operator: 'NUM_EQ',
+        value: '1',
+    };
+
+    const constraintValues = {
+        contextName: 'userId',
+        operator: 'IN',
+        values: ['a', 'b', 'c'],
+    };
+
+    const mockStrategy = (constraint: unknown) => ({
+        name: uuidv4(),
+        constraints: [constraint],
+    });
+
+    const expectValues = (res: supertest.Response, values: unknown[]) => {
+        expect(res.body.constraints.length).toEqual(1);
+        expect(res.body.constraints[0].values).toEqual(values);
+    };
+
+    const featureStrategiesPath = `/api/admin/projects/${project.id}/features/${toggle.name}/environments/default/strategies`;
+
+    await app.request
+        .post(featureStrategiesPath)
+        .send(mockStrategy(constraintValue))
+        .expect(200)
+        .expect((res) => expectValues(res, []));
+    const { body: strategy } = await app.request
+        .post(featureStrategiesPath)
+        .send(mockStrategy(constraintValues))
+        .expect(200)
+        .expect((res) => expectValues(res, constraintValues.values));
+
+    await app.request
+        .put(`${featureStrategiesPath}/${strategy.id}`)
+        .send(mockStrategy(constraintValue))
+        .expect(200)
+        .expect((res) => expectValues(res, []));
+    await app.request
+        .put(`${featureStrategiesPath}/${strategy.id}`)
+        .send(mockStrategy(constraintValues))
+        .expect(200)
+        .expect((res) => expectValues(res, constraintValues.values));
+});