From 1cc43c0a4a4656e093ace16283933278aca899fe Mon Sep 17 00:00:00 2001 From: olav Date: Thu, 17 Mar 2022 12:32:04 +0100 Subject: [PATCH] refactor: always add values to constraints (#1448) --- src/lib/schema/constraint-value-types.ts | 5 +- src/lib/schema/feature-schema.test.ts | 25 +++- src/lib/schema/feature-schema.ts | 3 +- src/lib/services/feature-toggle-service.ts | 26 +++-- .../api/admin/project/features.e2e.test.ts | 107 ++++++++++++++++++ 5 files changed, 150 insertions(+), 16 deletions(-) diff --git a/src/lib/schema/constraint-value-types.ts b/src/lib/schema/constraint-value-types.ts index 5861395169..1211218bb5 100644 --- a/src/lib/schema/constraint-value-types.ts +++ b/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(); diff --git a/src/lib/schema/feature-schema.test.ts b/src/lib/schema/feature-schema.test.ts index 700eb5bbc5..534f2722db 100644 --- a/src/lib/schema/feature-schema.test.ts +++ b/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); - expect(error.details[0].message).toEqual( - '"strategies[0].constraints[0].values" must contain at least 1 items', - ); + const validated = await featureSchema.validateAsync(toggle); + expect(validated.strategies.length).toEqual(1); + 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: [], + }); +}); diff --git a/src/lib/schema/feature-schema.ts b/src/lib/schema/feature-schema.ts index 93bd22f152..cf6692ed64 100644 --- a/src/lib/schema/feature-schema.ts +++ b/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(), diff --git a/src/lib/services/feature-toggle-service.ts b/src/lib/services/feature-toggle-service.ts index 80f4c8d984..515fd0a645 100644 --- a/src/lib/services/feature-toggle-service.ts +++ b/src/lib/services/feature-toggle-service.ts @@ -165,17 +165,19 @@ class FeatureToggleService { } } - async validateConstraints(constraints: IConstraint[]): Promise { + async validateConstraints( + constraints: IConstraint[], + ): Promise { const validations = constraints.map((constraint) => { return this.validateConstraint(constraint); }); - await Promise.all(validations); + return Promise.all(validations); } - async validateConstraint(constraint: IConstraint): Promise { + async validateConstraint(input: IConstraint): Promise { + 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); 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 ab7bb3fd70..95f56b810d 100644 --- a/src/test/e2e/api/admin/project/features.e2e.test.ts +++ b/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)); +});