diff --git a/src/lib/db/context-field-store.ts b/src/lib/db/context-field-store.ts index ab578bde3c..46be952224 100644 --- a/src/lib/db/context-field-store.ts +++ b/src/lib/db/context-field-store.ts @@ -16,12 +16,12 @@ const COLUMNS = [ ]; const TABLE = 'context_fields'; -const mapRow: (IContextRow) => IContextField = (row) => ({ +const mapRow: (object) => IContextField = (row) => ({ name: row.name, description: row.description, stickiness: row.stickiness, sortOrder: row.sort_order, - legalValues: row.legal_values ? row.legal_values.split(',') : undefined, + legalValues: row.legal_values || [], createdAt: row.created_at, }); @@ -52,7 +52,7 @@ class ContextFieldStore implements IContextFieldStore { description: data.description, stickiness: data.stickiness, sort_order: data.sortOrder, // eslint-disable-line - legal_values: data.legalValues ? data.legalValues.join(',') : undefined, // eslint-disable-line + legal_values: JSON.stringify(data.legalValues || []), }; } diff --git a/src/lib/routes/admin-api/context.test.ts b/src/lib/routes/admin-api/context.test.ts index bc49eeebf6..fcb8683b91 100644 --- a/src/lib/routes/admin-api/context.test.ts +++ b/src/lib/routes/admin-api/context.test.ts @@ -103,7 +103,7 @@ test('should create a context field with legal values', () => { .send({ name: 'page', description: 'Bla bla', - legalValues: ['blue', 'red'], + legalValues: [{ value: 'blue' }, { value: 'red' }], }) .set('Content-Type', 'application/json') .expect(201); @@ -137,7 +137,7 @@ test('should not create a context field with duplicate legal values', () => { .send({ name: 'page', description: 'Bla bla', - legalValues: ['blue', 'blue'], + legalValues: [{ value: 'blue' }, { value: 'blue' }], }) .set('Content-Type', 'application/json') .expect(400); @@ -151,7 +151,11 @@ test('should update a context field with new legal values', () => { .send({ name: 'environment', description: 'Used target application envrionments', - legalValues: ['local', 'stage', 'production'], + legalValues: [ + { value: 'local' }, + { value: 'stage' }, + { value: 'production' }, + ], }) .set('Content-Type', 'application/json') .expect(200); diff --git a/src/lib/services/context-schema.ts b/src/lib/services/context-schema.ts index d0c6126279..0819629498 100644 --- a/src/lib/services/context-schema.ts +++ b/src/lib/services/context-schema.ts @@ -3,6 +3,11 @@ import { nameType } from '../routes/util'; export const nameSchema = joi.object().keys({ name: nameType }); +const legalValueSchema = joi.object().keys({ + value: joi.string().min(1).max(100).required(), + description: joi.string().allow('').allow(null).optional(), +}); + export const contextSchema = joi .object() .keys({ @@ -11,9 +16,9 @@ export const contextSchema = joi legalValues: joi .array() .allow(null) - .unique() + .unique((a, b) => a.value === b.value) .optional() - .items(joi.string().max(100)), + .items(legalValueSchema), stickiness: joi.boolean().optional().default(false), }) .options({ allowUnknown: false, stripUnknown: true }); diff --git a/src/lib/types/stores/context-field-store.ts b/src/lib/types/stores/context-field-store.ts index 48ef5c29f3..bf7273608c 100644 --- a/src/lib/types/stores/context-field-store.ts +++ b/src/lib/types/stores/context-field-store.ts @@ -5,7 +5,12 @@ export interface IContextFieldDto { description: string; stickiness: boolean; sortOrder: number; - legalValues?: string[]; + legalValues?: ILegalValue[]; +} + +export interface ILegalValue { + value: string; + description?: string; } export interface IContextField extends IContextFieldDto { diff --git a/src/lib/util/validators/constraint-types.test.ts b/src/lib/util/validators/constraint-types.test.ts index 3a73824c6f..cee375af5a 100644 --- a/src/lib/util/validators/constraint-types.test.ts +++ b/src/lib/util/validators/constraint-types.test.ts @@ -1,4 +1,11 @@ import { validateSemver, validateLegalValues } from './constraint-types'; +import { ILegalValue } from '../../types/stores/context-field-store'; + +const legalValues: Readonly = [ + { value: '100' }, + { value: '200' }, + { value: '300' }, +]; test('semver validation should throw with bad format', () => { const badSemver = 'a.b.c'; @@ -54,7 +61,6 @@ test('semver validation should fail with leading v', () => { /* Legal values tests */ test('should fail validation if value does not exist in single legal value', () => { - const legalValues = ['100', '200', '300']; const value = '500'; expect.assertions(1); @@ -68,7 +74,6 @@ test('should fail validation if value does not exist in single legal value', () }); test('should pass validation if value exists in single legal value', () => { - const legalValues = ['100', '200', '300']; const value = '100'; expect.assertions(0); @@ -82,7 +87,6 @@ test('should pass validation if value exists in single legal value', () => { }); test('should fail validation if one of the values does not exist in multiple legal values', () => { - const legalValues = ['100', '200', '300']; const values = ['500', '100']; expect.assertions(1); @@ -96,7 +100,6 @@ test('should fail validation if one of the values does not exist in multiple leg }); test('should pass validation if all of the values exists in legal values', () => { - const legalValues = ['100', '200', '300']; const values = ['200', '100']; expect.assertions(0); diff --git a/src/lib/util/validators/constraint-types.ts b/src/lib/util/validators/constraint-types.ts index 588dd38e4b..c5953cafbe 100644 --- a/src/lib/util/validators/constraint-types.ts +++ b/src/lib/util/validators/constraint-types.ts @@ -6,6 +6,7 @@ import { constraintStringTypeSchema, } from '../../schema/constraint-value-types'; import BadDataError from '../../error/bad-data-error'; +import { ILegalValue } from '../../types/stores/context-field-store'; export const validateNumber = async (value: unknown): Promise => { await constraintNumberTypeSchema.validateAsync(value); @@ -31,18 +32,22 @@ export const validateDate = async (value: unknown): Promise => { }; export const validateLegalValues = ( - legalValues: string[], + legalValues: Readonly, match: string[] | string, ): void => { + const legalStrings = legalValues.map((legalValue) => { + return legalValue.value; + }); + if (Array.isArray(match)) { // Compare arrays to arrays - const valid = match.every((value) => legalValues.includes(value)); + const valid = match.every((value) => legalStrings.includes(value)); if (!valid) throw new BadDataError( `input values are not specified as a legal value on this context field`, ); } else { - const valid = legalValues.includes(match); + const valid = legalStrings.includes(match); if (!valid) throw new BadDataError( `${match} is not specified as a legal value on this context field`, diff --git a/src/migrations/20220411103724-add-legal-value-description.js b/src/migrations/20220411103724-add-legal-value-description.js new file mode 100644 index 0000000000..d4b092ca64 --- /dev/null +++ b/src/migrations/20220411103724-add-legal-value-description.js @@ -0,0 +1,56 @@ +'use strict'; + +exports.up = function (db, cb) { + db.runSql( + ` + -- 1. Convert '1,2,3'::text to "1,2,3"::json + ALTER TABLE context_fields + ALTER COLUMN legal_values SET DATA TYPE json USING to_json(legal_values); + + -- 2. Convert "1,2,3"::json to [{"value":"1"}, ...]::json. + WITH sub AS ( + SELECT a.name, b.json AS legal_values + FROM context_fields AS a + LEFT JOIN ( + SELECT name, json_agg(json_build_object('value', value)) AS json + FROM context_fields + CROSS JOIN unnest(string_to_array(legal_values #>> '{}', ',')) AS value + GROUP BY name + ) AS b ON a.name = b.name + ) + UPDATE context_fields SET legal_values = sub.legal_values + FROM sub + WHERE context_fields.name = sub.name; + `, + cb, + ); +}; + +exports.down = function (db, cb) { + db.runSql( + ` + -- 1. Revert []::json to null. + UPDATE context_fields SET legal_values = null + WHERE legal_values::jsonb = '[]'::jsonb; + + -- 2. Revert [{"value":"1"}, ...]::json to "1,2,3"::json. + WITH sub AS ( + SELECT name, string_agg(json->>'value', ',') AS legal_values + FROM context_fields + CROSS JOIN json_array_elements(legal_values) AS json + GROUP BY name + ) + UPDATE context_fields SET legal_values = to_json(sub.legal_values) + FROM sub + WHERE context_fields.name = sub.name; + + -- 3. Revert "1,2,3"::json to '"1,2,3"'::text + ALTER TABLE context_fields + ALTER COLUMN legal_values SET DATA TYPE text; + + -- 4. Revert '"1,2,3"'::text to '1,2,3'::text + UPDATE context_fields SET legal_values = legal_values::json #>> '{}'; + `, + cb, + ); +}; diff --git a/src/test/e2e/api/admin/context.e2e.test.ts b/src/test/e2e/api/admin/context.e2e.test.ts index b771b2fe7a..b8bd9d316f 100644 --- a/src/test/e2e/api/admin/context.e2e.test.ts +++ b/src/test/e2e/api/admin/context.e2e.test.ts @@ -50,29 +50,75 @@ test('should create context field', async () => { }); test('should create context field with legalValues', async () => { - expect.assertions(0); - return app.request + expect.assertions(3); + + const data = { + name: 'region', + description: 'A region', + legalValues: [ + { value: 'north' }, + { value: 'south', description: 'south-desc' }, + ], + }; + + await app.request .post('/api/admin/context') - .send({ - name: 'region', - description: 'A region', - legalValues: ['north', 'south'], - }) + .send(data) .set('Content-Type', 'application/json') .expect(201); + + const res = await app.request.get(`/api/admin/context/${data.name}`); + expect(res.body.name).toEqual(data.name); + expect(res.body.description).toEqual(data.description); + expect(res.body.legalValues).toEqual(data.legalValues); }); test('should update context field with legalValues', async () => { - expect.assertions(0); - return app.request + expect.assertions(3); + + const data = { + name: 'environment', + description: 'Updated description', + legalValues: [ + { value: 'dev', description: 'dev-desc' }, + { value: 'prod' }, + ], + }; + + await app.request .put('/api/admin/context/environment') - .send({ - name: 'environment', - description: 'Updated description', - legalValues: ['dev', 'prod'], - }) + .send(data) .set('Content-Type', 'application/json') .expect(200); + + const res = await app.request.get(`/api/admin/context/${data.name}`); + expect(res.body.name).toEqual(data.name); + expect(res.body.description).toEqual(data.description); + expect(res.body.legalValues).toEqual(data.legalValues); +}); + +test('should reject string legalValues', async () => { + await app.request + .put('/api/admin/context/environment') + .send({ name: 'environment', legalValues: ['a'] }) + .set('Content-Type', 'application/json') + .expect(400); +}); + +test('should reject empty legalValues', async () => { + await app.request + .put('/api/admin/context/environment') + .send({ name: 'environment', legalValues: [{ description: 'b' }] }) + .set('Content-Type', 'application/json') + .expect(400); +}); + +test('should reject legalValues without value', async () => { + await app.request + .put('/api/admin/context/environment') + .send({ name: 'environment', legalValues: [{ description: 'b' }] }) + .set('Content-Type', 'application/json') + .expect(400); }); test('should create context field with stickiness', async () => {