Festo-se / cyclonedx-editor-validator

Tool for creating, modifying and validating CycloneDX SBOMs.
https://festo-se.github.io/cyclonedx-editor-validator/
GNU General Public License v3.0
18 stars 4 forks source link

Use model from the "official" python library #7

Open CBeck-96 opened 1 year ago

CBeck-96 commented 1 year ago

Use the model from cyclonedx-python-lib. This helps working on classes instead of dicts, which would make our code more robust.

italvi commented 1 week ago

@mmarseu after internal discussion, we are starting to get doubts, if this transition is really worth it:

  1. The model does not support every field, e.g. support for composition is an PR, which is open since April.
  2. We add an additional dependency, we will rely on them accepting our required changes.
    1. This additional dependency even would increase the work for our colleagues, as e.g. SBOMs for Conan require a cyclonedx-python-lib<6, though the current one is 7.6.0
  3. Looking at the issues and open PRs in the project, it seems there are still some problems in the deserialization of XML/JSON.
  4. The only "true" benefit we see: Support of XML.

What is your opinion here? Your pro back then was "It's simply more robust to work with real classes than only dicts with string keys". Maybe it's more worth investigating to create own classes and functions, which you already started in sbomFunctions, where I honestly like our compare_components more than in the python-lib. There they use hash(component), so if e.g. a component has an external reference and the other not, it won't be recognized as same component, even though this shouldn't have any impact on the identity/coordinates of a component.

mmarseu commented 1 week ago
  1. The model does not support every field, e.g. support for composition is an PR, which is open since April.

True, the library is horrendously outdated. There is clearly a severe lack of developers working on this. That's no reason to skip it but a reason to get involved. Where has your open-source spirit gone? 😆

  1. We add an additional dependency, we will rely on them accepting our required changes.

Also true. I suspect that won't be a problem, though. I have the feeling they are happy to accept help. They are only too busy to do the work themselves. If ever there is a specific feature we'd like to introduce which they really don't want for some reason, we can always fork and maintain our customized stuff in our fork and the general things upstream.

i. This additional dependency even would increase the work for our colleagues, as e.g. SBOMs for Conan require a cyclonedx-python-lib<6, though the current one is 7.6.0

I don't understand. What's the relevance of the conan extension here? It can use whatever version of the library it likes, can't it?

  1. Looking at the issues and open PRs in the project, it seems there are still some problems in the deserialization of XML/JSON.

AFAIK those issues are simply that they can't deserialize upstream test data because they don't support the newer CDX versions, yet which goes back to point 1 above.

  1. The only "true" benefit we see: Support of XML.

What is your opinion here? Your pro back then was "It's simply more robust to work with real classes than only dicts with string keys".

I maintain my position on this question. What's better than writing code full of bugs? Writing tests to catch those bugs, obviously! But what's better than writing tests to catch bugs? Preventing the appearance of bugs in the first place by having a domain-specific class API which simply doesn't allow you to mess up.

Maybe it's more worth investigating to create own classes and functions, which you already started in sbomFunctions, where I honestly like our compare_components more than in the python-lib. There they use hash(component), so if e.g. a component has an external reference and the other not, it won't be recognized as same component, even though this shouldn't have any impact on the identity/coordinates of a component.

IMO that's simply because there is more than one way to interpret equality of a component and there are perfectly reasonable use cases for their version of equality. That doesn't mean we can't keep our way to compare components. It would simply have to be a separate function (which it already is). Recreating our own class hierarchy seems extremely inefficient. We'd have to redo all the work they already did and that only for the benefit of less coordination when adding features.

I see only two options:

  1. Use (and improve) the official library.
  2. Keep doing what we're doing.

I'm also convinced that the second option will make it harder and harder to maintain our code and create more technical debt with every feature, because our implementation tends to be closely tied to the representation of the model (a bunch of strings in a JSON file) instead of its content - if that makes any sense 😅

Well, that's my two cents, anyways...

italvi commented 10 hours ago

@mmarseu After internal discussion we agree with you and could eradicate the doubts of some colleagues, e.g. regarding not being able to use the conan-extension anymore. However, changing to the model should not be a breaking change to our tool but more refactoring. Hence, we decided to have a stable set of features for the 1.0.0 release and do the transition to the "official" model step by step. This way we are not depending on also improving/adjusting the model, which will also take some time. Do you agree on that procedure?

mmarseu commented 9 hours ago

Sure, we can try an incremental approach 👍 I agree that the change should not be breaking. Although it very likely will be 😆 With any new implementation, we're going to see an entirely new set of bugs, I imagine. But we'll see how it goes and we can always bump the version to v2 once the work is complete and features like XML support are automagically available.

I guess we need to have a discussion about the validate command in this context, at some point. It is very closely tied to the JSON representation. For the time being, the most efficient solution would be to leave it as is, I guess.