Issue 63: StaticView.add_nearest_neighbours() should not duplicate relationships

CHANGELOG.rst

@@ -4,6 +4,8 @@ History
 
 Next Release
 ------------
+* Fix: Don't duplicate relationships if ``add_nearest_neighbours()`` called twice (#63)
+* Fix: Support blank diagrams descriptions from the Structurizr UI (#40)
 
 0.3.0 (2020-11-29)
 ------------------

examples/big_bank.py

@@ -152,14 +152,16 @@ def create_big_bank_workspace():
     single_page_application.tags.add(web_browser_tag)
     mobile_app = internet_banking_system.add_container(
         "Mobile App",
-        "Provides a limited subset of the Internet banking functionality to customers via their mobile device.",
+        "Provides a limited subset of the Internet banking functionality to "
+        "customers via their mobile device.",
         "Xamarin",
         id="mobileApp",
     )
     mobile_app.tags.add(mobile_app_tag)
     web_application = internet_banking_system.add_container(
         "Web Application",
-        "Delivers the static content and the Internet banking single page application.",
+        "Delivers the static content and the Internet banking single page "
+        "application.",
         "Java and Spring MVC",
         id="webApplication",
     )
@@ -171,7 +173,8 @@ def create_big_bank_workspace():
     )
     database = internet_banking_system.add_container(
         "Database",
-        "Stores user registration information, hashed authentication credentials, access logs, etc.",
+        "Stores user registration information, hashed authentication credentials, "
+        "access logs, etc.",
         "Relational Database Schema",
         id="database",
     )
@@ -188,8 +191,9 @@ def create_big_bank_workspace():
     api_application.uses(email_system, "Sends e-mail using", technology="SMTP")
 
     # components
-    # - for a real-world software system, you would probably want to extract the components using
-    # - static analysis/reflection rather than manually specifying them all
+    # - for a real-world software system, you would probably want to extract the
+    #   components using static analysis/reflection rather than manually specifying
+    #   them all
 
     signin_controller = api_application.add_component(
         name="Sign In Controller",
@@ -211,7 +215,8 @@ def create_big_bank_workspace():
     )
     security_component = api_application.add_component(
         name="Security Component",
-        description="Provides functionality related to signing in, changing passwords, etc.",
+        description="Provides functionality related to signing in, changing passwords, "
+        "etc.",
         technology="Spring Bean",
         id="securityComponent",
     )
@@ -353,7 +358,8 @@ def create_big_bank_workspace():
     secondary_database_server.tags.add(failover_tag)
     secondary_database = secondary_database_server.add_container(database)
 
-    # # model.Relationships.Where(r=>r.Destination.Equals(secondary_database)).ToList().ForEach(r=>r.tags.add(failover_tag))
+    # model.Relationships.Where(r=>r.Destination.Equals(secondary_database)).ToList()
+    #   .ForEach(r=>r.tags.add(failover_tag))
     data_replication_relationship = primary_database_server.uses(
         secondary_database_server, "Replicates data to"
     )
@@ -400,7 +406,8 @@ def create_big_bank_workspace():
     component_view.add(email_system)
     component_view.paper_size = PaperSize.A5_Landscape
 
-    # systemLandscapeView.AddAnimation(internet_banking_system, customer, mainframe_banking_system, emailSystem)
+    # systemLandscapeView.AddAnimation(internet_banking_system, customer,
+    #   mainframe_banking_system, emailSystem)
     # systemLandscapeView.AddAnimation(atm)
     # systemLandscapeView.AddAnimation(customerServiceStaff, back_office_staff)
 
@@ -418,20 +425,24 @@ def create_big_bank_workspace():
 
     # componentView.AddAnimation(singlePageApplication, mobile_app)
     # componentView.AddAnimation(signinController, securityComponent, database)
-    # componentView.AddAnimation(accountsSummaryController, mainframe_banking_systemFacade, mainframe_banking_system)
+    # componentView.AddAnimation(accountsSummaryController,
+    #   mainframe_banking_systemFacade, mainframe_banking_system)
     # componentView.AddAnimation(resetPasswordController, emailComponent, database)
 
     # # dynamic diagrams and deployment diagrams are not available with the Free Plan
-    # DynamicView dynamicView = views.CreateDynamicView(apiApplication, "SignIn", "Summarises how the sign in feature works in the single-page application.")
+    # DynamicView dynamicView = views.CreateDynamicView(apiApplication, "SignIn",
+    #   "Summarises how the sign in feature works in the single-page application.")
     # dynamicView.Add(singlePageApplication, "Submits credentials to", signinController)
     # dynamicView.Add(signinController, "Calls isAuthenticated() on", securityComponent)
