refactor: rationalise add* methods for adding containers to software systems.

Author: yt-ms

src/structurizr/model/model.py

@@ -209,38 +209,29 @@ def add_software_system(self, software_system=None, **kwargs) -> SoftwareSystem:
         self.software_systems.add(software_system)
         return software_system
 
-    def add_container(
-        self, container: Optional[Container] = None, **kwargs
-    ) -> Container:
+    def add_container(self, container: Container) -> Container:
         """
-        Add a new container to the model.
+        Register a newly constructed container with the model.
 
         Args:
-            container (Container, optional): Either provide a
-                `Container` instance or
-            **kwargs: Provide keyword arguments for instantiating a `Container`
-                (recommended).
+            container (Container): `Container` instance to register.
 
         Returns:
-            SoftwareSystem: Either the same or a new instance, depending on arguments.
+            Container: The provided container.
 
         Raises:
-            ValueError: When a container with the same name already exists.
+            ValueError: When the container isn't a child of a `SoftwareSystem`
 
         See Also:
             Container
 
         """
-        if container is None:
-            container = Container(**kwargs)
-        if any(container.name == c.name for c in container.parent.containers):
-            ValueError(
-                f"A container with the name {container.name} already "
-                f"exists in the model."
+        if container.parent is None:
+            raise ValueError(
+                f"Container with name {container.name} has no parent software system."
+                f"Adding to software system will register with the system's model."
             )
-        # TODO (midnighter): Modifying the parent seems like creating an undesired
-        #  tight link here.
-        container.parent.add(container)
+
         self._add_element(container)
         return container
 

src/structurizr/model/software_system.py

@@ -74,19 +74,41 @@ def __init__(self, *, location: Location = Location.Unspecified, **kwargs) -> No
         self.tags.add(Tags.ELEMENT)
         self.tags.add(Tags.SOFTWARE_SYSTEM)
 
-    def add(self, container: Container):
-        self.containers.add(container)
-
     def add_container(
         self, name: str, description: str, technology: str = "", **kwargs
     ) -> Container:
-        return self.get_model().add_container(
-            parent=self,
-            name=name,
-            description=description,
-            technology=technology,
-            **kwargs,
+        """Construct a new `Container` and add to this system and its model."""
+        container = Container(
+            name=name, description=description, technology=technology, **kwargs
         )
+        self += container
+        return container
+
+    def __iadd__(self, container: Container) -> "SoftwareSystem":
+        """Add a newly constructed container to this system and register with its model."""
+        # TODO: once we move past python 3.6 change to proper return type via __future__.annotations
+        if container in self.containers:
+            # Nothing to do
+            return self
+
+        if self.get_container_with_name(container.name):
+            raise ValueError(
+                f"Container with name {container.name} already exists for {self}."
+            )
+
+        if container.parent is None:
+            container.parent = self
+        elif container.parent is not self:
+            raise ValueError(
+                f"Container with name {container.name} already has parent {container.parent}. Cannot add to {self}."
+            )
+        self.containers.add(container)
+        self.get_model().add_container(container)
+        return self
+
+    def get_container_with_name(self, name: str) -> Container:
+        """Return the container with the given name, or None."""
+        return next((c for c in self.containers if c.name == name), None)
 
     @classmethod
     def hydrate(

tests/unit/model/test_container.py

@@ -19,21 +19,23 @@
 from structurizr.model import Component, Container, Model
 
 
-_model = Model()
-_system = _model.add_software_system(name="Sys")
+_model = (
+    Model()
+)  # Have to create outside the fixture so it doesn't get garbage-collected.
 
 
 @pytest.fixture(scope="function")
 def empty_container() -> Container:
     """Provide an empty Container on demand for test cases to use."""
-    return _system.add_container(name="Container", description="Description")
+    system = _model.add_software_system(name="Sys")
+    return system.add_container(name="Container", description="Description")
 
 
 @pytest.mark.parametrize(
     "attributes",
     [
         pytest.param({}, marks=pytest.mark.raises(exception=TypeError)),
-        {"name": "Mobile App", "parent": _system, "technology": "tech1"},
+        {"name": "Mobile App", "technology": "tech1"},
     ],
 )
 def test_container_init(attributes):
@@ -81,7 +83,9 @@ def test_container_adding_component_with_same_name_fails(empty_container: Contai
 
 def test_adding_component_with_existing_parent_fails(empty_container: Container):
     """Check that adding a component with a different parent fails."""
-    container2 = _system.add_container(name="Container 2", description="Description")
+    container2 = empty_container.parent.add_container(
+        name="Container 2", description="Description"
+    )
     component = empty_container.add_component(name="Component")
     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 Component, Model
+from structurizr.model import Component, Container, Model
 
 
 @pytest.fixture(scope="function")
@@ -69,3 +69,10 @@ def test_model_add_component_must_have_parent(empty_model: Model):
     component = Component(name="c1")
     with pytest.raises(ValueError, match="Component with name .* has no parent"):
         empty_model.add_component(component)
+
+
+def test_model_add_container_must_have_parent(empty_model: Model):
+    """Ensure that Model rejects adding Containers that aren't within a SoftwareSystem."""
+    container = Container(name="c1")
+    with pytest.raises(ValueError, match="Container with name .* has no parent"):
+        empty_model.add_container(container)

tests/unit/model/test_software_system.py

@@ -18,21 +18,21 @@
 
 import pytest
 
-from structurizr.model.model import Model
 from structurizr.model.container import Container
+from structurizr.model.model import Model
 from structurizr.model.software_system import SoftwareSystem
 
 
-@pytest.fixture(scope="function")
-def empty_model() -> Model:
-    """Provide an empty Model on demand for test cases to use."""
-    return Model()
+_model = (
+    Model()
+)  # Have to create outside the fixture so it doesn't get garbage-collected.
 
 
 @pytest.fixture(scope="function")
 def empty_system() -> SoftwareSystem:
     """Provide an empty SoftwareSystem on demand for test cases to use."""
-    return Model().add_software_system("Sys")
+    software_system = _model.add_software_system(name="Sys")
+    return software_system
 
 
 @pytest.mark.parametrize(
@@ -57,35 +57,33 @@ def test_add_container_accepts_additional_args():
     assert container.id == "id1"
 
 
-def test_add_container_technology_is_optional(empty_model: Model):
+def test_add_container_technology_is_optional(empty_system: SoftwareSystem):
     """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")
+    container = empty_system.add_container(name="Container", description="Description")
     assert container.technology == ""
 
-    
-@pytest.mark.xfail(strict=True)
-def test_software_system_add_container_adds_to_container_list(empty_system: SoftwareSystem):
+
+def test_software_system_add_container_adds_to_container_list(
+    empty_system: SoftwareSystem,
+):
     """Ensure that add_container() adds the new container to SoftwareSystem.containers and sets up other properties."""
     container = empty_system.add_container(name="Container", description="Description")
     assert container in empty_system.containers
     assert container.id != ""
-    assert container.model is empty_system.model
+    assert container.model is _model
     assert container.parent is empty_system
 
 
-@pytest.mark.xfail(strict=True)
 def test_software_system_add_constructed_container(empty_system: SoftwareSystem):
     """Verify behaviour when adding a newly constructed Container rather than calling add_container()."""
     container = Container(name="Container")
     empty_system += container
     assert container in empty_system.containers
     assert container.id != ""
-    assert container.model is empty_system.model
+    assert container.model is _model
     assert container.parent is empty_system
 
 
-@pytest.mark.xfail(strict=True)
 def test_software_system_adding_container_twice_is_ok(empty_system: SoftwareSystem):
     """Defensive check that adding the same container twice is OK."""
     container = Container(name="Container")
@@ -94,20 +92,31 @@ def test_software_system_adding_container_twice_is_ok(empty_system: SoftwareSyst
     assert len(empty_system.containers) == 1
 
 
-@pytest.mark.xfail(strict=True)
-def test_software_system_adding_container_with_same_name_fails(empty_system: SoftwareSystem):
+def test_software_system_adding_container_with_same_name_fails(
+    empty_system: SoftwareSystem,
+):
     """Defensive check that adding a container with the same name as an existing one fails."""
-    empty_system.add_container(name="Container")
+    empty_system.add_container(name="Container", description="Description")
     with pytest.raises(ValueError, match="Container with name .* already exists"):
-        empty_system.add_container(name="Container")
+        empty_system.add_container(name="Container", description="Description2")
     with pytest.raises(ValueError, match="Container with name .* already exists"):
-        empty_system += Container(name="Container")
+        empty_system += Container(name="Container", description="Description2")
 
 
-@pytest.mark.xfail(strict=True)
-def test_software_system_adding_container_with_existing_parent_fails(empty_system: SoftwareSystem):
+def test_software_system_adding_container_with_existing_parent_fails(
+    empty_system: SoftwareSystem,
+):
     """Defensive check that if a container already has a (different) parent then it can't be added."""
-    system2 = empty_system.model.add_software_system(name="System 2", description="Description")
-    container = empty_system.add_container(name="Container")
+    system2 = empty_system.model.add_software_system(
+        name="System 2", description="Description"
+    )
+    container = empty_system.add_container(name="Container", description="Description")
     with pytest.raises(ValueError, match="Container with name .* already has parent"):
         system2 += container
+
+
+def test_software_system_get_container_with_name(empty_system: SoftwareSystem):
+    """Test getting containers by name."""
+    container = empty_system.add_container(name="Test", description="Description")
+    assert empty_system.get_container_with_name("Test") is container
+    assert empty_system.get_container_with_name("FooBar") is None