fix: model.BomRef no longer equal to unset peers (#543)

  fixes [#539](#539) 


---------

Signed-off-by: Jan Kowalleck <[email protected]>

Author: jkowalleck

.flake8

@@ -18,3 +18,7 @@ ignore =
     ANN101,ANN102
     # ignore ANN401 for dynamically typed *args and **kwargs
     ANN401
+    # See https://www.flake8rules.com/rules/W503.html
+    # > Despite being in the best practice section, this will soon be considered an anti-pattern.
+    # So lets ignore this "suggestion" that is actually an anti-pattern already!
+    W503

cyclonedx/model/bom_ref.py

@@ -23,7 +23,7 @@ class BomRef:
     """
     An identifier that can be used to reference objects elsewhere in the BOM.
 
-    This copies a similar pattern used in the CycloneDX Python Library.
+    This copies a similar pattern used in the CycloneDX PHP Library.
 
     .. note::
         See https://github.com/CycloneDX/cyclonedx-php-library/blob/master/docs/dev/decisions/BomDependencyDataModel.md
@@ -38,23 +38,29 @@ def value(self) -> Optional[str]:
 
     @value.setter
     def value(self, value: Optional[str]) -> None:
+        # empty strings become `None`
         self._value = value or None
 
     def __eq__(self, other: object) -> bool:
-        if isinstance(other, BomRef):
-            return other._value == self._value
-        return False
+        return (self is other) or (
+            isinstance(other, BomRef)
+            # `None` value is not discriminative in this domain
+            # see also: `BomRefDiscriminator`
+            and other._value is not None
+            and self._value is not None
+            and other._value == self._value
+        )
 
     def __lt__(self, other: Any) -> bool:
         if isinstance(other, BomRef):
             return str(self) < str(other)
         return NotImplemented
 
     def __hash__(self) -> int:
-        return hash(str(self))
+        return hash(self._value or f'__id__{id(self)}')
 
     def __repr__(self) -> str:
-        return f'<BomRef {self._value!r}>'
+        return f'<BomRef {self._value!r} id={id(self)}>'
 
     def __str__(self) -> str:
         return self._value or ''

cyclonedx/model/component.py

@@ -1438,5 +1438,5 @@ def __hash__(self) -> int:
         ))
 
     def __repr__(self) -> str:
-        return f'<Component bom-ref={self.bom_ref}, group={self.group}, name={self.name}, ' \
+        return f'<Component bom-ref={self.bom_ref!r}, group={self.group}, name={self.name}, ' \
                f'version={self.version}, type={self.type}>'

cyclonedx/model/dependency.py

@@ -33,8 +33,8 @@ class _DependencyRepositorySerializationHelper(serializable.helpers.BaseHelper):
 
     @classmethod
     def serialize(cls, o: Any) -> List[str]:
-        if isinstance(o, SortedSet):
-            return list(map(lambda i: str(i.ref), o))
+        if isinstance(o, (SortedSet, set)):
+            return [str(i.ref) for i in o]
         raise SerializationOfUnexpectedValueException(
             f'Attempt to serialize a non-DependencyRepository: {o!r}')
 
@@ -102,7 +102,7 @@ def __hash__(self) -> int:
         return hash((self.ref, tuple(self.dependencies)))
 
     def __repr__(self) -> str:
-        return f'<Dependency ref={self.ref}, targets={len(self.dependencies)}>'
+        return f'<Dependency ref={self.ref!r}, targets={len(self.dependencies)}>'
 
 
 class Dependable(ABC):

tests/__init__.py

@@ -23,6 +23,7 @@
 
 from sortedcontainers import SortedSet
 
+from cyclonedx.output import BomRefDiscriminator as _BomRefDiscriminator
 from cyclonedx.schema import OutputFormat, SchemaVersion
 
 if TYPE_CHECKING:
@@ -145,6 +146,14 @@ def uuid_generator(offset: int = 0, version: int = 4) -> Generator[UUID, None, N
         yield UUID(int=v, version=version)
 
 
+class BomRefDiscriminator(_BomRefDiscriminator):
+    __uiter = 0
+
+    def _make_unique(self) -> str:
+        self.__uiter += 1
+        return f'TESTING_{self._prefix}{self.__uiter}'
+
+
 _SNAME_EXT = {
     OutputFormat.JSON: 'json',
     OutputFormat.XML: 'xml',

tests/test_model_bom.py

@@ -16,7 +16,7 @@
 # Copyright (c) OWASP Foundation. All Rights Reserved.
 
 
-from typing import Callable
+from typing import Callable, Tuple
 from unittest import TestCase
 from uuid import uuid4
 
@@ -209,3 +209,79 @@ def test_get_component_by_purl(self) -> None:
 
         self.assertIs(result, setuptools_simple)
         self.assertIsNone(bom.get_component_by_purl(get_component_setuptools_simple_no_version().purl))
+
+    @named_data(
+        ('none', tuple()),
+        # a = anonymous - bom-ref auto-set
+        # k = known - has bom-ref.value set
+        # d = known duplicate - has bom-ref.value set same as another
+        ('A(a), B(a)', ((Component(name='A'), tuple()),
+                        (Component(name='B'), tuple()))),
+        ('A(k), B(k)', ((Component(name='A', bom_ref='A'), tuple()),
+                        (Component(name='B', bom_ref='B'), tuple()))),
+        ('A(a) {A1(a)}, B(a) {B1(a)}', ((Component(name='A'), (Component(name='A1'),)),
+                                        (Component(name='B'), (Component(name='B1'),)))),
+        ('A(k) {A1(a)}', ((Component(name='A', bom_ref='A'), (Component(name='1'),)),)),
+        ('A(a) {A1(a), A2(a)}', ((Component(name='A'), (Component(name='A1'), Component(name='A2'))),)),
+        ('A(a) {A1(k)}', ((Component(name='A'), (Component(name='B', bom_ref='A1'),)),)),
+        ('A(k) {A1(k)}', ((Component(name='A', bom_ref='A'), (Component(name='A1', bom_ref='A1'),)),)),
+        ('A(d) {A1(d)}', ((Component(name='A', bom_ref='D'), (Component(name='B', bom_ref='D'),)),)),
+        ('duplicate name(a)', ((Component(name='A'), tuple()),
+                               (Component(name='A'), tuple()),)),
+        ('duplicate name(k)', ((Component(name='A', bom_ref='A1'), tuple()),
+                               (Component(name='A', bom_ref='A2'), tuple()))),
+    )
+    def test_register_dependency(self, dependencies: Tuple[Tuple[Component, Tuple[Component, ...]], ...]) -> None:
+        bom = Bom()
+        for d1, d2 in dependencies:
+            bom.components.update((d1,), d2)
+            bom.register_dependency(d1, d2)
+        bom_deps = tuple(bom.dependencies)
+        for d1, d2 in dependencies:
+            bom_dep = next((bd for bd in bom_deps if bd.ref is d1.bom_ref), None)
+            self.assertIsNotNone(bom_dep, f'missing {d1.bom_ref!r} in {bom_deps!r}')
+            self.assertEqual(len(d2), len(bom_dep.dependencies))
+            for dd in d2:
+                self.assertIn(dd.bom_ref, bom_dep.dependencies_as_bom_refs())
+
+    def test_regression_issue_539(self) -> None:
+        """regression test for issue #539
+        see https://github.com/CycloneDX/cyclonedx-python-lib/issues/539
+        """
+        # for showcasing purposes, bom-ref values MUST NOT be set
+        bom = Bom()
+        bom.metadata.component = root_component = Component(
+            name='myApp',
+            type=ComponentType.APPLICATION,
+        )
+        component1 = Component(
+            type=ComponentType.LIBRARY,
+            name='some-component',
+        )
+        component2 = Component(
+            type=ComponentType.LIBRARY,
+            name='some-library',
+        )
+        component3 = Component(
+            type=ComponentType.LIBRARY,
+            name='another-library',
+        )
+        bom.components.add(component1)
+        bom.components.add(component2)
+        bom.components.add(component3)
+        bom.register_dependency(root_component, [component1])
+        bom.register_dependency(component1, [component2])
+        bom.register_dependency(root_component, [component3])
+        # region assert root_component
+        d = next((d for d in bom.dependencies if d.ref is root_component.bom_ref), None)
+        self.assertIsNotNone(d, f'missing {root_component.bom_ref!r} in {bom.dependencies!r}')
+        self.assertEqual(2, len(d.dependencies))
+        self.assertIn(d.dependencies[0].ref, (component1.bom_ref, component3.bom_ref))
+        self.assertIn(d.dependencies[1].ref, (component1.bom_ref, component3.bom_ref))
+        # endregion assert root_component
+        # region assert component1
+        d = next((d for d in bom.dependencies if d.ref is component1.bom_ref), None)
+        self.assertIsNotNone(d, f'missing {component1.bom_ref!r} in {bom.dependencies!r}')
+        self.assertEqual(1, len(d.dependencies))
+        self.assertIs(component2.bom_ref, d.dependencies[0].ref)
+        # endregion assert component1

tests/test_model_bom_ref.py

@@ -17,22 +17,67 @@
 
 from unittest import TestCase
 
+from ddt import ddt, named_data
+
 from cyclonedx.model.bom_ref import BomRef
 from tests import reorder
 
+_BomRefNoneValue = BomRef(None)
+
 
+@ddt
 class TestBomRef(TestCase):
 
     def test_sort(self) -> None:
         # expected sort order: (value)
         expected_order = [0, 1, 2, 4, 3]
         refs = [
-            BomRef(value='a'),
-            BomRef(value='b'),
-            BomRef(value='c'),
-            BomRef(value='f'),
-            BomRef(value='d'),
+            BomRef('a'),
+            BomRef('b'),
+            BomRef('c'),
+            BomRef('f'),
+            BomRef('d'),
         ]
         sorted_refs = sorted(refs)
         expected_refs = reorder(refs, expected_order)
         self.assertListEqual(sorted_refs, expected_refs)
+
+    @named_data(
+        ('A-A', BomRef('A'), BomRef('A')),
+        ('self-BomRefNoneValue', _BomRefNoneValue, _BomRefNoneValue),
+    )
+    def test_equal(self, a: BomRef, b: BomRef) -> None:
+        self.assertEqual(a, b)
+
+    @named_data(
+        ('other-BomRefNoneValue', BomRef(None), _BomRefNoneValue),
+        ('None-None', BomRef(None), BomRef(None)),
+        ('X-None', BomRef('X'), BomRef(None)),
+        ('None-X', BomRef(None), BomRef('X')),
+        ('A-B', BomRef('A'), BomRef('B')),
+    )
+    def test_unequal(self, a: BomRef, b: BomRef) -> None:
+        self.assertNotEqual(a, b)
+
+    @named_data(
+        ('A-A', BomRef('A'), BomRef('A')),
+        ('self-BomRefNoneValue', _BomRefNoneValue, _BomRefNoneValue),
+    )
+    def test_hashes_equal(self, a: BomRef, b: BomRef) -> None:
+        self.assertEqual(hash(a), hash(b))
+        # internal usage of hash
+        self.assertEqual(1, len({a, b}))  # set
+        self.assertEqual(1, len({a: 1, b: 2}))  # dict
+
+    @named_data(
+        ('other-BomRefNoneValue', BomRef(None), _BomRefNoneValue),
+        ('None-None', BomRef(), BomRef()),
+        ('X-None', BomRef('X'), BomRef()),
+        ('None-X', BomRef(), BomRef('X')),
+        ('A-B', BomRef('A'), BomRef('B')),
+    )
+    def test_hashes_differ(self, a: BomRef, b: BomRef) -> None:
+        self.assertNotEqual(hash(a), hash(b))
+        # internal usage of hash
+        self.assertEqual(2, len({a, b}))  # set
+        self.assertEqual(2, len({a: 1, b: 2}))  # dict