fix(ec2): explorer assumes names are unique (#3640)

Problem:
In the explorer, assuming the names are unique has the following consequences:

it is used as a key in the map connecting parent -> child nodes.
it is displayed by name, meaning each instance with same name appears identical.

Solution:
Remove this assumption, and use the instanceIds to key the child nodes.
add instance Ids following the names in explorer.

Author: Hweinstock

src/ec2/explorer/ec2InstanceNode.ts

@@ -7,7 +7,6 @@ import { getNameOfInstance } from '../../shared/clients/ec2Client'
 import { AWSResourceNode } from '../../shared/treeview/nodes/awsResourceNode'
 import { AWSTreeNodeBase } from '../../shared/treeview/nodes/awsTreeNodeBase'
 import { Ec2Instance } from '../../shared/clients/ec2Client'
-import { build } from '@aws-sdk/util-arn-parser'
 import globals from '../../shared/extensionGlobals'
 import { Ec2Selection } from '../utils'
 
@@ -24,8 +23,8 @@ export class Ec2InstanceNode extends AWSTreeNodeBase implements AWSResourceNode
 
     public update(newInstance: Ec2Instance) {
         this.setInstance(newInstance)
-        this.label = this.name
-        this.tooltip = this.InstanceId
+        this.label = `${this.name} (${this.InstanceId})`
+        this.tooltip = `${this.name}\n${this.InstanceId}\n${this.arn}`
     }
 
     public setInstance(newInstance: Ec2Instance) {
@@ -40,20 +39,16 @@ export class Ec2InstanceNode extends AWSTreeNodeBase implements AWSResourceNode
     }
 
     public get name(): string {
-        return getNameOfInstance(this.instance) ?? `${this.InstanceId} (no name)`
+        return getNameOfInstance(this.instance) ?? `(no name)`
     }
 
     public get InstanceId(): string {
         return this.instance.InstanceId!
     }
 
     public get arn(): string {
-        return build({
-            partition: this.partitionId,
-            service: 'ec2',
-            region: this.regionCode,
-            accountId: globals.awsContext.getCredentialAccountId()!,
-            resource: 'instance',
-        })
+        return `arn:${this.partitionId}:ec2:${
+            this.regionCode
+        }:${globals.awsContext.getCredentialAccountId()}:instance/${this.InstanceId}`
     }
 }

src/ec2/explorer/ec2ParentNode.ts

@@ -8,7 +8,6 @@ import { makeChildrenNodes } from '../../shared/treeview/utils'
 import { PlaceholderNode } from '../../shared/treeview/nodes/placeholderNode'
 import { Ec2InstanceNode } from './ec2InstanceNode'
 import { Ec2Client } from '../../shared/clients/ec2Client'
-import { getNameOfInstance } from '../../shared/clients/ec2Client'
 import { updateInPlace } from '../../shared/utilities/collectionUtils'
 
 export const contextValueEc2 = 'awsEc2Node'
