Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add tests for multiple licenses. #187

Closed
wants to merge 1 commit into from

Conversation

khuey
Copy link

@khuey khuey commented Nov 18, 2022

Support for multiple licenses is broken (and it's broken differently for XML/JSON format SBOMs). It's not obvious to me how to fix it. The .NET serialization stuff isn't well documented. But this PR demonstrates the problem.

@khuey
Copy link
Author

khuey commented Nov 18, 2022

Actually JSON is fine, I just made a mistake in the structure. Recognizing that it only affects XML leads to the culprit:

has XMLElement instead of XMLArray.

Doing the obvious thing will introduce a layer of <LicenseChoice> elements which is not what we want. I don't know how to use XMLArray and avoid introducing another layer of elements, and while the JSON spec for CycloneDX has an extra object in this spot the XML spec doesn't have any intermediate tags between <licenses> and <license>/<expression>

@andreas-hilti
Copy link
Contributor

@khuey @coderpatros #218 would show one way how you could address this issue (without changing the data model; depends maybe also on CycloneDX/specification#204).
I couldn't come up with a better way to inject the namespace into the WriteXml, let me know if you see a better alternative.

@mtsfoni
Copy link
Contributor

mtsfoni commented May 12, 2024

I will try to find a solution in the 1.6 update for this. I don't know yet if we still need the proposed PR as the specification was changed, so that the json schema now matches the xml. Proto is still in the old way, however. in its own way.

Not sure how to bring it all together and staying downwards compatible to be honest...

@andreas-hilti
Copy link
Contributor

I will try to find a solution in the 1.6 update for this. I don't know yet if we still need the proposed PR as the specification was changed, so that the json schema now matches the xml. Proto is still in the old way, however. in its own way.

Not sure how to bring it all together and staying downwards compatible to be honest...

IMO a way would be: Merge #218 and then in addition put a constraint that only one license expression can be used wherever it is needed (for instance on export to the newer formats).

This is based on the following observations (if I remember it all correctly):

  • The library currently handles xml incorrectly, but json was handled correctly (before the specification change).
  • The PR would ensure that xml is handled correctly.
  • The specification change left xml untouched and only put an additional constraint for the json format that only a single license expression can be used.

@mtsfoni
Copy link
Contributor

mtsfoni commented May 20, 2024

I was curious to see what the core-team says about this.
But anyway, if they make a change it will be for 1.7 and later. So we will have to fix this.

All those misalignments and then the fixes for those cause much complexity... we will merge and change this for the next version.

@mtsfoni
Copy link
Contributor

mtsfoni commented May 28, 2024

problem should be solved now

@mtsfoni mtsfoni closed this May 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants