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

Proposed extension classes to support CDX properties #672

Merged
merged 5 commits into from
Apr 4, 2024
Merged

Conversation

goneall
Copy link
Member

@goneall goneall commented Mar 21, 2024

Following up on a discussion with @puerco I created this pull request to help in tools which translate between SPDX and CycloneDX.

There was a request to better support the CycloneDX properties property which is a keyword/keyvalue pair.

Reference the CycloneDX spec properties section.

A few interesting design choices that deserve review and comment:

  • I added the classes and properties in the Extension namespace - which I think is appropriate for this feature.
  • I could not reuse the DictionaryEntry class since the value type is xsd:string and CDX allows for any object, so I created a similar class.
  • I'm not sure how to express the range to be any object - I chose owl:Thing which is appropriate for RDF object properties, but I don't know if it includes data / literals in the range. I'm also not sure if the spec parser supports this type.

@goneall goneall added the enhancement New feature or request label Mar 21, 2024
@goneall goneall requested review from puerco and zvr March 21, 2024 18:53
@zvr
Copy link
Member

zvr commented Mar 22, 2024

I haven't thought about it for very long, but my approach would be to define an extension explicitly named CdxProperties and use our already existing Core/DictionaryEntry [1..*].

In the CDX properties definition (same link as above), both name and value are strings, so values are not objects. I understand that we could get more if we use objects, but I fear translations/conversions would become more difficult.

@goneall
Copy link
Member Author

goneall commented Mar 22, 2024

In the CDX properties definition (same link as above), both name and value are strings, so values are not objects. I understand that we could get more if we use objects, but I fear translations/conversions would become more difficult.

Agree with both points - I'll update the PR. For some reason, I thought CDX used Object rather than string. Thankfully, I was wrong. We can now use the same dicationaryEntry and simplify the spec.

@goneall
Copy link
Member Author

goneall commented Mar 23, 2024

@zvr - I updated the PR consistent with your feedback plus a couple other errors I spotted. On the naming, I renamed both the class and property to be cdx specific - not sure if that is what you had in mind.

Please take another look and let me know what you think.

@puerco Quite interested to see if you think this will help in the tooling that you support.

Copy link
Member

@zvr zvr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is very much like what I was thinking.

@goneall
Copy link
Member Author

goneall commented Mar 24, 2024

@zvr - One more check and we should be good to go

@stevespringett
Copy link

@goneall I don't think DictionaryEntry will work as the model states:

To implement a dictionary, this class is to be used in a collection with unique keys.

CycloneDX property names are not keys and are not required to be unique.

@goneall
Copy link
Member Author

goneall commented Mar 24, 2024

@goneall I don't think DictionaryEntry will work as the model states:

To implement a dictionary, this class is to be used in a collection with unique keys.

CycloneDX property names are not keys and are not required to be unique.

Thanks @stevespringett for pointing this out. Perhaps we should go back to creating a separate class for the CDX property entry as in the original commit but add a separate property for name instead of key and define it to be more compatible with CDX property extensions and change the type of value to be xsd:string.

@zvr thoughts?

@kestewart
Copy link
Contributor

@puerco - will you please review and weigh in.

From call, move discussion to Thursday serialization call.

This isn't a blocking issue for 3.0 release, it's nice to have.

Resolves feedback from Steve that the CDX properties are not
key/value pairs but rather a mapping that allows duplication
of property names with different values

Signed-off-by: Gary O'Neall <[email protected]>
@goneall
Copy link
Member Author

goneall commented Apr 3, 2024

@stevespringett @zvr @puerco - I just updated the PR to have consistent definitions with CycloneDX.

Note: I chose property in class names that are specific to CDX - one could argue we should make them more generic and part of core, but I thought if we wanted to go in that direction after the 3.0 release, we could make the CDX specific properties sub-properties in the RDF model.

Steve - let me know if the documentation is sufficient or if you have any other suggestions.

Adolfo - let me know if this eases the migration utilities

Alexios - let me know if you agree with the approach

@kestewart kestewart added this to the 3.0 milestone Apr 3, 2024
@stevespringett
Copy link

@goneall I think the docs look good.

@goneall goneall requested a review from kestewart April 4, 2024 08:55
Copy link
Contributor

@kestewart kestewart left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, and haven't seen feedback to the contrary.

@kestewart kestewart merged commit dcc78f8 into main Apr 4, 2024
1 check passed
rgopikrishnan91 added a commit to rgopikrishnan91/spdx-3-model that referenced this pull request Apr 4, 2024
Proposed extension classes to support CDX properties (spdx#672)
@goneall goneall deleted the propextensions branch April 30, 2024 19:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants