tools-golang icon indicating copy to clipboard operation
tools-golang copied to clipboard

[Optional Upgrade] SpdxItem and SpdxElement

Open RishabhBhatnagar opened this issue 4 years ago • 4 comments

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 avatar Nov 13 '20 13:11 RishabhBhatnagar

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.

swinslow avatar Nov 14 '20 17:11 swinslow

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).

RishabhBhatnagar avatar Nov 14 '20 18:11 RishabhBhatnagar

@RishabhBhatnagar regarding your thoughts on SpdxItem to make it more closer to spdx-spec model, I like them :+1: . 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 :)

bisakhmondal avatar Mar 07 '21 11:03 bisakhmondal

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

swinslow avatar Mar 14 '22 19:03 swinslow