spdx-spec icon indicating copy to clipboard operation
spdx-spec copied to clipboard

Annotations object doesn't have SPDXID property in json spec2.2

Open specter25 opened this issue 4 years ago • 11 comments

JSON example file in the repository Screenshot from 2021-06-12 21-52-52

JSON spdx schema Screenshot from 2021-06-12 21-46-33

The JSON specification doesn't have the spdx identifier property for annotations object , although it is defined in the spdx specification 2.2 for other formats like tag value and RDF as mentioned here

Issue discoved during weekly sync of GSoC'21 project ( JSON support for tools-golang repository ) wih @swinslow .

specter25 avatar Jun 12 '21 16:06 specter25

Currently, the annotations are properties of the SPDX element and therefore do not need an SPDX ID since the SPDX ID would be the ID of the enclosing element.

For example:

...
"packages" : [ {
    "SPDXID" : "SPDXRef-Package",
    "annotations" : [ {
      "annotationDate" : "2011-01-29T18:30:22Z",
      "annotationType" : "OTHER",
      "annotator" : "Person: Package Commenter",
      "comment" : "Package level annotation"
    } ],
...

the SPDX ID for the annotations would be SPDXRef-Package.

The Annotations at the top level are annotations for the SPDX Document SPDXRef-Document.

If we moved the Annotations outside the element (e.g. as a separate Annotations array for the entire document similar to how we treat Relationships), we would want to add the SpdxID's into the Annotation.

goneall avatar Jun 12 '21 20:06 goneall

Ah -- so if I follow, it sounds like an "annotations" object can appear nested within any SPDX element -- Document, Package, File or Snippet.

So the example JSON file has it for SPDXRef-DOCUMENT at line 40, and a separate annotation for the Package SPDXRef-Package starting at line 60.

@specter25 Does this resolve your question here? I think this means that for purposes of tracking the SPDX ID for an Annotation that you're parsing, you'll just need to keep track of the SPDX ID for the element that is currently being parsed that encloses the Annotation.

swinslow avatar Jun 14 '21 13:06 swinslow

Ohh I get it , sure . @goneall thanks a lot for the clarification . @swinslow I will make the required changes in the jsonparser asap .

specter25 avatar Jun 14 '21 13:06 specter25

@goneall I guess this raises the question -- does this work if someone wants to provide an Annotation for an element that is defined in a separate SPDX Document? For example, in tag-value I think I could write something like the following pseudo-SPDX data:

ExternalDocumentRef:DocumentRef-OtherDoc SHA1: . . .

. . .

Annotator: Person: Steve Winslow
AnnotationDate: 2021-06-14T18:30:00Z
AnnotationType: REVIEW
SPDXREF: DocumentRef-OtherDoc:SPDXRef-ElementA
AnnotationComment: ElementA had the wrong license data, should have been MIT.

Section 8.4.5 in the 2.2 spec gives an example like this, using a DocumentRef-.

Maybe I'm misunderstanding, but does the JSON format accommodate this, if Annotations are nested within their elements?

swinslow avatar Jun 14 '21 13:06 swinslow

@swinslow

does this work if someone wants to provide an Annotation for an element that is defined in a separate SPDX Document?

Good point - this would not be supported with the current serialization format.

This may be a more general problem for the Java tools and perhaps the Python tools. I believe they make the assumption that the SPDXREF for annotations are always for elements local to the document.

For the Review use cases, I've always made a copy of the entire document creating a new SPDX document, but having a much smaller review document would be useful.

This raises a number of other questions I haven't considered up until now. Can we have a DOCUMENT_DESCRIBES relationship to an element external to the document? This would be useful for the review document use case.

goneall avatar Jun 14 '21 14:06 goneall

@goneall Interesting... yes, in the Golang tools it makes a distinction between two different data types for handling SPDX Identifiers:

  • ElementID, which is just a string; or
  • DocElementID, which is a struct that includes both an optional DocumentRef- portion as well as the ElementID.

Then, for data fields that can only hold SPDX IDs defined within the scope of the current document, it uses ElementID. For example, when defining a Package's own ID, it uses ElementID since that has to be limited to just the current document.

Whereas for references to IDs that could be defined either here or in another Document, it uses DocElementID -- e.g. here for Annotations, or here for Relationships.

(DocElementID also has a third field that can be NONE or NOASSERTION -- this is to handle the use case that started in SPDX 2.2 for Relationships that point to one of those values.)

I haven't dug into either the Java or Python libraries to see how they handle this, but that's the approach I've taken for the Golang tools.

swinslow avatar Jun 14 '21 15:06 swinslow

As raised in issue #536 - the current spec would suggest we change the JSON example to a high level array of annotations similar to how we treat relationships.

Since this change would create incompatibilities, we would need to coordinate the effort with tooling providers.

@swinslow @tsteenbe @SamuraiAku Should we queue this for 3.0 or make the change sooner?

goneall avatar Aug 21 '21 18:08 goneall

@iamwillbar Just pointing out the discussion in this issue on annotating elements not included in the document as it may impact our current model discussions. TL;DR - @swinslow points out the current RDF model does not support SPDX annotating elements not included in the document.

A couple more thoughts - we can separate out 1) annotation addressing external elements from 2) being able to address annotation themselves. for 1) - we can solve this by moving annotations to a collection property of a document similar to how tag/value works today. For 2) - we would probably make Annotation and subclass of Element so that it can be referenced by externally and also by the documentDescribes property. I'm leaning towards supporting 1) but have reservations on 2).

goneall avatar Aug 21 '21 18:08 goneall

I added issue #537 to propose a change to the SPDX 3.0 model to support annotation Elements outside of the Document

goneall avatar Aug 22 '21 17:08 goneall

@swinslow @tsteenbe @SamuraiAku Should we queue this for 3.0 or make the change sooner?

One question -- is this an issue for RDF also, or just for JSON? I'm not familiar enough with RDF to really understand, but looking at section 8.4.6 in the 2.2 spec I guess I'm assuming that it could handle external document references in the rdf:about property.

If this is an issue for RDF, then I'd say it probably does warrant a fix for 2.2, since the RDF definition is explicitly part of the spec. I'd assume it would be as a new point release (2.2.2)?

If it's only an issue for JSON, then I guess the question is whether the JSON serialization definition is itself part of the 2.2 spec. I know it isn't defined inline in the spec text the way that RDF and tag-value are, but I'm not sure whether that means that it's considered "part of" the 2.2 or 2.2.1 specs.

swinslow avatar Aug 24 '21 12:08 swinslow

is this an issue for RDF also, or just for JSON?

I had to think about this question a while - I believe the answer is no - it is not an issue for RDF since all SPDX elements are required use a URI for an ID they can all be addressed outside of the document - you would just use the full element URI in the rdf:about attribute.

If it's only an issue for JSON, then I guess the question is whether the JSON serialization definition is itself part of the 2.2 spec.

I have the same questions. The only documentation we have for JSON is the example file and JSON schema. We intended YAML and JSON to be part of 2.2, however, we could not find any volunteers to add the documentation for the spec itself. Does this mean we are free to change the example file if we think the example is incorrect or does the example constitute the spec?

goneall avatar Aug 24 '21 19:08 goneall

This is resolved in 3.0 - closing

goneall avatar Apr 04 '24 16:04 goneall