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

Convert spdx structs to versioned pkgs #146

Merged
merged 2 commits into from
Aug 3, 2022
Merged

Conversation

lumjjb
Copy link
Collaborator

@lumjjb lumjjb commented Jun 6, 2022

This PR converts the structs into their own versioned packages. This organizes them so that they can be used in a more golang idiomatic way. Future work will include creating a common struct representation on the backend that will allow ease of interoperability between versions.

a := v2_1.Document{}
b := v2_2.Document{}

A diff can be done fairly easily (and can be used as a mechanism for creating new versions).

diff -u -I '^\s*//' -I '^package' -r v2_1/document.go v2_2/document.go
...<TRUNCATED>...
@@ -40,14 +45,14 @@
 	// Cardinality: optional, one; default value is "true" if omitted
 	FilesAnalyzed bool `json:"filesAnalyzed,omitempty"`
 	// NOT PART OF SPEC: did FilesAnalyzed tag appear?
-	IsFilesAnalyzedTagPresent bool `json:"-"`
+	IsFilesAnalyzedTagPresent bool
 
 	// 3.9: Package Verification Code
 	PackageVerificationCode common.PackageVerificationCode `json:"packageVerificationCode"`
 
 	// 3.10: Package Checksum: may have keys for SHA1, SHA256 and/or MD5
 	// Cardinality: optional, one or many
-	PackageChecksums []common.Checksum `json:"checksums,omitempty"`
+	PackageChecksums []common.Checksum `json:"checksums"`
 
 	// 3.11: Package Home Page
 	// Cardinality: optional, one
@@ -94,10 +99,18 @@
 	// Cardinality: optional, one or many
 	PackageExternalReferences []*PackageExternalReference `json:"externalRefs,omitempty"`
 
+	// 3.22: Package External Reference Comment
+	// Cardinality: conditional (optional, one) for each External Reference
+	// contained within PackageExternalReference2_1 struct, if present
+
+	// 3.23: Package Attribution Text
+	// Cardinality: optional, one or many
+	PackageAttributionTexts []string `json:"attributionTexts,omitempty"`
+
 	// Files contained in this Package
 	Files []*File
 
-	Annotations []Annotation `json:"annotations,omitempty"`
+	Annotations []Annotation `json:"annotations"`
 }
 
 // PackageExternalReference is an External Reference to additional info
diff -u -I '^\s*//' -I '^package' -r v2_1/snippet.go v2_2/snippet.go
--- v2_1/snippet.go	2022-06-06 10:22:50.000000000 -0400
+++ v2_2/snippet.go	2022-06-06 10:30:39.000000000 -0400
@@ -41,4 +41,8 @@
 	// 5.10: Snippet Name
 	// Cardinality: optional, one
 	SnippetName string `json:"name,omitempty"`
+
+	// 5.11: Snippet Attribution Text
+	// Cardinality: optional, one or many
+	SnippetAttributionTexts []string `json:"-"`
 }

Signed-off-by: Brandon Lum lumjjb@gmail.com

Signed-off-by: Brandon Lum <lumjjb@gmail.com>
@lumjjb lumjjb marked this pull request as draft June 6, 2022 15:22
@lumjjb
Copy link
Collaborator Author

lumjjb commented Jun 6, 2022

@swinslow @ianling @RishabhBhatnagar wanted to get some feedback on this before i go through the rest and make changes to all the callers.

@RishabhBhatnagar
Copy link
Collaborator

Thanks for the PR and a quick refactor to show us your views on how we can modularise versions of the spdx model.

I have one concern about this approach. Any changes made in v2_1's file will have to be replicated across v2_2's files as well. Even a structural change will affect both the directories.

In general, representing information related to code wrt file-location is not a good idea as extracting and using that data will depend purely on import location and resolution.

@lumjjb
Copy link
Collaborator Author

lumjjb commented Jun 6, 2022

Thanks for taking a look @RishabhBhatnagar !

I have one concern about this approach. Any changes made in v2_1's file will have to be replicated across v2_2's files as well. Even a structural change will affect both the directories.

What are scenarios that you're thinking about with changes?

Ah yes, when I was doing this - i was thinking about it in a way of how containerd handles different images and versions. They have an intermediate representation that all versions can be converted to. All functions and helpers would operate on this intermediate representation. That data representation can also be exported to either 2.1/2.2/2.3 structs. This would mean that the v2_1 and v2_2 folders would largely remain static as just struct definitions for (de)serialization. So for each, we would just need to write 1 function for each major version instead of having 2 copies... however, the same thing may be do-able now with generics. It still a fairly recent change in go, so we probably could wait a bit as well.

Let me know if i'm misunderstanding!

@lumjjb
Copy link
Collaborator Author

lumjjb commented Jun 7, 2022

Looked into this a bit with generics, it currently still doesn't support common field usage - and still unsure if it will be supported in the future.. The release notes currently state:

The Go compiler does not support accessing a struct field x.f where x is of type parameter type even if all types in the type parameter's type set have a field f. We may remove this restriction in a future release.

@lumjjb lumjjb marked this pull request as ready for review July 20, 2022 02:28
@lumjjb
Copy link
Collaborator Author

lumjjb commented Jul 20, 2022

Part 1 of #152, ready for testing and review!

Signed-off-by: Brandon Lum <lumjjb@gmail.com>
@swinslow
Copy link
Member

Hi @lumjjb, thanks for all the work here! I've run the tests and it looks like things are passing.

Given the scope of this I'd like to go through it just a bit more before merging, just to make sure I understand what is changing. I'll try to do that in the next couple of days. If I don't get to it and if others want to add a +1 here, then I'll go ahead and merge soon thereafter.

@swinslow
Copy link
Member

Hi @lumjjb, I've had a chance now to go through the PR. This is excellent! Thanks for all the hard work here.

I'm on board with the changes you've proposed. Because of the extent of the impact on users, I'm going to leave this open for a few more days for folks to weigh in on. If there are no objections before next Wednesday (Aug. 3), I'll go ahead and merge this as-is. Thanks again!

@swinslow
Copy link
Member

swinslow commented Aug 3, 2022

Seeing no complaints, I'm merging this. :) Thanks again @lumjjb!

@swinslow swinslow merged commit 0b2f713 into spdx:main Aug 3, 2022
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