@@ -39,9 +38,7 @@ export class Ec2ParentNode extends AWSTreeNodeBase {
     }
 
     public async updateChildren(): Promise<void> {
-        const ec2Instances = await (
-            await this.ec2Client.getInstances()
-        ).toMap(instance => getNameOfInstance(instance) ?? instance.InstanceId)
+        const ec2Instances = await (await this.ec2Client.getInstances()).toMap(instance => instance.InstanceId)
         updateInPlace(
             this.ec2InstanceNodes,
             ec2Instances.keys(),

src/test/ec2/explorer/ec2InstanceNode.test.ts

@@ -35,15 +35,15 @@ describe('ec2InstanceNode', function () {
     })
 
     it('initializes the label', async function () {
-        assert.strictEqual(testNode.label, getNameOfInstance(testInstance))
+        assert.strictEqual(testNode.label, `${getNameOfInstance(testInstance)} (${testInstance.InstanceId})`)
     })
 
     it('initializes the functionName', async function () {
         assert.strictEqual(testNode.name, getNameOfInstance(testInstance))
     })
 
     it('initializes the tooltip', async function () {
-        assert.strictEqual(testNode.tooltip, testInstance.InstanceId)
+        assert.strictEqual(testNode.tooltip, `${testNode.name}\n${testNode.InstanceId}\n${testNode.arn}`)
     })
 
     it('has no children', async function () {

src/test/ec2/explorer/ec2ParentNode.test.ts

@@ -6,7 +6,7 @@
 import * as assert from 'assert'
 import { Ec2ParentNode, contextValueEc2 } from '../../../ec2/explorer/ec2ParentNode'
 import { stub } from '../../utilities/stubber'
-import { Ec2Client } from '../../../shared/clients/ec2Client'
+import { Ec2Client, Ec2Instance } from '../../../shared/clients/ec2Client'
 import { intoCollection } from '../../../shared/utilities/collectionUtils'
 import {
     assertNodeListOnlyHasErrorNode,
@@ -16,28 +16,35 @@ import { Ec2InstanceNode } from '../../../ec2/explorer/ec2InstanceNode'
 
 describe('ec2ParentNode', function () {
     let testNode: Ec2ParentNode
-    let instanceNames: string[]
+    let instances: Ec2Instance[]
     const testRegion = 'testRegion'
     const testPartition = 'testPartition'
 
     function createClient() {
         const client = stub(Ec2Client, { regionCode: testRegion })
         client.getInstances.callsFake(async () =>
             intoCollection(
-                instanceNames.map(name => ({ InstanceId: name + name, Tags: [{ Key: 'Name', Value: name }] }))
+                instances.map(instance => ({
+                    InstanceId: instance.InstanceId,
+                    Tags: [{ Key: 'Name', Value: instance.name }],
+                }))
             )
         )
 
         return client
     }
 
     beforeEach(function () {
-        instanceNames = ['firstOne', 'secondOne']
+        instances = [
+            { name: 'firstOne', InstanceId: '0' },
+            { name: 'secondOne', InstanceId: '1' },
+        ]
+
         testNode = new Ec2ParentNode(testRegion, testPartition, createClient())
     })
 
     it('returns placeholder node if no children are present', async function () {
-        instanceNames = []
+        instances = []
 
         const childNodes = await testNode.getChildren()
 
@@ -47,7 +54,7 @@ describe('ec2ParentNode', function () {
     it('has instance child nodes', async function () {
         const childNodes = await testNode.getChildren()
 
-        assert.strictEqual(childNodes.length, instanceNames.length, 'Unexpected child count')
+        assert.strictEqual(childNodes.length, instances.length, 'Unexpected child count')
 
         childNodes.forEach(node =>
             assert.ok(node instanceof Ec2InstanceNode, 'Expected child node to be Ec2InstanceNode')
@@ -64,11 +71,18 @@ describe('ec2ParentNode', function () {
 
     it('sorts child nodes', async function () {
         const sortedText = ['aa', 'ab', 'bb', 'bc', 'cc', 'cd']
-        instanceNames = ['ab', 'bb', 'bc', 'aa', 'cc', 'cd']
+        instances = [
+            { name: 'ab', InstanceId: '0' },
+            { name: 'bb', InstanceId: '1' },
+            { name: 'bc', InstanceId: '2' },
+            { name: 'aa', InstanceId: '3' },
+            { name: 'cc', InstanceId: '4' },
+            { name: 'cd', InstanceId: '5' },
+        ]
 
         const childNodes = await testNode.getChildren()
 
-        const actualChildOrder = childNodes.map(node => node.label)
+        const actualChildOrder = childNodes.map(node => (node instanceof Ec2InstanceNode ? node.name : undefined))
         assert.deepStrictEqual(actualChildOrder, sortedText, 'Unexpected child sort order')
     })
 
@@ -79,4 +93,15 @@ describe('ec2ParentNode', function () {
         const node = new Ec2ParentNode(testRegion, testPartition, client)
         assertNodeListOnlyHasErrorNode(await node.getChildren())
     })
+
+    it('is able to handle children with duplicate names', async function () {
+        instances = [
+            { name: 'firstOne', InstanceId: '0' },
+            { name: 'secondOne', InstanceId: '1' },
+            { name: 'firstOne', InstanceId: '2' },
+        ]
+
+        const childNodes = await testNode.getChildren()
+        assert.strictEqual(childNodes.length, instances.length, 'Unexpected child count')
+    })
 })