[review] implement review comments

Signed-off-by: Meret Behrens <[email protected]>

Author: meretp

README.md

@@ -80,13 +80,16 @@ instead of `bin`.
 
 * For help use `pyspdxtools --help`
 
-3. **VISUALIZATION** (optional feature)
+3. **GRAPH GENERATION** (optional feature)
 
-* Make sure you install the optional dependencies `networkx` and `pygraphviz`. To do so run `pip install ".[visualization]"`.
-* Use `pyspdxtools -i <input_file> --visualize <output_file>` where `<output_file>` is a valid output for `pygraphviz` (check 
+* This feature generates a graph representing all elements in the SPDX document and their connections based on the provided
+  relationships. The graph can be rendered to a picture. Below is an example for the file `tests/data/formats/SPDXJSONExample-v2.3.spdx.json`:
+![SPDXJSONExample-v2.2.spdx.png](SPDXJSONExample-v2.2.spdx.png)
+* Make sure you install the optional dependencies `networkx` and `pygraphviz`. To do so run `pip install ".[graph_generation]"`.
+* Use `pyspdxtools -i <input_file> --graph_generation <output_file>` where `<output_file>` is a valid output for `pygraphviz` (check 
   the documentation [here](https://pygraphviz.github.io/documentation/stable/reference/agraph.html#pygraphviz.AGraph.draw)). 
 * If you are using a source distribution, try running
-  `pyspdxtools -i tests/data/formats/SPDXJSONExample-v2.3.spdx.json --visualize SPDXJSONExample-v2.3.spdx.png` to generate 
+  `pyspdxtools -i tests/data/formats/SPDXJSONExample-v2.3.spdx.json --graph_generation SPDXJSONExample-v2.3.spdx.png` to generate 
   a png with an overview of the structure of the example file.  
 
 ## Library usage

pyproject.toml

@@ -30,7 +30,7 @@ dynamic = ["version"]
 [project.optional-dependencies]
 test = ["pytest"]
 code_style = ["isort", "black", "flake8"]
-visualization = ["pygraphviz", "networkx"]
+graph_generation = ["pygraphviz", "networkx"]
 
 [project.scripts]
 pyspdxtools = "spdx.clitools.pyspdxtools:main"

src/common/visualization/__init__.py

@@ -18,7 +18,7 @@
 
 import click
 
-from common.visualization.visualization import visualize_document
+from common.visualization.visualization import export_graph_from_document
 from spdx.model.document import Document
 from spdx.parser.error import SPDXParsingError
 from spdx.parser.parse_anything import parse_file
@@ -43,12 +43,12 @@
 )
 @click.option("--novalidation", is_flag=True, help="Don't validate the provided document.")
 @click.option(
-    "--visualize",
+    "--graph_generation",
     default="",
     help="The file to write the structure of the spdx document as a pygraphviz AGraph to. Note: You need to install"
     " the optional dependencies 'networkx' and 'pygraphviz' for this feature.",
 )
-def main(infile: str, outfile: str, version: str, novalidation: bool, visualize: str):
+def main(infile: str, outfile: str, version: str, novalidation: bool, graph_generation: str):
     """
     CLI-tool for validating SPDX documents and converting between RDF, TAG-VALUE, JSON, YAML and XML formats.
     Formats are determined by the file endings.
@@ -82,13 +82,13 @@ def main(infile: str, outfile: str, version: str, novalidation: bool, visualize:
         if outfile and outfile != "-":
             write_file(document, outfile, validate=False)
 
-        if visualize:
+        if graph_generation:
             try:
-                visualize_document(document, visualize)
+                export_graph_from_document(document, graph_generation)
             except ImportError:
                 logging.error(
-                    "To be able to visualize the structure of the parsed document "
-                    "you need to install 'networkx' and 'pygraphviz'. Run 'pip install \".[visualization]\"'."
+                    "To be able to draw a relationship graph of the parsed document "
+                    "you need to install 'networkx' and 'pygraphviz'. Run 'pip install \".[graph_generation]\"'."
                 )
                 sys.exit(1)
 

src/spdx/clitools/pyspdxtools.py

@@ -12,23 +12,18 @@
 from spdx.model.relationship import Relationship
 
 
-def visualize_document(document: Document, visualize: str) -> None:
-    try:
-        from networkx.drawing import nx_agraph
-    except ImportError:
-        raise ImportError
+def export_graph_from_document(document: Document, file_name: str) -> None:
+    from networkx.drawing import nx_agraph
 
-    graph = generate_graph_from_spdx(document)
+    graph = generate_relationship_graph_from_spdx(document)
     _color_nodes(graph)
-    attributes_graph = nx_agraph.to_agraph(graph)  # convert to a graphviz graph
-    attributes_graph.draw(visualize, prog="dot")
+    attributes_graph = nx_agraph.to_agraph(graph)  # convert to a pygraphviz graph
+    attributes_graph.draw(file_name, prog="dot")
 
 
-def generate_graph_from_spdx(document: Document) -> DiGraph:
-    try:
-        from networkx import DiGraph
-    except ImportError:
-        raise ImportError
+def generate_relationship_graph_from_spdx(document: Document) -> DiGraph:
+    from networkx import DiGraph
+
     graph = DiGraph()
     graph.add_node(document.creation_info.spdx_id, element=document.creation_info)
 
@@ -38,42 +33,40 @@ def generate_graph_from_spdx(document: Document) -> DiGraph:
     ]
     graph.add_nodes_from(contained_element_nodes)
 
-    relationships_by_spdx_id: Dict[str, List[Relationship]] = _get_relationships_by_spdx_id(document.relationships)
+    relationships_by_spdx_id: Dict[str, List[Relationship]] = dict()
+    for relationship1 in document.relationships:
+        relationships_by_spdx_id.setdefault(relationship1.spdx_element_id, []).append(relationship1)
 
     for spdx_id, relationships in relationships_by_spdx_id.items():
         if spdx_id not in graph.nodes():
-            graph.add_node(spdx_id, element=get_element_from_spdx_id(document, spdx_id))
+            # this will add any external spdx_id to the graph where we have no further information about the element
+            # to indicate that this node represents an element we add the attribute "element"
+            graph.add_node(spdx_id, element=None)
         for relationship in relationships:
             relationship_node_key = relationship.spdx_element_id + "_" + relationship.relationship_type.name
             graph.add_node(relationship_node_key, comment=relationship.comment)
             graph.add_edge(relationship.spdx_element_id, relationship_node_key)
-            # if the related spdx element is SpdxNone or SpdxNoAssertion we need a
-            # type conversion
+            # if the related spdx element is SpdxNone or SpdxNoAssertion we need a type conversion
             related_spdx_element_id = str(relationship.related_spdx_element_id)
 
             if related_spdx_element_id not in graph.nodes():
+                # this will add any external spdx_id to the graph where we have no further information about the element
+                # to indicate that this node represents an element we add the attribute "element"
                 graph.add_node(
                     related_spdx_element_id,
-                    element=get_element_from_spdx_id(document, spdx_id),
+                    element=None,
                 )
             graph.add_edge(relationship_node_key, related_spdx_element_id)
 
     return graph
 
 
-def _get_relationships_by_spdx_id(
-    relationships: List[Relationship],
-) -> Dict[str, List[Relationship]]:
-    relationships_by_spdx_id: Dict[str, List[Relationship]] = dict()
-    for relationship in relationships:
-        relationships_by_spdx_id.setdefault(relationship.spdx_element_id, []).append(relationship)
-
-    return relationships_by_spdx_id
-
-
 def _color_nodes(graph: DiGraph) -> None:
     for node in graph.nodes():
         if "_" in node:
+            # nodes representing a RelationshipType are concatenated with the spdx_element_id
+            # to only see the RelationshipType when rendering the graph to a picture we add
+            # a label to these nodes
             graph.add_node(node, color="lightgreen", label=node.split("_", 1)[-1])
         elif node == "SPDXRef-DOCUMENT":
             graph.add_node(node, color="indianred2")

src/common/visualization/visualization.py src/spdx/graph_generation.pysrc/common/visualization/visualization.py renamed to src/spdx/graph_generation.py

@@ -1,4 +1,4 @@
-# SPDX-FileCopyrightText: 2023 TNG Technology Consulting GmbH <https://www.tngtech.com>
+# SPDX-FileCopyrightText: 2023 spdx contributors
 #
 # SPDX-License-Identifier: Apache-2.0
 from pathlib import Path
@@ -7,9 +7,8 @@
 
 import pytest
 
-from common.visualization.visualization import generate_graph_from_spdx
+from spdx.graph_generation import generate_relationship_graph_from_spdx
 from spdx.model.document import Document
-from spdx.model.package import Package
 from spdx.model.relationship import Relationship, RelationshipType
 from spdx.parser.parse_anything import parse_file
 from tests.spdx.fixtures import document_fixture, file_fixture, package_fixture
@@ -63,10 +62,9 @@ def test_generate_graph_from_spdx(
     relationship_node_keys: List[str],
 ) -> None:
     document = parse_file(str(Path(__file__).resolve().parent.parent / "spdx" / "data" / "formats" / file_name))
-    graph = generate_graph_from_spdx(document)
+    graph = generate_relationship_graph_from_spdx(document)
 
     assert document.creation_info.spdx_id in graph.nodes()
-
     assert graph.number_of_nodes() == nodes_count
     assert graph.number_of_edges() == edges_count
     assert "SPDXRef-DOCUMENT_DESCRIBES" in graph.nodes()
@@ -77,7 +75,7 @@ def test_generate_graph_from_spdx(
 def test_complete_connected_graph() -> None:
     document = _create_minimal_document()
 
-    graph = generate_graph_from_spdx(document)
+    graph = generate_relationship_graph_from_spdx(document)
 
     TestCase().assertCountEqual(
         graph.nodes(),
@@ -107,15 +105,9 @@ def test_complete_connected_graph() -> None:
 
 def test_complete_unconnected_graph() -> None:
     document = _create_minimal_document()
-    document.packages += [
-        Package(
-            spdx_id="SPDXRef-Package-C",
-            name="Package without connection to document",
-            download_location="https://download.location.com",
-        )
-    ]
+    document.packages += [package_fixture(spdx_id="SPDXRef-Package-C", name="Package without connection to document")]
 
-    graph = generate_graph_from_spdx(document)
+    graph = generate_relationship_graph_from_spdx(document)
 
     TestCase().assertCountEqual(
         graph.nodes(),