From 7401fceee3e747e13bbaa0b954f5da664ec09d64 Mon Sep 17 00:00:00 2001 From: Alan Quach Date: Fri, 15 Jan 2021 15:08:21 -0800 Subject: [PATCH] fix(core): Fix incorrect removal when merging serverGroups response (#8853) --- .../core/src/cluster/cluster.service.spec.ts | 132 ++++++++++++++++++ .../core/src/cluster/cluster.service.ts | 18 ++- 2 files changed, 148 insertions(+), 2 deletions(-) diff --git a/app/scripts/modules/core/src/cluster/cluster.service.spec.ts b/app/scripts/modules/core/src/cluster/cluster.service.spec.ts index 9f10d6ed72f..8edc73d9115 100644 --- a/app/scripts/modules/core/src/cluster/cluster.service.spec.ts +++ b/app/scripts/modules/core/src/cluster/cluster.service.spec.ts @@ -157,6 +157,138 @@ describe('Service: Cluster', function () { }); }); + describe('addServerGroupsToApplication merging to preserve referential equality', () => { + const asgFabricator = (x: string): IServerGroup => ({ + cluster: `cluster-${x}`, + name: `cluster-${x}-v001`, + account: 'test', + region: 'us-east-1', + category: 'serverGroup', + cloudProvider: 'titus', + type: 'titus', + instances: [], + instanceCounts: { total: 1, up: 1, down: 0, starting: 0, succeeded: 0, failed: 0, unknown: 0, outOfService: 0 }, + }); + + it('merges single new server group', () => { + // local data + application.serverGroups.data = ['mike', 'dustin', 'lucas', 'will'].map(asgFabricator); + // remote data + const serverGroups = ['mike', 'dustin', 'lucas', 'will', 'eleven'].map(asgFabricator); + + const merged = clusterService.addServerGroupsToApplication(application, serverGroups); + + expect(merged.find((sg) => sg.name === 'cluster-mike-v001')).toBeDefined( + 'Existing server group should be in merged output1' + JSON.stringify(merged, null, 4), + ); + expect(merged.find((sg) => sg.name === 'cluster-dustin-v001')).toBeDefined( + 'Existing server group should be in merged output2', + ); + expect(merged.find((sg) => sg.name === 'cluster-lucas-v001')).toBeDefined( + 'Existing server group should be in merged output3', + ); + expect(merged.find((sg) => sg.name === 'cluster-will-v001')).toBeDefined( + 'Existing server group should be in merged output4', + ); + + expect(merged.find((sg) => sg.name === 'cluster-eleven-v001')).toBeDefined( + 'New server group should be added in merged output', + ); + }); + + it('merges multiple new server groups', () => { + // local data + application.serverGroups.data = ['mike', 'dustin', 'lucas', 'will'].map(asgFabricator); + // remote data + const serverGroups = ['mike', 'dustin', 'lucas', 'will', 'eleven', 'hopper'].map(asgFabricator); + + const merged = clusterService.addServerGroupsToApplication(application, serverGroups); + + expect(merged.find((sg) => sg.name === 'cluster-mike-v001')).toBeDefined( + 'Existing server group should be in merged output', + ); + expect(merged.find((sg) => sg.name === 'cluster-dustin-v001')).toBeDefined( + 'Existing server group should be in merged output', + ); + expect(merged.find((sg) => sg.name === 'cluster-lucas-v001')).toBeDefined( + 'Existing server group should be in merged output', + ); + expect(merged.find((sg) => sg.name === 'cluster-will-v001')).toBeDefined( + 'Existing server group should be in merged output', + ); + + expect(merged.find((sg) => sg.name === 'cluster-eleven-v001')).toBeDefined( + 'New server group should be added in merged output', + ); + expect(merged.find((sg) => sg.name === 'cluster-hopper-v001')).toBeDefined( + 'New server group should be added in merged output', + ); + }); + + it('removes single server group that no longer exists', () => { + // local data + application.serverGroups.data = ['mike', 'dustin', 'lucas', 'will', 'eleven'].map(asgFabricator); + // remote data + const serverGroups = ['mike', 'dustin', 'lucas', 'eleven'].map(asgFabricator); + + const merged = clusterService.addServerGroupsToApplication(application, serverGroups); + + expect(merged.find((sg) => sg.name === 'cluster-mike-v001')).toBeDefined( + 'Remaining server group should be in merged output', + ); + expect(merged.find((sg) => sg.name === 'cluster-dustin-v001')).toBeDefined( + 'Remaining server group should be in merged output', + ); + expect(merged.find((sg) => sg.name === 'cluster-lucas-v001')).toBeDefined( + 'Remaining server group should be in merged output', + ); + expect(merged.find((sg) => sg.name === 'cluster-eleven-v001')).toBeDefined( + 'Remaining server group should be in merged output', + ); + + expect(merged.find((sg) => sg.name === 'cluster-will-v001')).toBeUndefined( + 'Removed server group should be absent in merged output', + ); + }); + + it('removes multiple server group that no longer exists', () => { + // This test is specifically meant to catch a shifting iterative splice + // If we started with [0, 1, 2, 3, 4, 5] and wanted toRemove [0, 1], + // Blindly forEach'ing and splicing like so: toRemove.forEach(i => data.splice(i, 1)) + // would result in the following at each step: + // data // [0, 1, 2, 3, 4, 5] + // data.splice(0,1); // [1, 2, 3, 4, 5] + // data.splice(1,1); // [1, 3, 4, 5] + // If toRemove is in ascending order, every splice will cause everything to shift left + // and every remaning index will no longer be correct (off by 1 for every iteration) + // Works perfect in descending order though. + + // local data + application.serverGroups.data = ['mike', 'dustin', 'lucas', 'will', 'eleven', 'hopper'].map(asgFabricator); + // remote data + const serverGroups = ['dustin', 'lucas', 'will', 'hopper'].map(asgFabricator); + + const merged = clusterService.addServerGroupsToApplication(application, serverGroups); + + expect(merged.find((sg) => sg.name === 'cluster-dustin-v001')).toBeDefined( + 'Remaining server group should be in merged output', + ); + expect(merged.find((sg) => sg.name === 'cluster-lucas-v001')).toBeDefined( + 'Remaining server group should be in merged output', + ); + expect(merged.find((sg) => sg.name === 'cluster-will-v001')).toBeDefined( + 'Remaining server group should be in merged output', + ); + + expect(merged.find((sg) => sg.name === 'cluster-mike-v001')).toBeUndefined( + 'Removed server group should be absent in merged output', + ); + expect(merged.find((sg) => sg.name === 'cluster-eleven-v001')).toBeUndefined( + 'Removed server group should be absent in merged output', + ); + }); + }); + describe('addTasksToServerGroups', () => { describe('rollback tasks', function () { it('attaches to source and target', function () { diff --git a/app/scripts/modules/core/src/cluster/cluster.service.ts b/app/scripts/modules/core/src/cluster/cluster.service.ts index 5590f621d26..c25375d05cf 100644 --- a/app/scripts/modules/core/src/cluster/cluster.service.ts +++ b/app/scripts/modules/core/src/cluster/cluster.service.ts @@ -110,8 +110,22 @@ export class ClusterService { } }); - // splice is necessary to preserve referential equality - toRemove.forEach((idx) => data.splice(idx, 1)); + // IMPORTANT!!! - toRemove must be forEach'ed in decending order, so that we splice backwards. + // For example, if we started with [0, 1, 2, 3, 4, 5] and wanted toRemove [0, 1], + // Blindly forEach'ing and splicing like so: toRemove.forEach(idx => data.splice(idx, 1)) + // would result in the following at each step: + // data // [0, 1, 2, 3, 4, 5] + // data.splice(0,1); // [1, 2, 3, 4, 5] + // data.splice(1,1); // [1, 3, 4, 5] wait, what?? + // If toRemove is in ascending order, every splice will cause everything to shift left + // and every remaning index will no longer be correct (off by 1 for every iteration) + // Works perfect in descending order though. + toRemove + // ensure indices are in descending order so splice can work properly + .sort() + .reverse() + // splice is necessary to preserve referential equality + .forEach((idx) => data.splice(idx, 1)); // add any new ones serverGroups.forEach((serverGroup) => {