From 1c0a5e442be2960581ba12acdc43c84c4f3b4405 Mon Sep 17 00:00:00 2001
From: Alex Chantavy <achantavy@lyft.com>
Date: Wed, 22 Mar 2023 00:09:13 -0700
Subject: [PATCH] Support nodes updated by multiple modules, standardize on ARN
 for ID with ec2 instances, keypairs, nics, securitygroups, subnets, volumes

---
 cartography/models/core/common.py                         | 7 +++++--
 .../cartography/intel/aws/ec2/test_ec2_instances.py       | 4 ++--
 tests/unit/cartography/graph/test_querybuilder_complex.py | 2 +-
 tests/unit/cartography/graph/test_querybuilder_simple.py  | 4 ++--
 .../intel/aws/test_resourcegroupstaggingapi.py            | 8 ++++----
 5 files changed, 14 insertions(+), 11 deletions(-)

diff --git a/cartography/models/core/common.py b/cartography/models/core/common.py
index 2cdc5b0b16..23747611de 100644
--- a/cartography/models/core/common.py
+++ b/cartography/models/core/common.py
@@ -55,6 +55,9 @@ def __repr__(self) -> str:
         querybuilder.build_ingestion_query(). This is used for things like applying the same update tag to all nodes of
         a given run.
         """
+        if self.set_in_kwargs:
+            return self._parameterize_name()
+        if self.name.lower() == 'id' or self.ignore_case:  # Don't do coalesce() on caseinsensitive attr match.
+            return f"item.{self.name}"
         # TODO add doc on why we do the self.name.lower() and the assumed convention of lowercase for the node
-        return f"COALESCE(item.{self.name}, i.{self.name.lower()})" if not self.set_in_kwargs else \
-            self._parameterize_name()
+        return f"COALESCE(item.{self.name}, i.{self.name.lower()})"
diff --git a/tests/integration/cartography/intel/aws/ec2/test_ec2_instances.py b/tests/integration/cartography/intel/aws/ec2/test_ec2_instances.py
index 12ae9fe3df..84d7265dcf 100644
--- a/tests/integration/cartography/intel/aws/ec2/test_ec2_instances.py
+++ b/tests/integration/cartography/intel/aws/ec2/test_ec2_instances.py
@@ -196,7 +196,7 @@ def test_sync_ec2_instances(mock_get_instances, neo4j_session):
     assert check_rels(
         neo4j_session,
         'EC2Subnet',
-        'subnet_id',
+        'subnetid',
         'EC2Instance',
         'instanceid',
         'PART_OF_SUBNET',
@@ -211,7 +211,7 @@ def test_sync_ec2_instances(mock_get_instances, neo4j_session):
     assert check_rels(
         neo4j_session,
         'EC2Subnet',
-        'subnet_id',
+        'subnetid',
         'AWSAccount',
         'id',
         'RESOURCE',
diff --git a/tests/unit/cartography/graph/test_querybuilder_complex.py b/tests/unit/cartography/graph/test_querybuilder_complex.py
index f228fb0f45..6de7d5330b 100644
--- a/tests/unit/cartography/graph/test_querybuilder_complex.py
+++ b/tests/unit/cartography/graph/test_querybuilder_complex.py
@@ -9,7 +9,7 @@ def test_build_ingestion_query_complex():
 
     expected = """
         UNWIND $DictList AS item
-            MERGE (i:InterestingAsset{id: COALESCE(item.Id, i.id)})
+            MERGE (i:InterestingAsset{id: item.Id})
             ON CREATE SET i.firstseen = timestamp()
             SET
                 i.lastupdated = $lastupdated,
diff --git a/tests/unit/cartography/graph/test_querybuilder_simple.py b/tests/unit/cartography/graph/test_querybuilder_simple.py
index f786a33d4a..5bb9fc887b 100644
--- a/tests/unit/cartography/graph/test_querybuilder_simple.py
+++ b/tests/unit/cartography/graph/test_querybuilder_simple.py
@@ -69,8 +69,8 @@ def test_build_ingestion_query_case_insensitive_match():
             ON CREATE SET i.firstseen = timestamp()
             SET
                 i.lastupdated = $lastupdated,
-                i.email = item.email,
-                i.github_username = item.github_username
+                i.email = COALESCE(item.email, i.email),
+                i.github_username = COALESCE(item.github_username, i.github_username)
 
             WITH i, item
             CALL {
diff --git a/tests/unit/cartography/intel/aws/test_resourcegroupstaggingapi.py b/tests/unit/cartography/intel/aws/test_resourcegroupstaggingapi.py
index 84b06916cd..120cc27d43 100644
--- a/tests/unit/cartography/intel/aws/test_resourcegroupstaggingapi.py
+++ b/tests/unit/cartography/intel/aws/test_resourcegroupstaggingapi.py
@@ -6,17 +6,17 @@
 
 def test_compute_resource_id():
     """
-    Test that the id_func function pointer behaves as expected and returns the instanceid from an EC2Instance's ARN.
+    Test that the id_func function pointer behaves as expected and returns the vpcid from a VPC's ARN.
     """
     tag_mapping = {
-        'ResourceARN': 'arn:aws:ec2:us-east-1:1234:instance/i-abcd',
+        'ResourceARN': 'arn:aws:ec2:us-east-1:1234:vpc/vpc-123',
         'Tags': [{
             'Key': 'my_key',
             'Value': 'my_value',
         }],
     }
-    ec2_short_id = 'i-abcd'
-    assert ec2_short_id == rgta.compute_resource_id(tag_mapping, 'ec2:instance')
+    ec2_short_id = 'vpc-123'
+    assert ec2_short_id == rgta.compute_resource_id(tag_mapping, 'ec2:vpc')
 
 
 def test_get_bucket_name_from_arn():