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

[Optional Upgrade] SpdxItem and SpdxElement #48

Open
RishabhBhatnagar opened this issue Nov 13, 2020 · 4 comments
Open

[Optional Upgrade] SpdxItem and SpdxElement #48

RishabhBhatnagar opened this issue Nov 13, 2020 · 4 comments
Labels
enhancement New feature or request
Milestone

Comments

@RishabhBhatnagar
Copy link
Collaborator

RishabhBhatnagar commented Nov 13, 2020

As evident from the UML diagram, classes like Package, File, Snippets, etc derive some of their fields/properties from an abstract class called SpdxItem.

Currently, each of the subclasses defines each of those properties in their own structs. I suggest pulling out the common fields and create a new struct type "SpdxItem" which will hold those common attributes. The subclasses can then use composition to include those fields in their own definition.
This will allow the model to have less entry points for common fields allowing

Similar is the argument for SpdxElement

Sample Working:

type SpdxItem struct {
    licenseConcluded AnyLicenseInfo
    licenseInfoFromFiles SimpleLicenseInfo
    copyrightText String
    licenseComments String
}

type Package struct {
    SpdxItem
    summary String
    description String
    // ...other fields...
}

type File struct {
    SpdxItem
    fileType FileType
    checksum Checksum
    // ...other fields...
}
@RishabhBhatnagar RishabhBhatnagar changed the title [Optional Upgrade] SPDX Item [Optional Upgrade] SpdxItem and SpdxElement Nov 13, 2020
@swinslow
Copy link
Member

This is an interesting thought, @RishabhBhatnagar. For the future SPDX 3.0 spec, when we implement that in the Golang tools, I definitely agree that we should more closely follow the model.

For the implementation of the current versions 2.1 and 2.2, I am less sure but could be convinced. Would this make it easier to interchange with the RDF parser / writer that you've been working on? If so, I'd definitely be interested in making this change.

With the change you're proposing here, would any of the existing tag-value parsing code need to change? If I understand correctly, I'm assuming not -- the fields from SpdxItem would just get populated into Package, File and Snippet with the same names, so in theory no other code changes should be needed. Is that correct?

One thing to note is that I'm not sure that model diagram is entirely correct, at least about the "licenseInfoFromFiles" field. There is a field similar to this in Snippets, for example, but it does not have this name. So I don't think that would be one to pull up into SpdxItem.

@RishabhBhatnagar
Copy link
Collaborator Author

For the implementation of the current versions 2.1 and 2.2, I am less sure but could be convinced. Would this make it easier to interchange with the RDF parser / writer that you've been working on? If so, I'd definitely be interested in making this change.

It doesn't really affect any of the parser or writer (be it rdf or tag-value).

With the change you're proposing here, would any of the existing tag-value parsing code need to change? If I understand correctly, I'm assuming not -- the fields from SpdxItem would just get populated into Package, File and Snippet with the same names, so in theory no other code changes should be needed. Is that correct?

Yes. You are 100% correct. We won't be required to change any code.

One thing to note is that I'm not sure that model diagram is entirely correct, at least about the "licenseInfoFromFiles" field. There is a field similar to this in Snippets, for example, but it does not have this name. So I don't think that would be one to pull up into SpdxItem.

Correct me if I am wrong. licenseInfoInSnippet is the field which is similar to licenseInfoFromFiles that you're talking about.
I've been referring to the rdf spec document all the time. Even in that document, we can see Snippets deriving properties from SpdxItem class. It is quite possible that Snippet might not use the licenseInfoFromFiles property and use licenseInfoInSnippet as it's alternative.

My thoughts:

The following are the ways to resolve the issue ambiguity with Snippet.

  1. not to include the licenseInfoInFiles field in the SpdxItem and let Snippet use licenseInfoInSnippet as it's alternative.
  2. include the licenseInfoInFiles field in the SpdxItem and let Snippet declare another property for licenseInfoInSnippet. (this will effectively leave licenseInfoInFiles as a redundant field in the Snippet class).

first way will reduce redundancy and second way will let the tools-golang model be close to the actual data model defined by the uml and the rdf spec.

I'm okay with either of the approaches. I'm inclined more to second way as it will stick to the documentation.
@swinslow, let me know which approach are you more comfortable with (first, second or scrap the idea).

@bisakhmondal
Copy link
Contributor

@RishabhBhatnagar regarding your thoughts on SpdxItem to make it more closer to spdx-spec model, I like them 👍 . If we need this, extending the thoughts you mentioned, can we go for an extra layer of abstraction on top of SpdxItem (let's assume it's the grandparent here), though it's slightly deviating from the UML diagram?

Instead of directly deriving from SpdxItem, File & Snippet structs derive their props (embedding) from SpdxItemFile & SpdxItemSnippet (two direct child struct of SpdxItem with customised fields specific to File & Snippet).

Would you mind if I work on this issue for SpdxItem as well as SpdxElement? Thank you :)

@swinslow swinslow added this to the 0.4.0 milestone Mar 14, 2022
@swinslow
Copy link
Member

Marking this as part of 0.4.0, so we can consider it together with some other potential API changes for that release.

@swinslow swinslow added the enhancement New feature or request label Mar 14, 2022
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

No branches or pull requests

3 participants