refactor: rationalise add* methods for adding components to containers.

Author: yt-ms

src/structurizr/model/container.py

@@ -15,6 +15,7 @@
 
 """Provide a container model."""
 
+from __future__ import annotations
 
 from typing import TYPE_CHECKING, Iterable, List, Optional
 
@@ -70,7 +71,7 @@ class Container(StaticStructureElement):
         technology: The technology associated with this container
                     (e.g. Apache Tomcat).
         tags: A comma separated list of tags associated with this container.
-        components: The set of components within this container.
+        components: The set of components within this container.  Do not modify directly.
         properties: A set of arbitrary name-value properties.
         relationships: The set of relationships from this container to
                        other elements.
@@ -83,7 +84,7 @@ def __init__(
         parent: "SoftwareSystem",
         technology: str = "",
         components: Iterable[Component] = (),
-        **kwargs
+        **kwargs,
     ):
         """
         Initialize a container model.
@@ -116,22 +117,39 @@ def hydrate(
             parent=software_system,
             technology=container_io.technology,
         )
+        model.add_container(container)
 
         for component_io in container_io.components:
             component = Component.hydrate(component_io, container=container)
-            model.add_component(component, parent=container)
+            container += component
 
         return container
 
-    def add_component(self, component: Component = None, **kwargs) -> Component:
-        return self.get_model().add_component(
-            parent=self,
-            component=component,
-            **kwargs,
-        )
-
-    def add(self, component: Component) -> None:
+    def add_component(self, **kwargs) -> Component:
+        """Add a new component to this container."""
+        component = Component(**kwargs)
+        self += component
+        return component
+
+    def __iadd__(self, component: Component) -> Container:
+        if component in self.components:
+            # Nothing to do
+            return self
+
+        if self.get_component_with_name(component.name):
+            raise ValueError(
+                f"Component with name {component.name} already exists for {self}."
+            )
+
+        if component.parent is None:
+            component.parent = self
+        elif component.parent is not self:
+            raise ValueError(
+                f"Component with name {component.name} already has parent {component.parent}. Cannot add to {self}."
+            )
         self.components.add(component)
+        self.get_model().add_component(component)
+        return self
 
     def get_component_with_name(self, name: str) -> Component:
         for component in self.components:

src/structurizr/model/model.py

@@ -274,22 +274,15 @@ def add_container_instance(
 
     def add_component(
         self,
-        component: Optional[Component] = None,
-        **kwargs,
+        component: Component,
     ) -> Component:
-        if not component:
-            component = Component(**kwargs)
-
-        # TODO (Midnighter): It seems like a lot of this logic could be on the
-        #  Component constructor.
-        if component.parent.get_component_with_name(component.name):
+        """Register a newly constructed Component with the model."""
+        if component.parent is None:
             raise ValueError(
-                f"Component with name {component.name} already exists for "
-                f"{component.parent}."
+                f"Component with name {component.name} has no parent container.  "
+                f"Adding to container will register with the container's model."
             )
 
-        # TODO(ilaif): @midnighter - Might want to improve this impl:
-        component.parent.add(component)
         self._add_element(component)
         return component
 

src/structurizr/model/software_system.py

@@ -99,11 +99,10 @@ def hydrate(
         )
 
         for container_io in software_system_io.containers:
-            container = Container.hydrate(
+            Container.hydrate(
                 container_io,
                 software_system=software_system,
                 model=model,
             )
-            model.add_container(container, parent=software_system)
 
         return software_system

tests/unit/model/test_container.py

@@ -16,7 +16,7 @@
 
 import pytest
 
-from structurizr.model import Component, Container, Model, SoftwareSystem
+from structurizr.model import Component, Container, Model
 
 
 _model = Model()
@@ -44,47 +44,44 @@ def test_container_init(attributes):
 
 
 def test_container_add_component_adds_to_component_list(empty_container: Container):
-    """Ensure that add_component() adds the new component to Container.components and sets up other properties."""
+    """Verify add_component() adds the new component to Container.components."""
     component = empty_container.add_component(name="Component")
     assert component in empty_container.components
     assert component.id != ""
     assert component.model is _model
     assert component.parent is empty_container
 
 
-@pytest.mark.xfail(strict=True)
 def test_container_add_constructed_component(empty_container: Container):
-    """Verify behaviour when adding a newly constructed Container rather than calling add_container()."""
+    """Verify behaviour when adding a newly constructed Container."""
     component = Component(name="Component")
-    empty_container.add(component)
+    empty_container += component
     assert component in empty_container.components
-    assert component.id != ""  # Currently failing
-    assert component.model is _model  # Currently failing
-    assert component.parent is empty_container  # Currently failing
+    assert component.id != ""
+    assert component.model is _model
+    assert component.parent is empty_container
 
 
 def test_container_adding_component_twice_is_ok(empty_container: Container):
     """Defensive check that adding the same component twice is OK."""
     component = Component(name="Component")
-    empty_container.add(component)
-    empty_container.add(component)
+    empty_container += component
+    empty_container += component
     assert len(empty_container.components) == 1
 
 
-@pytest.mark.xfail(strict=True)
 def test_container_adding_component_with_same_name_fails(empty_container: Container):
-    """Defensive check that adding a component with the same name as an existing one fails."""
+    """Check that adding a component with the same name as an existing one fails."""
     empty_container.add_component(name="Component")
     with pytest.raises(ValueError, match="Component with name .* already exists"):
         empty_container.add_component(name="Component")
     with pytest.raises(ValueError, match="Component with name .* already exists"):
-        empty_container.add(Component(name="Component"))  # Doesn't currently raise
+        empty_container += Component(name="Component")
 
 
-@pytest.mark.xfail(strict=True)
 def test_adding_component_with_existing_parent_fails(empty_container: Container):
-    """Defensive check that if a component already has a (different) parent then it can't be added."""
+    """Check that adding a component with a different parent fails."""
     container2 = _system.add_container(name="Container 2", description="Description")
     component = empty_container.add_component(name="Component")
-    with pytest.raises(ValueError):
-        container2.add(component)  # Doesn't currently raise
+    with pytest.raises(ValueError, match="Component with name .* already has parent"):
+        container2 += component

tests/unit/model/test_model.py

@@ -16,7 +16,7 @@
 
 import pytest
 
-from structurizr.model import Model
+from structurizr.model import Component, Model
 
 
 @pytest.fixture(scope="function")
@@ -62,3 +62,10 @@ def test_model_cannot_add_relationship_with_same_id_as_element(empty_model: Mode
         ValueError, match="Relationship.* has the same ID as SoftwareSystem.*"
     ):
         empty_model.add_relationship(source=sys1, destination=sys2, id=sys1.id)
+
+
+def test_model_add_component_must_have_parent(empty_model: Model):
+    """Ensure that Model rejects adding Components that aren't within a Container."""
+    component = Component(name="c1")
+    with pytest.raises(ValueError, match="Component with name .* has no parent"):
+        empty_model.add_component(component)

tests/unit/model/test_software_system.py

@@ -51,6 +51,7 @@ def test_add_container_accepts_additional_args():
 
 
 def test_add_container_technology_is_optional(empty_model: Model):
+    """Ensure that you don't have to specify the technology."""
     system = empty_model.add_software_system(name="sys")
     container = system.add_container(name="Container", description="Description")
     assert container.technology == ""