refactor: make operations on deployment nodes consistent with software systems and containers in Model.

Author: yt-ms

src/structurizr/model/model.py

@@ -111,7 +111,6 @@ def __init__(
         """
         super().__init__(**kwargs)
         self.enterprise = enterprise
-        self.deployment_nodes = set(deployment_nodes)
         self.implied_relationship_strategy = implied_relationship_strategy
         # TODO: simply iterate attributes
         self._elements_by_id = {}
@@ -132,6 +131,15 @@ def people(self) -> Set[Person]:
         """Return the people in the model."""
         return {e for e in self.get_elements() if isinstance(e, Person)}
 
+    @property
+    def deployment_nodes(self) -> Set[DeploymentNode]:
+        """Return the *top level* deployment nodes in the model."""
+        return {
+            e
+            for e in self.get_elements()
+            if isinstance(e, DeploymentNode) and e.parent is None
+        }
+
     @classmethod
     def hydrate(cls, model_io: ModelIO) -> "Model":
         """Return a new model, hydrated from its IO."""
@@ -219,6 +227,15 @@ def __iadd__(self, element: Element) -> "Model":
                     f"A software system with the name '{element.name}' already "
                     f"exists in the model."
                 )
+        elif isinstance(element, DeploymentNode):
+            if any(
+                element.name == d.name and isinstance(d, DeploymentNode)
+                for d in self.get_elements()
+            ):
+                raise ValueError(
+                    f"A deployment node with the name '{element.name}' already "
+                    f"exists in the model."
+                )
         elif element.parent is None:
             raise ValueError(
                 f"Element with name {element.name} has no parent.  Please ensure "
@@ -255,20 +272,15 @@ def add_container_instance(
         # TODO: implement
         # instance_number =
 
-    def add_deployment_node(
-        self, deployment_node: Optional[DeploymentNode] = None, **kwargs
-    ) -> DeploymentNode:
+    def add_deployment_node(self, **kwargs) -> DeploymentNode:
         """
         Add a new deployment node to the model.
 
         Args:
-            deployment_node (DeploymentNode, optional): Either provide a
-                `DeploymentNode` instance or
-            **kwargs: Provide keyword arguments for instantiating a `DeploymentNode`
-                (recommended).
+            **kwargs: Provide keyword arguments for instantiating a `DeploymentNode`.
 
         Returns:
-            DeploymentNode: Either the same or a new instance, depending on arguments.
+            DeploymentNode: The newly created DeploymentNode.
 
         Raises:
             ValueError: When a deployment node with the same name already exists.
@@ -277,15 +289,8 @@ def add_deployment_node(
             DeploymentNode
 
         """
-        if deployment_node is None:
-            deployment_node = DeploymentNode(**kwargs)
-        if deployment_node.id in {d.id for d in self.deployment_nodes}:
-            ValueError(
-                f"A deployment node with the ID {deployment_node.id} already "
-                f"exists in the model."
-            )
-        self.deployment_nodes.add(deployment_node)
-        self._add_element(deployment_node)
+        deployment_node = DeploymentNode(**kwargs)
+        self += deployment_node
         return deployment_node
 
     def add_relationship(

tests/unit/model/test_model.py

@@ -17,6 +17,7 @@
 import pytest
 
 from structurizr.model import Component, Container, Model, Person, SoftwareSystem
+from structurizr.model.deployment_node import DeploymentNode
 
 
 @pytest.fixture(scope="function")
@@ -78,6 +79,33 @@ def test_model_add_container_must_have_parent(empty_model: Model):
         empty_model += container
 
 
+def test_model_add_top_level_deployment_node(empty_model: Model):
+    """Make sure top-level deployment nodes are reflected in Model.deployent_nodes."""
+    node = empty_model.add_deployment_node(name="node1")
+    assert node is not None
+    assert node in empty_model.deployment_nodes
+    assert node in empty_model.get_elements()
+
+
+def test_model_cant_add_two_deployment_nodes_with_same_name(empty_model: Model):
+    """Make sure that deployment nodes (at any level) can't share a name."""
+    node = empty_model.add_deployment_node(name="node1")
+    with pytest.raises(
+        ValueError,
+        match="A deployment node with the name 'node1' already exists in the model.",
+    ):
+        node.add_deployment_node(name="node1")
+
+
+def test_model_add_lower_level_deployment_node(empty_model: Model):
+    """Make sure child deployment nodes are not reflected in Model.deployent_nodes."""
+    node1 = empty_model.add_deployment_node(name="node1")
+    node2 = DeploymentNode(name="node2", parent=node1)
+    empty_model += node2
+    assert node2 not in empty_model.deployment_nodes
+    assert node2 in empty_model.get_elements()
+
+
 def test_model_add_person_with_plusequals(empty_model: Model):
     """Check that adding a Person to a Model with += works."""
     bob = Person(name="Bob")