Fix #1064: handle firstseen as a reserved word (#1065)

cartography/graph/model.py

@@ -78,37 +78,56 @@ def __repr__(self) -> str:
 class CartographyNodeProperties(abc.ABC):
     """
     Abstract base dataclass that represents the properties on a CartographyNodeSchema. This class is abstract so that we
-    can enforce that all subclasses have an id and a lastupdated field.
+    can enforce that all subclasses have an id and a lastupdated field. These fields are assigned to the node in the
+    `SET` clause.
     """
     id: PropertyRef = field(init=False)
     lastupdated: PropertyRef = field(init=False)
 
     def __post_init__(self):
         """
-        Designed to prevent direct instantiation. This workaround is needed since this is a dataclass and an abstract
-        class without an abstract method defined.
-        See https://stackoverflow.com/q/60590442.
+        Data validation.
+        1. Prevents direct instantiation. This workaround is needed since this is a dataclass and an abstract
+        class without an abstract method defined. See https://stackoverflow.com/q/60590442.
+        2. Stops reserved words from being used as attribute names. See https://github.com/lyft/cartography/issues/1064.
         """
         if self.__class__ == CartographyNodeProperties:
             raise TypeError("Cannot instantiate abstract class.")
 
+        if hasattr(self, 'firstseen'):
+            raise TypeError(
+                "`firstseen` is a reserved word and is automatically set by the querybuilder on cartography nodes, so "
+                f'it cannot be used on class "{type(self).__name__}(CartographyNodeProperties)". Please either choose '
+                "a different name for `firstseen` or omit altogether.",
+            )
+
 
 @dataclass(frozen=True)
 class CartographyRelProperties(abc.ABC):
     """
     Abstract class that represents the properties on a CartographyRelSchema. This is intended to enforce that all
-    subclasses will have a lastupdated field defined on their resulting relationships.
+    subclasses will have a lastupdated field defined on their resulting relationships. These fields are assigned to the
+    relationship in the `SET` clause.
     """
     lastupdated: PropertyRef = field(init=False)
 
     def __post_init__(self):
         """
-        Designed to prevent direct instantiation. This workaround is needed since this is a dataclass and an abstract
-        class without an abstract method defined.
+        Data validation.
+        1. Prevents direct instantiation. This workaround is needed since this is a dataclass and an abstract
+        class without an abstract method defined. See https://stackoverflow.com/q/60590442.
+        2. Stops reserved words from being used as attribute names. See https://github.com/lyft/cartography/issues/1064.
         """
         if self.__class__ == CartographyRelProperties:
             raise TypeError("Cannot instantiate abstract class.")
 
+        if hasattr(self, 'firstseen'):
+            raise TypeError(
+                "`firstseen` is a reserved word and is automatically set by the querybuilder on cartography rels, so "
+                f'it cannot be used on class "{type(self).__name__}(CartographyRelProperties)". Please either choose '
+                "a different name for `firstseen` or omit altogether.",
+            )
+
 
 @dataclass(frozen=True)
 class TargetNodeMatcher:

cartography/intel/aws/emr.py

@@ -65,7 +65,6 @@ class EMRClusterNodeProperties(CartographyNodeProperties):
     auto_terminate: PropertyRef = PropertyRef('AutoTerminate')
     autoscaling_role: PropertyRef = PropertyRef('AutoScalingRole')
     custom_ami_id: PropertyRef = PropertyRef('CustomAmiId')
-    firstseen: PropertyRef = PropertyRef('firstseen')
     id: PropertyRef = PropertyRef('Id')
     instance_collection_type: PropertyRef = PropertyRef('InstanceCollectionType')
     lastupdated: PropertyRef = PropertyRef('lastupdated', set_in_kwargs=True)

tests/integration/cartography/intel/aws/test_emr.py

@@ -9,6 +9,7 @@
 
 
 def test_load_emr_clusters_nodes(neo4j_session):
+    # Act
     data = tests.data.aws.emr.DESCRIBE_CLUSTERS
     cartography.intel.aws.emr.load_emr_clusters(
         neo4j_session,
@@ -18,6 +19,7 @@ def test_load_emr_clusters_nodes(neo4j_session):
         TEST_UPDATE_TAG,
     )
 
+    # Assert
     expected_nodes = {
         ("arn:aws:elasticmapreduce:us-east-1:190000000000:cluster/j-awesome",),
         ("arn:aws:elasticmapreduce:us-east-1:190000000000:cluster/j-meh",),
@@ -26,7 +28,7 @@ def test_load_emr_clusters_nodes(neo4j_session):
 
 
 def test_load_emr_clusters_relationships(neo4j_session):
-    # Create Test AWSAccount
+    # Arrange: Create Test AWSAccount
     neo4j_session.run(
         """
         MERGE (aws:AWSAccount{id: $aws_account_id})
@@ -37,7 +39,7 @@ def test_load_emr_clusters_relationships(neo4j_session):
         aws_update_tag=TEST_UPDATE_TAG,
     )
 
-    # Load Test EMR Clusters
+    # Act: Load Test EMR Clusters
     data = tests.data.aws.emr.DESCRIBE_CLUSTERS
     cartography.intel.aws.emr.load_emr_clusters(
         neo4j_session,
@@ -47,6 +49,7 @@ def test_load_emr_clusters_relationships(neo4j_session):
         TEST_UPDATE_TAG,
     )
 
+    # Assert
     expected = {
         (TEST_ACCOUNT_ID, 'arn:aws:elasticmapreduce:us-east-1:190000000000:cluster/j-awesome'),
         (TEST_ACCOUNT_ID, 'arn:aws:elasticmapreduce:us-east-1:190000000000:cluster/j-meh'),