-    # dynamicView.Add(securityComponent, "select * from users where username = ?", database)
+    # dynamicView.Add(securityComponent, "select * from users where username = ?",
+    #   database)
     # dynamicView.PaperSize = PaperSize.A5_Landscape
 
     development_deployment_view = views.create_deployment_view(
         software_system=internet_banking_system,
         key="DevelopmentDeployment",
-        description="An example development deployment scenario for the Internet Banking System.",
+        description="An example development deployment scenario for the Internet "
+        "Banking System.",
         environment="Development",
     )
     development_deployment_view.add(developer_laptop)
@@ -440,7 +451,8 @@ def create_big_bank_workspace():
     live_deployment_view = views.create_deployment_view(
         software_system=internet_banking_system,
         key="LiveDeployment",
-        description="An example live deployment scenario for the Internet Banking System.",
+        description="An example live deployment scenario for the Internet Banking "
+        "System.",
         environment="Live",
     )
     live_deployment_view += big_bank_data_center

examples/financial_risk_system.py

@@ -127,7 +127,10 @@ def main() -> Workspace:
     contextView = views.create_system_context_view(
         software_system=financial_risk_system,
         key="Context",
-        description="An example System Context diagram for the Financial Risk System architecture kata.",
+        description=(
+            "An example System Context diagram for the Financial Risk System "
+            "architecture kata."
+        ),
     )
     contextView.add_all_software_systems()
     contextView.add_all_people()

src/structurizr/view/static_view.py

@@ -72,7 +72,7 @@ def hydrate_arguments(cls, static_view_io: StaticViewIO) -> Dict:
     @abstractmethod
     def add_all_elements(self) -> None:
         """Add all permitted elements from a model to this view."""
