Fixes create/update duplicate Switcher key - allow moving Switcher (#540)

Author: petruki

src/routers/config.js

@@ -129,9 +129,11 @@ router.delete('/config/:id', auth, [
 });
 
 router.patch('/config/:id', auth, [
-    check('id').isMongoId()
+    check('id').isMongoId(),
+    check('key').optional().isLength({ min: 3, max: 50 }),
+    check('group').optional().isMongoId(), 
 ], validate, verifyInputUpdateParameters([
-    'key', 'description', 'relay', 'disable_metrics'
+    'key', 'description', 'group', 'relay', 'disable_metrics'
 ]), async (req, res) => {
     try {
         const config = await Services.updateConfig(req.params.id, req.body, req.admin);

src/services/config.js

@@ -59,6 +59,8 @@ export async function populateAdmin(configs) {
 }
 
 export async function createConfig(args, admin) {
+    args.key = formatInput(args.key, { toUpper: true, autoUnderscore: true });
+
     // validates account plan permissions
     const group = await getGroupConfigById(args.group);
     await checkSwitcher(group);
@@ -70,7 +72,7 @@ export async function createConfig(args, admin) {
     });
 
     if (config) {
-        throw new BadRequestError(`Config ${args.key} already exists`);
+        throw new BadRequestError(`Config ${config.key} already exists`);
     }
 
     config = new Config({
@@ -79,8 +81,6 @@ export async function createConfig(args, admin) {
         owner: admin._id
     });
 
-    config.key = formatInput(config.key, { toUpper: true, autoUnderscore: true });
-
     // validates permissions
     config = await verifyOwnership(admin, config, group.domain, ActionTypes.CREATE, RouterTypes.CONFIG);
 
@@ -112,19 +112,12 @@ export async function updateConfig(id, args, admin) {
 
     // validates existing switcher key 
     if (args.key) {
-        const duplicatedKey = await getConfig({ 
-            key: args.key, 
-            group: config.group,
-            domain: config.domain 
-        });
-        
-        if (duplicatedKey) {
-            throw new BadRequestError(`Config ${args.key} already exists`);
-        }
+        args = await updateConfigKey(args, config);
+    }
 
-        // resets permission cache
-        permissionCache.permissionReset(config.domain, ActionTypes.ALL, RouterTypes.CONFIG, config.group);
-        permissionCache.permissionReset(config.domain, ActionTypes.ALL, RouterTypes.CONFIG, config.key);
+    // validates existing group
+    if (args.group) {
+        await getGroupConfigById(args.group);
     }
 
     // validates existing environment
@@ -139,13 +132,32 @@ export async function updateConfig(id, args, admin) {
     config.updatedBy = admin.email;
     const updates = Object.keys(args);
     updates.forEach((update) => config[update] = args[update]);
-    config.key = formatInput(config.key, { toUpper: true, autoUnderscore: true });
+    
     await config.save();
     updateDomainVersion(config.domain);
 
     return config;
 }
 
+async function updateConfigKey(args, config) {
+    args.key = formatInput(args.key, { toUpper: true, autoUnderscore: true });
+
+    const duplicatedKey = await getConfig({
+        key: args.key,
+        domain: config.domain
+    });
+
+    if (duplicatedKey) {
+        throw new BadRequestError(`Config ${duplicatedKey.key} already exists`);
+    }
+
+    // resets permission cache
+    permissionCache.permissionReset(config.domain, ActionTypes.ALL, RouterTypes.CONFIG, config.group);
+    permissionCache.permissionReset(config.domain, ActionTypes.ALL, RouterTypes.CONFIG, config.key);
+
+    return args;
+}
+
 export async function updateConfigRelay(id, args, admin) {
     let config = await getConfigById(id);
     isRelayValid(args);

tests/config.test.js

@@ -15,6 +15,7 @@ import {
     adminMasterAccountToken,
     domainId,
     groupConfigId,
+    groupConfigId2,
     configId1,
     config1Document,
     configId2,
@@ -52,7 +53,7 @@ describe('Testing configuration insertion', () => {
             .post('/config/create')
             .set('Authorization', `Bearer ${adminMasterAccountToken}`)
             .send({
-                key: 'NEW_CONFIG',
+                key: 'new_config', // case insensitive - it will be converted to uppercase
                 description: 'Description of my new Config',
                 group: groupConfigId
             }).expect(400);
@@ -277,7 +278,7 @@ describe('Testing update info', () => {
         const response = await request(app)
             .patch('/config/' + configId1)
             .set('Authorization', `Bearer ${adminMasterAccountToken}`)
-            .send({ key: 'NEWKEY' }).expect(400);
+            .send({ key: 'newkey' }).expect(400);
 
         expect(response.body.error).toEqual('Config NEWKEY already exists');
     });
@@ -324,6 +325,41 @@ describe('Testing update info', () => {
     });
 });
 
+describe('Testing moving Config from GroupConfig to another', () => {
+    beforeAll(setupDatabase);
+
+    test('CONFIG_SUITE - Should move Config to a different GroupConfig', async () => {
+        await request(app)
+            .patch('/config/' + configId1)
+            .set('Authorization', `Bearer ${adminMasterAccountToken}`)
+            .send({
+                group: groupConfigId2
+            }).expect(200);
+        
+        // DB validation - verify flag updated
+        const config = await Config.findById(configId1).lean().exec();
+        expect(config).not.toBeNull();
+        expect(config.group).toEqual(groupConfigId2);
+    });
+
+    test('CONFIG_SUITE - Should NOT move Config to a different GroupConfig - GroupConfig not found', async () => {
+        await request(app)
+            .patch('/config/' + configId1)
+            .set('Authorization', `Bearer ${adminMasterAccountToken}`)
+            .send({
+                group: new mongoose.Types.ObjectId()
+            }).expect(404);
+
+        await request(app)
+            .patch('/config/' + configId1)
+            .set('Authorization', `Bearer ${adminMasterAccountToken}`)
+            .send({
+                group: 'INVALID_VALUE_ID'
+            }).expect(422);
+    });
+
+});
+
 describe('Testing Environment status change', () => {
     beforeAll(setupDatabase);
 

tests/fixtures/db_api.js

@@ -122,6 +122,16 @@ export const groupConfigDocument = {
     domain: domainId
 };
 
+export const groupConfigId2 = new mongoose.Types.ObjectId();
+export const groupConfig2Document = {
+    _id: groupConfigId2,
+    name: 'Group Test 2',
+    description: 'Test Group 2',
+    activated: new Map().set(EnvType.DEFAULT, true),
+    owner: adminMasterAccountId,
+    domain: domainId
+};
+
 export const configId1 = new mongoose.Types.ObjectId();
 export const config1Document = {
     _id: configId1,
@@ -265,6 +275,7 @@ export const setupDatabase = async () => {
     await new Environment(environment3).save();
     await new Domain(domainDocument).save();
     await new GroupConfig(groupConfigDocument).save();
+    await new GroupConfig(groupConfig2Document).save();
     await new Config(config1Document).save();
     await new Config(config2Document).save();
     await new ConfigStrategy(configStrategyDocument).save();

tests/group-config.test.js

@@ -92,7 +92,7 @@ describe('Testing fetch Group info', () => {
             .set('Authorization', `Bearer ${adminMasterAccountToken}`)
             .send().expect(200);
 
-        expect(response.body.length).toEqual(1);
+        expect(response.body.length).toBeGreaterThan(0);
 
         expect(String(response.body[0]._id)).toEqual(String(groupConfigDocument._id));
         expect(response.body[0].name).toEqual(groupConfigDocument.name);
@@ -119,7 +119,7 @@ describe('Testing fetch Group info', () => {
             .set('Authorization', `Bearer ${adminMasterAccountToken}`)
             .send().expect(200);
 
-        expect(response.body.length).toEqual(2);
+        expect(response.body.length).toBeGreaterThan(1);
 
         // Query filter tests
         response = await request(app)
@@ -134,7 +134,7 @@ describe('Testing fetch Group info', () => {
             .set('Authorization', `Bearer ${adminMasterAccountToken}`)
             .send().expect(200);
 
-        expect(response.body.length).toEqual(2);
+        expect(response.body.length).toBeGreaterThan(1);
     });
 
     test('GROUP_SUITE - Should get Group Config information - only fields (name, activated.default)', async () => {