feat!: Add component and services for tools

[jkugler]

CycloneDX spec 1.5 deprecated an array of tools in bom.metadata and instead prefers object with an array of components and an array of services.

This PR implements that.

This works de-serializing a Syft SBOM with a tool section like so:

  "metadata": {
    "timestamp": "2024-06-10T13:06:52-08:00",
    "tools": {
      "components": [
        {
          "type": "application",
          "author": "anchore",
          "name": "syft",
          "version": "1.4.1"
        }
      ]
    },
    "component": {
      "bom-ref": "08329a07b4eb8eac",
      "type": "file",
      "name": "./"
    }
  },

Next up: docs, XML (de)serialization code, and tests.

fixes #561

[jkowalleck]

i dislike the current concept of ToolRepository that holds tools, services and components at the same time.
This is an abomination of a data type, and teaching people how to use it properly will just be a hack of an effort.

i'd rather have a type ToolRepository that holds services and components and no Tool!
So, Metadata.tools would be truly Union[ToolRepository, SortedSet[Tool]].
The (de)serialize would be handled by a (de)normaliser (aka "Helper" and thats it.

Is there something that speaks against this very simple and still pythonic solution?

[jkowalleck]

a feature i would love to see in the end: metadata tools converters, that allow me to have a bunch of Components and Services , and when normalizing to CycloneDX 1.3, the Compoennts and Servioces are converted to Tools in the resulting XML/JSON. So that i dont loose any data...

[jkugler]

i dislike the current concept of ToolRepository that holds tools, services and components at the same time. This is an abomination of a data type, and teaching people how to use it properly will just be a hack of an effort.

i'd rather have a type ToolRepository that holds services and components and no Tool! So, Metadata.tools would be truly Union[ToolRepository, SortedSet[Tool]]. The (de)serialize would be handled by a (de)normaliser (aka "Helper" and thats it.

Is there something that speaks against this very simple and still pythonic solution?

@jkowalleck:

So drivers for my approach were two-fold:

1. I am not 100% acquainted with the use cases of the the library; that is how people use the library.
2. I was trying to maintain 100% backward compatibility.

Thus, the reason for ToolRepository which combined the three types. I wanted to be able to initialize and empty BomMetaData (which is allowed now) and then do:

# old code still works
bom.metadata.tools.add(....)
# or
bom.metadata.tools = my_sorted_set_of_tools

as well as be able to do:

# new code would work as expected
bom.metadata.tools.components.add(...)
bom.metadata.tools.services.add(...)

If we don't have an object in bom.metadata.tools which responds to components and services attributes, then calling those attributes will generate an exception, and cause unexpected behavior. At least in my mind, I would expect to be able to instantiate a Bom, and then call bom.metadata.tools.components.add(...) or bom.metadata.tools.components = ...

a feature i would love to see in the end: metadata tools converters, that allow me to have a bunch of Components and Services , and when normalizing to CycloneDX 1.3, the Compoennts and Servioces are converted to Tools in the resulting XML/JSON. So that i dont loose any data...

That would be really neat. Do you mean a stand-alone tool? Or built in to the new type? Is there a way to know in the normalizing functions which version we're serializing for? I did not see that anywhere in the docs, but it would be great if we could do that.

[jkowalleck]

re #635 (comment)

Thank you for the explanation.
I understand the reasoning behind the current implementation of ToolsRepo now.
For sake of usability, you are right to using it.

A need for API backwards compatibility is not really needed. Clean code and documentation are more important.

An improvement I could imagine:
have it a much simpler, documented container-object like so:
(untested python code)

class ToolsRepository:
  """our implementation of the tools repo"""

  tools: SortedSet[Tool]
  """DEPRECATED tools"""

  components: SortedSet[Component]
  """docstring here..."""
  
  services: SortedSet[Service]
  """docstring here..."""

  def __init__(self, *,
    components: Optional[Iterable[Components]] = None,
    services: Optional[Iterable[Service]] = None,
    # Deprecated in v1.x
    tools: Optional[Iterable[Tool]] = None,
  ): 
    if tools:
      warn("deprecation message here...", DeprecationWarning)
    self.tools = SortedSet(tools or ())
    self.components = SortedSet(components or ())
    self.services = SortedSet(services or ())

[jkugler]

I don't know if this is the best solution. We could just put all the Tools stuff into this file, and avoid the circular-imports work-around.

[jkowalleck]

sure. put all in the __init__.py.

[jkugler]

I spent a few hours trying to solve circular import errors in cylclonedx.model.bom (after putting all the tool stuff in __init__.py and then gave up when I hit a circular import error in cyclonedx.serialization. If I don't stop, I'm going to end up owning all the code . :D I'll leave this as-is for now, and we can do a code restructuring as part of another PR.

[jkugler]

Thanks for the explanation. Given the comment here: https://github.com/CycloneDX/cyclonedx-python-lib/blob/main/cyclonedx/model/__init__.py#L1079 I'd like to try to keep backward compatibility for now. The code to maintain this is probably only 60 to 70 or so lines more than breaking backward compatibility, and once the support for List[Tool] goes away completely, the semantics for tools.components and tools.services won't change.

Everything seems to be working, save the problem I mentioned here: https://cyclonedx.slack.com/archives/CVA0QJEVA/p1718821402381719

[jkugler]

Tests for new functionality are in place. Pretty sure I didn't do it the best way, but just wanted to show the new functionality works. :) We can re-work the tests before merge if need be.

[jkugler]

BTW, maintains 93% test coverage. I have a few more code paths I want to test as well.

[jkowalleck]

@jkugler, before we can use your contribution, we need you to sign-off your commits.

here is why this is required and what it implies: https://github.com/CycloneDX/cyclonedx-python-lib/blob/main/CONTRIBUTING.md#sign-off-your-commits

here is a step-by-step instruction on how to do this: https://github.com/CycloneDX/cyclonedx-python-lib/pull/635/checks?check_run_id=26536713378

[jkugler]

There is a decision we'll need to make in how to handle the current behavior in BomMetaData:

        if not tools:
            self.tools.add(ThisTool)

This adds the CycloneDX information if none is provided when initializing BomMetaData, and the will generate an error if someone, later does this:

bom.metadata.tools.components = ...
# or
bom.metadata.tools.services = 

However, there will not be an error if a developer does

bom.metadata.tools.components.add(...)

or similar with services, and if there are components or services, those will be rendered and not the [Tools].

If we're OK with that behavior, that is, telling users to provide a ToolsRepository themselves, or use .add(), I'm OK with that.

[jkugler]

@jkugler, before we can use your contribution, we need you to sign-off your commits.

Yes, I did that in 2bbd659

I didn't realize I would need to do that for every commit, as I assumed the PR would be squash merged into a single commit, and thus include the sign-off. I'll get it fixed.

[jkugler]

I am trying to go through the changes made, but I'm not understanding where the code is. I see a bunch of code was moved to cyclonedx/serialization/__init__.py in this commit: 376dfa8 but when I pull down my branch: https://github.com/jkugler/cyclonedx-python-lib/blob/561_add_components_and_services/cyclonedx/serialization/__init__.py I don't see that code. Was that code not pushed to my branch?

Sorry for the confusionl.

[jkowalleck]

I am trying to go through the changes made, but I'm not understanding where the code is. I see a bunch of code was moved to cyclonedx/serialization/__init__.py in this commit: 376dfa8 but when I pull down my branch: https://github.com/jkugler/cyclonedx-python-lib/blob/561_add_components_and_services/cyclonedx/serialization/__init__.py I don't see that code. Was that code not pushed to my branch?

Sorry for the confusionl.

i am very sorry for the inconvenience.
376dfa8 resp 427add4 was a version where i tried to move ToolsRepositoryHelper the helper to place where other existing helpers were. this did not work properly, as it caused cyclic includes.
So it was reverted via 4a2ac65

maybe this helps to see the actual changes: git diff 73007f84fc043924f65560e143ba5adbdab56be2...c937f215e3ac4b56c33cc5da2b0444ee0a22807c
see the diff on github: jkugler/cyclonedx-python-lib@73007f8...c937f21

[jkugler]

I noticed the test coverage for cyclonedx/model/tool.py went from 100% to 97% after the recent changes. Would you like me to make sure the missed statements are checked?

[jkowalleck]

I noticed the test coverage for cyclonedx/model/tool.py went from 100% to 97% after the recent changes. Would you like me to make sure the missed statements are checked?

did that, cov at 100% now. please review

[jkugler]

did that, cov at 100% now. please review

Looks great! Let's get it merged! Thanks again for all your help and patience on this.