-        pass
+        pass  # pragma: no cover
 
     def add(
         self,
@@ -106,7 +106,7 @@ def add_nearest_neighbours(
         element_type: Union[Type[Person], Type[SoftwareSystem], Type[Container]],
     ) -> None:
         """Add all permitted elements from a model to this view."""
-        self._add_element(element, True)
+        self._add_element(element, False)
 
         # TODO(ilaif): @midnighter - Should we move to @property instead
         #  of get_X()? More pythonic.

src/structurizr/view/view.py

@@ -42,7 +42,7 @@ class ViewIO(BaseModel, ABC):
     """
 
     key: str
-    description: str
+    description: str = ""
     software_system_id: Optional[str] = Field(default=None, alias="softwareSystemId")
     paper_size: Optional[PaperSize] = Field(default=None, alias="paperSize")
     automatic_layout: Optional[AutomaticLayoutIO] = Field(
@@ -173,14 +173,23 @@ def _add_relationship(self, relationship: Relationship) -> RelationshipView:
         """Add a single relationship to this view.
 
         Returns:
-            The new view if both the source and destination for the relationship are
-            in this view, else `None`.
+            The new (or existing) view if both the source and destination for the
+            relationship are in this view, else `None`.
         """
         if self.is_element_in_view(relationship.source) and self.is_element_in_view(
             relationship.destination
         ):
-            view = RelationshipView(relationship=relationship)
-            self.relationship_views.add(view)
+            view = next(
+                (
+                    rv
+                    for rv in self.relationship_views
+                    if rv.relationship is relationship
+                ),
+                None,
+            )
+            if not view:
+                view = RelationshipView(relationship=relationship)
+                self.relationship_views.add(view)
             return view
         return None
 

tests/unit/view/test_static_view.py

@@ -0,0 +1,72 @@
+# Copyright (c) 2020
+#
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+#     https://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+
+"""Ensure the expected behaviour of StaticView."""
+
+
+from structurizr.model import Model, Person, SoftwareSystem
+from structurizr.view.static_view import StaticView
+
+
+class DerivedView(StaticView):
+    """Mock class for testing."""
+
+    def add_all_elements(self) -> None:
+        """Stub method because base is abstract."""
+        pass
+
+
+def test_add_nearest_neighbours():
+    """Test basic behaviour of add_nearest_neighbours."""
+    model = Model()
+    sys1 = model.add_software_system(name="System 1")
+    sys2 = model.add_software_system(name="System 2")
+    person = model.add_person(name="Person 1")
+    sys1.uses(sys2)
+    person.uses(sys1)
+
+    # Check neighbours from outbound relationships
+    view = DerivedView(software_system=sys1, description="")
+    view.add_nearest_neighbours(sys1, SoftwareSystem)
+    assert any((elt_view.element is sys1 for elt_view in view.element_views))
+    assert any((elt_view.element is sys2 for elt_view in view.element_views))
+    assert not any((elt_view.element is person for elt_view in view.element_views))
+    assert len(view.relationship_views) == 1
+
+    # Check neighbours from inbound relationships
+    view = DerivedView(software_system=sys1, description="")
+    view.add_nearest_neighbours(sys2, SoftwareSystem)
+    assert any((elt_view.element is sys1 for elt_view in view.element_views))
+    assert any((elt_view.element is sys2 for elt_view in view.element_views))
+    assert not any((elt_view.element is person for elt_view in view.element_views))
+    assert len(view.relationship_views) == 1
+
+
+def test_add_nearest_neighbours_doesnt_dupe_relationships():
+    """Test relationships aren't duplicated if neighbours added more than once.
+
+    See https://github.com/Midnighter/structurizr-python/issues/63.
+    """
+    model = Model()
+    sys1 = model.add_software_system(name="System 1")
+    sys2 = model.add_software_system(name="System 2")
+    sys1.uses(sys2)
+    view = DerivedView(software_system=sys1, description="")
+    view.add_nearest_neighbours(sys1, SoftwareSystem)
+    assert len(view.relationship_views) == 1
+
+    # The next line should not add any new relationships
+    view.add_nearest_neighbours(sys1, Person)
+    assert len(view.relationship_views) == 1

tests/unit/view/test_view.py

@@ -0,0 +1,99 @@
+# Copyright (c) 2020
+#
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+#     https://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+
+"""Ensure the expected behaviour of View."""
+
+from structurizr.model import Model
+from structurizr.view.view import View, ViewIO
+
+
+class DerivedView(View):
+    """Mock class for testing."""
+
+    pass
+
+
+def test_add_relationship_doesnt_duplicate():
+    """Test that adding a relationships twice doesn't duplicate it."""
+    model = Model()
+    sys1 = model.add_software_system(name="System 1")
+    sys2 = model.add_software_system(name="System 2")
+    rel = sys1.uses(sys2)
+
+    view = DerivedView(software_system=sys1, description="")
+    view._add_element(sys1, False)
+    view._add_element(sys2, False)
+
+    rel_view1 = view._add_relationship(rel)
+    assert len(view.relationship_views) == 1
+    rel_view2 = view._add_relationship(rel)
+    assert len(view.relationship_views) == 1
+    assert rel_view2 is rel_view1
+
+
+def test_add_relationship_for_element_not_in_view():
+    """Ensures relationships for elements outside the view are ignored."""
+    model = Model()
+    sys1 = model.add_software_system(name="System 1")
+    sys2 = model.add_software_system(name="System 2")
+    rel = sys1.uses(sys2)
+
+    view = DerivedView(software_system=sys1, description="")
+    view._add_element(sys1, False)
+
+    # This relationship should be ignored as sys2 isn't in the view
+    rel_view1 = view._add_relationship(rel)
+    assert rel_view1 is None
+    assert view.relationship_views == set()
+
+
+def test_adding_all_relationships():
+    """Test adding all relationships for elements in the view."""
+    model = Model()
+    sys1 = model.add_software_system(name="System 1")
+    sys2 = model.add_software_system(name="System 2")
+    sys3 = model.add_software_system(name="System 3")
+    rel1 = sys1.uses(sys2)
+    rel2 = sys3.uses(sys1)
+
+    view = DerivedView(software_system=sys1, description="")
+    view._add_element(sys1, False)
+    view._add_element(sys2, False)
+    view._add_element(sys3, False)
+    assert view.relationship_views == set()
+
+    view._add_relationships(sys1)
+    assert len(view.relationship_views) == 2
+    assert rel1 in [vr.relationship for vr in view.relationship_views]
+    assert rel2 in [vr.relationship for vr in view.relationship_views]
+
+
+def test_missing_json_description_allowed():
+    """
+    Ensure that missing descriptions in the JSON form are supported.
+
+    Raised as https://github.com/Midnighter/structurizr-python/issues/40, it is
+    permitted through the Structurizr UI to create views with a blank description,
+    which then gets ommitted from the workspace JSON, so this needs to be allowed by
+    the Pydantic validation also.
+    """
+
+    json = """
+    {
+        "key": "System1-SystemContext"
+    }
+    """
+    io = ViewIO.parse_raw(json)
+    assert io is not None